OLS-3450: Add credentialHotReload flag to skip app-server restart on LLM secret rotation#1779
OLS-3450: Add credentialHotReload flag to skip app-server restart on LLM secret rotation#1779lalan7 wants to merge 6 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@lalan7: This pull request references OLS-3450 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughAdds an optional ChangesCredential Hot-Reload
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OLSConfig
participant OLSConfigReconciler
participant LLMSecret
participant AppServerConfig
participant SecretWatcherFilter
participant AppServerDeployment
OLSConfig->>OLSConfigReconciler: provide CredentialHotReload
OLSConfigReconciler->>AppServerConfig: set credential_hot_reload
OLSConfigReconciler->>LLMSecret: remove watcher annotation
LLMSecret-->>SecretWatcherFilter: credential secret event
alt hot-reload enabled
SecretWatcherFilter-->>AppServerDeployment: no restart trigger
else hot-reload disabled
SecretWatcherFilter->>AppServerDeployment: trigger restart
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
[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 |
![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
🧹 Nitpick comments (1)
internal/controller/watchers/watchers_test.go (1)
363-471: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the repeated Deployment/reconciler setup into a helper.
The three new tests duplicate identical
dep/createTestReconcilerboilerplate. A small local helper (e.g.,newTestDeploymentAndReconciler(cr)) would reduce repetition within this file.🤖 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/watchers/watchers_test.go` around lines 363 - 471, The new Credential hot-reload tests duplicate the same Deployment and reconciler setup in each case. Extract that repeated boilerplate into a small local helper in watchers_test.go, such as a helper that builds the standard Deployment and calls createTestReconciler, then reuse it in the three Describe/It blocks while keeping the test-specific watcher config and Secret setup inline.
🤖 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/controller/utils/types.go`:
- Around line 60-73: `WatcherConfig` is shared mutable state and is accessed
concurrently by `SecretWatcherFilter()` while `annotateExternalResources()`
updates `CredentialHotReload` and `LLMSecretNames`; add synchronization or
switch to an immutable snapshot approach. Update the `WatcherConfig` type in
`types.go` to guard these fields with a mutex (or refactor to store/retrieve a
copied config atomically), and make both `SecretWatcherFilter()` and
`annotateExternalResources()` use the same protection when reading or writing
`CredentialHotReload` and `LLMSecretNames`.
In `@test/e2e/reconciliation_test.go`:
- Around line 400-416: The reconciliation test uses a fixed sleep after updating
OLSConfig.CredentialHotReload, which makes the hot-reload setup timing-dependent
and flaky. Replace the blind time.Sleep in the credentialHotReload setup with an
Eventually-based wait on an observable signal from the reconciler, such as a
status condition, annotation, or other field updated when the latest
spec.generation has been processed. Use the existing client.Update, client.Get,
and deployment.Generation flow to locate the test, and keep the later
Consistently assertion only after the reconciler-ready signal is observed.
---
Nitpick comments:
In `@internal/controller/watchers/watchers_test.go`:
- Around line 363-471: The new Credential hot-reload tests duplicate the same
Deployment and reconciler setup in each case. Extract that repeated boilerplate
into a small local helper in watchers_test.go, such as a helper that builds the
standard Deployment and calls createTestReconciler, then reuse it in the three
Describe/It blocks while keeping the test-specific watcher config and Secret
setup inline.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b0d1bbf1-2bfc-49b1-a705-c12229c81079
⛔ Files ignored due to path filters (1)
config/crd/bases/ols.openshift.io_olsconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (8)
api/v1alpha1/olsconfig_types.godocs/credential-hot-reload.mdinternal/controller/olsconfig_controller.gointernal/controller/olsconfig_helpers.gointernal/controller/utils/types.gointernal/controller/watchers/watchers.gointernal/controller/watchers/watchers_test.gotest/e2e/reconciliation_test.go
|
Hi @blublinsky — this is the operator companion to service PR openshift/lightspeed-service#2955 (RFE-9380). Adds an opt-in All CI checks pass. The Would appreciate a review when you have a chance. |
|
Suggestion: drive hot-reload from The flag is currently wired in two places: A simpler approach is to handle this inside the When the flag is off, behavior stays as today: foreach annotates LLM credential secrets and rotations trigger a rolling restart as they do now. That keeps “which secrets does the operator react to?” entirely in the existing foreach/mapping configuration model, instead of splitting it between reconcile-time config and filter-time exceptions. One thing to nail when toggling the flag on for clusters that already have annotated LLM secrets: reconcile may need to remove the watcher annotation when hot-reload is enabled, not only skip adding it for new ones. I’d hold off on deeper review until this is reworked — the foreach-based approach removes most of the PR’s watcher-layer changes and is the pattern we already use for other secret types. |
|
Follow-up: Today this flag lives only on the CR and in operator That leaves two independent behaviors with no runtime coupling:
For this to work end-to-end, the flag should flow: Without that, enabling the CR flag on a cluster with an older service image (or before #2955 is deployed) silently disables restarts while credentials stay stale at startup — the CR comment warns about this, but nothing in config enforces it. Suggest:
That gives one user-facing switch that coordinates both “don’t restart” and “do re-read from disk,” instead of two repos assuming the other half is always correct. |
|
Scoping: operator-only PR + bundle follow-up This PR is correctly scoped to the operator only — service changes stay in openshift/lightspeed-service#2955. Keep it that way. Once the operator changes here are in good shape, suggest landing this PR without blocking on the OLM bundle, then a follow-up PR that runs Order I’d expect:
That keeps review focused and matches how we usually ship CRD schema vs bundle image updates. |
openshift-ci
Bot
added
the
needs-rebase
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/controller/olsconfig_helpers.go`:
- Around line 389-416: The credential hot-reload branch in the
ForEachExternalSecret callback currently logs failures from
removeSecretAnnotationIfNeeded without recording them. Append that error to errs
before returning, matching the existing annotateSecretIfNeeded error handling so
reconciliation reports the failure and retries.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6896947a-047f-45c2-a404-05c4b75468c8
📒 Files selected for processing (6)
internal/controller/appserver/assets.gointernal/controller/olsconfig_helpers.gointernal/controller/utils/types.gointernal/controller/watchers/watchers.gointernal/controller/watchers/watchers_test.gotest/e2e/reconciliation_test.go
💤 Files with no reviewable changes (2)
- internal/controller/watchers/watchers.go
- internal/controller/watchers/watchers_test.go
…LLM secret rotation Adds an opt-in credentialHotReload boolean to OLSSpec CRD. When enabled, the operator's secret watcher skips rolling restart for LLM credential secrets, allowing the service's in-process hot-reload to handle rotated tokens without downtime (RFE-9380). Default: false (preserves existing behavior). - New CRD field: spec.olsConfig.credentialHotReload (default: false) - Watcher: skip restart for LLM secrets when flag is true - Info log emitted during reconciliation when enabled - Unit tests (3 new) + e2e test for hot-reload skip - Documentation: docs/credential-hot-reload.md Companion to lightspeed-service PR #2955.
- Drive hot-reload from ForEachExternalSecret callback: skip annotation for LLM secrets when credentialHotReload is enabled, remove existing annotations when toggling on - Remove CredentialHotReload/LLMSecretNames from WatcherConfig and the skip block from SecretWatcherFilter - Propagate credentialHotReload into olsconfig ConfigMap via utils.OLSConfig and buildOLSConfig() - Add removeSecretAnnotationIfNeeded helper - Rework e2e test to verify annotation removal and ConfigMap content - Remove obsolete watcher hot-reload unit tests
lalan7
force-pushed
the
feat/credential-hot-reload
branch
from
1eaa6a2 to
1cda906
Compare
openshift-ci
Bot
removed
the
needs-rebase
… add unit tests - Append removeSecretAnnotationIfNeeded errors to errs slice so annotateExternalResources reports failures consistently. - Replace hardcoded "ols.openshift.io/watcher" in e2e test with utils.WatcherAnnotationKey constant. - Add unit tests for removeSecretAnnotationIfNeeded covering annotation present, absent, and secret-not-found cases.
|
Addressed all code review findings in the latest push: Service (lightspeed-service): Consolidated AWS credential reads — Bedrock.load() now calls get_aws_credentials() once and passes the tuple through to _build_boto3_session() / _build_sigv4_auth() via an optional parameter. This cuts disk reads per load() from 6 to 3 and eliminates a potential mid-request partial rotation race (e.g. new access_key + old secret_key if rotation lands between calls). Guarded optional role_arn read — get_aws_credentials() now checks os.path.isfile() before attempting to read the role_arn file, avoiding spurious warning logs when it's not configured (common scenario). Operator (lightspeed-operator): Collected removeSecretAnnotationIfNeeded errors — failures are now appended to the errs slice, consistent with annotateSecretIfNeeded error handling. Previously these were logged but silently swallowed. Added unit tests for removeSecretAnnotationIfNeeded — covers annotation present (removed), annotation absent (no-op), and secret not found (no-op). All service tests pass (28/28). Operator compiles and passes go vet cleanly (envtest unit tests require kubebuilder binaries available only in CI). |
|
/retest |
The controller-gen output in config/crd/bases/ had the field but the bundle manifest was stale, causing the e2e test to fail with "unknown field spec.ols.credentialHotReload".
…ation Annotation failures in annotateExternalResources caused the reconciler to return early, preventing subsequent steps (including ConfigMap generation) from running. This starved the olsconfig ConfigMap of the credential_hot_reload flag, causing the e2e test to time out. Validation errors (LLM credentials, TLS secrets) remain blocking. Annotation operations are idempotent and will succeed on the next reconciliation cycle.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/controller/olsconfig_helpers.go`:
- Around line 408-413: Protect shared WatcherConfig map access with one
sync.RWMutex: guard the AnnotatedSecretMapping write in
internal/controller/olsconfig_helpers.go lines 408-413 and the
AnnotatedConfigMapMapping write in lines 440-442, and use the same mutex in
SecretWatcherFilter and ConfigMapWatcherFilter for corresponding reads.
- Around line 454-458: Update the error-return path in the helper containing the
errs check so non-empty errs is aggregated and returned instead of returning
nil, ensuring reconciliation retries. Use r.GetLogger().Info for the retry
message, and wrap the aggregated error with fmt.Errorf using the applicable
shared error constant from internal/controller/utils/errors.go. Preserve the
existing successful nil return when errs is empty.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7d5c034e-00f0-4ca2-b7e6-6f8776b361d0
⛔ Files ignored due to path filters (2)
bundle/manifests/ols.openshift.io_olsconfigs.yamlis excluded by!bundle/manifests/ols.openshift.io_olsconfigs.yamlconfig/crd/bases/ols.openshift.io_olsconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (10)
api/v1alpha1/olsconfig_types.godocs/credential-hot-reload.mdinternal/controller/appserver/assets.gointernal/controller/olsconfig_controller.gointernal/controller/olsconfig_helpers.gointernal/controller/olsconfig_helpers_test.gointernal/controller/utils/types.gointernal/controller/watchers/watchers.gointernal/controller/watchers/watchers_test.gotest/e2e/reconciliation_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/controller/watchers/watchers.go
- internal/controller/watchers/watchers_test.go
- api/v1alpha1/olsconfig_types.go
- test/e2e/reconciliation_test.go
- internal/controller/appserver/assets.go
- internal/controller/olsconfig_controller.go
- docs/credential-hot-reload.md
- internal/controller/utils/types.go
| if r.WatcherConfig != nil && source == "tls" { | ||
| r.WatcherConfig.AnnotatedSecretMapping[name] = []string{ | ||
| utils.ConsoleUIDeploymentName, | ||
| "ACTIVE_BACKEND", | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Concurrent map read/write data race on r.WatcherConfig.
Both map writes in the reconciler loop cause a fatal data race with concurrent reads from the informer event handlers (SecretWatcherFilter and ConfigMapWatcherFilter). Maps in Go are not thread-safe, and this overlap will crash the operator with a panic. The shared root cause is a lack of synchronization when accessing WatcherConfig state across distinct execution threads.
internal/controller/olsconfig_helpers.go#L408-L413: guard writes tor.WatcherConfig.AnnotatedSecretMappingwith async.RWMutex.internal/controller/olsconfig_helpers.go#L440-L442: guard writes tor.WatcherConfig.AnnotatedConfigMapMappingwith the same mutex.
📍 Affects 1 file
internal/controller/olsconfig_helpers.go#L408-L413(this comment)internal/controller/olsconfig_helpers.go#L440-L442
🤖 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/olsconfig_helpers.go` around lines 408 - 413, Protect
shared WatcherConfig map access with one sync.RWMutex: guard the
AnnotatedSecretMapping write in internal/controller/olsconfig_helpers.go lines
408-413 and the AnnotatedConfigMapMapping write in lines 440-442, and use the
same mutex in SecretWatcherFilter and ConfigMapWatcherFilter for corresponding
reads.
| if len(errs) > 0 { | ||
| return fmt.Errorf("failed to annotate %d external resources", len(errs)) | ||
| r.Logger.Info("some external resource annotations failed, will retry on next reconciliation", | ||
| "failureCount", len(errs)) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Return the aggregated error to ensure the reconciliation is retried.
Returning nil when errs is non-empty swallows the errors and causes the controller to consider the reconciliation successful. The controller will not requeue the request, meaning the retry mentioned in the log will not happen unless another external event triggers a reconciliation.
Additionally, as per coding guidelines and path instructions:
- Use
r.GetLogger().Infoinstead ofr.Logger.Info. - Wrap the returned error using
fmt.Errorfwith the appropriate shared constant frominternal/controller/utils/errors.go(if applicable).
🐛 Proposed fix
if len(errs) > 0 {
- r.Logger.Info("some external resource annotations failed, will retry on next reconciliation",
+ r.GetLogger().Info("some external resource annotations failed, will retry on next reconciliation",
"failureCount", len(errs))
+ return fmt.Errorf("failed to process annotations: %w", errors.Join(errs...))
}
return nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(errs) > 0 { | |
| return fmt.Errorf("failed to annotate %d external resources", len(errs)) | |
| r.Logger.Info("some external resource annotations failed, will retry on next reconciliation", | |
| "failureCount", len(errs)) | |
| } | |
| return nil | |
| if len(errs) > 0 { | |
| r.GetLogger().Info("some external resource annotations failed, will retry on next reconciliation", | |
| "failureCount", len(errs)) | |
| return fmt.Errorf("failed to process annotations: %w", errors.Join(errs...)) | |
| } | |
| return nil |
🤖 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/olsconfig_helpers.go` around lines 454 - 458, Update the
error-return path in the helper containing the errs check so non-empty errs is
aggregated and returned instead of returning nil, ensuring reconciliation
retries. Use r.GetLogger().Info for the retry message, and wrap the aggregated
error with fmt.Errorf using the applicable shared error constant from
internal/controller/utils/errors.go. Preserve the existing successful nil return
when errs is empty.
Sources: Coding guidelines, Path instructions
The CA-cert test (test 2) sets AdditionalCAConfigMapRef on the CR and deletes the ConfigMap in a defer, but never clears the CR reference. When the hot-reload test (test 3) runs, GenerateOLSConfigMap fails with NotFound for the deleted ConfigMap, permanently blocking the credential_hot_reload flag from being propagated to olsconfig.yaml. Also reverts the non-blocking annotation error change from ee29f5d — that was not the actual root cause.
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/olsconfig_helpers.go (1)
419-426: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the required reconciler logger.
Replace
r.Logger.Errorwithr.GetLogger().Errorin both error paths to follow the repository’s mandated logging pattern.🤖 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/olsconfig_helpers.go` around lines 419 - 426, Replace r.Logger.Error with r.GetLogger().Error in both error paths within the secret annotation/removal flow, including the calls for “Failed to remove annotation from secret” and “Failed to annotate secret,” while preserving their existing arguments.Sources: Coding guidelines, 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/olsconfig_helpers.go`:
- Around line 419-426: Replace r.Logger.Error with r.GetLogger().Error in both
error paths within the secret annotation/removal flow, including the calls for
“Failed to remove annotation from secret” and “Failed to annotate secret,” while
preserving their existing arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7a81df0a-1ec3-449f-91ee-c57671646807
📒 Files selected for processing (2)
internal/controller/olsconfig_helpers.gotest/e2e/reconciliation_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/reconciliation_test.go
|
@lalan7: all tests passed! 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. |
3 participants
Summary
credentialHotReloadboolean to OLSSpec CRD (default:false)Test plan
Consistentlygo vet ./...cleango build ./...cleanmake manifestsRelated
Summary by CodeRabbit
spec.ols.credentialHotReloadto enable in-process hot-reload of LLM provider credential secrets without app-server restarts.lightspeed-servicebehavior, and clarified expected handling for credential secret data vs reference changes.