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 1 commit into
Conversation
|
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 |
![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: |
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 | |
| }) | |
| 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: |
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.
|
Scheduling required tests: |
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>
|
Scheduling required tests: |
3 similar comments
|
Scheduling required tests: |
|
Scheduling required tests: |
|
Scheduling required tests: |
|
/lgtm |
|
/test e2e-aws-tls-observed-config |
openshift-ci-robot
added
the
verified
|
@weliang1: This PR has been marked as verified by 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. |
openshift-ci-robot
removed
the
verified
| nextStepNum = "Step 8" | ||
| } | ||
| e2e.Logf("%s: Waiting %v for TLS configuration to propagate to component pods", nextStepNum, TLSConfigPropagationTimeout) | ||
| time.Sleep(TLSConfigPropagationTimeout) |
There was a problem hiding this comment.
This is fragile. A more robust approach would be to retry the port test until it succeeds (with a reasonably large timeout). This still could be coupled with a reasonable artificial delay. However, this could be done in a follow-up PR.
There was a problem hiding this comment.
This will cause flakes, and given how much of a problem we have had with test flakes never getting fixed and just causing problems for everyone forever, I feel that this needs to be fixed before it goes in.
You could either
- poll the CNO ClusterOperator status here, to observe it rolling out the updates and completing
- (more simply) just add a loop to
It("should verify TLS compliance for all networking components"and check multiple times until everything succeeds, waitingTLSConfigPropagationTimeoutin between.
|
/lgtm |
|
Scheduling required tests: |
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>
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 10bac56
New tests seen in this PR at sha: 10bac56
|
|
/verified by weliang |
openshift-ci-robot
added
the
verified
|
@weliang1: This PR has been marked as verified by 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. |
|
@danwinship both e2e-gcp-ovn-upgrade and e2e-aws-tls-observed-config are not required CI job, could you help to approve this PR. |
10bac56 to
40ad5f7
Compare
openshift-ci-robot
removed
the
verified
|
/verified by weliang |
openshift-ci-robot
added
the
verified
|
@weliang1: This PR has been marked as verified by 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tpantelis, 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 |
| stepNum = "Step 6" | ||
| if requiresMCPRollout { | ||
| stepNum = "Step 7" | ||
| } |
There was a problem hiding this comment.
This is going to be really annoying to keep in sync if anything in this test changes. It doesn't seem like the step numbers are really all that necessary in the log messages, so you could just remove them. If there's really some good reason for keeping them, then add a step variable at the top, and log "Step %d: blah blah blah", step, ... at each step, and do step++ between steps, and then the difference in counting will be accounted for automatically because you'll call step++ one extra time in the MCP rollout case.
| nextStepNum = "Step 8" | ||
| } | ||
| e2e.Logf("%s: Waiting %v for TLS configuration to propagate to component pods", nextStepNum, TLSConfigPropagationTimeout) | ||
| time.Sleep(TLSConfigPropagationTimeout) |
There was a problem hiding this comment.
This will cause flakes, and given how much of a problem we have had with test flakes never getting fixed and just causing problems for everyone forever, I feel that this needs to be fixed before it goes in.
You could either
- poll the CNO ClusterOperator status here, to observe it rolling out the updates and completing
- (more simply) just add a loop to
It("should verify TLS compliance for all networking components"and check multiple times until everything succeeds, waitingTLSConfigPropagationTimeoutin between.
| // Give port-forward time to establish the connection | ||
| time.Sleep(500 * time.Millisecond) |
There was a problem hiding this comment.
What is this? Doesn't the fact that ReadPartialFrom completed mean that the connection was already established?
| Config: tlsShouldWork, | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) |
There was a problem hiding this comment.
Isn't the context.WithTimeout redundant with the dialer.NetDialer.Timeout? (Likewise below in the negative case)
…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 - Networking components tested per profile: * multus-cni kube-rbac-proxy (port 9091) * cluster-network-operator metrics (port 9091) * networking-console plugin (port 9443) * ovn-kubernetes control-plane metrics (port 9108) * ovn-kubernetes node metrics (ports 9103, 9105) - Port-forward based TLS handshake verification using tls.Dial - Automatic cluster configuration and MCP rollout wait - Separate test cases for TLS 1.2 and TLS 1.3 where applicable Test behavior by profile and adherence policy: - Intermediate + LegacyAdheringComponentsOnly: Accept TLS 1.2 and 1.3 - Modern + LegacyAdheringComponentsOnly: Accept TLS 1.2 and 1.3 (legacy components) - Modern + StrictAllComponents: Enforce TLS 1.3 only, reject TLS 1.2 Implementation details: - Serial execution required due to cluster-wide TLS profile changes - Robust MCP rollout detection with retry logic - Component-specific port configurations matching actual deployments - Helper functions for TLS config, pod selection, and connection verification - Extended timeout (60s) for TLS handshake to handle slow environments - Suite tags: [sig-network][Feature:TLS][Serial] - Feature gate detection for proper test categorization Also update test/extended/util/tls.go: - Support port-forwarding to pods in addition to services - Increase connection timeout for TLS verification - Add pod namespace and name parameters to VerifyTLSConnection Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
40ad5f7 to
92513ca
Compare
openshift-ci-robot
removed
the
verified
|
New changes are detected. LGTM label has been removed. |
|
/verified by weliang |
openshift-ci-robot
added
the
verified
|
@weliang1: This PR has been marked as verified by 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. |
4 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