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 17 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds 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 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 rollout, readiness, and FeatureGate status
TLSAdherenceTest->>NetworkPod: Select running ready pod
TLSAdherenceTest->>TLSUtility: Forward component ports
TLSUtility->>NetworkingComponent: Check accepted and rejected TLS versions
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: |
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>
| return err == nil, err | ||
| } | ||
|
|
||
| func ConfigureTLSProfileWithAdherence(oc *exutil.CLI, tlsProfileType string, tlsAdherencePolicy string) error { |
There was a problem hiding this comment.
Use the configv1 type.
| func ConfigureTLSProfileWithAdherence(oc *exutil.CLI, tlsProfileType string, tlsAdherencePolicy string) error { | |
| func ConfigureTLSProfileWithAdherence(oc *exutil.CLI, tlsProfileType configv1.TLSProfileType, tlsAdherencePolicy sconfigv1.TLSAdherencePolicy) error { |
| var expectedProfileType configv1.TLSProfileType | ||
| switch tlsProfileType { | ||
| case string(configv1.TLSProfileModernType): | ||
| expectedProfileType = configv1.TLSProfileModernType | ||
| case string(configv1.TLSProfileIntermediateType): | ||
| expectedProfileType = configv1.TLSProfileIntermediateType | ||
| case string(configv1.TLSProfileOldType): | ||
| expectedProfileType = configv1.TLSProfileOldType | ||
| default: | ||
| return fmt.Errorf("unsupported TLS profile type: %s", tlsProfileType) | ||
| } |
There was a problem hiding this comment.
expectedProfileType isn't needed - use tlsProfileType directly.
| var expectedProfileType configv1.TLSProfileType | |
| switch tlsProfileType { | |
| case string(configv1.TLSProfileModernType): | |
| expectedProfileType = configv1.TLSProfileModernType | |
| case string(configv1.TLSProfileIntermediateType): | |
| expectedProfileType = configv1.TLSProfileIntermediateType | |
| case string(configv1.TLSProfileOldType): | |
| expectedProfileType = configv1.TLSProfileOldType | |
| default: | |
| return fmt.Errorf("unsupported TLS profile type: %s", tlsProfileType) | |
| } |
| o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("Failed to configure %s TLS profile with %s", profile.profileType, profile.adherencePolicy)) | ||
| }) | ||
|
|
||
| g.It("should verify TLS compliance for all networking components", func() { |
There was a problem hiding this comment.
The It func signature can accept a context.Context parameter. We can then thread the context from here through call stacks rather than functions using context.Background(). Same with the BeforeEach on line 87.
| g.It("should verify TLS compliance for all networking components", func() { | |
| g.It("should verify TLS compliance for all networking components", func(ctx context.Context) { |
This commit addresses all review feedback from PR openshift#766: 1. Combine wasteful test setup (tpantelis) - Merge 3 separate It specs into single It spec - Reduces test time by ~2-4 hours (avoids redundant MCP rollouts) 2. Use configv1 typed constants (tpantelis) - Replace string literals with configv1.TLSProfileType - Replace string literals with configv1.TLSAdherencePolicy - Update all function signatures and comparisons 3. Fix pod readiness check (tpantelis) - Use podutil.IsPodReady() instead of only checking Phase==Running - Prevents race conditions by ensuring pod is actually ready 4. Simplify node readiness check (tpantelis) - Use slices.ContainsFunc() for cleaner code 5. Fix step numbering (tpantelis) - Renumber steps to start from 1 instead of 2 6. Use errors.As() for error type checking (tpantelis) - Replace type assertion with errors.As() - Future-proof for wrapped errors 7. Rename variable for clarity (tpantelis) - Rename featureGateEnabled to alreadyEnabled 8. Add [OCPFeatureGate:TLSAdherence][Serial] tags (CodeRabbit) - Ensures tests run on dedicated, disposable CI infrastructure - Update documentation to match openshift/origin#31500 pattern Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
| {configv1.TLSProfileIntermediateType, configv1.TLSAdherencePolicyLegacyAdheringComponentsOnly, "Intermediate TLS Profile with LegacyAdheringComponentsOnly"}, | ||
| {configv1.TLSProfileModernType, configv1.TLSAdherencePolicyLegacyAdheringComponentsOnly, "Modern TLS Profile with LegacyAdheringComponentsOnly"}, | ||
| {configv1.TLSProfileModernType, configv1.TLSAdherencePolicyStrictAllComponents, "Modern TLS Profile with StrictAllComponents"}, | ||
| } |
There was a problem hiding this comment.
I believe the revised profile sequence we discussed was:
- "Modern TLS Profile with LegacyAdheringComponentsOnly" (tests baseline - profile not honored)
- "Modern TLS Profile with StrictAllComponents" (tests TLSAdherence change)
- "Intermediate Profile with StrictAllComponents" (tests TLSProfile change)
|
Scheduling required tests: |
Replace Intermediate+LegacyAdheringComponentsOnly with Intermediate+StrictAllComponents test case to align with new TLS compliance requirements. Changes: - Remove test case: Intermediate TLS Profile with LegacyAdheringComponentsOnly - Add test case: Intermediate TLS Profile with StrictAllComponents - Update MCP rollout logic to handle Intermediate+StrictAllComponents - Enhance Intermediate profile verification to test both TLS 1.2 and TLS 1.3 independently (positive tests only, no rejection tests) Test execution order: 1. Modern + LegacyAdheringComponentsOnly 2. Modern + StrictAllComponents 3. Intermediate + StrictAllComponents 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/networking/tls.go`:
- Around line 531-550: Update the Intermediate-profile branch in the TLS test so
the TLS 1.2 CheckTLSConnection call passes tlsShouldNotWork, preserving the TLS
1.1 rejection assertion. Keep the independent TLS 1.3 handshake without a
negative configuration, and retain the existing error handling and early return.
🪄 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: b34a34e3-65bd-473f-ac41-b26d2a560a2c
📒 Files selected for processing (1)
test/extended/networking/tls.go
| // For Intermediate profile, test both TLS 1.2 and TLS 1.3 separately to ensure both work | ||
| if apiserver.Spec.TLSSecurityProfile != nil && apiserver.Spec.TLSSecurityProfile.Type == configv1.TLSProfileIntermediateType { | ||
| // Test TLS 1.2 specifically | ||
| tls12Config := &tls.Config{MinVersion: tls.VersionTLS12, MaxVersion: tls.VersionTLS12, InsecureSkipVerify: true} | ||
| e2e.Logf("Testing TLS 1.2 on port %s", port) | ||
| if err := exutil.CheckTLSConnection(localPort, tls12Config, nil); err != nil { | ||
| return fmt.Errorf("TLS 1.2 test failed: %w", err) | ||
| } | ||
|
|
||
| // Test TLS 1.3 specifically | ||
| tls13Config := &tls.Config{MinVersion: tls.VersionTLS13, MaxVersion: tls.VersionTLS13, InsecureSkipVerify: true} | ||
| e2e.Logf("Testing TLS 1.3 on port %s", port) | ||
| if err := exutil.CheckTLSConnection(localPort, tls13Config, nil); err != nil { | ||
| return fmt.Errorf("TLS 1.3 test failed: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
| // For other profiles, use the standard test | ||
| return exutil.CheckTLSConnection(localPort, tlsShouldWork, tlsShouldNotWork) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the TLS 1.1 rejection check for the Intermediate profile.
The early return bypasses tlsShouldNotWork. The test passes when TLS 1.2 and TLS 1.3 work, even if the endpoint also accepts TLS 1.1.
Run the TLS 1.2 handshake with tlsShouldNotWork, then run the independent TLS 1.3 handshake without a negative configuration.
Proposed fix
- if err := exutil.CheckTLSConnection(localPort, tls12Config, nil); err != nil {
+ if err := exutil.CheckTLSConnection(localPort, tls12Config, tlsShouldNotWork); err != nil {
return fmt.Errorf("TLS 1.2 test failed: %w", err)
}📝 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.
| // For Intermediate profile, test both TLS 1.2 and TLS 1.3 separately to ensure both work | |
| if apiserver.Spec.TLSSecurityProfile != nil && apiserver.Spec.TLSSecurityProfile.Type == configv1.TLSProfileIntermediateType { | |
| // Test TLS 1.2 specifically | |
| tls12Config := &tls.Config{MinVersion: tls.VersionTLS12, MaxVersion: tls.VersionTLS12, InsecureSkipVerify: true} | |
| e2e.Logf("Testing TLS 1.2 on port %s", port) | |
| if err := exutil.CheckTLSConnection(localPort, tls12Config, nil); err != nil { | |
| return fmt.Errorf("TLS 1.2 test failed: %w", err) | |
| } | |
| // Test TLS 1.3 specifically | |
| tls13Config := &tls.Config{MinVersion: tls.VersionTLS13, MaxVersion: tls.VersionTLS13, InsecureSkipVerify: true} | |
| e2e.Logf("Testing TLS 1.3 on port %s", port) | |
| if err := exutil.CheckTLSConnection(localPort, tls13Config, nil); err != nil { | |
| return fmt.Errorf("TLS 1.3 test failed: %w", err) | |
| } | |
| return nil | |
| } | |
| // For other profiles, use the standard test | |
| return exutil.CheckTLSConnection(localPort, tlsShouldWork, tlsShouldNotWork) | |
| // For Intermediate profile, test both TLS 1.2 and TLS 1.3 separately to ensure both work | |
| if apiserver.Spec.TLSSecurityProfile != nil && apiserver.Spec.TLSSecurityProfile.Type == configv1.TLSProfileIntermediateType { | |
| // Test TLS 1.2 specifically | |
| tls12Config := &tls.Config{MinVersion: tls.VersionTLS12, MaxVersion: tls.VersionTLS12, InsecureSkipVerify: true} | |
| e2e.Logf("Testing TLS 1.2 on port %s", port) | |
| if err := exutil.CheckTLSConnection(localPort, tls12Config, tlsShouldNotWork); err != nil { | |
| return fmt.Errorf("TLS 1.2 test failed: %w", err) | |
| } | |
| // Test TLS 1.3 specifically | |
| tls13Config := &tls.Config{MinVersion: tls.VersionTLS13, MaxVersion: tls.VersionTLS13, InsecureSkipVerify: true} | |
| e2e.Logf("Testing TLS 1.3 on port %s", port) | |
| if err := exutil.CheckTLSConnection(localPort, tls13Config, nil); err != nil { | |
| return fmt.Errorf("TLS 1.3 test failed: %w", err) | |
| } | |
| return nil | |
| } | |
| // For other profiles, use the standard test | |
| return exutil.CheckTLSConnection(localPort, tlsShouldWork, tlsShouldNotWork) |
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 533-533: TLS certificate verification is disabled by setting InsecureSkipVerify: true on the tls.Config. This makes the connection vulnerable to man-in-the-middle attacks because the server's certificate chain and host name are not validated. Remove InsecureSkipVerify (or set it to false) and provide a proper RootCAs pool to trust custom certificates instead.
Context: tls.Config{MinVersion: tls.VersionTLS12, MaxVersion: tls.VersionTLS12, InsecureSkipVerify: true}
Note: [CWE-295] Improper Certificate Validation.
(tls-insecure-skip-verify-go)
[warning] 540-540: TLS certificate verification is disabled by setting InsecureSkipVerify: true on the tls.Config. This makes the connection vulnerable to man-in-the-middle attacks because the server's certificate chain and host name are not validated. Remove InsecureSkipVerify (or set it to false) and provide a proper RootCAs pool to trust custom certificates instead.
Context: tls.Config{MinVersion: tls.VersionTLS13, MaxVersion: tls.VersionTLS13, InsecureSkipVerify: true}
Note: [CWE-295] Improper Certificate Validation.
(tls-insecure-skip-verify-go)
🪛 OpenGrep (1.26.0)
[ERROR] 534-534: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 534-534: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 541-541: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 541-541: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
🤖 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 531 - 550, Update the
Intermediate-profile branch in the TLS test so the TLS 1.2 CheckTLSConnection
call passes tlsShouldNotWork, preserving the TLS 1.1 rejection assertion. Keep
the independent TLS 1.3 handshake without a negative configuration, and retain
the existing error handling and early return.
…uration Address review feedback from tpantelis: use strongly-typed configv1 types instead of strings for TLS profile and adherence policy parameters. Benefits: - Type safety with compile-time checking - No runtime string conversions - Self-documenting function signatures - Cleaner code (-13 lines) Changes: - ConfigureTLSProfileWithAdherence: accept typed parameters - patchAPIServerTLSProfile: accept typed parameters - Remove switch statement converting strings back to types - Remove string() conversions at call sites and comparisons - Direct type comparisons instead of string comparisons Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Scheduling required tests: |
This commit addresses multiple issues found during TLS compliance testing: 1. Fix WaitForMCP to handle transient API errors gracefully - Add isTransientAPIError() helper to identify retriable errors - Retry on 504 Gateway Timeout, 503 Service Unavailable, 500 Internal Error - Prevents test failures due to temporary API server overload during config changes - Location: test/extended/node/node_utils.go 2. Fix OVN-Kubernetes port configuration - Remove non-existent port 9443 from OVN tests - Split into control-plane (port 9108) and node tests (ports 9103, 9105) - Matches actual OVN deployment configuration - Location: test/extended/networking/tls.go 3. Correct TLS adherence policy expectations - Modern + StrictAllComponents: Enforce TLS 1.3 only (reject TLS 1.2) - Modern + LegacyAdheringComponentsOnly: Accept both TLS 1.2 and 1.3 - Intermediate + StrictAllComponents: Accept both TLS 1.2 and 1.3 - Remove unnecessary TLS 1.1 negative tests - Location: test/extended/networking/tls.go These fixes ensure the TLS compliance tests accurately verify the expected behavior of TLS profile enforcement based on the configured adherence policy.
Address review feedback from tpantelis: use Ginkgo's context parameter instead of creating context.Background() instances throughout the code. Benefits: - Consistent with origin codebase patterns (network_diagnostics.go, node tests) - Proper context lifecycle management via Ginkgo - Automatic cancellation on test timeout - Better timeout propagation through call stack Changes: - Update g.BeforeEach and g.It to accept context.Context parameter - Thread context through all 10 functions in the call stack - Remove 5 context.Background() calls - Update ~15 call sites to pass context - Cleaner code (-7 lines: 31 insertions, 38 deletions) Fixes: openshift#31500 (comment) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Remove [OCPFeatureGate:TLSAdherence] label from TLS compliance tests to fix test filtering issue in CI. The cluster-state filter was excluding these tests even when TLSAdherence was enabled, resulting in "no tests to run" errors. The test already has robust logic to automatically enable TLSAdherence feature gate if not already enabled, following the pattern from ingress-node-firewall PR openshift#766. This includes: - Checking if TLSAdherence is enabled - Enabling the feature gate if needed - Waiting for MCP rollout, node stability, and operator settlement - Verifying feature gate is active before running tests Impact: - Tests now discoverable via openshift/conformance/serial suite - All 3 test cases (Modern+Legacy, Modern+Strict, Intermediate+Strict) now appear in test discovery - CI will successfully run tests without manual feature gate setup Before: cluster-state filter removed test (0 tests to run) After: All 3 tests kept and assigned to serial conformance suite Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Remove overly cautious "disposable cluster" warning now that test is part of the standard openshift/conformance/serial suite (not gated by OCPFeatureGate label). The test still makes cluster-wide configuration changes and runs serially, but this is normal for serial conformance tests. The scary warning about "MUST NOT run on shared infrastructure" was only appropriate when it had the OCPFeatureGate label. Simplified comment now just documents: - Makes cluster-wide configuration changes - Auto-enables TLSAdherence feature gate if needed - Runs serially to avoid interference Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Decided to keep just [Serial] label instead of adding [Disruptive] because: 1. These tests verify TLS compliance - a conformance requirement 2. [Serial] is sufficient to prevent interference with concurrent tests 3. Adding [Disruptive] would exclude tests from openshift/conformance/serial 4. Tests would become orphaned without a dedicated suite definition While the tests do make cluster-wide changes (enable feature gate, modify APIServer config, trigger MCP rollouts), this is similar to other conformance tests. The [Serial] label handles the concurrency concern. Verified: All 3 test cases appear in openshift/conformance/serial suite Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…irewall PR openshift#766 Add missing MCP rollout handling functions to make test more robust when enabling feature gates and applying TLS profile configurations. Following the proven pattern from openshift/ingress-node-firewall PR openshift#766. New functions: - waitForMCPRolloutStart(): Waits for MCP "Updating" condition before checking completion, avoiding race where we check status before rollout begins - areAllMCPsComplete(): Checks if MCPs are already stable before waiting, avoiding unnecessary waits Flow improvements: 1. After enabling TLSAdherence feature gate: - Wait for MCP rollout to START (10 min timeout) - If started, wait for completion (60 min timeout) - Prevents false positives from checking before rollout begins 2. After applying TLS profile configuration: - Check if MCPs are already complete - If not complete, wait for rollout start then completion - Avoids redundant waits when MCPs are already stable This eliminates race conditions where we might check MCP status too early before the rollout has begun, leading to false positives. Reference: openshift/ingress-node-firewall#766 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add explicit port numbers (9104, 9103, 9105) to the cluster-network-operator TLS test description for consistency with other component test descriptions. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Scheduling required tests: |
Changes: 1. Remove OVN-Kubernetes control plane TLS verification step (port 9108) - Test now covers 4 components instead of 5 - Keeps: Multus, OVN nodes, CNO, Network Console 2. Simplify MCP rollout waiting logic - Remove areAllMCPsComplete pre-check - Always wait for rollout to start and complete when required - Fixes Modern+LegacyAdheringComponentsOnly requiring MCP rollout - Modern+StrictAllComponents does not require MCP rollout Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Scheduling required tests: |
Update TLS profile compliance tests to use proper suite categorization: - Add [Suite:openshift/tls-observed-config] for TLS compliance suite integration - Add [OCPFeatureGate:TLSAdherence] to skip on unsupported clusters This ensures tests run in the correct CI context with appropriate timeouts (90 minutes) and only on clusters with TLSAdherence feature gate enabled. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…tations
Update TLS compliance tests to always test both TLS 1.2 and 1.3 separately
with correct expectations based on profile, adherence policy, and component type.
Changes:
- Add isAdheringComponent parameter to distinguish between adhering (Multus)
and non-adhering (OVN, CNO, Network Console) components
- Always test both TLS 1.2 and 1.3 explicitly instead of relying on version
negotiation during handshake
- Apply conditional expectations:
* Modern + StrictAllComponents: All components reject TLS 1.2
* Modern + LegacyAdheringComponentsOnly: Multus rejects TLS 1.2,
other components accept both TLS 1.2 and 1.3
* Intermediate + StrictAllComponents: All components accept both versions
- Add detailed logging for test expectations and results
This ensures proper validation of TLS compliance across all test scenarios
and verifies backward compatibility for legacy components.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Scheduling required tests: |
… tag Increase MCP rollout and node stability timeouts to allow sufficient time for full cluster rollout during TLS profile configuration: - MCPRolloutCompleteTimeout: 60m → 120m - NodeStabilityTimeout: 60m → 90m With 6 nodes (3 master + 3 worker), each requiring 15-20 minutes to reboot and stabilize, the previous 60-minute timeout was insufficient. Also remove [OCPFeatureGate:TLSAdherence] tag to fix test filtering issue. The cluster-state filter was excluding these tests even when TLSAdherence was enabled, resulting in "no tests to run" errors. Tests are now properly included in the openshift/tls-observed-config suite and can be executed successfully.
|
Scheduling required tests: |
|
@weliang1: The following test 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. |
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