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 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new Ginkgo E2E test suite that validates NetworkPolicy enforcement and reconciliation for OpenShift Config Operator namespaces, including connectivity probes across policy states, DNS egress checks, namespace policy enumeration, and operator-driven policy restoration. (50 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![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: |
|
@gangwgr: The following test 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: ca0fdce
New tests seen in this PR at sha: ca0fdce
|
2 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.