Fix dropped NodePort traffic to hostNetwork backends with Geneve+DSR #36978

tommasopozzetti · 2025-01-14T21:13:53Z

This PR fixes DSR dispatch with Geneve to hostNetworked pods by ensuring tunnel_endpoint is always populated in ipcache, even for directly reachable endpoints. Unneeded encapsulation is avoided by passing the skiptunnel flag for such endpoints.

Fixes: #36901

julianwiedmann · 2025-01-15T07:44:32Z

Thank you! I'll have a closer look later.

Also queued up #36982. Once that's merged, we can rebase this PR so that we get immediate feedback on the connectivity test.

Artyop

lgtm

julianwiedmann · 2025-01-15T14:40:35Z

Also queued up #36982. Once that's merged, we can rebase this PR so that we get immediate feedback on the connectivity test.

Merged, I'll auto-rebase the branch...

julianwiedmann · 2025-01-15T14:41:28Z

/ci-e2e-upgrade

tommasopozzetti · 2025-01-15T21:56:26Z

Thanks for the quick look!
I see some connectivity test cases failed waiting for the new pod that gets introduced though not 100% sure how to go check more details about those setups. Any pointer would be appreciated, first time going through the cilium CI process! 😄

tommasopozzetti · 2025-01-16T16:02:52Z

@julianwiedmann fixed the conflict. Any tips on next steps to get this in?

julianwiedmann

Some comments inline, before I forgot about the CLI compatibility aspect again ...

bpf/lib/nodeport.h

cilium-cli/connectivity/builder/north_south_loadbalancing.go

bpf/lib/nodeport.h

julianwiedmann · 2025-01-17T07:39:11Z

@julianwiedmann fixed the conflict. Any tips on next steps to get this in?

In general - please rebase on-top of main, instead of merging main into your branch.

I see some connectivity test cases failed waiting for the new pod that gets introduced though not 100% sure how to go check more details about those setups.

IIRC the CLI tests for cluster-internal connectivity to all the nodeport services on startup, before running the connectivity tests. Is that what's failing? Let's re-run the workflow and find out.

tommasopozzetti · 2025-02-05T15:19:42Z

@dylandreimerink Thanks! Done!
Hopefully now we can get it in before it lags too much behind again 😅

dylandreimerink · 2025-02-05T15:54:58Z

/test

doniacld

LGTM but asking peers from cilium/ipcache to have a second look regarding the addition of a new field to IPCache. cc @squeed

doniacld · 2025-02-10T15:07:07Z

LGTM but asking peers from cilium/ipcache to have a second look regarding the addition of a new field to IPCache. cc @squeed

@squeed mentioned that's fine to add a new field to IPCache since we are mapping with the BPF side.

joestringer · 2025-02-10T16:46:44Z

Thanks for the fix! Merging.

…onfig With cilium#36978 the IPcache entry for a remote node now contains a tunnel_endpoint, typically in combination with the `skip_tunnel` flag set. But when dealing with replies for a connection that was established via the overlay network, we want to continue routing those replies via the overlay. Also fix up the corresponding BPF test. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>

With cilium#36978 the IPcache entry for a remote node now contains a tunnel_endpoint, typically in combination with the `skip_tunnel` flag set. But when dealing with replies for a connection that was established via the overlay network, we want to continue routing those replies via the overlay. Also fix up the corresponding BPF test. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>

This reproduces disruption for an established pod-to-remote-node connection, caused by the changes in cilium#36978. In old code such a connection would be masqueraded, but in new code the masquerading is skipped. Thus the destination will see requests with different source IPs. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>

julianwiedmann · 2025-03-09T15:06:55Z

Sorry, I think we'll have to revert this from v1.17. I believe it causes a regression, as described here. We can keep it in main for now if that helps.

julianwiedmann · 2025-03-09T15:12:54Z

I've queued up the revert here.

I actually like the approach of always setting the tunnel_endpoint, and thus associating an endpoint with its node's "primary" IP. I don't think we've had such an association before, and it feels beneficial. It even allows for a subsequent lookup in the node_map, and thus a decision whether two remote endpoints live on the same remote node.

But the way we currently handle pod-to-remote-node traffic (SNAT instead of tunnel), along with how the skip_tunnel flag was initially intended, makes this harder to implement. I'd be happy to give this a bit more thought & experimentation (post-PTO :)), I'm optimistic we can make it work and end up with a better overall design for pod-to-remote-node connections.

tommasopozzetti · 2025-03-09T15:25:58Z

@julianwiedmann sorry to hear that!
Fixing the original issue here is still very much critical to us as without this fix there still is a significant issue with dropped traffic with that configuration!
I’d be happy to help tackle it differently in the meantime if you have suggestions or contribute to the solution any way I can

tommasopozzetti · 2025-03-09T15:37:48Z

I’m also curious if you could expand (or point to docs) on the original intent of that flag
Reading through the code block you pointed out causes the issue with the pod to remote node traffic it’s not clear to me why the presence of the skip_tunnel flag would skip SNAT. Considering the current implementation never uses overlay tunneling for that communication to begin with, why would it need a different behavior depending on the value of that flag?

…nfig With cilium#36978 the IPcache entry for a remote node now contains a tunnel_endpoint, typically in combination with the `skip_tunnel` flag set. But when dealing with replies for a connection that was established via the overlay network, we want to continue routing those replies via the overlay. Also fix up the corresponding BPF test. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>

…unnel With cilium#36978 the IPcache entry for a remote node now contains a tunnel_endpoint, typically in combination with the `skip_tunnel` flag set. But when dealing with replies for a connection that was established via the overlay network, we want to continue routing those replies via the overlay. Also fix up the corresponding BPF test for inter-cluster-SNAT. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>

…unnel With #36978 the IPcache entry for a remote node now contains a tunnel_endpoint, typically in combination with the `skip_tunnel` flag set. But when dealing with replies for a connection that was established via the overlay network, we want to continue routing those replies via the overlay. Also fix up the corresponding BPF test for inter-cluster-SNAT. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>

tommasopozzetti requested review from a team as code owners January 14, 2025 21:13

tommasopozzetti requested review from dylandreimerink and Artyop January 14, 2025 21:13

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 14, 2025

github-actions bot added cilium-cli This PR contains changes related with cilium-cli kind/community-contribution This was a contribution made by a community member. labels Jan 14, 2025

tommasopozzetti mentioned this pull request Jan 14, 2025

Cilium with geneve and DSR drops external traffic to host network backends #36901

Open

3 tasks

julianwiedmann self-requested a review January 15, 2025 07:43

maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jan 15, 2025

julianwiedmann added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch needs-backport/1.17 This PR / issue needs backporting to the v1.17 branch labels Jan 15, 2025

maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jan 15, 2025

Artyop approved these changes Jan 15, 2025

View reviewed changes

julianwiedmann force-pushed the pr/fix-nodeport-to-hostnetns-dsr-geneve branch from ce585d8 to aa75d28 Compare January 15, 2025 14:40

julianwiedmann reviewed Jan 17, 2025

View reviewed changes

bpf/lib/nodeport.h Outdated Show resolved Hide resolved

cilium-cli/connectivity/builder/north_south_loadbalancing.go Outdated Show resolved Hide resolved

bpf/lib/nodeport.h Outdated Show resolved Hide resolved

doniacld approved these changes Feb 10, 2025

View reviewed changes

joestringer added affects/v1.16 This issue affects v1.16 branch and removed needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch cilium-cli This PR contains changes related with cilium-cli labels Feb 10, 2025

joestringer added this pull request to the merge queue Feb 10, 2025

Merged via the queue into cilium:main with commit e8db5d3 Feb 10, 2025
66 checks passed

nbusseneau mentioned this pull request Feb 14, 2025

v1.17 Backports 2025-02-14 #37648

Merged

22 tasks

nbusseneau added backport-pending/1.17 The backport for Cilium 1.17.x for this PR is in progress. and removed needs-backport/1.17 This PR / issue needs backporting to the v1.17 branch labels Feb 14, 2025

julianwiedmann mentioned this pull request Feb 19, 2025

IPsec VinE Implementation #37723

Merged

github-actions bot added backport-done/1.17 The backport for Cilium 1.17.x for this PR is done. and removed backport-pending/1.17 The backport for Cilium 1.17.x for this PR is in progress. labels Feb 20, 2025

julianwiedmann mentioned this pull request Mar 3, 2025

bpf: lxc: prioritize ct_state->from_tunnel over destination's .skip_tunnel #37972

Merged

julianwiedmann mentioned this pull request Mar 9, 2025

bpf: tests: add BPF-masq test for pod-to-remote node connection #38099

Closed

julianwiedmann mentioned this pull request Mar 9, 2025

[v1.17] Revert "Fix dropped NodePort traffic to hostNetwork backends with Geneve+DSR" #38101

Merged

Fix dropped NodePort traffic to hostNetwork backends with Geneve+DSR #36978

Fix dropped NodePort traffic to hostNetwork backends with Geneve+DSR #36978

Uh oh!

Conversation

tommasopozzetti commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

julianwiedmann commented Jan 15, 2025

Uh oh!

Artyop left a comment

Choose a reason for hiding this comment

Uh oh!

julianwiedmann commented Jan 15, 2025

Uh oh!

julianwiedmann commented Jan 15, 2025

Uh oh!

tommasopozzetti commented Jan 15, 2025

Uh oh!

tommasopozzetti commented Jan 16, 2025

Uh oh!

julianwiedmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

julianwiedmann commented Jan 17, 2025

Uh oh!

tommasopozzetti commented Feb 5, 2025

Uh oh!

dylandreimerink commented Feb 5, 2025

Uh oh!

doniacld left a comment

Choose a reason for hiding this comment

Uh oh!

doniacld commented Feb 10, 2025

Uh oh!

joestringer commented Feb 10, 2025

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

tommasopozzetti commented Mar 9, 2025

Uh oh!

tommasopozzetti commented Mar 9, 2025

Uh oh!

Uh oh!

tommasopozzetti commented Jan 14, 2025 •

edited

Loading

julianwiedmann commented Mar 9, 2025 •

edited

Loading

julianwiedmann commented Mar 9, 2025 •

edited

Loading