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.

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

daemon: Update code comment regarding PolicyReactionEvent #25607

Merged
merged 1 commit into from May 23, 2023

Conversation

christarazi
Copy link
Member

This code comment is confusing because it states that
reactToRuleUpdates() waits until all endpoints have been regenerated.

Upon code inspection, this is incorrect and not necessary either. It is
not necessary because 2820a1f ("daemon: Switch policy over to new
IPCache interface") fixed the previously buggy behavior where prefixes
were pushed into the ipcache BPF map before the policy recalculation
(i.e. endpoint regeneration) has completed (endpoint policymap entries).
This buggy behavior likely caused CI flakes of some sort when new
policies were added.

The 8f20d3b commit mistakenly asserted this, so fix it in this
commit.

Fixes: 8f20d3b ("daemon: Postpone ipcache upserts until after policy
changes have been regenerated by endpoints.")

Reported-by: Joe Stringer joe@cilium.io
Signed-off-by: Chris Tarazi chris@isovalent.com

This code comment is confusing because it states that
reactToRuleUpdates() waits until all endpoints have been regenerated.

Upon code inspection, this is incorrect and not necessary either. It is
not necessary because 2820a1f ("daemon: Switch policy over to new
IPCache interface") fixed the previously buggy behavior where prefixes
were pushed into the ipcache BPF map before the policy recalculation
(i.e. endpoint regeneration) has completed (endpoint policymap entries).
This buggy behavior likely caused CI flakes of some sort when new
policies were added.

The 8f20d3b commit mistakenly asserted this, so fix it in this
commit.

Fixes: 8f20d3b ("daemon: Postpone ipcache upserts until after policy
changes have been regenerated by endpoints.")

Reported-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi added kind/cleanup This includes no functional changes. area/daemon Impacts operation of the Cilium daemon. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. release-note/misc This PR makes changes that have no direct user impact. labels May 23, 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 May 23, 2023
@christarazi christarazi marked this pull request as ready for review May 23, 2023 00:27
@christarazi christarazi requested a review from a team as a code owner May 23, 2023 00:27
@christarazi christarazi requested a review from nebril May 23, 2023 00:27
@christarazi
Copy link
Member Author

/test

@christarazi christarazi merged commit 847e8ad into main May 23, 2023
59 checks passed
@christarazi christarazi deleted the pr/christarazi/update-policy-daemon-doc branch May 23, 2023 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/daemon Impacts operation of the Cilium daemon. kind/cleanup This includes no functional changes. 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
Development

Successfully merging this pull request may close these issues.

None yet

2 participants