Allow delegated IPAM to specify uplink interface #34779

ruicao93 · 2024-09-09T13:50:26Z

For the scenario where there are multiple interfaces on a Node and delegated IPAM is used to determine the IP and the uplink interface of a Pod. The "interfaces" field can be used by IPAM plugin to pass uplink interface info to Cilium. And then Cilium can setup host routes for the Pod according to the uplink interface info.

The "interfaces" field is allowed to specify the related host interface according to the CNI spec and is already widely used by existing plugins, such as bridge and ptp plugin. It's actually not used by any IPAM plugin for now, but I think it's reasonable and conforms to the original definition of the field.

Motivations/Benefits of the PR for the native routing mode:

More cloud(public or private) envs with multiple uplink can be easily ingregrated with Cilium without involving any cloud specific logic. A common way is provided for the case: "uplink interface" support.
Cilium keeps full control of the Pod traffic. Cilium choose the implement of the "routing" for egress traffic, through Linux routes or eBPF forwarding rules or any other methods.

- interfaces: An array of all interfaces created by the attachment, including any host-level interfaces:

bridge.go

	// Assume L2 interface only
	result := &current.Result{
		CNIVersion: current.ImplementedSpecVersion,
		Interfaces: []*current.Interface{
			brInterface,
			hostInterface,
			containerInterface,
		},
	}

bridge plugin result:

{
    "ips": [
        {
          "address": "10.1.0.5/16",
          "gateway": "10.1.0.1",
          "interface": 2
        }
    ],
    "routes": [
      {
        "dst": "0.0.0.0/0"
      }
    ],
    "interfaces": [
        {
            "name": "cni0",
            "mac": "00:11:22:33:44:55"
        },
        {
            "name": "veth3243",
            "mac": "55:44:33:22:11:11"
        },
        {
            "name": "eth0",
            "mac": "99:88:77:66:55:44",
            "sandbox": "/var/run/netns/blue"
        }
    ],
    "dns": {
      "nameservers": [ "10.1.0.1" ]
    }
}

Fixes: #34604

maintainer-s-little-helper · 2024-09-09T13:50:30Z

Commit 2916ed4 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

joamaki · 2024-09-10T06:34:45Z

As the title still has Draft in it, I'll wait before reviewing. Edit the title and ping me when you think it's ready.

ruicao93 · 2024-09-10T18:26:45Z

As the title still has Draft in it, I'll wait before reviewing. Edit the title and ping me when you think it's ready.

Thanks @joamaki, the commit is ready for review and more discussions could be found in #34604

ruicao93 · 2024-09-11T19:40:55Z

Hi @joestringer @gandro , could you please help review the PR as we discussed.
cc @cilium/sig-ipam

joestringer · 2024-09-11T19:46:50Z

I'm not so familiar with delegated IPAM, but one crucial part of this will be to test that this doesn't break existing delegated IPAM setups. I see that issue #34788 is filed to create automated testing for that.

dylandreimerink · 2024-09-12T09:19:01Z

/test

joestringer · 2024-09-12T23:16:23Z

I see that there is now #34839 out for review, so it should be possible to combine that test with this PR and test it out on your own repository?

joestringer · 2024-09-12T23:16:54Z

@dylandreimerink I don't think it's meaningful to run /test on this PR, as the delegated IPAM path isn't enabled for any of the existing tests merged into the tree.

ruicao93 · 2024-09-13T03:05:59Z

I see that there is now #34839 out for review, so it should be possible to combine that test with this PR and test it out on your own repository?

Sure, l will run the test provided in #34839 . The Cilium e2e and k8s conformance tests have been verified on Volcengine cloud combined with Volcengine delegated IPAM:

CIlium e2e tests:

With one minor error, but it doesn't matters.

Kubernetes conformance network tests
Plugin: e2e
Status: passed
Total: 6965
Passed: 42
Failed: 0
Skipped: 6923

gandro

Thanks! I have a few questions on this approach that are still unclear to me

plugins/cilium-cni/cmd/cmd.go

ruicao93 · 2024-09-18T12:57:24Z

hi @tamilmani1989. We're discussing whether it is better to install node routes for Pod in Cilium or in delegated IPAM plugin.

I have a few of questions about the Delegated Azure IPAM mode. Could you please help me with them?

Does the azure ipam will create multiple network interfaces for Pods like how aws ENI mode works?
If there're multile ENIs, is the azure IPAM compoment responsible for install egress routes for Pods to redirect traffic to right network interface?

Do you think it would be helpful if delegated IPAM plugin only handle IP/interface management and Cilium controls all Pod routes(implemented by linux routes or bpf forwarding rules)?

wedaly · 2024-09-20T16:30:20Z

hi @tamilmani1989. We're discussing whether it is better to install node routes for Pod in Cilium or in delegated IPAM plugin.

I have a few of questions about the Delegated Azure IPAM mode. Could you please help me with them?

Does the azure ipam will create multiple network interfaces for Pods like how aws ENI mode works?

If there're multile ENIs, is the azure IPAM compoment responsible for install egress routes for Pods to redirect traffic to right network interface?

Do you think it would be helpful if delegated IPAM plugin only handle IP/interface management and Cilium controls all Pod routes(implemented by linux routes or bpf forwarding rules)?

azure-ipam currently handles only IP address management, not installing routes or multiple network interfaces.

I mentioned this in the parent issue, but my understanding is that a conforming delegated IPAM plugin is not allowed to return interfaces. (The bridge and ptp plugins are CNI plugins, but are not delegated IPAM plugins.) We want azure-ipam to conform to the CNI spec, so I think even if we had this use case we'd look for a different solution or work with the CNI maintainers to extend the spec so the semantics of interfaces from a delegated IPAM plugin are well-defined.

ruicao93 · 2024-09-23T09:15:35Z

azure-ipam currently handles only IP address management, not installing routes or multiple network interfaces.

I mentioned this in the parent issue, but my understanding is that a conforming delegated IPAM plugin is not allowed to return interfaces. (The bridge and ptp plugins are CNI plugins, but are not delegated IPAM plugins.) We want azure-ipam to conform to the CNI spec, so I think even if we had this use case we'd look for a different solution or work with the CNI maintainers to extend the spec so the semantics of interfaces from a delegated IPAM plugin are well-defined.

Thanks @wedaly for your reply. Dose the single uplink(eth0) is enough for AKS? The IPs and bandwidth of single ENI may be limited by the
control plane(quota) or dataplane(performance).

squeed · 2024-10-15T14:26:00Z

Adding @dylandreimerink, who added the "uplink" or "parent" mechanism for ENI.

squeed · 2024-10-15T14:27:12Z

CNI spec author here :-). There's nothing that says the IPAM result is not allowed to contain interfaces; it's not done out of convention. However, if we would like to change the interpretation of the types, I would be amenable to revisiting that in the spec.

The best part is that it wouldn't require any code changes, so we could "ship" it entirely in text.

ruicao93 · 2024-10-15T15:41:29Z

CNI spec author here :-). There's nothing that says the IPAM result is not allowed to contain interfaces; it's not done out of convention. However, if we would like to change the interpretation of the types, I would be amenable to revisiting that in the spec.

The best part is that it wouldn't require any code changes, so we could "ship" it entirely in text.

Thanks @squeed for the clarification, it's extremely helpful for the current discussion. We also created another PR to update the deletation IPAM result part, could you help review containernetworking/cni#1121

So it's reasonable to handle the returned uplink hints. Install the routes according to the uplink info could help Cilium have full control of the Pod traffic in delegate IPAM mode. Based on the current code modifications, we might also consider adding an option for users to decide whether the routing is controlled by Cilium or controlled by the user.

tamilmani1989 · 2024-10-28T19:52:12Z

Late to the party. We have a similar requirement where pod egress traffic to leave via secondary interface instead of primary interface. i noticed eni ipam mode already does this. https://docs.cilium.io/en/latest/network/concepts/routing/#egress. @ruicao93 Does this PR follow the same pattern/code?

ruicao93 · 2024-10-29T03:07:16Z

Late to the party. We have a similar requirement where pod egress traffic to leave via secondary interface instead of primary interface. i noticed eni ipam mode already does this. https://docs.cilium.io/en/latest/network/concepts/routing/#egress. @ruicao93 Does this PR follow the same pattern/code?

Yes @tamilmani1989 , we reused the logic(codes) with ENI IPAM mode. For the long term, ENI IPAM mode could consider to be migrated to Delegation IPAM mode and can be developed and evolved independently of Cilium.

ruicao93 · 2024-11-28T16:16:52Z

Yes, I can gladly do that. But before, could also squash your two commits into one commit? Otherwise the CI results will be invalidated once the commits are changed again.

@gandro Okay, done. I thought just need to squash on merge.

gandro · 2024-11-28T16:23:47Z

/test

ruicao93 · 2024-11-29T03:16:28Z

/test

tklauser · 2024-11-29T09:14:29Z

/test

ruicao93 · 2024-12-02T06:31:09Z

Rebsed the commit. @gandro @tklauser , could you help run the ci tests?

gandro · 2024-12-02T09:38:44Z

/test

Support uplink interface info from IPAM result and install related ingress/egress routes. Signed-off-by: Rui Cao <caorui.io@bytedance.com>

gandro · 2024-12-02T12:53:22Z

@ruicao93 There is no need to rebase if CI fails. We can restart individual tests separately (assuming those are flakes). In fact. You should only rebase if a) there are conflicts or b) there are persistent CI failures because the tests have been updated on main.

gandro · 2024-12-02T12:53:37Z

/test

ruicao93 · 2024-12-02T12:57:49Z

@ruicao93 There is no need to rebase if CI fails. We can restart individual tests separately (assuming those are flakes). In fact. You should only rebase if a) there are conflicts or b) there are persistent CI failures because the tests have been updated on main.

@gandro Thanks~ Gotcha~

ruicao93 · 2024-12-03T08:36:05Z

@ruicao93 There is no need to rebase if CI fails. We can restart individual tests separately (assuming those are flakes). In fact. You should only rebase if a) there are conflicts or b) there are persistent CI failures because the tests have been updated on main.

@gandro Thanks~ Gotcha~

@gandro Finally... all CI cases has passed. Dose the PR could be merged?

gandro · 2024-12-03T09:34:45Z

There is still a review needed from the @cilium/api CODEOWNER before we can merge. The designated reviewer is @learnitall

learnitall

API changes LGTM! Thank you for the ping, I apologize for the delay.

joestringer · 2024-12-03T20:31:43Z

@squeed @gandro I take it from the above discussion that with the spec loosening in containernetworking/cni#1137 we are happy with this solution? Just want to check given that this is now approved by all codeowners and the MLH bot has marked the PR as ready-to-merge.

gandro · 2024-12-04T09:40:25Z

@squeed @gandro I take it from the above discussion that with the spec loosening in containernetworking/cni#1137 we are happy with this solution? Just want to check given that this is now approved by all codeowners and the MLH bot has marked the PR as ready-to-merge.

Yes, from my side I'm happy with the proposed solution. The functionality in this PR is opt-in, so I the risk of Cilium accidentally misinterpreting a delegated plugin's result is very low.

rjdenney · 2024-12-06T22:36:10Z

@ruicao93 Do you have a timeline for the additional V6 changes that would be needed, or could you tell us what would need to be added so that we can try working on it from our side?

ruicao93 · 2024-12-09T12:16:11Z

@ruicao93 Do you have a timeline for the additional V6 changes that would be needed, or could you tell us what would need to be added so that we can try working on it from our side?

@rjdenney Let me create a PR for v6 this week. We need to modify it carefully to avoid disrupting the ENI feature. A draft PR will be available soon for early discussion.

ruicao93 requested a review from a team as a code owner September 9, 2024 13:50

ruicao93 requested a review from joamaki September 9, 2024 13:50

maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Sep 9, 2024

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Sep 9, 2024

ruicao93 mentioned this pull request Sep 9, 2024

CFP: add Volcengine cloud ENI IPAM support #34604

Closed

4 tasks

ruicao93 force-pushed the ipam-uplink branch from 2916ed4 to 1bc3a26 Compare September 9, 2024 14:00

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Sep 9, 2024

ruicao93 changed the title ~~[Draft]Allow delegated IPAM to specify uplink interface~~ Allow delegated IPAM to specify uplink interface Sep 10, 2024

joestringer added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Sep 11, 2024

gandro reviewed Sep 16, 2024

View reviewed changes

plugins/cilium-cni/cmd/cmd.go Outdated Show resolved Hide resolved

ruicao93 mentioned this pull request Oct 29, 2024

spec: allow interfaces to be included in the delegated IPAM plugin result containernetworking/cni#1121

Open

ruicao93 force-pushed the ipam-uplink branch from fca87d4 to 2f16485 Compare November 28, 2024 16:14

ruicao93 force-pushed the ipam-uplink branch from 2f16485 to 752988b Compare November 29, 2024 03:13

tklauser approved these changes Nov 29, 2024

View reviewed changes

ruicao93 force-pushed the ipam-uplink branch 2 times, most recently from a0d6e75 to 2fb3e11 Compare December 2, 2024 06:22

Allow delegated IPAM to specify uplink interface

2110ef1

Support uplink interface info from IPAM result and install related ingress/egress routes. Signed-off-by: Rui Cao <caorui.io@bytedance.com>

ruicao93 force-pushed the ipam-uplink branch from 2fb3e11 to 2110ef1 Compare December 2, 2024 12:45

gandro removed the request for review from joamaki December 3, 2024 09:35

learnitall approved these changes Dec 3, 2024

View reviewed changes

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 3, 2024

ldelossa added this pull request to the merge queue Dec 4, 2024

Merged via the queue into cilium:main with commit c5308b5 Dec 4, 2024
64 checks passed

cilium-release-bot bot mentioned this pull request Dec 18, 2024

Prepare for release v1.17.0-rc.0 #36692

Merged

kadevu mentioned this pull request Mar 18, 2025

feat: Support IPv6 for delegated IPAM #38249

Merged

Allow delegated IPAM to specify uplink interface #34779

Allow delegated IPAM to specify uplink interface #34779

Uh oh!

Conversation

ruicao93 commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maintainer-s-little-helper bot commented Sep 9, 2024

Uh oh!

joamaki commented Sep 10, 2024

Uh oh!

ruicao93 commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ruicao93 commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joestringer commented Sep 11, 2024

Uh oh!

dylandreimerink commented Sep 12, 2024

Uh oh!

joestringer commented Sep 12, 2024

Uh oh!

joestringer commented Sep 12, 2024

Uh oh!

ruicao93 commented Sep 13, 2024

Uh oh!

gandro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ruicao93 commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wedaly commented Sep 20, 2024

Uh oh!

ruicao93 commented Sep 23, 2024

Uh oh!

squeed commented Oct 15, 2024

Uh oh!

squeed commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ruicao93 commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tamilmani1989 commented Oct 28, 2024

Uh oh!

ruicao93 commented Oct 29, 2024

Uh oh!

ruicao93 commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gandro commented Nov 28, 2024

Uh oh!

ruicao93 commented Nov 29, 2024

Uh oh!

tklauser commented Nov 29, 2024

Uh oh!

ruicao93 commented Dec 2, 2024

Uh oh!

gandro commented Dec 2, 2024

Uh oh!

gandro commented Dec 2, 2024

Uh oh!

gandro commented Dec 2, 2024

Uh oh!

ruicao93 commented Dec 2, 2024

Uh oh!

ruicao93 commented Dec 3, 2024

Uh oh!

gandro commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

learnitall left a comment

Choose a reason for hiding this comment

Uh oh!

joestringer commented Dec 3, 2024

Uh oh!

gandro commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

ruicao93 commented Sep 9, 2024 •

edited

Loading

ruicao93 commented Sep 10, 2024 •

edited

Loading

ruicao93 commented Sep 11, 2024 •

edited

Loading

ruicao93 commented Sep 18, 2024 •

edited

Loading

squeed commented Oct 15, 2024 •

edited

Loading

ruicao93 commented Oct 15, 2024 •

edited

Loading

ruicao93 commented Nov 28, 2024 •

edited

Loading

gandro commented Dec 3, 2024 •

edited

Loading

gandro commented Dec 4, 2024 •

edited

Loading