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: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 🚥 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
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: machine424 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: f3bcd07
|
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.
| // Known NetworkPolicies that still use named ports. | ||
| // Each entry is "namespace/name". | ||
| // TODO: remove entries as the owning teams fix them. | ||
| networkPoliciesToSkip := []string{ |
|
Scheduling required tests: |
|
@machine424: The following tests failed, say
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. |
1 participant
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
NetworkPolicyresources to ensure ports are specified numerically.