From 5ec800107a986cb1d1896e7e9eb5398ce553e95b Mon Sep 17 00:00:00 2001 From: Luca Miccini Date: Mon, 22 Jun 2026 12:23:04 +0200 Subject: [PATCH 1/2] Add credential rotation helpers for secret rotation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add shared helpers for transport URL secret rotation across all openstack-k8s-operators consumer operators: - object.ManageSecretConsumerFinalizer: adds a consumer finalizer to a secret so the provider knows consumers still depend on it - object.RemoveSecretConsumerFinalizer: removes the finalizer - object.FinalizeSecretRotation: the rotation guard — if old != new and guard is ready, removes finalizer from old and returns new; otherwise returns old to preserve the reference - object.ManageRotationGracePeriod: time-based grace period that gives sub-CRs time to detect config changes, roll pods, and update conditions before the guard evaluates readiness - condition.CredentialRotationGuardReady: returns true when all sub-CR specs are stable and all mirrored conditions are True - condition.ServiceInstanceIsReady: generation/observedGeneration guard with replica count and DeploymentReadyCondition check - statefulset.IsReady: checks ReadyReplicas, UpdatedReplicas, ObservedGeneration, and CurrentRevision == UpdateRevision so DeploymentReady is only True when all pods have rolled - deployment.IsReady: checks ReadyReplicas, UpdatedReplicas, Status.Replicas, and ObservedGeneration Note: statefulset.IsReady now additionally requires CurrentRevision == UpdateRevision compared to the previous implementation. This tightens the readiness check to prevent declaring ready during in-progress rolling updates. Co-Authored-By: Claude Opus 4.6 (1M context) --- modules/common/annotations/annotations.go | 4 + .../condition/credential_rotation_guard.md | 189 ++++++++++++ modules/common/condition/funcs.go | 31 ++ modules/common/condition/funcs_test.go | 35 +++ modules/common/deployment/deployment.go | 18 +- modules/common/deployment/deployment_test.go | 148 +++++++++ modules/common/object/metadata.go | 168 +++++++++- modules/common/object/metadata_test.go | 286 ++++++++++++++++++ modules/common/statefulset/statefulset.go | 22 +- .../common/statefulset/statefulset_test.go | 155 ++++++++++ modules/common/test/helpers/statefulset.go | 4 + 11 files changed, 1048 insertions(+), 12 deletions(-) create mode 100644 modules/common/condition/credential_rotation_guard.md create mode 100644 modules/common/deployment/deployment_test.go create mode 100644 modules/common/statefulset/statefulset_test.go diff --git a/modules/common/annotations/annotations.go b/modules/common/annotations/annotations.go index 4b23a18f..b6e12ef0 100644 --- a/modules/common/annotations/annotations.go +++ b/modules/common/annotations/annotations.go @@ -33,6 +33,10 @@ const ( // SkipValidationAnnotation is set on a resource to skip webhook validation. // The annotation key presence is what matters; the value is ignored. SkipValidationAnnotation = "openstack.org/skip-webhook-validation" + + // RotationGraceAnnotation is set on an object to track when a + // credential rotation grace period started. + RotationGraceAnnotation = "openstack.org/rotation-grace-until" ) // IsPaused returns true if the PausedAnnotation key is present on the object. diff --git a/modules/common/condition/credential_rotation_guard.md b/modules/common/condition/credential_rotation_guard.md new file mode 100644 index 00000000..a934ea84 --- /dev/null +++ b/modules/common/condition/credential_rotation_guard.md @@ -0,0 +1,189 @@ +# Credential rotation consumer finalizer guard + +OpenStack service operators that consume rotating credential secrets +(transport URL, application credentials, or similar) attach a consumer +finalizer to the old secret so it is not deleted until every sub-service +has rolled out with the new credential. + +## Shared helpers + +Use helpers from these packages instead of duplicating guard logic in each +operator: + +- `object.ManageSecretConsumerFinalizer` — add finalizer to current secret +- `object.RemoveSecretConsumerFinalizer` — remove finalizer from old secret +- `object.FinalizeSecretRotation` — rotation guard: hold old finalizer + until all sub-services are ready, then release +- `object.ManageRotationGracePeriod` — time-based grace period that gives + sub-CRs time to detect config changes, roll pods, and update conditions + before the guard evaluates readiness +- `condition.CredentialRotationGuardReady` — compute `guardReady` for the + rotation guard +- `condition.ServiceInstanceIsReady` — check whether a sub-CR has + finished reconciling (generation/observedGeneration guard, replica + counts, and DeploymentReadyCondition) +- `statefulset.IsReady` — check `*Replicas == ReadyReplicas`, + `*Replicas == UpdatedReplicas`, `Generation == ObservedGeneration`, and + `CurrentRevision == UpdateRevision` so DeploymentReady is only True when + all pods have rolled to the new config +- `deployment.IsReady` — check `*Replicas == ReadyReplicas`, + `*Replicas == UpdatedReplicas`, `Status.Replicas == ReadyReplicas`, and + `Generation == ObservedGeneration` + +## Parent controller integration + +### 1. Pass transport URL directly — never through Status + +Pass `transportURL.Status.SecretName` as a parameter to sub-CR creation +functions and config generation. Never read from +`instance.Status.TransportURLSecret` when building sub-CR specs — the +status field is only used as the "old" value for `FinalizeSecretRotation`. + +Set `instance.Status.TransportURLSecret` early only for first-time setup: + +```go +if instance.Status.TransportURLSecret == "" || + instance.Status.TransportURLSecret == transportURL.Status.SecretName { + instance.Status.TransportURLSecret = transportURL.Status.SecretName +} +``` + +During rotation (old != current), the status is updated solely by +`FinalizeSecretRotation` at the end of reconcile. + +### 2. Track stability + +Use a simple boolean to track whether all sub-CRs are stable: + +```go +allSubCRsStable := true + +// After each sub-CR CreateOrPatch: +subCR, op, err := r.subCRCreateOrUpdate(ctx, instance, transportURL.Status.SecretName) +if err != nil { return ctrl.Result{}, err } +if op != controllerutil.OperationResultNone { + allSubCRsStable = false +} +``` + +### 3. Transport URL annotation (operators without TransportURLSecret in sub-CR spec) + +For operators whose sub-CR specs do not include a `TransportURLSecret` +field (e.g. nova, watcher, glance, designate, octavia, telemetry), add +an annotation inside the `CreateOrPatch` mutate function to force a +spec change when the transport URL rotates: + +```go +op, err := controllerutil.CreateOrPatch(ctx, r.Client, subCR, func() error { + subCR.Spec = spec + if subCR.Annotations == nil { + subCR.Annotations = map[string]string{} + } + subCR.Annotations["openstack.org/transport-url-secret"] = transportURLSecretName + return controllerutil.SetControllerReference(instance, subCR, r.Scheme) +}) +``` + +This is not needed for operators that pass `TransportURLSecret` directly +in the sub-CR spec (cinder, manila, ironic, barbican, heat) — the spec +field change already makes `CreateOrPatch` return `"updated"`. + +### 4. Compute the rotation guard and finalize + +```go +guardReady := condition.CredentialRotationGuardReady( + allSubCRsStable, + &instance.Status.Conditions, +) + +instance.Status.TransportURLSecret, err = object.FinalizeSecretRotation( + ctx, helper, instance.Namespace, + instance.Status.TransportURLSecret, + transportURL.Status.SecretName, + myTransportConsumerFinalizer, + guardReady, +) +``` + +The same pattern works for application credential secrets: + +```go +instance.Status.ApplicationCredentialSecret, err = object.FinalizeSecretRotation( + ctx, helper, instance.Namespace, + instance.Status.ApplicationCredentialSecret, + instance.Spec.Auth.ApplicationCredentialSecret, + myACConsumerFinalizer, + guardReady, +) +``` + +### 5. Rotation grace period (optional) + +Use `ManageRotationGracePeriod` to give sub-CRs time to detect config +changes, update their Deployments/StatefulSets, and roll pods before the +guard releases the old secret's consumer finalizer: + +```go +rotationPending := instance.Status.TransportURLSecret != "" && + instance.Status.TransportURLSecret != transportURL.Status.SecretName + +result, graceActive, err := object.ManageRotationGracePeriod( + ctx, r.Client, instance, + rotationPending, + 30*time.Second, +) +if err != nil { return ctrl.Result{}, err } +if graceActive { + return result, nil +} +``` + +When `rotationPending` is true and no grace period is active, it sets the +`openstack.org/rotation-grace-until` annotation and returns a requeue. +While the grace period is active, it continues to requeue. After the +grace period expires, it returns `graceActive == false` so the caller +can proceed to evaluate the rotation guard. When `rotationPending` is +false, it clears any existing annotation. + +## Sub-CR integration + +Sub-CR controllers should use `statefulset.IsReady()` or +`deployment.IsReady()` for the `DeploymentReadyCondition` check: + +```go +if statefulset.IsReady(sts) { + instance.Status.Conditions.MarkTrue( + condition.DeploymentReadyCondition, + condition.DeploymentReadyMessage) +} else if *instance.Spec.Replicas > 0 { + instance.Status.Conditions.Set(condition.FalseCondition( + condition.DeploymentReadyCondition, + condition.RequestedReason, + condition.SeverityInfo, + condition.DeploymentReadyRunningMessage)) +} +``` + +`statefulset.IsReady` takes an `appsv1.StatefulSet` and checks +`*Replicas == ReadyReplicas`, `*Replicas == UpdatedReplicas`, +`Generation == ObservedGeneration`, and `CurrentRevision == UpdateRevision`. + +`deployment.IsReady` takes an `appsv1.Deployment` and checks +`*Replicas == ReadyReplicas`, `*Replicas == UpdatedReplicas`, +`Status.Replicas == ReadyReplicas`, and `Generation == ObservedGeneration`. + +Both ensure DeploymentReady is only True when all pods have rolled to +the new config. + +For parent operators that need to check sub-CR readiness directly (e.g. +to feed into the stability tracker), use `ServiceInstanceIsReady`: + +```go +ready := condition.ServiceInstanceIsReady( + subCR.Generation, + subCR.Status.ObservedGeneration, + subCR.Status.ReadyCount, + *subCR.Spec.Replicas, + &subCR.Status.Conditions, +) +``` diff --git a/modules/common/condition/funcs.go b/modules/common/condition/funcs.go index ecfb31a7..ff8c19e2 100644 --- a/modules/common/condition/funcs.go +++ b/modules/common/condition/funcs.go @@ -166,6 +166,37 @@ func (conditions *Conditions) IsUnknown(t Type) bool { return true } +// ServiceInstanceIsReady reports whether a service sub-CR has finished +// reconciling its current spec and its deployment is ready to serve +// requests. It implements the generation/observedGeneration guard used +// by OpenStack service operators during credential rotation rollouts. +func ServiceInstanceIsReady(generation, observedGeneration int64, readyCount, replicas int32, conditions *Conditions) bool { + if conditions == nil { + return false + } + if generation != observedGeneration || readyCount != replicas { + return false + } + if replicas == 0 { + return true + } + return conditions.IsTrue(DeploymentReadyCondition) +} + +// CredentialRotationGuardReady reports whether it is safe to remove a +// consumer finalizer from an old transport URL or application credential +// secret. The guard requires all sub-CR specs to be stable (no pending +// CreateOrPatch updates) and all parent sub-conditions to be True. +// Note: ReadyCondition is NOT checked because operators reset conditions +// via Init() at the top of each Reconcile, so ReadyCondition is Unknown +// until the very end. AllSubConditionIsTrue covers the same safety. +func CredentialRotationGuardReady(allSubCRsStable bool, conditions *Conditions) bool { + if conditions == nil { + return false + } + return allSubCRsStable && conditions.AllSubConditionIsTrue() +} + // AllSubConditionIsTrue validates if all subconditions are True // It assumes that all conditions report success via the True status func (conditions *Conditions) AllSubConditionIsTrue() bool { diff --git a/modules/common/condition/funcs_test.go b/modules/common/condition/funcs_test.go index 27a845ae..69e7b545 100644 --- a/modules/common/condition/funcs_test.go +++ b/modules/common/condition/funcs_test.go @@ -351,6 +351,41 @@ func TestIsMethods(t *testing.T) { g.Expect(conditions.IsUnknown("unknownB")).To(BeTrue()) } +func TestServiceInstanceIsReady(t *testing.T) { + g := NewWithT(t) + + readyConditions := CreateList( + TrueCondition(DeploymentReadyCondition, "deployment ready"), + ) + + notReadyConditions := CreateList( + FalseCondition(DeploymentReadyCondition, InitReason, SeverityInfo, "creating"), + ) + + g.Expect(ServiceInstanceIsReady(2, 2, 3, 3, &readyConditions)).To(BeTrue()) + g.Expect(ServiceInstanceIsReady(2, 1, 3, 3, &readyConditions)).To(BeFalse()) + g.Expect(ServiceInstanceIsReady(2, 2, 2, 3, &readyConditions)).To(BeFalse()) + g.Expect(ServiceInstanceIsReady(2, 2, 3, 3, ¬ReadyConditions)).To(BeFalse()) + g.Expect(ServiceInstanceIsReady(1, 1, 0, 0, ¬ReadyConditions)).To(BeTrue()) + g.Expect(ServiceInstanceIsReady(1, 1, 0, 0, nil)).To(BeFalse()) +} + +func TestCredentialRotationGuardReady(t *testing.T) { + g := NewWithT(t) + + conditions := Conditions{} + conditions.Init(nil) + conditions.MarkTrue("a", "ready") + conditions.MarkTrue("b", "ready") + + g.Expect(CredentialRotationGuardReady(true, &conditions)).To(BeTrue()) + conditions.MarkFalse("a", InitReason, SeverityInfo, "not ready") + g.Expect(CredentialRotationGuardReady(true, &conditions)).To(BeFalse()) + conditions.MarkTrue("a", "ready") + g.Expect(CredentialRotationGuardReady(false, &conditions)).To(BeFalse()) + g.Expect(CredentialRotationGuardReady(true, nil)).To(BeFalse()) +} + func TestAllSubConditionIsTrue(t *testing.T) { conditions := Conditions{} diff --git a/modules/common/deployment/deployment.go b/modules/common/deployment/deployment.go index e4f7d27f..11a8682c 100644 --- a/modules/common/deployment/deployment.go +++ b/modules/common/deployment/deployment.go @@ -87,10 +87,20 @@ func (d *Deployment) CreateOrPatch( h.GetLogger().Info(fmt.Sprintf("Deployment %s - %s", deployment.Name, op)) } - // update the deployment object of the deployment type - d.deployment, err = GetDeploymentWithName(ctx, h, deployment.GetName(), deployment.GetNamespace()) - if err != nil { - return ctrl.Result{}, err + if op == controllerutil.OperationResultNone { + // Re-read from cache to pick up status updates from the + // Deployment controller (e.g. updated ReadyReplicas). + d.deployment, err = GetDeploymentWithName(ctx, h, deployment.GetName(), deployment.GetNamespace()) + if err != nil { + return ctrl.Result{}, err + } + } else { + // After a create/update the informer cache may still hold the + // previous version where Generation == ObservedGeneration. Using + // the server-returned object preserves the correct (bumped) + // Generation so that callers' readiness checks do not pass on + // stale data. + d.deployment = deployment } return ctrl.Result{}, nil diff --git a/modules/common/deployment/deployment_test.go b/modules/common/deployment/deployment_test.go new file mode 100644 index 00000000..d2431764 --- /dev/null +++ b/modules/common/deployment/deployment_test.go @@ -0,0 +1,148 @@ +/* +Copyright 2026 Red Hat + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package deployment + +import ( + "testing" + + . "github.com/onsi/gomega" + appsv1 "k8s.io/api/apps/v1" + "k8s.io/utils/ptr" +) + +func TestIsReady(t *testing.T) { + tests := []struct { + name string + depl appsv1.Deployment + want bool + }{ + { + name: "all ready", + depl: appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To[int32](3), + }, + Status: appsv1.DeploymentStatus{ + Replicas: 3, + ReadyReplicas: 3, + UpdatedReplicas: 3, + ObservedGeneration: 1, + }, + }, + want: true, + }, + { + name: "replicas nil", + depl: appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{}, + Status: appsv1.DeploymentStatus{ + Replicas: 0, + ReadyReplicas: 0, + UpdatedReplicas: 0, + ObservedGeneration: 0, + }, + }, + want: false, + }, + { + name: "ready replicas mismatch", + depl: appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To[int32](3), + }, + Status: appsv1.DeploymentStatus{ + Replicas: 3, + ReadyReplicas: 2, + UpdatedReplicas: 3, + ObservedGeneration: 1, + }, + }, + want: false, + }, + { + name: "updated replicas mismatch", + depl: appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To[int32](3), + }, + Status: appsv1.DeploymentStatus{ + Replicas: 3, + ReadyReplicas: 3, + UpdatedReplicas: 2, + ObservedGeneration: 1, + }, + }, + want: false, + }, + { + name: "status replicas differs from ready replicas during rolling update", + depl: appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To[int32](3), + }, + Status: appsv1.DeploymentStatus{ + Replicas: 4, + ReadyReplicas: 3, + UpdatedReplicas: 3, + ObservedGeneration: 1, + }, + }, + want: false, + }, + { + name: "generation mismatch", + depl: appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To[int32](3), + }, + Status: appsv1.DeploymentStatus{ + Replicas: 3, + ReadyReplicas: 3, + UpdatedReplicas: 3, + ObservedGeneration: 0, + }, + }, + want: false, + }, + { + name: "zero replicas all match", + depl: appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To[int32](0), + }, + Status: appsv1.DeploymentStatus{ + Replicas: 0, + ReadyReplicas: 0, + UpdatedReplicas: 0, + ObservedGeneration: 0, + }, + }, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + tt.depl.Generation = tt.depl.Status.ObservedGeneration + if tt.name == "generation mismatch" { + tt.depl.Generation = tt.depl.Status.ObservedGeneration + 1 + } + g.Expect(IsReady(tt.depl)).To(Equal(tt.want)) + }) + } +} diff --git a/modules/common/object/metadata.go b/modules/common/object/metadata.go index a66b5fdb..ae5e99f6 100644 --- a/modules/common/object/metadata.go +++ b/modules/common/object/metadata.go @@ -22,15 +22,18 @@ import ( "encoding/json" "fmt" "slices" + "time" + commonannotations "github.com/openstack-k8s-operators/lib-common/modules/common/annotations" "github.com/openstack-k8s-operators/lib-common/modules/common/helper" + corev1 "k8s.io/api/core/v1" + k8s_errors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - - k8s_errors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) // CheckOwnerRefExist - returns true if the owner is already in the owner ref list @@ -181,3 +184,162 @@ func ManageConsumerFinalizer( return nil } + +// ManageSecretConsumerFinalizer ensures consumerFinalizer is present on the +// secret identified by secretName. It is a no-op when secretName is empty. +func ManageSecretConsumerFinalizer( + ctx context.Context, + h *helper.Helper, + namespace string, + secretName string, + consumerFinalizer string, +) error { + if secretName == "" { + return nil + } + + secret := &corev1.Secret{} + key := types.NamespacedName{Name: secretName, Namespace: namespace} + if err := h.GetClient().Get(ctx, key, secret); err != nil { + return fmt.Errorf("failed to get secret %s: %w", secretName, err) + } + + return AddConsumerFinalizer(ctx, h, secret, consumerFinalizer) +} + +// RemoveSecretConsumerFinalizer removes consumerFinalizer from the secret +// identified by secretName. It is a no-op when secretName is empty or the +// secret no longer exists. +func RemoveSecretConsumerFinalizer( + ctx context.Context, + h *helper.Helper, + namespace string, + secretName string, + consumerFinalizer string, +) error { + if secretName == "" { + return nil + } + + secret := &corev1.Secret{} + key := types.NamespacedName{Name: secretName, Namespace: namespace} + if err := h.GetClient().Get(ctx, key, secret); err != nil { + if k8s_errors.IsNotFound(err) { + return nil + } + return fmt.Errorf("failed to get secret %s: %w", secretName, err) + } + return RemoveConsumerFinalizer(ctx, h, secret, consumerFinalizer) +} + +// FinalizeSecretRotation handles the rotation guard for a credential secret +// (transport URL, application credential, or any other rotating secret). +// It detects whether rotation is in progress and: +// - If no rotation (statusSecretName == currentSecretName or statusSecretName +// is empty): returns currentSecretName +// - If rotation detected and guardReady is true: removes the consumer +// finalizer from the old secret and returns currentSecretName +// - If rotation detected and guardReady is false: returns +// statusSecretName unchanged (finalizer held, rotation pending) +// +// Compute guardReady with condition.CredentialRotationGuardReady from lib-common. +func FinalizeSecretRotation( + ctx context.Context, + h *helper.Helper, + namespace string, + statusSecretName string, + currentSecretName string, + consumerFinalizer string, + guardReady bool, +) (string, error) { + if statusSecretName == "" || statusSecretName == currentSecretName { + return currentSecretName, nil + } + + if !guardReady { + return statusSecretName, nil + } + + if err := RemoveSecretConsumerFinalizer( + ctx, h, namespace, statusSecretName, consumerFinalizer, + ); err != nil { + return statusSecretName, err + } + return currentSecretName, nil +} + +// ManageRotationGracePeriod manages a time-based grace period during +// credential rotation. When rotationPending is true and no grace period +// is active, it sets a timestamp annotation and returns (requeue, true, nil). +// While the grace period is active, it returns (requeue, true, nil) with +// the remaining time. After the grace period expires, it returns +// ({}, false, nil) so the caller can evaluate the rotation guard. +// When rotationPending is false, it clears any existing annotation. +// +// This gives sub-CRs time to detect config changes, update their +// Deployments/StatefulSets, and roll pods before the guard releases +// the old secret's consumer finalizer. +func ManageRotationGracePeriod( + ctx context.Context, + c client.Client, + obj client.Object, + rotationPending bool, + gracePeriod time.Duration, +) (ctrl.Result, bool, error) { + annotations := obj.GetAnnotations() + + if !rotationPending { + if annotations != nil { + if _, has := annotations[commonannotations.RotationGraceAnnotation]; has { + before := obj.DeepCopyObject().(client.Object) + delete(annotations, commonannotations.RotationGraceAnnotation) + obj.SetAnnotations(annotations) + if err := c.Patch(ctx, obj, client.MergeFrom(before)); err != nil { + return ctrl.Result{}, false, err + } + } + } + return ctrl.Result{}, false, nil + } + + graceUntilStr := "" + if annotations != nil { + graceUntilStr = annotations[commonannotations.RotationGraceAnnotation] + } + + if graceUntilStr == "" { + before := obj.DeepCopyObject().(client.Object) + graceUntil := time.Now().Add(gracePeriod) + if annotations == nil { + annotations = map[string]string{} + } + annotations[commonannotations.RotationGraceAnnotation] = graceUntil.Format(time.RFC3339) + obj.SetAnnotations(annotations) + if err := c.Patch(ctx, obj, client.MergeFrom(before)); err != nil { + return ctrl.Result{}, false, err + } + return ctrl.Result{RequeueAfter: gracePeriod}, true, nil + } + + graceUntil, err := time.Parse(time.RFC3339, graceUntilStr) + if err != nil { + before := obj.DeepCopyObject().(client.Object) + delete(annotations, commonannotations.RotationGraceAnnotation) + obj.SetAnnotations(annotations) + if patchErr := c.Patch(ctx, obj, client.MergeFrom(before)); patchErr != nil { + return ctrl.Result{}, false, patchErr + } + return ctrl.Result{}, false, nil + } + + if time.Now().Before(graceUntil) { + remaining := time.Until(graceUntil) + return ctrl.Result{RequeueAfter: remaining}, true, nil + } + + // Grace period expired — let the caller evaluate the guard. + // The annotation stays until rotationPending becomes false + // (after FinalizeSecretRotation updates the status), at which + // point the !rotationPending branch above clears it. + return ctrl.Result{}, false, nil +} diff --git a/modules/common/object/metadata_test.go b/modules/common/object/metadata_test.go index 7cdf7282..88791812 100644 --- a/modules/common/object/metadata_test.go +++ b/modules/common/object/metadata_test.go @@ -17,15 +17,30 @@ limitations under the License. package object import ( + "context" + "errors" "testing" + "time" + + commonannotations "github.com/openstack-k8s-operators/lib-common/modules/common/annotations" + "github.com/openstack-k8s-operators/lib-common/modules/common/helper" . "github.com/onsi/gomega" // nolint:revive + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/scheme" "k8s.io/utils/ptr" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) +var errSyntheticAPI = errors.New("synthetic API server error") + var ( metadata = metav1.ObjectMeta{ Name: "foo", @@ -43,6 +58,37 @@ var ( } ) +func setupHelper(objs ...client.Object) (*helper.Helper, error) { + s := scheme.Scheme + + fakeClient := fake.NewClientBuilder(). + WithScheme(s). + WithObjects(objs...). + Build() + + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "test-namespace"}, + } + + return helper.NewHelper(ns, fakeClient, nil, s, ctrl.Log) +} + +func setupHelperWithInterceptor(funcs interceptor.Funcs, objs ...client.Object) (*helper.Helper, error) { + s := scheme.Scheme + + fakeClient := fake.NewClientBuilder(). + WithScheme(s). + WithObjects(objs...). + WithInterceptorFuncs(funcs). + Build() + + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "test-namespace"}, + } + + return helper.NewHelper(ns, fakeClient, nil, s, ctrl.Log) +} + func TestCheckOwnerRefExist(t *testing.T) { tests := []struct { name string @@ -72,3 +118,243 @@ func TestCheckOwnerRefExist(t *testing.T) { }) } } + +func TestFinalizeSecretRotation(t *testing.T) { + t.Parallel() + + ctx := context.Background() + const ( + namespace = "openstack" + finalizer = "openstack.org/test-consumer" + ) + + t.Run("early return paths", func(t *testing.T) { + tests := []struct { + name string + statusSecret string + currentSecret string + guardReady bool + wantSecret string + }{ + { + name: "no rotation - empty status", + statusSecret: "", + currentSecret: "new-secret", + guardReady: true, + wantSecret: "new-secret", + }, + { + name: "no rotation - same secret", + statusSecret: "same-secret", + currentSecret: "same-secret", + guardReady: true, + wantSecret: "same-secret", + }, + { + name: "rotation in progress - guard not ready", + statusSecret: "old-secret", + currentSecret: "new-secret", + guardReady: false, + wantSecret: "old-secret", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + got, err := FinalizeSecretRotation( + ctx, nil, namespace, + tt.statusSecret, tt.currentSecret, + finalizer, tt.guardReady, + ) + + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(got).To(Equal(tt.wantSecret)) + }) + } + }) + + t.Run("rotation complete - removes finalizer from old secret", func(t *testing.T) { + g := NewWithT(t) + + oldSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "old-secret", + Namespace: namespace, + }, + } + controllerutil.AddFinalizer(oldSecret, finalizer) + + h, err := setupHelper(oldSecret) + g.Expect(err).NotTo(HaveOccurred()) + + got, err := FinalizeSecretRotation( + ctx, h, namespace, + "old-secret", "new-secret", + finalizer, true, + ) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(got).To(Equal("new-secret")) + + updated := &corev1.Secret{} + g.Expect(h.GetClient().Get(ctx, types.NamespacedName{ + Name: "old-secret", Namespace: namespace, + }, updated)).To(Succeed()) + g.Expect(controllerutil.ContainsFinalizer(updated, finalizer)).To(BeFalse()) + }) + + t.Run("rotation complete - old secret already deleted", func(t *testing.T) { + g := NewWithT(t) + + h, err := setupHelper() + g.Expect(err).NotTo(HaveOccurred()) + + got, err := FinalizeSecretRotation( + ctx, h, namespace, + "old-secret", "new-secret", + finalizer, true, + ) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(got).To(Equal("new-secret")) + }) + + t.Run("rotation complete - Get fails with non-NotFound error", func(t *testing.T) { + g := NewWithT(t) + + h, err := setupHelperWithInterceptor(interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*corev1.Secret); ok { + return errSyntheticAPI + } + return c.Get(ctx, key, obj, opts...) + }, + }) + g.Expect(err).NotTo(HaveOccurred()) + + got, err := FinalizeSecretRotation( + ctx, h, namespace, + "old-secret", "new-secret", + finalizer, true, + ) + g.Expect(err).To(HaveOccurred()) + g.Expect(got).To(Equal("old-secret")) + }) +} + +func TestManageRotationGracePeriod(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + newConfigMap := func(annotations map[string]string) *corev1.ConfigMap { + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-instance", + Namespace: "openstack", + Annotations: annotations, + }, + } + } + + newFakeClient := func(objs ...client.Object) client.Client { + return fake.NewClientBuilder(). + WithScheme(scheme.Scheme). + WithObjects(objs...). + Build() + } + + getAnnotation := func(g Gomega, c client.Client, cm *corev1.ConfigMap) string { + updated := &corev1.ConfigMap{} + g.Expect(c.Get(ctx, types.NamespacedName{ + Name: cm.Name, Namespace: cm.Namespace, + }, updated)).To(Succeed()) + return updated.Annotations[commonannotations.RotationGraceAnnotation] + } + + t.Run("not pending, no annotation - no-op", func(t *testing.T) { + g := NewWithT(t) + cm := newConfigMap(nil) + c := newFakeClient(cm) + + result, graceActive, err := ManageRotationGracePeriod(ctx, c, cm, false, 30*time.Second) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(graceActive).To(BeFalse()) + g.Expect(result).To(Equal(ctrl.Result{})) + }) + + t.Run("not pending, annotation present - clears it", func(t *testing.T) { + g := NewWithT(t) + cm := newConfigMap(map[string]string{ + commonannotations.RotationGraceAnnotation: time.Now().Add(1 * time.Minute).Format(time.RFC3339), + }) + c := newFakeClient(cm) + + result, graceActive, err := ManageRotationGracePeriod(ctx, c, cm, false, 30*time.Second) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(graceActive).To(BeFalse()) + g.Expect(result).To(Equal(ctrl.Result{})) + g.Expect(getAnnotation(g, c, cm)).To(BeEmpty()) + }) + + t.Run("pending, no annotation - sets grace period", func(t *testing.T) { + g := NewWithT(t) + cm := newConfigMap(nil) + c := newFakeClient(cm) + + before := time.Now() + result, graceActive, err := ManageRotationGracePeriod(ctx, c, cm, true, 30*time.Second) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(graceActive).To(BeTrue()) + g.Expect(result.RequeueAfter).To(Equal(30 * time.Second)) + + ann := getAnnotation(g, c, cm) + g.Expect(ann).NotTo(BeEmpty()) + parsed, parseErr := time.Parse(time.RFC3339, ann) + g.Expect(parseErr).NotTo(HaveOccurred()) + g.Expect(parsed).To(BeTemporally("~", before.Add(30*time.Second), 2*time.Second)) + }) + + t.Run("pending, annotation in future - requeues with remaining", func(t *testing.T) { + g := NewWithT(t) + future := time.Now().Add(1 * time.Minute) + cm := newConfigMap(map[string]string{ + commonannotations.RotationGraceAnnotation: future.Format(time.RFC3339), + }) + c := newFakeClient(cm) + + result, graceActive, err := ManageRotationGracePeriod(ctx, c, cm, true, 30*time.Second) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(graceActive).To(BeTrue()) + g.Expect(result.RequeueAfter).To(BeNumerically(">", 0)) + g.Expect(result.RequeueAfter).To(BeNumerically("<=", 1*time.Minute)) + }) + + t.Run("pending, annotation expired - grace period over", func(t *testing.T) { + g := NewWithT(t) + past := time.Now().Add(-1 * time.Minute) + cm := newConfigMap(map[string]string{ + commonannotations.RotationGraceAnnotation: past.Format(time.RFC3339), + }) + c := newFakeClient(cm) + + result, graceActive, err := ManageRotationGracePeriod(ctx, c, cm, true, 30*time.Second) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(graceActive).To(BeFalse()) + g.Expect(result).To(Equal(ctrl.Result{})) + }) + + t.Run("pending, malformed annotation - clears it", func(t *testing.T) { + g := NewWithT(t) + cm := newConfigMap(map[string]string{ + commonannotations.RotationGraceAnnotation: "not-a-timestamp", + }) + c := newFakeClient(cm) + + result, graceActive, err := ManageRotationGracePeriod(ctx, c, cm, true, 30*time.Second) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(graceActive).To(BeFalse()) + g.Expect(result).To(Equal(ctrl.Result{})) + g.Expect(getAnnotation(g, c, cm)).To(BeEmpty()) + }) +} diff --git a/modules/common/statefulset/statefulset.go b/modules/common/statefulset/statefulset.go index eb3c8452..fd42eb2e 100644 --- a/modules/common/statefulset/statefulset.go +++ b/modules/common/statefulset/statefulset.go @@ -108,10 +108,20 @@ func (s *StatefulSet) CreateOrPatch( h.GetLogger().Info(fmt.Sprintf("StatefulSet %s - %s", statefulset.Name, op)) } - // update the statefulset object of the statefulset type - s.statefulset, err = GetStatefulSetWithName(ctx, h, statefulset.GetName(), statefulset.GetNamespace()) - if err != nil { - return ctrl.Result{}, err + if op == controllerutil.OperationResultNone { + // Re-read from cache to pick up status updates from the + // StatefulSet controller (e.g. updated ReadyReplicas). + s.statefulset, err = GetStatefulSetWithName(ctx, h, statefulset.GetName(), statefulset.GetNamespace()) + if err != nil { + return ctrl.Result{}, err + } + } else { + // After a create/update the informer cache may still hold the + // previous version where Generation == ObservedGeneration. Using + // the server-returned object preserves the correct (bumped) + // Generation so that callers' readiness checks do not pass on + // stale data. + s.statefulset = statefulset } return ctrl.Result{}, nil @@ -157,9 +167,11 @@ func (s *StatefulSet) Delete( // - the requested replicas in the spec matches the ReadyReplicas of the status // - all pods run the current spec (UpdatedReplicas == requested replicas) // - both when the Generatation of the object matches the ObservedGeneration of the Status +// - the rollout is complete (CurrentRevision == UpdateRevision) func IsReady(deployment appsv1.StatefulSet) bool { return deployment.Spec.Replicas != nil && *deployment.Spec.Replicas == deployment.Status.ReadyReplicas && *deployment.Spec.Replicas == deployment.Status.UpdatedReplicas && - deployment.Generation == deployment.Status.ObservedGeneration + deployment.Generation == deployment.Status.ObservedGeneration && + deployment.Status.CurrentRevision == deployment.Status.UpdateRevision } diff --git a/modules/common/statefulset/statefulset_test.go b/modules/common/statefulset/statefulset_test.go new file mode 100644 index 00000000..da162e7f --- /dev/null +++ b/modules/common/statefulset/statefulset_test.go @@ -0,0 +1,155 @@ +/* +Copyright 2026 Red Hat + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package statefulset + +import ( + "testing" + + . "github.com/onsi/gomega" + appsv1 "k8s.io/api/apps/v1" + "k8s.io/utils/ptr" +) + +func TestIsReady(t *testing.T) { + tests := []struct { + name string + sts appsv1.StatefulSet + want bool + }{ + { + name: "all ready and revisions match", + sts: appsv1.StatefulSet{ + Spec: appsv1.StatefulSetSpec{ + Replicas: ptr.To[int32](3), + }, + Status: appsv1.StatefulSetStatus{ + ReadyReplicas: 3, + UpdatedReplicas: 3, + ObservedGeneration: 1, + CurrentRevision: "rev-abc", + UpdateRevision: "rev-abc", + }, + }, + want: true, + }, + { + name: "replicas nil", + sts: appsv1.StatefulSet{ + Spec: appsv1.StatefulSetSpec{}, + Status: appsv1.StatefulSetStatus{ + ReadyReplicas: 0, + UpdatedReplicas: 0, + ObservedGeneration: 0, + CurrentRevision: "rev-abc", + UpdateRevision: "rev-abc", + }, + }, + want: false, + }, + { + name: "ready replicas mismatch", + sts: appsv1.StatefulSet{ + Spec: appsv1.StatefulSetSpec{ + Replicas: ptr.To[int32](3), + }, + Status: appsv1.StatefulSetStatus{ + ReadyReplicas: 2, + UpdatedReplicas: 3, + ObservedGeneration: 1, + CurrentRevision: "rev-abc", + UpdateRevision: "rev-abc", + }, + }, + want: false, + }, + { + name: "updated replicas mismatch", + sts: appsv1.StatefulSet{ + Spec: appsv1.StatefulSetSpec{ + Replicas: ptr.To[int32](3), + }, + Status: appsv1.StatefulSetStatus{ + ReadyReplicas: 3, + UpdatedReplicas: 2, + ObservedGeneration: 1, + CurrentRevision: "rev-abc", + UpdateRevision: "rev-abc", + }, + }, + want: false, + }, + { + name: "generation mismatch", + sts: appsv1.StatefulSet{ + Spec: appsv1.StatefulSetSpec{ + Replicas: ptr.To[int32](3), + }, + Status: appsv1.StatefulSetStatus{ + ReadyReplicas: 3, + UpdatedReplicas: 3, + ObservedGeneration: 0, + CurrentRevision: "rev-abc", + UpdateRevision: "rev-abc", + }, + }, + want: false, + }, + { + name: "current revision differs from update revision", + sts: appsv1.StatefulSet{ + Spec: appsv1.StatefulSetSpec{ + Replicas: ptr.To[int32](3), + }, + Status: appsv1.StatefulSetStatus{ + ReadyReplicas: 3, + UpdatedReplicas: 3, + ObservedGeneration: 1, + CurrentRevision: "rev-old", + UpdateRevision: "rev-new", + }, + }, + want: false, + }, + { + name: "zero replicas all match", + sts: appsv1.StatefulSet{ + Spec: appsv1.StatefulSetSpec{ + Replicas: ptr.To[int32](0), + }, + Status: appsv1.StatefulSetStatus{ + ReadyReplicas: 0, + UpdatedReplicas: 0, + ObservedGeneration: 0, + CurrentRevision: "", + UpdateRevision: "", + }, + }, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + tt.sts.Generation = tt.sts.Status.ObservedGeneration + if tt.name == "generation mismatch" { + tt.sts.Generation = tt.sts.Status.ObservedGeneration + 1 + } + g.Expect(IsReady(tt.sts)).To(Equal(tt.want)) + }) + } +} diff --git a/modules/common/test/helpers/statefulset.go b/modules/common/test/helpers/statefulset.go index 170dc3e8..431ac31b 100644 --- a/modules/common/test/helpers/statefulset.go +++ b/modules/common/test/helpers/statefulset.go @@ -52,6 +52,8 @@ func (tc *TestHelper) SimulateStatefulSetReplicaReady(name types.NamespacedName) ss.Status.ReadyReplicas = *ss.Spec.Replicas ss.Status.UpdatedReplicas = *ss.Spec.Replicas ss.Status.ObservedGeneration = ss.Generation + ss.Status.CurrentRevision = fmt.Sprintf("%s-rev", name.Name) + ss.Status.UpdateRevision = fmt.Sprintf("%s-rev", name.Name) g.Expect(tc.K8sClient.Status().Update(tc.Ctx, ss)).To(gomega.Succeed()) }, tc.Timeout, tc.Interval).Should(gomega.Succeed()) @@ -115,6 +117,8 @@ func (tc *TestHelper) SimulateStatefulSetReplicaReadyWithPods(name types.Namespa ss.Status.ReadyReplicas = *ss.Spec.Replicas ss.Status.UpdatedReplicas = *ss.Spec.Replicas ss.Status.ObservedGeneration = ss.Generation + ss.Status.CurrentRevision = fmt.Sprintf("%s-rev", name.Name) + ss.Status.UpdateRevision = fmt.Sprintf("%s-rev", name.Name) g.Expect(tc.K8sClient.Status().Update(tc.Ctx, ss)).To(gomega.Succeed()) }, tc.Timeout, tc.Interval).Should(gomega.Succeed()) From 8dfcc3e3c06c70c9fc5db3b1478548d25156a707 Mon Sep 17 00:00:00 2001 From: Luca Miccini Date: Thu, 25 Jun 2026 10:17:40 +0200 Subject: [PATCH 2/2] Make test pod simulation helpers idempotent SimulateStatefulSetReplicaReadyWithPods and SimulateDeploymentReadyWithPods fail when called multiple times because they unconditionally Create pods that may already exist. This prevents using them inside Eventually loops, which is needed for operators like designate where multiple controllers compete and readiness simulation must be retried. Changes: - Check if pod exists before creating; update annotations on existing pods instead of failing - Fix nil Annotations map that could panic when template has no annotations - Use deterministic pod names for Deployments ({name}-{index} instead of GenerateName) so pods can be found on re-call Co-Authored-By: Claude Opus 4.6 (1M context) --- modules/common/test/helpers/deployment.go | 14 ++++++++++++-- modules/common/test/helpers/statefulset.go | 11 ++++++++++- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/modules/common/test/helpers/deployment.go b/modules/common/test/helpers/deployment.go index bf5677b3..363300aa 100644 --- a/modules/common/test/helpers/deployment.go +++ b/modules/common/test/helpers/deployment.go @@ -108,7 +108,8 @@ func (tc *TestHelper) SimulateDeploymentReadyWithPods(name types.NamespacedName, Spec: depl.Spec.Template.Spec, } pod.Namespace = name.Namespace - pod.GenerateName = name.Name + pod.Name = fmt.Sprintf("%s-%d", name.Name, i) + pod.GenerateName = "" // NOTE(gibi): If there is a mount that refers to a volume created via // persistent volume claim then that mount won't have a corresponding // volume created in EnvTest as we are not simulating the k8s volume @@ -136,10 +137,19 @@ func (tc *TestHelper) SimulateDeploymentReadyWithPods(name types.NamespacedName, } netStatusAnnotation, err := json.Marshal(netStatus) gomega.Expect(err).NotTo(gomega.HaveOccurred()) + if pod.Annotations == nil { + pod.Annotations = map[string]string{} + } pod.Annotations[networkv1.NetworkStatusAnnot] = string(netStatusAnnotation) } - gomega.Expect(tc.K8sClient.Create(tc.Ctx, pod)).Should(gomega.Succeed()) + existingPod := &corev1.Pod{} + if tc.K8sClient.Get(tc.Ctx, types.NamespacedName{Name: pod.Name, Namespace: pod.Namespace}, existingPod) == nil { + existingPod.Annotations = pod.Annotations + gomega.Expect(tc.K8sClient.Update(tc.Ctx, existingPod)).Should(gomega.Succeed()) + } else { + gomega.Expect(tc.K8sClient.Create(tc.Ctx, pod)).Should(gomega.Succeed()) + } } gomega.Eventually(func(g gomega.Gomega) { diff --git a/modules/common/test/helpers/statefulset.go b/modules/common/test/helpers/statefulset.go index 431ac31b..c34bd3ff 100644 --- a/modules/common/test/helpers/statefulset.go +++ b/modules/common/test/helpers/statefulset.go @@ -105,9 +105,18 @@ func (tc *TestHelper) SimulateStatefulSetReplicaReadyWithPods(name types.Namespa } netStatusAnnotation, err := json.Marshal(netStatus) gomega.Expect(err).NotTo(gomega.HaveOccurred()) + if pod.Annotations == nil { + pod.Annotations = map[string]string{} + } pod.Annotations[networkv1.NetworkStatusAnnot] = string(netStatusAnnotation) - gomega.Expect(tc.K8sClient.Create(tc.Ctx, pod)).Should(gomega.Succeed()) + existingPod := &corev1.Pod{} + if tc.K8sClient.Get(tc.Ctx, types.NamespacedName{Name: pod.Name, Namespace: pod.Namespace}, existingPod) == nil { + existingPod.Annotations = pod.Annotations + gomega.Expect(tc.K8sClient.Update(tc.Ctx, existingPod)).Should(gomega.Succeed()) + } else { + gomega.Expect(tc.K8sClient.Create(tc.Ctx, pod)).Should(gomega.Succeed()) + } } gomega.Eventually(func(g gomega.Gomega) {