From 12126341a263db0a5ea1fa4cf2d2471b87a36356 Mon Sep 17 00:00:00 2001 From: Luca Miccini Date: Wed, 17 Jun 2026 14:52:01 +0200 Subject: [PATCH 1/3] Immutable secrets and consumer finalizers for TransportURL credential rotation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implement safe credential rotation for TransportURL by creating immutable secrets with content-hashed names, adding per-consumer finalizers to coordinate lifecycle between TransportURL and RabbitMQUser CRs, and gating old user release on both consumer finalizer removal and NodeSet secret hash synchronization. Key changes: - Create immutable transport secrets (rabbitmq-transport-url-{name}-{hash}) during rotation to prevent content mutation by consumers - Add per-consumer finalizers (turl.openstack.org/t-{name}) on shared RabbitMQUser and RabbitMQVhost CRs to track active consumers - Add transport secret consumer finalizer protocol for consuming operators to signal rollout completion - Unified release path: wait for consumer finalizer removal, then check NodeSet secret hash sync — if hashes are in sync the secret is not tracked by the dataplane and the old user is released immediately; if out of sync, wait for full NodeSet deployment to complete - Prevent SecretName flip-flop by comparing content hashes before deciding whether to create a new immutable secret - Auto-delete orphaned RabbitMQUser CRs when all consumers release their finalizers Co-Authored-By: Claude Opus 4.6 --- .../rabbitmq.openstack.org_transporturls.yaml | 12 + apis/rabbitmq/v1beta1/conditions.go | 29 +- apis/rabbitmq/v1beta1/transporturl_helpers.go | 97 +++++ apis/rabbitmq/v1beta1/transporturl_types.go | 10 + .../rabbitmq.openstack.org_transporturls.yaml | 12 + config/rbac/role.yaml | 23 -- .../rabbitmq/transporturl_controller.go | 296 +++++++++++---- .../transporturl_controller_test.go | 359 ++++++++++++------ 8 files changed, 597 insertions(+), 241 deletions(-) create mode 100644 apis/rabbitmq/v1beta1/transporturl_helpers.go diff --git a/apis/bases/rabbitmq.openstack.org_transporturls.yaml b/apis/bases/rabbitmq.openstack.org_transporturls.yaml index 7593b89b..b6fa7333 100644 --- a/apis/bases/rabbitmq.openstack.org_transporturls.yaml +++ b/apis/bases/rabbitmq.openstack.org_transporturls.yaml @@ -132,6 +132,12 @@ spec: finalizer removal was deferred during a credential rotation, pending deployment verification. Only set while a rotation is in progress. type: string + previousSecretName: + description: |- + PreviousSecretName - name of the previous TransportURL secret retained + during credential rotation, pending consumer finalization. + Cleared when the old secret is cleaned up after all consumers release it. + type: string queueType: description: QueueType - the queue type from the associated RabbitMq instance @@ -153,6 +159,12 @@ spec: rabbitmqVhost: description: RabbitmqVhost - the actual vhost name used type: string + secretHash: + description: |- + SecretHash - hash of the current TransportURL secret content. + Consuming services compare this with the hash they computed to + confirm credential rotation is complete. + type: string secretName: description: SecretName - name of the secret containing the rabbitmq transport URL diff --git a/apis/rabbitmq/v1beta1/conditions.go b/apis/rabbitmq/v1beta1/conditions.go index d225ab5a..6de146c6 100644 --- a/apis/rabbitmq/v1beta1/conditions.go +++ b/apis/rabbitmq/v1beta1/conditions.go @@ -62,28 +62,17 @@ const ( // completion, so the user controller can safely auto-delete the CR on sight. RabbitMQUserOrphanedLabel = "rabbitmq.openstack.org/orphaned" - // EDPMServiceAnnotation overrides the automatic owner-based EDPM detection - // for a TransportURL. When set to "true", the controller always uses the - // two-phase NodeSet sync check. When "false", it releases the old user - // immediately. When unset, the controller infers EDPM status from the - // ownerReference Kind (Nova and NeutronAPI are EDPM; everything else is not). - EDPMServiceAnnotation = "rabbitmq.openstack.org/edpm-service" + // TransportSecretProtectionFinalizer prevents a transport URL secret from + // being deleted while it is referenced by a TransportURL (current or previous). + // Added atomically at secret creation; removed during cleanup. + TransportSecretProtectionFinalizer = "openstack.org/transport-secret-protection" + + // TransportSecretConsumerSuffix is the suffix appended to the operator name + // to form the consumer finalizer on transport URL secrets. + // Example: "openstack.org/keystone-transport-consumer" + TransportSecretConsumerSuffix = "-transport-consumer" ) -// edpmOwnerKinds lists the owner CR Kinds whose TransportURLs serve -// EDPM-deployed agents and therefore require NodeSet hash-sync gating -// during credential rotation. -var edpmOwnerKinds = map[string]bool{ - "Nova": true, - "NeutronAPI": true, -} - -// IsEDPMOwnerKind reports whether the given owner Kind serves -// EDPM-deployed agents that require NodeSet hash-sync gating. -func IsEDPMOwnerKind(kind string) bool { - return edpmOwnerKinds[kind] -} - // TransportURLFinalizerFor returns the per-consumer finalizer for a TransportURL. // If the name fits within Kubernetes' 63-char name segment limit, it is used directly // (preserving human readability and reverse mapping). For longer names, the suffix diff --git a/apis/rabbitmq/v1beta1/transporturl_helpers.go b/apis/rabbitmq/v1beta1/transporturl_helpers.go new file mode 100644 index 00000000..c5202fc4 --- /dev/null +++ b/apis/rabbitmq/v1beta1/transporturl_helpers.go @@ -0,0 +1,97 @@ +/* +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 v1beta1 + +import ( + "context" + "fmt" + "strings" + + "github.com/openstack-k8s-operators/lib-common/modules/common/helper" + "github.com/openstack-k8s-operators/lib-common/modules/common/object" + corev1 "k8s.io/api/core/v1" + k8s_errors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +// ManageTransportSecretFinalizer ensures consumerFinalizer is present on the +// transport secret identified by secretName. It never removes the finalizer +// from a previous secret — that is the consumer's responsibility after it has +// confirmed its deployment is running with the new credentials (typically via +// RemoveTransportSecretConsumerFinalizer). This ensures the TransportURL +// controller waits for all consumers before releasing the old RabbitMQ user. +func ManageTransportSecretFinalizer( + 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 transport secret %s: %w", secretName, err) + } + + return object.AddConsumerFinalizer(ctx, h, secret, consumerFinalizer) +} + +// RemoveTransportSecretConsumerFinalizer removes consumerFinalizer from the +// transport secret identified by secretName. It is a no-op when secretName +// is empty or the secret no longer exists. +func RemoveTransportSecretConsumerFinalizer( + 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 err + } + return object.RemoveConsumerFinalizer(ctx, h, secret, consumerFinalizer) +} + +// HasTransportConsumerFinalizer returns true if the secret has any finalizer +// matching the transport consumer pattern (openstack.org/*-transport-consumer). +func HasTransportConsumerFinalizer(secret *corev1.Secret) bool { + for _, f := range secret.Finalizers { + if strings.HasSuffix(f, TransportSecretConsumerSuffix) && + strings.HasPrefix(f, "openstack.org/") { + return true + } + } + return false +} + +// HasSpecificTransportConsumerFinalizer returns true if the secret has the +// given consumer finalizer. +func HasSpecificTransportConsumerFinalizer(secret *corev1.Secret, consumerFinalizer string) bool { + return controllerutil.ContainsFinalizer(secret, consumerFinalizer) +} diff --git a/apis/rabbitmq/v1beta1/transporturl_types.go b/apis/rabbitmq/v1beta1/transporturl_types.go index c4dd7ffe..b91eab7b 100644 --- a/apis/rabbitmq/v1beta1/transporturl_types.go +++ b/apis/rabbitmq/v1beta1/transporturl_types.go @@ -66,6 +66,16 @@ type TransportURLStatus struct { // Empty if using default cluster admin credentials (no dedicated RabbitMQUser CR) RabbitmqUserRef string `json:"rabbitmqUserRef,omitempty"` + // SecretHash - hash of the current TransportURL secret content. + // Consuming services compare this with the hash they computed to + // confirm credential rotation is complete. + SecretHash string `json:"secretHash,omitempty"` + + // PreviousSecretName - name of the previous TransportURL secret retained + // during credential rotation, pending consumer finalization. + // Cleared when the old secret is cleaned up after all consumers release it. + PreviousSecretName string `json:"previousSecretName,omitempty"` + // PreviousRabbitmqUserRef - the name of a previous RabbitMQUser CR whose // finalizer removal was deferred during a credential rotation, pending // deployment verification. Only set while a rotation is in progress. diff --git a/config/crd/bases/rabbitmq.openstack.org_transporturls.yaml b/config/crd/bases/rabbitmq.openstack.org_transporturls.yaml index 7593b89b..b6fa7333 100644 --- a/config/crd/bases/rabbitmq.openstack.org_transporturls.yaml +++ b/config/crd/bases/rabbitmq.openstack.org_transporturls.yaml @@ -132,6 +132,12 @@ spec: finalizer removal was deferred during a credential rotation, pending deployment verification. Only set while a rotation is in progress. type: string + previousSecretName: + description: |- + PreviousSecretName - name of the previous TransportURL secret retained + during credential rotation, pending consumer finalization. + Cleared when the old secret is cleaned up after all consumers release it. + type: string queueType: description: QueueType - the queue type from the associated RabbitMq instance @@ -153,6 +159,12 @@ spec: rabbitmqVhost: description: RabbitmqVhost - the actual vhost name used type: string + secretHash: + description: |- + SecretHash - hash of the current TransportURL secret content. + Consuming services compare this with the hash they computed to + confirm credential rotation is complete. + type: string secretName: description: SecretName - name of the secret containing the rabbitmq transport URL diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 1d5ee99e..b882ad11 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -73,29 +73,6 @@ rules: - patch - update - watch -- apiGroups: - - barbican.openstack.org - - cinder.openstack.org - - designate.openstack.org - - glance.openstack.org - - heat.openstack.org - - horizon.openstack.org - - ironic.openstack.org - - keystone.openstack.org - - manila.openstack.org - - neutron.openstack.org - - nova.openstack.org - - octavia.openstack.org - - ovn.openstack.org - - placement.openstack.org - - swift.openstack.org - - telemetry.openstack.org - - watcher.openstack.org - resources: - - '*' - verbs: - - get - - list - apiGroups: - config.openshift.io resources: diff --git a/internal/controller/rabbitmq/transporturl_controller.go b/internal/controller/rabbitmq/transporturl_controller.go index 3db74940..292d8476 100644 --- a/internal/controller/rabbitmq/transporturl_controller.go +++ b/internal/controller/rabbitmq/transporturl_controller.go @@ -29,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -86,7 +87,6 @@ func (r *TransportURLReconciler) GetLogger(ctx context.Context) logr.Logger { //+kubebuilder:rbac:groups=rabbitmq.openstack.org,resources=rabbitmqs,verbs=get;list;watch //+kubebuilder:rbac:groups=rabbitmq.openstack.org,resources=rabbitmqusers,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=rabbitmq.openstack.org,resources=rabbitmqvhosts,verbs=get;list;watch;create;update;patch;delete -//+kubebuilder:rbac:groups=cinder.openstack.org;glance.openstack.org;heat.openstack.org;horizon.openstack.org;ironic.openstack.org;keystone.openstack.org;manila.openstack.org;neutron.openstack.org;nova.openstack.org;octavia.openstack.org;ovn.openstack.org;placement.openstack.org;swift.openstack.org;telemetry.openstack.org;designate.openstack.org;barbican.openstack.org;watcher.openstack.org,resources=*,verbs=get;list //+kubebuilder:rbac:groups=core,resources=secrets,verbs=get;list;watch;create;update;patch;delete; //+kubebuilder:rbac:groups=dataplane.openstack.org,resources=openstackdataplanenodesets,verbs=get;list;watch @@ -517,29 +517,77 @@ func (r *TransportURLReconciler) reconcileNormal(ctx context.Context, instance * } } - // Create a new secret with the transport URL for this CR - secret := r.createTransportURLSecret(instance, finalUsername, finalPassword, hosts, string(port), vhostName, tlsEnabled, quorum) - _, op, err := oko_secret.CreateOrPatchSecret(ctx, helper, instance, secret) - if err != nil { - instance.Status.Conditions.Set(condition.FalseCondition( - rabbitmqv1.TransportURLReadyCondition, - condition.ErrorReason, - condition.SeverityWarning, - rabbitmqv1.TransportURLReadyErrorMessage, - err.Error())) - return ctrl.Result{}, err - } - if op != controllerutil.OperationResultNone { - instance.Status.Conditions.Set(condition.FalseCondition( - rabbitmqv1.TransportURLReadyCondition, - condition.RequestedReason, - condition.SeverityInfo, - rabbitmqv1.TransportURLReadyInitMessage)) - return ctrl.Result{RequeueAfter: time.Second * 5}, nil + // Create or update the transport URL secret. + // During credential rotation, create a new immutable secret so consumers + // can safely switch via the finalizer protocol. Otherwise patch the + // existing mutable secret. + if instance.Status.PreviousRabbitmqUserRef != "" && instance.Status.PreviousSecretName == "" { + // First reconcile after rotation detection: create new immutable secret + newSecret, hash, err := r.createImmutableTransportSecret(ctx, instance, finalUsername, finalPassword, hosts, string(port), vhostName, tlsEnabled, quorum) + if err != nil { + instance.Status.Conditions.Set(condition.FalseCondition( + rabbitmqv1.TransportURLReadyCondition, + condition.ErrorReason, + condition.SeverityWarning, + rabbitmqv1.TransportURLReadyErrorMessage, + err.Error())) + return ctrl.Result{}, err + } + instance.Status.PreviousSecretName = instance.Status.SecretName + instance.Status.SecretName = newSecret.Name + instance.Status.SecretHash = hash + Log.Info("Created immutable transport secret for rotation", + "secret", newSecret.Name, + "previousSecret", instance.Status.PreviousSecretName) + } else if instance.Status.PreviousSecretName == "" { + // Normal path: create/patch mutable secret + secret := r.createTransportURLSecret(instance, finalUsername, finalPassword, hosts, string(port), vhostName, tlsEnabled, quorum) + + // After credential rotation, SecretName points to an immutable secret + // with a content-hash suffix. If the transport URL data hasn't changed, + // keep the mutable secret up-to-date but don't flip SecretName back to + // the base name — consumers would interpret that as a second rotation. + skipSecretNameFlip := false + if instance.Status.SecretName != "" && instance.Status.SecretName != secret.Name { + newHash, err := oko_secret.Hash(secret) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to hash transport secret: %w", err) + } + if instance.Status.SecretHash == newHash { + skipSecretNameFlip = true + } + } + + hash, op, err := oko_secret.CreateOrPatchSecret(ctx, helper, instance, secret) + if err != nil { + instance.Status.Conditions.Set(condition.FalseCondition( + rabbitmqv1.TransportURLReadyCondition, + condition.ErrorReason, + condition.SeverityWarning, + rabbitmqv1.TransportURLReadyErrorMessage, + err.Error())) + return ctrl.Result{}, err + } + if op != controllerutil.OperationResultNone { + instance.Status.Conditions.Set(condition.FalseCondition( + rabbitmqv1.TransportURLReadyCondition, + condition.RequestedReason, + condition.SeverityInfo, + rabbitmqv1.TransportURLReadyInitMessage)) + return ctrl.Result{RequeueAfter: time.Second * 5}, nil + } + if !skipSecretNameFlip { + if instance.Status.SecretName != "" && instance.Status.SecretName != secret.Name { + instance.Status.PreviousSecretName = instance.Status.SecretName + } + instance.Status.SecretName = secret.Name + instance.Status.SecretHash = hash + } } + // else: rotation in progress, immutable secret already created — keep + // current status values and proceed to tryReleasePendingUser. // Update the CR status with actual values used - instance.Status.SecretName = secret.Name instance.Status.RabbitmqClusterName = instance.Spec.RabbitmqClusterName instance.Status.RabbitmqUsername = finalUsername instance.Status.RabbitmqVhost = vhostName @@ -562,10 +610,34 @@ func (r *TransportURLReconciler) reconcileNormal(ctx context.Context, instance * return ctrl.Result{}, err } if !released { - Log.Info("Pending user release waiting for deployment", - "pendingUser", instance.Status.PreviousRabbitmqUserRef) + Log.Info("Credential rotation in progress, old user retained until full NodeSet deployment completes", + "oldUser", instance.Status.PreviousRabbitmqUserRef, + "newUser", instance.Status.RabbitmqUserRef) + return ctrl.Result{RequeueAfter: PendingReleaseCheckInterval}, nil + } + } + + // Clean up orphaned previous transport secrets that are no longer part of + // an active rotation (e.g., immutable secrets left behind after switching + // back to the standard mutable secret). + if instance.Status.PreviousSecretName != "" && instance.Status.PreviousRabbitmqUserRef == "" { + oldSecret := &corev1.Secret{} + err := r.Get(ctx, types.NamespacedName{ + Name: instance.Status.PreviousSecretName, + Namespace: instance.Namespace, + }, oldSecret) + if err != nil && !k8s_errors.IsNotFound(err) { + return ctrl.Result{}, err + } + if err == nil && rabbitmqv1.HasTransportConsumerFinalizer(oldSecret) { + Log.Info("Waiting for consumer to release previous transport secret", + "secret", instance.Status.PreviousSecretName) return ctrl.Result{RequeueAfter: PendingReleaseCheckInterval}, nil } + if err := r.cleanupOldTransportSecret(ctx, instance); err != nil { + return ctrl.Result{}, err + } + instance.Status.PreviousSecretName = "" } // Only TransportURLs with names >61 chars need a periodic requeue because @@ -627,6 +699,85 @@ func (r *TransportURLReconciler) createTransportURLSecret( } } +// createImmutableTransportSecret creates a new immutable secret for credential +// rotation. The secret name includes a content hash suffix so it is unique per +// credential set. A protection finalizer is added atomically to prevent +// premature deletion while the TransportURL still references it. +func (r *TransportURLReconciler) createImmutableTransportSecret( + ctx context.Context, + instance *rabbitmqv1.TransportURL, + username string, + password string, + hosts []string, + port string, + vhost string, + tlsEnabled bool, + quorum bool, +) (*corev1.Secret, string, error) { + template := r.createTransportURLSecret(instance, username, password, hosts, port, vhost, tlsEnabled, quorum) + + contentHash, err := oko_secret.Hash(template) + if err != nil { + return nil, "", fmt.Errorf("failed to hash transport secret data: %w", err) + } + + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("rabbitmq-transport-url-%s-%s", instance.Name, contentHash[:8]), + Namespace: instance.Namespace, + Finalizers: []string{rabbitmqv1.TransportSecretProtectionFinalizer}, + }, + Data: template.Data, + Immutable: ptr.To(true), + } + if err := controllerutil.SetControllerReference(instance, secret, r.Scheme); err != nil { + return nil, "", fmt.Errorf("failed to set controller reference on transport secret: %w", err) + } + + if err := r.Create(ctx, secret); err != nil { + if !k8s_errors.IsAlreadyExists(err) { + return nil, "", fmt.Errorf("failed to create immutable transport secret %s: %w", secret.Name, err) + } + } + + return secret, contentHash, nil +} + +// cleanupOldTransportSecret removes the protection finalizer from the previous +// transport secret and deletes it. No-op if PreviousSecretName is empty or +// the secret no longer exists. +func (r *TransportURLReconciler) cleanupOldTransportSecret(ctx context.Context, instance *rabbitmqv1.TransportURL) error { + secretName := instance.Status.PreviousSecretName + if secretName == "" { + return nil + } + Log := r.GetLogger(ctx) + + secret := &corev1.Secret{} + if err := r.Get(ctx, types.NamespacedName{Name: secretName, Namespace: instance.Namespace}, secret); err != nil { + if k8s_errors.IsNotFound(err) { + return nil + } + return fmt.Errorf("failed to get old transport secret %s: %w", secretName, err) + } + + if controllerutil.RemoveFinalizer(secret, rabbitmqv1.TransportSecretProtectionFinalizer) { + if err := r.Update(ctx, secret); err != nil { + if k8s_errors.IsNotFound(err) { + return nil + } + return fmt.Errorf("failed to remove protection finalizer from %s: %w", secretName, err) + } + } + + Log.Info("Deleting old transport secret", "secret", secretName) + if err := r.Delete(ctx, secret); err != nil && !k8s_errors.IsNotFound(err) { + return fmt.Errorf("failed to delete old transport secret %s: %w", secretName, err) + } + + return nil +} + // fields to index to reconcile when change const ( rabbitmqClusterNameField = ".spec.rabbitmqClusterName" @@ -644,7 +795,11 @@ func (r *TransportURLReconciler) reconcileDelete(ctx context.Context, instance * if err := r.releaseUserFinalizer(ctx, instance.Status.PreviousRabbitmqUserRef, instance.Namespace, perConsumerFinalizer); err != nil { return ctrl.Result{}, err } + if err := r.cleanupOldTransportSecret(ctx, instance); err != nil { + return ctrl.Result{}, err + } instance.Status.PreviousRabbitmqUserRef = "" + instance.Status.PreviousSecretName = "" instance.Status.NodeSetSynced = "" } @@ -876,75 +1031,57 @@ func (r *TransportURLReconciler) deleteOrphanedCR(ctx context.Context, obj clien return nil } -// tryReleasePendingUser checks whether it is safe to release the pending old user -// from a credential rotation. Uses a two-phase state machine: +// tryReleasePendingUser checks whether it is safe to release the pending old +// user from a credential rotation. // -// Phase 1 (NodeSetSynced=""): wait for AreSecretHashesInSync to return false, -// indicating service operators have regenerated config secrets with new -// credentials (creating a hash mismatch against NodeSet secretHashes). -// -// Phase 2 (NodeSetSynced="false"): wait for AreSecretHashesInSync to return -// true, indicating a NodeSet deployment has completed with the new config -// secrets. +// The flow is unified for all services: +// 1. Wait for the consumer finalizer on the old transport secret to be +// removed, indicating the consuming service has completed its rollout +// with the new credentials. +// 2. If NodeSets exist, check AreSecretHashesInSync. If hashes are out of +// sync, the regenerated config secret is tracked by the dataplane and a +// deployment is needed before the old user can be released. If hashes +// are in sync, the secret is either not tracked by the dataplane or the +// deployment has already completed. // // Returns (true, nil) if the user was released. func (r *TransportURLReconciler) tryReleasePendingUser(ctx context.Context, instance *rabbitmqv1.TransportURL) (bool, error) { Log := r.GetLogger(ctx) pendingRef := instance.Status.PreviousRabbitmqUserRef - // Determine whether this TransportURL serves an EDPM-deployed service. - // Explicit annotation wins; otherwise infer from the ownerReference Kind. - edpmService := false - if v, ok := instance.Annotations[rabbitmqv1.EDPMServiceAnnotation]; ok { - edpmService = v == "true" - } else { - for _, ref := range instance.OwnerReferences { - if rabbitmqv1.IsEDPMOwnerKind(ref.Kind) { - edpmService = true - break - } + if instance.Status.PreviousSecretName != "" { + oldSecret := &corev1.Secret{} + err := r.Get(ctx, types.NamespacedName{ + Name: instance.Status.PreviousSecretName, + Namespace: instance.Namespace, + }, oldSecret) + if err != nil && !k8s_errors.IsNotFound(err) { + return false, err + } + if err == nil && rabbitmqv1.HasTransportConsumerFinalizer(oldSecret) { + Log.Info("Waiting for consumer to release old transport secret", + "secret", instance.Status.PreviousSecretName, + "pendingUser", pendingRef) + return false, nil } - } - if !edpmService { - Log.Info("Non-EDPM service, releasing old user immediately", - "pendingUser", pendingRef) - return r.releasePendingUser(ctx, instance) } - // No NodeSets with secretHashes -> no computes affected, release immediately haveNS, err := edpm.HaveNodeSets(ctx, r.Client, instance.Namespace) if err != nil { Log.Error(err, "Failed to check for nodesets") return false, nil } - if !haveNS { - return r.releasePendingUser(ctx, instance) - } - - inSync, info, err := edpm.AreSecretHashesInSync(ctx, r.Client, instance.Namespace) - if err != nil { - Log.Error(err, "Failed to check nodeset sync status for pending release") - return false, nil - } - - if instance.Status.NodeSetSynced != "false" { - // Phase 1: waiting for config secrets to go out of sync + if haveNS { + inSync, info, err := edpm.AreSecretHashesInSync(ctx, r.Client, instance.Namespace) + if err != nil { + Log.Error(err, "Failed to check nodeset sync status for pending release") + return false, nil + } if !inSync { - instance.Status.NodeSetSynced = "false" - Log.Info("Config regeneration detected, waiting for deployment to complete", + Log.Info("Waiting for dataplane deployment to complete", "pendingUser", pendingRef, "info", info) - } else { - Log.Info("Waiting for service operators to regenerate config", - "pendingUser", pendingRef) + return false, nil } - return false, nil - } - - // Phase 2: waiting for deployment to complete - if !inSync { - Log.Info("Deployment in progress, waiting for completion", - "pendingUser", pendingRef, "info", info) - return false, nil } return r.releasePendingUser(ctx, instance) @@ -996,9 +1133,9 @@ func (r *TransportURLReconciler) releaseUserFinalizer(ctx context.Context, userR return nil } -// releasePendingUser removes the consumer finalizer from the pending user and -// marks it as orphaned if no consumer finalizers remain. -// Clears the pending release fields from status. +// releasePendingUser removes the consumer finalizer from the pending user, +// marks it as orphaned if no consumer finalizers remain, and cleans up the +// old transport secret. Clears the pending release fields from status. func (r *TransportURLReconciler) releasePendingUser(ctx context.Context, instance *rabbitmqv1.TransportURL) (bool, error) { Log := r.GetLogger(ctx) pendingRef := instance.Status.PreviousRabbitmqUserRef @@ -1010,7 +1147,12 @@ func (r *TransportURLReconciler) releasePendingUser(ctx context.Context, instanc return false, err } + if err := r.cleanupOldTransportSecret(ctx, instance); err != nil { + return false, err + } + instance.Status.PreviousRabbitmqUserRef = "" + instance.Status.PreviousSecretName = "" instance.Status.NodeSetSynced = "" return true, nil } diff --git a/test/functional/transporturl_controller_test.go b/test/functional/transporturl_controller_test.go index a3de764e..911d794f 100644 --- a/test/functional/transporturl_controller_test.go +++ b/test/functional/transporturl_controller_test.go @@ -1208,7 +1208,7 @@ var _ = Describe("TransportURL controller", func() { rabbitmq := CreateRabbitMQ(rabbitmqName, spec) DeferCleanup(th.DeleteInstance, rabbitmq) - // Create TransportURL with a Nova ownerReference (EDPM service) + // Create TransportURL with a Nova ownerReference tu := &rabbitmqv1.TransportURL{ ObjectMeta: metav1.ObjectMeta{ Name: turlName.Name, @@ -1262,8 +1262,16 @@ var _ = Describe("TransportURL controller", func() { g.Expect(k8sClient.Get(ctx, transportURLSecretName, s)).Should(Succeed()) }, timeout, interval).Should(Succeed()) - // Compute hash of the current transport URL secret and set it on the NodeSet. - // This simulates a NodeSet that has deployed with the current credentials. + // Add a consumer finalizer to the transport secret, simulating + // the service operator adopting the secret. + Eventually(func(g Gomega) { + secret := &corev1.Secret{} + g.Expect(k8sClient.Get(ctx, transportURLSecretName, secret)).Should(Succeed()) + controllerutil.AddFinalizer(secret, "openstack.org/nova-transport-consumer") + g.Expect(k8sClient.Update(ctx, secret)).Should(Succeed()) + }, timeout, interval).Should(Succeed()) + + // Set NodeSet secretHashes with the current transport URL secret. initialHash := GetSecretHash(transportURLSecretName) SetNodeSetSecretHashes(nodesetName, map[string]string{ transportURLSecretName.Name: initialHash, @@ -1289,34 +1297,55 @@ var _ = Describe("TransportURL controller", func() { SimulateRabbitMQUserReady(userBCRName, "/") - // The transport URL secret now has new credentials (user-b), so its hash - // differs from what the NodeSet has. AreSecretHashesInSync returns false. - // The controller should detect this (Phase 1 → Phase 2) but NOT release - // the old user yet. - // - // Verify old user-a still exists (not released during Phase 2) - Consistently(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - g.Expect(k8sClient.Get(ctx, userACRName, user)).Should(Succeed()) - g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizerFor(turlName.Name))).To(BeTrue(), - "Old user should still have per-consumer finalizer during Phase 2") - }, 5*time.Second, interval).Should(Succeed()) - - // Verify the TransportURL has PreviousRabbitmqUserRef set + // The consumer finalizer on the old secret gates the release. + // Wait for rotation state to be set up. + var newSecretName string Eventually(func(g Gomega) { tr := th.GetTransportURL(turlName) g.Expect(tr.Status.PreviousRabbitmqUserRef).ToNot(BeEmpty(), "PreviousRabbitmqUserRef should be set during rotation") + g.Expect(tr.Status.PreviousSecretName).To(Equal(transportURLSecretName.Name), + "PreviousSecretName should track the old mutable secret") + g.Expect(tr.Status.SecretName).ToNot(Equal(transportURLSecretName.Name), + "SecretName should point to the new immutable secret") + newSecretName = tr.Status.SecretName + }, timeout, interval).Should(Succeed()) + + // Set NodeSet with stale hash for new secret to simulate a + // config secret regenerated by the service operator but not + // yet deployed to compute nodes. + SetNodeSetSecretHashes(nodesetName, map[string]string{ + newSecretName: "stale-before-deploy", + }) + + // Remove consumer finalizer — simulates service operator + // completing its rollout with the new credentials. + Eventually(func(g Gomega) { + secret := &corev1.Secret{} + g.Expect(k8sClient.Get(ctx, transportURLSecretName, secret)).Should(Succeed()) + controllerutil.RemoveFinalizer(secret, "openstack.org/nova-transport-consumer") + g.Expect(k8sClient.Update(ctx, secret)).Should(Succeed()) }, timeout, interval).Should(Succeed()) + // Old user should still exist — NodeSet hashes are stale so + // the controller waits for a dataplane deployment. + Consistently(func(g Gomega) { + user := &rabbitmqv1.RabbitMQUser{} + g.Expect(k8sClient.Get(ctx, userACRName, user)).Should(Succeed()) + g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizerFor(turlName.Name))).To(BeTrue(), + "Old user should still have per-consumer finalizer while waiting for deployment") + }, 5*time.Second, interval).Should(Succeed()) + // --- Simulate NodeSet deployment completing --- - // Update the NodeSet's secretHashes to match the new (rotated) secret hash - newHash := GetSecretHash(transportURLSecretName) + newHash := GetSecretHash(types.NamespacedName{ + Name: newSecretName, + Namespace: namespace, + }) SetNodeSetSecretHashes(nodesetName, map[string]string{ - transportURLSecretName.Name: newHash, + newSecretName: newHash, }) - // Now AreSecretHashesInSync returns true → Phase 2 completes → old user released + // Hashes now in sync → old user released Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} err := k8sClient.Get(ctx, userACRName, user) @@ -1324,7 +1353,7 @@ var _ = Describe("TransportURL controller", func() { "Old user should be deleted after NodeSet deployment completes") }, timeout, interval).Should(Succeed()) - // Verify PreviousRabbitmqUserRef is cleared + // Verify cleanup Eventually(func(g Gomega) { tr := th.GetTransportURL(turlName) g.Expect(tr.Status.PreviousRabbitmqUserRef).To(BeEmpty(), @@ -1378,7 +1407,7 @@ var _ = Describe("TransportURL controller", func() { rabbitmq := CreateRabbitMQ(rabbitmqName, spec) DeferCleanup(th.DeleteInstance, rabbitmq) - // Create TransportURL with a non-EDPM ownerReference (simulating Cinder) + // Create TransportURL with a Cinder ownerReference tu := &rabbitmqv1.TransportURL{ ObjectMeta: metav1.ObjectMeta{ Name: turlName.Name, @@ -1452,8 +1481,8 @@ var _ = Describe("TransportURL controller", func() { SimulateRabbitMQUserReady(userBCRName, "/") - // Old user should be released immediately — owner is Cinder - // (not in EDPMOwnerKinds), so no NodeSet sync needed + // Old user should be released immediately — NodeSet hashes + // are in sync (the tracked secret hasn't changed) Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} err := k8sClient.Get(ctx, userACRName, user) @@ -1469,218 +1498,306 @@ var _ = Describe("TransportURL controller", func() { }) }) - When("annotation overrides owner-based EDPM detection", func() { + When("username is changed for standalone TransportURL without owner", func() { var rabbitmqName types.NamespacedName - var turlName types.NamespacedName - var nodesetName types.NamespacedName + var transportURLName types.NamespacedName BeforeEach(func() { rabbitmqName = types.NamespacedName{ - Name: "rabbitmq-anno-override", - Namespace: namespace, - } - turlName = types.NamespacedName{ - Name: "turl-anno-override", + Name: "rabbitmq-no-owner", Namespace: namespace, } - nodesetName = types.NamespacedName{ - Name: "compute-anno-override", + transportURLName = types.NamespacedName{ + Name: "transporturl-no-owner", Namespace: namespace, } SetupMockRabbitMQAPI() DeferCleanup(StopMockRabbitMQAPI) + // Create RabbitMQCluster first CreateRabbitMQCluster(rabbitmqName, GetDefaultRabbitMQClusterSpec(false)) DeferCleanup(DeleteRabbitMQCluster, rabbitmqName) + // Create RabbitMq CR spec := GetDefaultRabbitMQSpec() rabbitmq := CreateRabbitMQ(rabbitmqName, spec) DeferCleanup(th.DeleteInstance, rabbitmq) - // Nova-owned TransportURL with edpm-service=false annotation override + // Create TransportURL WITHOUT owner reference (standalone) tu := &rabbitmqv1.TransportURL{ ObjectMeta: metav1.ObjectMeta{ - Name: turlName.Name, - Namespace: turlName.Namespace, - Annotations: map[string]string{ - rabbitmqv1.EDPMServiceAnnotation: "false", - }, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: "nova.openstack.org/v1beta1", - Kind: "Nova", - Name: "nova", - UID: "fake-nova-uid-2", - }, - }, + Name: transportURLName.Name, + Namespace: transportURLName.Namespace, }, Spec: rabbitmqv1.TransportURLSpec{ RabbitmqClusterName: rabbitmqName.Name, - Username: "anno-user-a", + Username: "noowner-olduser", }, } Expect(k8sClient.Create(ctx, tu)).Should(Succeed()) DeferCleanup(th.DeleteInstance, tu) - - CreateNodeSet(nodesetName) - DeferCleanup(DeleteNodeSet, nodesetName) }) - It("should release immediately when annotation says false despite Nova owner", func() { + It("should proceed with cleanup immediately when there is no owner", func() { SimulateRabbitMQClusterReady(rabbitmqName) - userACRName := types.NamespacedName{ - Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "anno-user-a"), + // Wait for initial RabbitMQUser to be created + oldUserCRName := types.NamespacedName{ + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "noowner-olduser"), Namespace: namespace, } Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} - g.Expect(k8sClient.Get(ctx, userACRName, user)).Should(Succeed()) + g.Expect(k8sClient.Get(ctx, oldUserCRName, user)).Should(Succeed()) }, timeout, interval).Should(Succeed()) - SimulateRabbitMQUserReady(userACRName, "/") + // Simulate user being ready + SimulateRabbitMQUserReady(oldUserCRName, "/") - transportURLSecretName := types.NamespacedName{ - Name: "rabbitmq-transport-url-" + turlName.Name, - Namespace: namespace, - } + // Verify TransportURL has NO owner references Eventually(func(g Gomega) { - tr := th.GetTransportURL(turlName) - g.Expect(tr.Status.RabbitmqUsername).To(Equal("anno-user-a")) - s := &corev1.Secret{} - g.Expect(k8sClient.Get(ctx, transportURLSecretName, s)).Should(Succeed()) + tr := th.GetTransportURL(transportURLName) + g.Expect(tr.GetOwnerReferences()).To(BeEmpty(), "TransportURL should have no owner references") }, timeout, interval).Should(Succeed()) - initialHash := GetSecretHash(transportURLSecretName) - SetNodeSetSecretHashes(nodesetName, map[string]string{ - transportURLSecretName.Name: initialHash, - }) - - // Trigger credential rotation + // Change username Eventually(func(g Gomega) { - tr := th.GetTransportURL(turlName) - tr.Spec.Username = "anno-user-b" + tr := th.GetTransportURL(transportURLName) + tr.Spec.Username = "noowner-newuser" g.Expect(k8sClient.Update(ctx, tr)).Should(Succeed()) }, timeout, interval).Should(Succeed()) - userBCRName := types.NamespacedName{ - Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "anno-user-b"), + // Wait for new RabbitMQUser to be created + newUserCRName := types.NamespacedName{ + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "noowner-newuser"), Namespace: namespace, } Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} - g.Expect(k8sClient.Get(ctx, userBCRName, user)).Should(Succeed()) + g.Expect(k8sClient.Get(ctx, newUserCRName, user)).Should(Succeed()) }, timeout, interval).Should(Succeed()) - SimulateRabbitMQUserReady(userBCRName, "/") + // Simulate new user being ready + SimulateRabbitMQUserReady(newUserCRName, "/") - // Annotation override: despite Nova owner, edpm-service=false - // should cause immediate release + // With no NodeSets, the user controller verifies credentials are safe + // to delete and removes the orphaned user CR Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} - err := k8sClient.Get(ctx, userACRName, user) + err := k8sClient.Get(ctx, oldUserCRName, user) g.Expect(k8s_errors.IsNotFound(err)).To(BeTrue(), - "Annotation override should bypass owner-based EDPM detection") - }, timeout, interval).Should(Succeed()) - - Eventually(func(g Gomega) { - tr := th.GetTransportURL(turlName) - g.Expect(tr.Status.PreviousRabbitmqUserRef).To(BeEmpty()) + "Old user CR should be auto-deleted when no NodeSets exist") }, timeout, interval).Should(Succeed()) }) }) - When("username is changed for standalone TransportURL without owner", func() { + When("credential rotation creates immutable secret and respects consumer finalizers", func() { var rabbitmqName types.NamespacedName - var transportURLName types.NamespacedName + var turlName types.NamespacedName BeforeEach(func() { rabbitmqName = types.NamespacedName{ - Name: "rabbitmq-no-owner", + Name: "rabbitmq-immut", Namespace: namespace, } - transportURLName = types.NamespacedName{ - Name: "transporturl-no-owner", + turlName = types.NamespacedName{ + Name: "turl-immut", Namespace: namespace, } SetupMockRabbitMQAPI() DeferCleanup(StopMockRabbitMQAPI) - // Create RabbitMQCluster first CreateRabbitMQCluster(rabbitmqName, GetDefaultRabbitMQClusterSpec(false)) DeferCleanup(DeleteRabbitMQCluster, rabbitmqName) - // Create RabbitMq CR spec := GetDefaultRabbitMQSpec() rabbitmq := CreateRabbitMQ(rabbitmqName, spec) DeferCleanup(th.DeleteInstance, rabbitmq) - // Create TransportURL WITHOUT owner reference (standalone) tu := &rabbitmqv1.TransportURL{ ObjectMeta: metav1.ObjectMeta{ - Name: transportURLName.Name, - Namespace: transportURLName.Namespace, + Name: turlName.Name, + Namespace: turlName.Namespace, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "heat.openstack.org/v1beta1", + Kind: "Heat", + Name: "heat", + UID: "fake-heat-uid", + }, + }, }, Spec: rabbitmqv1.TransportURLSpec{ RabbitmqClusterName: rabbitmqName.Name, - Username: "noowner-olduser", + Username: "immut-user-a", }, } Expect(k8sClient.Create(ctx, tu)).Should(Succeed()) DeferCleanup(th.DeleteInstance, tu) }) - It("should proceed with cleanup immediately when there is no owner", func() { + It("should create immutable secret on rotation and wait for consumer finalizer", func() { SimulateRabbitMQClusterReady(rabbitmqName) - // Wait for initial RabbitMQUser to be created - oldUserCRName := types.NamespacedName{ - Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "noowner-olduser"), + userACRName := types.NamespacedName{ + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "immut-user-a"), Namespace: namespace, } Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} - g.Expect(k8sClient.Get(ctx, oldUserCRName, user)).Should(Succeed()) + g.Expect(k8sClient.Get(ctx, userACRName, user)).Should(Succeed()) }, timeout, interval).Should(Succeed()) - // Simulate user being ready - SimulateRabbitMQUserReady(oldUserCRName, "/") + SimulateRabbitMQUserReady(userACRName, "/") - // Verify TransportURL has NO owner references + // Wait for TransportURL to be ready with mutable secret + mutableSecretName := types.NamespacedName{ + Name: "rabbitmq-transport-url-" + turlName.Name, + Namespace: namespace, + } Eventually(func(g Gomega) { - tr := th.GetTransportURL(transportURLName) - g.Expect(tr.GetOwnerReferences()).To(BeEmpty(), "TransportURL should have no owner references") + tr := th.GetTransportURL(turlName) + g.Expect(tr.Status.RabbitmqUsername).To(Equal("immut-user-a")) + g.Expect(tr.Status.SecretName).To(Equal(mutableSecretName.Name)) + g.Expect(tr.Status.SecretHash).ToNot(BeEmpty()) + s := &corev1.Secret{} + g.Expect(k8sClient.Get(ctx, mutableSecretName, s)).Should(Succeed()) }, timeout, interval).Should(Succeed()) - // Change username + // Simulate consumer adding a finalizer to the mutable secret + consumerFinalizer := "openstack.org/heat-transport-consumer" Eventually(func(g Gomega) { - tr := th.GetTransportURL(transportURLName) - tr.Spec.Username = "noowner-newuser" + s := &corev1.Secret{} + g.Expect(k8sClient.Get(ctx, mutableSecretName, s)).Should(Succeed()) + controllerutil.AddFinalizer(s, consumerFinalizer) + g.Expect(k8sClient.Update(ctx, s)).Should(Succeed()) + }, timeout, interval).Should(Succeed()) + + // --- Trigger credential rotation --- + Eventually(func(g Gomega) { + tr := th.GetTransportURL(turlName) + tr.Spec.Username = "immut-user-b" g.Expect(k8sClient.Update(ctx, tr)).Should(Succeed()) }, timeout, interval).Should(Succeed()) - // Wait for new RabbitMQUser to be created - newUserCRName := types.NamespacedName{ - Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "noowner-newuser"), + userBCRName := types.NamespacedName{ + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "immut-user-b"), Namespace: namespace, } Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} - g.Expect(k8sClient.Get(ctx, newUserCRName, user)).Should(Succeed()) + g.Expect(k8sClient.Get(ctx, userBCRName, user)).Should(Succeed()) }, timeout, interval).Should(Succeed()) - // Simulate new user being ready - SimulateRabbitMQUserReady(newUserCRName, "/") + SimulateRabbitMQUserReady(userBCRName, "/") - // With no NodeSets, the user controller verifies credentials are safe - // to delete and removes the orphaned user CR + // Verify rotation created an immutable secret Eventually(func(g Gomega) { + tr := th.GetTransportURL(turlName) + g.Expect(tr.Status.SecretName).ToNot(Equal(mutableSecretName.Name), + "SecretName should change to the new immutable secret") + g.Expect(tr.Status.PreviousSecretName).To(Equal(mutableSecretName.Name), + "PreviousSecretName should track the old mutable secret") + g.Expect(tr.Status.SecretHash).ToNot(BeEmpty()) + + // New secret should exist, be immutable, and have protection finalizer + newSecret := &corev1.Secret{} + g.Expect(k8sClient.Get(ctx, types.NamespacedName{ + Name: tr.Status.SecretName, + Namespace: namespace, + }, newSecret)).Should(Succeed()) + g.Expect(newSecret.Immutable).ToNot(BeNil()) + g.Expect(*newSecret.Immutable).To(BeTrue()) + g.Expect(controllerutil.ContainsFinalizer(newSecret, + rabbitmqv1.TransportSecretProtectionFinalizer)).To(BeTrue()) + }, timeout, interval).Should(Succeed()) + + // Old user should NOT be released while consumer finalizer is present + Consistently(func(g Gomega) { + tr := th.GetTransportURL(turlName) + g.Expect(tr.Status.PreviousRabbitmqUserRef).ToNot(BeEmpty(), + "Old user should not be released while consumer holds finalizer") user := &rabbitmqv1.RabbitMQUser{} - err := k8sClient.Get(ctx, oldUserCRName, user) + g.Expect(k8sClient.Get(ctx, userACRName, user)).Should(Succeed()) + }, "3s", interval).Should(Succeed()) + + // Simulate consumer removing finalizer from old secret (rollout complete) + Eventually(func(g Gomega) { + s := &corev1.Secret{} + g.Expect(k8sClient.Get(ctx, mutableSecretName, s)).Should(Succeed()) + controllerutil.RemoveFinalizer(s, consumerFinalizer) + g.Expect(k8sClient.Update(ctx, s)).Should(Succeed()) + }, timeout, interval).Should(Succeed()) + + // Old user should now be released + Eventually(func(g Gomega) { + user := &rabbitmqv1.RabbitMQUser{} + err := k8sClient.Get(ctx, userACRName, user) g.Expect(k8s_errors.IsNotFound(err)).To(BeTrue(), - "Old user CR should be auto-deleted when no NodeSets exist") + "Old user should be deleted after consumer releases finalizer") + }, timeout, interval).Should(Succeed()) + + // PreviousSecretName and PreviousRabbitmqUserRef should be cleared + Eventually(func(g Gomega) { + tr := th.GetTransportURL(turlName) + g.Expect(tr.Status.PreviousRabbitmqUserRef).To(BeEmpty()) + g.Expect(tr.Status.PreviousSecretName).To(BeEmpty()) + }, timeout, interval).Should(Succeed()) + }) + + It("should release immediately when no consumer finalizer exists (backward compat)", func() { + SimulateRabbitMQClusterReady(rabbitmqName) + + userACRName := types.NamespacedName{ + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "immut-user-a"), + Namespace: namespace, + } + Eventually(func(g Gomega) { + user := &rabbitmqv1.RabbitMQUser{} + g.Expect(k8sClient.Get(ctx, userACRName, user)).Should(Succeed()) + }, timeout, interval).Should(Succeed()) + + SimulateRabbitMQUserReady(userACRName, "/") + + Eventually(func(g Gomega) { + tr := th.GetTransportURL(turlName) + g.Expect(tr.Status.RabbitmqUsername).To(Equal("immut-user-a")) + }, timeout, interval).Should(Succeed()) + + // Do NOT add any consumer finalizer (simulating old operator) + + // Trigger credential rotation + Eventually(func(g Gomega) { + tr := th.GetTransportURL(turlName) + tr.Spec.Username = "immut-user-b" + g.Expect(k8sClient.Update(ctx, tr)).Should(Succeed()) + }, timeout, interval).Should(Succeed()) + + userBCRName := types.NamespacedName{ + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "immut-user-b"), + Namespace: namespace, + } + Eventually(func(g Gomega) { + user := &rabbitmqv1.RabbitMQUser{} + g.Expect(k8sClient.Get(ctx, userBCRName, user)).Should(Succeed()) + }, timeout, interval).Should(Succeed()) + + SimulateRabbitMQUserReady(userBCRName, "/") + + // No consumer finalizer → old user released immediately + Eventually(func(g Gomega) { + user := &rabbitmqv1.RabbitMQUser{} + err := k8sClient.Get(ctx, userACRName, user) + g.Expect(k8s_errors.IsNotFound(err)).To(BeTrue(), + "Old user should be released immediately without consumer finalizer") + }, timeout, interval).Should(Succeed()) + + Eventually(func(g Gomega) { + tr := th.GetTransportURL(turlName) + g.Expect(tr.Status.PreviousRabbitmqUserRef).To(BeEmpty()) + g.Expect(tr.Status.PreviousSecretName).To(BeEmpty()) }, timeout, interval).Should(Succeed()) }) }) From 97f116366c2254ef58a7bff470285189fa538d96 Mon Sep 17 00:00:00 2001 From: Luca Miccini Date: Sat, 20 Jun 2026 09:21:23 +0200 Subject: [PATCH 2/3] Remove consumer-facing transport secret helpers superseded by lib-common ManageTransportSecretFinalizer and RemoveTransportSecretConsumerFinalizer are now available as generic object.ManageSecretConsumerFinalizer and object.RemoveSecretConsumerFinalizer in lib-common. Consumer operators should migrate to the lib-common versions. Keep HasTransportConsumerFinalizer and HasSpecificTransportConsumerFinalizer which are provider-side helpers used by the TransportURL controller. Co-Authored-By: Claude Opus 4.6 (1M context) --- apis/rabbitmq/v1beta1/transporturl_helpers.go | 57 ------------------- 1 file changed, 57 deletions(-) diff --git a/apis/rabbitmq/v1beta1/transporturl_helpers.go b/apis/rabbitmq/v1beta1/transporturl_helpers.go index c5202fc4..c4b19139 100644 --- a/apis/rabbitmq/v1beta1/transporturl_helpers.go +++ b/apis/rabbitmq/v1beta1/transporturl_helpers.go @@ -15,69 +15,12 @@ limitations under the License. package v1beta1 import ( - "context" - "fmt" "strings" - "github.com/openstack-k8s-operators/lib-common/modules/common/helper" - "github.com/openstack-k8s-operators/lib-common/modules/common/object" corev1 "k8s.io/api/core/v1" - k8s_errors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) -// ManageTransportSecretFinalizer ensures consumerFinalizer is present on the -// transport secret identified by secretName. It never removes the finalizer -// from a previous secret — that is the consumer's responsibility after it has -// confirmed its deployment is running with the new credentials (typically via -// RemoveTransportSecretConsumerFinalizer). This ensures the TransportURL -// controller waits for all consumers before releasing the old RabbitMQ user. -func ManageTransportSecretFinalizer( - 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 transport secret %s: %w", secretName, err) - } - - return object.AddConsumerFinalizer(ctx, h, secret, consumerFinalizer) -} - -// RemoveTransportSecretConsumerFinalizer removes consumerFinalizer from the -// transport secret identified by secretName. It is a no-op when secretName -// is empty or the secret no longer exists. -func RemoveTransportSecretConsumerFinalizer( - 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 err - } - return object.RemoveConsumerFinalizer(ctx, h, secret, consumerFinalizer) -} - // HasTransportConsumerFinalizer returns true if the secret has any finalizer // matching the transport consumer pattern (openstack.org/*-transport-consumer). func HasTransportConsumerFinalizer(secret *corev1.Secret) bool { From aca54b995462c1d7dd8d29a511c23027c3afd58f Mon Sep 17 00:00:00 2001 From: Luca Miccini Date: Sat, 20 Jun 2026 09:24:33 +0200 Subject: [PATCH 3/3] Inline HasTransportConsumerFinalizer into TransportURL controller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the transport consumer finalizer check from the exported API helper into a private function in the controller — the only consumer. Delete transporturl_helpers.go entirely since all exported helpers are now either in lib-common (ManageSecretConsumerFinalizer, RemoveSecretConsumerFinalizer) or inlined here. Co-Authored-By: Claude Opus 4.6 (1M context) --- apis/rabbitmq/v1beta1/transporturl_helpers.go | 40 ------------------- .../rabbitmq/transporturl_controller.go | 14 ++++++- 2 files changed, 12 insertions(+), 42 deletions(-) delete mode 100644 apis/rabbitmq/v1beta1/transporturl_helpers.go diff --git a/apis/rabbitmq/v1beta1/transporturl_helpers.go b/apis/rabbitmq/v1beta1/transporturl_helpers.go deleted file mode 100644 index c4b19139..00000000 --- a/apis/rabbitmq/v1beta1/transporturl_helpers.go +++ /dev/null @@ -1,40 +0,0 @@ -/* -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 v1beta1 - -import ( - "strings" - - corev1 "k8s.io/api/core/v1" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" -) - -// HasTransportConsumerFinalizer returns true if the secret has any finalizer -// matching the transport consumer pattern (openstack.org/*-transport-consumer). -func HasTransportConsumerFinalizer(secret *corev1.Secret) bool { - for _, f := range secret.Finalizers { - if strings.HasSuffix(f, TransportSecretConsumerSuffix) && - strings.HasPrefix(f, "openstack.org/") { - return true - } - } - return false -} - -// HasSpecificTransportConsumerFinalizer returns true if the secret has the -// given consumer finalizer. -func HasSpecificTransportConsumerFinalizer(secret *corev1.Secret, consumerFinalizer string) bool { - return controllerutil.ContainsFinalizer(secret, consumerFinalizer) -} diff --git a/internal/controller/rabbitmq/transporturl_controller.go b/internal/controller/rabbitmq/transporturl_controller.go index 292d8476..782a548b 100644 --- a/internal/controller/rabbitmq/transporturl_controller.go +++ b/internal/controller/rabbitmq/transporturl_controller.go @@ -629,7 +629,7 @@ func (r *TransportURLReconciler) reconcileNormal(ctx context.Context, instance * if err != nil && !k8s_errors.IsNotFound(err) { return ctrl.Result{}, err } - if err == nil && rabbitmqv1.HasTransportConsumerFinalizer(oldSecret) { + if err == nil && hasTransportConsumerFinalizer(oldSecret) { Log.Info("Waiting for consumer to release previous transport secret", "secret", instance.Status.PreviousSecretName) return ctrl.Result{RequeueAfter: PendingReleaseCheckInterval}, nil @@ -1058,7 +1058,7 @@ func (r *TransportURLReconciler) tryReleasePendingUser(ctx context.Context, inst if err != nil && !k8s_errors.IsNotFound(err) { return false, err } - if err == nil && rabbitmqv1.HasTransportConsumerFinalizer(oldSecret) { + if err == nil && hasTransportConsumerFinalizer(oldSecret) { Log.Info("Waiting for consumer to release old transport secret", "secret", instance.Status.PreviousSecretName, "pendingUser", pendingRef) @@ -1309,6 +1309,16 @@ func (r *TransportURLReconciler) deleteLegacyOwnedResources(ctx context.Context, return nil } +func hasTransportConsumerFinalizer(secret *corev1.Secret) bool { + for _, f := range secret.Finalizers { + if strings.HasSuffix(f, rabbitmqv1.TransportSecretConsumerSuffix) && + strings.HasPrefix(f, "openstack.org/") { + return true + } + } + return false +} + // GetRabbitmqCluster - get RabbitMq object in namespace func getRabbitmqCluster( ctx context.Context,