CORENET-7131: migrate UDN Security and OVNK CNI test cases - #3213
CORENET-7131: migrate UDN Security and OVNK CNI test cases#3213SachinNinganure wants to merge 20 commits into
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:
WalkthroughThis PR introduces a comprehensive OTP (OpenShift Tests Private) test suite covering Multus networking, OVN integration, and security validation. Five new test cases are registered and implemented across Multus secondary networks, pod tracing, and CNI configuration security. ChangesOTP Test Suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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)
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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: SachinNinganure 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
🧹 Nitpick comments (2)
openshift/test/otp/networking_tools_test.go (2)
17-17: ⚡ Quick win
defer g.GinkgoRecover()here is ineffective.Placed in the
Describecontainer body, this defer runs during spec-tree construction and returns immediately — it does not protect any spec.GinkgoRecoveris only needed inside goroutines you spawn; it can be removed here.♻️ Proposed change
var _ = g.Describe("[sig-networking] OTP Networking Tools", func() { - defer g.GinkgoRecover() - var (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift/test/otp/networking_tools_test.go` at line 17, Remove the ineffective defer g.GinkgoRecover() call that lives in the Describe container body; it runs at spec-tree construction time and does not protect specs. Delete the defer g.GinkgoRecover() line from the Describe block in networking_tools_test.go (where g.GinkgoRecover() is referenced) and, if you spawn any goroutines inside individual specs, add g.GinkgoRecover() as a deferred call inside those goroutines instead.
81-82: 💤 Low valueSimplify the false-positive filter.
This condition is equivalent to
!strings.Contains(lowerLine, "token: ")— the first clause is always true whenever the negatedtoken:case is false, so the compound expression reduces to skipping only lines with an emptytoken:field.♻️ Proposed change
- if !strings.Contains(lowerLine, "token:") || - (strings.Contains(lowerLine, "token:") && !strings.Contains(lowerLine, "token: ")) { + if !strings.Contains(lowerLine, "token: ") {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift/test/otp/networking_tools_test.go` around lines 81 - 82, The conditional that filters out false-positive token lines is overly complex; replace the compound condition that checks lowerLine with the simpler check for absence of "token: " (i.e., use !strings.Contains(lowerLine, "token: ")) so the logic only skips lines missing a token value; update the expression that currently inspects lowerLine in the test (the if using strings.Contains(lowerLine, "token:") / "token: ") to the single simplified check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openshift/test/otp/networking_tools_test.go`:
- Around line 84-85: The check that treats any occurrence of "ey" in lowerLine
as a JWT marker is too broad and causes false positives; update the condition in
the strings.Contains checks (the conditional using strings.Contains(lowerLine,
"ey") || strings.Contains(lowerLine, "bearer ")) to look specifically for the
JWT prefix "eyJ" instead of "ey" (i.e., replace the "ey" substring match with
"eyJ") so the lowerLine JWT detection is precise while retaining the existing
"bearer " check.
- Around line 62-65: The test currently silently continues when DoRaw fails,
which allows a false negative if every pod's logs are unreadable; modify the
test to record skipped pods and fail (or assert) when no pods were actually
scanned. Specifically, in the loop that calls DoRaw (reference the DoRaw call
and the totalViolations variable), increment a skippedPods counter or append pod
identifiers when err != nil instead of just continue, and after the loop assert
that (len(pods.Items) - len(skippedPods)) > 0 (or fail the test with a clear
message) so the test fails if all pods were skipped and no logs were examined.
---
Nitpick comments:
In `@openshift/test/otp/networking_tools_test.go`:
- Line 17: Remove the ineffective defer g.GinkgoRecover() call that lives in the
Describe container body; it runs at spec-tree construction time and does not
protect specs. Delete the defer g.GinkgoRecover() line from the Describe block
in networking_tools_test.go (where g.GinkgoRecover() is referenced) and, if you
spawn any goroutines inside individual specs, add g.GinkgoRecover() as a
deferred call inside those goroutines instead.
- Around line 81-82: The conditional that filters out false-positive token lines
is overly complex; replace the compound condition that checks lowerLine with the
simpler check for absence of "token: " (i.e., use !strings.Contains(lowerLine,
"token: ")) so the logic only skips lines missing a token value; update the
expression that currently inspects lowerLine in the test (the if using
strings.Contains(lowerLine, "token:") / "token: ") to the single simplified
check.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 20057199-ce7e-4281-8ed2-bae45629f5c6
📒 Files selected for processing (2)
openshift/test/otp/networking_tools_test.goopenshift/test/otp/otp_suite_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openshift/test/otp/networking_tools_test.go (1)
444-471: ⚡ Quick win
srcIPanddstIPparameters are never used.
runOVNKubeTraceacceptssrcIP/dstIPbut the builtexecCmdonly references the namespaces/pod names, protocol and port. The callers (lines 346-349 and 425-428) perform extraGetcalls solely to populate these unused arguments. Drop the parameters (and the corresponding caller plumbing) to remove the dead/misleading surface, or wire them into the trace command if they were intended to be used.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift/test/otp/networking_tools_test.go` around lines 444 - 471, runOVNKubeTrace currently accepts srcIP and dstIP but never uses them (execCmd only uses namespaces/pod names), so remove the dead parameters: update the runOVNKubeTrace function signature to drop srcIP and dstIP, remove any related local variables, and leave execCmd as-is; then update all callers that were performing extra Get calls solely to obtain those IPs to stop fetching them (remove that plumbing). If instead you intended to trace by IP, wire the values into execCmd (e.g., add the appropriate ovnkube-trace flags for source/destination IP) and keep callers, but choose one approach and make matching changes to runOVNKubeTrace and its callers to eliminate the mismatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openshift/test/otp/networking_tools_test.go`:
- Around line 455-457: The current check "if len(pods.Items) == 0 { return \"\",
err }" returns a nil error when no ovnkube-node pods are found; change it to
return an explicit error (e.g., using fmt.Errorf) so callers get a clear failure
instead of a nil error and later confusing assertions; update the imports to
include "fmt" and replace the return in the pods.Items empty branch with a
descriptive fmt.Errorf error referencing "no ovnkube-node pods found".
---
Nitpick comments:
In `@openshift/test/otp/networking_tools_test.go`:
- Around line 444-471: runOVNKubeTrace currently accepts srcIP and dstIP but
never uses them (execCmd only uses namespaces/pod names), so remove the dead
parameters: update the runOVNKubeTrace function signature to drop srcIP and
dstIP, remove any related local variables, and leave execCmd as-is; then update
all callers that were performing extra Get calls solely to obtain those IPs to
stop fetching them (remove that plumbing). If instead you intended to trace by
IP, wire the values into execCmd (e.g., add the appropriate ovnkube-trace flags
for source/destination IP) and keep callers, but choose one approach and make
matching changes to runOVNKubeTrace and its callers to eliminate the mismatch.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: eeb3c5f7-788c-4b8c-a1c6-2fcf7d08615e
📒 Files selected for processing (1)
openshift/test/otp/networking_tools_test.go
SachinNinganure
changed the title
✅ Actions performedReview triggered.
|
|
@SachinNinganure Change the name of file to networking_tools.go from networking_tools_test.go. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
openshift/test/otp/multus_test.go (1)
20-21: 💤 Low value
defer g.GinkgoRecover()at Describe scope is ineffective.
GinkgoRecover()is intended to recover panics inside goroutines spawned during test execution. At the Describe scope, this defer runs when the Describe closure returns (immediately after spec registration), not during test execution. It serves no purpose here and should be removed.🧹 Proposed fix
var _ = g.Describe("[sig-networking] OTP Multus", func() { - defer g.GinkgoRecover() - var (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift/test/otp/multus_test.go` around lines 20 - 21, Remove the ineffective defer g.GinkgoRecover() placed inside the g.Describe closure: the call is executed at spec registration time rather than during test execution, so delete the line containing "defer g.GinkgoRecover()" next to the g.Describe(...) block; if panic recovery for goroutines is required during tests, add g.GinkgoRecover() as a defer inside appropriate runtime scopes such as It, BeforeEach, AfterEach, or inside spawned goroutines instead of at Describe scope.openshift/test/otp/security_test.go (2)
82-102: 💤 Low valueToken detection logic is convoluted and hard to follow.
The nested conditionals and double-negative checks make the filtering intent unclear. Consider restructuring to explicitly document what constitutes a "false positive" vs "actual token". The current
breakat line 96 also exits after the first violation per pattern, which may miss other violations in the same pod's logs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift/test/otp/security_test.go` around lines 82 - 102, The token-detection loop around patterns/logsStr is convoluted; simplify by iterating each log line (split logsStr into lines), for each line lowerLine check if it contains the pattern and then explicitly classify false-positives (e.g., lines matching `token:\s*$` or `token:\s*$` with no non-whitespace value) versus real tokens using clear positive checks (e.g., regex or checks for "eyj" prefix for JWT or "bearer " token), increment podViolations for each real token found (do not break after the first match so subsequent violations in the same pod are counted), and replace the nested double-negatives around "token:" with an explicit if/else clarity using functions like strings.Contains, strings.TrimSpace or a compiled regex to locate non-empty token values.
20-21: 💤 Low value
defer g.GinkgoRecover()at Describe scope is ineffective.Same issue as in
multus_test.go— this serves no purpose at Describe scope and should be removed.🧹 Proposed fix
var _ = g.Describe("[sig-networking] OTP Security", func() { - defer g.GinkgoRecover() - var (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift/test/otp/security_test.go` around lines 20 - 21, Remove the ineffective defer g.GinkgoRecover() call placed at the Describe scope in the OTP Security test; locate the var _ = g.Describe("[sig-networking] OTP Security", func() { ... }) block and delete the defer g.GinkgoRecover() line (as done in multus_test.go) — do not reintroduce it at Describe scope, only use GinkgoRecover within individual goroutines or appropriate It/BeforeEach scopes if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openshift/test/otp/security_test.go`:
- Around line 216-219: The anonymous probe passed to o.Eventually can
dereference a nil pod when clientset.CoreV1().Pods(...).Get(...) returns an
error; update that closure (the function returning corev1.PodPhase used in the
Eventually call) to capture the error from Get, and if err != nil return
corev1.PodPending (instead of using p.Status.Phase), otherwise return
p.Status.Phase; reference debugPodName and the clientset.CoreV1().Pods(...).Get
call in your change so the test mirrors the multus_test.go pattern.
---
Nitpick comments:
In `@openshift/test/otp/multus_test.go`:
- Around line 20-21: Remove the ineffective defer g.GinkgoRecover() placed
inside the g.Describe closure: the call is executed at spec registration time
rather than during test execution, so delete the line containing "defer
g.GinkgoRecover()" next to the g.Describe(...) block; if panic recovery for
goroutines is required during tests, add g.GinkgoRecover() as a defer inside
appropriate runtime scopes such as It, BeforeEach, AfterEach, or inside spawned
goroutines instead of at Describe scope.
In `@openshift/test/otp/security_test.go`:
- Around line 82-102: The token-detection loop around patterns/logsStr is
convoluted; simplify by iterating each log line (split logsStr into lines), for
each line lowerLine check if it contains the pattern and then explicitly
classify false-positives (e.g., lines matching `token:\s*$` or `token:\s*$` with
no non-whitespace value) versus real tokens using clear positive checks (e.g.,
regex or checks for "eyj" prefix for JWT or "bearer " token), increment
podViolations for each real token found (do not break after the first match so
subsequent violations in the same pod are counted), and replace the nested
double-negatives around "token:" with an explicit if/else clarity using
functions like strings.Contains, strings.TrimSpace or a compiled regex to locate
non-empty token values.
- Around line 20-21: Remove the ineffective defer g.GinkgoRecover() call placed
at the Describe scope in the OTP Security test; locate the var _ =
g.Describe("[sig-networking] OTP Security", func() { ... }) block and delete the
defer g.GinkgoRecover() line (as done in multus_test.go) — do not reintroduce it
at Describe scope, only use GinkgoRecover within individual goroutines or
appropriate It/BeforeEach scopes if needed.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 46fe9741-dce5-4920-8ff9-9d4219b8a305
📒 Files selected for processing (3)
openshift/test/otp/multus_test.goopenshift/test/otp/networking_tools_test.goopenshift/test/otp/security_test.go
💤 Files with no reviewable changes (1)
- openshift/test/otp/networking_tools_test.go
| ) | ||
|
|
||
| // otpBlockingTests lists the substring patterns for OTP tests that should be blocking | ||
| var otpBlockingTests = []string{ |
There was a problem hiding this comment.
@SachinNinganure Thanks. lets hold on making tests blocking. Lets make all informing for now.
| "should not expose API tokens in ovnkube-node logs", | ||
| "should have secure permissions on CNI configuration files", | ||
| "should handle large IPv6 exclude ranges without timeout", | ||
| "should support Dummy CNI plugin with Multus", |
There was a problem hiding this comment.
Multus tests should go in openshift/multus-cni repo. SO I guess we all need to carefully look at our sheets/tasks and not assume default that everything shall go in openshift/ovn-kubernetes repo.
In this list it seems Whereabouts IPAM and CI Plugin with multus are only 2 cases which belongs to openshift/multus-cni repo
There was a problem hiding this comment.
You are a contributor to openshift/multus-cni#303. You can clone it and keep adding .You can add musltus cases there and
we can test with its registered origin binary openshift/origin#31231 (you are also contributor there). Happy to chat more on this.
There was a problem hiding this comment.
Also apparently some cases might belongs to https://github.com/openshift/network-tools as well. So your cases here belongs to 3 repos here IMO.
There was a problem hiding this comment.
Multus tests should go in openshift/multus-cni repo. SO I guess we all need to carefully look at our sheets/tasks and not assume default that everything shall go in openshift/ovn-kubernetes repo.
In this list it seems Whereabouts IPAM and CI Plugin with multus are only 2 cases which belongs to openshift/multus-cni repo
Added the 4 Multus test cases to PR #303 framework
Tested them on live cluster (all PASS)
Created PR: anuragthehatter/multus-cni#1
There was a problem hiding this comment.
Also apparently some cases might belongs to https://github.com/openshift/network-tools as well. So your cases here belongs to 3 repos here IMO.
openshift/network-tools - Security and tools tests (5 tests) TODO
OCP-49216: API token logging security
OCP-77102: CNI file permissions
OCP-55889: ovn-db-run-command script
OCP-67625: ovnkube-trace pod-to-pod
OCP-67648: ovnkube-trace pod-to-hostnetworkpod
| }) | ||
|
|
||
| // Medium-77102: CIS File Permissions for CNI Config | ||
| g.It("[OTP][blocking][case_id:77102] should have secure permissions on CNI configuration files", func() { |
There was a problem hiding this comment.
remove "case_id:" from tests. Lets just keep numbers as we discussed.
e6f46f7 to
d367cdf
Compare
| }) | ||
|
|
||
| // High-57589: Whereabouts CNI Timeout with Large Exclude Range | ||
| g.It("[OTP][informing][57589] should handle large IPv6 exclude ranges without timeout", func() { |
There was a problem hiding this comment.
@SachinNinganure The tests themselves do not have [informing] or [blocking] label
| "[Feature:EVPN][Feature:RouteAdvertisements][FeatureGate:EVPN][ovn-kubernetes-ote][sig-network] BGP: For BGP configured networks When the tested network is of type Layer 3 CUDN EVPN IP-VRF random VTEP When a pod runs on the tested network When there is other network Of type Layer 2 UDN Both networks are isolated [Suite:openshift/conformance/parallel]", | ||
| "[Feature:EVPN][Feature:RouteAdvertisements][FeatureGate:EVPN][ovn-kubernetes-ote][sig-network] BGP: For BGP configured networks When the tested network is of type Layer 3 CUDN EVPN IP-VRF shared VTEP When a pod runs on the tested network When there is other network Of type Layer 3 CUDN EVPN IP-VRF random VTEP Both networks are isolated [Suite:openshift/conformance/parallel]", | ||
| "[Feature:EVPN][Feature:RouteAdvertisements][FeatureGate:EVPN][ovn-kubernetes-ote][sig-network] BGP: For BGP configured networks When the tested network is of type Layer 3 CUDN EVPN IP-VRF random VTEP When a pod runs on the tested network When there is other network Of type Layer 3 UDN Both networks are isolated [Suite:openshift/conformance/parallel]", | ||
| "[Feature:EVPN][Feature:RouteAdvertisements][FeatureGate:EVPN][ovn-kubernetes-ote][sig-network] BGP: For BGP configured networks When the tested network is of type Layer 2 CUDN EVPN MAC-VRF and IP-VRF When a pod runs on the tested network Can reach KAPI service [Suite:openshift/conformance/parallel]", |
There was a problem hiding this comment.
Should these changes be there at all? This file is for upstream tests that are being enabled with OTE.
There was a problem hiding this comment.
I had some confusion for this, claude helped out;
Removed all OTP test entries from tests.go. You're correct - this file is for upstream ovn-kubernetes tests. understood that
OTP tests are:
- Discovered via the import in main.go
- Lifecycle controlled by isOTPBlocking() function
- Don't need to be listed in tests.go
Thank you @asood-rh
| // import OVN-Kubernetes E2Es | ||
| _ "github.com/ovn-kubernetes/ovn-kubernetes/test/e2e" | ||
|
|
||
| // import OTP (OpenShift Tests Private) migration tests |
There was a problem hiding this comment.
This import is happening in main.go, why is it needed here?
There was a problem hiding this comment.
Removed duplicate OTP import from e2e_test.go
| // Check that at least one message mentions the configured IPs | ||
| hasExpectedIPs := false | ||
| for _, msg := range messages { | ||
| msgStr := msg.(string) |
There was a problem hiding this comment.
Inconsistent with code at line#452
There was a problem hiding this comment.
Changed line 474 to use the safer two-value type assertion pattern msgStr, ok := msg.(string) with explicit check, consistent with the pattern at line 452
| }) | ||
|
|
||
| // Medium-55889: ovn-db-run-command Script Functionality | ||
| g.It("[OTP][informing][55889] should execute ovn-db-run-command script successfully", func() { |
There was a problem hiding this comment.
[informing] or [blocking] is not a test label
| }) | ||
|
|
||
| // Medium-49216: API Token Logging Security | ||
| g.It("[OTP][informing][49216] should not expose API tokens in ovnkube-node logs", func() { |
There was a problem hiding this comment.
Same as in multus.go and networking_tools.go , [informing] or [blocking] is not a test label.
| }) | ||
|
|
||
| // OCP-80525: Verify pods with isolated and non-isolated ports using bridge-cni | ||
| g.It("[OTP][informing][80525] should allow communication on non-isolated network but not on isolated network", func() { |
There was a problem hiding this comment.
[informing] is not a test label.
| // All OTP tests marked as [informing] per reviewer feedback | ||
| return false | ||
|
|
||
| // Original logic (commented out for now): |
There was a problem hiding this comment.
Why is this code commented?
There was a problem hiding this comment.
per @anuragthehatter feedback, all OTP tests should start as [informing] for the initial rollout. The commented code is kept for future reference when tests graduate to blocking.
while I can remove it, I thought tests matching blocking will fail when we use this, and this would not be triggered as we have all our cases to be set as informing, should I take it off @asood-rh , or wait for some more stable runs and then open
|
@SachinNinganure For the ovntrace tests, please look at the fixing the tests. I recall a change went in where pod containers for pod ovnkube-node changed. [67625] Pod-to-pod traffic trace - - ovnkube-trace issue [67648] Pod-to-hostnetworkpod traffic trace - ovnkube-trace issue |
| o.Expect(nonIsolatedOutput).To(o.ContainSubstring("0% packet loss"), "Should show successful ping on non-isolated bridge") | ||
| }) | ||
|
|
||
| // OCP-80526: UDN Verify pods with isolated and non-isolated ports using bridge-cni |
There was a problem hiding this comment.
@SachinNinganure Please avoid using references anywhere like OCP-xxxx. Having just numerical IDsin our test case titles is enough.
There was a problem hiding this comment.
done! this was only pending and somehow got missed. thank you @anuragthehatter
| @@ -0,0 +1,547 @@ | |||
| package otp | |||
There was a problem hiding this comment.
As discussed in earlier comment, networking_tools.go cases belongs to openshift/network-tools repo so you probably need to enable that repro from OTE framework perspective and then migrate these all network_tools.go cases there
There was a problem hiding this comment.
Hello @anuragthehatter I have some questions before moving them
'openshift/network-tools' is not a test framework right, I should 1st enable it and then move if decided to move right?.
While
1 these 3 tests validate OVN debugging and are OVN-specific
55889 ovn-nbctl
67625 ovnkube-trace
67648 ovnkube-trace
2 tests validate OVN features (NOT tools):
45146: OVN routing behavior (single-stack GW on dual-stack)
69761: AdminPolicyBasedExternalRoute CRD status
I have created ovn_features.go to have the above 2 cases
only thing remaining is about the 3 cases 55889, 67625 and 67648; get them to openshift/network-tools .
I will put this in to-do to discuss more. Thank you so much for all the reviews and assist @anuragthehatter
| }) | ||
|
|
||
| // Medium-77102: CIS File Permissions for CNI Config | ||
| g.It("77102-should have secure permissions on CNI configuration files", func() { |
There was a problem hiding this comment.
This usecase and associated util if any should be moved to multus PR openshift/multus-cni#303
There was a problem hiding this comment.
moved 77102 to multus-cni
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| }) | ||
|
|
||
| // Medium-49216: API Token Logging Security |
There was a problem hiding this comment.
Please remove such comments as its not followed upstream
There was a problem hiding this comment.
removed all the comment lines to follow upstream
| "k8s.io/client-go/tools/remotecommand" | ||
| ) | ||
|
|
||
| var _ = g.Describe("[JIRA:Networking][OTP][sig-network] OTP Security", func() { |
There was a problem hiding this comment.
Please replace OTP with SDN network segmentation security. Also we just need [OTP] in g.Describe but in each case title like 2445c2b#diff-17e9ab766308533e44e7fe206902b89c107a36433efc64e1d1d6894e8eafab78R42
There was a problem hiding this comment.
I followed this for all the cases including the multus ones in other repo;
Changed all g.Describe() blocks per the reference pattern in commit 2445c2b:
ovn-kubernetes security.go:
- Before: [JIRA:Networking][OTP][sig-network] OTP Security
- After: [OTP] SDN network segmentation security
ovn-kubernetes port_isolation.go:
- Before: [JIRA:Networking][OTP][sig-network] OTP Multus Port Isolation
- After: [OTP] UDN Port Isolation
multus-cni multus.go:
- Before: [JIRA:Networking][OTP][sig-network] OTP Multus [Suite:openshift/multus-cni]
- After: [OTP] Multus CNI [Suite:openshift/multus-cni]
|
/retitle CORENET-7131: migrate UDN Security and OVNK CNI test cases |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@SachinNinganure: This pull request references CORENET-7131 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 "5.0.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. |
0412b51 to
659cbd9
Compare
|
/retest |
|
/test e2e-aws-ovn |
|
/test e2e-aws-ovn-fdp-qe |
|
/test e2e-aws-ovn-edge-zones |
03d5278 to
07f5d86
Compare
|
/test e2e-aws-ovn-fdp-qe e2e-metal-ipi-ovn-ipv6 |
Validates that ovnkube-node container logs do not expose API tokens or Bearer authentication credentials. Test searches ovnkube-controller container logs for sensitive patterns (api-token, authorization, bearer) and filters false positives to ensure no actual tokens are logged. Signed-off-by: Sachin Ninganure <sninganu@sninganu.pnq.csb>
Address reviewer feedback: prevent false negatives by tracking skipped pods and failing if no pods were successfully scanned. Changes: - Track skippedPods when log retrieval fails - Print warning to GinkgoWriter for debugging - Assert at least one pod was scanned before checking for violations This ensures the test cannot pass vacuously if all log retrievals fail due to RBAC, container restarts, or API errors. Signed-off-by: Sachin Ninganure <sninganu@sninganu.pnq.csb>
…eviewer feedback: replace broad 'ey' substring match with 'eyJ' to avoid flagging common words Signed-off-by: Sachin Ninganure <sninganu@sninganu.pnq.csb>
- Medium-55889: ovn-nbctl database command (uses ovn-nbctl directly) - High-57589: Whereabouts CNI large IPv6 exclude ranges (268M IPs) - Medium-67625: ovnkube-trace pod-to-pod [informing] - Medium-67648: ovnkube-trace pod-to-hostnetworkpod [informing] Tests 67625/67648 marked [informing] due to RBAC limitations - ovnkube-trace requires cluster-wide pod list permissions not available in default service accounts. Test results - 3 PASSING [blocking]: 49216, 55889, 57589 - 2 FAILING [informing]: 67625, 67648 (RBAC - expected) All tests compiled successfully
Add fmt import for explicit error messages Return explicit error when no ovnkube-node pods found Remove unused srcIP/dstIP parameters from runOVNKubeTrace Remove unnecessary pod Get calls
and adds new test coverage:
Changes:
- Split networking_tools_test.go into 3 category-based files:
* security_test.go - Security/compliance tests
* networking_tools_test.go - Tools group tests only
* multus_test.go - Multus CNI tests
New Tests Added:
1. Medium-76652: Dummy CNI plugin support with Multus
- Tests dummy interface creation with static IPAM
- Validates network-status annotation contains dummy network
- Confirms IP assignment (10.10.10.2/24)
2. Medium-77102: CIS file permissions for CNI configs
- Checks multus config permissions (600 expected)
- Validates whereabouts config permissions (600 expected)
- Ensures CIS compliance for CNI configuration files
- Uses exec into multus pods and debug pod on nodes
Test Results (7 tests total):
5 blocking tests PASS (100% pass rate)
2 informing tests FAIL (expected - RBAC limitations)
Test Organization:
a) security_test.go: 49216, 77102
b) networking_tools_test.go: 55889, 67625, 67648
c) multus_test.go: 57589, 76652
Changes:
- Split networking_tools_test.go into 3 category-based files:
security.go - Security/compliance tests
networking_tools.go - Tools group tests only
multus.go - Multus CNI tests
- Removed otp_suite_test.go (not needed in OTE pattern)
Added new tests:
1. Medium-76652: Dummy CNI plugin support with Multus
- Tests dummy interface creation with static IPAM
- Validates network-status annotation contains dummy network
- Confirms IP assignment (10.10.10.2/24)
2. Medium-77102: CIS file permissions for CNI configs
Code Review Fixes:
- Fixed runOVNKubeTrace to return explicit error when no ovnkube-node
pods found (previously returned nil error)
- Removed unused srcIP/dstIP parameters from runOVNKubeTrace function
- Updated all callers (67625, 67648 tests)
Test Registration:
- Added all 7 tests to openshift/test/tests.go (5 blocking, 2 informing)
- Imported OTP package in main.go and e2e_test.go
- Tests compile successfully but not appearing in OTE list (needs help)
Test Results (verified with go test before OTE migration):
- 5 blocking tests PASS (100% pass rate)
- 2 informing tests FAIL (expected - RBAC limitations)
Test Organization:
- security.go: 49216, 77102
- networking_tools.go: 55889, 67625, 67648
- multus.go: 57589, 76652
- Replaced clientcmd kubeconfig loading with exutil.NewCLI() - Converted Kubernetes client-go calls to oc.Run() commands - Removed manual namespace creation/deletion (exutil handles this) - Simplified pod exec operations to use oc exec instead of SPDY executor - All test operations now use 'oc' commands for consistency Files refactored: - security.go: Token logging + CIS permissions tests - multus.go: Whereabouts + Dummy CNI tests - networking_tools.go: ovn-nbctl + ovnkube-trace tests Known issue: This refactor requires github.com/openshift/origin dependency which is not yet in main. This PR needs to be rebased on top of PR openshift#3200 (Anurag's networkpolicy migration) which includes the required infrastructure.needs PR openshift#3200 merged or rebase onto that branch to get openshift/origin@v1.5.0-alpha dependency.
- Add [JIRA:Networking][OTP][sig-network] tags to all Describe blocks (required by OTE) - Add isOTPBlocking() function to main.go for OTP lifecycle assignment - Update specs.Walk() to skip label prepending for OTP tests - Fix test names in tests.go to match actual format (without prepended labels)
- Test 45146: Pod health with single-stack gateway on dual-stack cluster
Tests pod with k8s.ovn.org/routing-* annotations
Verified pod reaches Running state and is healthy
- Test 69761: AdminPolicyBasedExternalRoute status aggregation
Validates status.messages from all OVN-IC zones (nodes)
Checks status.status shows Success when all zones report
Tests external gateway IP configuration (172.18.0.8, 172.18.0.9),verified on 422 cluster
Both tests added to networking_tools.go in ExternalRoute category.
Added dynamic client imports for CRD interaction.
Updated main.go with blocking test patterns.
OCP-66876: Whereabouts dual-stack IPAM test
- Validates IPv4 + IPv6 assignment from Whereabouts
- Tests same-node connectivity (IPv4 and IPv6)
- UBI9 Python server with dual-stack HTTP listener
OCP-69947: Macvlan Unsolicited Neighbor Advertisements test
- Validates ICMPv6 NA packets sent when macvlan pods are created
- Packet capture with tcpdump filtering icmp6[0]=136
- Marked as [informing] due to timing sensitivity in automation
- Manually validated successfully (6 NAs captured to ff02::1)
1. Changed all 8 blocking tests to [informing]
- Updated main.go isOTPBlocking() to return false
- Changed test tags: [OTP][blocking] → [OTP][informing]
2. Removed 'case_id:' prefix from test IDs
- Was: [OTP][informing][case_id:77102]
- Now: [OTP][informing][77102]
3. Added PodSecurity labels to fix CI failures
- Added privileged labels to 7 test namespaces
- Fixes tests 45146, 76652, 57589 (403 Forbidden in CI)
Repository redistribution plan (per reviewer):
- 4 Multus tests → openshift/multus-cni (PR created)
- 5 Security/tools tests → openshift/network-tools (future)
- 2 Core OVN-K tests → remain here
Changes:
- 11 informing tests (was 8 blocking + 3 informing)
- PodSecurity labels on 7 namespaces
- Test IDs simplified (no case_id: prefix)
Add bridge CNI port isolation test cases: - [80524]: Basic port isolation on bridge CNI - [80525]: Mixed isolated/non-isolated ports - [81757]: UDN with port isolation (requires UDN feature) - [80526]: UDN with mixed isolation (requires UDN feature) Tests 80524 and 80525 pass on live cluster. Tests 81757 and 80526 require UDN feature to be enabled. All tests marked [informing] per requirements. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1. Fixed test naming format across all OTP test files:
- Removed [OTP][informing]/[blocking] labels from test names
- Changed format from "[OTP][informing][57589]" to "57589-"
- Follows pattern established in PR openshift#3200
- Lifecycle now controlled solely by main.go isOTPBlocking()
2. Removed OTP entries from tests.go:
- OTP tests discovered via import in main.go line 18
- Lifecycle set by isOTPBlocking() function (lines 200-203)
- tests.go is for upstream ovn-kubernetes tests only (EVPN, UDN)
- Removed 11 redundant OTP test entries
3. Removed duplicate OTP import from e2e_test.go:
- Import in main.go line 18 is sufficient for test discovery
- Duplicate was unnecessary
4. Fixed type assertion consistency in networking_tools.go:
- Line 474: Changed unsafe single-value assertion to safe two-value
- Now consistent with line 452 pattern
- Added explicit check: msgStr, ok := msg.(string)
…IPv6 exclude ranges - [76652]: Dummy CNI plugin support - [66876]: Whereabouts dual-stack IPAM - [69947]: Macvlan Unsolicited Neighbor Advertisements - [80524]: Basic port isolation with bridge CNI - [80525]: Mixed port isolation networks These tests were moved as they are pure Multus CNI tests without ovn-kubernetes dependencies. Total OTP tests now: 9 (2 security + 5 networking tools + 2 port isolation) Related: openshift/multus-cni#303
- 81757: Port isolation with UDN and bridge CNI - NOW PASSING
- 80526: UDN with mixed port isolation - NOW PASSING
- 69947: Macvlan Unsolicited NA - AWS skip logic (already working)
Tests Documented (require infrastructure changes):
- 67625: ovnkube-trace pod-to-pod - RBAC permissions needed
- 67648: ovnkube-trace pod-to-hostnet - RBAC permissions needed
Key Changes for UDN Tests:
1. IPv4-only subnet configuration (cluster doesn't support IPv6):
- Changed from dual-stack to single subnet: 10.132.0.0/16
- Matches scale team's working configuration
- Fixes: cluster does not support ipv6 controller errors
2. Correct UDN status condition check:
- Changed from NetworkReady to NetworkCreated
- This is the actual condition type used by UDN controller
- Fixes: 180-second timeout waiting for wrong condition
3. Added required namespace labels:
- k8s.ovn.org/primary-user-defined-network: layer2-ipv4-pudn1
- security.openshift.io/scc.podSecurityLabelSync: false
- Per Arti's guidance and scale team patterns
4. Increased timeout from 120s to 180s for slower clusters
RBAC Documentation (67625, 67648):
Added detailed FIXME comments explaining:
- ovnkube-trace needs pods/exec permission
- Current ClusterRole has pods/nodes get/list but missing pods/exec
- Cannot add temporary RBAC in tests (security risk in CI)
- Tests marked [informing] so won't block CI
- Requires team decision on proper RBAC solution
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Test validates ovn-get script functionality: - ovn-get -h: Verifies help shows leaders, dbs, mode commands - ovn-get leaders: Gets ovn-k master, nbdb, sbdb leaders - ovn-get mode: Checks if cluster runs in ovn-ic or legacy mode Manually validated commands on 4.22 cluster (Jul): - Help output shows correct commands - Leaders command shows ovn-k master leader - Mode command reports multi-zone (ovn-ic) Added execCommandInPod helper for pod exec operations. Per Anurag's request to add test 55890 to network tools. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…x ovnkube-trace RBAC
- 55890: Deploy network-tools pod from imagestream, run ovn-get via /opt/bin/network-tools wrapper
- 67625/67648: Create separate trace runner pod with temporary cluster-admin SA instead of
exec'ing into ovnkube-node pod (which has restricted SA lacking pods/list and pods/exec)
- All three tests create temporary RBAC (ClusterRoleBinding) and clean up in defer
07f5d86 to
a73e6bf
Compare
|
/retest |
|
/test e2e-metal-ipi-ovn-ipv6 |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
@SachinNinganure: 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. |
5 participants
Validates that ovnkube-node container logs do not expose API tokens
or Bearer authentication credentials.
Test searches ovnkube-controller container logs for sensitive patterns
(api-token, authorization, bearer) and filters false positives to ensure
no actual tokens are logged.
Description
Migrate OTP test case 49216 (API Token Logging Security) to OTE framework as part of the broader OTP-to-OTE migration effort.
Test Case: Medium-49216 - API Token Logging Security
Priority: Blocking (port=yes, >95% pass rate)
Lifecycle: blocking
Fixes
N/A (migration work, not fixing a specific issue)
Additional Information for reviewers
openshift/test/otp/networking_tools_test.go- First OTP networking tools testopenshift/test/otp/otp_suite_test.go- Ginkgo test suite registration for OTP package[sig-networking][OTP][blocking][case_id:49216]Checks
[ ] My code requires changes to the documentation → Leave UNCHECKED
[ ] if so, I have updated the documentation as required → Leave UNCHECKED
[x] My code requires tests → CHECK THIS (type x between the brackets)
[x] if so, I have added and/or updated the tests as required → CHECK THIS
[ ] All the tests have passed in the CI → Leave UNCHECKED (say "will verify once CI runs")
How to verify it
Test Type: End-to-end Ginkgo test
Local verification: