WIP: 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
WalkthroughThe operator retrieves API server TLS settings, validates adherence policy, applies TLS configuration to the metrics server, and registers a watcher that restarts the operator when TLS profile or adherence 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->>MetricsServer: Apply TLS options
Operator->>SecurityProfileWatcher: Register watcher
SecurityProfileWatcher->>Operator: Notify of TLS changes
Operator->>Operator: Log change and exit for restart
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 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
| // Fetch initial TLS profile and adherence for the watcher | ||
| initialTLSProfile, err := lsotls.FetchAPIServerTLSProfile(ctx, configClient) | ||
| if err != nil { | ||
| klog.Warningf("failed to fetch initial TLS profile for watcher: %v", err) | ||
| } | ||
| initialAdherence, err := lsotls.GetAdherencePolicyForLogging(ctx, configClient) | ||
| if err != nil { | ||
| klog.Warningf("failed to fetch initial TLS adherence for watcher: %v", err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'initialTLSProfile|initialAdherence|NewSecurityProfileWatcher|SetupWithManager' \
cmd/local-storage-operator/main.go pkg/tlsRepository: openshift/local-storage-operator
Length of output: 5459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'type SecurityProfileWatcher|InitialTLSProfileSpec|InitialTLSAdherencePolicy|OnProfileChange|OnAdherenceChange|SetupWithManager|Compare|mismatch|restart' .Repository: openshift/local-storage-operator
Length of output: 50389
Require valid watcher baselines before registering the TLS watcher. If either baseline fetch fails, NewSecurityProfileWatcher gets the zero value and can treat the current cluster state as a change, exiting the operator on startup. Fail fast or skip watcher setup until both values are loaded.
🤖 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` around lines 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.
Source: Path instructions
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
| configFn, unsupportedCiphers := crcommon.NewTLSConfigFromProfile(profileSpec) | ||
| if len(unsupportedCiphers) > 0 { | ||
| return nil, fmt.Errorf("TLS profile contains unsupported cipher suites: %v", unsupportedCiphers) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== pkg/tls/tlsprofile.go ==\n'
wc -l pkg/tls/tlsprofile.go
sed -n '1,180p' pkg/tls/tlsprofile.go
printf '\n== search for NewTLSConfigFromProfile ==\n'
rg -n "NewTLSConfigFromProfile|unsupportedCiphers|TLS profile contains unsupported cipher suites" -S .
printf '\n== controller-runtime-common reference (if vendored or in go.mod) ==\n'
rg -n "controller-runtime-common|fea68df23430" -S go.mod go.sum vendor . 2>/dev/null || trueRepository: openshift/local-storage-operator
Length of output: 9060
🌐 Web query:
OpenShift controller-runtime-common NewTLSConfigFromProfile unsupportedCiphers omit unsupported cipher names profile contract retain supported entries rather than fail
💡 Result:
In OpenShift, the controller-runtime-common utility function NewTLSConfigFromProfile is designed to handle TLS configuration from a TLSProfileSpec while gracefully managing unsupported ciphers [1][2][3]. Its contract does not fail when encountering unsupported cipher names; instead, it adopts a policy of omission, retaining only the supported entries to ensure the application maintains a functional TLS configuration [1][3]. The function returns two values: the configured TLS settings (as a function or struct) and a slice of strings containing the names of any ciphers that were unsupported or could not be mapped [4][2][3]. Standard implementation patterns across OpenShift operators include checking if this unsupported cipher slice is non-empty and logging a warning or informational message to notify administrators that specific ciphers were ignored [1][4][2][3]. This approach prioritizes system availability by applying the valid subset of the requested security profile rather than causing the operator to crash or fail to start due to minor configuration mismatches [5][3].
Citations:
- 1: https://github.com/openshift/cluster-cloud-controller-manager-operator/blob/c31c4aaab6d9/pkg/tls/tls.go
- 2: https://github.com/openshift/insights-operator/blob/ad672d905ed6/pkg/cmd/start/manager.go
- 3: https://github.com/openshift/cluster-node-tuning-operator/blob/8978bc513cff/cmd/cluster-node-tuning-operator/main.go
- 4: https://github.com/openshift-kni/oran-o2ims/blob/c48d8ee4cb26/internal/controllers/utils/tls_profile.go
- 5: OCPCLOUD-3346: tls: use centralized TLS machine-api-operator#1456
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '96,112p' pkg/tls/tlsprofile.goRepository: openshift/local-storage-operator
Length of output: 1015
Don't fail GetTLSConfigFromProfile on unsupported ciphers. crcommon.NewTLSConfigFromProfile already drops ciphers this binary can't use and returns the supported config; returning an error here blocks metrics startup even when the profile still has usable entries. Keep the supported ciphers and log a warning for the rest.
🤖 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 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.
Source: MCP tools
| // Only enforce in StrictAllComponents mode | ||
| if adherence == configv1.TLSAdherencePolicyStrictAllComponents { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- pkg/tls/tlsprofile.go (relevant section) ---\n'
sed -n '1,220p' pkg/tls/tlsprofile.go | cat -n | sed -n '1,220p'
printf '\n--- search adherence policy usage ---\n'
rg -n "TLSAdherencePolicy|StrictAllComponents|adherence" -S .
printf '\n--- locate configv1 type definition in repo ---\n'
rg -n "type TLSAdherencePolicy|StrictAllComponents" -S pkg vendor api .Repository: openshift/local-storage-operator
Length of output: 261
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the exact file and nearby code paths.
sed -n '1,220p' pkg/tls/tlsprofile.go | cat -nRepository: openshift/local-storage-operator
Length of output: 7151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look up the upstream API docs and enum definition if not vendored locally.
python3 - <<'PY'
import sys, json, urllib.request
url = "https://pkg.go.dev/github.com/openshift/api/config/v1?tab=doc"
try:
with urllib.request.urlopen(url, timeout=15) as r:
data = r.read().decode('utf-8', 'replace')
print("fetched", len(data))
for needle in ["TLSAdherencePolicyStrictAllComponents", "TLSAdherencePolicy"]:
print(needle, needle in data)
except Exception as e:
print("ERR", repr(e))
PYRepository: openshift/local-storage-operator
Length of output: 334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search the repository for the TLS adherence policy enum and call sites.
rg -n "TLSAdherencePolicyStrictAllComponents|TLSAdherencePolicy" -S .Repository: openshift/local-storage-operator
Length of output: 4787
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the file in question with line numbers.
cat -n pkg/tls/tlsprofile.go | sed -n '1,220p'Repository: openshift/local-storage-operator
Length of output: 7151
🌐 Web query:
github.com/openshift/api config/v1 TLSAdherencePolicyStrictAllComponents enum unknown values validation
💡 Result:
In the OpenShift API (config/v1), the TLSAdherencePolicy field is validated using a Kubernetes custom resource validation enum [1][2][3]. The valid values for this field are restricted to "LegacyAdheringComponentsOnly" and "StrictAllComponents" [1][4]. To ensure forward compatibility, OpenShift components are designed to handle unknown values for the tlsAdherence field as follows [1][4]: 1. Defaulting Behavior: If a component encounters an unknown TLS adherence policy value, it must treat the setting as "StrictAllComponents" [1][4]. 2. Observability: Components are expected to log a warning when an unknown value is encountered, providing visibility into the configuration mismatch while defaulting to the more secure behavior [1][4][5]. This approach ensures that if new, stricter policies are added to the API in the future, existing components will default to the most secure state rather than falling back to potentially insecure legacy configurations [1][4]. Internal utility functions, such as those found in openshift/library-go, have been updated to support this pattern, including the addition of logging mechanisms for handling unrecognized enum values [5].
Citations:
- 1: https://github.com/openshift/api/blob/master/config/v1/types_apiserver.go
- 2: openshift/api@5c75e62
- 3: openshift/api@5e946e2...1f2fa3f
- 4: openshift/api@76fab1a
- 5: Log unknown adherence values and add support for legacy adhering components to ShouldHonorClusterTLSProfile library-go#2308
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' vendor/github.com/openshift/controller-runtime-common/pkg/tls/tls.go | cat -n
printf '\n---\n'
sed -n '1,220p' vendor/github.com/openshift/library-go/pkg/crypto/tls_adherence.go | cat -n
printf '\n---\n'
sed -n '270,320p' vendor/github.com/openshift/api/config/v1/types_apiserver.go | cat -nRepository: openshift/local-storage-operator
Length of output: 11537
Treat unknown adherence policies as strict. ValidateMetricsAccess only handles StrictAllComponents explicitly, so any future enum value follows the permissive path. Use ShouldHonorClusterTLSProfile or add a default-to-strict branch here.
🤖 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 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.
Sources: Path instructions, MCP tools
| profileSpec, err := crcommon.FetchAPIServerTLSProfile(ctx, c) | ||
| if err != nil { | ||
| return fmt.Errorf("strict mode requires valid TLS profile: %w", err) | ||
| } | ||
| // Check if profile has any configuration | ||
| if profileSpec.Ciphers == nil && profileSpec.MinTLSVersion == "" { | ||
| return fmt.Errorf("strict mode requires explicit TLS profile") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repo files of interest:\n'
git ls-files 'pkg/tls/*' 'pkg/**/tls*' | sed -n '1,120p'
printf '\nOutline for pkg/tls/tlsprofile.go:\n'
ast-grep outline pkg/tls/tlsprofile.go --view expanded || true
printf '\nRelevant lines from pkg/tls/tlsprofile.go:\n'
sed -n '1,220p' pkg/tls/tlsprofile.go
printf '\nSearch for FetchAPIServerTLSProfile definitions/usages:\n'
rg -n "FetchAPIServerTLSProfile|APIServerTLSProfile" -S .
printf '\nIf helper exists in vendored/internal path, show its file:\n'
fd -a "tls.go" . | sed -n '1,120p'Repository: openshift/local-storage-operator
Length of output: 9164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- pkg/tls/tlsprofile.go ---'
sed -n '1,220p' pkg/tls/tlsprofile.go
echo
echo '--- references in repo ---'
rg -n "FetchAPIServerTLSProfile|strict mode requires explicit TLS profile|explicit TLS profile|APIServerTLSProfile" pkg vendor . --glob '!**/node_modules/**' || true
echo
echo '--- attempt to inspect referenced upstream helper if network is available ---'
python3 - <<'PY'
import urllib.request
url = "https://raw.githubusercontent.com/openshift/controller-runtime-common/fea68df23430/pkg/tls/tls.go"
try:
with urllib.request.urlopen(url, timeout=10) as r:
data = r.read().decode("utf-8", "replace")
for i, line in enumerate(data.splitlines(), 1):
if "FetchAPIServerTLSProfile" in line or "TLSProfile" in line:
start = max(1, i-25)
end = min(len(data.splitlines()), i+80)
for j in range(start, end + 1):
print(f"{j:4d}: {data.splitlines()[j-1]}")
break
except Exception as e:
print(f"NETWORK_ERROR: {e}")
PYRepository: openshift/local-storage-operator
Length of output: 8355
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- vendor/github.com/openshift/controller-runtime-common/pkg/tls/tls.go ---'
sed -n '1,220p' vendor/github.com/openshift/controller-runtime-common/pkg/tls/tls.go
echo
echo '--- cmd/local-storage-operator/main.go around the TLS logic ---'
sed -n '110,155p' cmd/local-storage-operator/main.goRepository: openshift/local-storage-operator
Length of output: 8607
Check profile presence before defaulting. FetchAPIServerTLSProfile returns the intermediate default when spec.tlsSecurityProfile is unset, so this nil/empty check never rejects an absent profile. Strict mode still accepts the defaulted config instead of requiring an explicit APIServer TLS profile.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 125-125: : # github.com/openshift/local-storage-operator/api/v1
api/v1/localvolume_types.go:125:25: cannot use &LocalVolume{} (value of type *LocalVolume) as "k8s.io/apimachinery/pkg/runtime".Object value in argument to SchemeBuilder.Register: *LocalVolume does not implement "k8s.io/apimachinery/pkg/runtime".Object (missing method DeepCopyObject)
api/v1/localvolume_types.go:125:41: cannot use &LocalVolumeList{} (value of type *LocalVolumeList) as "k8s.io/apimachinery/pkg/runtime".Object value in argument to SchemeBuilder.Register: *LocalVolumeList does not implement "k8s.io/apimachinery/pkg/runtime".Object (missing method DeepCopyObject)
api/v1/localvolumedevicelink_types.go:133:25: cannot use &LocalVolumeDeviceLink{} (value of type *LocalVolumeDeviceLink) as "k8s.io/apimachinery/pkg/runtime".Object value in argument to SchemeBuilder.Register: *LocalVolumeDeviceLink does not implement "k8s.io/apimachinery/pkg/runtime".Object (missing method DeepCopyObject)
api/v1/localvolumedevicelink_types.go:133:51: cannot use &LocalVolumeDeviceLinkList{} (value of type *LocalVolumeDeviceLinkList) as "k8s.io/apimachinery/pkg/runtime".Object value in argument to SchemeBuilder.Register: *LocalVolumeDeviceLinkList does not implement "k8s.io/apimachinery/pkg/runtime".Object (missing method DeepCopyObject)
(typecheck)
🤖 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 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.
Source: MCP tools
1 participant
Summary by CodeRabbit