CNTRLPLANE-2751:Adding e2e network policy cases for config-operator#30995
CNTRLPLANE-2751:Adding e2e network policy cases for config-operator#30995gangwgr wants to merge 2 commits intoopenshift:mainfrom
Conversation
|
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
| 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.
| 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.
| 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.
|
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. |
|
/retest-required |
|
/test e2e-aws-ovn-fips |
gangwgr
force-pushed
the
config-nw-policy-cases
branch
from
1622182 to
ca0fdce
Compare
|
Scheduling required tests: |
|
/pipeline required |
|
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
|
|
/test e2e-gcp-ovn |
gangwgr
force-pushed
the
config-nw-policy-cases
branch
from
ca0fdce to
7b4b29a
Compare
|
/payload 4.22 nightly blocking |
|
@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 |
|
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
|
|
/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. |
|
/assign @tssurya |
|
@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. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/extended/config_operator/config_operator_networkpolicy.go (1)
178-186:⚠️ Potential issue | 🟠 MajorDon't treat the target namespace policies as optional.
For
openshift-configandopenshift-config-managed, these branches only log orcontinuewhen the policy set is empty ordefault-deny-allis missing. That lets the existence, enforcement, and reconciliation specs go green without proving API-1646 actually rolled out there. Assert the policy is present before continuing.Also applies to: 218-221, 235-237, 261-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/config_operator/config_operator_networkpolicy.go` around lines 178 - 186, The test currently treats missing NetworkPolicies in target namespaces as optional by only logging or continuing when policies.Items is empty or when "default-deny-all" is missing; update the test to assert presence instead: in the blocks that iterate policies (using variables policies, ns and policy.Name and calling logNetworkPolicyDetails) change the logic to fail the test (use Gomega/Expect or g.Fail) if policies.Items is empty for the target namespaces (e.g., "openshift-config" and "openshift-config-managed") and also assert that a policy named "default-deny-all" exists in policies.Items before proceeding, removing the current continue/log-only behavior so absence causes test failure. Ensure you apply the same assertion change to the other similar blocks that handle policies (the other occurrences referenced).
🤖 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/config_operator/config_operator_networkpolicy.go`:
- Around line 781-789: The poll currently only compares expected.Spec to
current.Spec and can pass against the pre-delete object; modify the flow around
the Delete + wait.PollUntilContextTimeout call to capture the original object's
UID (from the first Get before Delete), import
k8s.io/apimachinery/pkg/api/errors as apierrors, and in the
PollUntilContextTimeout callback first attempt to Get the object and accept the
loop iteration only when either apierrors.IsNotFound(err) (object observed
deleted) or the returned object's UID != originalUID (object was recreated);
only after observing a NotFound or UID change proceed to compare
equality.Semantic.DeepEqual(expected.Spec, current.Spec) before returning true.
---
Duplicate comments:
In `@test/extended/config_operator/config_operator_networkpolicy.go`:
- Around line 178-186: The test currently treats missing NetworkPolicies in
target namespaces as optional by only logging or continuing when policies.Items
is empty or when "default-deny-all" is missing; update the test to assert
presence instead: in the blocks that iterate policies (using variables policies,
ns and policy.Name and calling logNetworkPolicyDetails) change the logic to fail
the test (use Gomega/Expect or g.Fail) if policies.Items is empty for the target
namespaces (e.g., "openshift-config" and "openshift-config-managed") and also
assert that a policy named "default-deny-all" exists in policies.Items before
proceeding, removing the current continue/log-only behavior so absence causes
test failure. Ensure you apply the same assertion change to the other similar
blocks that handle policies (the other occurrences referenced).
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a0cdc01-dc10-42a1-9b0c-8581f8004745
📒 Files selected for processing (3)
test/extended/config_operator/OWNERStest/extended/config_operator/config_operator_networkpolicy.gotest/extended/include.go
✅ Files skipped from review due to trivial changes (2)
- test/extended/include.go
- test/extended/config_operator/OWNERS
gangwgr
force-pushed
the
config-nw-policy-cases
branch
2 times, most recently
from
f961ba6 to
774b316
Compare
|
Scheduling required tests: |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: andreacv98, gangwgr 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 |
|
/retest-required |
|
/test e2e-vsphere-ovn-upi |
|
/retest-required |
1 similar comment
|
/retest-required |
gangwgr
force-pushed
the
config-nw-policy-cases
branch
from
774b316 to
cffef51
Compare
|
New changes are detected. LGTM label has been removed. |
|
/retest-required |
|
Scheduling required tests: |
|
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
|
|
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
|
|
/retest-required |
1 similar comment
|
/retest-required |
|
/test e2e-metal-ipi-ovn-ipv6 |
|
/retest-required |
|
/test e2e-gcp-ovn |
|
/retest-required |
|
/test e2e-metal-ipi-ovn-ipv6 |
|
@gangwgr: 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. |
|
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
|
|
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
|
4 participants
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