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

daemon: Fail init if requirements for BPF masquerade are not met #29778

Merged
merged 2 commits into from
Jan 9, 2024
Merged

daemon: Fail init if requirements for BPF masquerade are not met #29778

merged 2 commits into from
Jan 9, 2024

Conversation

pippolo84
Copy link
Member

@pippolo84 pippolo84 commented Dec 11, 2023 •
edited

In case the requirements for BPF masquerade are not met, fail the daemon initialization instead of silently fall back to iptables based masquerading.

This is needed because the iptables cell does not support runtime configuration changes.

Fixes: #29663

@pippolo84 pippolo84 added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/agent Cilium agent related. labels Dec 11, 2023
@pippolo84
Copy link
Member Author

/test

@pippolo84 pippolo84 marked this pull request as ready for review December 11, 2023 10:48
@pippolo84 pippolo84 requested review from a team as code owners December 11, 2023 10:48
giorio94
giorio94 requested changes Dec 11, 2023
View reviewed changes
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Thanks!

There's another occurrence which also sets EnableBPFMasquerade to false a few lines below. It may not be affected (as in that case MasqueradingEnabled() is false), but it is likely worth to also address that one, so that the value is effectively never mutated.

learnitall
learnitall approved these changes Dec 11, 2023
View reviewed changes
Copy link
Contributor

@learnitall learnitall left a comment

Choose a reason for hiding this comment

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

Upgrade notes look good!

@pippolo84 pippolo84 force-pushed the pr/pippolo84/masquerade-kpr-disabled branch from 574441f to 0d321b4 Compare December 13, 2023 11:28
@pippolo84
Copy link
Member Author

Thanks!

There's another occurrence which also sets EnableBPFMasquerade to false a few lines below. It may not be affected (as in that case MasqueradingEnabled() is false), but it is likely worth to also address that one, so that the value is effectively never mutated.

Fixed, PTAL 🙏

giorio94
giorio94 approved these changes Dec 13, 2023
View reviewed changes
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Thanks!

@joestringer
Copy link
Member

I think that this is probably the right approach to make this behaviour more explicit and clear to the user. At the same time, I'm not sure whether we could backport this to any released version given that users could be currently relying on the fallback behaviour and they don't expect to see their cluster break upon upgrade to a .5 or .6 patch release based on an opinionated flag parsing behaviour like this. I'd be open to backport this into v1.15 if y'all strongly advocate for it, or otherwise we could defer to v1.16 if there's no specific rush.

@giorio94
Copy link
Member

I'm not sure whether we could backport this to any released version given that users could be currently relying on the fallback behaviour and they don't expect to see their cluster break upon upgrade to a .5 or .6 patch release

Totally agree with this.

I'd be open to backport this into v1.15 if y'all strongly advocate for it

IMHO we need to backport to v1.15 to fix #29663.

@pippolo84
Copy link
Member Author

I'd be open to backport this into v1.15 if y'all strongly advocate for it, or otherwise we could defer to v1.16 if there's no specific rush.

I think it is sufficient to backport this to v1.15 only, as the primary intent of the PR was to fix #29663 after iptables modularization (as Marco already pointed out).

@pippolo84 pippolo84 added the needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch label Dec 18, 2023
@pippolo84
Copy link
Member Author

/test

@pippolo84
Copy link
Member Author

pippolo84 commented Dec 20, 2023 •
edited

ci-ginkgo failure tracked here, rerunning Update: failure is related, not a flake

@pippolo84 pippolo84 mentioned this pull request Dec 21, 2023
Closed
@pippolo84 pippolo84 requested review from a team as code owners December 21, 2023 13:24
@pippolo84
daemon: Fail init if requirements for BPF masquerade are not met
d6c5a15 
In case the requirements for BPF masquerade are not met, fail the daemon
initialization instead of silently fall back to iptables based
masquerading.

This is needed because the iptables cell does not support runtime
configuration changes.

Fixes: a6a2b73 ("iptables: Add a cell for iptables config manager")

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84
test: Disable BPF masquerade in fromEntities policies tests
57d879d 
The previous commit removed the silent fallback to iptables based
masquerading if the requirements for BPF masquerading are not met.

Since having both enable-ipv4-masquerade and enable-ipv6-masquerade set
to false is incompatible with BPF masquerading, the ginkgo tests that
need this configuration now have to explicitly set iptables based
masquerading.

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84 pippolo84 force-pushed the pr/pippolo84/masquerade-kpr-disabled branch from 74ffb7a to 57d879d Compare December 21, 2023 13:26
@pippolo84
Copy link
Member Author

/test

@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 Jan 9, 2024
@nathanjsweet nathanjsweet added this pull request to the merge queue Jan 9, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in v1.15.0-rc.1 Jan 9, 2024
Merged via the queue into cilium:main with commit ca1de25 Jan 9, 2024
62 checks passed
@jibi jibi mentioned this pull request Jan 12, 2024
Merged
32 tasks
@jibi jibi added backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. and removed needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Jan 12, 2024
@aanm aanm mentioned this pull request Jan 16, 2024
Merged
@giorio94 giorio94 added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Jan 29, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport done to v1.15 in v1.15.0-rc.1 Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@christarazi christarazi christarazi approved these changes

@learnitall learnitall learnitall approved these changes

@thorn3r thorn3r thorn3r approved these changes

@giorio94 giorio94 giorio94 approved these changes

Assignees
No one assigned
Labels
backport-done/1.15 The backport for Cilium 1.15.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/agent Cilium agent related.
Projects
No open projects
v1.15.0-rc.1
Backport done to v1.15
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Masquerade silently broken when BPF masquerade is enabled, but KPR is disabled
8 participants
@pippolo84 @joestringer @giorio94 @nathanjsweet @christarazi @learnitall @thorn3r @jibi