Update ports to protocols

[bowei]

Makes the ports clause a more generic protocols block to allow for future expansion.

Example

apiVersion: policy.networking.k8s.io/v1alpha2
kind: ClusterNetworkPolicy
metadata:
  name: pub-svc-delegate-example
spec:
  tier: Admin
  priority: 20
  subject:
    namespaces: {}
  egress:
  - action: Pass
    to:
    - pods:
        namespaceSelector:
          matchLabels:
            kubernetes.io/metadata.name: bar-ns-1
        podSelector:
          matchLabels:
            app: svc-pub
    protocols:            #<
    - protocol: TCP       #<
      port:               #<
        number: 8080      #<

Another example:

protocols:
- protocol: TCP
  port:
    range:
      start: 1000
      end: 2000
- protocol: UDP
  port:
    number: 53

Ref: #187

[netlify]

✅ Deploy Preview for kubernetes-sigs-network-policy-api ready!

Name	Link
🔨 Latest commit	10bc1ba
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-network-policy-api/deploys/68f272354facf7000826d391
😎 Deploy Preview	https://deploy-preview-326--kubernetes-sigs-network-policy-api.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[bowei]

@tssurya @npinaeva

[aojea]

is Protocol defaulted if Port is nil? I'm confused on how this will work since Port is optional

[bowei]

See CEL validation : TCP => need to set port.

[bowei]

I'm assuming the order will be default and then validate, but have to check.

[npinaeva]

nice!
One difference form the "perfect yaml" we came up with last time is one extra layer, so you have

protocol: TCP
port:
  range:
    start: 1000
    end: 2000

instead of

protocol: TCP
portRange:
  start: 1000
  end: 2000

is there any special reason for it? I think we wanted to have new embedded fields for non-port-based protocols, like

protocol: ICMP
icmp:
  type: 7
  code: 3

[bowei]

I was looking at some of the other Kubernetes APIs -- and this nesting seems to be more natural, as we are using the string prefix as a grouping instead of nesting, which is more explicit. From a future-proofing perspective and validation perspective, it feels like nesting might be the right choice:

Here is what each variant would look like, it doesn't look that bad?

protocol: TCP
port:
  number: 80

protocol: UDP
port:
  range:
    start: 1000
    end :1100

protocol: TCP
port:
  name: www

protocol: ICMP
icmp:
  type: 7
  code: 3

I'm happy to change it to the other one -- maybe let's quickly discuss this in the meeting to see what others think.

[danwinship]

The goal from the KubeCon notes was "Make TCP and UDP easy to write and easy to read. Make ICMP and named ports possible."

So

protocols:
  - port: 80

is nice because it's very easy for the user. But as we also said in the KubeCon notes, it "[makes] the structs more complicated to make the YAML nicer", and it will make it really hard to understand the Protocols struct because there will be a whole bunch of fields, some of which can be specified together and some of which can't.

I feel like people use ports too much in NetworkPolicies anyway, so I don't really object to making them more annoying to use. 🙂

[danwinship]

Hm... this preserves the existing "bug" where you can specify

protocols:
  - protocol: TCP
    port:
      name: foo

where the "TCP" is either redundant or incorrect. Named ports should not have an explicitly-specified protocol.

[bowei]

I think (!(has(self.Port) && has(self.Port.Name)) || self.Protocol == '') would cover this case?

There is a slight problem in that we can only generate this validation for experimental fields.

[fasaxc]

I can live with either

protocol: TCP
port: 80

or

protocol: TCP
port: 
  number: 80

It's nice that the latter doesn't have several fields with same prefix, which would be a bit of a smell. But, it has the downside that it takes three lines of YAML to specify a single port instead of two.

[danwinship]

three lines of YAML to specify a single port instead of two.

protocol: TCP is the default, so in the common case it's 2-rather-than-1, not 3-rather-than-2

[fasaxc]

protocol: TCP is the default

Did we decide that in the end, I know it's the case for NetPol but ISTR that we wanted explicit for A/CNP

[danwinship]

the notes from kubecon say

protocol defaults to TCP, except with named ports, where it defaults to “”

which I guess addresses my comment above about named ports too... (but the validation and defaulting in this patch need to be updated if that's what we're going with)

[bowei]

Sounds like people are mostly ok with either choice, in which case, my recommendation is to go with the one proposed here and then people can play around with it to figure out if there is a significant drawback we haven't thought of?

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bowei
Once this PR has been reviewed and has the lgtm label, please assign tssurya for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

@bowei: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-network-policy-api-crd-e2e	10bc1ba	link	true	/test pull-network-policy-api-crd-e2e
pull-network-policy-api-verify	10bc1ba	link	true	/test pull-network-policy-api-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.