[BUG] AdminNetworkPolicyIngressRule.Ports has two nil values?

[fasaxc]

While reviewing our WIP implementation of ANP, I noticed that the Ports field is defined as

	// Ports allows for matching traffic based on port and protocols.
	// This field is a list of ports which should be matched on
	// the pods selected for this policy i.e the subject of the policy.
	// So it matches on the destination port for the ingress traffic.
	// If Ports is not set then the rule does not filter traffic via port.
	//
	// Support: Core
	//
	// +optional
	// +kubebuilder:validation:MaxItems=100
	Ports *[]AdminNetworkPolicyPort `json:"ports,omitempty"`

which means that it has two/three "empty" values:

• (*[]AdminNetworkPolicyPort)(nil)
• &([]AdminNetworkPolicyPort(nil))
• &([]AdminNetworkPolicyPort{})

It's not clear what they're each supposed to mean, "If Ports is not set then the rule does not filter traffic via port". Clearly a nil pointer is "not set" but what about a pointer to an empty/nil slice? Would those mean "match no ports" (which in turn is a bit ambiguous since not all protocols have ports)?

I suggest dropping the extra pointer indirection and interpreting any zero-length slice (nil or otherwise) as "do not filter on ports". If we can't do that, we should at least document the meanings (where I think we should probably make them all mean "do not filter on ports").

[tssurya]

cc @astoycos

[tssurya]

We spoke about this in sig-network sync meeting today
Everyone was in favor of adding a minItems to port just like we have to peers:

network-policy-api/apis/v1alpha1/adminnetworkpolicy_types.go

Line 145 in 0054138

// +kubebuilder:validation:MinItems=1

But we need to check the KEP to ensure we didn't have a usecase for not doing minItems in ports maybe @Dyanngg or @astoycos know or remember?

1. empty list of ports today means match on nothing so "allow to 0 ports means ; allow to nothing"
2. nil ports today means match on L3 alone not L4

that is how we meant it in the API but this is not super intuitive in API docs and hard for implementations to figure out. We are not really sure if there is a usecase for (1) ever?

[danwinship]

For future reference, the rationale for the existing semantics is:

• nil/not-present must mean "this has no effect on matching". Eg, if we add Protocols in the future, then the behavior when you don't specify Protocols must be the same as what the behavior was in the version of ANP where that field didn't even exist. eg, "don't restrict based on ICMP". So by extension, not specifying Ports should mean "don't restrict based on port".
• For array-valued fields, we had decided that 0-length should not be treated as a special case with non-standard behavior; it should just be the obvious extension of the non-0-length behavior. An ANP whose Ports is a 3-element array matches 3 ports. An ANP whose Ports is a 2-element array matches 2 ports. An ANP whose Ports is a 1-element array matches 1 port. Therefore an ANP whose Ports is a 0-element array should match 0 ports. Note that this doesn't imply the existence of any use case for matching 0 ports, it's just that people felt that it was better for the semantics to be consistent, even if that meant it was possible to specify something that wasn't very useful.

So anyway, MinItems=1 is fine. People might come up with reasons why you would want to write "match 0 ports", but we didn't design that in intentionally. We just didn't want to special-case the empty list, and didn't consider the fact that we could just forbid it.