Update the text about duplicate priority values #229
Conversation
|
@danwinship: GitHub didn't allow me to request PR reviews from the following users: fasaxc, nathanjsweet. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
cncf-cla: yes
✅ Deploy Preview for kubernetes-sigs-network-policy-api ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
k8s-ci-robot
added
the
size/XS
| // The behavior is undefined if two ANP objects have same priority. | ||
| // Every AdminNetworkPolicy should have a unique priority value. If multiple | ||
| // policies have the same priority, and match the same connection, then it is | ||
| // implementation-defined which policy's rules get applied to it. |
There was a problem hiding this comment.
maybe replace "implementation-defined" with "the policies will be applied in some order"? implementation-defined feels like implementations must tell you an order, but I think the point is to not rely on any specific order here
There was a problem hiding this comment.
I said "implementation-defined" because I wanted to make it clear that different implementations might do different things. I also didn't want to say anything that could be read as implying that the result will be predictable...
Updated... what do you think of the new text?
danwinship
force-pushed
the
priority-conflict
branch
from
75c8ea2
to
978f917
Compare
k8s-ci-robot
added
size/S
| // Every AdminNetworkPolicy should have a unique priority value; if two (or more) | ||
| // policies with the same priority could both match a connection, then there is no | ||
| // way for the user to reliably determine which of the policies will actually be | ||
| // applied to it. |
There was a problem hiding this comment.
looks nicer now than saying implementation defined..
| // Every AdminNetworkPolicy should have a unique priority value; if two (or more) | |
| // policies with the same priority could both match a connection, then there is no | |
| // way for the user to reliably determine which of the policies will actually be | |
| // applied to it. | |
| // Every AdminNetworkPolicy should have a unique priority value; if two (or more) | |
| // policies have the same priority and both have rules that match the same connection at the same index (rules have precedence based on ordering), then there is no | |
| // way for the user to reliably determine which of the policies will actually be | |
| // applied to the connection. If both policies match different non-overlapping connections | |
| // then both policies are expected to work correctly |
something like that? I wanted to make sure we highlight the fact that both ANPs are expected to be created and applied correctly at least for the non overlapping case.. don't want to overengineer this but if I have ANP1 at prio3 whose rule0 matches connectionA and ANP2 at prio3 whose rule1 matches connectionA for sure its guaranteed ANP1 wins, so I guess its a combo of prio+precedence when we speak about two ANPs?
There was a problem hiding this comment.
if I have ANP1 at prio3 whose rule0 matches connectionA and ANP2 at prio3 whose rule1 matches connectionA for sure its guaranteed ANP1 wins
Wait Is it? I don't think we can count on ANP internal rule ordering when there's conflicting priorities, i.e ordering rule0 vs rule1 across two different ANPs doesn't make much sense to me 🤔 . In this ^^ case based on what Dan has written I would think the rules do overlap (regardless of rule index) and expect that only one of the ANPs takes effect (which one is up to the implementation).
But I'm happy to be told otherwise :)
There was a problem hiding this comment.
I don't think we have ever said how internal precedence works across multiple policies https://github.com/kubernetes-sigs/network-policy-api/blob/main/apis/v1alpha1/adminnetworkpolicy_types.go#L80-L82, so I think we should also add a note there for policies with the same priority, wdyt?
There was a problem hiding this comment.
if I have ANP1 at prio3 whose rule0 matches connectionA and ANP2 at prio3 whose rule1 matches connectionA for sure its guaranteed ANP1 wins
No, that's an allowed implementation, but it's not guaranteed.
eg, assuming "ANP1" and "ANP2" are the actual object names, then in the calico-esque "the ANPs get sorted alphabetically" interpretation, then rule1 in ANP1 has precedence over rule0 in ANP2.
And in the "denies beat allows" example, it depends on the actual rules: if ANP1 rule0 is Allow and ANP2 rule1 is Deny, then the result is Deny.
There was a problem hiding this comment.
how about: "if two (or more) policies with the same priority could both match a connection, then the implementation can apply any of the matching policies to the connection, and there is no way for the user to reliably determine which one it will choose" ?
There was a problem hiding this comment.
if I have ANP1 at prio3 whose rule0 matches connectionA and ANP2 at prio3 whose rule1 matches connectionA for sure its guaranteed ANP1 wins
No, that's an allowed implementation, but it's not guaranteed.
eg, assuming "ANP1" and "ANP2" are the actual object names, then in the calico-esque "the ANPs get sorted alphabetically" interpretation, then rule1 in ANP1 has precedence over rule0 in ANP2.
And in the "denies beat allows" example, it depends on the actual rules: if ANP1 rule0 is Allow and ANP2 rule1 is Deny, then the result is Deny.
ah I see your point; withdrawing that statement then.
There was a problem hiding this comment.
how about: "if two (or more) policies with the same priority could both match a connection, then the implementation can apply any of the matching policies to the connection, and there is no way for the user to reliably determine which one it will choose" ?
yes this works, lgtm
|
Only thing I might do is consider adding the #216 link to the comment? I know it's not great to embed issues there but if folks are confused the issue conversation could help clear things up. |
|
LGTM |
danwinship
force-pushed
the
priority-conflict
branch
from
978f917
to
8be8ff5
Compare
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, tssurya The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Clarify the limits of the undefined behavior when two policies have the same priority.
Fixes #216
/cc @astoycos @tssurya @npinaeva @fasaxc @nathanjsweet @aojea