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/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 170dc3e8..c34bd3ff 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()) @@ -103,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) { @@ -115,6 +126,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())