OCPBUGS-84036: CNTRLPLANE-2751:Adding e2e network policy cases for config-operator#30995
Merged
openshift-merge-bot[bot] merged 2 commits intoopenshift:mainfrom Apr 21, 2026
Merged
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new extended E2E test suite that validates OpenShift Config Operator NetworkPolicy creation, enforcement, DNS egress behavior, and reconciliation; an OWNERS file for the new tests; and registers the suite via a blank import in the extended tests package. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 7 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/networking/config_operator_networkpolicy.go`:
- Around line 153-191: The test currently only logs NetworkPolicies for
namespaces in namespacesToTest and allows empty results, so it can pass without
verifying reconciliation; update the loop (referencing namespacesToTest and the
call cs.NetworkingV1().NetworkPolicies(ns).List and logNetworkPolicyDetails) to
assert that the target namespaces "openshift-config" and
"openshift-config-managed" contain at least one NetworkPolicy (use
o.Expect(len(policies.Items)).To(o.BeNumerically(">", 0)) or equivalent) and
fail the test if empty; keep the existing logging for debugging but replace the
permissive if/else with a strict assertion for those two namespaces while
leaving observational logging for other namespaces.
- Around line 138-147: The loop over DNS ports unconditionally breaks on the
first iteration so only port 53 is tested and dnsReachable is set true
regardless; update the loop that iterates []int32{53, 5353} to attempt both
ports (remove the unconditional break) and set dnsReachable only when a
successful probe occurs (i.e., call logConnectivityBestEffort for each port and
use its success result or modify/use a helper that returns a bool), e.g., call
logConnectivityBestEffort(configOperatorNamespace, operatorLabels, dnsIPs, port)
for each port and set dnsReachable = true only if that call indicates
reachability, leaving the loop to continue trying remaining ports until one
succeeds or all fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c052d7ae-5c11-42df-9b95-f211197f537a
📒 Files selected for processing (1)
test/extended/networking/config_operator_networkpolicy.go
Comment on lines
+138
to
+147
| dnsReachable := false | ||
| for _, port := range []int32{53, 5353} { | ||
| g.GinkgoWriter.Printf("checking DNS connectivity on port %d\n", port) | ||
| logConnectivityBestEffort(ctx, cs, configOperatorNamespace, operatorLabels, dnsIPs, port, true) | ||
| dnsReachable = true | ||
| break | ||
| } | ||
| if !dnsReachable { | ||
| g.GinkgoWriter.Printf("DNS connectivity check skipped (no ports tested)\n") | ||
| } |
There was a problem hiding this comment.
DNS probe loop only tests the first port due to unconditional break.
Line 143 exits on the first iteration, so port 5353 is never checked, and dnsReachable is always true once the loop runs.
Suggested fix
- dnsReachable := false
- for _, port := range []int32{53, 5353} {
+ for _, port := range []int32{53, 5353} {
g.GinkgoWriter.Printf("checking DNS connectivity on port %d\n", port)
logConnectivityBestEffort(ctx, cs, configOperatorNamespace, operatorLabels, dnsIPs, port, true)
- dnsReachable = true
- break
- }
- if !dnsReachable {
- g.GinkgoWriter.Printf("DNS connectivity check skipped (no ports tested)\n")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| dnsReachable := false | |
| for _, port := range []int32{53, 5353} { | |
| g.GinkgoWriter.Printf("checking DNS connectivity on port %d\n", port) | |
| logConnectivityBestEffort(ctx, cs, configOperatorNamespace, operatorLabels, dnsIPs, port, true) | |
| dnsReachable = true | |
| break | |
| } | |
| if !dnsReachable { | |
| g.GinkgoWriter.Printf("DNS connectivity check skipped (no ports tested)\n") | |
| } | |
| for _, port := range []int32{53, 5353} { | |
| g.GinkgoWriter.Printf("checking DNS connectivity on port %d\n", port) | |
| logConnectivityBestEffort(ctx, cs, configOperatorNamespace, operatorLabels, dnsIPs, port, true) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/networking/config_operator_networkpolicy.go` around lines 138 -
147, The loop over DNS ports unconditionally breaks on the first iteration so
only port 53 is tested and dnsReachable is set true regardless; update the loop
that iterates []int32{53, 5353} to attempt both ports (remove the unconditional
break) and set dnsReachable only when a successful probe occurs (i.e., call
logConnectivityBestEffort for each port and use its success result or modify/use
a helper that returns a bool), e.g., call
logConnectivityBestEffort(configOperatorNamespace, operatorLabels, dnsIPs, port)
for each port and set dnsReachable = true only if that call indicates
reachability, leaving the loop to continue trying remaining ports until one
succeeds or all fail.
Comment on lines
+153
to
+191
| g.It("should verify config namespace NetworkPolicies exist [apigroup:config.openshift.io]", func() { | ||
| ctx := context.Background() | ||
| namespacesToTest := []string{configOperatorNamespace, configNamespace, configManagedNamespace} | ||
|
|
||
| for _, ns := range namespacesToTest { | ||
| g.GinkgoWriter.Printf("=== Testing namespace: %s ===\n", ns) | ||
|
|
||
| g.By(fmt.Sprintf("Verifying namespace %s exists", ns)) | ||
| _, err := cs.CoreV1().Namespaces().Get(ctx, ns, metav1.GetOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| g.By(fmt.Sprintf("Checking for NetworkPolicies in %s", ns)) | ||
| policies, err := cs.NetworkingV1().NetworkPolicies(ns).List(ctx, metav1.ListOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| if len(policies.Items) > 0 { | ||
| g.GinkgoWriter.Printf("Found %d NetworkPolicy(ies) in %s\n", len(policies.Items), ns) | ||
| for _, policy := range policies.Items { | ||
| g.GinkgoWriter.Printf(" - %s\n", policy.Name) | ||
| logNetworkPolicyDetails(fmt.Sprintf("%s/%s", ns, policy.Name), &policy) | ||
| } | ||
| } else { | ||
| g.GinkgoWriter.Printf("No NetworkPolicies found in %s\n", ns) | ||
| } | ||
|
|
||
| g.By(fmt.Sprintf("Checking for pods in %s", ns)) | ||
| pods, err := cs.CoreV1().Pods(ns).List(ctx, metav1.ListOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| if len(pods.Items) > 0 { | ||
| g.GinkgoWriter.Printf("Found %d pod(s) in %s\n", len(pods.Items), ns) | ||
| for _, pod := range pods.Items { | ||
| g.GinkgoWriter.Printf(" - %s (phase: %s, labels: %v)\n", pod.Name, pod.Status.Phase, pod.Labels) | ||
| } | ||
| } else { | ||
| g.GinkgoWriter.Printf("No pods found in %s\n", ns) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
These specs are mostly observational and can pass even when target policies are missing.
Line 168 and Line 210 allow empty policy sets (or only log them), so the tests can pass without proving reconciliation in openshift-config and openshift-config-managed, which is the core PR objective.
Suggested assertion-focused update
@@
- if len(policies.Items) > 0 {
+ if len(policies.Items) > 0 {
g.GinkgoWriter.Printf("Found %d NetworkPolicy(ies) in %s\n", len(policies.Items), ns)
for _, policy := range policies.Items {
g.GinkgoWriter.Printf(" - %s\n", policy.Name)
logNetworkPolicyDetails(fmt.Sprintf("%s/%s", ns, policy.Name), &policy)
}
} else {
g.GinkgoWriter.Printf("No NetworkPolicies found in %s\n", ns)
}
+ // Assert policy reconciliation for namespaces covered by API-1646.
+ if ns == configNamespace || ns == configManagedNamespace {
+ o.Expect(policies.Items).NotTo(o.BeEmpty(), "expected reconciled NetworkPolicies in %s", ns)
+ }
@@
- if len(policies.Items) == 0 {
- g.GinkgoWriter.Printf("No NetworkPolicies found in %s, skipping enforcement tests\n", ns.namespace)
- continue
- }
+ o.Expect(policies.Items).NotTo(o.BeEmpty(), "expected NetworkPolicies in %s before enforcement checks", ns.namespace)Also applies to: 193-251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/networking/config_operator_networkpolicy.go` around lines 153 -
191, The test currently only logs NetworkPolicies for namespaces in
namespacesToTest and allows empty results, so it can pass without verifying
reconciliation; update the loop (referencing namespacesToTest and the call
cs.NetworkingV1().NetworkPolicies(ns).List and logNetworkPolicyDetails) to
assert that the target namespaces "openshift-config" and
"openshift-config-managed" contain at least one NetworkPolicy (use
o.Expect(len(policies.Items)).To(o.BeNumerically(">", 0)) or equivalent) and
fail the test if empty; keep the existing logging for debugging but replace the
permissive if/else with a strict assertion for those two namespaces while
leaving observational logging for other namespaces.
Contributor
|
Scheduling required tests: |
gangwgr
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@gangwgr: This pull request references CNTRLPLANE-2751 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.22.0" version, but no target version was set. 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. |
Contributor
Author
|
/retest-required |
Contributor
Author
|
/test e2e-aws-ovn-fips |
gangwgr
force-pushed
the
config-nw-policy-cases
branch
from
1622182 to
ca0fdce
Compare
Contributor
|
Scheduling required tests: |
Contributor
Author
|
/pipeline required |
Contributor
|
Scheduling required tests: |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: ca0fdce
New tests seen in this PR at sha: ca0fdce
|
Contributor
Author
|
/test e2e-gcp-ovn |
gangwgr
force-pushed
the
config-nw-policy-cases
branch
from
ca0fdce to
7b4b29a
Compare
Contributor
Author
|
/payload 4.22 nightly blocking |
Contributor
|
@gangwgr: trigger 13 job(s) of type blocking for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/8ee1bb20-36fd-11f1-80d9-14c53d83a1f4-0 trigger 66 job(s) of type informing for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/8ee1bb20-36fd-11f1-80d9-14c53d83a1f4-1 |
Contributor
|
Scheduling required tests: |
|
/lgtm |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 7b4b29a
New tests seen in this PR at sha: 7b4b29a
|
Contributor
Author
|
/verified by ci runs |
openshift-ci-robot
added
the
verified
|
@gangwgr: 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. |
Contributor
Author
|
/assign @tssurya |
|
Job Failure Risk Analysis for sha: cffef51
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: cffef51
New tests seen in this PR at sha: cffef51
|
Contributor
Author
|
/retest-required |
3 similar comments
Contributor
Author
|
/retest-required |
Contributor
Author
|
/retest-required |
Contributor
Author
|
/retest-required |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: cffef51
New tests seen in this PR at sha: cffef51
|
|
Job Failure Risk Analysis for sha: cffef51
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: cffef51
New tests seen in this PR at sha: cffef51
|
Contributor
Author
|
/verified by @gangwgr |
openshift-ci-robot
added
the
verified
|
@gangwgr: 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. |
Contributor
Author
|
/retest-required |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: cffef51
New tests seen in this PR at sha: cffef51
|
Contributor
Contributor
Author
|
/retest-required |
|
Job Failure Risk Analysis for sha: cffef51
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: cffef51
New tests seen in this PR at sha: cffef51
|
|
Job Failure Risk Analysis for sha: cffef51
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: cffef51
New tests seen in this PR at sha: cffef51
|
|
Job Failure Risk Analysis for sha: cffef51
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: cffef51
New tests seen in this PR at sha: cffef51
|
Contributor
Author
|
/retest-required |
Contributor
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: cffef51
New tests seen in this PR at sha: cffef51
|
Contributor
|
@gangwgr: 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. |
gangwgr
changed the title
openshift-ci-robot
added
the
jira/valid-bug
|
@gangwgr: This pull request references Jira Issue OCPBUGS-84036, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
@gangwgr: Jira Issue Verification Checks: Jira Issue OCPBUGS-84036 Jira Issue OCPBUGS-84036 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
Contributor
Author
|
/cherry-pick release-4.22 |
|
@gangwgr: new pull request created: #31053 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 kubernetes-sigs/prow repository. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
Adding e2e network policy cases for config-operator.
cluster-config-operator PR #463 (API-1646) adds NetworkPolicies to the namespaces managed by the cluster-config-operator (openshift-config-operator, openshift-config, openshift-config-managed). This PR adds E2E tests to verify those policies are correctly applied and reconciled.
Summary by CodeRabbit