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

policy: Describe CIDR superset logic for denies and FQDN #26720

Merged
merged 1 commit into from Jul 13, 2023

Conversation

joestringer
Copy link
Member

This commit attempts to elaborate on the design decisions that have
introduced the need for the mapstate entry merging layer to handle
CIDR supersets.

There is a part of me that would be fond of removing all CIDR superset
evaluation logic from the mapstate layer in order to simplify the logic
and reduce iteration while handling incremental updates. I might argue
that in an ideal world, CIDR policy overlaps would be handled at the
selector layer, ie when evaluating L4PolicyMap / L4Filter against a set
of identities to generate MapState. In such a world, there would be no
need for incremental policy calculation to iterate through all current
mapstate in order to evaluate conflicts between CIDRs in the policy
rules. However, for now this is how we implement the policy correctly
and (relatively) efficiently. In the absence of a more concrete proposal
in that direction, it's worthwhile at least documenting the background
here. This may assist the contemplation of how newer implementations at
the mapstate layer (such as policy auth) may interact with identities
where those identities have a superset relationship with other CIDRs.

This commit attempts to elaborate on the design decisions that have
introduced the need for the mapstate entry merging layer to handle
CIDR supersets.

There is a part of me that would be fond of removing all CIDR superset
evaluation logic from the mapstate layer in order to simplify the logic
and reduce iteration while handling incremental updates. I might argue
that in an ideal world, CIDR policy overlaps would be handled at the
selector layer, ie when evaluating L4PolicyMap / L4Filter against a set
of identities to generate MapState. In such a world, there would be no
need for incremental policy calculation to iterate through all current
mapstate in order to evaluate conflicts between CIDRs in the policy
rules. However, for now this is how we implement the policy correctly
and (relatively) efficiently. In the absence of a more concrete proposal
in that direction, it's worthwhile at least documenting the background
here. This may assist the contemplation of how newer implementations at
the mapstate layer (such as policy auth) may interact with identities
where those identities have a superset relationship with other CIDRs.

Signed-off-by: Joe Stringer <joe@cilium.io>
@joestringer joestringer requested a review from a team as a code owner July 7, 2023 22:11
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Jul 7, 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 Jul 7, 2023
@joestringer joestringer requested a review from squeed July 8, 2023 00:28
Copy link
Member

@nathanjsweet nathanjsweet left a comment

Choose a reason for hiding this comment

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

Wow, that is a fantastically accurate, succinct, yet comprehensive summary of policy map insertion and why it is so complex. Thanks!

@nathanjsweet nathanjsweet added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 11, 2023
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

Thanks, this was surprisingly easy to parse and understand!

@christarazi christarazi added kind/cleanup This includes no functional changes. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. labels Jul 12, 2023
@julianwiedmann julianwiedmann merged commit 051b5c3 into main Jul 13, 2023
56 checks passed
@julianwiedmann julianwiedmann deleted the pr/joe/superset-policy-note branch July 13, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup This includes no functional changes. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. 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

4 participants