CNTRLPLANE-3423: TLS config tests for console-operator - #31436
CNTRLPLANE-3423: TLS config tests for console-operator#31436kaleemsiddiqu wants to merge 1 commit into
Conversation
Signed-off-by: Kaleemullah Siddiqui <ksiddiqu@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@kaleemsiddiqu: This pull request references CNTRLPLANE-3423 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaleemsiddiqu 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
WalkthroughAdded console-operator TLS observed-configuration tests and disruptive console-route checks covering Intermediate and Modern TLS profiles, with helpers for route target discovery, TLS dialing, reconciliation polling, and profile restoration. ChangesConsole operator TLS validation
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant APIServer
participant console-operator
participant ConsoleRouteTLSClient
APIServer->>console-operator: Update tlsSecurityProfile
console-operator->>ConsoleRouteTLSClient: Reconcile console route TLS settings
ConsoleRouteTLSClient->>APIServer: Test TLS 1.2 and TLS 1.3 behavior
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 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: 1
🧹 Nitpick comments (4)
test/extended/tls/tls_observed_config.go (4)
2008-2042: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
InsecureSkipVerify: trueflagged by static analysis — confirm intent and document it.Both OpenGrep and ast-grep flag
InsecureSkipVerify: truehere as a MITM risk (CWE-295). This mirrors the pre-existing pattern incaptureTLSConfiguration(Lines 1393-1400) used elsewhere in this file, and appears intentional: the test only validates TLS protocol-version negotiation against the console route, not certificate identity, and the connection is torn down immediately. Given two SAST tools flag it, consider adding a short inline comment stating this rationale to prevent future confusion and repeated tool noise.💡 Suggested clarifying comment
tlsConf := &tls.Config{ MinVersion: tlsVersion, MaxVersion: tlsVersion, + // Cert identity is not under test here; only TLS version + // negotiation with the console route is being validated. InsecureSkipVerify: true, ServerName: host, }🤖 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 2008 - 2042, Add a concise inline comment next to InsecureSkipVerify in testConsoleRouteTLS explaining that certificate verification is intentionally disabled because this test only validates TLS protocol-version negotiation, matching the existing captureTLSConfiguration pattern.
695-709: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
consoleTLSTargets.observedConfig[0]instead of duplicating the target.This constructs a
newObservedConfigTargetwith identical parameters to the one already defined inconsoleTLSTargets(Lines 2002-2006). Keeping two hand-written copies risks silent drift if one is updated without the other, making one of the tests validate the wrong resource without failing loudly.♻️ Proposed fix
- target := newObservedConfigTarget( - "openshift-console-operator", - gvr("operator.openshift.io", "v1", "consoles"), - "cluster", - []string{"servingInfo"}, - ) + target := consoleTLSTargets.observedConfig[0]🤖 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 695 - 709, Update the TLS test in the “should have TLS entries in the Console operator observedConfig” case to reuse consoleTLSTargets.observedConfig[0] for the target passed to testTLS, removing the duplicate newObservedConfigTarget construction while preserving the existing expected configuration and assertion.
1999-2006: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate target definition — see the linked comment at Lines 695-709.
consoleTLSTargetsduplicates the exact samenewObservedConfigTargetcall constructed independently in the "should have TLS entries in the Console operator observedConfig" test. Consider having that test source its target from here instead.🤖 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 1999 - 2006, Reuse the target defined in consoleTLSTargets within the “should have TLS entries in the Console operator observedConfig” test instead of constructing another identical newObservedConfigTarget call. Keep the test behavior unchanged while making consoleTLSTargets the single source of truth for this observedConfig target.
2044-2062: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSurface the last dial error when the poll times out.
On timeout,
wait.PollUntilContextTimeoutreturns a generic deadline-exceeded error, discarding the lasttestConsoleRouteTLSfailure captured inside the closure (Lines 2055-2059). When this test fails in CI, the reported error won't say whether the connection kept succeeding, kept failing, or hit a network error — only that it "timed out," making triage harder.♻️ Proposed fix
func waitForConsoleRouteTLSState(host string, tlsVersion uint16, shouldSucceed bool, ctx context.Context) error { versionName := tlsVersionName(tlsVersion) expected := "succeed" if !shouldSucceed { expected = "be rejected" } - return wait.PollUntilContextTimeout(ctx, 10*time.Second, 5*time.Minute, true, + var lastErr error + err := wait.PollUntilContextTimeout(ctx, 10*time.Second, 5*time.Minute, true, func(ctx context.Context) (bool, error) { - err := testConsoleRouteTLS(host, tlsVersion, shouldSucceed) - if err != nil { - e2e.Logf("Console route TLS %s should %s but: %v (retrying)", versionName, expected, err) + lastErr = testConsoleRouteTLS(host, tlsVersion, shouldSucceed) + if lastErr != nil { + e2e.Logf("Console route TLS %s should %s but: %v (retrying)", versionName, expected, lastErr) return false, nil } return true, nil }) + if err != nil && lastErr != nil { + return fmt.Errorf("%w: last error: %v", err, lastErr) + } + 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/tls/tls_observed_config.go` around lines 2044 - 2062, The waitForConsoleRouteTLSState polling closure currently logs but discards the last testConsoleRouteTLS failure when polling times out. Capture the most recent error outside the closure, update it on each failed attempt, and after wait.PollUntilContextTimeout returns an error, return a wrapped error that includes the last dial failure while preserving the original polling error; retain the existing success behavior.
🤖 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 752-832: Increase the test’s outer timeout in the TLS profile
change test to cover the additive worst-case waits from waitForTLSReconciliation
and waitForConsoleRouteTLSState, updating both the context duration and the
[Timeout:60m] annotation to 120 minutes. Preserve the existing reconciliation
and route verification steps.
---
Nitpick comments:
In `@test/extended/tls/tls_observed_config.go`:
- Around line 2008-2042: Add a concise inline comment next to InsecureSkipVerify
in testConsoleRouteTLS explaining that certificate verification is intentionally
disabled because this test only validates TLS protocol-version negotiation,
matching the existing captureTLSConfiguration pattern.
- Around line 695-709: Update the TLS test in the “should have TLS entries in
the Console operator observedConfig” case to reuse
consoleTLSTargets.observedConfig[0] for the target passed to testTLS, removing
the duplicate newObservedConfigTarget construction while preserving the existing
expected configuration and assertion.
- Around line 1999-2006: Reuse the target defined in consoleTLSTargets within
the “should have TLS entries in the Console operator observedConfig” test
instead of constructing another identical newObservedConfigTarget call. Keep the
test behavior unchanged while making consoleTLSTargets the single source of
truth for this observedConfig target.
- Around line 2044-2062: The waitForConsoleRouteTLSState polling closure
currently logs but discards the last testConsoleRouteTLS failure when polling
times out. Capture the most recent error outside the closure, update it on each
failed attempt, and after wait.PollUntilContextTimeout returns an error, return
a wrapped error that includes the last dial failure while preserving the
original polling error; retain the existing success behavior.
🪄 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: 323503f5-f2da-495c-b635-b8715af8ea38
📒 Files selected for processing (1)
test/extended/tls/tls_observed_config.go
| g.It("should enforce TLS versions through console route across profile changes [Timeout:60m]", func() { | ||
| configChangeCtx, configChangeCancel := context.WithTimeout(ctx, 60*time.Minute) | ||
| defer configChangeCancel() | ||
|
|
||
| g.By("getting console route hostname") | ||
| routeGVR := schema.GroupVersionResource{Group: "route.openshift.io", Version: "v1", Resource: "routes"} | ||
| route, err := oc.AdminDynamicClient().Resource(routeGVR).Namespace("openshift-console").Get(configChangeCtx, "console", metav1.GetOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to get console route") | ||
| consoleHost, found, err := unstructured.NestedString(route.Object, "spec", "host") | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(found).To(o.BeTrue(), "console route should have spec.host") | ||
| e2e.Logf("Console route hostname: %s", consoleHost) | ||
|
|
||
| g.By("saving original TLS profile for restoration") | ||
| originalAPIServer, err := oc.AdminConfigClient().ConfigV1().APIServers().Get(configChangeCtx, "cluster", metav1.GetOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| originalProfile := originalAPIServer.Spec.TLSSecurityProfile | ||
|
|
||
| defer func() { | ||
| g.By("restoring original TLS profile") | ||
| setAPIServerTLSProfile(oc, ctx, originalProfile, "original") | ||
| if err := waitForTLSReconciliation(oc, ctx, false, consoleTLSTargets, captureTLSConfiguration(originalProfile)); err != nil { | ||
| e2e.Logf("Warning: console TLS reconciliation during cleanup: %v", err) | ||
| } | ||
| }() | ||
|
|
||
| // Step 1: Set Intermediate profile and verify both TLS 1.2 and TLS 1.3 work | ||
| g.By("setting TLS profile to Intermediate") | ||
| intermediateProfile := &configv1.TLSSecurityProfile{ | ||
| Type: configv1.TLSProfileIntermediateType, | ||
| Intermediate: &configv1.IntermediateTLSProfile{}, | ||
| } | ||
| setAPIServerTLSProfile(oc, configChangeCtx, intermediateProfile, "Intermediate") | ||
| intermediateTLSConfig := captureTLSConfiguration(intermediateProfile) | ||
| err = waitForTLSReconciliation(oc, configChangeCtx, false, consoleTLSTargets, intermediateTLSConfig) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "console operator should reconcile to Intermediate profile") | ||
|
|
||
| g.By("verifying TLS 1.2 works with Intermediate profile") | ||
| err = waitForConsoleRouteTLSState(consoleHost, tls.VersionTLS12, true, configChangeCtx) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "TLS 1.2 should work with Intermediate profile") | ||
|
|
||
| g.By("verifying TLS 1.3 works with Intermediate profile") | ||
| err = waitForConsoleRouteTLSState(consoleHost, tls.VersionTLS13, true, configChangeCtx) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "TLS 1.3 should work with Intermediate profile") | ||
|
|
||
| // Step 2: Switch to Modern profile — only TLS 1.3 should work | ||
| g.By("switching to Modern TLS profile") | ||
| modernProfile := &configv1.TLSSecurityProfile{ | ||
| Type: configv1.TLSProfileModernType, | ||
| Modern: &configv1.ModernTLSProfile{}, | ||
| } | ||
| setAPIServerTLSProfile(oc, configChangeCtx, modernProfile, "Modern") | ||
| modernTLSConfig := captureTLSConfiguration(modernProfile) | ||
| err = waitForTLSReconciliation(oc, configChangeCtx, false, consoleTLSTargets, modernTLSConfig) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "console operator should reconcile to Modern profile") | ||
|
|
||
| g.By("verifying TLS 1.3 works with Modern profile") | ||
| err = waitForConsoleRouteTLSState(consoleHost, tls.VersionTLS13, true, configChangeCtx) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "TLS 1.3 should work with Modern profile") | ||
|
|
||
| g.By("verifying TLS 1.2 is rejected with Modern profile") | ||
| err = waitForConsoleRouteTLSState(consoleHost, tls.VersionTLS12, false, configChangeCtx) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "TLS 1.2 should be rejected with Modern profile") | ||
|
|
||
| // Step 3: Switch back to Intermediate — both TLS 1.2 and 1.3 should work again | ||
| g.By("switching back to Intermediate TLS profile") | ||
| setAPIServerTLSProfile(oc, configChangeCtx, intermediateProfile, "Intermediate") | ||
| err = waitForTLSReconciliation(oc, configChangeCtx, false, consoleTLSTargets, intermediateTLSConfig) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "console operator should reconcile back to Intermediate profile") | ||
|
|
||
| g.By("verifying TLS 1.2 works after downgrade to Intermediate") | ||
| err = waitForConsoleRouteTLSState(consoleHost, tls.VersionTLS12, true, configChangeCtx) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "TLS 1.2 should work after downgrade to Intermediate") | ||
|
|
||
| g.By("verifying TLS 1.3 works after downgrade to Intermediate") | ||
| err = waitForConsoleRouteTLSState(consoleHost, tls.VersionTLS13, true, configChangeCtx) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "TLS 1.3 should work after downgrade to Intermediate") | ||
|
|
||
| e2e.Logf("=== Console operator TLS profile change validation complete ===") | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Outer 60m timeout can't cover the worst-case additive wait budget.
The test wraps everything in a 60-minute configChangeCtx (Line 753) and is tagged [Timeout:60m], but it makes 3 calls to waitForTLSReconciliation (25m timeout each, Line 1566) and 6 calls to waitForConsoleRouteTLSState (5m timeout each, Line 2053) — a worst-case total of 105 minutes. Under slow reconciliation (plausible in CI), the outer context will cancel mid-step, producing a misleading "reconciliation timeout after 25m0s" failure that actually stems from the outer 60m budget being exhausted early, rather than genuine reconciliation failure.
Increase the outer timeout (and the [Timeout:60m] annotation) to comfortably cover the worst case, e.g. 120m, or reduce the per-call timeouts.
🤖 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 752 - 832, Increase
the test’s outer timeout in the TLS profile change test to cover the additive
worst-case waits from waitForTLSReconciliation and waitForConsoleRouteTLSState,
updating both the context duration and the [Timeout:60m] annotation to 120
minutes. Preserve the existing reconciliation and route verification steps.
|
@kaleemsiddiqu: all tests passed! 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
e2e tests for openshift/console-operator#1196
Summary by CodeRabbit