chore: use centralised TLS Profile by OpinionatedHeron · Pull Request #3234 · redhat-developer/rhdh-operator

OpinionatedHeron · 2026-07-21T14:20:53Z

Description

Implemented centralized OpenShift TLS security profile support for the RHDH operator (controller-runtime path via github.com/openshift/controller-runtime-common), so the operator’s TLS servers honor apiservers.config.openshift.io/cluster.

Which issue(s) does this PR fix or relate to

Fixes RHIDP-13852

PR acceptance criteria

Tests
Documentation

Building Container Images for Testing

Need to test container images from this PR?

For Maintainers: To trigger a test image build, review the code and comment /build-images.
This always builds the HEAD of the PR branch.

For Contributors: Ask a maintainer to run /build-images.

Images will be built and pushed to Quay with links posted in comments.

Signed-off-by: Leanne Ahern <lahern@redhat.com>

into tls

Signed-off-by: Leanne Ahern <lahern@redhat.com>

…ices Signed-off-by: Leanne Ahern <lahern@redhat.com>

rhdh-qodo-merge · 2026-07-21T14:25:35Z

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Context used

⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials

✅ Compliance rules (platform): 18 rules

✅ Cross-repo context

Not relevant to this PR: redhat-developer/rhdh

Not relevant to this PR: redhat-developer/rhdh-plugins

1. TLS fallback hard-exits 🐞 Bug ☼ Reliability

Description

cmd/main.go advertises an Intermediate fallback when TLS profile lookup/fetch fails, but it
terminates the process if tlspkg.GetTLSProfileSpec(nil) returns an error, preventing any fallback
and blocking operator startup in that case.

Code

cmd/main.go[R109-115]

+	// Fetch the TLS profile from apiservers.config.openshift.io/cluster.
+	// Fall back to Intermediate on non-OpenShift clusters (or if the fetch fails).
+	tlsSecurityProfileSpec, err := tlspkg.GetTLSProfileSpec(nil)
+	if err != nil {
+		setupLog.Error(err, "unable to get default TLS profile")
+		os.Exit(1)
+	}

Relevance

⭐⭐ Medium
No historical evidence on preferring fallback vs hard-exit for TLS default profile errors in main
startup.
ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code comment says it should fall back to Intermediate, but the error branch exits the process
immediately, so the fallback cannot occur when the default-profile call errors.

cmd/main.go[109-132]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`cmd/main.go` intends to fall back to an Intermediate TLS profile when OpenShift TLS profile discovery fails, but currently calls `os.Exit(1)` if `tlspkg.GetTLSProfileSpec(nil)` returns an error. This bypasses the fallback behavior entirely.

### Issue Context
The code path logs and comments about using an "Intermediate fallback" for non-OpenShift clusters or fetch failures, but it still hard-exits before the fetch/fallback code can run.

### Fix Focus Areas
- cmd/main.go[109-132]

### Suggested fix
- Replace the `os.Exit(1)` branch with:
 - a log that you’re falling back, and
 - an explicit initialization of `tlsSecurityProfileSpec` to the Intermediate profile (or another safe default), then continue startup.
- Ensure the log messages match the actual behavior (i.e., only claim "Intermediate fallback" when you actually set it).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. TLS profile fetch unguarded 📘 Rule violation ≡ Correctness

Description

cmd/main.go calls tlspkg.FetchAPIServerTLSProfile/FetchAPIServerTLSAdherencePolicy before any
explicit check that the OpenShift apiservers.config.openshift.io API is available. On
non-OpenShift clusters this performs requests against a potentially-missing API and relies on error
fallback instead of guarding access up front as required.

Code

cmd/main.go[R107-132]

+	restConfig := ctrl.GetConfigOrDie()
+
+	// Fetch the TLS profile from apiservers.config.openshift.io/cluster.
+	// Fall back to Intermediate on non-OpenShift clusters (or if the fetch fails).
+	tlsSecurityProfileSpec, err := tlspkg.GetTLSProfileSpec(nil)
+	if err != nil {
+		setupLog.Error(err, "unable to get default TLS profile")
+		os.Exit(1)
+	}
+	tlsAdherence := configv1.TLSAdherencePolicyNoOpinion
+
+	k8sClient, err := client.New(restConfig, client.Options{Scheme: scheme})
+	if err != nil {
+		setupLog.Info("unable to create client for TLS profile fetch, using Intermediate fallback", "error", err)
+	} else {
+		if profile, fetchErr := tlspkg.FetchAPIServerTLSProfile(ctx, k8sClient); fetchErr != nil {
+			setupLog.Info("unable to get TLS profile from API server, using Intermediate fallback", "error", fetchErr)
+		} else {
+			tlsSecurityProfileSpec = profile
+		}
+		if adherence, fetchErr := tlspkg.FetchAPIServerTLSAdherencePolicy(ctx, k8sClient); fetchErr != nil {
+			setupLog.Info("unable to get TLS adherence policy from API server", "error", fetchErr)
+		} else {
+			tlsAdherence = adherence
+		}
+	}

Relevance

⭐⭐⭐ High
Team favored explicit CRD/API guards; kept ServiceMonitor CRD existence check, rejected relying on
fetch errors.
PR-#1374

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 11 requires an explicit CRD/API existence guard before accessing optional kinds.
The added code performs FetchAPIServerTLSProfile and FetchAPIServerTLSAdherencePolicy calls
prior to any OpenShift/CRD availability check (platform detection happens later), so access is not
properly guarded.

Rule 11: Guard reconciliation logic on optional CRD existence
cmd/main.go[107-132]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`cmd/main.go` fetches the OpenShift APIServer TLS profile/adherence policy before verifying that the OpenShift `apiservers.config.openshift.io` API/CRD is installed. The compliance rule requires guarding access to optional CRDs/APIs with an explicit availability check before making client calls.

## Issue Context
The platform detection (`plf, err := controller.DetectPlatform()`) and OpenShift-only gating is currently done later (for the watcher), but the initial fetch happens unconditionally.

## Fix Focus Areas
- cmd/main.go[107-132]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. Mock Apply panics 🐞 Bug ☼ Reliability

Description

MockClient.Apply unconditionally panics, so any unit test (or test helper) that starts using
controller-runtime v0.23+ Apply semantics will crash instead of failing with a usable error.

Code

internal/controller/mock_client.go[R128-131]

+// Apply was added to client.Writer in controller-runtime v0.23.
+func (m MockClient) Apply(_ context.Context, _ runtime.ApplyConfiguration, _ ...client.ApplyOption) error {
+	panic(implementMe)
+}

Relevance

⭐⭐⭐ High
Team accepted removing panics in core code; likely expects MockClient.Apply to return error not
panic.
PR-#1949

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR adds a new Apply method that panics, and the mock client is actively used as the reconciler
client in unit tests, so invoking Apply would currently crash the tests.
internal/controller/mock_client.go[125-135]
internal/controller/monitor_test.go[20-30]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`MockClient.Apply(...)` was added to satisfy controller-runtime v0.23’s `client.Writer` interface, but it panics. This turns any future use of server-side apply in unit tests into a hard crash.

### Issue Context
The mock client is used in controller unit tests as the reconciler client.

### Fix Focus Areas
- internal/controller/mock_client.go[128-135]

### Suggested fix
- Replace the `panic(implementMe)` with a deterministic, non-panicking behavior:
 - Prefer returning a descriptive error like `fmt.Errorf("mock Apply not implemented")`.
 - Alternatively, implement minimal Apply semantics backed by the in-memory store (if tests are expected to use Apply soon).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

rhdh-qodo-merge · 2026-07-21T14:26:37Z

PR Summary by Qodo

Adopt OpenShift centralized TLS profile for operator servers

✨ Enhancement ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Align operator TLS settings with OpenShift APIServer/cluster TLS profile.
• Restart operator on TLS profile/adherence changes to reapply server config.
• Update RBAC/CSV and Go deps to support TLS profile watch + fetch.

Diagram

graph TD
  A["Operator main"] --> B["Create REST config"] --> C["Create k8s client"] --> D["Fetch TLS profile"] --> E[("APIServer: cluster")] --> F["Build tls.Config"] --> G["Start ctrl.Manager"] --> H["Serve HTTPS endpoints"]
  G --> I["SecurityProfileWatcher"] --> J{"Profile changed?"} --> K["cancel ctx"] --> L["Pod restarts"]
  I -."watch".-> E

  subgraph Legend
    direction LR
    _svc["Component"] ~~~ _api[("Cluster API resource")] ~~~ _dec{"Decision"}
  end

High-Level Assessment

The following are alternative approaches to this PR:

1. Manual APIServer watch + TLS config mapping

➕ Avoids adding controller-runtime-common + transitive deps
➕ Full control over logging, fallback behavior, and watcher semantics
➖ Reimplements OpenShift-standard behavior already available in shared library
➖ Higher maintenance burden; more risk of subtle divergence from OpenShift expectations

2. Dynamic TLS reload without restart

➕ No downtime or reconciliation interruption during TLS policy changes
➕ Avoids relying on pod restart behavior
➖ Harder to implement correctly across all controller-runtime servers
➖ May require deeper hooks into controller-runtime/http servers and careful concurrency handling

3. Only apply TLS profile when on OpenShift (no fallback)

➕ Simpler logic and clearer contract
➕ Avoids creating extra clients on non-OpenShift clusters
➖ Makes non-OpenShift behavior less predictable; reduces portability
➖ Harder for downstreams running on vanilla Kubernetes to reason about TLS defaults

Recommendation: Current approach (use openshift/controller-runtime-common for profile fetch + SecurityProfileWatcher and restart-on-change) is the best tradeoff: it standardizes behavior with other OpenShift operators and keeps the TLS mapping logic centralized. The restart-on-change strategy is operationally simple and reliable for ensuring all TLS listeners pick up the new configuration, at the cost of a brief manager restart.

Files changed (15) +143 / -97

Enhancement (1) +68 / -3

main.goFetch OpenShift TLS profile and restart manager on profile changes +68/-3

Fetch OpenShift TLS profile and restart manager on profile changes

• Adds OpenShift config APIs to the scheme, fetches the APIServer/cluster TLS profile (with Intermediate fallback), and builds tls.Config options for controller-runtime servers. Registers a SecurityProfileWatcher that cancels the manager context on profile/adherence changes to force a pod restart and reload TLS settings.

cmd/main.go

Refactor (5) +5 / -5