Add support for kubernetes endport field #1080
Conversation
|
@rikatz Thanks for thinking of us and submitting this! Definitely appreciate it! Give me a shout when you finish with the other checkboxes in there and want a review and I'll take another look. I was looking at the documentation for this feature (https://kubernetes.io/docs/concepts/services-networking/network-policies/#targeting-a-range-of-ports) and I can't tell if this is only for egress, or if |
|
Hey @aauren for sure. I'm getting some time during this weekend to finish here. EndPort works for both Ingress and Egress: It's part of the NetworkPolicyPort struct (used in both). In case of Ingress it's more feasible when working with Intra Pod communications (or headless) as services nowadays does not support portRanges (yet! there's a KEP ongoing about that). https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/2079-network-policy-port-range/README.md Here's the specification if you wanna take a look :) Thank you very much! |
|
@aauren I've added some unit tests, and tested the correct rule creation in Kubernetes as well. Everything seems to work fine, although I'm running inside KinD and it seems to be complaining about some things. I'll try to run on a pure kubeadm cluster, but if you can take a look into the PR I would appreciate. Thanks!! |
pkg/controllers/netpol/policy.go
Outdated
| } | ||
| } | ||
| portproto.protocol, portproto.port = protocol, npPort.Port.String() | ||
| numericPorts = append(numericPorts, portproto) | ||
| } else { | ||
| if protocol2eps, ok := namedPort2eps[npPort.Port.String()]; ok { | ||
| if numericPort2eps, ok := protocol2eps[string(*npPort.Protocol)]; ok { |
There was a problem hiding this comment.
Can you substitute out this conversion of string(*npPort.Protocol) to protocol like you refactored the others?
pkg/controllers/netpol/policy.go
Outdated
| } else { | ||
| // Per k8s api validation we should never reach this | ||
| // but if EndPort is < than Port we should skip | ||
| continue |
There was a problem hiding this comment.
With this, if endPort < Port then we don't add the port at all to the network policy rather than just resorting to the old functionality of only using the port. I would think that it would be better to resort to the old functionality of adding the Port and leaving off the endPort if it can't be validated.
Is this intended by the spec?
There was a problem hiding this comment.
Hum, agreed with you! Will change here. Actually the spec drops the netpol object in validation, when this situation happens so in this case this is a last resort here (we assume here that something wrong happened in k8s api validation).
|
BTW I was also able to test this on my test cluster and validated that the rules were created as we expected. |
|
Hey @aauren sorry for my long delay. I'll cycle back to this probably tomorrow :) |
|
@aauren fixed per your comments and already squashed things (as those are minor changes!) Sorry once more about the delay, things are pretty busy here :) |
This PR adds support for the recently added endPort field in kubernetes, which now is feature gated but will be marked as default in next release.
TODO: