Skip to content

Revert #36978 #39333

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 6, 2025
Merged

Revert #36978 #39333

merged 1 commit into from
May 6, 2025

Conversation

brb
Copy link
Member

@brb brb commented May 5, 2025 •
edited
Loading

Same reasoning as in #38101.

In addition, @giorio94 discovered:

One more issue caused by this change. Essentially, when Cilium runs in tunnel mode and BPF masquerade is enabled, pod to node traffic is no longer masqueraded due to [1]. Depending on the setup, this can have various consequences, including NodePort traffic being dropped when targeting the node hosting the target pod, if SocketLB is disabled (hence translation is not performed at the source) and rp_filter of the network interface is set to 1. The same issue does not occur when BPF masquerade is disabled, as in that case traffic is correctly masqueraded.

[1]:

cilium/bpf/lib/nat.h

Lines 745 to 758 in 20db286

/* In overlay routing mode, pod-to-remote-node traffic
* typically doesn't get transported via the overlay
* network (https://github.com/cilium/cilium/issues/12624).
*
* Therefore such packet has to be masqueraded.
* Otherwise it might be dropped
* either by underlying network (e.g. AWS drops
* packets by default from unknown subnets) or
* by the remote node if its native dev's
* rp_filter=1.
*/
if (remote_ep->flag_skip_tunnel)
return NAT_PUNT_TO_STACK;

Keep in mind, that this PR does not revert the plumbing, as it might be used in the future (#39333 (comment)).

cc @julianwiedmann @tommasopozzetti

@brb
Revert "Fix NodePort traffic to host netns backends with Geneve+DSR"
b378cb7 
This reverts commit e8db5d3.

Signed-off-by: Martynas Pumputis <martynas@isovalent.com>
@brb brb added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels May 5, 2025
@github-actions github-actions bot added the sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. label May 5, 2025
@brb brb force-pushed the pr/brb/revert-geneve-dsr-skip-tunnel branch 2 times, most recently from cb09e3f to 3555a39 Compare May 5, 2025 11:13
@julianwiedmann
Copy link
Member

It might be easier to keep all the ipcache plumbing in place, but just roll the tunnel_endpoint and skip_tunnel decision logic back to how we've done things in the past?

@brb
Copy link
Member Author

brb commented May 5, 2025

@julianwiedmann No strong opinion. Do you think we might reuse them in the future?

@brb brb force-pushed the pr/brb/revert-geneve-dsr-skip-tunnel branch 2 times, most recently from b22a8ba to c876b93 Compare May 5, 2025 13:11
@julianwiedmann
Copy link
Member

@julianwiedmann No strong opinion. Do you think we might reuse them in the future?

I could see us eventually wanting the solution that this PR implemented. We're just not there architecture-wise (ie. pod-to-remote-node should flow via the overlay network, not require masquerading).

@brb brb force-pushed the pr/brb/revert-geneve-dsr-skip-tunnel branch from c876b93 to b378cb7 Compare May 5, 2025 13:36
@brb
Copy link
Member Author

brb commented May 5, 2025

👍 OK, then keeping the plumbing.

@brb
Copy link
Member Author

brb commented May 5, 2025

/test

@tommasopozzetti tommasopozzetti mentioned this pull request May 5, 2025
Open
3 tasks
@brb brb marked this pull request as ready for review May 6, 2025 09:34
@brb brb requested a review from a team as a code owner May 6, 2025 09:34
@brb brb requested a review from gandro May 6, 2025 09:34
@brb brb added this pull request to the merge queue May 6, 2025
Merged via the queue into main with commit 07f37d3 May 6, 2025
356 of 381 checks passed
@brb brb deleted the pr/brb/revert-geneve-dsr-skip-tunnel branch May 6, 2025 13:15
@joestringer joestringer added release-note/misc This PR makes changes that have no direct user impact. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jul 22, 2025
@joestringer
Copy link
Member

I changed this to release-note/misc. It looks like the revert was already included in a v1.17.x release, and it doesn't make sense to have a user-facing release note for a PR that just says "revert X". If you think this may be relevant directly to some users, please update the title to reflect the user impact of the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

Assignees
No one assigned
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brb @julianwiedmann @joestringer @gandro