having two ANPs with the same priority must be well-defined

[danwinship]

It's unacceptable to say that the behavior when you have two ANPs with the same priority is completely undefined.

We can say that it's undefined which order they are applied in, but it must be the case that it is guaranteed that both are applied, and that two policies with priority: N are both applied in between policies with priority: N+1 and policies with priority: N-1.

[tssurya]

hmm so in OVNKube we took the decision of removing the "undefined" behaviour around this and saying "You cannot do this. No two anp's can be created at the same priority and we error out (so only the 1st one gets created its a bit buggy today because we use caches not the kapi to figure this out to save that list operation for scale reasons but then we just also document, don't do this.)". But seems like we want to actually enforce "they are both applied" ?

[astoycos]

Hrm Is there any use cases for having two distinctly separate rules at the same priority? i.e could we allow a single ANP to have multiple Subject/Rule mapping and then actually say "Users are not allowed to create two ANPs at the same priority"

Or is the problem more so that there's no mechanism for us to actually stop users from creating two ANPs at the same priority?

[tssurya]

Hrm Is there any use cases for having two distinctly separate rules at the same priority? i.e could we allow a single ANP to have multiple Subject/Rule mapping and then actually say "Users are not allowed to create two ANPs at the same priority"

yea I can't think of any use case for why I want two ANPs at same prio unless I actually have 100+ ingress and egress rules I want at same priority, so you now need two ANPs at same prio but then again since we have precedence within the single ANP it doesn't make sense because you don't now have a way to compare the two ANPs' rules

Or is the problem more so that there's no mechanism for us to actually stop users from creating two ANPs at the same priority?

I think its this really. If we can perhaps have a CEL expression to ensure priority field is unique across the crds maybe that is something we want to explore?

[tssurya]

btw I know this might not be accessible to all because its a slack link, but I wanted to put this here for posterity that I tried to add this validation a few months ago: https://redhat-internal.slack.com/archives/CB48XQ4KZ/p1691419205773149 but couldn't and api team confirmed there is no way to do this for stuff outside the metadata.

[danwinship]

and we error out

oh, you can't do that. Customers will complain. We tried it with a few things in openshift-sdn (like "you can't have more than one EgressNetworkPolicy object per Namespace and if you do, neither of them works") and that led to us having to figure out a better way to do it in ovn-k EgressFirewall ("an EgressFirewall object must be named 'default'")

[danwinship]

Or is the problem more so that there's no mechanism for us to actually stop users from creating two ANPs at the same priority?

This. We can't prevent it from happening, so it doesn't matter if there's a good use case for it or not. It needs clear semantics.

And I think there are good use cases for "two people create ANPs without coordination between them". If there is any scenario where it makes sense to include an ANP as part of a helm chart, third-party OLM operator, etc, to ensure some default set of permissions around a component, then you have to assume that eventually two components will ship ANPs with the same priority.

[aojea]

why not sort lexicographical , first priority number, second name

[danwinship]

We don't want people to depend on any specific behavior; if there is any situation where someone needs to depend on the relative ordering of two policies with the same priority, then that means that we have failed and priority was not a good API and we need something else.

(eg, there was discussion in the KEP about having a combined Action and Priority, so like first all the "VeryHighPriorityDeny" rules from all policies take effect, then all the "HighPriorityAllow" rules from all policies, then "MediumPriorityDeny", then "LowPriorityAllow", then "VeryLowPriorityDeny". Or vice versa. Nobody could agree on whether the highest priority should be "allow" or "deny". But anyway, that approach would avoid the "conflicting ANPs" problem because any two rules with the same priority would also have the same action and so could be evaluated in either order.)

(I'm not suggesting we should switch to that. I hope we can make priority work.)

[tssurya]

And I think there are good use cases for "two people create ANPs without coordination between them". Iif there is any scenario where it makes sense to include an ANP as part of a helm chart, third-party OLM operator, etc, to ensure some default set of permissions around a component, then you have to assume that eventually two components will ship ANPs with the same priority.

ok I found a use case for this :D (someone actually asked me this and that's how I found a use case for this); its useful to create two ANPs at same priority currently because we offer no support for tenancy currently, example I have:
tenant: coke and tenant: pepsi labels on coke(nsC,D) and pepsi (nsA,B) tenants. I want to simply express "allow to myself and deny to others". Today we'd need to do two different ANPs for this since subject is pretty much pinned here without tenancy and we don't have relational isSame,notSame peers anymore:

ANP1:

  subject:
     namespaces:
        tenant: coke
  peer:
    ingress:
        - allow
          namespaces:
             tenant: coke

ANP2:

  subject:
     namespaces:
        tenant: pepsi
  peer:
    ingress:
        - allow
          namespaces:
             tenant: pepsi

finally the default deny BANP:

  subject:
     namespaces: {}
  peer:
    ingress:
        - deny
          namespaces: {}

now if I have a 100 tenants, I can very easily just create that ANP at same prio they don't overlap at all..if not I will be wasting one prio per ANP which makes no sense.
So currently this is useful because its not like we can express subject: {} and allow to myself and deny to others at the moment, so users will be forced to do one ANP per tenant.

[tssurya]

and we error out

oh, you can't do that. Customers will complain. We tried it with a few things in openshift-sdn (like "you can't have more than one EgressNetworkPolicy object per Namespace and if you do, neither of them works") and that led to us having to figure out a better way to do it in ovn-k EgressFirewall ("an EgressFirewall object must be named 'default'")

ah good to know! glad we spoke before we GA-ed this :D I am considering removing this in OVNK and just supporting multiple ANPs at same prio but being very careful around wordings in docs like keep them non-overlapping for best results etc.

[nathanjsweet]

It's unacceptable to say that the behavior when you have two ANPs with the same priority is completely undefined.

Why can't we say that this is implementation-specific? AFAIK, here are some current, real vendor implementations:

• Error out (yes, customers might complain, but why isn't that the vendor's issue, why is it this SIG's responsibility?).
• Have further precedence logic that is documented.
• Have further precedence logic that is not documented.

I haven't seen a good argument in this discussion that pertains to API consistency only in this thread. There are plenty of edge cases and undocumented behavior in K8s netpol beyond this issue, is there more context that I'm not getting as to why this issue must must be consistent across vendors? Or do we just need more explicit documentation as to the different ways vendors will handle this problem?

[danwinship]

Why can't we say that this is implementation-specific? AFAIK, here are some current, real vendor implementations:

Those are vendor implementations of vendor APIs. And if users want to use those vendor APIs, they already can! They don't need anything from us.

Having a standard k8s API is only beneficial to users if it's standard.

There are plenty of edge cases and undocumented behavior in K8s netpol beyond this issue

I am trying to get rid of as many as possible of them in ANP. (This is one of the big reasons we don't yet have cluster ingress policy; because it's really unclear how to do that in a way that doesn't end up full of implementation-specific behavior.)

[aojea]

I want to reinforce what Dan says, API must define the semantics, not only the fields ...