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

pkg: don't cache Host identity rule matches #30548

Merged
merged 1 commit into from
May 2, 2024
Merged

pkg: don't cache Host identity rule matches #30548

merged 1 commit into from
May 2, 2024

Conversation

squeed
Copy link
Contributor

@squeed squeed commented Jan 31, 2024

I was poking around the codebase and noticed this issue.

Unlike every other identity, the set of labels for the reserved:host identity is mutable. That means that rules should not cache matches for this identity.

So, clean up the code around determining matches.

Fixes an (unlikely) bug where HostFirewall policies may miss updates to a node's labels.

@squeed squeed 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/policy Impacts whether traffic is allowed or denied based on user-defined policies. area/host-firewall Impacts the host firewall or the host endpoint. needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Jan 31, 2024
@squeed squeed requested a review from a team as a code owner January 31, 2024 11:52
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.12 Jan 31, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in v1.15.0-rc.1 Jan 31, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.7 Jan 31, 2024
@aanm aanm added this to Needs backport from main in 1.15.1 Jan 31, 2024
@aanm aanm removed this from Needs backport from main in v1.15.0-rc.1 Jan 31, 2024
@nathanjsweet
Copy link
Member

/test

@michi-covalent michi-covalent added this to Needs backport from main in 1.14.8 Feb 13, 2024
@michi-covalent michi-covalent removed this from Needs backport from main in 1.14.7 Feb 13, 2024
@michi-covalent michi-covalent added this to Needs backport from main in 1.13.13 Feb 13, 2024
@michi-covalent michi-covalent removed this from Needs backport from main in 1.13.12 Feb 13, 2024
@julianwiedmann
Copy link
Member

@squeed 👋 does this need anything beyond a rebase + green CI?

@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Mar 31, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.7 Mar 31, 2024
qmonnet
qmonnet approved these changes Apr 11, 2024
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks good as far as I can tell. Given that this is a bug fix, can we please move this forward?

@qmonnet qmonnet removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Apr 11, 2024
@qmonnet
Copy link
Member

qmonnet commented Apr 24, 2024

@squeed Anything I can do to assist with this PR? I'm happy to rebase and trigger CI if this helps.

@squeed
Copy link
Contributor Author

squeed commented Apr 24, 2024

argh sorry, fell off my radar. I'll get it fixed.

@squeed
Copy link
Contributor Author

squeed commented Apr 24, 2024

/test

@squeed
pkg: don't cache Host identity rule matches
191358e 
Unlike every other identity, the set of labels for the reserved:host
identity is mutable. That means that rules should not cache matches for
this identity.

So, clean up the code around determining matches.

Signed-off-by: Casey Callendrello <cdc@isovalent.com>
@squeed
Copy link
Contributor Author

squeed commented Apr 24, 2024

Aha, missed a test case. Fixed now, with minor unit test fixes.

@squeed
Copy link
Contributor Author

squeed commented Apr 25, 2024

/test

@aanm aanm added this to Needs backport from main in 1.14.11 Apr 26, 2024
@aanm aanm removed this from Needs backport from main in 1.14.7 Apr 26, 2024
@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 30, 2024
@julianwiedmann julianwiedmann added this pull request to the merge queue May 2, 2024
Merged via the queue into cilium:main with commit 8397e45 May 2, 2024
64 checks passed
@pippolo84 pippolo84 mentioned this pull request May 6, 2024
Merged
14 tasks
@pippolo84 pippolo84 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 May 6, 2024
@pippolo84 pippolo84 mentioned this pull request May 6, 2024
Merged
7 tasks
@pippolo84 pippolo84 added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels May 6, 2024
@pippolo84 pippolo84 mentioned this pull request May 6, 2024
Merged
6 tasks
@pippolo84 pippolo84 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 May 6, 2024
@github-actions github-actions bot added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels May 8, 2024
@nebril nebril mentioned this pull request May 10, 2024
Merged
@nebril nebril moved this from Needs backport from main to Backport done to v1.14 in 1.14.11 May 10, 2024
This was referenced May 10, 2024
Merged
Merged
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

@qmonnet qmonnet qmonnet approved these changes

Assignees
No one assigned
Labels
area/host-firewall Impacts the host firewall or the host endpoint. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. 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/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
No open projects
1.13.13
Needs backport from main
1.14.11
Backport done to v1.14
1.14.8
Needs backport from main
1.15.1
Needs backport from main
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@squeed @nathanjsweet @julianwiedmann @qmonnet @pippolo84