[BUG] ANP description disables KNP denies on upgrade

[matthewdupre]

I created this PR (kubernetes/enhancements#4688) against the KEP repo that's apparently not used and there was some discussion so I figured I'd open this here to clarify what the intended specification is before I raise a PR against this repo.

Example
To make this more concrete, let's imagine a scenario with the following three pods, which I will abbreviate later as R, G and B:

kind: Pod
metadata:
  name: red-pod
  labels:
    color: red

kind: Pod
metadata:
  name: green-pod
  labels:
    color: green

kind: Pod
metadata:
  name: blue-pod
  labels:
    color: blue

And the following Kubernetes NetworkPolicy:

apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
  name: red-only-from-green
spec:
  podSelector:
    matchLabels:
      color: red
  policyTypes:
  - Ingress
  ingress:
  - from:
    - podSelector:
        matchLabels:
          color: green

Current (pre-ANP/BANP) behavior:
The current behavior of this policy is that G->R traffic is allowed and B->R traffic is denied. Traffic to the B or G pods is allowed.
G->R Allowed explicitly by KNP
B->R Denied because R has a KNP that polices ingress traffic and no KNP allowed the traffic
*->B Allowed by absence of KNP and BANP
*->G Allowed by absence of KNP and BANP

What should happen when upgrading to a BANP capable implementation but not creating any BANPs:
Surely the intention here is that the behavior doesn't change, even if the reason may change to account for the new capabilities.
G->R Allowed
B->R Denied
*->B Allowed
*->G Allowed

What does the spec actually say?:
The only User Story in the site for BaselineAdminNetworkPolicy is number 5. This story fully defines what happens when there's a Deny BANP, but doesn't say anything about when there are no BANPs (or only BANPs that select pods other than R, G and B).

The BANP Resource section (https://network-policy-api.sigs.k8s.io/api-overview/#the-baselineadminnetworkpolicy-resource) implies that the behavior shouldn't change.

However, the only concrete description of what should happen to a specific connection after the KNP stage is found in the Pass section of the ANP actions (https://network-policy-api.sigs.k8s.io/api-overview/#adminnetworkpolicy-actions). It states that "If there is no K8s NetworkPolicy of BaselineAdminNetworkPolicy rule match traffic will be governed by the implementation".

Governed by the implementation typically means allowed, so this line states that in our situation all traffic that does not have a "rule match" will be allowed. In this example, that means that the behavior changes to:
G->R Allowed explicitly by KNP
B->R Allowed because this traffic does not match any rule in the KNP
*->B Allowed by absence of KNP and BANP
*->G Allowed by absence of KNP and BANP

Since KNPs only have Allow rules, the distinction between a "rule match" and "the pod is selected by at least one KNP that polices the specific ingress/egress direction in question" becomes important. I don't really expect anyone to implement this as a complete bypass of KNP, but it may confuse users later on.

I'd like to change this wording so it doesn't talk about rule matches. Assuming the purpose of a BANP is to force developers to create KNPs covering every pod, I think it makes sense to continue allowing KNPs to deny traffic (in this example B->R), and the way to do that would be to have traffic only pass to the BANPs if the source (egress) or destination (ingress) pod is not selected by any (egress / ingress respectively) KNP and not Allowed or Denied by an ANP.

[Dyanngg]

@matthewdupre Thanks for bringing up the issue and providing the exhaustive writeup. My thoughts:

1. Agreed that the wording needs to be improved. On the other hand, I would like to clarify that the intention of this API is during upgrade, the original behavior of KNPs should NOT change, meaning that B->R should still be denied given that it falls under the "implicit isolation" umbrella, which means, an implicit rule created by the original KNP that says, "for any traffic coming to R, if it's not G, deny it". I would think this is what we originally meant by "rule match", which is totally not ideal.
2. We've long been trying to add "deny" mechanism to the KNP as a subgroup, however it is already a v1 API and we're not supposed to make such disruptive changes. This is why we have a NetworkPolicy v2 initiative, and you can find more info here.
3. Back to your original proposal(KEP-2091: Do not pass K8s NetworkPolicy implicit denies to BaselineAdminNetworkPolicy kubernetes/enhancements#4688),

 If there is no K8s NetworkPolicy rule match, and no BaselineAdminNetworkPolicy rule match
(more on this in the [priority section](#priority)), traffic will be governed by the implementation.

I’d say s/rule match/selecting the pod/g is probably not enough for complete clarification. In KNP, pod selected in spec.podSelector and selected as an ingress/egress peer have different implications. As you pointed out in the writeup, the former puts some “isolation effect” on the pod, while the later does not. So in essence, if we consider a connection from G -> R, it is regulated by a KNP if at least one of the two following applies: 1) a KNP selects R in spec.podSelector and it has ingress rules 2) a KNP selects G in spec.podSelector and it has egress rules. If any of these KNPs are there, they already dictates if the traffic will be allowed/dropped. BANP won’t even be evaluated unless none of the above two scenarios apply. I believe we need to somehow convey this in the doc.