Open
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughAdds a new end-to-end NetworkPolicy test suite and supporting docs/scripts: four e2e test implementations and utilities, test harness entrypoint, a runner script, and multiple documentation files (examples, quick start, README, troubleshooting) for executing and diagnosing tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Orchestrator
participant K8s as Kubernetes API
participant NS as Test Namespace
participant Client as Client Pod (netexec)
participant Server as Server Pod (netexec)
participant Policy as NetworkPolicy
Test->>K8s: Create `NS`
K8s-->>Test: Namespace ready
Test->>K8s: Create `Server`
K8s-->>Server: Pod running
Test->>K8s: Create `Client`
K8s-->>Client: Pod running
Test->>K8s: Apply `default-deny` Policy
K8s-->>Policy: Policy enforced
Client->>Server: Attempt connection
Server-->>Client: Connection blocked (deny)
Client-->>Test: Report blocked
Test->>K8s: Apply allow-ingress Policy
K8s-->>Policy: Policy updated
Client->>Server: Attempt connection
Server-->>Client: Connection allowed
Client-->>Test: Report allowed
Test->>K8s: Cleanup `NS`
K8s-->>Test: Resources removed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zhouying7780 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: 10
🤖 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/main_test.go`:
- Around line 12-14: The TestMain function runs unconditionally and can
accidentally execute live-cluster tests; gate it behind an explicit opt-in by
either adding a build tag (e.g., add a //go:build e2e line and corresponding
+build comment at the top and require running `go test -tags=e2e`) or by
checking an environment variable (e.g., read os.Getenv("E2E") or "RUN_E2E" at
the top of TestMain and call os.Exit(0) or skip when not set); update TestMain
to early-exit or skip tests unless the opt-in is present and document the chosen
opt-in mechanism in the test (referencing the TestMain function to locate the
change).
In `@test/e2e/network_policy_enforcement_test.go`:
- Around line 129-134: The test uses fixed pod names ("np-operator-allowed",
"np-operator-denied") in createServerPodT which causes flakes on reruns because
stranded pods cause AlreadyExists; change the calls that create pods (calls to
createServerPodT in this file, including the block around lines 129-134 and the
other occurrences around 364-391) to use unique names (e.g., append a short
random/UUID or use Kubernetes GenerateName behavior) so each test run creates
distinct pod names; update the arguments passed to createServerPodT (and any
related cleanup references) to use the generated unique names.
- Around line 303-326: The test currently only logs when the namespace has no
pods or pods are not ready, so it never fails; update the check in the test (the
block that calls kubeClient.CoreV1().Pods and inspects pods.Items and
pod.Status) to assert expectations: if len(pods.Items) == 0 call t.Fatalf (or
t.Fatalf with context) to fail the test, and for each pod that is not Running
and Ready call t.Fatalf (or accumulate failures and t.Fatalf after the loop) so
a degraded operator namespace causes test failure; reference the pods variable,
pods.Items, pod.Status.Phase, and the PodReady condition when implementing the
assertions.
- Around line 166-185: The test currently treats each failed DNS probe as a
warning, allowing the test to pass even if no DNS connectivity exists; modify
the block that iterates ports in the DNS egress check (referencing hasDNSEgress,
dnsSvc/serviceClusterIPs, testConnectivityWithTimeout, and the for loop over
ports) to require at least one successful connectivity check: track a success
flag and the last error, call testConnectivityWithTimeout for each port as you
do now, and if none succeed after the loop call t.Fatalf (or t.Errorf +
t.FailNow) with a clear message including the collected error and the
dnsIPs/configOperatorNamespace/operatorLabels context instead of only t.Logf
warnings so the test fails when DNS egress is actually unusable.
In `@test/e2e/network_policy_utils.go`:
- Around line 55-69: formatPeers currently ignores ipBlock entries and prints
"{}" when NamespaceSelector and PodSelector are empty; update formatPeers (and
use its helper formatSelector) to detect when peer.IPBlock != nil and render the
CIDR and any Except entries (e.g., "ipBlock=CIDR except=[...]" or similar)
instead of "{}". Specifically, inside the loop in formatPeers, after ns :=
formatSelector(...) and pod := formatSelector(...), check if peer.IPBlock != nil
and build a string showing peer.IPBlock.CIDR and a joined list of
peer.IPBlock.Except (use joinStrings for the list), then append that
representation to out; keep existing behavior for ns/pod combinations and
preserve joinStrings(out) at the end.
- Around line 12-19: The function getKubeConfig currently forces
loadingRules.ExplicitPath = "/home/yinzhou/kubeconfig", which ignores the
KUBECONFIG env var; update getKubeConfig (and its use of
clientcmd.NewDefaultClientConfigLoadingRules / loadingRules) so it honors
KUBECONFIG by either removing the hardcoded ExplicitPath assignment or setting
loadingRules.ExplicitPath from os.Getenv("KUBECONFIG") only when that env var is
non-empty; keep the rest of the flow (configOverrides,
NewNonInteractiveDeferredLoadingClientConfig, kubeConfig.ClientConfig())
unchanged.
In `@test/e2e/README_NETWORK_POLICY_TESTS.md`:
- Around line 94-96: The example command currently uses the regex
TestConfig.*NetworkPolicy.* which only matches tests whose names start with
"TestConfig", so update the example to use the broader pattern
Test.*NetworkPolicy.* (matching both TestConfig... and
TestGenericNetworkPolicyEnforcement) wherever the run-all NetworkPolicy example
appears (e.g., the README entry showing ./cluster-config-operator-tests
-test.run 'TestConfig.*NetworkPolicy.*' -test.v); replace that pattern with
Test.*NetworkPolicy.* to ensure the full suite runs.
In `@test/e2e/run-tests.sh`:
- Around line 7-8: The script sets a hardcoded developer kubeconfig path; change
the default KUBECONFIG fallback to the standard per-user kubeconfig (use
$HOME/.kube/config) instead of /home/yinzhou/kubeconfig by updating the
KUBECONFIG assignment (the KUBECONFIG variable in run-tests.sh) so it falls back
to $HOME/.kube/config when KUBECONFIG is unset, then export KUBECONFIG.
In `@test/e2e/TROUBLESHOOTING.md`:
- Around line 8-10: Update the Markdown fenced code blocks that contain the
error snippets so they include a language tag (e.g., use ```text or ```console)
on the opening fence; specifically edit the fences containing the snippet
"connectivity check failed for openshift-config-operator/172.30.0.10:5353
expected=true: timed out waiting for the condition" and the other similar error
blocks referenced (the blocks at the other occurrences) to satisfy MD040 by
adding the language identifier to each opening ``` fence.
In `@test/GO_TEST_EXAMPLES.md`:
- Around line 194-195: The two relative links at the bottom of
GO_TEST_EXAMPLES.md point to ./test/e2e/... which resolves incorrectly to
test/test/e2e/...; update both link targets to ./e2e/QUICK_START.md and
./e2e/README_NETWORK_POLICY_TESTS.md (replace ./test/e2e/ with ./e2e/) so the
Quick Start and Network Policy README links resolve correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04b717c6-28d6-4d03-b4f2-e11536e75f88
📒 Files selected for processing (9)
test-binarytest/GO_TEST_EXAMPLES.mdtest/e2e/QUICK_START.mdtest/e2e/README_NETWORK_POLICY_TESTS.mdtest/e2e/TROUBLESHOOTING.mdtest/e2e/main_test.gotest/e2e/network_policy_enforcement_test.gotest/e2e/network_policy_utils.gotest/e2e/run-tests.sh
Comment on lines
+12
to
+14
| func TestMain(m *testing.M) { | ||
| os.Exit(m.Run()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.go" -path "*/test/e2e/*" | head -20Repository: openshift/cluster-config-operator
Length of output: 181
🏁 Script executed:
cat test/e2e/main_test.goRepository: openshift/cluster-config-operator
Length of output: 253
🏁 Script executed:
cat test/e2e/network_policy_enforcement_test.goRepository: openshift/cluster-config-operator
Length of output: 23337
🏁 Script executed:
grep -r "getKubeConfig" test/e2e/Repository: openshift/cluster-config-operator
Length of output: 748
🏁 Script executed:
grep -r "//+build" test/e2e/ || echo "No build tags found"Repository: openshift/cluster-config-operator
Length of output: 96
🏁 Script executed:
grep -r "testing.Skip\|SkipNow\|os.Getenv" test/e2e/Repository: openshift/cluster-config-operator
Length of output: 59
🏁 Script executed:
ls -la test/e2e/Repository: openshift/cluster-config-operator
Length of output: 690
🏁 Script executed:
cat test/e2e/network_policy_utils.goRepository: openshift/cluster-config-operator
Length of output: 2929
🏁 Script executed:
cat test/e2e/run-tests.shRepository: openshift/cluster-config-operator
Length of output: 645
🏁 Script executed:
cat test/e2e/README_NETWORK_POLICY_TESTS.md | head -80Repository: openshift/cluster-config-operator
Length of output: 2736
Gate this package behind an explicit e2e opt-in.
TestMain provides no gating, so go test ./... will attempt to run these live-cluster tests if /home/yinzhou/kubeconfig exists. These tests create and delete namespaces and pods against a live cluster, which can break normal unit-test runs or mutate infrastructure unintentionally. Use build tags or environment variable guards to require explicit opt-in.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/main_test.go` around lines 12 - 14, The TestMain function runs
unconditionally and can accidentally execute live-cluster tests; gate it behind
an explicit opt-in by either adding a build tag (e.g., add a //go:build e2e line
and corresponding +build comment at the top and require running `go test
-tags=e2e`) or by checking an environment variable (e.g., read os.Getenv("E2E")
or "RUN_E2E" at the top of TestMain and call os.Exit(0) or skip when not set);
update TestMain to early-exit or skip tests unless the opt-in is present and
document the chosen opt-in mechanism in the test (referencing the TestMain
function to locate the change).
Comment on lines
+129
to
+134
| t.Log("Creating test pods in openshift-config-operator for allow/deny checks") | ||
| t.Logf("creating operator server pods in %s", configOperatorNamespace) | ||
| allowedServerIPs, cleanupAllowed := createServerPodT(t, kubeClient, configOperatorNamespace, "np-operator-allowed", operatorLabels, 8443) | ||
| defer cleanupAllowed() | ||
| deniedServerIPs, cleanupDenied := createServerPodT(t, kubeClient, configOperatorNamespace, "np-operator-denied", operatorLabels, 12345) | ||
| defer cleanupDenied() |
There was a problem hiding this comment.
Fixed pod names will make reruns flaky.
These pods are created in a shared namespace, and createServerPodT only hands the cleanup closure back after readiness succeeds. Any interrupted or failed run can strand np-operator-allowed/np-operator-denied, and the next run will fail on AlreadyExists.
Also applies to: 364-391
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/network_policy_enforcement_test.go` around lines 129 - 134, The test
uses fixed pod names ("np-operator-allowed", "np-operator-denied") in
createServerPodT which causes flakes on reruns because stranded pods cause
AlreadyExists; change the calls that create pods (calls to createServerPodT in
this file, including the block around lines 129-134 and the other occurrences
around 364-391) to use unique names (e.g., append a short random/UUID or use
Kubernetes GenerateName behavior) so each test run creates distinct pod names;
update the arguments passed to createServerPodT (and any related cleanup
references) to use the generated unique names.
Comment on lines
+303
to
+326
| pods, err := kubeClient.CoreV1().Pods(ns.namespace).List(context.TODO(), metav1.ListOptions{}) | ||
| if err != nil { | ||
| t.Fatalf("failed to list pods in %s: %v", ns.namespace, err) | ||
| } | ||
|
|
||
| if len(pods.Items) > 0 { | ||
| t.Logf("Verifying that %d existing pod(s) in %s are healthy despite NetworkPolicies", len(pods.Items), ns.namespace) | ||
| for _, pod := range pods.Items { | ||
| // Check if pod is running and ready | ||
| isReady := false | ||
| for _, condition := range pod.Status.Conditions { | ||
| if condition.Type == corev1.PodReady && condition.Status == corev1.ConditionTrue { | ||
| isReady = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if pod.Status.Phase == corev1.PodRunning && isReady { | ||
| t.Logf(" ✓ Pod %s is running and ready", pod.Name) | ||
| } else { | ||
| t.Logf(" - Pod %s phase: %s, ready: %v", pod.Name, pod.Status.Phase, isReady) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This “enforcement” test never fails on a degraded operator namespace.
len(pods.Items) == 0 and the non-ready branch both only log, so the test still passes when the namespace is empty or its pods are unhealthy. If this is supposed to validate that NetworkPolicies are not breaking workloads, those conditions need to fail the test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/network_policy_enforcement_test.go` around lines 303 - 326, The test
currently only logs when the namespace has no pods or pods are not ready, so it
never fails; update the check in the test (the block that calls
kubeClient.CoreV1().Pods and inspects pods.Items and pod.Status) to assert
expectations: if len(pods.Items) == 0 call t.Fatalf (or t.Fatalf with context)
to fail the test, and for each pod that is not Running and Ready call t.Fatalf
(or accumulate failures and t.Fatalf after the loop) so a degraded operator
namespace causes test failure; reference the pods variable, pods.Items,
pod.Status.Phase, and the PodReady condition when implementing the assertions.
Comment on lines
+8
to
+10
| ``` | ||
| connectivity check failed for openshift-config-operator/172.30.0.10:5353 expected=true: timed out waiting for the condition | ||
| ``` |
There was a problem hiding this comment.
Add a language to these fenced error snippets.
markdownlint is already flagging these blocks. Using text or console on each opening fence will clear MD040.
Also applies to: 55-57, 85-87, 111-113, 138-140, 161-163
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 8-8: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/TROUBLESHOOTING.md` around lines 8 - 10, Update the Markdown fenced
code blocks that contain the error snippets so they include a language tag
(e.g., use ```text or ```console) on the opening fence; specifically edit the
fences containing the snippet "connectivity check failed for
openshift-config-operator/172.30.0.10:5353 expected=true: timed out waiting for
the condition" and the other similar error blocks referenced (the blocks at the
other occurrences) to satisfy MD040 by adding the language identifier to each
opening ``` fence.
zhouying7780
force-pushed
the
networkpolicy-e2e
branch
from
31a08d5 to
380a63e
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
test/e2e/network_policy_enforcement_test.go (2)
313-331:⚠️ Potential issue | 🟠 MajorFail when operator namespace pods are absent or unhealthy.
Current logic only logs degraded state, so this “enforcement” check can pass despite broken workloads.
Suggested fix
- if len(pods.Items) > 0 { - t.Logf("Verifying that %d existing pod(s) in %s are healthy despite NetworkPolicies", len(pods.Items), ns.namespace) - for _, pod := range pods.Items { + if len(pods.Items) == 0 { + t.Fatalf("expected at least one pod in %s to validate enforcement health", ns.namespace) + } + t.Logf("Verifying that %d existing pod(s) in %s are healthy despite NetworkPolicies", len(pods.Items), ns.namespace) + unhealthy := []string{} + for _, pod := range pods.Items { // Check if pod is running and ready isReady := false @@ - if pod.Status.Phase == corev1.PodRunning && isReady { - t.Logf(" ✓ Pod %s is running and ready", pod.Name) - } else { - t.Logf(" - Pod %s phase: %s, ready: %v", pod.Name, pod.Status.Phase, isReady) - } - } - } + if pod.Status.Phase == corev1.PodRunning && isReady { + t.Logf(" ✓ Pod %s is running and ready", pod.Name) + continue + } + unhealthy = append(unhealthy, fmt.Sprintf("%s(phase=%s ready=%t)", pod.Name, pod.Status.Phase, isReady)) + } + if len(unhealthy) > 0 { + t.Fatalf("unhealthy pods in %s: %s", ns.namespace, strings.Join(unhealthy, ", ")) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/network_policy_enforcement_test.go` around lines 313 - 331, The current check only logs pod health for pods in pods.Items but does not fail the test when pods are missing or unhealthy; update the block that examines pods.Items (using pods, pods.Items, pod, ns.namespace and the test's t) to make the test fail on bad state: if len(pods.Items) == 0 call t.Fatalf with a clear message about missing pods in ns.namespace, and when iterating pods.Items collect any pod that is not in Phase==corev1.PodRunning or not PodReady and after the loop call t.Fatalf (or t.Errorf + t.FailNow) listing the unhealthy pods and their phases/ready status so the test fails instead of merely logging.
131-134:⚠️ Potential issue | 🟠 MajorUse unique pod names and clean up on readiness failure to avoid rerun flakes.
Lines 131/133 still use static names, and readiness failure path can leave created pods behind.
Suggested fix
-allowedServerIPs, cleanupAllowed := createServerPodT(t, kubeClient, configOperatorNamespace, "np-operator-allowed", operatorLabels, 8443) +allowedName := "np-operator-allowed-" + rand.String(5) +allowedServerIPs, cleanupAllowed := createServerPodT(t, kubeClient, configOperatorNamespace, allowedName, operatorLabels, 8443) defer cleanupAllowed() -deniedServerIPs, cleanupDenied := createServerPodT(t, kubeClient, configOperatorNamespace, "np-operator-denied", operatorLabels, 12345) +deniedName := "np-operator-denied-" + rand.String(5) +deniedServerIPs, cleanupDenied := createServerPodT(t, kubeClient, configOperatorNamespace, deniedName, operatorLabels, 12345) defer cleanupDenied()if err := waitForPodReadyT(t, kubeClient, namespace, name); err != nil { + _ = kubeClient.CoreV1().Pods(namespace).Delete(context.TODO(), name, metav1.DeleteOptions{}) t.Fatalf("server pod not ready: %v", err) }Also applies to: 378-380
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/network_policy_enforcement_test.go` around lines 131 - 134, The test uses static pod names ("np-operator-allowed", "np-operator-denied") and defers cleanup after readiness checks, which can leak pods on failures; update the calls to createServerPodT in network_policy_enforcement_test.go to generate unique pod names per test run (e.g., append a random or test-unique suffix) when calling createServerPodT and call defer cleanupAllowed() and defer cleanupDenied() immediately after each createServerPodT return so cleanup always runs even if readiness/assertions fail; locate the two call sites (createServerPodT and their cleanup variables around the blocks that define allowedServerIPs/deniedServerIPs and ensure the same changes are applied to the other occurrence at the later lines referenced (around the 378-380 region).test/e2e/main_test.go (1)
12-14:⚠️ Potential issue | 🟠 MajorGate e2e execution behind explicit opt-in.
Line 12 runs this package unconditionally. That still allows accidental live-cluster mutation from normal test invocations.
Suggested fix
import ( + "fmt" "os" "testing" ) @@ func TestMain(m *testing.M) { + if os.Getenv("RUN_E2E") != "1" { + fmt.Fprintln(os.Stderr, "Skipping e2e tests (set RUN_E2E=1 to enable)") + os.Exit(0) + } os.Exit(m.Run()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/main_test.go` around lines 12 - 14, Gate the e2e package behind an explicit opt-in in TestMain: before calling os.Exit(m.Run()), check a clear opt-in signal (e.g., an environment variable like "RUN_E2E" or "E2E") and if it is not set, print/log a short message and exit cleanly (os.Exit(0)) so tests do not run against live clusters by accident; if the opt-in is present then proceed to call m.Run() as before.test/e2e/TROUBLESHOOTING.md (1)
8-10:⚠️ Potential issue | 🟡 MinorAdd language identifiers to error fenced blocks.
These blocks still use bare triple backticks; use
text/consoleto satisfy markdown lint and keep formatting consistent.Suggested fix
-``` +```text connectivity check failed for openshift-config-operator/172.30.0.10:5353 expected=true: timed out waiting for the condition@@
-+text
failed to create server pod: pods "np-operator-allowed" is forbidden: unable to validate against any security context constraint@@ -``` +```text failed to get config operator NetworkPolicy: networkpolicies.networking.k8s.io "config-operator-networkpolicy" not found@@
-+text
failed to get kubeconfig: invalid configuration: no configuration has been provided@@ -``` +```text failed to get namespace openshift-config-operator: namespaces "openshift-config-operator" not found@@
-+text
panic: test timed out after 10m0sAlso applies to: 55-57, 85-87, 111-113, 138-140, 161-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/TROUBLESHOOTING.md` around lines 8 - 10, Update the markdown fenced code blocks that currently use bare ``` so they include a language identifier (use "text" or "console"); specifically find the error message blocks (e.g., "connectivity check failed for openshift-config-operator/172.30.0.10:5353 expected=true: timed out waiting for the condition", "failed to create server pod: pods \"np-operator-allowed\" is forbidden: unable to validate against any security context constraint", "failed to get config operator NetworkPolicy: networkpolicies.networking.k8s.io \"config-operator-networkpolicy\" not found", "failed to get kubeconfig: invalid configuration: no configuration has been provided", "failed to get namespace openshift-config-operator: namespaces \"openshift-config-operator\" not found", and "panic: test timed out after 10m0s") and change their opening fences from ``` to ```text (or ```console) so markdown lint rules are satisfied and formatting remains consistent.
🤖 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/QUICK_START.md`:
- Around line 50-52: Replace the hardcoded kubeconfig example
`/home/yinzhou/kubeconfig` in QUICK_START.md with guidance that the test suite
respects the KUBECONFIG environment variable and the default kubeconfig loading
behavior; update the two occurrences (the current bullet and the block at lines
~68-70) to instruct users to either set KUBECONFIG or rely on the default
kubeconfig lookup rather than editing a hardcoded path. Ensure the new wording
mentions the KUBECONFIG env var explicitly and that test utilities will load the
default kubeconfig if KUBECONFIG is not set.
- Line 6: Replace the machine-specific command "cd
/home/yinzhou/repos/cluster-config-operator" with a portable repo-root
navigation command; update the QUICK_START.md line that contains that exact
string to use something like cd "$(git rev-parse --show-toplevel)" or cd
cluster-config-operator (if repo directory name is guaranteed) so contributors
can run the quick start regardless of their local path.
In `@test/e2e/TROUBLESHOOTING.md`:
- Around line 24-25: Update the TROUBLESHOOTING.md DNS behavior lines to match
current test semantics: change the statement that DNS connectivity issues are
only warnings to indicate the test will fail if a NetworkPolicy declares DNS
egress and none of the tested DNS ports are reachable, and keep the note that
DNS tests are skipped when the NetworkPolicy does not specify DNS rules; ensure
both places that mention this (the two DNS-related bullet points) are updated to
the new behavior.
- Around line 116-131: The troubleshooting doc incorrectly states kubeconfig is
hardcoded and requires editing getKubeConfig(); update the TROUBLESHOOTING.md
text to reflect the current behavior of getKubeConfig() in
test/e2e/network_policy_utils.go by removing the hardcoded path example and
replacing it with instructions to either set the KUBECONFIG environment variable
(export KUBECONFIG=/path/to/kubeconfig) or rely on the default
$HOME/.kube/config, and note that no code change is required to pick up
KUBECONFIG.
In `@test/GO_TEST_EXAMPLES.md`:
- Around line 7-10: Replace the developer-specific absolute paths in the example
by using a repo-root discovery command and a kubeconfig fallback; e.g., change
the hard-coded cd and export to use a repo variable like REPO_ROOT="$(git
rev-parse --show-toplevel || pwd)" and export
KUBECONFIG="${KUBECONFIG:-$HOME/.kube/config}" (or document using ./ as repo
root), so the example becomes portable across hosts and respects an existing
KUBECONFIG environment variable.
- Around line 178-179: Remove or update the outdated troubleshooting note that
claims "kubeconfig path is hardcoded in test/e2e/network_policy_utils.go" and
instead state that network_policy_utils.go honors the KUBECONFIG environment and
client-go default loading rules (e.g., uses the default client config loading
behavior). Edit the text in GO_TEST_EXAMPLES.md to reflect that the code reads
KUBECONFIG/default kubeconfig locations rather than hardcoding
/home/yinzhou/kubeconfig, and optionally mention how to override via the
KUBECONFIG env var.
---
Duplicate comments:
In `@test/e2e/main_test.go`:
- Around line 12-14: Gate the e2e package behind an explicit opt-in in TestMain:
before calling os.Exit(m.Run()), check a clear opt-in signal (e.g., an
environment variable like "RUN_E2E" or "E2E") and if it is not set, print/log a
short message and exit cleanly (os.Exit(0)) so tests do not run against live
clusters by accident; if the opt-in is present then proceed to call m.Run() as
before.
In `@test/e2e/network_policy_enforcement_test.go`:
- Around line 313-331: The current check only logs pod health for pods in
pods.Items but does not fail the test when pods are missing or unhealthy; update
the block that examines pods.Items (using pods, pods.Items, pod, ns.namespace
and the test's t) to make the test fail on bad state: if len(pods.Items) == 0
call t.Fatalf with a clear message about missing pods in ns.namespace, and when
iterating pods.Items collect any pod that is not in Phase==corev1.PodRunning or
not PodReady and after the loop call t.Fatalf (or t.Errorf + t.FailNow) listing
the unhealthy pods and their phases/ready status so the test fails instead of
merely logging.
- Around line 131-134: The test uses static pod names ("np-operator-allowed",
"np-operator-denied") and defers cleanup after readiness checks, which can leak
pods on failures; update the calls to createServerPodT in
network_policy_enforcement_test.go to generate unique pod names per test run
(e.g., append a random or test-unique suffix) when calling createServerPodT and
call defer cleanupAllowed() and defer cleanupDenied() immediately after each
createServerPodT return so cleanup always runs even if readiness/assertions
fail; locate the two call sites (createServerPodT and their cleanup variables
around the blocks that define allowedServerIPs/deniedServerIPs and ensure the
same changes are applied to the other occurrence at the later lines referenced
(around the 378-380 region).
In `@test/e2e/TROUBLESHOOTING.md`:
- Around line 8-10: Update the markdown fenced code blocks that currently use
bare ``` so they include a language identifier (use "text" or "console");
specifically find the error message blocks (e.g., "connectivity check failed for
openshift-config-operator/172.30.0.10:5353 expected=true: timed out waiting for
the condition", "failed to create server pod: pods \"np-operator-allowed\" is
forbidden: unable to validate against any security context constraint", "failed
to get config operator NetworkPolicy: networkpolicies.networking.k8s.io
\"config-operator-networkpolicy\" not found", "failed to get kubeconfig: invalid
configuration: no configuration has been provided", "failed to get namespace
openshift-config-operator: namespaces \"openshift-config-operator\" not found",
and "panic: test timed out after 10m0s") and change their opening fences from
``` to ```text (or ```console) so markdown lint rules are satisfied and
formatting remains consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e481744d-e314-44e9-b919-d101dea485ba
📒 Files selected for processing (9)
test-binarytest/GO_TEST_EXAMPLES.mdtest/e2e/QUICK_START.mdtest/e2e/README_NETWORK_POLICY_TESTS.mdtest/e2e/TROUBLESHOOTING.mdtest/e2e/main_test.gotest/e2e/network_policy_enforcement_test.gotest/e2e/network_policy_utils.gotest/e2e/run-tests.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/README_NETWORK_POLICY_TESTS.md
test/e2e/QUICK_START.md
Outdated
| ## TL;DR - Run Tests Now | ||
|
|
||
| ```bash | ||
| cd /home/yinzhou/repos/cluster-config-operator |
There was a problem hiding this comment.
Use a portable repo path in quick start commands.
Line 6 is machine-specific and won’t work for most contributors as written.
Suggested fix
-cd /home/yinzhou/repos/cluster-config-operator
+cd /path/to/cluster-config-operator📝 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
| cd /home/yinzhou/repos/cluster-config-operator | |
| cd /path/to/cluster-config-operator |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/QUICK_START.md` at line 6, Replace the machine-specific command "cd
/home/yinzhou/repos/cluster-config-operator" with a portable repo-root
navigation command; update the QUICK_START.md line that contains that exact
string to use something like cd "$(git rev-parse --show-toplevel)" or cd
cluster-config-operator (if repo directory name is guaranteed) so contributors
can run the quick start regardless of their local path.
Comment on lines
+7
to
+10
| ```bash | ||
| cd /home/yinzhou/repos/cluster-config-operator | ||
| export KUBECONFIG=/home/yinzhou/kubeconfig | ||
| ``` |
There was a problem hiding this comment.
Avoid developer-specific absolute setup paths.
Lines 8-10 are host-specific and reduce portability. Prefer a generic repo path plus standard kubeconfig fallback.
Suggested fix
-cd /home/yinzhou/repos/cluster-config-operator
-export KUBECONFIG=/home/yinzhou/kubeconfig
+cd /path/to/cluster-config-operator
+export KUBECONFIG="${KUBECONFIG:-$HOME/.kube/config}"📝 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
| ```bash | |
| cd /home/yinzhou/repos/cluster-config-operator | |
| export KUBECONFIG=/home/yinzhou/kubeconfig | |
| ``` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/GO_TEST_EXAMPLES.md` around lines 7 - 10, Replace the developer-specific
absolute paths in the example by using a repo-root discovery command and a
kubeconfig fallback; e.g., change the hard-coded cd and export to use a repo
variable like REPO_ROOT="$(git rev-parse --show-toplevel || pwd)" and export
KUBECONFIG="${KUBECONFIG:-$HOME/.kube/config}" (or document using ./ as repo
root), so the example becomes portable across hosts and respects an existing
KUBECONFIG environment variable.
380a63e to
3dc6ed2
Compare
Contributor
|
@zhouying7780 Why have you chosen to implement this testing here? We currently don't have any E2E defined in this repo, I'd expect this is better suited to living in origin |
Contributor
|
@zhouying7780: 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
2 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.
Summary by CodeRabbit
Documentation
Tests