-
Notifications
You must be signed in to change notification settings - Fork 4.8k
NO-JIRA: Add e2e test to detect named ports in NetworkPolicies #31375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+88
−0
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| package networking | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "slices" | ||
| "strings" | ||
|
|
||
| g "github.com/onsi/ginkgo/v2" | ||
| o "github.com/onsi/gomega" | ||
|
|
||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/util/intstr" | ||
| e2e "k8s.io/kubernetes/test/e2e/framework" | ||
|
|
||
| exutil "github.com/openshift/origin/test/extended/util" | ||
| ) | ||
|
|
||
| var _ = g.Describe("[sig-network] NetworkPolicy", func() { | ||
| oc := exutil.NewCLIWithoutNamespace("networkpolicy-validation") | ||
|
|
||
| // Some CNIs like OVN-Kubernetes do not support named ports in NetworkPolicies. | ||
| // When a named port is used, they may silently convert the rule | ||
| // into an allow-all, effectively bypassing the intended restriction. | ||
| g.It("should use numeric ports in all platform NetworkPolicies [apigroup:networking.k8s.io]", func() { | ||
| // Known NetworkPolicies that still use named ports. | ||
| // Each entry is "namespace/name". | ||
| // TODO: remove entries as the owning teams fix them. | ||
| networkPoliciesToSkip := []string{ | ||
| // https://redhat.atlassian.net/browse/OCPBUGS-99494 | ||
| "openshift-ingress/router-default", | ||
| // https://redhat.atlassian.net/browse/OCPBUGS-99492 | ||
| "openshift-cluster-csi-drivers/allow-to-dns", | ||
| // https://redhat.atlassian.net/browse/OCPBUGS-99492 | ||
| "openshift-cluster-storage-operator/allow-to-dns", | ||
| // https://redhat.atlassian.net/browse/OCPBUGS-99493 | ||
| "openshift-cluster-olm-operator/allow-egress-to-openshift-dns", | ||
| // https://redhat.atlassian.net/browse/OCPBUGS-99493 | ||
| "openshift-operator-lifecycle-manager/catalog-operator", | ||
| // https://redhat.atlassian.net/browse/OCPBUGS-99493 | ||
| "openshift-operator-lifecycle-manager/olm-operator", | ||
| } | ||
|
|
||
| ctx := context.Background() | ||
|
|
||
| nps, err := oc.AdminKubeClient().NetworkingV1().NetworkPolicies("").List(ctx, metav1.ListOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to list NetworkPolicies across all namespaces") | ||
| o.Expect(nps.Items).NotTo(o.BeEmpty(), "expected at least one NetworkPolicy on the cluster") | ||
|
|
||
| var violations []string | ||
| for _, np := range nps.Items { | ||
| // Only check platform namespaces. This won't catch broken (on OVN-Kubernetes e.g.) NPs | ||
| // added by tests in ephemeral namespaces, but this will avoid false | ||
| // positives from tests that intentionally use named ports | ||
| // (e.g. verifying named-port flows or running on a CNI that | ||
| // supports them). | ||
| // For consistency, platform NetworkPolicies should use numeric ports across all CNIs. | ||
| if !strings.HasPrefix(np.Namespace, "openshift-") && !strings.HasPrefix(np.Namespace, "kube-") && np.Namespace != "default" { | ||
| continue | ||
| } | ||
| key := fmt.Sprintf("%s/%s", np.Namespace, np.Name) | ||
| skip := slices.Contains(networkPoliciesToSkip, key) | ||
| for _, rule := range np.Spec.Ingress { | ||
| for _, p := range rule.Ports { | ||
| if p.Port != nil && p.Port.Type != intstr.Int { | ||
| msg := fmt.Sprintf("%s ingress has named port %q (skip=%v)", key, p.Port.StrVal, skip) | ||
| e2e.Logf("%s", msg) | ||
| if !skip { | ||
| violations = append(violations, msg) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| for _, rule := range np.Spec.Egress { | ||
| for _, p := range rule.Ports { | ||
| if p.Port != nil && p.Port.Type != intstr.Int { | ||
| msg := fmt.Sprintf("%s egress has named port %q (skip=%v)", key, p.Port.StrVal, skip) | ||
| e2e.Logf("%s", msg) | ||
| if !skip { | ||
| violations = append(violations, msg) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| o.Expect(violations).To(o.BeEmpty(), "some NetworkPolicies use named ports instead of numeric") | ||
| }) | ||
| }) | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test name says "in all platform NetworkPolicies" but we collect all NPs across all namespaces. This will be intermittently misbehaving because while other tests are creating&deleting namespaces, we will be also sweeping through those.
Maybe it's better to only filter for
openshift-* / kube-*namespaces?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's safer, you're right.
Pushed a filter with a justification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if that's actually better so please confirm. But my gut feeling is that you could list the namespaces, get the ones you are interested in and only afterwards call
NetworkPolicies.Liston those namespaces only.Maybe that's better for apiserver, maybe not. Fell free to judge yourself and ignore this comment completely.
If anything, that would be something along the lines of
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe most of the NetworkPolicies will live in platform namespaces, and the overall NetworkPolicies footprint shouldn't pose a problem for either the API server or the client. Plus, it's a single query instead of N+1