NO-JIRA: Add e2e test to detect named ports in NetworkPolicies#31375
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
/pipeline required |
|
Scheduling required tests: |
WalkthroughAdds a Ginkgo test that lists all cluster NetworkPolicies, skips known exceptions, scans ingress and egress ports for named ports, and fails when violations are found. ChangesNetworkPolicy Port Validation Test
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Ginkgo
participant OpenShiftCLI
participant KubernetesAPI
participant PolicyValidator
Ginkgo->>OpenShiftCLI: create cluster-wide client
OpenShiftCLI->>KubernetesAPI: list NetworkPolicies across namespaces
KubernetesAPI-->>OpenShiftCLI: return NetworkPolicies
PolicyValidator->>PolicyValidator: filter platform namespaces and exceptions
PolicyValidator->>PolicyValidator: inspect ingress and egress port types
PolicyValidator->>Ginkgo: report violations
Ginkgo->>Ginkgo: assert violations are empty
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/extended/networking/networkpolicy_validation.go (2)
39-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTypo: "Sone CNI" → "Some CNI".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/networking/networkpolicy_validation.go` at line 39, Fix the typo in the comment inside the NetworkPolicy validation test by changing the wording in the nearby comment under the networking validation logic from “Sone CNI” to “Some CNI”; this is only a text correction and should be applied where the comment mentions named ports in NetworkPolicies.
43-46: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a bounded context instead of
context.Background().The
Listcall has no deadline, so it can hang indefinitely if the API server is slow or unresponsive.⏱️ Suggested fix
- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel()(remember to add
"time"to imports)As per path instructions, Go code should use
context.Context for cancellation and timeouts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/networking/networkpolicy_validation.go` around lines 43 - 46, The NetworkPolicies list call is using an unbounded context, so it can hang indefinitely if the API server is slow. Update the NetworkPolicy validation test to use a bounded context with timeout instead of `context.Background()` near the `List` call in the validation flow, and add the needed `time` import when creating that timeout-based context.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/extended/networking/networkpolicy_validation.go`:
- Line 39: Fix the typo in the comment inside the NetworkPolicy validation test
by changing the wording in the nearby comment under the networking validation
logic from “Sone CNI” to “Some CNI”; this is only a text correction and should
be applied where the comment mentions named ports in NetworkPolicies.
- Around line 43-46: The NetworkPolicies list call is using an unbounded
context, so it can hang indefinitely if the API server is slow. Update the
NetworkPolicy validation test to use a bounded context with timeout instead of
`context.Background()` near the `List` call in the validation flow, and add the
needed `time` import when creating that timeout-based context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 28d1c930-b729-45ed-80e8-b0124854a545
📒 Files selected for processing (1)
test/extended/networking/networkpolicy_validation.go
openshift-ci
Bot
added
the
ready-for-human-review
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: f3bcd07
|
|
Scheduling required tests: |
Some network plugins (e.g. OVN-Kubernetes) do not support named ports in NetworkPolicies and may silently convert them into allow-all rules. Add a test that lists all cluster NetworkPolicies and fails if any use named ports instead of numeric ones, with a skip list for now.
| nps, err := oc.AdminKubeClient().NetworkingV1().NetworkPolicies("").List(ctx, metav1.ListOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to list NetworkPolicies across all namespaces") |
There was a problem hiding this comment.
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.
that's safer, you're right.
Pushed a filter with a justification.
There was a problem hiding this comment.
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.List on 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
+ namespaces, err := oc.AdminKubeClient().CoreV1().Namespaces().List(ctx, metav1.ListOptions{})
+ var networkPolicies []networkingv1.NetworkPolicy
+ for _, namespace := range namespaces.Items {
+ name := namespace.Name
+ if !strings.HasPrefix(name, "openshift-") &&
+ !strings.HasPrefix(name, "kube-") &&
+ name != "default" {
+ continue
+ }
+ nps, err := oc.AdminKubeClient().
+ NetworkingV1().
+ NetworkPolicies(name).
+ List(ctx, metav1.ListOptions{})
+ )
+
+ networkPolicies = append(networkPolicies, nps.Items...)
+ }
There was a problem hiding this comment.
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
|
/approve |
openshift-ci
Bot
added
the
approved
|
Scheduling required tests: |
|
/retitle NO-JIRA: Add e2e test to detect named ports in NetworkPolicies |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@machine424: This pull request explicitly references no jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
|
/verified by #31375 (comment) |
openshift-ci-robot
added
the
verified
|
@machine424: This PR has been marked as verified by DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: machine424, mkowalski The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@machine424: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 14a8058
New tests seen in this PR at sha: 14a8058
|
3 participants
Some network plugins (e.g. OVN-Kubernetes) do not support named ports in NetworkPolicies and may silently convert them into allow-all rules. Add a test that lists all cluster NetworkPolicies and fails if any use named ports instead of numeric ones, with a skip list for now.
Summary by CodeRabbit
networking.k8s.io/v1NetworkPolicy resources to ensure rule ports are specified numerically.