wip: CNTRLPLANE-3540: refactor(test/extended/tls): simplify the testing and remove duplication#31277
wip: CNTRLPLANE-3540: refactor(test/extended/tls): simplify the testing and remove duplication#31277ingvagabund wants to merge 22 commits into
Conversation
So the checks can be run in parallel without waiting for all CMs and operators to renoncile. No need to reconcile at the end if the check is only about observing CMs get restored to their original state.
… TLS connecion via getWireLevelTLSConfigs
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
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:
WalkthroughRefactors TLS observed-config tests to capture an expected tlsConfig from the live APIServer profile, thread that expected config through observedConfig, ConfigMap injection, deployment env-vars, and wire-level checks, centralize APIServer profile set/restore helpers, and add reconciliation polling and a disruptive reconciliation test. ChangesTLS Profile Test Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ingvagabund The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
…estWireLevelTLS to reduce duplicated invocation
|
Scheduling required tests: |
…neric testWireLevelTLS to have a single place with all settings where forwardPortAndExecute is invoked
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
openshift-ci
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
…onfig: first few steps of getting the current tls config, generating a new one and validating all the components are align with the current tls configuration
|
Job Failure Risk Analysis for sha: 3caf382
|
…onfig: change the TLS profile and wait until the ew TLS config is reconciled
ingvagabund
force-pushed
the
tls-refactoring
branch
from
b050cbe to
2ef7e93
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/tls/tls_observed_config.go`:
- Around line 503-537: This test uses the guest oc and hard-codes false into
verifyAllTLSConfiguration and waitForTLSReconciliation while reconciliation
helpers probe all ConfigMaps regardless of managementClusterComponent, which
breaks HyperShift; update the test to follow the existing
HostedCluster/HyperShift flow used by the Modern/Custom cases: when running on
HyperShift (detect via the same helper used elsewhere in this file), wire the
guest vs management client through the hosted-cluster path (pass the hosted
client or set the managementClusterComponent appropriately) into
generateDifferentTLSProfile/setAPIServerTLSProfile/verifyAllTLSConfiguration/waitForTLSReconciliation
instead of using the hard-coded oc and false, or alternatively call Skip for
HyperShift runs; ensure allTLSTestTargets are filtered per
managementClusterComponent when invoking the reconciliation helpers.
- Around line 503-545: This test mutates the cluster TLS profile but doesn't
restore it; after reading originalAPIServer (originalAPIServer and
originalProfile) register a g.DeferCleanup that calls setAPIServerTLSProfile to
apply originalProfile and then calls waitForTLSReconciliation to wait for
targets to return to the original config (use context.Background() in the
cleanup to avoid relying on configChangeCtx), and handle/expect no error so the
cluster is restored even if the test fails.
- Around line 1422-1424: In the tls.VersionTLS11 case update the "should work"
probe so it is clamped to exactly TLS 1.1 by setting MaxVersion:
tls.VersionTLS11 on the tls.Config assigned to tlsShouldWork (the current
assignment uses only MinVersion and allows higher TLS versions); modify the
tlsShouldWork assignment in the tls.VersionTLS11 branch to include MaxVersion:
tls.VersionTLS11 while leaving tlsShouldNotWork unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f937e07f-5927-4cfa-bb56-4a129631f25a
📒 Files selected for processing (1)
test/extended/tls/tls_observed_config.go
| case tls.VersionTLS11: | ||
| tlsShouldWork = &tls.Config{MinVersion: tls.VersionTLS11, InsecureSkipVerify: true} | ||
| tlsShouldNotWork = &tls.Config{MinVersion: tls.VersionTLS10, MaxVersion: tls.VersionTLS10, InsecureSkipVerify: true} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go's crypto/tls, if a client config sets MinVersion=tls.VersionTLS11 and leaves MaxVersion unset, can the handshake negotiate TLS 1.2 or TLS 1.3? To force exactly TLS 1.1, do both MinVersion and MaxVersion need to be tls.VersionTLS11?
💡 Result:
In Go's crypto/tls package, if a client config sets MinVersion to tls.VersionTLS11 and leaves MaxVersion unset (its zero value), the handshake can negotiate TLS 1.2 or TLS 1.3 [1][2][3]. When MaxVersion is unset (zero), it defaults to the maximum version supported by the package, which is currently TLS 1.3 [1][4][5]. The handshake process iterates through the package's supported versions—sorted from highest to lowest—and includes any version that is greater than or equal to MinVersion and less than or equal to MaxVersion (if set) [2][5][6]. Therefore, leaving MaxVersion unset does not restrict the client to the specified minimum version; it allows the negotiation of any supported protocol version equal to or higher than the minimum [2][5]. To force the client to use exactly TLS 1.1, you must set both MinVersion and MaxVersion to tls.VersionTLS11 [2][5][6]. This restricts the range of acceptable versions to that single value [2][5].
Citations:
- 1: https://pkg.go.dev/crypto/tls@go1.26.3
- 2: https://go.dev/src/crypto/tls/common.go?m=text
- 3: https://github.com/golang/go/blob/go1.26.2/src/crypto/tls/common.go
- 4: https://pkg.go.dev/crypto/tls
- 5: https://go.googlesource.com/go/+/refs/heads/master/src/crypto/tls/common.go
- 6: https://tip.golang.org/src/crypto/tls/common.go?m=text
Clamp the TLS 1.1 “should work” probe to exactly TLS 1.1.
In test/extended/tls/tls_observed_config.go (around lines 1422-1424), tls.Config{MinVersion: tls.VersionTLS11, ...} still permits negotiating TLS 1.2/1.3 when MaxVersion is left unset. Set MaxVersion: tls.VersionTLS11 too so the probe allows only TLS 1.1.
🧰 Tools
🪛 OpenGrep (1.22.0)
[ERROR] 1423-1423: 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] 1423-1423: 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] 1424-1424: 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] 1424-1424: 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/tls/tls_observed_config.go` around lines 1422 - 1424, In the
tls.VersionTLS11 case update the "should work" probe so it is clamped to exactly
TLS 1.1 by setting MaxVersion: tls.VersionTLS11 on the tls.Config assigned to
tlsShouldWork (the current assignment uses only MinVersion and allows higher TLS
versions); modify the tlsShouldWork assignment in the tls.VersionTLS11 branch to
include MaxVersion: tls.VersionTLS11 while leaving tlsShouldNotWork unchanged.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
test/extended/tls/tls_observed_config.go (3)
1424-1426:⚠️ Potential issue | 🟠 MajorClamp the TLS 1.1 success probe to exactly TLS 1.1.
Line 1425 sets only
MinVersion: tls.VersionTLS11. WithMaxVersionunset, the client can negotiate TLS 1.2/1.3, which can mask a TLS 1.1 regression.Suggested fix
- tlsShouldWork = &tls.Config{MinVersion: tls.VersionTLS11, InsecureSkipVerify: true} + tlsShouldWork = &tls.Config{MinVersion: tls.VersionTLS11, MaxVersion: tls.VersionTLS11, InsecureSkipVerify: true}In Go's crypto/tls, if tls.Config sets MinVersion=tls.VersionTLS11 and leaves MaxVersion unset, can negotiation still use TLS 1.2 or TLS 1.3?🤖 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/tls/tls_observed_config.go` around lines 1424 - 1426, The TLS 1.1 success probe currently only sets MinVersion in the case tls.VersionTLS11, allowing negotiation to upgrade to TLS 1.2/1.3; update the tlsShouldWork config used in that case to explicitly set MaxVersion: tls.VersionTLS11 (in addition to MinVersion: tls.VersionTLS11) so the probe is clamped to exactly TLS 1.1; locate the switch handling tls.VersionTLS11 (the case block that sets tlsShouldWork and tlsShouldNotWork) and add the MaxVersion field to tlsShouldWork.
515-547:⚠️ Potential issue | 🟠 MajorThis disruptive test still does not restore the original APIServer TLS profile.
You capture
originalProfileat Line 515, mutate the cluster profile at Line 533, but only leave a TODO at Line 545. This leaves cluster TLS state mutated after the test (especially on failure).Suggested fix
originalProfile := originalAPIServer.Spec.TLSSecurityProfile currentTLSConfig := captureTLSConfiguration(originalProfile) + +g.DeferCleanup(func(_ context.Context) { + cleanupCtx, cancel := context.WithTimeout(context.Background(), 60*time.Minute) + defer cancel() + restoreOriginalTLSProfile(oc, cleanupCtx, originalProfile, string(currentTLSConfig.profileType)) +})🤖 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/tls/tls_observed_config.go` around lines 515 - 547, The test mutates the APIServer TLS profile (via setAPIServerTLSProfile) but never restores originalAPIServer.Spec.TLSSecurityProfile on exit; add a cleanup that captures originalProfile before calling setAPIServerTLSProfile and registers a deferred restore (using the test harness DeferCleanup or a Go defer) that calls setAPIServerTLSProfile(oc, configChangeCtx, originalProfile, "<original-type>") and waits for reconciliation (e.g., waitForTLSReconciliation) so the cluster is returned to its prior state on success or failure; register the cleanup immediately after computing originalProfile (before generateDifferentTLSProfile / setAPIServerTLSProfile) and ensure errors from the restore are logged/fail the cleanup so failures are visible.
528-540:⚠️ Potential issue | 🟠 MajorHyperShift routing is still broken in the disruptive reconciliation flow.
Line 528 and Line 539 hard-code
falsewithallTLSTestTargets, and the ConfigMap loop (Line 1510 onward) still does not skipmanagementClusterComponenttargets. On HyperShift this can probe management-side targets via the guest client and fail against the wrong scope.Suggested fix
- verifyAllTLSConfiguration(oc, configChangeCtx, false, allTLSTestTargets) + selectedTargets := allTLSTestTargets + if isHyperShiftCluster { + selectedTargets = tlsTestTargets{ + observedConfig: guestObservedCfg, + configMaps: guestCMs, + deploymentEnvVars: guestEnvVars, + services: guestSvcs, + } + } + verifyAllTLSConfiguration(oc, configChangeCtx, isHyperShiftCluster, selectedTargets) ... - err = waitForTLSReconciliation(oc, configChangeCtx, false, allTLSTestTargets) + err = waitForTLSReconciliation(oc, configChangeCtx, isHyperShiftCluster, selectedTargets)for _, target := range targets.configMaps { + if isHyperShiftCluster && target.managementClusterComponent { + continue + } totalCount++ ... }Also applies to: 1510-1524
🤖 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/tls/tls_observed_config.go` around lines 528 - 540, The test is hard-coding the disruptive-mode boolean (false) when calling verifyAllTLSConfiguration and waitForTLSReconciliation with allTLSTestTargets and the ConfigMap loop still probes managementClusterComponent targets; update the calls to pass a variable that reflects the HyperShift context (e.g., disruptiveModeOrUseGuestClient boolean) instead of the literal false, and modify the ConfigMap processing loop that iterates targets (the code referencing managementClusterComponent) to skip managementClusterComponent targets when running under HyperShift/guest-client mode (use configChangeCtx or a new isHyperShift flag to decide), ensuring verifyAllTLSConfiguration(allTLSTestTargets, configChangeCtx, <flag>) and waitForTLSReconciliation(...) use that flag and the loop excludes managementClusterComponent targets accordingly.
🤖 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.
Duplicate comments:
In `@test/extended/tls/tls_observed_config.go`:
- Around line 1424-1426: The TLS 1.1 success probe currently only sets
MinVersion in the case tls.VersionTLS11, allowing negotiation to upgrade to TLS
1.2/1.3; update the tlsShouldWork config used in that case to explicitly set
MaxVersion: tls.VersionTLS11 (in addition to MinVersion: tls.VersionTLS11) so
the probe is clamped to exactly TLS 1.1; locate the switch handling
tls.VersionTLS11 (the case block that sets tlsShouldWork and tlsShouldNotWork)
and add the MaxVersion field to tlsShouldWork.
- Around line 515-547: The test mutates the APIServer TLS profile (via
setAPIServerTLSProfile) but never restores
originalAPIServer.Spec.TLSSecurityProfile on exit; add a cleanup that captures
originalProfile before calling setAPIServerTLSProfile and registers a deferred
restore (using the test harness DeferCleanup or a Go defer) that calls
setAPIServerTLSProfile(oc, configChangeCtx, originalProfile, "<original-type>")
and waits for reconciliation (e.g., waitForTLSReconciliation) so the cluster is
returned to its prior state on success or failure; register the cleanup
immediately after computing originalProfile (before generateDifferentTLSProfile
/ setAPIServerTLSProfile) and ensure errors from the restore are logged/fail the
cleanup so failures are visible.
- Around line 528-540: The test is hard-coding the disruptive-mode boolean
(false) when calling verifyAllTLSConfiguration and waitForTLSReconciliation with
allTLSTestTargets and the ConfigMap loop still probes managementClusterComponent
targets; update the calls to pass a variable that reflects the HyperShift
context (e.g., disruptiveModeOrUseGuestClient boolean) instead of the literal
false, and modify the ConfigMap processing loop that iterates targets (the code
referencing managementClusterComponent) to skip managementClusterComponent
targets when running under HyperShift/guest-client mode (use configChangeCtx or
a new isHyperShift flag to decide), ensuring
verifyAllTLSConfiguration(allTLSTestTargets, configChangeCtx, <flag>) and
waitForTLSReconciliation(...) use that flag and the loop excludes
managementClusterComponent targets accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c9190c70-6845-4e3a-87bd-77fa4b33f8da
📒 Files selected for processing (1)
test/extended/tls/tls_observed_config.go
|
Scheduling required tests: |
|
Job Failure Risk Analysis for sha: 2ef7e93
|
ingvagabund
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@ingvagabund: This pull request references CNTRLPLANE-3540 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Scheduling required tests: |
…st run in itw own cycle
… data instead of checking for substrings
…ation into modification and waiting
…on into modification and waiting
…ll CMs are processed
|
@ingvagabund: 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. |
2 participants
SSIA
Summary by CodeRabbit
Tests
Refactor