feat: integrate with cluster TLS security profile - #31
Conversation
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds TLS bootstrap logic that reads OpenShift API-server TLS profiles and adherence policies, applies derived options to metrics and webhook servers, and reloads through a security profile watcher. Adds tests for defaults, error classification, protocol selection, adherence handling, and unsupported ciphers. Grants read access to OpenShift API-server resources and updates Go dependencies. Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (45.45%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
==========================================
- Coverage 62.84% 62.29% -0.56%
==========================================
Files 15 16 +1
Lines 1674 1753 +79
==========================================
+ Hits 1052 1092 +40
- Misses 490 529 +39
Partials 132 132
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
b7bfa9f to
4be9266
Compare
|
I haven't tested this anyhow yet, but let's undraft to see how this goes and what coderabbitai has to say. |
4be9266 to
ada2934
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: 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 `@internal/tlsconfig/tlsconfig.go`:
- Around line 87-91: Update isTransientAPIError to recognize client-side context
deadlines by importing the standard errors package and including an errors.Is
check for context.DeadlineExceeded alongside the existing Kubernetes
transient-error checks.
🪄 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: 2fdda4a4-7f64-429a-b31e-00adc712f973
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (7)
charts/operator/templates/clusterrole.yamlcmd/main.goconfig/rbac/role.yamlgo.modinternal/controller/workbenches_controller.gointernal/tlsconfig/tlsconfig.gointernal/tlsconfig/tlsconfig_test.go
ada2934 to
9f1c257
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/workbenches_controller.go (1)
140-146: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMissing OwnerReferences for managed child resources (CWE-459).
The controller watches
appsv1.Deploymentvia a custom event mapper instead of.Owns(). This violates the path instruction to setOwnerReferenceson all child resources. Without owner references, Kubernetes garbage collection cannot track and delete child resources when the parent CR is deleted, leading to resource leaks (CWE-459). Refactor the manifest application logic to injectOwnerReferencesand replace this watch with.Owns(&appsv1.Deployment{}).🤖 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 `@internal/controller/workbenches_controller.go` around lines 140 - 146, The Workbenches controller currently maps Deployment events without establishing ownership. Update the manifest application logic to inject the owning Workbenches resource into child Deployment OwnerReferences, then replace the custom deployment watch in ctrlBuilder with .Owns(&appsv1.Deployment{}), preserving the availability-change predicate if applicable.Source: Path instructions
🤖 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.
Outside diff comments:
In `@internal/controller/workbenches_controller.go`:
- Around line 140-146: The Workbenches controller currently maps Deployment
events without establishing ownership. Update the manifest application logic to
inject the owning Workbenches resource into child Deployment OwnerReferences,
then replace the custom deployment watch in ctrlBuilder with
.Owns(&appsv1.Deployment{}), preserving the availability-change predicate if
applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 17b15231-18f3-409e-b95a-894acf75295e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (7)
charts/operator/templates/clusterrole.yamlcmd/main.goconfig/rbac/role.yamlgo.modinternal/controller/workbenches_controller.gointernal/tlsconfig/tlsconfig.gointernal/tlsconfig/tlsconfig_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- config/rbac/role.yaml
- charts/operator/templates/clusterrole.yaml
- cmd/main.go
- internal/tlsconfig/tlsconfig.go
- go.mod
Read the cluster TLS profile from apiservers.config.openshift.io/cluster at startup. Apply MinVersion, CipherSuites, and NextProtos to webhook and metrics server TLS configs. Fail closed on unexpected errors. Use Mozilla Intermediate defaults on non-OpenShift clusters. 1. Transient error handling: IsServiceUnavailable/IsTimeout/IsTooManyRequests fall back to hardened defaults instead of crashing. 2. TLSAdherencePolicy: fetches the OCP 5.0 adherence policy and wires it into SecurityProfileWatcher. 3. Watcher self-healing: on transient errors, the watcher still registers and self-heals when the API recovers. 4. 10s context timeout for bootstrap TLS fetch.
9f1c257 to
429bc4e
Compare
harshad16
left a comment
harshad16
left a comment
There was a problem hiding this comment.
Thanks for the TLS integration.
Great direction toward cluster-wide TLS profile compliance
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: harshad16, jstourac 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 |
36ce8ae
into
opendatahub-io:main
3 participants
https://redhat.atlassian.net/browse/RHOAIENG-76050
Summary
Integrate
controller-runtime-common/pkg/tlsTLS profile support into the workbenches-operator to comply with the OCP 5.0 centralized TLS profile requirement (OCPSTRAT-2611).MinVersion=TLS12+ Mozilla Intermediate cipher fallback for non-OpenShift clustersSecurityProfileWatcherto trigger graceful restart on profile changesTLSAdherencePolicyinto the watcher (inert on OCP 4.19, active when the FeatureGate is available)config.openshift.io/apiservers(get/list/watch)Motivation
OCP 5.0 (GA October 2026) requires all components to honor the centralized TLS profile (OCPSTRAT-2611). This is modeled after the same integration done for odh-notebook-controller in opendatahub-io/kubeflow#836 and its follow-up opendatahub-io/kubeflow#847.
Ref: RHOAIENG-76050
How Has This Been Tested?
Build & manifests
go build ./...passesmake manifestsregenerates RBAC cleanly (no uncommitted diff)apiserverspermission (hack/chart-sync-rbac.sh)OpenShift cluster (OCP 4.19+)
unable to read APIServer TLS profileerroroc get clusterrole <operator-role> -o yamlshould includeconfig.openshift.io/apiserverswithget,list,watchverbsoc edit apiserver clusterand modifyspec.tlsSecurityProfile(e.g., switch fromIntermediatetoOldorCustom)"TLS profile changed, initiating graceful shutdown to reload"and the pod restartingNon-OpenShift / vanilla Kubernetes
"TLS profile not available, using hardened defaults (non-OpenShift cluster)"Merge criteria:
Summary by CodeRabbit
New Features
Bug Fixes