feat: integrate with cluster TLS security profile - #140
Merged
Merged
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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:
📝 WalkthroughWalkthroughThe PR consumes OpenShift API server TLSProfile (and optional adherence) to build tls.Options for the operator metrics server (always forcing NextProtos=http/1.1), moves ctrl.GetConfigOrDie() earlier for reuse, replaces the local HTTP/2-disable helper, registers a SecurityProfileWatcher that cancels the manager context on profile/adherence changes so mgr.Start(ctx) exits, adds RBAC for apiservers.config.openshift.io, bumps go.mod dependencies and Go toolchain, updates Makefile GOBIN evaluation, and updates the Docker builder image to Go toolset 1.25. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Security observations
🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
ugiordan
force-pushed
the
RHOAIENG-61072-tls-profile
branch
from
June 10, 2026 09:14
9520138 to
390db04
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/main.go`:
- Around line 126-139: The bootstrap reads use context.Background() which can
block indefinitely; hoist rootCtx := ctrl.SetupSignalHandler() before these
fetches and replace Background with derived contexts that have timeouts (e.g.,
ctx, cancel := context.WithTimeout(rootCtx, <reasonableDuration>); defer
cancel()) when calling tlspkg.FetchAPIServerTLSProfile and
tlspkg.FetchAPIServerTLSAdherencePolicy (which populate tlsProfileFetched,
tlsAdherenceFetched and feed tlsConfigFn / unsupported handling); ensure you
pass the timeout-backed ctx and cancel appropriately so the process fails fast
instead of hanging on bootstrapClient reads.
- Around line 126-141: The current code treats any error from
tlspkg.FetchAPIServerTLSProfile and tlspkg.FetchAPIServerTLSAdherencePolicy as
benign and continues with defaults; change this to "fail closed": only treat
explicit API/resource-absent errors as recoverable (e.g.,
apierrors.IsNotFound(err) or meta.IsNoMatchError(err)); for all other errors log
them with setupLog.Error (include the error) and abort startup (return non-nil
error from main or call os.Exit(1)). Update the blocks around
FetchAPIServerTLSProfile / tlsProfileFetched and
FetchAPIServerTLSAdherencePolicy / tlsAdherenceFetched so
tlsProfileFetched/tlsAdherenceFetched are set only on success and
non-recoverable errors cause process termination while recoverable
IsNotFound/IsNoMatch cases fall back to defaults.
In `@go.mod`:
- Around line 11-13: The go.mod currently pins modules using pseudo-versions
which weakens provenance; replace the pseudo-versions for
github.com/opendatahub-io/operator-chaos, github.com/openshift/api, and
github.com/openshift/controller-runtime-common with their appropriate tagged
releases (or an approved, audited tag/announced release) instead of the
v0.0.0-YYYYMMDD... pseudo-versions: update the require entries for these module
paths to the correct semantic version tags (or verified release commit hashes)
and run `go get`/`go mod tidy` to refresh go.sum so the module graph uses the
tagged releases.
- Line 3: Update the Go toolchain version in the module directive to a patched
1.25.x release to avoid known security issues: change the go directive in go.mod
from "go 1.25.0" to a patched version such as "go 1.25.11" so CI (which reads
go.mod) will use the fixed toolchain; after updating, run go mod tidy and your
CI build to verify no further version conflicts, and review any pseudo-versioned
dependencies (operator-chaos / OpenShift/k8s) for resolved secure releases
before merging.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ab6ba34c-6a43-4dc3-a36a-58754f6a959a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (3)
cmd/main.gogo.modinternal/controller/mlflow_controller.go
ugiordan
force-pushed
the
RHOAIENG-61072-tls-profile
branch
5 times, most recently
from
June 10, 2026 09:34
3bdb8c0 to
85eca00
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
go.mod (2)
3-3:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGo 1.25.0 remains obsolete; CVE-2026-39825 & CVE-2026-39819 still unpatched.
This was flagged in a prior review but the toolchain directive still pins
go 1.25.0instead of a patched release (e.g.,go 1.25.11). CI will continue to use the vulnerable toolchain version.🤖 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 3, The go.mod toolchain is pinned to the vulnerable string "go 1.25.0"; update that directive to a patched release (for example change "go 1.25.0" -> "go 1.25.11"), then run go mod tidy to refresh module files and ensure CI/toolchain configuration (any workflow or Dockerfile that pins the Go version) is updated to the same patched version so builds no longer use the vulnerable Go 1.25.0 toolchain.
11-12:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCWE-829: OpenShift pseudo-versions remain unpinned to tagged releases.
This was flagged in a prior review.
openshift/apiandopenshift/controller-runtime-commonstill use pseudo-versions instead of semantic version tags, weakening supply-chain auditability.🤖 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` around lines 11 - 12, The go.mod currently pins github.com/openshift/api and github.com/openshift/controller-runtime-common to pseudo-versions; update those entries to specific semantic version tags for supply-chain traceability by editing go.mod (look for the module lines for github.com/openshift/api and github.com/openshift/controller-runtime-common) and replace the pseudo-versions with the corresponding released tags (or run `go get github.com/openshift/api@<tag>` and `go get github.com/openshift/controller-runtime-common@<tag>` to resolve and update to the canonical tagged versions), then run `go mod tidy` to ensure the lockfile and dependencies are consistent.
🤖 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 `@go.mod`:
- Line 54: The go.mod contains indirect pseudo-versions (github.com/google/pprof
v0.0.0-20260115054156-294ebfa9ad83, github.com/openshift/library-go
v0.0.0-20260213153706-03f1709971c5, and sigs.k8s.io/structured-merge-diff/v6
v6.3.2-0.20260122202528-d9cc6641c482) which weakens supply-chain provenance;
update the sigs.k8s.io/structured-merge-diff/v6 entry to the corresponding
stable v6.x.y tag (e.g., run go get sigs.k8s.io/structured-merge-diff/v6@v6.x.y
and then go mod tidy), and for github.com/google/pprof and
github.com/openshift/library-go either replace each pseudo-version with a
published tagged release if available or add a short rationale comment in the
repository (and a tracking TODO) explaining why the untagged commit is required
and confirming no suitable tag exists, then run go mod tidy to refresh go.sum.
---
Duplicate comments:
In `@go.mod`:
- Line 3: The go.mod toolchain is pinned to the vulnerable string "go 1.25.0";
update that directive to a patched release (for example change "go 1.25.0" ->
"go 1.25.11"), then run go mod tidy to refresh module files and ensure
CI/toolchain configuration (any workflow or Dockerfile that pins the Go version)
is updated to the same patched version so builds no longer use the vulnerable Go
1.25.0 toolchain.
- Around line 11-12: The go.mod currently pins github.com/openshift/api and
github.com/openshift/controller-runtime-common to pseudo-versions; update those
entries to specific semantic version tags for supply-chain traceability by
editing go.mod (look for the module lines for github.com/openshift/api and
github.com/openshift/controller-runtime-common) and replace the pseudo-versions
with the corresponding released tags (or run `go get
github.com/openshift/api@<tag>` and `go get
github.com/openshift/controller-runtime-common@<tag>` to resolve and update to
the canonical tagged versions), then run `go mod tidy` to ensure the lockfile
and dependencies are consistent.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 006d144b-ce98-49e9-93bf-7cea97f93cf3
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (5)
Dockerfilecmd/main.goconfig/rbac/role.yamlgo.modinternal/controller/mlflow_controller.go
✅ Files skipped from review due to trivial changes (1)
- Dockerfile
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/controller/mlflow_controller.go
- config/rbac/role.yaml
- cmd/main.go
ugiordan
force-pushed
the
RHOAIENG-61072-tls-profile
branch
3 times, most recently
from
June 10, 2026 09:44
31080b9 to
991e555
Compare
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 `@Makefile`:
- Around line 6-12: The Makefile currently silences all errors from the
`GOTOOLCHAIN=local go env ... 2>/dev/null` calls which can produce a bad or
empty GOBIN (e.g., "/bin") and mask missing/broken Go installations; remove the
`2>/dev/null` redirections so `go env` errors are visible, capture stderr to a
temp logfile if noise is a concern, and add validation after computing GOBIN
(from the `GOTOOLCHAIN=local go env GOBIN`/`GOPATH` branches) to fail fast if
GOBIN is empty or points to unsafe locations like "/" or "/bin" (emit a clear
error message referencing GOBIN and GOTOOLCHAIN and exit non‑zero).
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 47e45f3b-36ba-46ec-b54c-c83f6c5e03de
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (6)
DockerfileMakefilecmd/main.goconfig/rbac/role.yamlgo.modinternal/controller/mlflow_controller.go
✅ Files skipped from review due to trivial changes (1)
- Dockerfile
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/main.go
- go.mod
ugiordan
force-pushed
the
RHOAIENG-61072-tls-profile
branch
from
June 10, 2026 10:20
991e555 to
03881ad
Compare
This was referenced Jun 10, 2026
ugiordan
changed the title
ugiordan
force-pushed
the
RHOAIENG-61072-tls-profile
branch
from
June 12, 2026 08:11
03881ad to
437b179
Compare
ugiordan
force-pushed
the
RHOAIENG-61072-tls-profile
branch
from
June 16, 2026 11:05
437b179 to
cfca0e8
Compare
Honor the cluster-wide TLS security profile from apiservers.config.openshift.io/cluster instead of hardcoding TLS settings. Uses controller-runtime-common/pkg/tls to fetch the profile at startup, apply it to the metrics server TLSOpts, and watch for profile changes via SecurityProfileWatcher. On profile or adherence policy change, the manager context is cancelled so the pod restarts with the new configuration. Gracefully falls back to defaults if the APIServer resource is not available (non-OpenShift environments). RHOAIENG-61072 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ugo Giordano <ugiordan@redhat.com>
ugiordan
force-pushed
the
RHOAIENG-61072-tls-profile
branch
from
June 26, 2026 15:44
cfca0e8 to
7cb44fa
Compare
Keep the nested api module on the same Kubernetes/controller-runtime stack as the root module, and let the metrics e2e accept either HTTP/1.1 or HTTP/2 now that the TLS profile work can negotiate h2.
mprahl
approved these changes
Jun 29, 2026
Contributor
|
/retest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
apiservers.config.openshift.io/clustercontroller-runtime-common/pkg/tlsto fetch the profile at startup and apply it to metrics server TLSOptsSecurityProfileWatcherto restart on profile or adherence policy changesconfig.openshift.io/apiservers(get/list/watch)Motivation
RHOAIENG-61072
OCP 5.0 (GA October 2026) requires all components to honor the centralized TLS profile. This is a release blocker (OCPSTRAT-2611). Components that do not comply receive Critical bugs.
Reference: OCP TLS Implementation Reference
Upstream example: openshift/cluster-machine-approver #286
Changes
cmd/main.go: 5-step TLS profile integration (fetch profile, build tls.Config, apply TLSOpts, register watcher, cancellable context)internal/controller/mlflow_controller.go: RBAC marker for apiserversgo.mod/go.sum: addedcontroller-runtime-common,controller-runtimeupgraded v0.22.4 to v0.23.3Test plan
go build ./...passesopenssl s_client -alpnSummary by CodeRabbit
New Features
Chores