feat: integrate odh-notebook-controller with cluster TLS profile - #836
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR updates Kubernetes/controller-runtime and OpenShift API dependency versions in both notebook-controller and odh-notebook-controller. The webhook registration refactors from fluent chaining to direct argument passing. RBAC permissions are expanded to allow config.openshift.io/apiservers access. The odh-notebook-controller main bootstraps the OpenShift TLS security profile at startup, derives TLS options with explicit NextProtos, applies them to metrics and webhook servers, and registers a SecurityProfileWatcher that cancels the manager context to trigger graceful reload on profile changes. The Notebook CRD schema is extended across all versioned sections with fileKeyRef environment variable sources, restartPolicyRules for container restarts, hostnameOverride for pods, workloadRef for workload identification, and podCertificate for projected volume sources. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Supply Chain & Security ObservationsDependency Updates (CWE-1324: Weak Supply Chain)
Local Replace Directive (CWE-494: Supply Chain)
TLS/Crypto Configuration (CWE-295: Improper Certificate Validation)
SecurityProfileWatcher Context Cancellation (CWE-400: Uncontrolled Resource Consumption)
RBAC Expansion (CWE-276: Incorrect Default Permissions)
🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
/group-test |
604e2d6 to
5a6738c
Compare
|
/group-test |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #836 +/- ##
==========================================
- Coverage 63.36% 63.16% -0.21%
==========================================
Files 15 15
Lines 2962 2962
==========================================
- Hits 1877 1871 -6
- Misses 899 903 +4
- Partials 186 188 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (2)
components/odh-notebook-controller/main.go (2)
186-188: 💤 Low valueNextProtos override silently discards profile's protocol preferences.
Lines 186-188 unconditionally set
NextProtos = ["h2", "http/1.1"]after the profile-based TLS config is applied. This overwrites any NextProtos directives from the TLS profile, which could surprise operators who expect the profile to control all TLS settings.While the chosen protocols (h2, http/1.1) are appropriate for Kubernetes webhook and metrics servers, the silent override should be documented:
// Force h2 and http/1.1 for webhook/metrics compatibility, overriding profile settings tlsOpts = append(tlsOpts, func(c *tls.Config) { c.NextProtos = []string{"h2", "http/1.1"} })🤖 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 `@components/odh-notebook-controller/main.go` around lines 186 - 188, The code unconditionally overwrites TLS NextProtos by appending a tlsOpts function that sets c.NextProtos = []string{"h2","http/1.1"}, which silently discards any profile-provided NextProtos; change this to either preserve profile preferences or make the override explicit and documented: update the tlsOpts append (the function that mutates tls.Config.NextProtos) to check for an existing NextProtos in the config and only set it when empty, or add a clear comment above the append explaining that NextProtos is intentionally forced for webhook/metrics compatibility (mentioning NextProtos, tlsOpts and tls.Config to locate the code).
168-169: 💤 Low valueBootstrap timeout of 10 seconds may be insufficient in slow environments.
In resource-constrained clusters or during initial cluster startup, the API server may take longer than 10 seconds to respond. This would trigger fallback to implicit defaults (see prior comment) when the profile is actually available.
Consider making the timeout configurable via flag:
flag.DurationVar(&bootstrapTimeout, "tls-profile-fetch-timeout", 10*time.Second, "Timeout for fetching OpenShift TLS profile during startup")🤖 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 `@components/odh-notebook-controller/main.go` around lines 168 - 169, Replace the hardcoded 10s context timeout used in context.WithTimeout (bootstrapCtx, bootstrapCancel := context.WithTimeout(context.Background(), 10*time.Second)) with a configurable flag variable (e.g., bootstrapTimeout) so the TLS profile fetch timeout can be tuned; add a package-level or main-local variable bootstrapTimeout and register it with flag.DurationVar(&bootstrapTimeout, "tls-profile-fetch-timeout", 10*time.Second, "Timeout for fetching OpenShift TLS profile during startup"), ensure flag.Parse() runs before using bootstrapTimeout, then call context.WithTimeout(context.Background(), bootstrapTimeout) and keep defer bootstrapCancel() as before.
🤖 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 `@components/odh-notebook-controller/go.mod`:
- Line 97: The replace directive currently pointing the module
github.com/kubeflow/kubeflow/components/notebook-controller at a local
filesystem path must be removed and replaced with a versioned reference to
ensure integrity and reproducible builds; update the go.mod replace for the
module github.com/kubeflow/kubeflow/components/notebook-controller to reference
a specific published version or a pseudo-version that includes the commit SHA
(or switch to a fork with a tagged release) so the dependency has a verifiable
checksum in go.sum and cannot be silently substituted by local files.
In `@components/odh-notebook-controller/main.go`:
- Around line 176-178: When the TLS profile fetch fails (the err != nil branch),
explicitly set a hardened fallback TLS configuration instead of relying on
implicit Go defaults: update the err != nil block (where setupLog.Info is
called) to append a tls.Options setter to tlsOpts that sets c.MinVersion =
tls.VersionTLS12 and a conservative CipherSuites list (e.g., ECDHE AES-GCM
suites), and adjust the log to say "using hardened defaults"; reference tlsOpts
and the err != nil branch in main.go to locate where to add this fallback.
- Around line 298-314: The TLS profile watcher currently calls cancel()
immediately in tlspkg.SecurityProfileWatcher.OnProfileChange which can cause
restart storms; modify the OnProfileChange handler to debounce restarts (e.g.,
start or reset a single timer/AfterFunc for ~30s) and only call cancel() when
that timer fires, ensuring you guard the timer with a small mutex or an atomic
flag to avoid concurrent timers; update the handler attached to
watcher.SetupWithManager(mgr) so rapid successive profile changes reset the
debounce timer instead of immediately calling cancel().
- Around line 164-189: The OpenShift TLS profile returned by
tlspkg.FetchAPIServerTLSProfile is used directly in
tlspkg.NewTLSConfigFromProfile (building tlsOpts) without validating minimum TLS
version, disallowed ciphers, or InsecureSkipVerify/certificate verification
settings; add a validateTLSProfile(profile) helper and call it after
FetchAPIServerTLSProfile and before NewTLSConfigFromProfile to enforce:
MinTLSVersion is TLS1.2 or TLS1.3, profile.Ciphers does not include
NULL/EXPORT/MD5/SHA1/RC4/3DES patterns, and any settings that would disable
certificate verification (e.g., InsecureSkipVerify) are rejected; on validation
failure log via setupLog.Error and fall back to secure defaults (do not append
tlsConfigFn) so the webhook/metrics servers never run with an insecure profile.
---
Nitpick comments:
In `@components/odh-notebook-controller/main.go`:
- Around line 186-188: The code unconditionally overwrites TLS NextProtos by
appending a tlsOpts function that sets c.NextProtos = []string{"h2","http/1.1"},
which silently discards any profile-provided NextProtos; change this to either
preserve profile preferences or make the override explicit and documented:
update the tlsOpts append (the function that mutates tls.Config.NextProtos) to
check for an existing NextProtos in the config and only set it when empty, or
add a clear comment above the append explaining that NextProtos is intentionally
forced for webhook/metrics compatibility (mentioning NextProtos, tlsOpts and
tls.Config to locate the code).
- Around line 168-169: Replace the hardcoded 10s context timeout used in
context.WithTimeout (bootstrapCtx, bootstrapCancel :=
context.WithTimeout(context.Background(), 10*time.Second)) with a configurable
flag variable (e.g., bootstrapTimeout) so the TLS profile fetch timeout can be
tuned; add a package-level or main-local variable bootstrapTimeout and register
it with flag.DurationVar(&bootstrapTimeout, "tls-profile-fetch-timeout",
10*time.Second, "Timeout for fetching OpenShift TLS profile during startup"),
ensure flag.Parse() runs before using bootstrapTimeout, then call
context.WithTimeout(context.Background(), bootstrapTimeout) and keep defer
bootstrapCancel() as before.
🪄 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: 57e7e600-e30b-4246-bdb7-4ffe59af6ebd
⛔ Files ignored due to path filters (2)
components/notebook-controller/go.sumis excluded by!**/*.sum,!**/*.sumcomponents/odh-notebook-controller/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (5)
components/notebook-controller/api/v1beta1/notebook_webhook.gocomponents/notebook-controller/go.modcomponents/odh-notebook-controller/controllers/notebook_controller.gocomponents/odh-notebook-controller/go.modcomponents/odh-notebook-controller/main.go
|
/group-test |
|
/group-test |
60a7e33 to
7cadeea
Compare
ugiordan
changed the title
|
/group-test |
7e68e35 to
a7b58dc
Compare
jstourac
left a comment
jstourac
left a comment
There was a problem hiding this comment.
Thank you for this. I put some comments and replied to the ones from coderabbitai.
Do you plan to rebase this against main so that only true changes brought in by this PR are shown here?
|
/group-test |
a7b58dc to
4931cd0
Compare
|
/group-test |
4931cd0 to
8bd8822
Compare
|
/group-test |
1 similar comment
|
/group-test |
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 Intermediate defaults on non-OpenShift clusters. Signed-off-by: Ugo Giordano <ugiordan@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ugo Giordano <ugiordan@redhat.com>
|
/group-test |
|
I just tried on my OCP 4.21.18 cluster. I installed a very recent RHOAI 3.5.0-ea.2 nightly build and updated the operator CSV so that it incorporates nbc builds from this PR: I also added Then the odh-kf-notebook-controller seem to work just fine and nothing extraordinary is seen in its log. I haven't noticed any specific error with regards the events as was identified by the cursor dependency bump analysis shared above. So, hopefully this is good too (though, I don't have OCP 4.19 cluster at the moment!). |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
Great analysis, thanks. Risk 1 (go.mod comment): Fixed. Risk 2 (Events RBAC): Verified, not an issue for us. We call Risks 3-5: All transitive from the CR v0.23.3 bump. structured-merge-diff v6, gogo/protobuf removal, and PriorityQueue default are all pulled in by CR v0.23.3 and k8s v0.35. We don't configure any custom queue settings and don't directly depend on gogo/protobuf. Nothing actionable on our side. |
4 participants
Summary
controller-runtime-common/pkg/tlsTLS profile support into odh-notebook-controllerMinVersion=TLS12fallback for non-OpenShift clustersSecurityProfileWatcherto restart on profile changesconfig.openshift.io/apiservers(get/list/watch)Motivation
OCP 5.0 (GA October 2026) requires all components to honor the centralized TLS profile (OCPSTRAT-2611).
Reference: openshift/cluster-machine-approver #286
Test plan
go build ./...passesgofmtcleanRef: RHOAIENG-67673