fix(tls): correct TLS adherence implementation - #646
Conversation
- Gate TLS profile application on ShouldHonorClusterTLSProfile instead of a non-empty profile check, so new-to-TLS components only honor the cluster profile under StrictAllComponents mode - Replace os.Exit(0) with context cancellation for graceful shutdown, allowing leader election lease release and defer cleanup - Remove dead ValidateMetricsAccess and trivial wrapper functions, calling controller-runtime-common directly
Replace manual TLSProfileSpec construction with crcommon.GetTLSProfileSpec(nil) which returns the same Intermediate default profile.
WalkthroughThe operator now derives metrics TLS settings from cluster adherence and profile data, watches for TLS changes, and uses shared context cancellation for lifecycle control. A new dependency and engineering plan document support the implementation. ChangesTLS startup and lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant LocalStorageOperator
participant APIServer
participant Manager
participant SecurityProfileWatcher
LocalStorageOperator->>APIServer: Fetch TLS adherence policy and TLS profile
LocalStorageOperator->>Manager: Configure secure metrics serving
LocalStorageOperator->>SecurityProfileWatcher: Register initial TLS state and cancellation callback
SecurityProfileWatcher->>LocalStorageOperator: Notify TLS profile or adherence change
LocalStorageOperator->>Manager: Cancel shared startup context
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RomanBednar 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/plans/2026-07-30-001-fix-lso-tls-adherence-plan.md (1)
146-153: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd behavior-specific verification gates.
The aggregate commands do not explicitly verify the two functional changes. Include tests or checks confirming that
StrictAllComponentsapplies the cluster profile, other adherence modes use the default profile, and watcher callbacks cancel the shared context gracefully.🤖 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 `@docs/plans/2026-07-30-001-fix-lso-tls-adherence-plan.md` around lines 146 - 153, Extend the Verification Contract with behavior-specific gates covering the StrictAllComponents cluster-profile selection, default-profile selection for other adherence modes, and graceful shared-context cancellation from watcher callbacks. Define tests or checks that explicitly validate each behavior alongside the existing aggregate build, test, vet, and dead-code gates.
🤖 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 `@docs/plans/2026-07-30-001-fix-lso-tls-adherence-plan.md`:
- Line 49: Update the fenced code block in the plan document by adding the go
language identifier to its opening fence, while leaving the snippet content and
closing fence unchanged.
In `@go.mod`:
- Line 99: Update the controller-runtime-common dependency entry in go.mod to
use an available tagged stable release instead of the pseudo-version. If no
suitable tagged release exists, add the dependency to the repository’s explicit
approved exception mechanism while preserving the required version.
---
Nitpick comments:
In `@docs/plans/2026-07-30-001-fix-lso-tls-adherence-plan.md`:
- Around line 146-153: Extend the Verification Contract with behavior-specific
gates covering the StrictAllComponents cluster-profile selection,
default-profile selection for other adherence modes, and graceful shared-context
cancellation from watcher callbacks. Define tests or checks that explicitly
validate each behavior alongside the existing aggregate build, test, vet, and
dead-code gates.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3eed9eb7-581b-45cd-9e93-621dee3e6878
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/controller-runtime-common/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/controller-runtime-common/pkg/tls/controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/controller-runtime-common/pkg/tls/tls.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (5)
cmd/local-storage-operator/main.godocs/plans/2026-07-30-001-fix-lso-tls-adherence-plan.mdgo.modpkg/tls/tlsprofile.gopkg/tls/watcher.go
|
|
||
| The `SecurityProfileWatcher`'s own source code documents the `cancel()` pattern as the intended usage — from [`controller-runtime-common/pkg/tls/controller.go:63-77`](https://github.com/openshift/controller-runtime-common/blob/fea68df23430b5c1e86599ac37d1de7e7fe55eb6/pkg/tls/controller.go#L63-L77): | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify the fenced block language.
Add go after the opening fence so Markdown linters and readers can identify the snippet correctly.
Proposed fix
-```
+```go📝 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.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/plans/2026-07-30-001-fix-lso-tls-adherence-plan.md` at line 49, Update
the fenced code block in the plan document by adding the go language identifier
to its opening fence, while leaving the snippet content and closing fence
unchanged.
Source: Linters/SAST tools
| github.com/aws/aws-sdk-go-v2/credentials v1.19.29 | ||
| github.com/aws/aws-sdk-go-v2/service/ec2 v1.316.1 | ||
| github.com/onsi/ginkgo/v2 v2.32.0 | ||
| github.com/openshift/controller-runtime-common v0.0.0-20260722095319-fea68df23430 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
module='github.com/openshift/controller-runtime-common'
version='v0.0.0-20260722095319-fea68df23430'
rg -n "^${module} ${version}( /go\.mod)? h1:" go.sum
curl -fsSL "https://proxy.golang.org/${module}/@v/list" |
grep -Ev -- '-' || true
curl -fsSL https://api.osv.dev/v1/query \
-H 'content-type: application/json' \
-d "{\"package\":{\"ecosystem\":\"Go\",\"name\":\"${module}\"},\"version\":\"${version}\"}" |
jq .
curl -fsSL "https://api.github.com/repos/openshift/controller-runtime-common/license" |
jq -r '.license.spdx_id'Repository: openshift/local-storage-operator
Length of output: 358
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'go.mod context:\n'
sed -n '90,105p' go.mod
echo
echo 'Available versions from proxy.golang.org (stable tags only):'
curl -fsSL "https://proxy.golang.org/github.com/openshift/controller-runtime-common/@v/list" \
| sed '/-/d' \
| tail -n 20 || true
echo
echo 'All listed versions count:'
curl -fsSL "https://proxy.golang.org/github.com/openshift/controller-runtime-common/@v/list" \
| wc -lRepository: openshift/local-storage-operator
Length of output: 874
Use a tagged release or add an explicit exception for this dependency.
github.com/openshift/controller-runtime-common v0.0.0-20260722095319-fea68df23430 is a pseudo-version, so it falls under the no pre-release versions policy for production dependencies.
🤖 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 `@go.mod` at line 99, Update the controller-runtime-common dependency entry in
go.mod to use an available tagged stable release instead of the pseudo-version.
If no suitable tagged release exists, add the dependency to the repository’s
explicit approved exception mechanism while preserving the required version.
Source: Path instructions
|
@RomanBednar: 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. |
There was a problem hiding this comment.
I think this file does not belong to the repository.
|
Closing in favor of: github.com//pull/643 |
3 participants
Summary
Fixes the TLS adherence implementation from PR #643 (STOR-3054) by addressing four issues identified during cross-reference review against the OpenShift centralized-tls-config enhancement and the cluster-machine-approver reference implementation.
Changes
ShouldHonorClusterTLSProfile gate: Replace the non-empty profile check (
tlsProfile.Ciphers != nil || tlsProfile.MinTLSVersion != "") withlibcrypto.ShouldHonorClusterTLSProfile(adherence). As a new-to-TLS component, LSO should only honor the cluster TLS profile underStrictAllComponentsmode, notLegacyAdheringComponentsOnly.Graceful shutdown: Replace
os.Exit(0)in the TLS watcher callbacks withcontext.CancelFunc, following the pattern documented incontroller-runtime-commonand used by cluster-machine-approver. This ensures leader election leases are released,defercleanup runs, and in-flight reconciles finish.Dead code removal: Remove
ValidateMetricsAccess(never called from main.go) and three trivial pass-through wrapper functions (GetTLSConfigFromProfile,GetAdherencePolicyForLogging,FetchAPIServerTLSProfile). Main.go now callscontroller-runtime-commondirectly, matching the cluster-machine-approver pattern.References
Verification
go build ./cmd/local-storage-operator/... ./pkg/tls/...passesgo vet ./cmd/local-storage-operator/... ./pkg/tls/...passesgolangci-lint run --new-from-rev=origin/mainreports 0 issuesSummary by CodeRabbit
New Features
Documentation