CORENET-7243: Add TLS Profile Compliance tests for networking components - #31500
CORENET-7243: Add TLS Profile Compliance tests for networking components#31500weliang1 wants to merge 4 commits into
Conversation
…onents Add comprehensive e2e tests to verify TLS compliance for OpenShift networking components (multus-cni, ovn-kubernetes, cluster-network-operator, networking-console) across different TLS profiles and adherence policies. Test coverage: - Three TLS profile configurations: * Intermediate + LegacyAdheringComponentsOnly * Modern + LegacyAdheringComponentsOnly * Modern + StrictAllComponents - Four networking components tested per profile (12 total test cases) - Port-forward based TLS handshake verification - Automatic cluster configuration and stabilization Also update test/extended/util/tls.go to support port-forwarding to pods in addition to services, and increase connection timeout for TLS verification. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@weliang1: GitHub didn't allow me to request PR reviews from the following users: weliang1, openshift/networking-qe. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
WalkthroughAdds OpenShift TLS adherence tests for API server profiles and networking components. The change adds feature-gate configuration, rollout and readiness checks, TLS compliance validation, and bounded port-forward execution. ChangesTLS adherence networking validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TLSAdherenceTest
participant FeatureGateAPI
participant APIServer
participant ClusterStatus
participant NetworkPod
participant TLSUtility
participant NetworkingComponent
TLSAdherenceTest->>FeatureGateAPI: Enable TLSAdherence
TLSAdherenceTest->>APIServer: Apply TLS profile and adherence policy
TLSAdherenceTest->>ClusterStatus: Wait for cluster stability and FeatureGate status
TLSAdherenceTest->>NetworkPod: Select running ready pod
TLSAdherenceTest->>TLSUtility: Forward component ports
TLSUtility->>NetworkingComponent: Check accepted and rejected TLS versions
Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: weliang1 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: 6
🧹 Nitpick comments (6)
test/extended/networking/tls.go (5)
545-563: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused
ocparameter and collapse the four wrappers.
verifyTLSComplianceInPodsnever usesoc, and the fourVerify*TLSComplianceInPodfunctions differ only in the port list and the display name. Remove the parameter and replace the wrappers with a single table in the spec body that holds namespace, selector, ports, and component name.🤖 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 `@test/extended/networking/tls.go` around lines 545 - 563, Remove the unused oc parameter from verifyTLSComplianceInPods and its callers, then replace VerifyMultusTLSComplianceInPod, VerifyOVNKubernetesTLSComplianceInPod, VerifyCNOTLSComplianceInPod, and VerifyNetworkConsoleTLSComplianceInPod with one table-driven specification in the relevant test body containing namespace, selector, ports, and component name. Iterate over the table while preserving each wrapper’s existing port list and display name.
311-362: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRetry on conflict when updating
FeatureGate/clusterandAPIServer/cluster.
patchFeatureGateandpatchAPIServerTLSProfilecallUpdatewith an object read earlier. If a controller writes the same object in between, the update fails with a conflict error and the whole spec fails. Wrap both updates inretry.RetryOnConflictwith a freshGetinside the retry function, or use a server-side apply/merge patch.♻️ Proposed pattern
return retry.RetryOnConflict(retry.DefaultRetry, func() error { cur, err := configClient.ConfigV1().APIServers().Get(ctx, "cluster", metav1.GetOptions{}) if err != nil { return err } // mutate cur ... _, err = configClient.ConfigV1().APIServers().Update(ctx, cur, metav1.UpdateOptions{}) return err })🤖 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 `@test/extended/networking/tls.go` around lines 311 - 362, Update patchFeatureGate and patchAPIServerTLSProfile to wrap their resource mutations and Update calls in retry.RetryOnConflict using retry.DefaultRetry. Fetch a fresh FeatureGate/cluster or APIServer/cluster inside each retry attempt, apply the existing changes to that object, and return update errors so conflicts are retried while preserving the current contextual error handling.
364-391: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse one deadline for the complete MCP rollout.
node.WaitForMCPappliestimeoutto each pool. With two pools, the 60-minute timeout can take up to 120 minutes. Compute one deadline before the loop and pass the remaining duration to each call. Retain the concrete client assertion becausenode.WaitForMCPrequires*machineconfigclient.Clientset.🤖 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 `@test/extended/networking/tls.go` around lines 364 - 391, Update waitForAllMCPsComplete to compute a single deadline before iterating over mcps, using the provided timeout from the current time. Before each node.WaitForMCP call, calculate the remaining duration and pass it instead of the full timeout, while preserving the concrete *machineconfigclient.Clientset assertion and existing MCP handling.
393-418: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace deprecated polling APIs and propagate cancellation.
Use
wait.PollUntilContextTimeoutin both helpers. Pass its callback context to each Kubernetes API call. ChangewaitForNodesStabilityto accept a context instead of creatingcontext.Background(). A spec cancellation requires passing a cancellable context throughConfigureTLSProfileWithAdherence, which currently usescontext.Background().The
nodeloop variable does not affect the current function because the imported package is not referenced there.🤖 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 `@test/extended/networking/tls.go` around lines 393 - 418, Replace the deprecated polling API in waitForNodesStability and the other related helper with wait.PollUntilContextTimeout, passing the callback context to each Kubernetes API call. Change waitForNodesStability to accept the caller’s context instead of creating context.Background(), and update ConfigureTLSProfileWithAdherence to create and propagate a cancellable context through these helpers so spec cancellation is honored.Source: Path instructions
27-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the TLS adherence helpers unexported.
These constants,
TLSAdherenceNotSupportedError, and the listed helper functions have no callers outsidetest/extended/networking/tls.go. Rename them to lowercase names to reduce the package export surface.🤖 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 `@test/extended/networking/tls.go` around lines 27 - 44, Rename the unused exported TLS adherence constants, TLSAdherenceNotSupportedError, and its helper functions in tls.go to lowercase names, including updating all references within the file. Preserve their existing values and behavior while reducing the package export surface.test/extended/util/tls.go (1)
56-59: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the fixed sleep with a readiness check on the local port.
A 500 ms sleep is a guess. On a loaded cluster the forward is not ready in time, and the callback fails for a reason unrelated to TLS. The sleep also consumes 5 percent of the 10 second command budget on every attempt.
Poll
net.Dial("tcp", "127.0.0.1:<localPort>")until it connects or a short deadline expires, then run the callback. This also detects the case wherelocalPortwas already bound by another process, whichrand.Intnat line 38 does not prevent.♻️ Proposed change
// Read and discard port-forward output to avoid logging sensitive cluster metadata _ = ReadPartialFrom(stdout, 1024) - // Give port-forward time to establish the connection before attempting TLS handshake - time.Sleep(500 * time.Millisecond) + // Wait until the forwarded local port accepts connections, so the callback + // does not fail for a reason unrelated to the TLS handshake. + if err := waitForLocalPort(ctx, localPort); err != nil { + return err + } return toExecute(localPort)// waitForLocalPort waits until the forwarded local port accepts TCP connections. func waitForLocalPort(ctx context.Context, localPort int) error { addr := fmt.Sprintf("127.0.0.1:%d", localPort) return wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, 5*time.Second, true, func(ctx context.Context) (bool, error) { conn, err := (&net.Dialer{}).DialContext(ctx, "tcp", addr) if err != nil { return false, nil } return true, conn.Close() }) }🤖 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 `@test/extended/util/tls.go` around lines 56 - 59, Replace the fixed 500 ms sleep in the port-forward setup with a readiness helper such as waitForLocalPort that polls 127.0.0.1:<localPort> using context-aware TCP dialing until connection succeeds or a short timeout expires. Close successful probe connections, propagate timeout errors, and invoke the TLS callback only after the local port is ready so an already-occupied port is detected.
🤖 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 `@test/extended/networking/tls.go`:
- Around line 70-79: Update the TLS profile suite around
ConfigureTLSProfileWithAdherence to capture the original APIServer/cluster
Spec.TLSSecurityProfile and Spec.TLSAdherence before the first mutation, then
register g.DeferCleanup to restore both fields and wait for the APIServer
rollout to complete. Confirm the suite runs only on disposable clusters because
patchFeatureGate permanently changes FeatureGate/cluster to CustomNoUpgrade, and
document that constraint in the spec.
- Line 72: Remove the redundant fmt.Sprintf wrapper from the g.Context call in
the profile description test, passing profile.description directly as the
context name. Clean up the fmt import if it becomes unused, and ensure
formatting and lint checks pass.
- Around line 133-145: Update IsOpenShiftCluster to treat only a NotFound or
IsNoMatchError from the FeatureGates retrieval as “not an OpenShift cluster”;
propagate other configuration-client or API retrieval errors so the calling spec
fails with the real cause instead of returning false. Adjust the helper’s
error-handling contract and callers as needed to preserve this distinction.
- Around line 517-527: Update the pod-selection loop around testPod so it
selects only a pod whose phase is Running and whose status conditions include
PodReady with a true status. Continue scanning other pods when the running pod
is not ready, and retain the existing no-running-pods error path when no
eligible pod is found.
- Around line 488-492: In the TLSProfileOldType branch, remove the
tlsShouldNotWork SSL 3.0 configuration and its associated negative
CheckTLSConnection coverage. Keep the TLS 1.0–1.3 positive configuration and
profile logging unchanged; do not use an unsupported Go TLS version for this
test.
In `@test/extended/util/tls.go`:
- Around line 36-45: Update CheckTLSConnection to separate port-forward startup
timeout from the command lifetime: use a startup context only to wait for
readiness, then keep the exec.CommandContext context active while toExecute runs
and cancel it afterward. Add explicit bounded timeouts to both tls.Dial calls so
blocked connections cannot outlive the callback or trigger unnecessary retries.
---
Nitpick comments:
In `@test/extended/networking/tls.go`:
- Around line 545-563: Remove the unused oc parameter from
verifyTLSComplianceInPods and its callers, then replace
VerifyMultusTLSComplianceInPod, VerifyOVNKubernetesTLSComplianceInPod,
VerifyCNOTLSComplianceInPod, and VerifyNetworkConsoleTLSComplianceInPod with one
table-driven specification in the relevant test body containing namespace,
selector, ports, and component name. Iterate over the table while preserving
each wrapper’s existing port list and display name.
- Around line 311-362: Update patchFeatureGate and patchAPIServerTLSProfile to
wrap their resource mutations and Update calls in retry.RetryOnConflict using
retry.DefaultRetry. Fetch a fresh FeatureGate/cluster or APIServer/cluster
inside each retry attempt, apply the existing changes to that object, and return
update errors so conflicts are retried while preserving the current contextual
error handling.
- Around line 364-391: Update waitForAllMCPsComplete to compute a single
deadline before iterating over mcps, using the provided timeout from the current
time. Before each node.WaitForMCP call, calculate the remaining duration and
pass it instead of the full timeout, while preserving the concrete
*machineconfigclient.Clientset assertion and existing MCP handling.
- Around line 393-418: Replace the deprecated polling API in
waitForNodesStability and the other related helper with
wait.PollUntilContextTimeout, passing the callback context to each Kubernetes
API call. Change waitForNodesStability to accept the caller’s context instead of
creating context.Background(), and update ConfigureTLSProfileWithAdherence to
create and propagate a cancellable context through these helpers so spec
cancellation is honored.
- Around line 27-44: Rename the unused exported TLS adherence constants,
TLSAdherenceNotSupportedError, and its helper functions in tls.go to lowercase
names, including updating all references within the file. Preserve their
existing values and behavior while reducing the package export surface.
In `@test/extended/util/tls.go`:
- Around line 56-59: Replace the fixed 500 ms sleep in the port-forward setup
with a readiness helper such as waitForLocalPort that polls
127.0.0.1:<localPort> using context-aware TCP dialing until connection succeeds
or a short timeout expires. Close successful probe connections, propagate
timeout errors, and invoke the TLS callback only after the local port is ready
so an already-occupied port is detected.
🪄 Autofix
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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: cf4e2af8-e310-457f-8cec-dd656ab535ec
📒 Files selected for processing (2)
test/extended/networking/tls.gotest/extended/util/tls.go
| for _, profile := range tlsProfiles { | ||
| profile := profile | ||
| g.Context(fmt.Sprintf("%s", profile.description), func() { | ||
| g.BeforeEach(func() { | ||
| err := ConfigureTLSProfileWithAdherence(oc, profile.profileType, profile.adherencePolicy) | ||
| if IsTLSAdherenceNotSupported(err) { | ||
| g.Skip(fmt.Sprintf("Skipping test - tlsAdherence API field not supported in this cluster version: %s", err.Error())) | ||
| } | ||
| o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("Failed to configure %s TLS profile with %s", profile.profileType, profile.adherencePolicy)) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Restore the APIServer TLS configuration after the tests.
ConfigureTLSProfileWithAdherence mutates the cluster-scoped APIServer/cluster object and never restores it. The last profile applied (Modern + StrictAllComponents) stays active for every test that runs after this suite on the same cluster. Add g.DeferCleanup (or an AfterEach) that captures the original Spec.TLSSecurityProfile and Spec.TLSAdherence before the first patch and restores them afterwards, including the rollout wait.
Note that patchFeatureGate switches FeatureGate/cluster to CustomNoUpgrade, which cannot be reverted. Confirm this suite only runs on disposable clusters, and document that constraint in the spec.
🤖 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 `@test/extended/networking/tls.go` around lines 70 - 79, Update the TLS profile
suite around ConfigureTLSProfileWithAdherence to capture the original
APIServer/cluster Spec.TLSSecurityProfile and Spec.TLSAdherence before the first
mutation, then register g.DeferCleanup to restore both fields and wait for the
APIServer rollout to complete. Confirm the suite runs only on disposable
clusters because patchFeatureGate permanently changes FeatureGate/cluster to
CustomNoUpgrade, and document that constraint in the spec.
There was a problem hiding this comment.
The added documentation now makes it explicit that this test has cluster-wide side effects and should only run in the appropriate context, which addresses CodeRabbit's stability concern.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/extended/networking/tls.go"
ast-grep outline "$file" --items all --type function,method --match 'ConfigureTLSProfileWithAdherence|patchFeatureGate|Restore|cleanup|TLS'
printf '\n--- Relevant declarations and uses ---\n'
rg -n -C 5 -P '\b(ConfigureTLSProfileWithAdherence|patchFeatureGate|DeferCleanup|AfterEach|TLSSecurityProfile|TLSAdherence|CustomNoUpgrade|disposable|cleanup|rollout)\b' "$file"
printf '\n--- Setup section ---\n'
sed -n '1,180p' "$file"Length of output: 17939
@weliang1, the documentation addresses the FeatureGate/cluster constraint.
The APIServer/cluster concern remains unresolved. ConfigureTLSProfileWithAdherence still updates Spec.TLSSecurityProfile and Spec.TLSAdherence for each test. The current file has no g.DeferCleanup or AfterEach to restore these fields.
Please retain this finding until the suite captures and restores the original APIServer configuration, including the required rollout wait.
🧠 Learnings used
Learnt from: ngopalak-redhat
Repo: openshift/origin PR: 31339
File: test/extended/node/node_utils.go:304-306
Timestamp: 2026-06-25T02:59:24.710Z
Learning: In openshift/origin Go test helper code under test/extended/, avoid switching to context.Background() for deferred cleanup that must run after the spec/test context is canceled. Instead, detach cancellation from the existing context (e.g., ctxNoCancel := context.WithoutCancel(ctx)) to preserve context values, then apply an explicit timeout to keep cleanup bounded (e.g., context.WithTimeout(ctxNoCancel, ...)) so cleanup can complete safely without relying on an uncanceled root context.
You are interacting with an AI system.
weliang1
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@weliang1: This pull request references CORENET-7243 which is a valid jira issue. 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. |
|
Scheduling required tests: |
|
@weliang1: 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 commit addresses all CodeRabbit review comments: 1. Add disposable cluster requirement documentation - Document irreversible cluster state modifications - Clarify FeatureGate CustomNoUpgrade cannot be reverted - Explain [Serial] and [OCPFeatureGate:TLSAdherence] markers 2. Remove redundant fmt.Sprintf wrapper - Pass profile.description directly to g.Context() 3. Fix error handling in IsOpenShiftCluster() - Return (bool, error) instead of just bool - Distinguish NotFound/NoMatchError from real API failures - Propagate real errors instead of silent skip 4. Remove invalid SSLv3 negative test - SSLv3 removed from Go 1.14+ (using Go 1.26.5) - Set tlsShouldNotWork=nil for Old profile - Update CheckTLSConnection to skip negative test when nil 5. Select pods that are Ready, not just Running - Check PodReady condition in addition to Running phase - Prevents flaky failures from containers not yet listening 6. Fix port-forward race condition (CRITICAL) - Separate startup timeout from command lifetime - Keep port-forward alive during callback execution - Add explicit timeouts to TLS dials (5s TCP + 10s total) - Prevents "connection refused" false negatives Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/extended/util/tls.go`:
- Around line 58-76: Update the port-forward startup goroutine around
ReadPartialFrom and startupDone to verify the expected readiness message instead
of treating any completed stdout read as success. Capture startup output and
read errors, detect early process exit or missing readiness, and return the
relevant output or failure through the existing retry flow before invoking
toExecute.
🪄 Autofix
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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1bf91a0a-f723-41d7-9df8-df9fcc28dd72
📒 Files selected for processing (2)
test/extended/networking/tls.gotest/extended/util/tls.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/extended/networking/tls.go
| // Wait for port-forward to establish with a startup timeout | ||
| startupCtx, startupCancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| defer startupCancel() | ||
| startupDone := make(chan struct{}) | ||
| go func() { | ||
| // Read and discard port-forward output to avoid logging sensitive cluster metadata | ||
| _ = ReadPartialFrom(stdout, 1024) | ||
| // Give port-forward time to establish the connection | ||
| time.Sleep(500 * time.Millisecond) | ||
| close(startupDone) | ||
| }() | ||
| select { | ||
| case <-startupDone: | ||
| // Port-forward ready, proceed with callback | ||
| case <-startupCtx.Done(): | ||
| return fmt.Errorf("port-forward startup timeout after 10s") | ||
| } | ||
|
|
||
| // Execute callback with port-forward kept alive |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Verify port-forward readiness before the callback.
Line 64 treats any completed stdout read as readiness. ReadPartialFrom also returns after EOF or a read error. The code then waits 500 ms and invokes toExecute, even when oc port-forward exited or did not create a local listener.
Wait for the expected port-forward readiness message and return startup output or process failures to the retry loop.
🤖 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 `@test/extended/util/tls.go` around lines 58 - 76, Update the port-forward
startup goroutine around ReadPartialFrom and startupDone to verify the expected
readiness message instead of treating any completed stdout read as success.
Capture startup output and read errors, detect early process exit or missing
readiness, and return the relevant output or failure through the existing retry
flow before invoking toExecute.
| g.By(fmt.Sprintf("Testing TLS compliance for networking-console-plugin in %s (port 9443)", namespace)) | ||
| err := VerifyNetworkConsoleTLSComplianceInPod(oc, configClient, k8sClient, namespace, labelSelector) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "TLS compliance verification failed") | ||
| }) |
There was a problem hiding this comment.
There's 4 It specs and each will run the expensive setup process which is wasteful in an e2e test. I suggest combining the checks for each component in a single It spec.
| } | ||
| e2e.Logf("APIServer TLS profile configured successfully") | ||
|
|
||
| requiresMCPRollout := (tlsProfileType == "Modern" && (tlsAdherencePolicy == "LegacyAdheringComponentsOnly" || tlsAdherencePolicy == "StrictAllComponents")) |
There was a problem hiding this comment.
Use the constants defined in configv1.
| profileType string | ||
| adherencePolicy string |
There was a problem hiding this comment.
Use the constants defined in configv1.
| } | ||
|
|
||
| var tlsShouldWork, tlsShouldNotWork *tls.Config | ||
| profileType := "Intermediate" |
There was a problem hiding this comment.
This is unnecessary - use apiserver.Spec.TLSSecurityProfile.Type.
| func waitForNodesStability(client kubernetes.Interface, timeout time.Duration) error { | ||
| ctx := context.Background() | ||
|
|
||
| return wait.PollImmediate(30*time.Second, timeout, func() (bool, error) { |
There was a problem hiding this comment.
PollImmediate is deprecated, use PollUntilContextTimeout
instead.
| var _ = g.Describe("[sig-network][OCPFeatureGate:TLSAdherence][Serial]", func() { | ||
| defer g.GinkgoRecover() | ||
|
|
||
| oc := exutil.NewCLIWithoutNamespace("multus-tls") |
There was a problem hiding this comment.
The project name is "multus-tls" but it tests all networking components so perhaps "networking-tls" or "tls-compliance".
|
|
||
| var testPod string | ||
| for _, pod := range pods.Items { | ||
| if pod.Status.Phase == corev1.PodRunning { |
There was a problem hiding this comment.
This only checks pod.Status.Phase == corev1.PodRunning, which is insufficient for ensuring the pod is actually ready to accept connections. It should also check if pod.Status.Conditions includes Ready=True:
slices.ContainsFunc(pod.Status.Conditions, func(condition corev1.PodCondition) bool {
return condition.Type == corev1.PodReady && condition.Status == corev1.ConditionTrue
})
|
|
||
| e2e.Logf("Verifying TLSAdherence is active for cluster version %s", version) | ||
|
|
||
| return wait.PollImmediate(15*time.Second, 15*time.Minute, func() (bool, error) { |
There was a problem hiding this comment.
PollImmediate is deprecated, use PollUntilContextTimeout
instead.
| for _, condition := range node.Status.Conditions { | ||
| if condition.Type == corev1.NodeReady && condition.Status == corev1.ConditionTrue { | ||
| return true | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
This could be simplified to:
| for _, condition := range node.Status.Conditions { | |
| if condition.Type == corev1.NodeReady && condition.Status == corev1.ConditionTrue { | |
| return true | |
| } | |
| } | |
| return false | |
| return slices.ContainsFunc(node.Status.Conditions, func(condition corev1.NodeCondition) bool { | |
| return condition.Type == corev1.NodeReady && condition.Status == corev1.ConditionTrue | |
| }) | |
Address review feedback from tpantelis: combine the 4 separate It specs (multus, ovn, cno, console) into a single It spec per profile context. Benefits: - BeforeEach (expensive setup) now runs once per profile instead of 4x - Cleaner test structure - Still maintains test granularity via g.By() steps - More explicit that setup runs once Each component is still tested separately with clear g.By() messages, so failures are easy to diagnose while avoiding redundant setup overhead. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Use configv1 types for struct fields (TLSProfileType, TLSAdherencePolicy) - Replace hardcoded strings with configv1 constants throughout - Remove unnecessary profileType variable - Replace deprecated wait.PollImmediate with wait.PollUntilContextTimeout - Simplify Ready condition checks using slices.ContainsFunc - Rename CLI from "multus-tls" to "networking-tls" Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
3 participants
Summary
Add comprehensive e2e tests to verify TLS compliance for OpenShift networking components across different TLS profiles and adherence policies.
Components Tested
Test Coverage
This PR adds 12 e2e test cases covering three TLS profile configurations:
Each configuration tests all four networking components (4 × 3 = 12 tests total).
Test Methodology
Changes
New Files
test/extended/networking/tls.go- Main test implementation (563 lines)Modified Files
test/extended/util/tls.go- Enhanced port-forwarding utilities:Test Execution
Tests are marked with:
[sig-network]- Networking SIG ownership[OCPFeatureGate:TLSAdherence]- Requires TLSAdherence feature gate[Serial]- Must run sequentially (cluster-wide TLS configuration changes)Run with:
./openshift-tests run all --run="TLS Profile Compliance"Validation
make buildgofmt/cc @weliang1 @openshift/networking-qe
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes