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 adds OpenShift TLS profile bootstrap and dynamic reload to the feast operator. The operator now imports OpenShift config/v1 types, registers them in the controller-runtime scheme, fetches TLS profile and adherence policy at startup via a timeout-scoped bootstrap client (falling back on NotFound/NoMatch errors, failing fast on others), builds TLS options from profiles, registers a Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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 |
6eeb2b5 to
e6fe91f
Compare
ugiordan
changed the title
ugiordan
changed the title
e6fe91f to
29495ec
Compare
jyejare
left a comment
jyejare
left a comment
There was a problem hiding this comment.
This PR adds cluster TLS security profile integration to the Feast operator, enabling it to fetch OpenShift's cluster TLS configuration and adapt accordingly. The implementation follows OpenShift security best practices with proper fallback handling and includes extensive CRD updates. However, there are some concerns around HTTP/2 configuration and error handling that should be addressed.
| // Register SecurityProfileWatcher to restart on TLS profile changes | ||
| ctx, cancel := context.WithCancel(ctrl.SetupSignalHandler()) | ||
| defer cancel() | ||
|
|
||
| if tlsProfileFetched { | ||
| watcher := &tlspkg.SecurityProfileWatcher{ | ||
| Client: mgr.GetClient(), | ||
| InitialTLSProfileSpec: tlsProfile, | ||
| OnProfileChange: func(_ context.Context, _, _ configv1.TLSProfileSpec) { | ||
| setupLog.Info("TLS profile changed, initiating shutdown to reload") | ||
| cancel() | ||
| }, | ||
| } | ||
| if tlsAdherenceFetched { | ||
| watcher.InitialTLSAdherencePolicy = tlsAdherence | ||
| watcher.OnAdherencePolicyChange = func(_ context.Context, _, _ configv1.TLSAdherencePolicy) { | ||
| setupLog.Info("TLS adherence policy changed, initiating shutdown to reload") | ||
| cancel() | ||
| } | ||
| } | ||
| if err := watcher.SetupWithManager(mgr); err != nil { | ||
| setupLog.Error(err, "unable to set up TLS profile watcher") | ||
| os.Exit(1) | ||
| } | ||
| } | ||
|
|
||
| if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { |
There was a problem hiding this comment.
[Suggestion] Consider more graceful shutdown handling
The TLS profile watcher immediately cancels the context when profiles change, which triggers operator shutdown. While this ensures the new TLS config is applied, it might be disruptive in environments with frequent TLS profile changes. Consider adding a delay or debouncing mechanism.
There was a problem hiding this comment.
The SecurityProfileWatcher from controller-runtime-common invokes a caller-provided OnProfileChange callback when the profile changes. In our implementation (same as the OCP reference), that callback calls cancel() on the manager context, which triggers a graceful shutdown. Kubelet then restarts the pod with the new TLS config.
This is the standard pattern from openshift/cluster-machine-approver, and it's built into the SecurityProfileWatcher API design.
Debouncing isn't needed because TLS profile changes are rare cluster-level operations, and kubelet's restart backoff naturally handles the case where multiple changes happen in quick succession.
29495ec to
e1a1952
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ugiordan 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 |
e1a1952 to
bc90065
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infra/feast-operator/cmd/main.go (1)
112-112: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDead code:
enableHTTP2flag and stale comment blockThe
enableHTTP2flag (line 112, 123-124) is declared and parsed but never read—NextProtosis now unconditionally set at lines 181-183. The comment block at lines 136-141 references the old HTTP/2 disabling behavior that no longer exists.Remove to avoid confusion:
Proposed cleanup
var secureMetrics bool - var enableHTTP2 bool var featureStoreMetrics bool- flag.BoolVar(&enableHTTP2, "enable-http2", false, - "If set, HTTP/2 will be enabled for the metrics and webhook servers")- // if the enable-http2 flag is false (the default), http/2 should be disabled - // due to its vulnerabilities. More specifically, disabling http/2 will - // prevent from being vulnerable to the HTTP/2 Stream Cancellation and - // Rapid Reset CVEs. For more information see: - // - https://github.com/advisories/GHSA-qppj-fm5r-hxr3 - // - https://github.com/advisories/GHSA-4374-p667-p6c8 // Fetch cluster TLS profile from apiservers.config.openshift.io/clusterAlso applies to: 123-124, 136-141
🤖 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 `@infra/feast-operator/cmd/main.go` at line 112, The `enableHTTP2` variable declaration and its associated flag parsing are dead code that is never used since `NextProtos` is now set unconditionally elsewhere in the code. Remove the `enableHTTP2` variable declaration, remove the flag parsing code that registers this flag, and remove the stale comment block that references the old HTTP/2 disabling behavior that no longer exists. These removals will clean up confusion and eliminate unused code.
🤖 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 `@infra/feast-operator/cmd/main.go`:
- Around line 150-166: The bootstrap API calls use context.Background() without
any deadline, which can cause indefinite blocking and operator startup hangs.
Replace context.Background() with a context created using context.WithTimeout or
context.WithDeadline to add a reasonable timeout for the
FetchAPIServerTLSProfile call around line 151, and apply the same timeout
context to the other bootstrap API call mentioned at line 169. This ensures the
operator fails fast if the API server is slow or unreachable rather than hanging
indefinitely.
- Around line 181-183: The tlsOpts append operation unconditionally enables
HTTP/2 by setting NextProtos to include "h2", but this should only happen when
the enableHTTP2 flag is true. Wrap the tlsOpts append call that sets
c.NextProtos with a conditional check for the enableHTTP2 flag, so that HTTP/2
is only added to the TLS configuration when the flag is explicitly enabled. If
enableHTTP2 is false, either skip appending this TLS option entirely or
explicitly set NextProtos to only include "http/1.1".
---
Outside diff comments:
In `@infra/feast-operator/cmd/main.go`:
- Line 112: The `enableHTTP2` variable declaration and its associated flag
parsing are dead code that is never used since `NextProtos` is now set
unconditionally elsewhere in the code. Remove the `enableHTTP2` variable
declaration, remove the flag parsing code that registers this flag, and remove
the stale comment block that references the old HTTP/2 disabling behavior that
no longer exists. These removals will clean up confusion and eliminate unused
code.
🪄 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: dce0ab20-8128-4620-aa0f-88e95a10d4f2
⛔ Files ignored due to path filters (2)
infra/feast-operator/dist/install.yamlis excluded by!**/dist/**infra/feast-operator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (5)
infra/feast-operator/cmd/main.goinfra/feast-operator/config/crd/bases/feast.dev_featurestores.yamlinfra/feast-operator/config/rbac/role.yamlinfra/feast-operator/go.modinfra/feast-operator/internal/controller/featurestore_controller.go
🚧 Files skipped from review as they are similar to previous changes (3)
- infra/feast-operator/internal/controller/featurestore_controller.go
- infra/feast-operator/config/rbac/role.yaml
- infra/feast-operator/go.mod
33881a8 to
c92fcdf
Compare
|
/retest |
|
@ugiordan Should the changes be applied at upstream itself? |
jyejare
added
ok-to-test
sync-to-stable
Good question. The feast-operator already depends on openshift/api upstream (https://github.com/feast-dev/feast/blob/master/infra/feast-operator/go.mod). The TLS code gracefully falls back to hardened defaults on non-OpenShift clusters ( |
c92fcdf to
e82c4b5
Compare
|
@jyejare any thoughts? |
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 webhook and 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. Fails closed on OpenShift (abort on 403/transport errors), 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>
e82c4b5 to
817ef09
Compare
|
/retest |
|
@ugiordan: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@ugiordan: The following test has Succeeded: OCI Artifact Browser URLInspecting Test Artifacts ManuallyTo inspect your test artifacts manually, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/opendatahub/odh-ci-artifacts:feast-group-test-vsklj |
@ugiordan Yes, I think it would be great if this change goes upstream even though it;s openshift specific. It would cause less conflicts while syncing code in future. |
Here it is feast-dev#6567 |
|
Closing this midstream PR. The upstream PR (feast-dev#6567) has been merged. The changes will come into the midstream via the regular upstream sync. @ntkathole, @jyejare, once the sync cherry-pick PR is created, please link it to RHOAIENG-67682. |
4 participants
Summary
apiservers.config.openshift.io/clustercontroller-runtime-common/pkg/tlsto fetch the profile at startup and apply it to webhook and metrics server TLSOptsSecurityProfileWatcherto restart on profile or adherence policy 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
Changes
infra/feast-operator/cmd/main.go: TLS profile integration (fetch profile, build tls.Config, apply TLSOpts, register watcher)infra/feast-operator/internal/controller/featurestore_controller.go: RBAC marker for apiserversinfra/feast-operator/config/rbac/role.yaml: regenerated with apiservers permissioninfra/feast-operator/go.mod/go.sum: addedcontroller-runtime-common, controller-runtime upgraded to v0.23.3Test plan
go build ./...passesopenssl s_client -alpnRef: RHOAIENG-67682
Summary by CodeRabbit