Skip to content
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

node/manager: Remove ipset config from previous node state #24825

Merged
merged 1 commit into from
Apr 18, 2023
Merged

node/manager: Remove ipset config from previous node state #24825

merged 1 commit into from
Apr 18, 2023

Conversation

christarazi
Copy link
Member

@christarazi christarazi commented Apr 11, 2023 •
edited

Found by code inspection from
#23208 (comment),
thanks to Joe.

Fix this discrepancy so that we can potentially backport this fix if
needed to older branches. The surrounding logic will get refactored in
the aforementioned PR.

This affects users that are running with the following:

  • --tunnel=disabled (native routing)
  • --enable-bpf-masquerade=false
  • --enable-ipv{4,6}-masquerade=true

Fixes: 49cb220 ("iptables: Don't masquerade traffic to cluster nodes")
Suggested-by: Joe Stringer joe@cilium.io
Signed-off-by: Chris Tarazi chris@isovalent.com

Fix bug where Cilium configurations running with tunneling disabled, BPF-masq disabled, but with masquerading enabled, do not clean up ipset configuration when a node IP changes. This can lead to a lack of masquerading on those node IPs.

cc @pchaigno

@christarazi
node/manager: Remove ipset config from previous node state
150d866 
Found by code inspection from
#23208 (comment),
thanks to Joe.

Fix this discrepancy so that we can potentially backport this fix if
needed to older branches. The surrounding logic will get refactored in
the aforementioned PR.

This affects users that are running with the following:
  - --tunnel=disabled (native routing)
  - --enable-bpf-masquerade=false
  - --enable-ipv{4,6}-masquerade=true

Fixes: 49cb220 ("iptables: Don't masquerade traffic to cluster nodes")
Suggested-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi added kind/bug This is a bug in the Cilium logic. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Apr 11, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Apr 11, 2023
@christarazi christarazi added needs-backport/1.12 affects/v1.11 This issue affects v1.11 branch needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Apr 11, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.13.2 Apr 11, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.12.9 Apr 11, 2023
@christarazi christarazi marked this pull request as ready for review April 11, 2023 23:50
@christarazi christarazi requested a review from a team as a code owner April 11, 2023 23:50
@christarazi
Copy link
Member Author

/test

@christarazi christarazi mentioned this pull request Apr 11, 2023
Merged
pchaigno
pchaigno approved these changes Apr 12, 2023
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Nice catch!

Should we update the release note to point out that this can lead to a lack of masquerading for some traffic?

@michi-covalent michi-covalent added this to Needs backport from master in 1.12.10 Apr 14, 2023
@michi-covalent michi-covalent removed this from Needs backport from master in 1.12.9 Apr 14, 2023
@gentoo-root gentoo-root added this to Needs backport from master in 1.13.3 Apr 14, 2023
@gentoo-root gentoo-root removed this from Needs backport from master in 1.13.2 Apr 14, 2023
thorn3r
thorn3r approved these changes Apr 18, 2023
View reviewed changes
Copy link
Contributor

@thorn3r thorn3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

@maintainer-s-little-helper 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 Apr 18, 2023
@christarazi christarazi merged commit d5e5bf3 into main Apr 18, 2023
43 checks passed
@christarazi christarazi deleted the pr/christarazi/remove-ipset-on-update branch April 18, 2023 19:04
@nbusseneau nbusseneau mentioned this pull request Apr 20, 2023
Merged
7 tasks
@nbusseneau nbusseneau mentioned this pull request Apr 20, 2023
Merged
15 tasks
@nbusseneau nbusseneau added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Apr 20, 2023
@sayboras sayboras added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. backport-pending/1.12 labels Apr 26, 2023
@thorn3r thorn3r moved this from Needs backport from main to Backport done to v1.12 in 1.12.10 May 16, 2023
@thorn3r thorn3r mentioned this pull request May 16, 2023
Merged
@thorn3r thorn3r moved this from Needs backport from main to Backport done to v1.13 in 1.13.3 May 17, 2023
@thorn3r thorn3r mentioned this pull request May 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno approved these changes

@thorn3r thorn3r thorn3r approved these changes

Assignees
No one assigned
Labels
affects/v1.11 This issue affects v1.11 branch backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.12.10
Backport done to v1.12
1.13.3
Backport done to v1.13
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@christarazi @pchaigno @thorn3r @nbusseneau @sayboras