NetworkPolicy validation improvements #39769
Conversation
|
This change is |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-github-robot
added
size/L
| allErrs = append(allErrs, field.Invalid(portPath.Child("port"), port.Port.IntVal, msg)) | ||
| } | ||
| } else { | ||
| if _, err := net.LookupPort(strings.ToLower(protocol), port.Port.StrVal); err != nil { |
There was a problem hiding this comment.
String portnames actually mean to look up that named port on Pod. Yes, it means ports could vary across different pods.
danwinship
force-pushed
the
networkpolicy-validation
branch
from
737b129 to
97d8ae0
Compare
Lovely. As far as I can tell, none of Calico, Romana, or Weave implement this so I guess I don't have to feel bad if I don't either... |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
k8s-github-robot
added
the
do-not-merge
|
Jenkins Bazel Build failed for commit 97d8ae016126633767d7f34e661559b371fd75a5. Full PR test history. cc @danwinship The magic incantation to run this job again is 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/test-infra repository. I understand the commands that are listed here. |
|
Can someone add "release-note-none"? |
Paths were wrong for most errors. Field name was wrong for namespaceSelector.
Protocol must be "TCP", "UDP", or nil. Integer-valued port must be 1-65535. String-valued port must be a syntactically valid ContainerPort name.
danwinship
force-pushed
the
networkpolicy-validation
branch
from
97d8ae0 to
1f6735c
Compare
|
(pushed fix for s/api.ObjectMeta/metav1.ObjectMeta/ in master) |
k8s-github-robot
removed
the
lgtm
|
Jenkins GCI GKE smoke e2e failed for commit 1f6735c. Full PR test history. cc @danwinship The magic incantation to run this job again is 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/test-infra repository. I understand the commands that are listed here. |
|
Jenkins GKE smoke e2e failed for commit 1f6735c. Full PR test history. cc @danwinship The magic incantation to run this job again is 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/test-infra repository. I understand the commands that are listed here. |
|
/lgtm @grodrigues3 @apelisse What do you think about making the bot look at people already on the thread instead of spamming a new person? E.g. it would have made more sense for the bot to bug me than Clayton. Or maybe this isn't a concern in steady state? |
k8s-ci-robot
added
the
lgtm
|
@grodrigues3 is working on improving the algorithm right now, that's a good suggestion, though it might not be easy at first. Also please note that we have this bug open kubernetes/test-infra#1672 preventing you from doing /lgtm and /approve in the same comment for now (I'm working on it as we speak) |
k8s-github-robot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED The following people have approved this PR: lavalamp Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/release-note-none |
|
@eparis: you can only set release notes if you are the author or an assignee. In response to this comment:
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/test-infra repository. I understand the commands that are listed here. |
eparis
added
the
release-note-none
eparis
removed
release-note-label-needed
do-not-merge
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue (batch tested with PRs 40497, 39769, 40554, 40569, 40597) |
I noticed while implementing NetworkPolicy that I we weren't validating the "Ports" field at all.
The docs are actually completely silent about what a string-valued Port field is supposed to mean. I had guessed it meant to call
net.LookupPort()on it (ie, map it from /etc/services) but in every other case where we have an IntOrString-valued Port field in an API struct, it refers to a named ContainerPort. But that would be extremely awkward to implement in this case; a policy specifying a named port could end up mapping to a different numeric port on every container in the namespace... Do other people actually implement string-valued ports that way? Or, for that matter, implement string-valued ports at all? (Related: I hadn't noticed until now that you can leave the Port value unspecified, allowing you to say "allow to all UDP ports, but no TCP ports" or "allow to all TCP ports, but no UDP ports". That seems like something that ended up in the spec just because it was possible, not because it was actually useful...)@kubernetes/sig-network-misc