API-1768:e2e network policy tests#2097
Open
kaleemsiddiqu wants to merge 1 commit into
Open
Conversation
|
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 comprehensive NetworkPolicy end-to-end tests and helpers, a new exported ClusterOperator polling helper, two test suites in the test runner extension (operator/parallel and operator/disruptive), and a small go.mod dependency adjustment. 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 |
openshift-ci
Bot
added
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/e2e/network_policy_enforcement.go`:
- Around line 161-188: The testMetricsIngressOpenAccess spec and its assertions
disagree: the description implies metrics ingress is open from any namespace but
expectConnectivity() denies openshift-etcd; either change the spec text to
describe selective allow rules or change the assertion to allow openshift-etcd
on port 8443 so it matches an “open access” policy. Locate
testMetricsIngressOpenAccess and update the failing expectConnectivity call (the
call using namespace "openshift-etcd", kasOperatorIPs, 8443) to expect true, or
alternatively narrow the spec description and the createServerPod/label
selectors so the denied case is testing a different policy—ensure
expectConnectivity, createServerPod, and the human-readable g.By(...) lines
consistently reflect the intended policy.
- Around line 218-246: The tests are probing ports that netexecPod doesn't
listen on, causing false negatives; update the tests to probe only the single
port passed into netexecPod (the port argument used when calling netexecPod)
instead of hardcoded ports like 12345/9999/80, or alternatively modify
netexecPod to expose every port the tests expect; specifically locate uses of
netexecPod and replace hardcoded probe/connection ports with the port variable
supplied to netexecPod so the connectivity checks truly exercise NetworkPolicy
filtering.
- Around line 96-100: Before creating pods and probing connectivity in
testKubeAPIServerOperatorNetworkPolicyEnforcement, wait for the kube-apiserver
operator to be present and reconciled: insert a blocking call (reuse the
existing test helper used by the network policy tests) such as
waitForOperatorReady("kube-apiserver-operator", kubeClient, kubeConfig, timeout)
right after kubeClient is created and before any pod creation/probing; apply the
same wait before the other live-check blocks in this function (the later probes
around the other kube-apiserver checks) so policy rollout/reconciliation
completes prior to executing tests.
In `@test/e2e/network_policy.go`:
- Around line 139-152: The requireDefaultDeny helper currently only validates an
empty PodSelector and that both PolicyTypes are present; update
requireDefaultDeny to also assert that policy.Spec.Ingress and
policy.Spec.Egress slices are empty (no allow rules) by checking
len(policy.Spec.Ingress) == 0 and len(policy.Spec.Egress) == 0 and failing with
a descriptive message (including policy.Namespace and policy.Name) if either
contains entries; keep existing PodSelector and PolicyTypes checks intact so the
function enforces both presence of Ingress/Egress types and absence of any allow
rules.
- Around line 259-272: The restoreNetworkPolicy function currently treats any
re-created NetworkPolicy with the same name as success and hides non-NotFound
Get errors; modify restoreNetworkPolicy to accept or capture the expected policy
spec (e.g., pass in an expected *networkingv1.NetworkPolicy or fetch and store
the original spec before deletion), then in the wait.PollImmediate loop call
client.NetworkingV1().NetworkPolicies(namespace).Get and if
apierrors.IsNotFound(err) return false, nil, but for other errors return false,
err so they surface immediately; when a policy is returned compare its Spec to
the expected Spec (using reflect.DeepEqual or an appropriate equality helper)
and only return true when the specs match; also add the import apierrors
"k8s.io/apimachinery/pkg/api/errors" to the imports.
- Around line 192-199: Replace the non-failing logger function
logEgressAllowAllTCP with an assertion-based helper: call
hasEgressAllowAllTCP(policy.Spec.Egress) and use a Gomega Expect to assert it is
true (e.g., Expect(hasEgressAllowAllTCP(...))).Include a clear failure message
that mentions the policy name/namespace (policy.Namespace, policy.Name) so tests
fail when the allow-all egress rule is missing; keep g.GinkgoHelper() and remove
only the non-failing GinkgoWriter-only branches so callers (previously using
logEgressAllowAllTCP) will fail the test on missing rule.
🪄 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: 3f382f9c-c7c6-418f-9420-e81c72bfcc2d
📒 Files selected for processing (5)
cmd/cluster-kube-apiserver-operator-tests-ext/main.gogo.modtest/e2e/network_policy.gotest/e2e/network_policy_enforcement.gotest/library/cluster_operator.go
kaleemsiddiqu
force-pushed
the
network-policy
branch
from
304edbb to
10f9051
Compare
openshift-ci
Bot
removed
the
needs-rebase
Author
|
/retest |
5 similar comments
Author
|
/retest |
Author
|
/retest |
Author
|
/retest |
Author
|
/retest |
Author
|
/retest |
Contributor
|
/lgtm |
kaleemsiddiqu
force-pushed
the
network-policy
branch
from
10f9051 to
e14fcc4
Compare
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 `@cmd/cluster-kube-apiserver-operator-tests-ext/main.go`:
- Around line 82-88: The comment above the extension.AddSuite call is
inaccurate: update the comment to reflect the actual Qualifiers filter (which
selects tests with "[Operator]" and excludes "[Serial]") or change the
Qualifiers to also require "[Parallel]" to match the current comment; adjust the
text near oteextension.Suite (Name, Parallelism, Qualifiers) so the
human-readable comment and the implemented filter are consistent (either
"selects [Operator] tests excluding [Serial]" or change the Qualifiers to
`name.contains("[Parallel]") && name.contains("[Operator]")` to preserve the
original "[Parallel] + [Operator]" intent).
In `@test/library/cluster_operator.go`:
- Around line 19-39: The function
WaitForClusterOperatorAvailableNotProgressingNotDegraded currently takes an
unused t testing.TB parameter and there is a duplicate function
WaitForKubeAPIServerClusterOperatorAvailableNotProgressingNotDegraded that
repeats the same polling logic with "kube-apiserver" hardcoded—remove the unused
t parameter from WaitForClusterOperatorAvailableNotProgressingNotDegraded,
delete the duplicate
WaitForKubeAPIServerClusterOperatorAvailableNotProgressingNotDegraded, and
update all callers (e.g. the two sites in test/e2e/network_policy.go) to call
the single generic helper with the name string only; ensure the remaining
function signature and its internal use of client.ClusterOperators().Get(...,
name, ...) and the polling logic (including IsStatusConditionPresentAndEqual
checks) remain unchanged.
🪄 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: 8f31ca8e-0100-44f0-a2b5-16eedee13af4
📒 Files selected for processing (4)
cmd/cluster-kube-apiserver-operator-tests-ext/main.gotest/e2e/network_policy.gotest/e2e/network_policy_enforcement.gotest/library/cluster_operator.go
✅ Files skipped from review due to trivial changes (1)
- test/e2e/network_policy_enforcement.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/network_policy.go
Comment on lines
+19
to
+39
| func WaitForClusterOperatorAvailableNotProgressingNotDegraded(t testing.TB, client configclient.ConfigV1Interface, name string) error { | ||
| err := wait.Poll(WaitPollInterval, WaitPollTimeout, func() (bool, error) { | ||
| clusterOperator, err := client.ClusterOperators().Get(context.TODO(), name, metav1.GetOptions{}) | ||
| if errors.IsNotFound(err) { | ||
| fmt.Printf("ClusterOperator/%s does not yet exist.\n", name) | ||
| return false, nil | ||
| } | ||
| if err != nil { | ||
| fmt.Printf("Unable to retrieve ClusterOperator/%s: %v\n", name, err) | ||
| return false, err | ||
| } | ||
| conditions := clusterOperator.Status.Conditions | ||
| available := clusteroperatorhelpers.IsStatusConditionPresentAndEqual(conditions, configv1.OperatorAvailable, configv1.ConditionTrue) | ||
| notProgressing := clusteroperatorhelpers.IsStatusConditionPresentAndEqual(conditions, configv1.OperatorProgressing, configv1.ConditionFalse) | ||
| notDegraded := clusteroperatorhelpers.IsStatusConditionPresentAndEqual(conditions, configv1.OperatorDegraded, configv1.ConditionFalse) | ||
| done := available && notProgressing && notDegraded | ||
| fmt.Printf("ClusterOperator/%s: Available: %v Progressing: %v Degraded: %v\n", name, available, !notProgressing, !notDegraded) | ||
| return done, nil | ||
| }) | ||
| return err | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify current call sites for signature impact.
rg -nP --type=go '\bWaitForClusterOperatorAvailableNotProgressingNotDegraded\s*\(' -C2
# Verify duplicate polling implementations that should be consolidated.
rg -nP --type=go -C2 'func\s+WaitFor(KubeAPIServerClusterOperatorAvailableNotProgressingNotDegraded|ClusterOperatorAvailableNotProgressingNotDegraded)\b|wait\.Poll\(WaitPollInterval,\s*WaitPollTimeout\)'Repository: openshift/cluster-kube-apiserver-operator
Length of output: 2799
🏁 Script executed:
#!/bin/bash
# Read the full implementations of both functions from cluster_operator.go
cat -n test/library/cluster_operator.go | sed -n '17,70p'Repository: openshift/cluster-kube-apiserver-operator
Length of output: 3599
🏁 Script executed:
#!/bin/bash
# Search for all call sites of WaitForClusterOperatorAvailableNotProgressingNotDegraded
rg -n '\bWaitForClusterOperatorAvailableNotProgressingNotDegraded\b' --type=go
# Also search for any other similar duplicated polling patterns
wc -l test/library/cluster_operator.goRepository: openshift/cluster-kube-apiserver-operator
Length of output: 716
Remove the unused t parameter and consolidate duplicate polling logic.
The t testing.TB parameter at line 19 is never used in the function body. Additionally, WaitForKubeAPIServerClusterOperatorAvailableNotProgressingNotDegraded duplicates the entire polling logic with "kube-apiserver" hardcoded—this duplication creates a maintenance burden and drift risk. Consolidating both into a single generic helper eliminates both issues.
♻️ Proposed refactor
-func WaitForClusterOperatorAvailableNotProgressingNotDegraded(t testing.TB, client configclient.ConfigV1Interface, name string) error {
+func WaitForClusterOperatorAvailableNotProgressingNotDegraded(client configclient.ConfigV1Interface, name string) error {
err := wait.Poll(WaitPollInterval, WaitPollTimeout, func() (bool, error) {
clusterOperator, err := client.ClusterOperators().Get(context.TODO(), name, metav1.GetOptions{})
if errors.IsNotFound(err) {
fmt.Printf("ClusterOperator/%s does not yet exist.\n", name)
return false, nil
}
if err != nil {
fmt.Printf("Unable to retrieve ClusterOperator/%s: %v\n", name, err)
return false, err
}
conditions := clusterOperator.Status.Conditions
available := clusteroperatorhelpers.IsStatusConditionPresentAndEqual(conditions, configv1.OperatorAvailable, configv1.ConditionTrue)
notProgressing := clusteroperatorhelpers.IsStatusConditionPresentAndEqual(conditions, configv1.OperatorProgressing, configv1.ConditionFalse)
notDegraded := clusteroperatorhelpers.IsStatusConditionPresentAndEqual(conditions, configv1.OperatorDegraded, configv1.ConditionFalse)
done := available && notProgressing && notDegraded
fmt.Printf("ClusterOperator/%s: Available: %v Progressing: %v Degraded: %v\n", name, available, !notProgressing, !notDegraded)
return done, nil
})
return err
} func WaitForKubeAPIServerClusterOperatorAvailableNotProgressingNotDegraded(t *testing.T, client configclient.ConfigV1Interface) {
+ t.Helper()
- err := wait.Poll(WaitPollInterval, WaitPollTimeout, func() (bool, error) {
- clusterOperator, err := client.ClusterOperators().Get(context.TODO(), "kube-apiserver", metav1.GetOptions{})
- if errors.IsNotFound(err) {
- fmt.Println("ClusterOperator/kube-apiserver does not yet exist.")
- return false, nil
- }
- if err != nil {
- fmt.Println("Unable to retrieve ClusterOperator/kube-apiserver:", err)
- return false, err
- }
- conditions := clusterOperator.Status.Conditions
- available := clusteroperatorhelpers.IsStatusConditionPresentAndEqual(conditions, configv1.OperatorAvailable, configv1.ConditionTrue)
- notProgressing := clusteroperatorhelpers.IsStatusConditionPresentAndEqual(conditions, configv1.OperatorProgressing, configv1.ConditionFalse)
- notDegraded := clusteroperatorhelpers.IsStatusConditionPresentAndEqual(conditions, configv1.OperatorDegraded, configv1.ConditionFalse)
- done := available && notProgressing && notDegraded
- fmt.Printf("ClusterOperator/kube-apiserver: Available: %v Progressing: %v Degraded: %v\n", available, !notProgressing, !notDegraded)
- return done, nil
- })
- if err != nil {
- t.Fatal(err)
- }
+ if err := WaitForClusterOperatorAvailableNotProgressingNotDegraded(client, "kube-apiserver"); err != nil {
+ t.Fatal(err)
+ }
}Update the two call sites in test/e2e/network_policy.go (lines 52, 97) to remove the first argument:
-err = test.WaitForClusterOperatorAvailableNotProgressingNotDegraded(g.GinkgoTB(), configClient, "kube-apiserver")
+err = test.WaitForClusterOperatorAvailableNotProgressingNotDegraded(configClient, "kube-apiserver")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/library/cluster_operator.go` around lines 19 - 39, The function
WaitForClusterOperatorAvailableNotProgressingNotDegraded currently takes an
unused t testing.TB parameter and there is a duplicate function
WaitForKubeAPIServerClusterOperatorAvailableNotProgressingNotDegraded that
repeats the same polling logic with "kube-apiserver" hardcoded—remove the unused
t parameter from WaitForClusterOperatorAvailableNotProgressingNotDegraded,
delete the duplicate
WaitForKubeAPIServerClusterOperatorAvailableNotProgressingNotDegraded, and
update all callers (e.g. the two sites in test/e2e/network_policy.go) to call
the single generic helper with the name string only; ensure the remaining
function signature and its internal use of client.ClusterOperators().Get(...,
name, ...) and the polling logic (including IsStatusConditionPresentAndEqual
checks) remain unchanged.
Author
|
/retitle API-1768:e2e network policy tests |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@kaleemsiddiqu: This pull request references API-1768 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 story 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. |
Author
|
/retest |
Contributor
|
/retest-required |
Author
|
/retest |
1 similar comment
Author
|
/retest |
Contributor
|
/lgtm |
Author
|
/verified by @kaleemsiddiqu |
gangwgr
reviewed
Apr 30, 2026
kaleemsiddiqu
force-pushed
the
network-policy
branch
from
0dcc284 to
08a40a0
Compare
Author
|
/retest |
kaleemsiddiqu
force-pushed
the
network-policy
branch
2 times, most recently
from
99db3c0 to
753f92f
Compare
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
openshift-ci
Bot
removed
the
approved
Author
|
/retest |
kaleemsiddiqu
force-pushed
the
network-policy
branch
3 times, most recently
from
2cc8ce7 to
61190a4
Compare
Author
|
/retest |
kaleemsiddiqu
force-pushed
the
network-policy
branch
2 times, most recently
from
42dba26 to
ce17986
Compare
kaleemsiddiqu
force-pushed
the
network-policy
branch
from
ce17986 to
f92bf10
Compare
openshift-ci
Bot
added
the
needs-rebase
kaleemsiddiqu
force-pushed
the
network-policy
branch
from
f92bf10 to
22f3727
Compare
openshift-ci
Bot
removed
the
needs-rebase
Author
|
/retest |
4 similar comments
Author
|
/retest |
Author
|
/retest |
Author
|
/retest |
Author
|
/retest |
kaleemsiddiqu
force-pushed
the
network-policy
branch
from
22f3727 to
1661b10
Compare
Author
|
/retest |
kaleemsiddiqu
force-pushed
the
network-policy
branch
from
1661b10 to
2338f8e
Compare
Add comprehensive NetworkPolicy tests for kube-apiserver and kube-apiserver-operator namespaces. Tests verify policy creation, reconciliation, and enforcement including default-deny and allow rules patterns. Signed-off-by: Kaleemullah Siddiqui <ksiddiqu@redhat.com>
kaleemsiddiqu
force-pushed
the
network-policy
branch
from
2338f8e to
93c198c
Compare
Author
|
/retest |
Contributor
|
@kaleemsiddiqu: 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. |
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
4 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.
tests for network policy
Summary by CodeRabbit
Tests
Chores