WIP: monitortests: Add monitor test that requires default deny-all NetworkPolicy on all platform namespaces#30710
Open
liouk wants to merge 1 commit intoopenshift:mainfrom
Open
WIP: monitortests: Add monitor test that requires default deny-all NetworkPolicy on all platform namespaces#30710liouk wants to merge 1 commit intoopenshift:mainfrom
liouk wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
do-not-merge/hold
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: liouk 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 |
liouk
force-pushed
the
e2e-network-policy
branch
from
cdffa44 to
21ed8ba
Compare
|
Scheduling required tests: |
liouk
force-pushed
the
e2e-network-policy
branch
from
21ed8ba to
83f670a
Compare
|
Scheduling required tests: |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 83f670a
New tests seen in this PR at sha: 83f670a
|
liouk
force-pushed
the
e2e-network-policy
branch
from
83f670a to
8b11781
Compare
|
Scheduling required tests: |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 8b11781
New tests seen in this PR at sha: 8b11781
|
liouk
force-pushed
the
e2e-network-policy
branch
from
8b11781 to
275551b
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughRegisters a new monitor test "network-policy-invariants" and adds a monitor that scans watched namespaces for a default-deny NetworkPolicy, reporting a failure per namespace when such a policy is missing. Changes
Sequence Diagram(s)sequenceDiagram
participant Registry as "Monitor Registry"
participant Monitor as "NetworkPolicy Monitor"
participant KubeAPI as "Kubernetes API"
participant Namespace as "Namespace"
Registry->>Monitor: instantiate NewNetworkPolicyMonitorTest()
Registry->>Monitor: StartCollection()
Monitor->>KubeAPI: build client from admin REST config
Monitor->>KubeAPI: list Namespaces
KubeAPI-->>Monitor: namespace list
loop per watched namespace
Monitor->>KubeAPI: list NetworkPolicies in Namespace
KubeAPI-->>Monitor: policies
Monitor->>Monitor: evaluate isDenyAllPolicy(policies)
alt deny-all missing
Monitor->>Registry: report test-case failure (namespace)
else deny-all present
Monitor->>Registry: no failure
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 `@pkg/monitortests/network/networkpolicy/monitortest.go`:
- Around line 48-49: The watched namespace list passed to sets.New(...) contains
a duplicated entry "openshift-oauth-apiserver" on consecutive lines; remove the
duplicate from the slice passed to sets.New (or replace the two identical
entries with the correct missing namespace if one was accidentally omitted) so
the list only contains unique intended namespaces; locate the call to
sets.New(...) in monitortest.go (the watched namespaces variable/argument) and
update it accordingly.
- Around line 93-95: The error branch that returns on failing to list network
policies currently discards the original error (err) — update the return to
preserve/wrap the underlying API error so callers can inspect it; replace
fmt.Errorf("could not list network policies of ns/%s", ns.Name) with a wrapped
error that includes err (e.g. use fmt.Errorf("could not list network policies of
ns/%s: %w", ns.Name, err) or include err in the message) in the same function
where ns.Name and err are in scope.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
pkg/defaultmonitortests/types.gopkg/monitortests/network/networkpolicy/monitortest.go
liouk
force-pushed
the
e2e-network-policy
branch
2 times, most recently
from
eee1f77 to
edac4b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/monitortests/network/networkpolicy/monitortest.go (1)
79-81:⚠️ Potential issue | 🟡 MinorPreserve underlying API errors in returned failures.
Line 80 and Line 97 drop the original error detail; wrap with
%wso callers/logs keep the full cause chain. This also matches the previously raised concern on the policy-list path.🛠️ Proposed fix
nsList, err := n.kubeClient.CoreV1().Namespaces().List(ctx, metav1.ListOptions{}) if err != nil { - return nil, nil, fmt.Errorf("could not list all namespaces; %v", err) + return nil, nil, fmt.Errorf("could not list all namespaces: %w", err) } @@ networkPolicies, err := n.kubeClient.NetworkingV1().NetworkPolicies(ns.Name).List(ctx, metav1.ListOptions{}) if err != nil { - return nil, nil, fmt.Errorf("could not list network policies of ns/%s", ns.Name) + return nil, nil, fmt.Errorf("could not list network policies of ns/%s: %w", ns.Name, err) }#!/bin/bash # Verify current error formatting for the two API list failure paths in CollectData. rg -n -C2 'could not list all namespaces|could not list network policies of ns/%s' pkg/monitortests/network/networkpolicy/monitortest.goAlso applies to: 95-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/monitortests/network/networkpolicy/monitortest.go` around lines 79 - 81, In CollectData, the two error returns that build messages "could not list all namespaces; %v" and "could not list network policies of ns/%s; %v" should wrap the underlying error using %w so the original error chain is preserved; update the fmt.Errorf calls in monitortest.go (the ones referencing those exact messages) to use "%w" and pass the original err as the wrapped value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/monitortests/network/networkpolicy/monitortest.go`:
- Around line 74-76: The current check "if n.kubeClient == nil { return nil,
nil, nil }" silently treats an uninitialized Kubernetes client as success;
change it to fail fast by returning an error when n.kubeClient is nil (e.g.,
return nil, nil, an error like "kubeClient not initialized" or wrap with
context). Update the return to propagate an error from the function that
contains this check (referencing n.kubeClient in
pkg/monitortests/network/networkpolicy/monitortest.go) so callers can detect and
handle the missing client instead of assuming success.
---
Duplicate comments:
In `@pkg/monitortests/network/networkpolicy/monitortest.go`:
- Around line 79-81: In CollectData, the two error returns that build messages
"could not list all namespaces; %v" and "could not list network policies of
ns/%s; %v" should wrap the underlying error using %w so the original error chain
is preserved; update the fmt.Errorf calls in monitortest.go (the ones
referencing those exact messages) to use "%w" and pass the original err as the
wrapped value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 147a7553-4da9-49e9-a880-8e457b081163
📒 Files selected for processing (2)
pkg/defaultmonitortests/types.gopkg/monitortests/network/networkpolicy/monitortest.go
…Policy on all platform namespaces
liouk
force-pushed
the
e2e-network-policy
branch
from
edac4b7 to
5789e96
Compare
|
Scheduling required tests: |
Contributor
|
@liouk: 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: 5789e96
New tests seen in this PR at sha: 5789e96
|
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.
This PR is currently used for investigation purposes only.
/hold
Summary by CodeRabbit