STOR-3054: tls setup - #643
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhrmo 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
openshift-ci
Bot
added
the
needs-rebase
|
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:
WalkthroughThe operator now fetches API server TLS settings at startup, validates strict adherence requirements, configures secure metrics serving, and registers a watcher that exits on TLS policy changes. ChangesTLS security integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant KubernetesAPI
participant MetricsServer
participant SecurityProfileWatcher
Operator->>KubernetesAPI: Fetch TLS profile and adherence policy
KubernetesAPI-->>Operator: Return TLS settings
Operator->>Operator: Validate and derive TLS configuration
Operator->>MetricsServer: Configure secure metrics serving
Operator->>SecurityProfileWatcher: Register watcher
SecurityProfileWatcher->>Operator: Notify of TLS changes
Operator->>Operator: Log change and exit for restart
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 inconclusive)
✅ 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: 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 `@cmd/local-storage-operator/main.go`:
- Around line 118-133: The TLS validation flow in main must retain the
adherence-policy result and handle every error instead of ignoring
GetAdherencePolicyForLogging failures. Reuse that policy when fetching TLS
configuration, and if the policy is StrictAllComponents, fail startup on
adherence or TLS retrieval/configuration errors rather than setting tlsConfigFn
to controller-runtime defaults; preserve the existing warning fallback only for
non-strict policies.
- Around line 109-143: Replace the unbounded context.Background() used by the
startup TLS setup with a context carrying a finite startup timeout, and ensure
it covers ValidateMetricsAccess, GetTLSConfigForController,
FetchAPIServerTLSProfile, and GetAdherencePolicyForLogging. Cancel the context
appropriately after these initial API calls while preserving the existing error
handling and fallback behavior.
- Around line 135-143: Update the watcher initialization flow around the
initialTLSProfile and initialAdherence fetches so NewSecurityProfileWatcher is
called only after both baselines load successfully. On either
FetchAPIServerTLSProfile or GetAdherencePolicyForLogging error, fail fast or
skip TLS watcher registration rather than continuing with zero-value baselines.
🪄 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: 41144704-6943-48f0-a24d-ef8459115693
⛔ 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 (4)
cmd/local-storage-operator/main.gogo.modpkg/tls/tlsprofile.gopkg/tls/watcher.go
openshift-ci
Bot
removed
the
needs-rebase
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 `@cmd/local-storage-operator/main.go`:
- Around line 135-140: Update the watcher initialization around
FetchAPIServerTLSProfile, GetAdherencePolicyForLogging, and the metrics TLS
configuration so each TLS profile and adherence value is fetched only once.
Derive the metrics configuration and watcher baseline from those same fetched
values, then pass the exact snapshot to the watcher so subsequent changes
trigger a restart.
🪄 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: c7ad2f7c-8b99-4d02-b145-f8cb79d38e7c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/local-storage-operator/main.gogo.modpkg/tls/tlsprofile.gopkg/tls/watcher.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/tls/watcher.go
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 `@pkg/tls/tlsprofile.go`:
- Around line 104-106: Update GetTLSConfigFromProfile after
crcommon.NewTLSConfigFromProfile so unsupportedCiphers no longer causes an error
return; retain the supported configFn and emit a warning listing the unsupported
cipher suites. Preserve normal configuration behavior when no unsupported
ciphers are returned.
- Around line 120-121: Update ValidateMetricsAccess so unknown or future
adherence policy values follow the strict enforcement path instead of the
permissive path. Reuse ShouldHonorClusterTLSProfile if it provides the intended
classification, or add a default-to-strict branch alongside the existing
StrictAllComponents check.
- Around line 122-128: Update the strict-mode logic around
FetchAPIServerTLSProfile to detect whether spec.tlsSecurityProfile is explicitly
present before using the returned profileSpec defaults. Reject an absent profile
with the existing explicit-profile error, while preserving validation of an
explicitly configured profile and propagation of fetch errors.
🪄 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: 06aa2954-df79-4526-bb31-7f7f1ad68042
⛔ 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 (4)
cmd/local-storage-operator/main.gogo.modpkg/tls/tlsprofile.gopkg/tls/watcher.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- pkg/tls/watcher.go
openshift-ci-robot
added
the
jira/valid-reference
|
@rhrmo: This pull request references STOR-3054 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. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/local-storage-operator/main.go (2)
1-1: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winTreat unknown TLS adherence values as strict
cmd/local-storage-operator/main.go#L127-L153still only treatsStrictAllComponentsas strict, so future/unknown enum values fall back to the permissive path.pkg/tls/tlsprofile.go#L114-L129has the same strict-only check inValidateMetricsAccess; treat anything other thanLegacyAdheringComponentsOnlyas strict there too.🤖 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 `@cmd/local-storage-operator/main.go` at line 1, Update the TLS adherence checks in the main operator flow and tlsprofile.ValidateMetricsAccess so only LegacyAdheringComponentsOnly selects the permissive behavior; treat StrictAllComponents and all unknown/future values as strict.
1-1: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftGate the strict-TLS check on APIServer config presence
FetchAPIServerTLSProfileresolves unset profiles to default values, soCiphers == nil && MinTLSVersion == ""can’t tell whetherspec.tlsSecurityProfilewas explicitly set. Use the APIServer spec field (or an equivalent explicit-presence signal) in bothcmd/local-storage-operator/main.go#L141-L146andpkg/tls/tlsprofile.go#L122-L129.🤖 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 `@cmd/local-storage-operator/main.go` at line 1, Update the strict-TLS checks in the main startup flow and the TLS profile logic around FetchAPIServerTLSProfile to determine explicit configuration from the APIServer spec field, or another equivalent presence signal, rather than relying on resolved Ciphers and MinTLSVersion values. Apply this presence gate consistently in both locations while preserving the existing behavior for explicitly configured profiles.
🧹 Nitpick comments (1)
pkg/tls/tlsprofile.go (1)
135-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrapper naming is slightly misleading.
GetAdherencePolicyForLoggingis also used by main.go to drive actual strict-mode branching (not just logging). Not blocking, just a naming nit.🤖 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 `@pkg/tls/tlsprofile.go` around lines 135 - 145, Rename GetAdherencePolicyForLogging to a neutral name that reflects its use for both policy retrieval and strict-mode branching, then update all callers including main.go while preserving the existing FetchAPIServerTLSAdherencePolicy 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 `@cmd/local-storage-operator/main.go`:
- Around line 132-153: Update the strict-mode checks around TLS profile
fetching, validation, and TLS configuration creation to use
ShouldHonorClusterTLSProfile instead of directly comparing adherence with
TLSAdherencePolicyStrictAllComponents. This must treat unknown future policy
values as strict while preserving the existing permissive path only for policies
explicitly not honoring the cluster TLS profile.
- Around line 141-146: Update the strict-adherence validation around
FetchAPIServerTLSProfile to detect whether APIServer.spec.tlsSecurityProfile was
explicitly configured before default normalization. Reject unset profiles in
StrictAllComponents mode, while preserving normalized profile handling for
explicitly configured profiles.
In `@pkg/tls/tlsprofile.go`:
- Around line 111-133: Remove the unused ValidateMetricsAccess helper, or
integrate a call to it into the startup path and replace the duplicated inline
strict-adherence checks with that call. Ensure startup continues enforcing TLS
profile requirements only for TLSAdherencePolicyStrictAllComponents and
preserves the existing error behavior.
---
Outside diff comments:
In `@cmd/local-storage-operator/main.go`:
- Line 1: Update the TLS adherence checks in the main operator flow and
tlsprofile.ValidateMetricsAccess so only LegacyAdheringComponentsOnly selects
the permissive behavior; treat StrictAllComponents and all unknown/future values
as strict.
- Line 1: Update the strict-TLS checks in the main startup flow and the TLS
profile logic around FetchAPIServerTLSProfile to determine explicit
configuration from the APIServer spec field, or another equivalent presence
signal, rather than relying on resolved Ciphers and MinTLSVersion values. Apply
this presence gate consistently in both locations while preserving the existing
behavior for explicitly configured profiles.
---
Nitpick comments:
In `@pkg/tls/tlsprofile.go`:
- Around line 135-145: Rename GetAdherencePolicyForLogging to a neutral name
that reflects its use for both policy retrieval and strict-mode branching, then
update all callers including main.go while preserving the existing
FetchAPIServerTLSAdherencePolicy 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 81c57545-711e-423a-a338-acecb5ac5d19
⛔ 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 (4)
cmd/local-storage-operator/main.gogo.modpkg/tls/tlsprofile.gopkg/tls/watcher.go
|
/test tls-scanner-local-storage |
|
|
||
| var tlsConfigFn func(*tls.Config) | ||
|
|
||
| if libcrypto.ShouldHonorClusterTLSProfile(adherence) { |
There was a problem hiding this comment.
We have the check now, but we miss the defaulting it seems. The requirement is to never use default Go TLS, so there's usually a fallback to this default: https://github.com/openshift/cluster-machine-approver/blob/ed5f7b438136c46207bf443bb89e6610872292ec/pkg/tls/tls.go#L135
If we leave tlsConfigFn nil controller-runtime will use Go TLS defaults - so this should never happen I think.
There was a problem hiding this comment.
(got access to this branch - resolving)
|
/test tls-scanner-local-storage |
|
@rhrmo: 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. |
|
/lgtm |
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@RomanBednar: 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. |
|
I also verified it in a real TP cluster with oc patch apiserver cluster --type=merge -p '{ not sure why there are duplicities, but the CVS says the ports are OK) |
6 participants
Summary by CodeRabbit