From e575a336ff6759c9e21d3049c5d23ab95220faae Mon Sep 17 00:00:00 2001 From: Erik Lalancette Date: Thu, 2 Jul 2026 16:32:43 -0400 Subject: [PATCH 1/6] OLS-3450: Add credentialHotReload flag to skip app-server restart on 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. --- api/v1alpha1/olsconfig_types.go | 10 ++ .../bases/ols.openshift.io_olsconfigs.yaml | 10 ++ docs/credential-hot-reload.md | 52 +++++++++ internal/controller/olsconfig_controller.go | 6 + internal/controller/olsconfig_helpers.go | 10 ++ internal/controller/utils/types.go | 9 +- internal/controller/watchers/watchers.go | 13 ++- internal/controller/watchers/watchers_test.go | 110 ++++++++++++++++++ test/e2e/reconciliation_test.go | 55 +++++++++ 9 files changed, 273 insertions(+), 2 deletions(-) create mode 100644 docs/credential-hot-reload.md diff --git a/api/v1alpha1/olsconfig_types.go b/api/v1alpha1/olsconfig_types.go index 7abcd1cbc..8a6fc97ba 100644 --- a/api/v1alpha1/olsconfig_types.go +++ b/api/v1alpha1/olsconfig_types.go @@ -319,6 +319,16 @@ type OLSSpec struct { // +kubebuilder:validation:Optional // +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Tools Approval Configuration",xDescriptors={"urn:alm:descriptor:com.tectonic.ui:advanced"} ToolsApprovalConfig *ToolsApprovalConfig `json:"toolsApprovalConfig,omitempty"` + // Enable in-process credential hot-reload for LLM provider secrets. + // When true, the operator will not restart the app-server when LLM credential + // secret data is rotated — the service re-reads credentials from disk on each request. + // IMPORTANT: Requires lightspeed-service with get_credentials() hot-reload support + // (service PR #2955 / RFE-9380). If enabled with an older service image, rotated + // credentials (including revoked keys) will remain stale until the pod is manually restarted. + // +kubebuilder:default=false + // +kubebuilder:validation:Optional + // +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Credential Hot Reload",xDescriptors={"urn:alm:descriptor:com.tectonic.ui:booleanSwitch"} + CredentialHotReload *bool `json:"credentialHotReload,omitempty"` } // Persistent Storage Configuration diff --git a/config/crd/bases/ols.openshift.io_olsconfigs.yaml b/config/crd/bases/ols.openshift.io_olsconfigs.yaml index 27bbe04a2..5b088403a 100644 --- a/config/crd/bases/ols.openshift.io_olsconfigs.yaml +++ b/config/crd/bases/ols.openshift.io_olsconfigs.yaml @@ -642,6 +642,16 @@ spec: - postgres type: string type: object + credentialHotReload: + default: false + description: |- + Enable in-process credential hot-reload for LLM provider secrets. + When true, the operator will not restart the app-server when LLM credential + secret data is rotated — the service re-reads credentials from disk on each request. + IMPORTANT: Requires lightspeed-service with get_credentials() hot-reload support + (service PR #2955 / RFE-9380). If enabled with an older service image, rotated + credentials (including revoked keys) will remain stale until the pod is manually restarted. + type: boolean defaultModel: description: Default model for usage type: string diff --git a/docs/credential-hot-reload.md b/docs/credential-hot-reload.md new file mode 100644 index 000000000..c914403a2 --- /dev/null +++ b/docs/credential-hot-reload.md @@ -0,0 +1,52 @@ +# Credential Hot-Reload (RFE-9380) + +## Overview + +When `credentialHotReload` is enabled on the OLSConfig CR, the operator skips +rolling restarts of the app-server pod when LLM credential secret **data** +changes. The service re-reads credential files from disk on every LLM request, +so rotated tokens take effect without downtime. + +This feature is a companion to +[lightspeed-service PR #2955](https://github.com/openshift/lightspeed-service/pull/2955), +which adds the in-process credential reload on the service side. + +## Configuration + +```yaml +apiVersion: ols.openshift.io/v1alpha1 +kind: OLSConfig +metadata: + name: cluster +spec: + ols: + credentialHotReload: true # default: false +``` + +## Behavior + +| Event | `credentialHotReload: false` (default) | `credentialHotReload: true` | +|---|---|---| +| LLM secret **data** rotated (same secret name) | Rolling restart | **No restart** — service picks up new credentials on next request | +| LLM secret **ref** changed in CR (different secret name) | Rolling restart | Rolling restart (deployment volume spec changes) | +| TLS / MCP / Postgres secret changed | Rolling restart | Rolling restart (unchanged) | + +## Prerequisites + +- The lightspeed-service image must include the `get_credentials()` hot-reload + support (lightspeed-service >= the version containing PR #2955). If an older + service image is used with this flag enabled, rotated credentials will not be + picked up until the pod is manually restarted. + +## How It Works + +1. During reconciliation, the operator reads `spec.ols.credentialHotReload` from + the OLSConfig CR and stores it in the internal `WatcherConfig`, along with the + set of LLM provider secret names. + +2. When the secret watcher detects a `.data` change on an annotated secret, it + checks whether the secret is an LLM credential and the hot-reload flag is + enabled. If both conditions are true, the restart is skipped. + +3. Non-LLM secrets (TLS, MCP headers, Postgres) always trigger restarts + regardless of the flag. diff --git a/internal/controller/olsconfig_controller.go b/internal/controller/olsconfig_controller.go index 066077356..a983c67ac 100644 --- a/internal/controller/olsconfig_controller.go +++ b/internal/controller/olsconfig_controller.go @@ -746,6 +746,12 @@ func (r *OLSConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, fmt.Errorf("failed to annotate external resources: %w", err) } + if olsconfig.Spec.OLSConfig.CredentialHotReload != nil && *olsconfig.Spec.OLSConfig.CredentialHotReload { + r.Logger.Info("credentialHotReload is enabled — LLM credential secret rotations will not "+ + "trigger app-server restarts. Requires lightspeed-service with get_credentials() "+ + "hot-reload support (service PR #2955 / RFE-9380)") + } + // 5. Phase 1: Reconcile independent resources if err := r.reconcileIndependentResources(ctx, olsconfig); err != nil { if isRESTMappingError(err) { diff --git a/internal/controller/olsconfig_helpers.go b/internal/controller/olsconfig_helpers.go index 2a3359be6..6357b28fa 100644 --- a/internal/controller/olsconfig_helpers.go +++ b/internal/controller/olsconfig_helpers.go @@ -404,6 +404,16 @@ func (r *OLSConfigReconciler) annotateExternalResources(ctx context.Context, if r.WatcherConfig != nil { r.WatcherConfig.AnnotatedConfigMapMapping = make(map[string][]string) r.WatcherConfig.AnnotatedSecretMapping = make(map[string][]string) + r.WatcherConfig.LLMSecretNames = make(map[string]bool) + + r.WatcherConfig.CredentialHotReload = cr.Spec.OLSConfig.CredentialHotReload != nil && + *cr.Spec.OLSConfig.CredentialHotReload + + for _, provider := range cr.Spec.LLMConfig.Providers { + if provider.CredentialsSecretRef.Name != "" { + r.WatcherConfig.LLMSecretNames[provider.CredentialsSecretRef.Name] = true + } + } } var errs []error diff --git a/internal/controller/utils/types.go b/internal/controller/utils/types.go index 52ed1a62f..9bd161bff 100644 --- a/internal/controller/utils/types.go +++ b/internal/controller/utils/types.go @@ -69,7 +69,12 @@ type ConfigMapWatcherConfig struct { SystemResources []SystemConfigMap } -// WatcherConfig contains all watcher configuration +// WatcherConfig contains all watcher configuration. +// NOTE: This struct is written by the reconciler and read by watcher event handlers +// (including predicate filters such as SecretWatcherFilter). This is safe because +// controller-runtime serializes reconcile calls and predicate evaluations on the same +// controller work queue. If MaxConcurrentReconciles is ever increased above 1, +// a sync.RWMutex must be added here. type WatcherConfig struct { Secrets SecretWatcherConfig ConfigMaps ConfigMapWatcherConfig @@ -82,6 +87,8 @@ type WatcherConfig struct { // RHOKPTLSWatchEnabled gates informer handling of lightspeed-rhokp-tls. // Same pattern: static entry, toggled from !byokRAGOnly. RHOKPTLSWatchEnabled atomic.Bool + CredentialHotReload bool + LLMSecretNames map[string]bool } // IsSystemSecretWatchEnabled reports whether a SystemResources entry should be active. diff --git a/internal/controller/watchers/watchers.go b/internal/controller/watchers/watchers.go index 9881ed265..77d0ab1a4 100644 --- a/internal/controller/watchers/watchers.go +++ b/internal/controller/watchers/watchers.go @@ -274,8 +274,19 @@ func SecretWatcherFilter(r reconciler.Reconciler, ctx context.Context, obj clien // Check 2: Look for watcher annotation (user-provided secrets) if _, exist := annotations[utils.WatcherAnnotationKey]; exist { - // For annotated secrets, determine affected deployments from mapping secretName := obj.GetName() + + // Skip restart for LLM credential secrets when hot-reload is enabled. + // The service re-reads credentials from disk on every request (RFE-9380). + if watcherConfig != nil && watcherConfig.CredentialHotReload { + if watcherConfig.LLMSecretNames[secretName] { + r.GetLogger().Info("Skipping restart for LLM credential secret (hot-reload enabled)", + "secret", secretName) + return + } + } + + // For annotated secrets, determine affected deployments from mapping var affectedDeployments []string var found bool if watcherConfig != nil { diff --git a/internal/controller/watchers/watchers_test.go b/internal/controller/watchers/watchers_test.go index 6e291f833..7bbeb102a 100644 --- a/internal/controller/watchers/watchers_test.go +++ b/internal/controller/watchers/watchers_test.go @@ -444,4 +444,114 @@ var _ = Describe("Watchers", func() { Expect(updated.Spec.Template.Annotations).To(HaveKey(utils.ForceReloadAnnotationKey)) }) }) + + Describe("Credential hot-reload", func() { + It("skips restart for LLM secret when CredentialHotReload is enabled", func() { + cr := utils.GetDefaultOLSConfigCR() + dep := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: utils.OLSAppServerDeploymentName, + Namespace: utils.OLSNamespaceDefault, + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "ols"}}, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "ols"}}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "c", Image: "img"}}}, + }, + }, + } + r := createTestReconciler(cr, dep) + + wc, _ := r.GetWatcherConfig().(*utils.WatcherConfig) + wc.CredentialHotReload = true + wc.LLMSecretNames = map[string]bool{"test-secret": true} + + sec := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: utils.OLSNamespaceDefault, + Name: "test-secret", + Annotations: map[string]string{utils.WatcherAnnotationKey: "1"}, + }, + Data: map[string][]byte{"apitoken": []byte("rotated-key")}, + } + SecretWatcherFilter(r, ctx, sec, true) + + updated := &appsv1.Deployment{} + Expect(r.Get(ctx, client.ObjectKeyFromObject(dep), updated)).To(Succeed()) + Expect(updated.Spec.Template.Annotations).NotTo(HaveKey(utils.ForceReloadAnnotationKey)) + }) + + It("restarts for LLM secret when CredentialHotReload is disabled", func() { + cr := utils.GetDefaultOLSConfigCR() + dep := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: utils.OLSAppServerDeploymentName, + Namespace: utils.OLSNamespaceDefault, + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "ols"}}, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "ols"}}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "c", Image: "img"}}}, + }, + }, + } + r := createTestReconciler(cr, dep) + + wc, _ := r.GetWatcherConfig().(*utils.WatcherConfig) + wc.CredentialHotReload = false + wc.LLMSecretNames = map[string]bool{"test-secret": true} + + sec := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: utils.OLSNamespaceDefault, + Name: "test-secret", + Annotations: map[string]string{utils.WatcherAnnotationKey: "1"}, + }, + Data: map[string][]byte{"apitoken": []byte("rotated-key")}, + } + SecretWatcherFilter(r, ctx, sec, true) + + updated := &appsv1.Deployment{} + Expect(r.Get(ctx, client.ObjectKeyFromObject(dep), updated)).To(Succeed()) + Expect(updated.Spec.Template.Annotations).To(HaveKey(utils.ForceReloadAnnotationKey)) + }) + + It("restarts for non-LLM secret even when CredentialHotReload is enabled", func() { + cr := utils.GetDefaultOLSConfigCR() + dep := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: utils.OLSAppServerDeploymentName, + Namespace: utils.OLSNamespaceDefault, + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "ols"}}, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "ols"}}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "c", Image: "img"}}}, + }, + }, + } + r := createTestReconciler(cr, dep) + + wc, _ := r.GetWatcherConfig().(*utils.WatcherConfig) + wc.CredentialHotReload = true + wc.LLMSecretNames = map[string]bool{"test-secret": true} + + sec := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: utils.OLSNamespaceDefault, + Name: "tls-secret", + Annotations: map[string]string{utils.WatcherAnnotationKey: "1"}, + }, + Data: map[string][]byte{"tls.crt": []byte("cert-data")}, + } + SecretWatcherFilter(r, ctx, sec, true) + + updated := &appsv1.Deployment{} + Expect(r.Get(ctx, client.ObjectKeyFromObject(dep), updated)).To(Succeed()) + Expect(updated.Spec.Template.Annotations).To(HaveKey(utils.ForceReloadAnnotationKey)) + }) + }) }) diff --git a/test/e2e/reconciliation_test.go b/test/e2e/reconciliation_test.go index 7c91d3da1..cec687851 100644 --- a/test/e2e/reconciliation_test.go +++ b/test/e2e/reconciliation_test.go @@ -389,4 +389,59 @@ var _ = Describe("Reconciliation From OLSConfig CR", Ordered, func() { }) + It("should skip app-server restart on LLM secret data rotation when credentialHotReload is enabled", func() { + deployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: AppServerDeploymentName, + Namespace: OLSNameSpace, + }, + } + + By("enable credentialHotReload on the CR") + err = client.Update(cr, func(obj ctrlclient.Object) error { + config := obj.(*olsv1alpha1.OLSConfig) + hotReload := true + config.Spec.OLSConfig.CredentialHotReload = &hotReload + return nil + }) + Expect(err).NotTo(HaveOccurred()) + + // credentialHotReload only affects in-memory WatcherConfig, not the + // Deployment spec, so there is no Generation bump to wait for. + // Give the reconciler time to process the CR update. + time.Sleep(5 * time.Second) + + err = client.Get(deployment) + Expect(err).NotTo(HaveOccurred()) + generation := deployment.Generation + + By("rotate the LLM secret data (using second secret — CR was switched to it by an earlier test)") + llmSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: LLMTokenSecondSecretName, + Namespace: OLSNameSpace, + }, + } + err = client.Update(llmSecret, func(obj ctrlclient.Object) error { + s := obj.(*corev1.Secret) + s.Data[LLMApiTokenFileName] = []byte("rotated-key-value") + return nil + }) + Expect(err).NotTo(HaveOccurred()) + + By("verify the deployment generation did NOT change (no restart)") + Consistently(func() int64 { + Expect(client.Get(deployment)).To(Succeed()) + return deployment.Generation + }, 15*time.Second, 2*time.Second).Should(Equal(generation)) + + By("disable credentialHotReload to restore default behavior") + err = client.Update(cr, func(obj ctrlclient.Object) error { + config := obj.(*olsv1alpha1.OLSConfig) + config.Spec.OLSConfig.CredentialHotReload = nil + return nil + }) + Expect(err).NotTo(HaveOccurred()) + }) + }) From 5fe961838e8973b4c9c8189181303e1e5b90639d Mon Sep 17 00:00:00 2001 From: Erik Lalancette Date: Tue, 14 Jul 2026 10:16:14 -0400 Subject: [PATCH 2/6] OLS-3450: Rework hot-reload to annotation-based approach per review - 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 --- internal/controller/appserver/assets.go | 4 + internal/controller/olsconfig_helpers.go | 46 ++++++-- internal/controller/utils/types.go | 4 +- internal/controller/watchers/watchers.go | 10 -- internal/controller/watchers/watchers_test.go | 109 ------------------ test/e2e/reconciliation_test.go | 58 ++++++---- 6 files changed, 74 insertions(+), 157 deletions(-) diff --git a/internal/controller/appserver/assets.go b/internal/controller/appserver/assets.go index c3480a3d1..6f53df3de 100644 --- a/internal/controller/appserver/assets.go +++ b/internal/controller/appserver/assets.go @@ -321,6 +321,10 @@ func buildOLSConfig(r reconciler.Reconciler, ctx context.Context, cr *olsv1alpha } } + if cr.Spec.OLSConfig.CredentialHotReload != nil && *cr.Spec.OLSConfig.CredentialHotReload { + olsConfig.CredentialHotReload = true + } + tlsProfile := cr.Spec.OLSConfig.TLSSecurityProfile if tlsProfile == nil { apiServerProfile, err := utiltls.FetchAPIServerTlsProfile(r) diff --git a/internal/controller/olsconfig_helpers.go b/internal/controller/olsconfig_helpers.go index 6357b28fa..4a1144400 100644 --- a/internal/controller/olsconfig_helpers.go +++ b/internal/controller/olsconfig_helpers.go @@ -404,18 +404,11 @@ func (r *OLSConfigReconciler) annotateExternalResources(ctx context.Context, if r.WatcherConfig != nil { r.WatcherConfig.AnnotatedConfigMapMapping = make(map[string][]string) r.WatcherConfig.AnnotatedSecretMapping = make(map[string][]string) - r.WatcherConfig.LLMSecretNames = make(map[string]bool) - - r.WatcherConfig.CredentialHotReload = cr.Spec.OLSConfig.CredentialHotReload != nil && - *cr.Spec.OLSConfig.CredentialHotReload - - for _, provider := range cr.Spec.LLMConfig.Providers { - if provider.CredentialsSecretRef.Name != "" { - r.WatcherConfig.LLMSecretNames[provider.CredentialsSecretRef.Name] = true - } - } } + credentialHotReload := cr.Spec.OLSConfig.CredentialHotReload != nil && + *cr.Spec.OLSConfig.CredentialHotReload + var errs []error // Annotate all external secrets @@ -428,11 +421,20 @@ func (r *OLSConfigReconciler) annotateExternalResources(ctx context.Context, } } + // When credentialHotReload is enabled, LLM secrets are not watched — + // the service re-reads credentials from disk (RFE-9380). + if credentialHotReload && strings.HasPrefix(source, "llm-provider-") { + if err := r.removeSecretAnnotationIfNeeded(ctx, name, r.Options.Namespace); err != nil { + r.Logger.Error(err, "Failed to remove annotation from secret", "secret", name) + } + return nil + } + if err := r.annotateSecretIfNeeded(ctx, name, r.Options.Namespace); err != nil { r.Logger.Error(err, "Failed to annotate secret", "source", source, "secret", name) errs = append(errs, err) } - return nil // Continue iteration even on error + return nil }) if err != nil { errs = append(errs, err) @@ -512,6 +514,28 @@ func (r *OLSConfigReconciler) annotateSecretIfNeeded(ctx context.Context, name, return r.Update(ctx, secret) } +// removeSecretAnnotationIfNeeded removes the watcher annotation from a secret if present. +func (r *OLSConfigReconciler) removeSecretAnnotationIfNeeded(ctx context.Context, name, namespace string) error { + secret := &corev1.Secret{} + err := r.Get(ctx, client.ObjectKey{Name: name, Namespace: namespace}, secret) + if err != nil { + if apierrors.IsNotFound(err) { + return nil + } + return err + } + + if secret.Annotations == nil { + return nil + } + if _, exists := secret.Annotations[utils.WatcherAnnotationKey]; !exists { + return nil + } + + delete(secret.Annotations, utils.WatcherAnnotationKey) + return r.Update(ctx, secret) +} + // annotateConfigMapIfNeeded annotates a configmap with the watcher annotation if it doesn't already have it. // Returns nil if the configmap doesn't exist (will be picked up on next reconciliation). func (r *OLSConfigReconciler) annotateConfigMapIfNeeded(ctx context.Context, name, namespace string) error { diff --git a/internal/controller/utils/types.go b/internal/controller/utils/types.go index 9bd161bff..43e3a8db0 100644 --- a/internal/controller/utils/types.go +++ b/internal/controller/utils/types.go @@ -87,8 +87,6 @@ type WatcherConfig struct { // RHOKPTLSWatchEnabled gates informer handling of lightspeed-rhokp-tls. // Same pattern: static entry, toggled from !byokRAGOnly. RHOKPTLSWatchEnabled atomic.Bool - CredentialHotReload bool - LLMSecretNames map[string]bool } // IsSystemSecretWatchEnabled reports whether a SystemResources entry should be active. @@ -246,6 +244,8 @@ type OLSConfig struct { Audit *AuditYAMLConfig `json:"audit,omitempty"` // Solr hybrid RAG (portal-rag /hybrid-search); mirrors lightspeed-service solr_hybrid SolrHybrid *SolrHybridSettings `json:"solr_hybrid,omitempty"` + // Enable in-process credential hot-reload for LLM provider secrets + CredentialHotReload bool `json:"credential_hot_reload,omitempty"` } type AuditYAMLConfig struct { diff --git a/internal/controller/watchers/watchers.go b/internal/controller/watchers/watchers.go index 77d0ab1a4..375acc26e 100644 --- a/internal/controller/watchers/watchers.go +++ b/internal/controller/watchers/watchers.go @@ -276,16 +276,6 @@ func SecretWatcherFilter(r reconciler.Reconciler, ctx context.Context, obj clien if _, exist := annotations[utils.WatcherAnnotationKey]; exist { secretName := obj.GetName() - // Skip restart for LLM credential secrets when hot-reload is enabled. - // The service re-reads credentials from disk on every request (RFE-9380). - if watcherConfig != nil && watcherConfig.CredentialHotReload { - if watcherConfig.LLMSecretNames[secretName] { - r.GetLogger().Info("Skipping restart for LLM credential secret (hot-reload enabled)", - "secret", secretName) - return - } - } - // For annotated secrets, determine affected deployments from mapping var affectedDeployments []string var found bool diff --git a/internal/controller/watchers/watchers_test.go b/internal/controller/watchers/watchers_test.go index 7bbeb102a..909a024dc 100644 --- a/internal/controller/watchers/watchers_test.go +++ b/internal/controller/watchers/watchers_test.go @@ -445,113 +445,4 @@ var _ = Describe("Watchers", func() { }) }) - Describe("Credential hot-reload", func() { - It("skips restart for LLM secret when CredentialHotReload is enabled", func() { - cr := utils.GetDefaultOLSConfigCR() - dep := &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: utils.OLSAppServerDeploymentName, - Namespace: utils.OLSNamespaceDefault, - }, - Spec: appsv1.DeploymentSpec{ - Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "ols"}}, - Template: corev1.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "ols"}}, - Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "c", Image: "img"}}}, - }, - }, - } - r := createTestReconciler(cr, dep) - - wc, _ := r.GetWatcherConfig().(*utils.WatcherConfig) - wc.CredentialHotReload = true - wc.LLMSecretNames = map[string]bool{"test-secret": true} - - sec := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: utils.OLSNamespaceDefault, - Name: "test-secret", - Annotations: map[string]string{utils.WatcherAnnotationKey: "1"}, - }, - Data: map[string][]byte{"apitoken": []byte("rotated-key")}, - } - SecretWatcherFilter(r, ctx, sec, true) - - updated := &appsv1.Deployment{} - Expect(r.Get(ctx, client.ObjectKeyFromObject(dep), updated)).To(Succeed()) - Expect(updated.Spec.Template.Annotations).NotTo(HaveKey(utils.ForceReloadAnnotationKey)) - }) - - It("restarts for LLM secret when CredentialHotReload is disabled", func() { - cr := utils.GetDefaultOLSConfigCR() - dep := &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: utils.OLSAppServerDeploymentName, - Namespace: utils.OLSNamespaceDefault, - }, - Spec: appsv1.DeploymentSpec{ - Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "ols"}}, - Template: corev1.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "ols"}}, - Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "c", Image: "img"}}}, - }, - }, - } - r := createTestReconciler(cr, dep) - - wc, _ := r.GetWatcherConfig().(*utils.WatcherConfig) - wc.CredentialHotReload = false - wc.LLMSecretNames = map[string]bool{"test-secret": true} - - sec := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: utils.OLSNamespaceDefault, - Name: "test-secret", - Annotations: map[string]string{utils.WatcherAnnotationKey: "1"}, - }, - Data: map[string][]byte{"apitoken": []byte("rotated-key")}, - } - SecretWatcherFilter(r, ctx, sec, true) - - updated := &appsv1.Deployment{} - Expect(r.Get(ctx, client.ObjectKeyFromObject(dep), updated)).To(Succeed()) - Expect(updated.Spec.Template.Annotations).To(HaveKey(utils.ForceReloadAnnotationKey)) - }) - - It("restarts for non-LLM secret even when CredentialHotReload is enabled", func() { - cr := utils.GetDefaultOLSConfigCR() - dep := &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: utils.OLSAppServerDeploymentName, - Namespace: utils.OLSNamespaceDefault, - }, - Spec: appsv1.DeploymentSpec{ - Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "ols"}}, - Template: corev1.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "ols"}}, - Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "c", Image: "img"}}}, - }, - }, - } - r := createTestReconciler(cr, dep) - - wc, _ := r.GetWatcherConfig().(*utils.WatcherConfig) - wc.CredentialHotReload = true - wc.LLMSecretNames = map[string]bool{"test-secret": true} - - sec := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: utils.OLSNamespaceDefault, - Name: "tls-secret", - Annotations: map[string]string{utils.WatcherAnnotationKey: "1"}, - }, - Data: map[string][]byte{"tls.crt": []byte("cert-data")}, - } - SecretWatcherFilter(r, ctx, sec, true) - - updated := &appsv1.Deployment{} - Expect(r.Get(ctx, client.ObjectKeyFromObject(dep), updated)).To(Succeed()) - Expect(updated.Spec.Template.Annotations).To(HaveKey(utils.ForceReloadAnnotationKey)) - }) - }) }) diff --git a/test/e2e/reconciliation_test.go b/test/e2e/reconciliation_test.go index cec687851..466fbe123 100644 --- a/test/e2e/reconciliation_test.go +++ b/test/e2e/reconciliation_test.go @@ -389,14 +389,23 @@ var _ = Describe("Reconciliation From OLSConfig CR", Ordered, func() { }) - It("should skip app-server restart on LLM secret data rotation when credentialHotReload is enabled", func() { - deployment := &appsv1.Deployment{ + It("should remove LLM secret annotation and propagate config when credentialHotReload is enabled", func() { + llmSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: AppServerDeploymentName, + Name: LLMTokenSecondSecretName, Namespace: OLSNameSpace, }, } + By("verify the LLM secret is annotated before enabling hot-reload") + Eventually(func() bool { + if err := client.Get(llmSecret); err != nil { + return false + } + _, exists := llmSecret.Annotations["ols.openshift.io/watcher"] + return exists + }, 30*time.Second, 2*time.Second).Should(BeTrue()) + By("enable credentialHotReload on the CR") err = client.Update(cr, func(obj ctrlclient.Object) error { config := obj.(*olsv1alpha1.OLSConfig) @@ -406,35 +415,25 @@ var _ = Describe("Reconciliation From OLSConfig CR", Ordered, func() { }) Expect(err).NotTo(HaveOccurred()) - // credentialHotReload only affects in-memory WatcherConfig, not the - // Deployment spec, so there is no Generation bump to wait for. - // Give the reconciler time to process the CR update. - time.Sleep(5 * time.Second) - - err = client.Get(deployment) - Expect(err).NotTo(HaveOccurred()) - generation := deployment.Generation + By("verify the LLM secret annotation is removed") + Eventually(func() bool { + if err := client.Get(llmSecret); err != nil { + return false + } + _, exists := llmSecret.Annotations["ols.openshift.io/watcher"] + return !exists + }, 30*time.Second, 2*time.Second).Should(BeTrue()) - By("rotate the LLM secret data (using second secret — CR was switched to it by an earlier test)") - llmSecret := &corev1.Secret{ + By("verify the olsconfig ConfigMap contains credential_hot_reload: true") + configMap := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ - Name: LLMTokenSecondSecretName, + Name: AppServerConfigMapName, Namespace: OLSNameSpace, }, } - err = client.Update(llmSecret, func(obj ctrlclient.Object) error { - s := obj.(*corev1.Secret) - s.Data[LLMApiTokenFileName] = []byte("rotated-key-value") - return nil - }) + err = client.WaitForConfigMapContainString(configMap, AppServerConfigMapKey, "credential_hot_reload: true") Expect(err).NotTo(HaveOccurred()) - By("verify the deployment generation did NOT change (no restart)") - Consistently(func() int64 { - Expect(client.Get(deployment)).To(Succeed()) - return deployment.Generation - }, 15*time.Second, 2*time.Second).Should(Equal(generation)) - By("disable credentialHotReload to restore default behavior") err = client.Update(cr, func(obj ctrlclient.Object) error { config := obj.(*olsv1alpha1.OLSConfig) @@ -442,6 +441,15 @@ var _ = Describe("Reconciliation From OLSConfig CR", Ordered, func() { return nil }) Expect(err).NotTo(HaveOccurred()) + + By("verify the LLM secret annotation is restored") + Eventually(func() bool { + if err := client.Get(llmSecret); err != nil { + return false + } + _, exists := llmSecret.Annotations["ols.openshift.io/watcher"] + return exists + }, 30*time.Second, 2*time.Second).Should(BeTrue()) }) }) From 36f9d6acba6053f3eaa651e05acf87e804446bb1 Mon Sep 17 00:00:00 2001 From: Erik Lalancette Date: Tue, 14 Jul 2026 11:41:59 -0400 Subject: [PATCH 3/6] Address code review: collect annotation removal errors, use constant, 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. --- internal/controller/olsconfig_helpers.go | 1 + internal/controller/olsconfig_helpers_test.go | 44 +++++++++++++++++++ test/e2e/reconciliation_test.go | 7 +-- 3 files changed, 49 insertions(+), 3 deletions(-) diff --git a/internal/controller/olsconfig_helpers.go b/internal/controller/olsconfig_helpers.go index 4a1144400..6407b84d8 100644 --- a/internal/controller/olsconfig_helpers.go +++ b/internal/controller/olsconfig_helpers.go @@ -426,6 +426,7 @@ func (r *OLSConfigReconciler) annotateExternalResources(ctx context.Context, if credentialHotReload && strings.HasPrefix(source, "llm-provider-") { if err := r.removeSecretAnnotationIfNeeded(ctx, name, r.Options.Namespace); err != nil { r.Logger.Error(err, "Failed to remove annotation from secret", "secret", name) + errs = append(errs, err) } return nil } diff --git a/internal/controller/olsconfig_helpers_test.go b/internal/controller/olsconfig_helpers_test.go index 6dc793562..b8b24ab71 100644 --- a/internal/controller/olsconfig_helpers_test.go +++ b/internal/controller/olsconfig_helpers_test.go @@ -854,4 +854,48 @@ var _ = Describe("Helper Functions", func() { Expect(found).To(BeTrue()) }) }) + + Context("removeSecretAnnotationIfNeeded", func() { + It("should remove annotation when present", func() { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "remove-test-secret", + Namespace: testNamespace, + Annotations: map[string]string{ + utils.WatcherAnnotationKey: utils.OLSConfigName, + }, + }, + Data: map[string][]byte{"key": []byte("val")}, + } + Expect(k8sClient.Create(ctx, secret)).To(Succeed()) + defer func() { _ = k8sClient.Delete(ctx, secret) }() + + err := reconciler.removeSecretAnnotationIfNeeded(ctx, "remove-test-secret", testNamespace) + Expect(err).NotTo(HaveOccurred()) + + fetched := &corev1.Secret{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "remove-test-secret", Namespace: testNamespace}, fetched)).To(Succeed()) + Expect(fetched.Annotations).NotTo(HaveKey(utils.WatcherAnnotationKey)) + }) + + It("should do nothing when annotation is absent", func() { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "no-annot-secret", + Namespace: testNamespace, + }, + Data: map[string][]byte{"key": []byte("val")}, + } + Expect(k8sClient.Create(ctx, secret)).To(Succeed()) + defer func() { _ = k8sClient.Delete(ctx, secret) }() + + err := reconciler.removeSecretAnnotationIfNeeded(ctx, "no-annot-secret", testNamespace) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should do nothing when secret does not exist", func() { + err := reconciler.removeSecretAnnotationIfNeeded(ctx, "nonexistent-secret", testNamespace) + Expect(err).NotTo(HaveOccurred()) + }) + }) }) diff --git a/test/e2e/reconciliation_test.go b/test/e2e/reconciliation_test.go index 466fbe123..caccad9e0 100644 --- a/test/e2e/reconciliation_test.go +++ b/test/e2e/reconciliation_test.go @@ -9,6 +9,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" olsv1alpha1 "github.com/openshift/lightspeed-operator/api/v1alpha1" + "github.com/openshift/lightspeed-operator/internal/controller/utils" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apiequality "k8s.io/apimachinery/pkg/api/equality" @@ -402,7 +403,7 @@ var _ = Describe("Reconciliation From OLSConfig CR", Ordered, func() { if err := client.Get(llmSecret); err != nil { return false } - _, exists := llmSecret.Annotations["ols.openshift.io/watcher"] + _, exists := llmSecret.Annotations[utils.WatcherAnnotationKey] return exists }, 30*time.Second, 2*time.Second).Should(BeTrue()) @@ -420,7 +421,7 @@ var _ = Describe("Reconciliation From OLSConfig CR", Ordered, func() { if err := client.Get(llmSecret); err != nil { return false } - _, exists := llmSecret.Annotations["ols.openshift.io/watcher"] + _, exists := llmSecret.Annotations[utils.WatcherAnnotationKey] return !exists }, 30*time.Second, 2*time.Second).Should(BeTrue()) @@ -447,7 +448,7 @@ var _ = Describe("Reconciliation From OLSConfig CR", Ordered, func() { if err := client.Get(llmSecret); err != nil { return false } - _, exists := llmSecret.Annotations["ols.openshift.io/watcher"] + _, exists := llmSecret.Annotations[utils.WatcherAnnotationKey] return exists }, 30*time.Second, 2*time.Second).Should(BeTrue()) }) From c40bb3f5c89068c558db4b81843c6ca201d919bf Mon Sep 17 00:00:00 2001 From: Erik Lalancette Date: Tue, 14 Jul 2026 13:51:20 -0400 Subject: [PATCH 4/6] Update bundle CRD with credentialHotReload field 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". --- .../manifests/ols.openshift.io_olsconfigs.yaml | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/bundle/manifests/ols.openshift.io_olsconfigs.yaml b/bundle/manifests/ols.openshift.io_olsconfigs.yaml index a785c5980..5b088403a 100644 --- a/bundle/manifests/ols.openshift.io_olsconfigs.yaml +++ b/bundle/manifests/ols.openshift.io_olsconfigs.yaml @@ -1,9 +1,9 @@ +--- apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: controller-gen.kubebuilder.io/version: v0.19.0 - creationTimestamp: null name: olsconfigs.ols.openshift.io spec: group: ols.openshift.io @@ -642,6 +642,16 @@ spec: - postgres type: string type: object + credentialHotReload: + default: false + description: |- + Enable in-process credential hot-reload for LLM provider secrets. + When true, the operator will not restart the app-server when LLM credential + secret data is rotated — the service re-reads credentials from disk on each request. + IMPORTANT: Requires lightspeed-service with get_credentials() hot-reload support + (service PR #2955 / RFE-9380). If enabled with an older service image, rotated + credentials (including revoked keys) will remain stale until the pod is manually restarted. + type: boolean defaultModel: description: Default model for usage type: string @@ -2302,9 +2312,3 @@ spec: storage: true subresources: status: {} -status: - acceptedNames: - kind: "" - plural: "" - conditions: null - storedVersions: null From 5fc2b745c217e700060c09799c5da95a211e4067 Mon Sep 17 00:00:00 2001 From: Erik Lalancette Date: Tue, 14 Jul 2026 16:23:17 -0400 Subject: [PATCH 5/6] Make annotation errors non-blocking to prevent ConfigMap update starvation 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. --- internal/controller/olsconfig_helpers.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/controller/olsconfig_helpers.go b/internal/controller/olsconfig_helpers.go index 6407b84d8..49a1d77c3 100644 --- a/internal/controller/olsconfig_helpers.go +++ b/internal/controller/olsconfig_helpers.go @@ -461,7 +461,8 @@ func (r *OLSConfigReconciler) annotateExternalResources(ctx context.Context, } 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)) } r.syncOpenShiftMCPServerTLSWatcher(cr) From 33109238d87ac8ed8aa248ea48c36f102fcfd480 Mon Sep 17 00:00:00 2001 From: Erik Lalancette Date: Tue, 14 Jul 2026 18:13:11 -0400 Subject: [PATCH 6/6] fix(e2e): clear dangling AdditionalCAConfigMapRef before hot-reload test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ee29f5d7 — that was not the actual root cause. --- internal/controller/olsconfig_helpers.go | 3 +-- test/e2e/reconciliation_test.go | 4 ++++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/controller/olsconfig_helpers.go b/internal/controller/olsconfig_helpers.go index 49a1d77c3..6407b84d8 100644 --- a/internal/controller/olsconfig_helpers.go +++ b/internal/controller/olsconfig_helpers.go @@ -461,8 +461,7 @@ func (r *OLSConfigReconciler) annotateExternalResources(ctx context.Context, } if len(errs) > 0 { - r.Logger.Info("some external resource annotations failed, will retry on next reconciliation", - "failureCount", len(errs)) + return fmt.Errorf("failed to annotate %d external resources", len(errs)) } r.syncOpenShiftMCPServerTLSWatcher(cr) diff --git a/test/e2e/reconciliation_test.go b/test/e2e/reconciliation_test.go index caccad9e0..952687a2e 100644 --- a/test/e2e/reconciliation_test.go +++ b/test/e2e/reconciliation_test.go @@ -412,6 +412,10 @@ var _ = Describe("Reconciliation From OLSConfig CR", Ordered, func() { config := obj.(*olsv1alpha1.OLSConfig) hotReload := true config.Spec.OLSConfig.CredentialHotReload = &hotReload + // The previous CA-cert test deletes its ConfigMap via defer but + // leaves the dangling ref on the CR. Clear it so + // GenerateOLSConfigMap won't fail with a NotFound error. + config.Spec.OLSConfig.AdditionalCAConfigMapRef = nil return nil }) Expect(err).NotTo(HaveOccurred())