Support AdminNetworkPolicy named ports #9254
Conversation
marvin-tigera
added
release-note-required
| if err != nil { | ||
| return | ||
| } | ||
| proto := kapiv1.ProtocolTCP |
There was a problem hiding this comment.
I think we only need to set this to make our parsing work, otherwise the protocol value does not matter. @fasaxc is this correct? There is no protocol field in the named port, and AFAIK, this is just name that will be matched with a named port.
There was a problem hiding this comment.
I don't think we should need a protocol for a named port. In NP there's an implicit default of TCP but there's no field in ANP so I think the idea is to match the named port, whatever protocol it uses. so, you could match "dns" and that will match pods that use TCP or UDP in their named port. so, I think you should set proto to nil and handle that further up the stack if needed.
There was a problem hiding this comment.
I'm not sure if the dataplane can handle named ports with no protocol match. Is an IP set port match allowed when there's no protocol specified?
We could work around that by generating a rule per protocol for UDP, TCP, SCTP but obviously that wouldn't be ideal.
| if err != nil { | ||
| return | ||
| } | ||
| proto := kapiv1.ProtocolTCP |
There was a problem hiding this comment.
I don't think we should need a protocol for a named port. In NP there's an implicit default of TCP but there's no field in ANP so I think the idea is to match the named port, whatever protocol it uses. so, you could match "dns" and that will match pods that use TCP or UDP in their named port. so, I think you should set proto to nil and handle that further up the stack if needed.
| if err != nil { | ||
| return | ||
| } | ||
| proto := kapiv1.ProtocolTCP |
There was a problem hiding this comment.
I'm not sure if the dataplane can handle named ports with no protocol match. Is an IP set port match allowed when there's no protocol specified?
We could work around that by generating a rule per protocol for UDP, TCP, SCTP but obviously that wouldn't be ideal.
Co-authored-by: Shaun Crampton <[email protected]>
| protocolPorts[""] = []numorstring.Port{*calicoPort} | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
@fasaxc Originally I handled it like this, but if a rule with both named ports and other types, this would generate a separate rule for named ports. Is this fine?
There was a problem hiding this comment.
I think that's the correct interpretation. Question is: does the dataplane handle that correctly (I'm not sure if you can match on ports without specifying a protocol so possible we fail or default it to TCP or something)?
There was a problem hiding this comment.
The conformance tests are passing, and those tests include named ports with both udp and tcp. From what I've seen, it seems the protocol does not matter for named ports. The underlying struct does not include a protocol value.
There was a problem hiding this comment.
It looks we default to TCP here:
calico/felix/calc/rule_scanner.go
Line 390 in d377c71
Also the conformance tests only include tcp protocols case. I think we should default to tcp here, and explicitly state that in our docs. WDYT?
mazdakn
changed the title
| // TODO: Add support for NamedPorts | ||
| if port.NamedPort != nil { | ||
| dstPort, err = k8sAdminPolicyNamedPortToCalico(*port.NamedPort) | ||
| proto := numorstring.ProtocolFromString(numorstring.ProtocolTCP) |
There was a problem hiding this comment.
We default to tcp in Felix for named ports.
mazdakn
added
docs-not-required
Description
Add support for AdminNetworkPolicy named ports.
We default to
tcpprotocol for named ports:calico/felix/calc/rule_scanner.go
Line 388 in 1e7b062
and we match it differently for named ports here:
calico/felix/labelindex/named_port_index.go
Line 122 in 1e7b062
Related issues/PRs
AdminNetworkPolicy core features: #9206
Todos
Release Note
Reminder for the reviewer
Make sure that this PR has the correct labels and milestone set.
Every PR needs one
docs-*label.docs-pr-required: This change requires a change to the documentation that has not been completed yet.docs-completed: This change has all necessary documentation completed.docs-not-required: This change has no user-facing impact and requires no docs.Every PR needs one
release-note-*label.release-note-required: This PR has user-facing changes. Most PRs should have this label.release-note-not-required: This PR has no user-facing changes.Other optional labels:
cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.