Dualstack cilium proposal #1673

rjdenney · 2022-10-24T20:34:53Z

Reason for Change:

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests

Notes:

rbtr

Needs a twice-over to improve formatting and language, needs to introduce the problem and involved components, etc. This is documentation of not just the what (and needs more of that), but the why, so that when we revisit this in the future, we will still be able to understand why we made the decisions we made.

docs/feature/dualstack/Cilium-dualstack.md

ashvindeodhar · 2022-10-25T19:33:23Z

docs/feature/dualstack/Cilium-dualstack.md

+
+## Overview 
+
+With the current CNS solution for dualstack we are setting the nnc to pass multiple NCs with CIDR to be unrolled and added to a pool that contains both v6 and v4 ips. Since we are planning to use Cilium CNI for the linux dualstack solution we need to make changes to azure-ipam to allow for a dualstack cluster to get multiple IPs. Currently azure-Ipam handles the [`CmdAdd`](https://github.com/Azure/azure-container-networking/blob/master/azure-ipam/ipam.go) command from Cilium CNI and sends an IP request to the CNS. The CNS hen in overlay then looks through the pool and picks the first IP found that isn’t used. For dualstack we need to change two things:


nit:
"The CNS hen in overlay"

docs/feature/dualstack/Cilium-dualstack.md

rbtr

this PR should stay scoped to proposal docs. impl should be separate (and should probably wait on this to solidify/get merged at least)

rbtr

I have reviewed up to the last comment and stopped - please go over this critically and make sure that it is polished. It is public-facing documentation and should read as such.

docs/feature/dualstack/Cilium-dualstack.md

thatmattlong · 2023-01-18T19:14:26Z

docs/feature/dualstack/Cilium-dualstack.md

+-   PodIpInfo PodIpInfo 
+   PodIpInfo []PodIpInfo 
+    Response  Response
+}


you'll need a new API for this, so a new type too probably. We need the existing API to continue returning just a PodIPInfo, and then callers that know how to work with the new changes can call a different API to get the slice. Then, if the caller gets a 404 when using the new API, it can fallback to the old API if desired

actually new type is probably not required, the new api can just return the slice as top-level lists are still valid json

When you say that I wouldn't need a new type are you suggesting that I just return a slice of the IPConfigResponse instead of the PodIPInfo within it?

@thatmattlong IMO it would be better to create a new type so that there will be a single 'Response' indicating if the client should consume the response. Can't do that if we return slice of IPConfigResponse

rjdenney · 2023-01-19T21:18:56Z

@thatmattlong and @rbtr, I have updated the doc to reflect more of an API and would appreciate if you guys could give me an comments or a thumbs up if you think it is good. I am also going to split up my changes into multiple PRs so that I can push the CNS changes with the new contract first and then push up the azure-ipam changes after.

rbtr

"approved with suggestions"

rbtr · 2023-01-19T22:52:41Z

docs/feature/dualstack/Cilium-dualstack.md

@@ -0,0 +1,74 @@
+# Cilium Azure-ipam dualstack Overlay Proposal 


nit: this doesn't really have anything to do with Cilium - azure-ipam complies with the CNI spec for delegated IPAM and could be used with any other CNI plugin capable of delegated IPAM. I would drop all specific references to Cilium.

rbtr · 2023-01-19T22:56:17Z

docs/feature/dualstack/Cilium-dualstack.md

+
+### New API
+
+A new API will called `RequestAllIPAddresses` be created for requesting IPs when running in Overlay. This will work similar to the current [`RequestIPAddress`](https://github.com/Azure/azure-container-networking/blob/master/cns/client/client.go) and will accept the same [`IPConfigRequest`](https://github.com/Azure/azure-container-networking/blob/master/cns/NetworkContainerContract.go), the main difference will be that it will return the new contract type `IPConfigsResponse` with the slice of `PodIpInfo`. 


You are inconsistent in naming here, and it would be better if we could pick a scheme and stick to it. I prefer being more concise so if we do IPConfigsResponse, the method should similarly be RequestIPAddresses (or, maybe better, RequestIPs).

I agree that RequestIPs would be a better naming convention and will shift some of the names to be more consistent with IPConfigsResponse

rbtr · 2023-01-19T22:59:51Z

docs/feature/dualstack/Cilium-dualstack.md

+}
+```
+
+If for some reason this function returns a 404 error we will then try again but use the original [`RequestIPAddress`](https://github.com/Azure/azure-container-networking/blob/master/cns/client/client.go) API instead. The swift case will not use the new API and will continue to use the current one.


swift case will not use the new API

Don't treat Swift as a special case, nothing about the delegated-ipam/CNS/NC interactions that happen here are any different in the Swift scenario. The new API should be the only API in use (and the previous one deprecated and removed), once all the pieces have rolled out.

rbtr · 2023-02-01T19:40:15Z

@ashvindeodhar I'm sad that this design proposal PR is still open, but there are already a half dozen implementation PRs in-flight.
cc @thatmattlong @rjdenney

ashvindeodhar · 2023-02-01T20:06:40Z

@ashvindeodhar I'm sad that this design proposal PR is still open, but there are already a half dozen implementation PRs in-flight. cc @thatmattlong @rjdenney

Hey Evan, I didn't realize this is not yet merged. I remember you approving it some time back so I thought this was merged with an ack from Matt too.
@thatmattlong Evan had approved this design and the PR that Ryan is creating adheres to the design. AFAIK you had looked thru the design and didn't have anything major. Do you mind taking a look at the design doc again? Ryan can merge the PR if things look ok to you.

rjdenney · 2023-02-01T20:38:08Z

@ashvindeodhar I'm sad that this design proposal PR is still open, but there are already a half dozen implementation PRs in-flight. cc @thatmattlong @rjdenney

@thatmattlong @rbtr @ashvindeodhar Just made the updates with the suggestions and bit more that I ended up changing in the implementation. Mostly just variable names and mentioned that I also updated GetExistingIPConfig and AssignDesiredIPConfig so that they can also work with multiple IPs. Should be ready to merge and the CNS side of changes I have up at #1773 . I also have a separate PR for the azure-ipam with the CNS changes here #1715 but we will need to push in CNS before azure-ipam goes in

thatmattlong · 2023-02-06T23:50:44Z

docs/feature/dualstack/Cilium-dualstack.md

+// IPConfigsResponse is used in CNS IPAM mode to return a slice of IP configs as a response to CNI ADD
+type IPConfigsResponse struct {
+	PodIpInfo []PodIpInfo
+	Response  Response


I don't know what we use this Response type for, but if it exists in each PodIpInfo, do we also need one for the whole list?

From my understanding I believe the response just is a return code and a message for the IPConfigsResponse and isn't specific to each PodIpInfo. If for some reason we get an error trying to get IPs it is adds a ReturnCode and the error message to Response

@rjdenney you need to consider what happens if we get one IPConfig and then fail the next one. Is the first one marked as "Assigned" already? We would need to roll that back, and make the whole op transactional - entirely successful or entirely failed.

@rbtr yes we had thought about it and have added a todo in the code as a reminder to address it before merging the change. We cannot have the partial success because in that case, we may mark IPs as 'Assigned' w/o the containers using those IPs. We cannot rely on the client to do the right thing of releasing the partially succeeded requests. We may run out of the IPs if we don't handle this ourselves.

tamilmani1989 · 2023-02-07T21:18:15Z

docs/feature/dualstack/Cilium-dualstack.md

+
+```diff
+// RequestIPs calls the RequestIPConfigs in CNS
+func (c *Client) RequestIPs(ctx context.Context, ipconfig cns.IPConfigRequest) (*cns.IPConfigsResponse, error)


will this return just ipv4 in an array when running in overlay singlestack ipv4?

Ryan is making changes in azure-ipam and CNS.
Paul Yu is making changes in azure-cni. So ipv4 overlay will still keep using the old API until new changes made in azure-cni get deployed which is when it will also start using slice of results. It will have single result in the slice.

ashvindeodhar · 2023-02-16T17:51:17Z

@rjdenney can this be merged?

Please enter the commit message for your changes. Lines starting

rjdenney · 2023-02-17T18:13:11Z

@rjdenney can this be merged?

yes

rbtr · 2023-02-17T20:02:12Z

go on then

* Add proposal for changes made to CNS for dualstack overlay

rbtr assigned rjdenney, tamilmani1989, ashvindeodhar, vipul-21 and thatmattlong Oct 24, 2022

rbtr requested review from rbtr, tamilmani1989, ashvindeodhar, vipul-21 and thatmattlong October 24, 2022 21:02

rbtr unassigned tamilmani1989, ashvindeodhar, vipul-21 and thatmattlong Oct 24, 2022

rbtr added enhancement cns Related to CNS. cni Related to CNI. docs Documentation only labels Oct 24, 2022

rbtr reviewed Oct 25, 2022

View reviewed changes

ashvindeodhar reviewed Oct 25, 2022

View reviewed changes

rjdenney requested a review from a team as a code owner October 28, 2022 20:26

rbtr requested changes Oct 31, 2022

View reviewed changes

rjdenney force-pushed the dualstack-cilium branch from 56a0ee6 to 829c40b Compare November 11, 2022 20:11

rbtr requested changes Jan 6, 2023

View reviewed changes

docs/feature/dualstack/Cilium-dualstack.md Outdated Show resolved Hide resolved

docs/feature/dualstack/Cilium-dualstack.md Outdated Show resolved Hide resolved

docs/feature/dualstack/Cilium-dualstack.md Outdated Show resolved Hide resolved

rbtr reviewed Jan 6, 2023

View reviewed changes

docs/feature/dualstack/Cilium-dualstack.md Outdated Show resolved Hide resolved

thatmattlong reviewed Jan 18, 2023

View reviewed changes

rbtr previously approved these changes Jan 19, 2023

View reviewed changes

rjdenney mentioned this pull request Feb 6, 2023

Dualstack CNS Changes #1773

Merged

3 tasks

rbtr force-pushed the dualstack-cilium branch from 7fa6798 to 8c87628 Compare February 6, 2023 22:44

thatmattlong reviewed Feb 6, 2023

View reviewed changes

thatmattlong approved these changes Feb 6, 2023

View reviewed changes

tamilmani1989 reviewed Feb 7, 2023

View reviewed changes

rjdenney added 17 commits February 16, 2023 16:27

Add proposal for Cilium dualstack

c4b3d7d

adding modifications to proposal

f77b820

format changes and adding struts

da9800e

formatting

3463797

highlight

5860b40

adding diff formatting to code block

fab116a

diff

bc819a7

formatting

d3327aa

Please enter the commit message for your changes. Lines starting

added removed line to IPConfigResponse

ae83a16

removing new lines to structs

9d81582

adding background

1273521

adding updates for added flag to NNC to determine IPMode

15f9d5c

Changed IPMode example to a diff to show it's being added

94338ae

changing to reflect a new API to be added in CNS to get multiple IPs

0064601

Updated overview

7726eeb

add link for IPConfigRequest

984665d

updates and suggested changes

69ecb7d

rjdenney force-pushed the dualstack-cilium branch from 8c87628 to 69ecb7d Compare February 16, 2023 21:27

rjdenney merged commit b84be88 into master Feb 17, 2023

rjdenney deleted the dualstack-cilium branch February 17, 2023 20:11

rjdenney added a commit that referenced this pull request Mar 13, 2023

Dualstack cilium proposal (#1673)

5a9e03f

* Add proposal for changes made to CNS for dualstack overlay

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dualstack cilium proposal #1673

Dualstack cilium proposal #1673

rjdenney commented Oct 24, 2022

rbtr left a comment

ashvindeodhar Oct 25, 2022

rbtr left a comment

rbtr left a comment

thatmattlong Jan 18, 2023

thatmattlong Jan 18, 2023

rjdenney Jan 18, 2023

ashvindeodhar Jan 19, 2023

rjdenney commented Jan 19, 2023

rbtr left a comment

rbtr Jan 19, 2023

rbtr Jan 19, 2023

rjdenney Jan 23, 2023

rbtr Jan 19, 2023

rbtr commented Feb 1, 2023

ashvindeodhar commented Feb 1, 2023

rjdenney commented Feb 1, 2023

thatmattlong Feb 6, 2023

rjdenney Feb 7, 2023

rbtr Feb 7, 2023 •

edited

Loading

ashvindeodhar Feb 8, 2023

tamilmani1989 Feb 7, 2023

ashvindeodhar Feb 8, 2023 •

edited

Loading

ashvindeodhar commented Feb 16, 2023

rjdenney commented Feb 17, 2023

rbtr commented Feb 17, 2023


		## Overview

		With the current CNS solution for dualstack we are setting the nnc to pass multiple NCs with CIDR to be unrolled and added to a pool that contains both v6 and v4 ips. Since we are planning to use Cilium CNI for the linux dualstack solution we need to make changes to azure-ipam to allow for a dualstack cluster to get multiple IPs. Currently azure-Ipam handles the [`CmdAdd`](https://github.com/Azure/azure-container-networking/blob/master/azure-ipam/ipam.go) command from Cilium CNI and sends an IP request to the CNS. The CNS hen in overlay then looks through the pool and picks the first IP found that isn’t used. For dualstack we need to change two things:

		@@ -0,0 +1,74 @@
		# Cilium Azure-ipam dualstack Overlay Proposal


		### New API

		A new API will called `RequestAllIPAddresses` be created for requesting IPs when running in Overlay. This will work similar to the current [`RequestIPAddress`](https://github.com/Azure/azure-container-networking/blob/master/cns/client/client.go) and will accept the same [`IPConfigRequest`](https://github.com/Azure/azure-container-networking/blob/master/cns/NetworkContainerContract.go), the main difference will be that it will return the new contract type `IPConfigsResponse` with the slice of `PodIpInfo`.

Dualstack cilium proposal #1673

Dualstack cilium proposal #1673

Conversation

rjdenney commented Oct 24, 2022

rbtr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbtr left a comment

Choose a reason for hiding this comment

rbtr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjdenney commented Jan 19, 2023

rbtr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbtr commented Feb 1, 2023

ashvindeodhar commented Feb 1, 2023

rjdenney commented Feb 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbtr Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashvindeodhar Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

ashvindeodhar commented Feb 16, 2023

rjdenney commented Feb 17, 2023

rbtr commented Feb 17, 2023

rbtr Feb 7, 2023 •

edited

Loading

ashvindeodhar Feb 8, 2023 •

edited

Loading