npep-187: More protocols support

[npinaeva]

Issue: #187
Also fixes #247 - no more pointers to lists
We have a plan for #340 so this npep can move on

Option3 has won the design discussion.

protocols:
  - tcp:
      destinationPort:
        number: 8080
      flags: [syn] # future extension example
  - tcp:
      destinationPort:
        range: 
          start: 8080
          end: 9090
  - udp:
      destinationPort:
        number: 8080
  - udp:
      destinationPort:
        number: 9090
  - namedPort: http
  - namedPort: monitoring
  - icmp: # that doesn't exist yet, but may be added
      type: 7
      code: 3

[netlify]

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

Name	Link
🔨 Latest commit	940a1f4
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-network-policy-api/deploys/6936edea428436000876d65d
😎 Deploy Preview	https://deploy-preview-297--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]

High level looks ok to me plus aligns with what was discussed.

/lgtm

[bowei]

Clarification: this is having just protocols: but no list?

[danwinship]

This needs to work the same way as whatever we eventually decided for ports.

[npinaeva]

it should be a list, minItems=1, so you can't define an empty list and empty list should work the same as unset

[bowei]

I think we were thinking to not have namedPort for the first pass?

[danwinship]

I think we were thinking to not have namedPort for the first pass?

We should at least account for it in the design.

(Hm... are we going to have "experimental" fields in v1beta1? That seems kind of contradictory...)

[npinaeva]

I think we just agreed no one likes them, but we can't get rid of them?

[bowei]

How do we make progress on this?

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[npinaeva]

/remove-lifecycle stale
/lifecycle freeze

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: npinaeva

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [npinaeva]

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

[fasaxc]

Feels like a type mismatch that namedPorts is a peer of tcp; how about any for this case; it makes it explicit that named ports bring their own protocol with them:

Suggested change

	namedPorts:
	- http
	- monitoring
	any:
	- namedPort: http
	- namedPort: monitoring

also opens up options like

  any:
  - all: true 

to explicitly match absolutely anything

[npinaeva]

named port is a protocol+port reference, so it kinda makes sense to me to be on the same level as tcp?
I guess I would agree with any if we had more cases like named ports in mind (but I do hope there are none :D)

[fasaxc]

Once it becomes an either/or, don't we need a way to say "match anything at all", so I think we'd need something like

  any:
  - all: true 

[fasaxc]

We should have an explicit list of use cases to test each of these approaches against:

• Rule that matches all traffic (how do you write your "default deny" for the baseline tier, or the end of your Admin tier if your policy set needs that)
• Rule that allows DNS to TCP and UDP port 53 (Tim's example of a common need)
• Rule that allows TCP port 80, 8080, 443
• Rule that allows dozens of ports (I've seen this with some customers monolith apps, good to make sure it doesn't get awful)
• Rule that allows a range of UDP ports (e.g. for telephony use cases).

[npinaeva]

We are talking about protocols field, so

  any:
  - all: true

is the same as not having a protocol match at all? Same way, it just should not be set for "deny all"?

[danwinship]

"any all" is confusing. Also, what would this mean:

any:
- all: false

[danwinship]

I think we were thinking to not have namedPort for the first pass?

We should at least account for it in the design.

(Hm... are we going to have "experimental" fields in v1beta1? That seems kind of contradictory...)

[danwinship]

So this loses the possibility of defaulting the protocol, and thus makes the most common case ("allow to specific TCP ports") more complicated. This is not awful, but it's a data point.

The fact that you can specify multiple ports under tcp helps with that though.

(EDIT: oh, you make both of these points later)

[thockin]

IMO, defaulting of port is sort of wart, anyway. It kind of hurts readability to leave it out.

[bowei]

In the era of generated configuration, I think trying to make configuration extremely concise seems to be less critical IMO

[danwinship]

maybe ports?

[npinaeva]

so you mean convert this to list? good point to discuss when we decide on the option (I think it only applies to option2) , but also we need to consider if having a nested list is a good idea (it will require more maxItems and more some extra validations)

[danwinship]

No, I just meant rename portRange to ports to make it a little simpler, since the start and end makes it clear it's a "range" anyway. (I guess this is another briefness/clarity trade-off.)

[npinaeva]

ah well I guess if I was confused by the name, others will be too :D
I kinda like portRange it is very clear and not too long?

[thockin]

If port is int, I would expect ports to be []int. I would stay with portRange if the goal is to elide the extra nesting level.

THAT SAID...

We generally want one-of groups to exist in a struct by themselves, if we can. Something like:

type PortSpec struct {
    // exactly one of these must be set

    // +optional
    Port *int

    // +optional
    PortRange *PortRange

    //... maybe more over time ...
}

The way this is described in YAML would be:

type Protocols struct {
    TCP []TCPPortSpec
     // ...
}

type TCPPortSpect struct {
    // exactly one of these must be set

    // +optional
    Port *int

    // +optional
    PortRange *PortRange

    //... maybe more over time ...
}

Are we 100% sure that we will never want to express something about the TCP protocol that doesn't fit into a repeated port-spec? E.g. "all" doesn't really fit. Might we ever want to express something like a timeout config or something which applies across "all ports in a proto" or "all ICMP codes"? If so, you need the extra nesting, and even if we can confidently say we will never need that, I bet we are wrong :)

[npinaeva]

You could add allow-all with one-of like

type TCPSpec struct {
    // exactly one of these must be set

    // +optional
    Port *int

    // +optional
    PortRange *PortRange

    // +optional
    AllowAll *bool
}

?
If we need to do "block port=80 AND flag(PSH)" I assume we would have to remove the one-of validation and add more CEL to restrict compatible fields.
So you wouldn't do

type TCPPortAndFlag {
   Port int
   Flag int
}

type TCPSpec struct {
    // exactly one of these must be set
    // +optional
    Port *int
    // +optional
    PortRange *PortRange
    // +optional
    TCPPortAndFlag *TCPPortAndFlag
}

you would probably do

// CEL: Port and PortRange can't be set at the same time
type TCPSpec struct {
    // +optional
    Port *int
    // +optional
    PortRange *PortRange
    // +optional
    Flag *int
}

?

[thockin]

We're trying to find more declarative ways to express validation with less traps. Your allowAll means I am able to specify:

tcp:
  - portNumber: 80
  - allowAll: true
  - portNumber: 443
  - allowAll: false

What does it mean? Should allowAll: false even be allowed? It is a *bool so violates the "exactly one" rule (another reason bool is frowned on).

Doing what you suggest for AND means that a) validation is much more complex to reason about (see service API) and b) a back-rev client might see portNumber: 80, flags: [ PSH ] but not know about flags, and so interpret it as just portNumber: 80 -- potentially failing open.

[npinaeva]

well allowAll is a wrong field name, it should be called something like any and only be allowed to set to true. Even better to make it non-bool, like match: all, where all is the only supported value for now (which is also not a good field name, but I don't have anything better from the top of my head)
It could be nice to make sure protocol filters don't intersect, but it may be hard to validate, since we have never had a way to make sure

tcp:
  - port: 5
  - portRange:
      start: 1
      end: 10

don't intersect. And the behaviour is well-defined: it is OR-ed, so anything that has at least one match: all match is equivalent to having only that match.
So in your example (considering allowAll: false is not possible) the result is equivalent to simply allow all.

The compatibility with the older clients is a "well-known" problem of this API as discussed here #297 (comment) (which could affect the whole design, but it is kinda separate discussion)
Extra validaiton is needed, as I mentioned in the example, but we need to compare its complexity (which is not too high) with the disadvantages of the alternative solutions. Not sure if you thinks that TCPPortAndFlag is better or if you have other ideas?

[thockin]

so anything that has at least one match: all match is equivalent to having only that match

That works but is a little awkward, IMO. At the bottom it is a data normalization exercise.

Extra validaiton is needed, as I mentioned in the example, but we need to compare its complexity (which is not too high) with the disadvantages of the alternative solutions.

I am trying to drive validation away from "here's some code" and into "here's the schema". In that regard...

Not sure if you thinks that TCPPortAndFlag is better or if you have other ideas?

...this is superior. Schematically it is an alternative to TCPPort and less likely to be mistaken

[npinaeva]

I am not sure how this strategy should work with more fields, AFAIU every struct have to have a one-of limitation and add all possible fields combinations as separate fields? See Option3 that I added on the enhancement and lmk if I am missing something

[danwinship]

"any all" is confusing. Also, what would this mean:

any:
- all: false

[danwinship]

This needs to work the same way as whatever we eventually decided for ports.

[danwinship]

i think a lot of people want to block pings but not host unreachable

[npinaeva]

I think all use cases I have heard of only want to allow ICMP(v6), but if someone does want to drop it, more options make sense

[bowei]

I think v6 would be icmp6. I'm looking at the codes and type and they don't line up:

• https://en.wikipedia.org/wiki/Internet_Control_Message_Protocol
• https://en.wikipedia.org/wiki/ICMPv6

[fasaxc]

i think a lot of people want to block pings but not host unreachable

Not that we need to figure it out now, but ICMP host unreachable is typically a response packet to an outgoing UDP/TCP packet. It's best handled as part of the UDP/TCP session from a firewall point of view (I think this is what Linux conntrack does, for example).

[danwinship]

yeah, conntrack will mark it as being related to the UDP/TCP connection; I talked about this a little in the allow/deny all bug too (that "deny all" should probably not deny related ICMP replies by default)

[thockin]

IMO, defaulting of port is sort of wart, anyway. It kind of hurts readability to leave it out.

[thockin]

This threw me for a minute - not having a protocol here is surprising. Of course a given pod has exactly 1 instance of a named port and that includes protocol, so this is strictly correct, but it is a) a little clunky; and b ) REALLY clunky under a struct named "protocols".

[bowei]

+1 to this comment, I think this definitely will confuse users.

The presence of namedPort kind of pushes us to have Port-like things by themselves.

[danwinship]

@thockin note that NetworkPolicy allows you to specify:

ports:
- protocol: TCP
  port: dns-udp

(which can't be ruled out at validation time)

and we're trying to avoid that here...

[thockin]

Understood, it's still awkward.

[thockin]

If port is int, I would expect ports to be []int. I would stay with portRange if the goal is to elide the extra nesting level.

THAT SAID...

We generally want one-of groups to exist in a struct by themselves, if we can. Something like:

type PortSpec struct {
    // exactly one of these must be set

    // +optional
    Port *int

    // +optional
    PortRange *PortRange

    //... maybe more over time ...
}

The way this is described in YAML would be:

type Protocols struct {
    TCP []TCPPortSpec
     // ...
}

type TCPPortSpect struct {
    // exactly one of these must be set

    // +optional
    Port *int

    // +optional
    PortRange *PortRange

    //... maybe more over time ...
}

Are we 100% sure that we will never want to express something about the TCP protocol that doesn't fit into a repeated port-spec? E.g. "all" doesn't really fit. Might we ever want to express something like a timeout config or something which applies across "all ports in a proto" or "all ICMP codes"? If so, you need the extra nesting, and even if we can confidently say we will never need that, I bet we are wrong :)

[thockin]

or the one you considered and evolved past:

protocols:
  tcp:
    ports:
      - number: 53
  udp:
    ports:
      - number: 53

What none of these do is make some forms of evolution more visible. NOTE - I am not saying we should do this but it's worth considering.

Suppose one day we add GRE support. The API understands an object with gre specified but we have a back-rev client which does not. The client will deserialize the gre field and discard it. One approach to defending that is to elevate the list, like:

type Protocols {
    // exactly one should be set, if you see this empty it's somethign you don't know how to handle

    TCP *TCPProtocol
    UDP *UDPProtocol
    GRE *GREProtocol
}

Then rather than the parent having a single Protocols Protocols it would have a list: Protocols []Protocols. Rendering as YAML like:

protocols
  - tcp:
    ports:
      - number : 53
  - udp:
    ports:
      - number: 53
  - gre:
    somethingHere: true

and an older client would see:

protocols
  - tcp:
    ports:
      - number : 53
  - udp:
    ports:
      - number: 53
  - {}  // <- something is obviously wrong!

Again, not sure we should do it, but it's a pattern used in some other places like DRA, where evolution is highly likely. I am not sure it matters here, and anyway it does not handle other kinds of new-field evolutions.

[bowei]

Do we consistently consider the old clients issue? I think inconsistent application of this rule doesn't give us much in terms of guarantees? Specifically, we should define what a client should do when they get something invalid. In the case above, this would be an API object that seemingly violates validation rules that is being sent be the server -- which would unexpected...

[thockin]

This is not a rule, it's an opportunity for indicating something went wrong. As you point out, an empty struct have failed at the server, so clearly there's something here but I don't understand it. Compare to the model where we just drop an unknown field, there might be other fields which I do know so I can't tell that you dropped an unknown one.

This matters most in places where we are likely to add more options over time, and where clients really need to know that they don't know something.

The reason I bring it up in this context is to ask, what should an implementation of CNP do if it finds a. "Protocol) that it doesn't understand? How dangerous is it to get that wrong?

[danwinship]

Do we consistently consider the old clients issue?

We try to (eg #252).

This matters most in places where we are likely to add more options over time, and where clients really need to know that they don't know something.

Note that (as we eventually decided in the PR above), this does not currently apply to ANP/CNP, because the implementation will be installing the CRD itself, so it should know all of the fields that exist in the CRD (though if they're installing the experimental CRD, they might not implement every field in the CRD, but in that case they are responsible for noticing that one of the fields they don't implement is set).

How dangerous is it to get that wrong?

Per the PR above, if the goal is to ensure that no traffic that is supposed to be denied gets accepted instead, then:

• If there's something you don't recognize in an action: Accept rule, then you must assume that the rule does not match anything (ie, it neither Accepts nor Denies, but allows further rule processing).
• If there's something you don't recognize in an action: Deny rule, then you must assume that the rule matches everything (so, "deny all").
• If there's something you don't recognize in an action: Pass rule, then theoretically you could split evaluation in two and follow both paths, but we currently recommend you just treat it as "deny all" as in the previous case.

Of course this is still "dangerous" in the "could totally break your cluster by denying too much" sense, just not in the "accidentally allows forbidden traffic" sense.

[npinaeva]

I added a section on this to not lose the nice examples.
So if we can't recognize that an unknown protocol is set (so it is just omitted), then we also can't implement the second rule, so something that should have been denied won't be.
Is it a good enough reason to add this requirement? I have added a third option that takes care of the old clients, let's discuss if it makes sense.
Basically supporting old clients means you can only have one-of types of structs, and no combination of fields, which is a pretty strong limitation on the API and makes it somewhat ugly

[danwinship]

If we assume it will always be the case that the NP impl installs the CRDs by itself, then we can argue that it's impossible for there to be unrecognized fields. But I feel like that's a fragile assumption and it would probably be better for us to design the APIs in such a way that unrecognized fields won't mess us up. (see also kubernetes-sigs/gateway-api#3624)

[npinaeva]

I did put a word "misconfiguration" in there, so should we claim it as such in more details? Saying that we don't think it is reasonable to run older clients with newer APIs, and especially using newer API fields?
Should we try to join forces with gateway api (as it is a common problem) but do not design the API based on this requirement?

[thockin]

I think it could be reasonable to have different parts of AND implemented in different ways. Regardless, any time we make assumptions about "this can't happen" we end up regretting it -- if it is possible to be correct by construction, that's almost always better.

[bowei]

I think the best policy for a controller when it encounters configuration it can't understand is to fail static and alert. This is already a failure scenario that needs to be handled (control plane outage) and we don't have to make guesses about future features that don't yet exist...

[bowei]

+1 to this comment, I think this definitely will confuse users.

The presence of namedPort kind of pushes us to have Port-like things by themselves.

[bowei]

Do we consistently consider the old clients issue? I think inconsistent application of this rule doesn't give us much in terms of guarantees? Specifically, we should define what a client should do when they get something invalid. In the case above, this would be an API object that seemingly violates validation rules that is being sent be the server -- which would unexpected...

[bowei]

Is icmp: {} an option (was discussed on the Slack thread).

This comment is meant for Option 2 (below)

[thockin]

Using an empty struct to represent something means that you can never ever add another way of selecting. This is the same problem with old clients. If a new user specifies one of the newer selection modes, and an older implementation reads it and drops that field, it will assume that means "all", which, for Network policy, could mean failing open.

It's better for "all" to be an explicit thing.

In an ideal world, you could perhaps make "all" be the default, applied by the server. If nothing else is specified. Then clients are not subject to the empty structure. That requires defaulting logic to look at multiple Fields, which isn't very well supported right now, unless you hand code the defaulting.

[npinaeva]

I am not even sure the defaulting is a good idea, how do I distinguish "I went to have some coffee and forgot to finish writing a rule" vs "I wanted to deny/allow ALL".

I don't see a single advantage of allowing icmp: {}, it is only saving one line?

[bowei]

Yes, I agree! Having an explicit enum type field description is IMO the most explicit and least error prone path.

In general, I think we should bias towards being more consistent and easy to understand with less emphasis on conciseness because most users will be generating their configuration using an (AI) tool.

[bowei]

I think v6 would be icmp6. I'm looking at the codes and type and they don't line up:

• https://en.wikipedia.org/wiki/Internet_Control_Message_Protocol
• https://en.wikipedia.org/wiki/ICMPv6

[fasaxc]

Doesn't this break our "exactly one field set" rule? A back-level client that doesn't know about icmp would see

protocols:
  tcp: ...
  icmp: ...

as

protocols:
  tcp:

and do the wrong thing (unless we have another plan for how to prevent that).

Should this just be "protocol" and allow only one protocol per rule (at cost of a little verbosity for the TCP+UDP case)

[danwinship]

(unless we have another plan for how to prevent that)

we do! Nadia talked to Jordan and he came up with a pretty easy way for all CNP clients to detect when fields have gotten dropped (which we need to document now...)

[danwinship]

we made a last-second decision in Atlanta to change this to destinationPort

[npinaeva]

huh true, let me update

[npinaeva]

should we do destinationNamedPort then? i guess not, but it kinda also applies :)

[danwinship]

Option2: Join by protocol has won the design discussion.

As discussed in the last meeting, this ends up being slightly weird because it means the fields of the protocols struct have "OR" semantics rather than "AND" semantics.

Option 3 fixes this, but we were only talking about option 3 as being the "better compatibility for future changes" option, and we eventually decided we didn't need to worry about that, so we discarded 3 in favor of 2. But I don't think we ever noticed that 2 had kinda weird semantics. Given that, I don't think there was strong opposition to 3 over 2.

[npinaeva]

@danwinship @bowei I agree that option 3 makes more sense, update the proposal and yaml, ptal

[bowei]

Looks ok to me

[bowei]

/lgtm

[danwinship]

belated lgtm

[bowei]

#347