Support tunnel routing in Multi-Pool IPAM mode by gandro · Pull Request #38015 · cilium/cilium

gandro · 2025-03-05T16:07:22Z

This PR introduces support for tunnel routing with the Multi-Pool IPAM mode. This is achieved by removing the reliance on the tunnel map as a fallback mechanism and instead use Pod CIDR entries in IPCache that act as this fallback. The pod CIDR entries contain the node's tunnel endpoint and encryption key and have the reserved:world identity (i.e. the same identity as the existing catch-all fallback, which would have been hit before this PR).

The bulk of the work in this PR was done by @pippolo84. This PR is a replacement for his PR #37146 with a few notable changes:

The first commit has been reworked to add unit tests and fix up some issues in the previous version (c.f. Use ipcache for tunneling #37146 (comment)).
A commit has been added adding some unit tests to the IPCache package emulating some of the expected corner cases.
Commit node: Remove insertions and deletions to tunnel map has been dropped. In other words, even though the tunnel map is no longer used in the main branch, we still populate it. This addresses concerns with regards to downgrades.
The Conformance Multi-Pool workflow has been extended to also test in tunnel mode.

This change will eventually allow us to remove the tunnel map completely (ref #20170).

gandro · 2025-03-05T16:09:14Z

/ci-multi-pool

gandro · 2025-03-05T16:11:46Z

/ci-multi-pool

gandro · 2025-03-05T16:42:58Z

/ci-multi-pool

gandro · 2025-03-05T16:51:10Z

/ci-multi-pool

gandro · 2025-03-06T08:43:53Z

/ci-multi-pool

gandro · 2025-03-06T09:18:09Z

/ci-multi-pool

gandro · 2025-03-06T09:55:24Z

/test

pippolo84

Thanks for completing (and improving) #37146 🙏

LGTM, just a (very minor) nit left inline ✅

pkg/node/manager/manager.go

christarazi

🚀

julianwiedmann · 2025-03-07T08:25:06Z

👋 Thank you for pushing this forward! Just some (potentially obvious) questions from the peanut gallery:

on initial upgrade, how do we learn about PodCIDRs for existing nodes and insert them into the ipcache? I suppose we replay NodeUpdated() as the agent initially syncs with k8s?
on initial upgrade, how do we handle the race condition where an endpoint's BPF program is re-generated (removing the tunnel-map lookup) before the PodCIDRs are populated into the ipcache? Is that just a dependency that's already enforced inside the agent?
for deleted nodes during agent restart, we rely on the synthesized node deletion feature to clean up the stale PodCIDRs?

ghost

docs good

Store allocation CIDRs from each remote node into the ipcache, using the world identity. This sets the ground for using the ipcache for tunneling instead of relying on the additional tunnel map. Co-authored-by: Sebastian Wicki <[email protected]> Signed-off-by: Sebastian Wicki <[email protected]> Signed-off-by: Fabio Falzoi <[email protected]>

This commit adds a unit test asserting the expected behavior for pod CIDR entries upserted to IPCache by the node manager. In particular, we also test two corner cases: - The first case tests a scenario where the pod CIDR is also selected by a second resource, ensuring that tunnel and encryption key are preserved. - The second case tests a scenario where the pod CIDR is a /32 and the legacy API upserts the pod IP (e.g. based on CEP or kvstore), thereby shadowing the existing pod CIDR entry. Signed-off-by: Sebastian Wicki <[email protected]>

The ipcache map now stores entries describing remote node pod CIDRs, including the tunnel endpoint and encryption key. Therefore, when a packet must be encapsulated or encrypted we can leverage the information from the lookup into the ipcache, avoiding the additional one into the tunnel map. Signed-off-by: Fabio Falzoi <[email protected]>

When handling the deletion of a node, we should take into account all the node allocations CIDRs, not just the primary one. Signed-off-by: Fabio Falzoi <[email protected]>

As a consequence of using the ipcache map to support tunneling, multi-pool IPAM is now compatible with it, thus lift the startup checks that stops the agent when both the features are enabled. Signed-off-by: Fabio Falzoi <[email protected]>

As it is now supported. Signed-off-by: Sebastian Wicki <[email protected]>

gandro · 2025-03-11T09:03:18Z

👋 Thank you for pushing this forward! Just some (potentially obvious) questions from the peanut gallery:

These are great questions!

1. on initial upgrade, how do we learn about PodCIDRs for existing nodes and insert them into the ipcache? I suppose we replay `NodeUpdated()` as the agent initially syncs with k8s?

Yes. The high-level source of information here is not changed. Previously, the node manager's NodeUpdated() forwarded the node object to the linuxNodeHandler for updating the tunnel map, now the node manager extracts the pod CIDRs itself (but linuxNodeHandler part is preserved for downgrade compatibility).

2. on initial upgrade, how do we handle the race condition where an endpoint's BPF program is re-generated (removing the tunnel-map lookup) **before** the PodCIDRs are populated into the ipcache? Is that just a dependency that's already enforced inside the agent?

Yes, this the dependency is enforced by the agent. Endpoints are not regenerated until IPCache is fully populated. We wait for the K8s caches to be synced (and the initial IPCache revision to be realized) before we allow endpoint regeneration to start.

3. for deleted nodes during agent restart, we rely on the [synthesized node deletion](https://github.com/cilium/cilium/pull/33278) feature to clean up the stale PodCIDRs?

Since IPCache is re-created when the agent restarts, there are no stale pod CIDRs to clean up (at least with regards to IPCache - when it comes to the $podCIDR via cilium_host routes installed by linuxNodeHandler, we do rely on those - but that behavior in unchanged with this PR).

doniacld

Nice 🌍

gandro · 2025-03-11T09:09:41Z

So I just had the realization that keeping the old tunnel map around for downgrade purposes is pretty pointless, since we are re-creating it every time agent restarts:

cilium/daemon/cmd/datapath.go

Line 162 in c6f05a5

if err := tunnel.TunnelMap().Recreate(); err != nil {

So it might make sense to re-add node: Remove insertions and deletions to tunnel map and remove the tunnel map completely.

gandro · 2025-03-12T08:13:15Z

/test

gandro · 2025-03-13T11:29:38Z

Putting this back into draft, as we have discovered that this needs a cluster-id aware upsert into IPCache.

julianwiedmann · 2025-03-17T12:29:39Z

2. on initial upgrade, how do we handle the race condition where an endpoint's BPF program is re-generated (removing the tunnel-map lookup) **before** the PodCIDRs are populated into the ipcache? Is that just a dependency that's already enforced inside the agent?
Yes, this the dependency is enforced by the agent. Endpoints are not regenerated until IPCache is fully populated. We wait for the K8s caches to be synced (and the initial IPCache revision to be realized) before we allow endpoint regeneration to start.
3. for deleted nodes during agent restart, we rely on the [synthesized node deletion](https://github.com/cilium/cilium/pull/33278) feature to clean up the stale PodCIDRs?
Since IPCache is re-created when the agent restarts, there are no stale pod CIDRs to clean up (at least with regards to IPCache - when it comes to the $podCIDR via cilium_host routes installed by linuxNodeHandler, we do rely on those - but that behavior in unchanged with this PR).

Awesome, thank you! This helps a lot to understand the bigger picture. And it also clarifies what happens on downgrade - as the IPCache is rebuilt, the PodCIDR entries naturally disappear again.

snowhanse · 2025-03-24T07:29:15Z

Just wanted to add to this to say that this is exactly what I've been looking for.
I've done a much cruder version of this, proving out that cilium can support my use case of having default pod ip pool use tunnel and separate pool as routable with IPs advertised over BGP.

So I'll also wait for this one to merge, and in the meantime use it as a base for my testing.

pippolo84 · 2025-03-25T12:50:09Z

Superseded by #38483

gandro force-pushed the pr/gandro/use-ipcache-map-for-tunneling-v1.1 branch from 2b92eb9 to 07e6d26 Compare March 5, 2025 16:08

gandro force-pushed the pr/gandro/use-ipcache-map-for-tunneling-v1.1 branch from 07e6d26 to 2bb568d Compare March 5, 2025 16:11

gandro force-pushed the pr/gandro/use-ipcache-map-for-tunneling-v1.1 branch from 2bb568d to 59f8ed8 Compare March 5, 2025 16:32

gandro force-pushed the pr/gandro/use-ipcache-map-for-tunneling-v1.1 branch from 59f8ed8 to a06a68b Compare March 5, 2025 16:51

gandro force-pushed the pr/gandro/use-ipcache-map-for-tunneling-v1.1 branch from a06a68b to 7b9439f Compare March 6, 2025 08:43

gandro force-pushed the pr/gandro/use-ipcache-map-for-tunneling-v1.1 branch 2 times, most recently from 763380b to d836551 Compare March 6, 2025 09:16

gandro force-pushed the pr/gandro/use-ipcache-map-for-tunneling-v1.1 branch from d836551 to 8067549 Compare March 6, 2025 09:54

gandro marked this pull request as ready for review March 6, 2025 09:54

gandro requested review from a team as code owners March 6, 2025 09:54

gandro requested review from a user, doniacld, thorn3r and ysksuzuki March 6, 2025 09:54

gandro requested review from aanm and christarazi March 6, 2025 09:54

julianwiedmann self-requested a review March 6, 2025 11:11

aanm approved these changes Mar 6, 2025

View reviewed changes

pippolo84 approved these changes Mar 6, 2025

View reviewed changes

pkg/node/manager/manager.go Show resolved Hide resolved

christarazi approved these changes Mar 6, 2025

View reviewed changes

ghost approved these changes Mar 10, 2025

View reviewed changes

pippolo84 and others added 6 commits March 11, 2025 09:49

node: Consider all node CIDRs when deleting a node

7994aa9

When handling the deletion of a node, we should take into account all the node allocations CIDRs, not just the primary one. Signed-off-by: Fabio Falzoi <[email protected]>

ci/multi-pool: Add test for tunnel mode

ef0deac

As it is now supported. Signed-off-by: Sebastian Wicki <[email protected]>

gandro force-pushed the pr/gandro/use-ipcache-map-for-tunneling-v1.1 branch from 8067549 to ef0deac Compare March 11, 2025 08:50

doniacld approved these changes Mar 11, 2025

View reviewed changes

pippolo84 mentioned this pull request Mar 12, 2025

[WIP] Remove insertions and deletions to deprecated tunnel map #38153

Closed

gandro marked this pull request as draft March 13, 2025 11:29

pippolo84 mentioned this pull request Mar 20, 2025

Make ipcache async api cluster id aware #38379

Merged

pippolo84 mentioned this pull request Mar 25, 2025

Support tunnel routing in Multi-Pool IPAM mode #38483

Merged

pippolo84 closed this Mar 25, 2025

christarazi deleted the pr/gandro/use-ipcache-map-for-tunneling-v1.1 branch March 26, 2025 01:47

Conversation

gandro commented Mar 5, 2025

Uh oh!

gandro commented Mar 5, 2025

Uh oh!

gandro commented Mar 5, 2025

Uh oh!

gandro commented Mar 5, 2025

Uh oh!

gandro commented Mar 5, 2025

Uh oh!

gandro commented Mar 6, 2025

Uh oh!

gandro commented Mar 6, 2025

Uh oh!

gandro commented Mar 6, 2025

Uh oh!

pippolo84 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

christarazi left a comment

Choose a reason for hiding this comment

Uh oh!

julianwiedmann commented Mar 7, 2025

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

gandro commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doniacld left a comment

Choose a reason for hiding this comment

Uh oh!

gandro commented Mar 11, 2025

Uh oh!

gandro commented Mar 12, 2025

Uh oh!

gandro commented Mar 13, 2025

Uh oh!

julianwiedmann commented Mar 17, 2025

Uh oh!

snowhanse commented Mar 24, 2025

Uh oh!

pippolo84 commented Mar 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

gandro commented Mar 11, 2025 •

edited

Loading