From eebffe6a4db861e4d4683985fd5c624a4970a7ba Mon Sep 17 00:00:00 2001 From: Luca Miccini Date: Tue, 5 May 2026 10:20:53 +0200 Subject: [PATCH] Use shared singleton vhost/user CRs with per-consumer finalizers Multiple TransportURL CRs pointing to the same RabbitMQ vhost/user (e.g., cinder and heat both using vhost "openstack") would each create separate RabbitMQVhost/RabbitMQUser CRs. Deleting any one of them destroyed the RabbitMQ resource for all other consumers. This replaces per-TransportURL CRs with shared singletons named deterministically via CanonicalVhostName/CanonicalUserName. Each consumer adds a per-TransportURL finalizer (turl.openstack.org/t-) so the RabbitMQ resource is only deleted when all consumers are gone. The RabbitMQ cluster CR is set as owner for automatic GC on cluster deletion. Shared CR lifecycle: - Orphaned user CRs are labeled (rabbitmq.openstack.org/orphaned) instead of deleted, keeping them reclaimable by new consumers. Auto-deletion only proceeds after admin removes the cleanup-blocked finalizer. - Vhost CRs show "Waiting for external finalizers" status when deletion is blocked by user finalizer cleanup. - User controller cascades vhost CR deletion after its own cleanup. Migration from legacy per-TransportURL CRs: - Legacy CRs (with TransportURL owner refs) are cleaned up after canonical CRs are created. - Credentials are pre-copied to the canonical secret to avoid password regeneration that would break existing connections. - Webhook skips legacy CRs and deleting CRs in uniqueness checks. Webhook hardening: - Reject permission changes on shared users with active consumers. - Enforce RabbitmqClusterName immutability on user, vhost, and policy. - Validate RabbitMQPolicy Pattern as valid regex at admission. - Remove duplicate +kubebuilder:webhook markers from apis/ package. - Propagate admission context and add timeouts for k8s API calls. API client hardening: - Add context.Context to all RabbitMQ API client methods. - Bound error response reads to 1MB and drain bodies on success. - Track actual vhost/policy name in RabbitMQPolicyStatus so reconcileDelete targets the correct resource. Controller fixes: - Move ObservedGeneration to end of reconcileNormal (user, vhost, policy) so it is only set after all work succeeds. - Propagate PatchInstance errors via named return instead of swallowing them. - Guard per-consumer finalizer names against the Kubernetes 63-char limit with SHA-256 hash truncation. - Add periodic 5-minute requeue for long TransportURL names where watch-based reverse mapping fails. Backup/restore: - Add backup.openstack.org labels to auto-generated credential secrets so Velero includes them. - Set restore-order to 10 so the secret is restored before the RabbitMQUser CR (order 40). Co-Authored-By: Claude Opus 4.6 --- ...bbitmq.openstack.org_rabbitmqpolicies.yaml | 6 + apis/rabbitmq/v1beta1/conditions.go | 43 +- apis/rabbitmq/v1beta1/conditions_test.go | 89 ++ apis/rabbitmq/v1beta1/rabbitmqpolicy_types.go | 6 + .../v1beta1/rabbitmqpolicy_webhook.go | 42 +- apis/rabbitmq/v1beta1/rabbitmquser_types.go | 21 +- apis/rabbitmq/v1beta1/rabbitmquser_webhook.go | 82 +- apis/rabbitmq/v1beta1/rabbitmqvhost_types.go | 10 + .../rabbitmq/v1beta1/rabbitmqvhost_webhook.go | 18 +- ...bbitmq.openstack.org_rabbitmqpolicies.yaml | 6 + config/webhook/manifests.yaml | 120 --- internal/controller/rabbitmq/helpers.go | 8 + .../rabbitmq/rabbitmqpolicy_controller.go | 46 +- .../rabbitmq/rabbitmquser_controller.go | 129 ++- .../rabbitmq/rabbitmqvhost_controller.go | 64 +- .../rabbitmq/transporturl_controller.go | 799 +++++++-------- .../rabbitmq/v1beta1/rabbitmquser_webhook.go | 4 +- .../v1beta1/rabbitmquser_webhook_test.go | 4 +- pkg/rabbitmq/api/client.go | 130 ++- pkg/rabbitmq/api/client_test.go | 17 +- .../rabbitmquser_controller_test.go | 12 +- .../rabbitmqvhost_controller_test.go | 102 +- .../transporturl_controller_test.go | 952 +++++------------- .../01-assert.yaml | 6 +- .../02-assert.yaml | 24 +- .../03-assert.yaml | 6 +- .../03-remove-blocked-finalizer.yaml | 12 +- .../04-cleanup.yaml | 3 + 28 files changed, 1320 insertions(+), 1441 deletions(-) create mode 100644 apis/rabbitmq/v1beta1/conditions_test.go diff --git a/apis/bases/rabbitmq.openstack.org_rabbitmqpolicies.yaml b/apis/bases/rabbitmq.openstack.org_rabbitmqpolicies.yaml index 4c61cd84..66fb792b 100644 --- a/apis/bases/rabbitmq.openstack.org_rabbitmqpolicies.yaml +++ b/apis/bases/rabbitmq.openstack.org_rabbitmqpolicies.yaml @@ -147,6 +147,12 @@ spec: for this resource format: int64 type: integer + policyName: + description: PolicyName - actual policy name used in RabbitMQ + type: string + vhost: + description: Vhost - actual vhost name where the policy was last applied + type: string type: object type: object served: true diff --git a/apis/rabbitmq/v1beta1/conditions.go b/apis/rabbitmq/v1beta1/conditions.go index 6662c092..1e838c57 100644 --- a/apis/rabbitmq/v1beta1/conditions.go +++ b/apis/rabbitmq/v1beta1/conditions.go @@ -16,6 +16,9 @@ limitations under the License. package v1beta1 import ( + "crypto/sha256" + "fmt" + condition "github.com/openstack-k8s-operators/lib-common/modules/common/condition" ) @@ -32,16 +35,46 @@ const ( // TransportURLReadyCondition Status=True condition which indicates if TransportURL is configured and operational TransportURLReadyCondition condition.Type = "TransportURLReady" - // TransportURLFinalizer - finalizer to add to RabbitMQUsers owned by TransportURL + // TransportURLFinalizer - legacy finalizer for backward compatibility during migration. + // New code should use TransportURLFinalizerFor() instead. TransportURLFinalizer = "transporturl.rabbitmq.openstack.org/finalizer" - // RabbitMQUserCleanupBlockedFinalizer - temporary finalizer to block automatic cleanup of RabbitMQUsers - // This finalizer prevents TransportURL from automatically deleting users during credential rotation. - // It must be manually removed by an operator/admin to allow cleanup to proceed. - // TODO: Replace with proper safe-to-delete logic, then remove this finalizer from existing users. + // TransportURLFinalizerPrefix - prefix for per-TransportURL finalizers on shared vhost/user CRs. + // Use TransportURLFinalizerFor() to build the full finalizer name safely. + TransportURLFinalizerPrefix = "turl.openstack.org/t-" + + // maxFinalizerNameSegment is the Kubernetes limit for the name segment after "/" + maxFinalizerNameSegment = 63 + + // RabbitMQUserCleanupBlockedFinalizer - safety finalizer that blocks automatic deletion of RabbitMQUsers. + // When a shared user CR is orphaned (no active consumers), the user controller will only + // auto-delete it after an admin removes this finalizer, confirming no external services + // depend on the RabbitMQ user. RabbitMQUserCleanupBlockedFinalizer = "rabbitmq.openstack.org/cleanup-blocked" + + // RabbitMQUserOrphanedLabel marks a shared RabbitMQUser CR as having no active consumers. + // The TransportURL controller sets this label instead of deleting the CR directly, + // keeping the CR reclaimable by new consumers. The user controller auto-deletes + // the CR only when this label is present AND the cleanup-blocked finalizer is removed. + RabbitMQUserOrphanedLabel = "rabbitmq.openstack.org/orphaned" ) +// 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 +// is truncated and a short hash is appended. +func TransportURLFinalizerFor(transportURLName string) string { + prefix := "t-" + maxNameLen := maxFinalizerNameSegment - len(prefix) + if len(transportURLName) <= maxNameLen { + return TransportURLFinalizerPrefix + transportURLName + } + hash := sha256.Sum256([]byte(transportURLName)) + hashHex := fmt.Sprintf("%x", hash[:4]) + truncLen := maxNameLen - len(hashHex) + return TransportURLFinalizerPrefix + transportURLName[:truncLen] + hashHex +} + // TransportURL Reasons used by API objects. const () diff --git a/apis/rabbitmq/v1beta1/conditions_test.go b/apis/rabbitmq/v1beta1/conditions_test.go new file mode 100644 index 00000000..a8db742c --- /dev/null +++ b/apis/rabbitmq/v1beta1/conditions_test.go @@ -0,0 +1,89 @@ +/* +Copyright 2025. + +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" + "testing" +) + +func TestTransportURLFinalizerFor(t *testing.T) { + tests := []struct { + name string + transportURL string + wantPrefix bool + wantMaxLen int + wantExact string + }{ + { + name: "short name used directly", + transportURL: "nova-api-transport", + wantPrefix: true, + wantExact: TransportURLFinalizerPrefix + "nova-api-transport", + }, + { + name: "61-char name fits exactly", + transportURL: strings.Repeat("a", 61), + wantPrefix: true, + wantExact: TransportURLFinalizerPrefix + strings.Repeat("a", 61), + }, + { + name: "62-char name gets truncated and hashed", + transportURL: strings.Repeat("b", 62), + wantPrefix: true, + wantMaxLen: maxFinalizerNameSegment + len("turl.openstack.org/"), + }, + { + name: "253-char name gets truncated and hashed", + transportURL: strings.Repeat("c", 253), + wantPrefix: true, + wantMaxLen: maxFinalizerNameSegment + len("turl.openstack.org/"), + }, + { + name: "different long names produce different finalizers", + transportURL: strings.Repeat("d", 100), + wantPrefix: true, + wantMaxLen: maxFinalizerNameSegment + len("turl.openstack.org/"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := TransportURLFinalizerFor(tt.transportURL) + + if !strings.HasPrefix(got, TransportURLFinalizerPrefix) { + t.Errorf("finalizer %q does not start with prefix %q", got, TransportURLFinalizerPrefix) + } + + nameSegment := strings.SplitN(got, "/", 2)[1] + if len(nameSegment) > maxFinalizerNameSegment { + t.Errorf("name segment %q is %d chars, exceeds max %d", nameSegment, len(nameSegment), maxFinalizerNameSegment) + } + + if tt.wantExact != "" && got != tt.wantExact { + t.Errorf("got %q, want %q", got, tt.wantExact) + } + }) + } + + // Verify two different long names produce different finalizers + fin1 := TransportURLFinalizerFor(strings.Repeat("x", 100)) + fin2 := TransportURLFinalizerFor(strings.Repeat("y", 100)) + if fin1 == fin2 { + t.Errorf("different long names produced the same finalizer: %q", fin1) + } +} diff --git a/apis/rabbitmq/v1beta1/rabbitmqpolicy_types.go b/apis/rabbitmq/v1beta1/rabbitmqpolicy_types.go index 3ab46897..f12cf013 100644 --- a/apis/rabbitmq/v1beta1/rabbitmqpolicy_types.go +++ b/apis/rabbitmq/v1beta1/rabbitmqpolicy_types.go @@ -65,6 +65,12 @@ type RabbitMQPolicyStatus struct { // ObservedGeneration - the most recent generation observed for this resource ObservedGeneration int64 `json:"observedGeneration,omitempty"` + + // Vhost - actual vhost name where the policy was last applied + Vhost string `json:"vhost,omitempty"` + + // PolicyName - actual policy name used in RabbitMQ + PolicyName string `json:"policyName,omitempty"` } //+kubebuilder:object:root=true diff --git a/apis/rabbitmq/v1beta1/rabbitmqpolicy_webhook.go b/apis/rabbitmq/v1beta1/rabbitmqpolicy_webhook.go index e7e393fa..0fdd388f 100644 --- a/apis/rabbitmq/v1beta1/rabbitmqpolicy_webhook.go +++ b/apis/rabbitmq/v1beta1/rabbitmqpolicy_webhook.go @@ -18,6 +18,7 @@ package v1beta1 import ( "fmt" + "regexp" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" @@ -30,8 +31,6 @@ import ( var rabbitmqpolicylog = logf.Log.WithName("rabbitmqpolicy-resource") -//+kubebuilder:webhook:path=/mutate-rabbitmq-openstack-org-v1beta1-rabbitmqpolicy,mutating=true,failurePolicy=fail,sideEffects=None,groups=rabbitmq.openstack.org,resources=rabbitmqpolicies,verbs=create;update,versions=v1beta1,name=mrabbitmqpolicy.kb.io,admissionReviewVersions=v1 - // Default implements defaulting for RabbitMQPolicy func (r *RabbitMQPolicy) Default(_ client.Client) { rabbitmqpolicylog.Info("default", "name", r.Name) @@ -42,8 +41,6 @@ func (r *RabbitMQPolicy) Default(_ client.Client) { } } -//+kubebuilder:webhook:path=/validate-rabbitmq-openstack-org-v1beta1-rabbitmqpolicy,mutating=false,failurePolicy=fail,sideEffects=None,groups=rabbitmq.openstack.org,resources=rabbitmqpolicies,verbs=create;update,versions=v1beta1,name=vrabbitmqpolicy.kb.io,admissionReviewVersions=v1 - // ValidateCreate validates the RabbitMQPolicy on creation func (r *RabbitMQPolicy) ValidateCreate(_ client.Client) (admission.Warnings, error) { rabbitmqpolicylog.Info("validate create", "name", r.Name) @@ -56,6 +53,10 @@ func (r *RabbitMQPolicy) ValidateCreate(_ client.Client) (admission.Warnings, er ) } + if err := r.validatePattern(); err != nil { + return nil, err + } + return nil, nil } @@ -68,6 +69,20 @@ func (r *RabbitMQPolicy) ValidateUpdate(_ client.Client, old runtime.Object) (ad return nil, fmt.Errorf("expected RabbitMQPolicy but got %T", old) } + // Prevent changing the cluster after creation + if r.Spec.RabbitmqClusterName != oldPolicy.Spec.RabbitmqClusterName { + return nil, apierrors.NewInvalid( + schema.GroupKind{Group: "rabbitmq.openstack.org", Kind: "RabbitMQPolicy"}, + r.Name, + field.ErrorList{ + field.Forbidden( + field.NewPath("spec", "rabbitmqClusterName"), + "rabbitmqClusterName cannot be changed after creation", + ), + }, + ) + } + // Prevent changing the policy name after creation if r.Spec.Name != oldPolicy.Spec.Name { return nil, apierrors.NewInvalid( @@ -82,6 +97,10 @@ func (r *RabbitMQPolicy) ValidateUpdate(_ client.Client, old runtime.Object) (ad ) } + if err := r.validatePattern(); err != nil { + return nil, err + } + return nil, nil } @@ -89,3 +108,18 @@ func (r *RabbitMQPolicy) ValidateUpdate(_ client.Client, old runtime.Object) (ad func (r *RabbitMQPolicy) ValidateDelete(_ client.Client) (admission.Warnings, error) { return nil, nil } + +// validatePattern validates that the Pattern field is a valid regex +func (r *RabbitMQPolicy) validatePattern() error { + if _, err := regexp.Compile(r.Spec.Pattern); err != nil { + return apierrors.NewInvalid( + schema.GroupKind{Group: "rabbitmq.openstack.org", Kind: "RabbitMQPolicy"}, + r.Name, + field.ErrorList{ + field.Invalid(field.NewPath("spec", "pattern"), r.Spec.Pattern, + fmt.Sprintf("invalid regex pattern: %v", err)), + }, + ) + } + return nil +} diff --git a/apis/rabbitmq/v1beta1/rabbitmquser_types.go b/apis/rabbitmq/v1beta1/rabbitmquser_types.go index 9399c6aa..b4380a5d 100644 --- a/apis/rabbitmq/v1beta1/rabbitmquser_types.go +++ b/apis/rabbitmq/v1beta1/rabbitmquser_types.go @@ -17,6 +17,9 @@ limitations under the License. package v1beta1 import ( + "fmt" + "strings" + condition "github.com/openstack-k8s-operators/lib-common/modules/common/condition" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -168,14 +171,28 @@ const ( // RabbitMQUserReadyErrorMessage is the message format for the RabbitMQUserReady condition when an error occurs RabbitMQUserReadyErrorMessage = "RabbitMQ user error occurred %s" + // RabbitMQUserOrphanedMessage is the message when a user CR is orphaned and awaiting admin approval + RabbitMQUserOrphanedMessage = "User has no active consumers. Remove finalizer %s to approve deletion" + // Internal controller finalizer (from rabbitmquser_controller.go) userControllerFinalizer = "rabbitmquser.openstack.org/finalizer" ) // IsInternalFinalizer returns true if the finalizer is managed by RabbitMQ controllers -// (as opposed to external controllers like dataplane) +// (as opposed to external controllers like dataplane). +// Note: RabbitMQUserCleanupBlockedFinalizer is intentionally excluded — it must block +// user deletion as an external-like finalizer requiring manual admin removal. func IsInternalFinalizer(finalizer string) bool { return finalizer == UserFinalizer || finalizer == TransportURLFinalizer || - finalizer == userControllerFinalizer + finalizer == userControllerFinalizer || + strings.HasPrefix(finalizer, TransportURLFinalizerPrefix) +} + +// CanonicalUserName returns the deterministic CR name for a shared user singleton. +func CanonicalUserName(clusterName, vhostName, username string) string { + if vhostName == "/" || vhostName == "" { + return fmt.Sprintf("%s-user-%s", clusterName, username) + } + return fmt.Sprintf("%s-%s-user-%s", clusterName, vhostName, username) } diff --git a/apis/rabbitmq/v1beta1/rabbitmquser_webhook.go b/apis/rabbitmq/v1beta1/rabbitmquser_webhook.go index e5c7a4fc..d90dadf6 100644 --- a/apis/rabbitmq/v1beta1/rabbitmquser_webhook.go +++ b/apis/rabbitmq/v1beta1/rabbitmquser_webhook.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "regexp" + "strings" "time" corev1 "k8s.io/api/core/v1" @@ -38,10 +39,8 @@ const rabbitMQVarNameFmt = "[-_.:A-Za-z0-9]+" var rabbitMQVarNameFmtRegexp = regexp.MustCompile("^" + rabbitMQVarNameFmt + "$") -//+kubebuilder:webhook:path=/mutate-rabbitmq-openstack-org-v1beta1-rabbitmquser,mutating=true,failurePolicy=fail,sideEffects=None,groups=rabbitmq.openstack.org,resources=rabbitmqusers,verbs=create;update,versions=v1beta1,name=mrabbitmquser.kb.io,admissionReviewVersions=v1 - // Default implements defaulting for RabbitMQUser -func (r *RabbitMQUser) Default(k8sClient client.Client) { +func (r *RabbitMQUser) Default(ctx context.Context, k8sClient client.Client) { rabbitmquserlog.Info("default", "name", r.Name) // If using a secret, extract and set username from secret @@ -55,7 +54,9 @@ func (r *RabbitMQUser) Default(k8sClient client.Client) { } secret := &corev1.Secret{} - if err := k8sClient.Get(context.TODO(), + getCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + if err := k8sClient.Get(getCtx, client.ObjectKey{Name: *r.Spec.Secret, Namespace: r.Namespace}, secret); err == nil { // Extract username from secret and set in spec @@ -75,8 +76,6 @@ func (r *RabbitMQUser) Default(k8sClient client.Client) { } } -//+kubebuilder:webhook:path=/validate-rabbitmq-openstack-org-v1beta1-rabbitmquser,mutating=false,failurePolicy=fail,sideEffects=None,groups=rabbitmq.openstack.org,resources=rabbitmqusers,verbs=create;update,versions=v1beta1,name=vrabbitmquser.kb.io,admissionReviewVersions=v1 - // ValidateCreate validates the RabbitMQUser on creation func (r *RabbitMQUser) ValidateCreate(k8sClient client.Client) (admission.Warnings, error) { rabbitmquserlog.Info("validate create", "name", r.Name) @@ -134,6 +133,20 @@ func (r *RabbitMQUser) ValidateUpdate(k8sClient client.Client, old runtime.Objec return nil, err } + // Prevent changing the cluster after creation + if r.Spec.RabbitmqClusterName != oldUser.Spec.RabbitmqClusterName { + return nil, apierrors.NewInvalid( + schema.GroupKind{Group: "rabbitmq.openstack.org", Kind: "RabbitMQUser"}, + r.Name, + field.ErrorList{ + field.Forbidden( + field.NewPath("spec", "rabbitmqClusterName"), + "rabbitmqClusterName cannot be changed after creation", + ), + }, + ) + } + // Prevent changing the username after creation // Check against status.Username if available (ground truth from RabbitMQ), // otherwise fall back to spec.Username @@ -202,7 +215,41 @@ func (r *RabbitMQUser) ValidateUpdate(k8sClient client.Client, old runtime.Objec } } - return nil, r.validateUniqueUsername(k8sClient, r.Spec.Username) + // Skip uniqueness check for legacy CRs (those with a TransportURL owner reference). + // During migration, both legacy and canonical CRs may coexist temporarily. + isLegacyCR := false + for _, ref := range r.GetOwnerReferences() { + if ref.Kind == "TransportURL" { + isLegacyCR = true + break + } + } + if !isLegacyCR { + if err := r.validateUniqueUsername(k8sClient, r.Spec.Username); err != nil { + return nil, err + } + } + + // Reject permission changes if other TransportURLs have per-consumer finalizers on this user. + // Shared users must keep consistent permissions while any consumer references them. + if oldUser.Spec.Permissions != r.Spec.Permissions { + for _, fin := range r.GetFinalizers() { + if strings.HasPrefix(fin, TransportURLFinalizerPrefix) { + return nil, apierrors.NewInvalid( + schema.GroupKind{Group: "rabbitmq.openstack.org", Kind: "RabbitMQUser"}, + r.Name, + field.ErrorList{ + field.Forbidden( + field.NewPath("spec", "permissions"), + "permissions cannot be changed while other TransportURLs reference this shared user", + ), + }, + ) + } + } + } + + return nil, nil } // ValidateDelete validates the RabbitMQUser on deletion @@ -237,7 +284,9 @@ func (r *RabbitMQUser) validateSecretAndExtractCredentials(k8sClient client.Clie } secret := &corev1.Secret{} - if err := k8sClient.Get(context.TODO(), + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := k8sClient.Get(ctx, client.ObjectKey{Name: secretName, Namespace: r.Namespace}, secret); err != nil { return apierrors.NewInvalid( @@ -335,6 +384,23 @@ func (r *RabbitMQUser) validateUniqueUsername(k8sClient client.Client, username continue } + // Skip CRs that are being deleted + if !user.DeletionTimestamp.IsZero() { + continue + } + + // Skip legacy CRs owned by a TransportURL (migration to shared singletons) + isLegacy := false + for _, ref := range user.GetOwnerReferences() { + if ref.Kind == "TransportURL" { + isLegacy = true + break + } + } + if isLegacy { + continue + } + // Check if same RabbitMQ cluster if user.Spec.RabbitmqClusterName != r.Spec.RabbitmqClusterName { continue diff --git a/apis/rabbitmq/v1beta1/rabbitmqvhost_types.go b/apis/rabbitmq/v1beta1/rabbitmqvhost_types.go index 64b284cc..5e0137af 100644 --- a/apis/rabbitmq/v1beta1/rabbitmqvhost_types.go +++ b/apis/rabbitmq/v1beta1/rabbitmqvhost_types.go @@ -17,6 +17,8 @@ limitations under the License. package v1beta1 import ( + "fmt" + condition "github.com/openstack-k8s-operators/lib-common/modules/common/condition" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -108,4 +110,12 @@ const ( // RabbitMQVhostReadyErrorMessage is the message format for the RabbitMQVhostReady condition when an error occurs RabbitMQVhostReadyErrorMessage = "RabbitMQ vhost error occurred %s" + + // RabbitMQVhostDeletionWaitingMessage is the message when vhost deletion is blocked by external finalizers + RabbitMQVhostDeletionWaitingMessage = "Waiting for external finalizers to be removed: %s" ) + +// CanonicalVhostName returns the deterministic CR name for a shared vhost singleton. +func CanonicalVhostName(clusterName, vhostName string) string { + return fmt.Sprintf("%s-vhost-%s", clusterName, vhostName) +} diff --git a/apis/rabbitmq/v1beta1/rabbitmqvhost_webhook.go b/apis/rabbitmq/v1beta1/rabbitmqvhost_webhook.go index 59d3b318..0442d64e 100644 --- a/apis/rabbitmq/v1beta1/rabbitmqvhost_webhook.go +++ b/apis/rabbitmq/v1beta1/rabbitmqvhost_webhook.go @@ -30,8 +30,6 @@ import ( var rabbitmqvhostlog = logf.Log.WithName("rabbitmqvhost-resource") -//+kubebuilder:webhook:path=/mutate-rabbitmq-openstack-org-v1beta1-rabbitmqvhost,mutating=true,failurePolicy=fail,sideEffects=None,groups=rabbitmq.openstack.org,resources=rabbitmqvhosts,verbs=create;update,versions=v1beta1,name=mrabbitmqvhost.kb.io,admissionReviewVersions=v1 - // Default implements defaulting for RabbitMQVhost func (r *RabbitMQVhost) Default(_ client.Client) { rabbitmqvhostlog.Info("default", "name", r.Name) @@ -42,8 +40,6 @@ func (r *RabbitMQVhost) Default(_ client.Client) { } } -//+kubebuilder:webhook:path=/validate-rabbitmq-openstack-org-v1beta1-rabbitmqvhost,mutating=false,failurePolicy=fail,sideEffects=None,groups=rabbitmq.openstack.org,resources=rabbitmqvhosts,verbs=create;update,versions=v1beta1,name=vrabbitmqvhost.kb.io,admissionReviewVersions=v1 - // ValidateCreate validates the RabbitMQVhost on creation func (r *RabbitMQVhost) ValidateCreate(_ client.Client) (admission.Warnings, error) { rabbitmqvhostlog.Info("validate create", "name", r.Name) @@ -71,6 +67,20 @@ func (r *RabbitMQVhost) ValidateUpdate(_ client.Client, old runtime.Object) (adm return nil, fmt.Errorf("expected RabbitMQVhost but got %T", old) } + // Prevent changing the cluster after creation + if r.Spec.RabbitmqClusterName != oldVhost.Spec.RabbitmqClusterName { + return nil, apierrors.NewInvalid( + schema.GroupKind{Group: "rabbitmq.openstack.org", Kind: "RabbitMQVhost"}, + r.Name, + field.ErrorList{ + field.Forbidden( + field.NewPath("spec", "rabbitmqClusterName"), + "rabbitmqClusterName cannot be changed after creation", + ), + }, + ) + } + // Prevent changing the vhost name after creation if r.Spec.Name != oldVhost.Spec.Name { return nil, apierrors.NewInvalid( diff --git a/config/crd/bases/rabbitmq.openstack.org_rabbitmqpolicies.yaml b/config/crd/bases/rabbitmq.openstack.org_rabbitmqpolicies.yaml index 4c61cd84..66fb792b 100644 --- a/config/crd/bases/rabbitmq.openstack.org_rabbitmqpolicies.yaml +++ b/config/crd/bases/rabbitmq.openstack.org_rabbitmqpolicies.yaml @@ -147,6 +147,12 @@ spec: for this resource format: int64 type: integer + policyName: + description: PolicyName - actual policy name used in RabbitMQ + type: string + vhost: + description: Vhost - actual vhost name where the policy was last applied + type: string type: object type: object served: true diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index ee419e33..3f4c80a9 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -224,66 +224,6 @@ webhooks: resources: - redises sideEffects: None -- admissionReviewVersions: - - v1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /mutate-rabbitmq-openstack-org-v1beta1-rabbitmqpolicy - failurePolicy: Fail - name: mrabbitmqpolicy.kb.io - rules: - - apiGroups: - - rabbitmq.openstack.org - apiVersions: - - v1beta1 - operations: - - CREATE - - UPDATE - resources: - - rabbitmqpolicies - sideEffects: None -- admissionReviewVersions: - - v1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /mutate-rabbitmq-openstack-org-v1beta1-rabbitmquser - failurePolicy: Fail - name: mrabbitmquser.kb.io - rules: - - apiGroups: - - rabbitmq.openstack.org - apiVersions: - - v1beta1 - operations: - - CREATE - - UPDATE - resources: - - rabbitmqusers - sideEffects: None -- admissionReviewVersions: - - v1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /mutate-rabbitmq-openstack-org-v1beta1-rabbitmqvhost - failurePolicy: Fail - name: mrabbitmqvhost.kb.io - rules: - - apiGroups: - - rabbitmq.openstack.org - apiVersions: - - v1beta1 - operations: - - CREATE - - UPDATE - resources: - - rabbitmqvhosts - sideEffects: None --- apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration @@ -511,63 +451,3 @@ webhooks: resources: - redises sideEffects: None -- admissionReviewVersions: - - v1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /validate-rabbitmq-openstack-org-v1beta1-rabbitmqpolicy - failurePolicy: Fail - name: vrabbitmqpolicy.kb.io - rules: - - apiGroups: - - rabbitmq.openstack.org - apiVersions: - - v1beta1 - operations: - - CREATE - - UPDATE - resources: - - rabbitmqpolicies - sideEffects: None -- admissionReviewVersions: - - v1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /validate-rabbitmq-openstack-org-v1beta1-rabbitmquser - failurePolicy: Fail - name: vrabbitmquser.kb.io - rules: - - apiGroups: - - rabbitmq.openstack.org - apiVersions: - - v1beta1 - operations: - - CREATE - - UPDATE - resources: - - rabbitmqusers - sideEffects: None -- admissionReviewVersions: - - v1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /validate-rabbitmq-openstack-org-v1beta1-rabbitmqvhost - failurePolicy: Fail - name: vrabbitmqvhost.kb.io - rules: - - apiGroups: - - rabbitmq.openstack.org - apiVersions: - - v1beta1 - operations: - - CREATE - - UPDATE - resources: - - rabbitmqvhosts - sideEffects: None diff --git a/internal/controller/rabbitmq/helpers.go b/internal/controller/rabbitmq/helpers.go index 3756b2ca..eeb8b610 100644 --- a/internal/controller/rabbitmq/helpers.go +++ b/internal/controller/rabbitmq/helpers.go @@ -109,5 +109,13 @@ func checkClusterReadiness(rabbit *rabbitmqv1.RabbitMq) *ClusterReadinessError { } } + if rabbit.Status.DefaultUser.SecretReference == nil { + return &ClusterReadinessError{ + ClusterName: rabbit.Name, + Reason: fmt.Sprintf("RabbitMQ cluster %s admin secret reference not yet available", rabbit.Name), + IsWaiting: true, + } + } + return nil } diff --git a/internal/controller/rabbitmq/rabbitmqpolicy_controller.go b/internal/controller/rabbitmq/rabbitmqpolicy_controller.go index 66355432..04283f72 100644 --- a/internal/controller/rabbitmq/rabbitmqpolicy_controller.go +++ b/internal/controller/rabbitmq/rabbitmqpolicy_controller.go @@ -57,7 +57,7 @@ type RabbitMQPolicyReconciler struct { //+kubebuilder:rbac:groups=rabbitmq.openstack.org,resources=rabbitmqs,verbs=get;list;watch // Reconcile reconciles a RabbitMQPolicy object -func (r *RabbitMQPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *RabbitMQPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, _err error) { Log := log.FromContext(ctx) instance := &rabbitmqv1.RabbitMQPolicy{} @@ -84,7 +84,6 @@ func (r *RabbitMQPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque condition.UnknownCondition(rabbitmqv1.RabbitMQPolicyReadyCondition, condition.InitReason, rabbitmqv1.RabbitMQPolicyReadyInitMessage), ) instance.Status.Conditions.Init(&cl) - instance.Status.ObservedGeneration = instance.Generation defer func() { // Restore condition timestamps if they haven't changed @@ -93,8 +92,10 @@ func (r *RabbitMQPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque if instance.Status.Conditions.IsUnknown(condition.ReadyCondition) { instance.Status.Conditions.Set(instance.Status.Conditions.Mirror(condition.ReadyCondition)) } - if err := h.PatchInstance(ctx, instance); err != nil { - Log.Error(err, "Failed to patch instance") + err := h.PatchInstance(ctx, instance) + if err != nil { + _err = err + return } }() @@ -185,34 +186,43 @@ func (r *RabbitMQPolicyReconciler) reconcileNormal(ctx context.Context, instance instance.Status.Conditions.Set(condition.FalseCondition(rabbitmqv1.RabbitMQPolicyReadyCondition, condition.ErrorReason, condition.SeverityWarning, rabbitmqv1.RabbitMQPolicyReadyErrorMessage, err.Error())) return ctrl.Result{}, err } - err = apiClient.CreateOrUpdatePolicy(vhostName, policyName, instance.Spec.Pattern, definition, instance.Spec.Priority, instance.Spec.ApplyTo) + err = apiClient.CreateOrUpdatePolicy(ctx, vhostName, policyName, instance.Spec.Pattern, definition, instance.Spec.Priority, instance.Spec.ApplyTo) if err != nil { instance.Status.Conditions.Set(condition.FalseCondition(rabbitmqv1.RabbitMQPolicyReadyCondition, condition.ErrorReason, condition.SeverityWarning, rabbitmqv1.RabbitMQPolicyReadyErrorMessage, err.Error())) return ctrl.Result{}, err } + instance.Status.Vhost = vhostName + instance.Status.PolicyName = policyName instance.Status.Conditions.MarkTrue(rabbitmqv1.RabbitMQPolicyReadyCondition, rabbitmqv1.RabbitMQPolicyReadyMessage) instance.Status.Conditions.MarkTrue(condition.ReadyCondition, condition.ReadyMessage) + instance.Status.ObservedGeneration = instance.Generation return ctrl.Result{}, nil } func (r *RabbitMQPolicyReconciler) reconcileDelete(ctx context.Context, instance *rabbitmqv1.RabbitMQPolicy, h *helper.Helper) (ctrl.Result, error) { - policyName := instance.Spec.Name + // Use status values (where the policy was actually created) with spec fallback + policyName := instance.Status.PolicyName if policyName == "" { - policyName = instance.Name + policyName = instance.Spec.Name + if policyName == "" { + policyName = instance.Name + } } - vhostName := "/" - if instance.Spec.VhostRef != "" { - vhost := &rabbitmqv1.RabbitMQVhost{} - err := r.Get(ctx, types.NamespacedName{Name: instance.Spec.VhostRef, Namespace: instance.Namespace}, vhost) - if err != nil && !k8s_errors.IsNotFound(err) { - // Log non-NotFound errors but continue with deletion - log.FromContext(ctx).Error(err, "Failed to get vhost", "vhost", instance.Spec.VhostRef) - } - if vhost.Spec.Name != "" { - vhostName = vhost.Spec.Name + vhostName := instance.Status.Vhost + if vhostName == "" { + vhostName = "/" + if instance.Spec.VhostRef != "" { + vhost := &rabbitmqv1.RabbitMQVhost{} + err := r.Get(ctx, types.NamespacedName{Name: instance.Spec.VhostRef, Namespace: instance.Namespace}, vhost) + if err != nil && !k8s_errors.IsNotFound(err) { + log.FromContext(ctx).Error(err, "Failed to get vhost", "vhost", instance.Spec.VhostRef) + } + if vhost.Spec.Name != "" { + vhostName = vhost.Spec.Name + } } } @@ -260,7 +270,7 @@ func (r *RabbitMQPolicyReconciler) reconcileDelete(ctx context.Context, instance // Delete policy from RabbitMQ // Note: DeletePolicy already treats 404 as success - if err := apiClient.DeletePolicy(vhostName, policyName); err != nil { + if err := apiClient.DeletePolicy(ctx, vhostName, policyName); err != nil { // Return error to trigger retry - this ensures proper cleanup in normal operations // Trade-off: CR may be stuck in Terminating state if RabbitMQ is persistently unavailable // Rationale: diff --git a/internal/controller/rabbitmq/rabbitmquser_controller.go b/internal/controller/rabbitmq/rabbitmquser_controller.go index 323c27d0..bdd0adab 100644 --- a/internal/controller/rabbitmq/rabbitmquser_controller.go +++ b/internal/controller/rabbitmq/rabbitmquser_controller.go @@ -37,6 +37,7 @@ import ( rabbitmqapi "github.com/openstack-k8s-operators/infra-operator/pkg/rabbitmq/api" condition "github.com/openstack-k8s-operators/lib-common/modules/common/condition" helper "github.com/openstack-k8s-operators/lib-common/modules/common/helper" + "github.com/openstack-k8s-operators/lib-common/modules/common/labels" "github.com/openstack-k8s-operators/lib-common/modules/common/object" oko_secret "github.com/openstack-k8s-operators/lib-common/modules/common/secret" k8s_errors "k8s.io/apimachinery/pkg/api/errors" @@ -82,7 +83,7 @@ type RabbitMQUserReconciler struct { //+kubebuilder:rbac:groups=core,resources=secrets,verbs=get;list;watch;create;update;patch;delete // Reconcile reconciles a RabbitMQUser object -func (r *RabbitMQUserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *RabbitMQUserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, _err error) { Log := log.FromContext(ctx) instance := &rabbitmqv1.RabbitMQUser{} @@ -109,7 +110,6 @@ func (r *RabbitMQUserReconciler) Reconcile(ctx context.Context, req ctrl.Request condition.UnknownCondition(rabbitmqv1.RabbitMQUserReadyCondition, condition.InitReason, rabbitmqv1.RabbitMQUserReadyInitMessage), ) instance.Status.Conditions.Init(&cl) - instance.Status.ObservedGeneration = instance.Generation defer func() { // Restore condition timestamps if they haven't changed @@ -118,8 +118,10 @@ func (r *RabbitMQUserReconciler) Reconcile(ctx context.Context, req ctrl.Request if instance.Status.Conditions.IsUnknown(condition.ReadyCondition) { instance.Status.Conditions.Set(instance.Status.Conditions.Mirror(condition.ReadyCondition)) } - if err := h.PatchInstance(ctx, instance); err != nil { - Log.Error(err, "Failed to patch instance") + err := h.PatchInstance(ctx, instance) + if err != nil { + _err = err + return } }() @@ -203,6 +205,24 @@ func (r *RabbitMQUserReconciler) reconcileNormal(ctx context.Context, instance * } } + // Check if this user CR has been marked as orphaned (no active consumers) + if instance.Labels[rabbitmqv1.RabbitMQUserOrphanedLabel] == "true" { + if controllerutil.ContainsFinalizer(instance, rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer) { + instance.Status.Conditions.Set(condition.FalseCondition( + rabbitmqv1.RabbitMQUserReadyCondition, + condition.RequestedReason, + condition.SeverityInfo, + rabbitmqv1.RabbitMQUserOrphanedMessage, + rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer)) + return ctrl.Result{RequeueAfter: 30 * time.Second}, nil + } + Log.Info("Orphaned user approved for deletion (cleanup-blocked removed)", "user", instance.Name) + if err := r.Delete(ctx, instance); err != nil && !k8s_errors.IsNotFound(err) { + return ctrl.Result{}, fmt.Errorf("failed to delete orphaned user %s: %w", instance.Name, err) + } + return ctrl.Result{}, nil + } + // Get vhost - default to "/" if VhostRef is empty vhostName := "/" var vhost *rabbitmqv1.RabbitMQVhost @@ -324,6 +344,11 @@ func (r *RabbitMQUserReconciler) reconcileNormal(ctx context.Context, instance * } _, err = controllerutil.CreateOrUpdate(ctx, r.Client, secret, func() error { + secret.Labels = labels.GetLabels(instance, "rabbitmquser", map[string]string{ + "backup.openstack.org/category": "controlplane", + "backup.openstack.org/restore": "true", + "backup.openstack.org/restore-order": "10", + }) secret.Data["username"] = []byte(username) secret.Data["password"] = []byte(password) return controllerutil.SetControllerReference(instance, secret, r.Scheme) @@ -369,7 +394,7 @@ func (r *RabbitMQUserReconciler) reconcileNormal(ctx context.Context, instance * oldPermissionsDeleted := true if vhostChanged && instance.Status.Vhost != "" { Log.Info("Vhost changed, deleting permissions from old vhost", "old_vhost", instance.Status.Vhost, "new_vhost", vhostName, "username", username) - if err := apiClient.DeletePermissions(instance.Status.Vhost, username); err != nil { + if err := apiClient.DeletePermissions(ctx, instance.Status.Vhost, username); err != nil { // Track that old permissions weren't deleted - we'll retry on next reconciliation // We continue to set new permissions so the user works in the new vhost, // but we won't update status.Vhost until old permissions are cleaned up @@ -383,7 +408,7 @@ func (r *RabbitMQUserReconciler) reconcileNormal(ctx context.Context, instance * if tags == nil { tags = []string{} } - err = apiClient.CreateOrUpdateUser(username, password, tags) + err = apiClient.CreateOrUpdateUser(ctx, username, password, tags) if err != nil { instance.Status.Conditions.Set(condition.FalseCondition(rabbitmqv1.RabbitMQUserReadyCondition, condition.ErrorReason, condition.SeverityWarning, rabbitmqv1.RabbitMQUserReadyErrorMessage, err.Error())) return ctrl.Result{}, err @@ -393,7 +418,7 @@ func (r *RabbitMQUserReconciler) reconcileNormal(ctx context.Context, instance * // Note: instance.Spec.Permissions is never nil because the field doesn't use omitempty. // Individual permission fields (Configure/Write/Read) are guaranteed to have values // either from user input or from kubebuilder defaults (".*" for full permissions). - err = apiClient.SetPermissions(vhostName, username, + err = apiClient.SetPermissions(ctx, vhostName, username, instance.Spec.Permissions.Configure, instance.Spec.Permissions.Write, instance.Spec.Permissions.Read) @@ -406,9 +431,18 @@ func (r *RabbitMQUserReconciler) reconcileNormal(ctx context.Context, instance * // This ensures we keep track of the old vhost and retry cleanup on next reconciliation if oldPermissionsDeleted { instance.Status.Vhost = vhostName + } else { + instance.Status.Conditions.Set(condition.FalseCondition( + rabbitmqv1.RabbitMQUserReadyCondition, + condition.ErrorReason, + condition.SeverityWarning, + rabbitmqv1.RabbitMQUserReadyErrorMessage, + fmt.Sprintf("failed to delete permissions from old vhost %s, will retry", instance.Status.Vhost))) + return ctrl.Result{RequeueAfter: time.Duration(10) * time.Second}, nil } instance.Status.Conditions.MarkTrue(rabbitmqv1.RabbitMQUserReadyCondition, rabbitmqv1.RabbitMQUserReadyMessage) instance.Status.Conditions.MarkTrue(condition.ReadyCondition, condition.ReadyMessage) + instance.Status.ObservedGeneration = instance.Generation return ctrl.Result{}, nil } @@ -416,8 +450,7 @@ func (r *RabbitMQUserReconciler) reconcileNormal(ctx context.Context, instance * func (r *RabbitMQUserReconciler) reconcileDelete(ctx context.Context, instance *rabbitmqv1.RabbitMQUser, h *helper.Helper) (ctrl.Result, error) { Log := log.FromContext(ctx) - // If TransportURL finalizer exists, wait for TransportURL to remove it - // The TransportURL controller manages this finalizer and removes it when switching users + // Wait for legacy TransportURL finalizer (backward compatibility) if controllerutil.ContainsFinalizer(instance, rabbitmqv1.TransportURLFinalizer) { instance.Status.Conditions.MarkFalse( rabbitmqv1.RabbitMQUserReadyCondition, @@ -429,6 +462,21 @@ func (r *RabbitMQUserReconciler) reconcileDelete(ctx context.Context, instance * return ctrl.Result{RequeueAfter: time.Duration(2) * time.Second}, nil } + // Wait for all per-TransportURL consumer finalizers to be removed + for _, finalizer := range instance.GetFinalizers() { + if strings.HasPrefix(finalizer, rabbitmqv1.TransportURLFinalizerPrefix) { + turlName := finalizer[len(rabbitmqv1.TransportURLFinalizerPrefix):] + instance.Status.Conditions.MarkFalse( + rabbitmqv1.RabbitMQUserReadyCondition, + condition.DeletingReason, + condition.SeverityInfo, + "Waiting for TransportURL %s to release user", + turlName, + ) + return ctrl.Result{}, nil + } + } + // Check for external finalizers (not managed by this controller) // Wait for all external finalizers to be removed before proceeding with cleanup // This ensures other controllers (e.g., dataplane) finish using this user before deletion @@ -494,6 +542,10 @@ func (r *RabbitMQUserReconciler) reconcileDelete(ctx context.Context, instance * Log.Info("Successfully removed finalizer from vhost", "vhost", vhostRef, "finalizer", userFinalizer) } } + // Delete the vhost CR if no consumers or users reference it anymore + if err := r.deleteOrphanedVhost(ctx, vhostRef, instance.Namespace); err != nil { + return ctrl.Result{}, err + } } else if k8s_errors.IsNotFound(err) { // Vhost doesn't exist - nothing to clean up Log.Info("Vhost not found during user deletion", "vhost", vhostRef) @@ -585,16 +637,31 @@ func (r *RabbitMQUserReconciler) reconcileDelete(ctx context.Context, instance * return ctrl.Result{}, fmt.Errorf("failed to create RabbitMQ API client: %w", err) } - // Delete permissions and user from RabbitMQ - // The Delete methods already treat 404 as success - if err := apiClient.DeletePermissions(vhostName, username); err != nil { - // Return error to trigger retry - see rabbitmqpolicy_controller.go for detailed rationale - return ctrl.Result{}, fmt.Errorf("failed to delete permissions for user %s from vhost %s in RabbitMQ: %w", username, vhostName, err) + // Safety check: skip RabbitMQ API delete if another CR manages the same user + skipRabbitMQDelete := false + userList := &rabbitmqv1.RabbitMQUserList{} + if err := r.List(ctx, userList, client.InNamespace(instance.Namespace)); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to list users for safety check: %w", err) + } + for _, other := range userList.Items { + if other.Name != instance.Name && + other.Spec.Username == instance.Spec.Username && + other.Spec.RabbitmqClusterName == instance.Spec.RabbitmqClusterName && + other.DeletionTimestamp.IsZero() { + Log.Info("Skipping user deletion from RabbitMQ -- another CR manages the same user", + "username", username, "otherCR", other.Name) + skipRabbitMQDelete = true + break + } } - if err := apiClient.DeleteUser(username); err != nil { - // Return error to trigger retry - see rabbitmqpolicy_controller.go for detailed rationale - return ctrl.Result{}, fmt.Errorf("failed to delete user %s from RabbitMQ: %w", username, err) + if !skipRabbitMQDelete { + if err := apiClient.DeletePermissions(ctx, vhostName, username); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to delete permissions for user %s from vhost %s in RabbitMQ: %w", username, vhostName, err) + } + if err := apiClient.DeleteUser(ctx, username); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to delete user %s from RabbitMQ: %w", username, err) + } } // Only delete auto-generated secret (when spec.secret is not set) @@ -622,6 +689,34 @@ func (r *RabbitMQUserReconciler) reconcileDelete(ctx context.Context, instance * return ctrl.Result{}, nil } +// deleteOrphanedVhost deletes a vhost CR if no TransportURL or user finalizers remain on it. +func (r *RabbitMQUserReconciler) deleteOrphanedVhost(ctx context.Context, name, namespace string) error { + Log := log.FromContext(ctx) + vhost := &rabbitmqv1.RabbitMQVhost{} + if err := r.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, vhost); err != nil { + if k8s_errors.IsNotFound(err) { + return nil + } + return fmt.Errorf("failed to get vhost %s: %w", name, err) + } + if !vhost.GetDeletionTimestamp().IsZero() { + return nil + } + for _, f := range vhost.GetFinalizers() { + if strings.HasPrefix(f, rabbitmqv1.TransportURLFinalizerPrefix) { + return nil + } + if strings.HasPrefix(f, rabbitmqv1.UserVhostFinalizerPrefix) { + return nil + } + } + Log.Info("Deleting orphaned vhost CR (no remaining consumers or users)", "name", name) + if err := r.Delete(ctx, vhost); err != nil && !k8s_errors.IsNotFound(err) { + return fmt.Errorf("failed to delete orphaned vhost CR %s: %w", name, err) + } + return nil +} + // vhostToUserMapFunc maps vhost changes to user reconciliation requests // This allows the user controller to react when a vhost changes, eliminating // the need to poll when waiting for old vhost permissions to be deleted diff --git a/internal/controller/rabbitmq/rabbitmqvhost_controller.go b/internal/controller/rabbitmq/rabbitmqvhost_controller.go index 5f6cbeb9..ac2500d3 100644 --- a/internal/controller/rabbitmq/rabbitmqvhost_controller.go +++ b/internal/controller/rabbitmq/rabbitmqvhost_controller.go @@ -19,6 +19,7 @@ package rabbitmq import ( "context" "fmt" + "strings" "time" ctrl "sigs.k8s.io/controller-runtime" @@ -64,7 +65,7 @@ type RabbitMQVhostReconciler struct { //+kubebuilder:rbac:groups=core,resources=secrets,verbs=get;list;watch // Reconcile reconciles a RabbitMQVhost object -func (r *RabbitMQVhostReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *RabbitMQVhostReconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, _err error) { Log := log.FromContext(ctx) instance := &rabbitmqv1.RabbitMQVhost{} @@ -91,7 +92,6 @@ func (r *RabbitMQVhostReconciler) Reconcile(ctx context.Context, req ctrl.Reques condition.UnknownCondition(rabbitmqv1.RabbitMQVhostReadyCondition, condition.InitReason, rabbitmqv1.RabbitMQVhostReadyInitMessage), ) instance.Status.Conditions.Init(&cl) - instance.Status.ObservedGeneration = instance.Generation defer func() { // Restore condition timestamps if they haven't changed @@ -100,8 +100,10 @@ func (r *RabbitMQVhostReconciler) Reconcile(ctx context.Context, req ctrl.Reques if instance.Status.Conditions.IsUnknown(condition.ReadyCondition) { instance.Status.Conditions.Set(instance.Status.Conditions.Mirror(condition.ReadyCondition)) } - if err := h.PatchInstance(ctx, instance); err != nil { - Log.Error(err, "Failed to patch instance") + err := h.PatchInstance(ctx, instance) + if err != nil { + _err = err + return } }() @@ -178,7 +180,7 @@ func (r *RabbitMQVhostReconciler) reconcileNormal(ctx context.Context, instance } if vhostName != "/" { - err = apiClient.CreateOrUpdateVhost(vhostName) + err = apiClient.CreateOrUpdateVhost(ctx, vhostName) if err != nil { instance.Status.Conditions.Set(condition.FalseCondition(rabbitmqv1.RabbitMQVhostReadyCondition, condition.ErrorReason, condition.SeverityWarning, rabbitmqv1.RabbitMQVhostReadyErrorMessage, err.Error())) return ctrl.Result{}, err @@ -187,6 +189,7 @@ func (r *RabbitMQVhostReconciler) reconcileNormal(ctx context.Context, instance instance.Status.Conditions.MarkTrue(rabbitmqv1.RabbitMQVhostReadyCondition, rabbitmqv1.RabbitMQVhostReadyMessage) instance.Status.Conditions.MarkTrue(condition.ReadyCondition, condition.ReadyMessage) + instance.Status.ObservedGeneration = instance.Generation return ctrl.Result{}, nil } @@ -194,13 +197,21 @@ func (r *RabbitMQVhostReconciler) reconcileNormal(ctx context.Context, instance func (r *RabbitMQVhostReconciler) reconcileDelete(ctx context.Context, instance *rabbitmqv1.RabbitMQVhost, h *helper.Helper) (ctrl.Result, error) { Log := log.FromContext(ctx) - // If TransportURL finalizer exists, wait for TransportURL to remove it - // The TransportURL controller manages this finalizer - // The watch on TransportURL will trigger reconciliation when the finalizer is removed + // Wait for legacy TransportURL finalizer (backward compatibility) if controllerutil.ContainsFinalizer(instance, rabbitmqv1.TransportURLFinalizer) { + Log.Info("Vhost deletion blocked by legacy TransportURL finalizer", "vhost", instance.Name) return ctrl.Result{}, nil } + // Wait for all per-TransportURL consumer finalizers to be removed + for _, finalizer := range instance.Finalizers { + if strings.HasPrefix(finalizer, rabbitmqv1.TransportURLFinalizerPrefix) { + turlName := finalizer[len(rabbitmqv1.TransportURLFinalizerPrefix):] + Log.Info("Vhost deletion blocked by TransportURL consumer", "vhost", instance.Name, "transportURL", turlName) + return ctrl.Result{}, nil + } + } + // Wait for user-managed finalizers to be removed before deleting vhost // These follow the pattern: rmquser.openstack.org/u- // The RabbitMQUser controller removes these finalizers during user deletion. @@ -265,11 +276,17 @@ func (r *RabbitMQVhostReconciler) reconcileDelete(ctx context.Context, instance // Active user still references this vhost - wait // The watch on RabbitMQUser will trigger reconciliation when the user is deleted Log.Info("Vhost deletion blocked by active user", "vhost", instance.Name, "user", matchingUser.Name, "username", username) + instance.Status.Conditions.Set(condition.FalseCondition( + rabbitmqv1.RabbitMQVhostReadyCondition, condition.RequestedReason, condition.SeverityInfo, + rabbitmqv1.RabbitMQVhostDeletionWaitingMessage, finalizer)) return ctrl.Result{}, nil } // User is being deleted - wait for it to remove its finalizer // The watch on RabbitMQUser will trigger reconciliation when the user deletion completes Log.Info("Vhost deletion waiting for user deletion to complete", "vhost", instance.Name, "user", matchingUser.Name, "username", username) + instance.Status.Conditions.Set(condition.FalseCondition( + rabbitmqv1.RabbitMQVhostReadyCondition, condition.RequestedReason, condition.SeverityInfo, + rabbitmqv1.RabbitMQVhostDeletionWaitingMessage, finalizer)) return ctrl.Result{}, nil } } @@ -335,10 +352,28 @@ func (r *RabbitMQVhostReconciler) reconcileDelete(ctx context.Context, instance vhostName = "/" } if vhostName != "/" { - // DeleteVhost already treats 404 as success - if err := apiClient.DeleteVhost(vhostName); err != nil { - // Return error to trigger retry - see rabbitmqpolicy_controller.go for detailed rationale - return ctrl.Result{}, fmt.Errorf("failed to delete vhost %s from RabbitMQ: %w", vhostName, err) + // Safety check: skip RabbitMQ API delete if another CR manages the same vhost + skipDelete := false + vhostList := &rabbitmqv1.RabbitMQVhostList{} + if err := r.List(ctx, vhostList, client.InNamespace(instance.Namespace)); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to list vhosts for safety check: %w", err) + } + for _, other := range vhostList.Items { + if other.Name != instance.Name && + other.Spec.Name == instance.Spec.Name && + other.Spec.RabbitmqClusterName == instance.Spec.RabbitmqClusterName && + other.DeletionTimestamp.IsZero() { + Log.Info("Skipping vhost deletion from RabbitMQ -- another CR manages the same vhost", + "vhost", vhostName, "otherCR", other.Name) + skipDelete = true + break + } + } + + if !skipDelete { + if err := apiClient.DeleteVhost(ctx, vhostName); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to delete vhost %s from RabbitMQ: %w", vhostName, err) + } } } @@ -368,16 +403,15 @@ func (r *RabbitMQVhostReconciler) userToVhostMapFunc(_ context.Context, obj clie func (r *RabbitMQVhostReconciler) transportURLToVhostMapFunc(_ context.Context, obj client.Object) []reconcile.Request { turl := obj.(*rabbitmqv1.TransportURL) - // Reconcile the vhost if it's specified in the spec or tracked in the status requests := []reconcile.Request{} vhostName := turl.Spec.Vhost if vhostName == "" { vhostName = turl.Status.RabbitmqVhost } - if vhostName != "" && vhostName != "/" { + if vhostName != "" && vhostName != "/" && turl.Spec.RabbitmqClusterName != "" { requests = append(requests, reconcile.Request{ NamespacedName: types.NamespacedName{ - Name: vhostName, + Name: rabbitmqv1.CanonicalVhostName(turl.Spec.RabbitmqClusterName, vhostName), Namespace: turl.Namespace, }, }) diff --git a/internal/controller/rabbitmq/transporturl_controller.go b/internal/controller/rabbitmq/transporturl_controller.go index 2b5fd1bd..86cf0b9f 100644 --- a/internal/controller/rabbitmq/transporturl_controller.go +++ b/internal/controller/rabbitmq/transporturl_controller.go @@ -26,10 +26,8 @@ import ( "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -76,332 +74,12 @@ func (r *TransportURLReconciler) GetLogger(ctx context.Context) logr.Logger { return log.FromContext(ctx).WithName("Controllers").WithName("TransportURL") } -// Constants for old user cleanup -const ( - // Grace period after removing TransportURL finalizer before deleting user. - // This ensures the owner service has time to reconcile with new credentials - // before the old user is removed from RabbitMQ. - userCleanupGracePeriod = 30 * time.Second - - // Requeue interval when waiting for owner service to be ready - ownerReadinessCheckInterval = 10 * time.Second - - // Annotation key for tracking when the TransportURL finalizer was removed - finalizerRemovedAtAnnotation = "rabbitmq.openstack.org/finalizer-removed-at" -) - -// isOwnerServiceReady checks if the owner service (Cinder, Nova, etc.) that owns this TransportURL is ready. -// Returns: -// - ready: true if the owner is ready, false if not ready -// - observedGen: the owner's observedGeneration (0 if no owner or not available) -// - error: only for unexpected failures -// -// If there's no owner with controller=true, it returns (true, 0, nil). -func (r *TransportURLReconciler) isOwnerServiceReady(ctx context.Context, instance *rabbitmqv1.TransportURL) (ready bool, observedGen int64, err error) { - Log := log.FromContext(ctx) - - // Find the controller owner reference (e.g., Cinder, Nova, etc.) - var ownerRef *metav1.OwnerReference - for _, owner := range instance.GetOwnerReferences() { - if owner.Controller != nil && *owner.Controller { - ownerRef = &owner - break - } - } - - // If no controlling owner, return ready - if ownerRef == nil { - Log.Info("No controller owner found") - return true, 0, nil - } - - // Parse the APIVersion to extract group and version - gv, err := schema.ParseGroupVersion(ownerRef.APIVersion) - if err != nil { - return false, 0, fmt.Errorf("failed to parse owner APIVersion %s: %w", ownerRef.APIVersion, err) - } - - // Fetch the owner resource using unstructured client - owner := &unstructured.Unstructured{} - owner.SetGroupVersionKind(schema.GroupVersionKind{ - Group: gv.Group, - Version: gv.Version, - Kind: ownerRef.Kind, - }) - - err = r.Get(ctx, types.NamespacedName{ - Name: ownerRef.Name, - Namespace: instance.Namespace, - }, owner) - - if err != nil { - if k8s_errors.IsNotFound(err) { - // Owner deleted, return ready - Log.Info("Owner resource not found", "kind", ownerRef.Kind, "name", ownerRef.Name) - return true, 0, nil - } - // Unexpected error, return error - return false, 0, fmt.Errorf("failed to fetch owner resource %s/%s: %w", ownerRef.Kind, ownerRef.Name, err) - } - - // Check status.conditions for Ready condition - conditions, found, err := unstructured.NestedSlice(owner.Object, "status", "conditions") - if err != nil || !found { - Log.Info("No conditions found in owner status, waiting", "kind", ownerRef.Kind, "name", ownerRef.Name) - return false, 0, nil - } - - // Look for Ready condition with status=True - isReady := false - for _, c := range conditions { - condition, ok := c.(map[string]any) - if !ok { - continue - } - - condType, _, _ := unstructured.NestedString(condition, "type") - status, _, _ := unstructured.NestedString(condition, "status") - - if condType == "Ready" && status == "True" { - isReady = true - break - } - } - - if !isReady { - Log.Info("Owner service not ready, waiting before deleting old user", "kind", ownerRef.Kind, "name", ownerRef.Name) - return false, 0, nil - } - - // Check if owner has reconciled (observedGeneration matches generation) - generation, foundGen, err := unstructured.NestedInt64(owner.Object, "metadata", "generation") - if err != nil || !foundGen { - Log.Info("Could not get owner generation, waiting", "kind", ownerRef.Kind, "name", ownerRef.Name) - return false, 0, nil - } - - observedGeneration, foundObsGen, err := unstructured.NestedInt64(owner.Object, "status", "observedGeneration") - if err != nil || !foundObsGen { - Log.Info("Could not get owner observedGeneration, waiting", "kind", ownerRef.Kind, "name", ownerRef.Name) - return false, 0, nil - } - - if observedGeneration != generation { - Log.Info("Owner service has not reconciled yet (observedGeneration != generation), waiting", - "kind", ownerRef.Kind, - "name", ownerRef.Name, - "generation", generation, - "observedGeneration", observedGeneration) - return false, 0, nil - } - - Log.Info("Owner service is ready and has reconciled", - "kind", ownerRef.Kind, - "name", ownerRef.Name, - "observedGeneration", observedGeneration) - return true, observedGeneration, nil -} - -// cleanupOldUser handles the cleanup of an old RabbitMQUser when credentials are rotated. -// It removes the TransportURL finalizer and deletes the user after the owner service has reconciled. -// -// State machine: -// 1. Owner service not ready → wait and requeue -// 2. Has TransportURL finalizer → remove finalizer, set timestamp, requeue -// 3. Grace period not elapsed → wait for remaining time -// 4. External finalizers present → skip deletion (let external controller handle it) -// 5. Grace period elapsed → delete user -// -// Returns the requeue duration (0 if no requeue needed) and any error encountered. -func (r *TransportURLReconciler) cleanupOldUser( - ctx context.Context, - instance *rabbitmqv1.TransportURL, - oldUserName types.NamespacedName, -) (requeueAfter time.Duration, err error) { - Log := r.GetLogger(ctx) - - // Always fetch fresh user data from API server - // The oldUserName comes from a List operation which may be stale - user := &rabbitmqv1.RabbitMQUser{} - if err := r.Get(ctx, oldUserName, user); err != nil { - if k8s_errors.IsNotFound(err) { - // User was already deleted - success - Log.Info("Old user already deleted", "user", oldUserName.Name) - return 0, nil - } - return 0, fmt.Errorf("failed to get user %s for cleanup: %w", oldUserName.Name, err) - } - - // Check if owner service is ready with new credentials - // We only proceed with cleanup after the owner has switched to the new user - ownerReady, _, err := r.isOwnerServiceReady(ctx, instance) - if err != nil { - return 0, fmt.Errorf("failed to check owner service readiness for user %s: %w", user.Name, err) - } - if !ownerReady { - Log.Info("Waiting for owner service to be ready before cleanup", "user", user.Name) - return ownerReadinessCheckInterval, nil - } - - // State machine based on whether TransportURL finalizer is present - hasTransportURLFinalizer := controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizer) - - if hasTransportURLFinalizer { - // Phase 1: Remove finalizer and start grace period - return r.startUserCleanupGracePeriod(ctx, user) - } - - // Phase 2: Wait for grace period, then delete - return r.deleteUserAfterGracePeriod(ctx, instance, user) -} - -// startUserCleanupGracePeriod removes the TransportURL finalizer and marks the timestamp -// when cleanup started. Returns the requeue duration (grace period) and any error. -func (r *TransportURLReconciler) startUserCleanupGracePeriod( - ctx context.Context, - user *rabbitmqv1.RabbitMQUser, -) (time.Duration, error) { - Log := r.GetLogger(ctx) - - // Remove TransportURL finalizer - controllerutil.RemoveFinalizer(user, rabbitmqv1.TransportURLFinalizer) - - // Set timestamp annotation to track when grace period started - if user.Annotations == nil { - user.Annotations = make(map[string]string) - } - user.Annotations[finalizerRemovedAtAnnotation] = time.Now().UTC().Format(time.RFC3339) - - // Update the user - if err := r.Update(ctx, user); err != nil { - if k8s_errors.IsNotFound(err) { - // User was deleted before we could update - that's fine - Log.Info("User deleted before finalizer could be removed", "user", user.Name) - return 0, nil - } - return 0, fmt.Errorf("failed to remove TransportURL finalizer from user %s: %w", user.Name, err) - } - - Log.Info("Removed TransportURL finalizer, starting grace period", - "user", user.Name, - "gracePeriod", userCleanupGracePeriod) - - // Requeue after the grace period - return userCleanupGracePeriod, nil -} - -// deleteUserAfterGracePeriod checks if the grace period has elapsed and deletes the user. -// Returns the requeue duration (remaining grace period) or 0 if deletion succeeded. -func (r *TransportURLReconciler) deleteUserAfterGracePeriod( - ctx context.Context, - instance *rabbitmqv1.TransportURL, - user *rabbitmqv1.RabbitMQUser, -) (time.Duration, error) { - Log := r.GetLogger(ctx) - - // Check for external finalizers - if present, we can't delete - if hasExternalFinalizers(user) { - Log.Info("External finalizers present, skipping deletion", - "user", user.Name, - "finalizers", user.GetFinalizers()) - return 0, nil - } - - // Check if grace period has elapsed - remaining, hasTimestamp := r.checkGracePeriod(ctx, user) - - if !hasTimestamp { - // Finalizer was removed but no timestamp set (shouldn't happen, but handle it) - // Set timestamp now and requeue - Log.Info("Finalizer removed but no timestamp found, setting it now", "user", user.Name) - return r.startUserCleanupGracePeriod(ctx, user) - } - - if remaining > 0 { - // Still in grace period - wait - Log.Info("Waiting for grace period before deletion", - "user", user.Name, - "remaining", remaining.Round(time.Second)) - return remaining, nil - } - - // Grace period elapsed - re-verify owner is still ready before deletion - // This ensures we don't delete the user if owner becomes not-ready during grace period - ownerReady, _, err := r.isOwnerServiceReady(ctx, instance) - if err != nil { - return 0, fmt.Errorf("failed to check owner service readiness before deletion for user %s: %w", user.Name, err) - } - if !ownerReady { - Log.Info("Owner service no longer ready, waiting before deletion", "user", user.Name) - return ownerReadinessCheckInterval, nil - } - - // Grace period elapsed and owner is ready - delete the user - Log.Info("Grace period elapsed, deleting old user", - "user", user.Name, - "gracePeriod", userCleanupGracePeriod) - - if err := r.Delete(ctx, user); err != nil { - if k8s_errors.IsNotFound(err) { - // Already deleted - success - Log.Info("User already deleted", "user", user.Name) - return 0, nil - } - return 0, fmt.Errorf("failed to delete user %s: %w", user.Name, err) - } - - Log.Info("Successfully deleted old user", "user", user.Name) - return 0, nil -} - -// checkGracePeriod checks if the grace period has elapsed based on the annotation timestamp. -// Returns: -// - remaining: time remaining in grace period (0 if elapsed or parse error) -// - hasTimestamp: whether a valid timestamp annotation exists -func (r *TransportURLReconciler) checkGracePeriod(ctx context.Context, user *rabbitmqv1.RabbitMQUser) (remaining time.Duration, hasTimestamp bool) { - Log := r.GetLogger(ctx) - - timestampStr, ok := user.Annotations[finalizerRemovedAtAnnotation] - if !ok { - return 0, false - } - - // Parse the timestamp - removedAt, err := time.Parse(time.RFC3339, timestampStr) - if err != nil { - // Can't parse timestamp - fail open (allow deletion) - Log.Error(err, "Failed to parse grace period timestamp, allowing deletion", - "user", user.Name, - "timestamp", timestampStr) - return 0, true - } - - // Calculate remaining time - elapsed := time.Since(removedAt) - if elapsed >= userCleanupGracePeriod { - return 0, true // Grace period elapsed - } - - return userCleanupGracePeriod - elapsed, true -} - -// hasExternalFinalizers returns true if the user has any non-internal finalizers. -// External finalizers indicate that another controller needs to perform cleanup. -func hasExternalFinalizers(user *rabbitmqv1.RabbitMQUser) bool { - for _, finalizer := range user.GetFinalizers() { - if !rabbitmqv1.IsInternalFinalizer(finalizer) { - return true - } - } - return false -} - //+kubebuilder:rbac:groups=rabbitmq.openstack.org,resources=transporturls,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=rabbitmq.openstack.org,resources=transporturls/status,verbs=get;update;patch //+kubebuilder:rbac:groups=rabbitmq.openstack.org,resources=transporturls/finalizers,verbs=update //+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 +//+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; @@ -499,9 +177,6 @@ func (r *TransportURLReconciler) reconcileNormal(ctx context.Context, instance * Log := r.GetLogger(ctx) Log.Info("Reconciling Service") - // Track if we need to requeue for old user cleanup - var requeueAfter time.Duration - // Get RabbitMQ cluster rabbit, err := getRabbitmqCluster(ctx, helper, instance) if err != nil { @@ -583,6 +258,15 @@ func (r *TransportURLReconciler) reconcileNormal(ctx context.Context, instance * return ctrl.Result{}, err } + // Migration: clean up legacy per-TransportURL owned CRs (remove old finalizers). + // Legacy CRs are kept alive (owner ref to TransportURL) until the TransportURL is deleted, + // at which point Kubernetes GC deletes them and the user controller's safety check prevents + // the RabbitMQ user from being deleted (the canonical CR still exists). + if _, err := r.cleanupLegacyOwnedResources(ctx, instance); err != nil { + Log.Error(err, "Failed to clean up legacy owned resources") + return ctrl.Result{}, err + } + // Determine credentials and vhost var finalUsername, finalPassword, vhostName string var userRef, vhostRef string @@ -596,21 +280,19 @@ func (r *TransportURLReconciler) reconcileNormal(ctx context.Context, instance * vhostName = "/" } if vhostName != "/" { - vhostRef = fmt.Sprintf("%s-%s-vhost", instance.Name, vhostName) + vhostRef = rabbitmqv1.CanonicalVhostName(instance.Spec.RabbitmqClusterName, vhostName) + perConsumerFinalizer := rabbitmqv1.TransportURLFinalizerFor(instance.Name) vhost := &rabbitmqv1.RabbitMQVhost{ ObjectMeta: metav1.ObjectMeta{ Name: vhostRef, Namespace: instance.Namespace, }, } - // Note: During normal reconciliation (not deletion), we return errors rather than - // just logging them, as we need these operations to succeed for correct functionality. if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, vhost, func() error { - if err := controllerutil.SetControllerReference(instance, vhost, r.Scheme); err != nil { + controllerutil.AddFinalizer(vhost, perConsumerFinalizer) + if err := controllerutil.SetOwnerReference(rabbit, vhost, r.Scheme); err != nil { return err } - // AddFinalizer is idempotent - safe to call even if finalizer already exists - controllerutil.AddFinalizer(vhost, rabbitmqv1.TransportURLFinalizer) vhost.Spec.RabbitmqClusterName = instance.Spec.RabbitmqClusterName vhost.Spec.Name = vhostName return nil @@ -620,8 +302,8 @@ func (r *TransportURLReconciler) reconcileNormal(ctx context.Context, instance * } } - // Create RabbitMQUser - use username in resource name for blue/green rotation - userRef = fmt.Sprintf("%s-%s-user", instance.Name, instance.Spec.Username) + userRef = rabbitmqv1.CanonicalUserName(instance.Spec.RabbitmqClusterName, vhostName, instance.Spec.Username) + perConsumerUserFinalizer := rabbitmqv1.TransportURLFinalizerFor(instance.Name) user := &rabbitmqv1.RabbitMQUser{ ObjectMeta: metav1.ObjectMeta{ Name: userRef, @@ -629,15 +311,17 @@ func (r *TransportURLReconciler) reconcileNormal(ctx context.Context, instance * }, } if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, user, func() error { - if err := controllerutil.SetControllerReference(instance, user, r.Scheme); err != nil { + controllerutil.AddFinalizer(user, perConsumerUserFinalizer) + controllerutil.AddFinalizer(user, rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer) + // Clear orphaned label — this consumer is (re)claiming the user CR + labels := user.GetLabels() + if labels != nil { + delete(labels, rabbitmqv1.RabbitMQUserOrphanedLabel) + user.SetLabels(labels) + } + if err := controllerutil.SetOwnerReference(rabbit, user, r.Scheme); err != nil { return err } - // Add TransportURL finalizer to protect user while in use - controllerutil.AddFinalizer(user, rabbitmqv1.TransportURLFinalizer) - // Add temporary blocking finalizer to prevent automatic cleanup - // This ensures users are not deleted automatically during credential rotation - // until proper safe-to-delete logic is implemented - controllerutil.AddFinalizer(user, rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer) user.Spec.RabbitmqClusterName = instance.Spec.RabbitmqClusterName user.Spec.Username = instance.Spec.Username user.Spec.VhostRef = vhostRef @@ -654,68 +338,35 @@ func (r *TransportURLReconciler) reconcileNormal(ctx context.Context, instance * } - // Remove TransportURL finalizer from previously used users that are no longer referenced - // This handles credential rotation, rollback scenarios, and removal of custom users - // This cleanup runs unconditionally to handle the case where a custom user is removed - // from the spec and the TransportURL switches to default admin credentials - userList := &rabbitmqv1.RabbitMQUserList{} - if err := r.List(ctx, userList, client.InNamespace(instance.Namespace)); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to list RabbitMQUsers: %w", err) - } - for i := range userList.Items { - oldUser := &userList.Items[i] - isOwned := object.CheckOwnerRefExist(instance.GetUID(), oldUser.GetOwnerReferences()) - - // If owned by this TransportURL but not the current user, handle cleanup - // When userRef is empty (using default credentials), all owned users should be cleaned up - if isOwned && oldUser.Name != userRef { - // Clean up old user - pass NamespacedName instead of full object - // This ensures we always fetch fresh data inside cleanupOldUser - oldUserName := types.NamespacedName{Name: oldUser.Name, Namespace: oldUser.Namespace} - userRequeueAfter, err := r.cleanupOldUser(ctx, instance, oldUserName) - if err != nil { - return ctrl.Result{}, fmt.Errorf("failed to cleanup old user %s: %w", oldUser.Name, err) - } - if userRequeueAfter > 0 && (requeueAfter == 0 || userRequeueAfter < requeueAfter) { - requeueAfter = userRequeueAfter - } - } + // Delete legacy CRs now that the canonical ones exist. + // The user/vhost controller safety checks will find the canonical CRs and skip the + // RabbitMQ API delete. This must run AFTER canonical CR creation above. + if err := r.deleteLegacyOwnedResources(ctx, instance); err != nil { + Log.Error(err, "Failed to delete legacy owned resources") } - // Remove TransportURL finalizer from owned vhosts that are being deleted - // but only if they're no longer referenced in the TransportURL spec - // This handles vhost removal when the vhost definition is removed from the spec - vhostList := &rabbitmqv1.RabbitMQVhostList{} - if err := r.List(ctx, vhostList, client.InNamespace(instance.Namespace)); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to list RabbitMQVhosts: %w", err) - } - for i := range vhostList.Items { - oldVhost := &vhostList.Items[i] - - // Skip vhosts not owned by this TransportURL - isOwned := object.CheckOwnerRefExist(instance.GetUID(), oldVhost.GetOwnerReferences()) - if !isOwned { - continue - } - - // Skip vhosts that are not being deleted - if oldVhost.DeletionTimestamp.IsZero() { - continue - } - - // Skip the current vhost - only clean up orphaned vhosts from previous specs - if oldVhost.Name == vhostRef { - continue + // Remove per-consumer finalizer from previously used shared user/vhost CRs + // when the TransportURL's spec changes (credential rotation, vhost change, etc.) + perConsumerFin := rabbitmqv1.TransportURLFinalizerFor(instance.Name) + if instance.Status.RabbitmqUserRef != "" && instance.Status.RabbitmqUserRef != userRef { + if err := r.removeFinalizerFromCR(ctx, &rabbitmqv1.RabbitMQUser{}, instance.Status.RabbitmqUserRef, instance.Namespace, perConsumerFin); err != nil { + return ctrl.Result{}, err } - - // Remove finalizer if present - if !controllerutil.RemoveFinalizer(oldVhost, rabbitmqv1.TransportURLFinalizer) { - continue + // If no other TransportURL uses this user, mark it as orphaned. + // The cleanup-blocked finalizer stays, requiring admin approval before deletion. + if err := r.markUserOrphaned(ctx, instance.Status.RabbitmqUserRef, instance.Namespace); err != nil { + return ctrl.Result{}, err } - - // Update the vhost - if err := r.Update(ctx, oldVhost); err != nil && !k8s_errors.IsNotFound(err) { - return ctrl.Result{}, fmt.Errorf("failed to remove TransportURL finalizer from old vhost %s: %w", oldVhost.Name, err) + } + if instance.Status.RabbitmqVhost != "" && instance.Status.RabbitmqVhost != "/" { + oldVhostRef := rabbitmqv1.CanonicalVhostName(instance.Spec.RabbitmqClusterName, instance.Status.RabbitmqVhost) + if oldVhostRef != vhostRef { + if err := r.removeFinalizerFromCR(ctx, &rabbitmqv1.RabbitMQVhost{}, oldVhostRef, instance.Namespace, perConsumerFin); err != nil { + return ctrl.Result{}, err + } + if err := r.deleteOrphanedCR(ctx, &rabbitmqv1.RabbitMQVhost{}, oldVhostRef, instance.Namespace); err != nil { + return ctrl.Result{}, err + } } } @@ -750,8 +401,20 @@ func (r *TransportURLReconciler) reconcileNormal(ctx context.Context, instance * instance.Status.Conditions.Set(condition.FalseCondition(rabbitmqv1.TransportURLReadyCondition, condition.ErrorReason, condition.SeverityWarning, rabbitmqv1.TransportURLReadyErrorMessage, err.Error())) return ctrl.Result{}, err } - finalUsername = string(userSecret.Data["username"]) - finalPassword = string(userSecret.Data["password"]) + usernameBytes, ok := userSecret.Data["username"] + if !ok { + err := fmt.Errorf("username key not found in user secret %s", rabbitUser.Status.SecretName) + instance.Status.Conditions.Set(condition.FalseCondition(rabbitmqv1.TransportURLReadyCondition, condition.ErrorReason, condition.SeverityWarning, rabbitmqv1.TransportURLReadyErrorMessage, err.Error())) + return ctrl.Result{}, err + } + passwordBytes, ok := userSecret.Data["password"] + if !ok { + err := fmt.Errorf("password key not found in user secret %s", rabbitUser.Status.SecretName) + instance.Status.Conditions.Set(condition.FalseCondition(rabbitmqv1.TransportURLReadyCondition, condition.ErrorReason, condition.SeverityWarning, rabbitmqv1.TransportURLReadyErrorMessage, err.Error())) + return ctrl.Result{}, err + } + finalUsername = string(usernameBytes) + finalPassword = string(passwordBytes) vhostName = rabbitUser.Status.Vhost if vhostName == "" { vhostName = "/" @@ -872,13 +535,10 @@ func (r *TransportURLReconciler) reconcileNormal(ctx context.Context, instance * } Log.Info("Reconciled Service successfully") - // If we skipped any old user cleanup due to grace period or owner readiness, - // schedule a requeue to check again later - if requeueAfter > 0 { - Log.Info("Scheduling requeue for old user cleanup", "after", requeueAfter.String()) - return ctrl.Result{RequeueAfter: requeueAfter}, nil - } - return ctrl.Result{}, nil + // Periodic requeue ensures finalizer cleanup converges even when the + // watch-based reverse mapping fails (TransportURL names >61 chars are + // truncated+hashed, making the reverse mapping incorrect). + return ctrl.Result{RequeueAfter: 5 * time.Minute}, nil } // Create k8s secret with transport URL @@ -941,50 +601,66 @@ func (r *TransportURLReconciler) reconcileDelete(ctx context.Context, instance * Log := r.GetLogger(ctx) Log.Info("Reconciling delete") - // Remove TransportURL finalizers from all owned users and vhosts - userList := &rabbitmqv1.RabbitMQUserList{} - if err := r.List(ctx, userList, client.InNamespace(instance.Namespace)); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to list RabbitMQUsers: %w", err) - } - for i := range userList.Items { - user := &userList.Items[i] - // Check if this user is owned by this TransportURL - isOwned := object.CheckOwnerRefExist(instance.GetUID(), user.GetOwnerReferences()) - if isOwned { - // Remove both the TransportURL finalizer and the cleanup-blocked finalizer - // When TransportURL is deleted, we want to allow full cleanup of owned users + perConsumerFinalizer := rabbitmqv1.TransportURLFinalizerFor(instance.Name) + + // Remove per-consumer finalizer from shared user CR + if instance.Spec.Username != "" { + vhostName := instance.Spec.Vhost + if vhostName == "" { + vhostName = "/" + } + userRef := rabbitmqv1.CanonicalUserName(instance.Spec.RabbitmqClusterName, vhostName, instance.Spec.Username) + user := &rabbitmqv1.RabbitMQUser{} + if err := r.Get(ctx, types.NamespacedName{Name: userRef, Namespace: instance.Namespace}, user); err == nil { updated := false - if controllerutil.RemoveFinalizer(user, rabbitmqv1.TransportURLFinalizer) { + if controllerutil.RemoveFinalizer(user, perConsumerFinalizer) { updated = true } if controllerutil.RemoveFinalizer(user, rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer) { updated = true } + // Clear orphaned label since we're explicitly releasing this user + labels := user.GetLabels() + if labels != nil && labels[rabbitmqv1.RabbitMQUserOrphanedLabel] == "true" { + delete(labels, rabbitmqv1.RabbitMQUserOrphanedLabel) + user.SetLabels(labels) + updated = true + } if updated { - if err := r.Update(ctx, user); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to remove finalizers from user %s: %w", user.Name, err) + if err := r.Update(ctx, user); err != nil && !k8s_errors.IsNotFound(err) { + return ctrl.Result{}, fmt.Errorf("failed to remove finalizers from user %s: %w", userRef, err) } } + // Mark orphaned if no other consumers remain — user controller will auto-delete + // since cleanup-blocked was removed above. + if err := r.markUserOrphaned(ctx, userRef, instance.Namespace); err != nil { + return ctrl.Result{}, err + } + } else if !k8s_errors.IsNotFound(err) { + return ctrl.Result{}, fmt.Errorf("failed to get user %s for cleanup: %w", userRef, err) } - } - vhostList := &rabbitmqv1.RabbitMQVhostList{} - if err := r.List(ctx, vhostList, client.InNamespace(instance.Namespace)); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to list RabbitMQVhosts: %w", err) - } - for i := range vhostList.Items { - vhost := &vhostList.Items[i] - // Check if this vhost is owned by this TransportURL - isOwned := object.CheckOwnerRefExist(instance.GetUID(), vhost.GetOwnerReferences()) - if isOwned { - if controllerutil.RemoveFinalizer(vhost, rabbitmqv1.TransportURLFinalizer) { - if err := r.Update(ctx, vhost); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to remove TransportURL finalizer from vhost %s: %w", vhost.Name, err) + // Remove per-consumer finalizer from shared vhost CR + if vhostName != "/" { + vhostCRName := rabbitmqv1.CanonicalVhostName(instance.Spec.RabbitmqClusterName, vhostName) + vhost := &rabbitmqv1.RabbitMQVhost{} + if err := r.Get(ctx, types.NamespacedName{Name: vhostCRName, Namespace: instance.Namespace}, vhost); err == nil { + if controllerutil.RemoveFinalizer(vhost, perConsumerFinalizer) { + if err := r.Update(ctx, vhost); err != nil && !k8s_errors.IsNotFound(err) { + return ctrl.Result{}, fmt.Errorf("failed to remove finalizer from vhost %s: %w", vhostCRName, err) + } } + } else if !k8s_errors.IsNotFound(err) { + return ctrl.Result{}, fmt.Errorf("failed to get vhost %s for cleanup: %w", vhostCRName, err) } } } + // Migration: clean up legacy per-TransportURL owned CRs + if _, err := r.cleanupLegacyOwnedResources(ctx, instance); err != nil { + Log.Error(err, "Failed to clean up legacy owned resources") + } + // Remove own finalizer to allow deletion controllerutil.RemoveFinalizer(instance, transportURLFinalizer) return ctrl.Result{}, nil @@ -1011,8 +687,14 @@ func (r *TransportURLReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&rabbitmqv1.TransportURL{}). Owns(&corev1.Secret{}). - Owns(&rabbitmqv1.RabbitMQUser{}). - Owns(&rabbitmqv1.RabbitMQVhost{}). + Watches( + &rabbitmqv1.RabbitMQUser{}, + handler.EnqueueRequestsFromMapFunc(r.findTransportURLsFromFinalizers), + ). + Watches( + &rabbitmqv1.RabbitMQVhost{}, + handler.EnqueueRequestsFromMapFunc(r.findTransportURLsFromFinalizers), + ). Watches( &rabbitmqv1.RabbitMq{}, handler.EnqueueRequestsFromMapFunc(r.findObjectsForSrc), @@ -1055,6 +737,247 @@ func (r *TransportURLReconciler) findObjectsForSrc(ctx context.Context, src clie return requests } +// findTransportURLsFromFinalizers maps changes on shared vhost/user CRs back to the +// TransportURLs that have per-consumer finalizers on them. +// Limitation: for TransportURL names >61 chars, TransportURLFinalizerFor truncates+hashes +// the name, so the reverse mapping here produces incorrect names. Those TransportURLs +// rely on requeue timers rather than this watch for reconciliation. +func (r *TransportURLReconciler) findTransportURLsFromFinalizers(_ context.Context, obj client.Object) []reconcile.Request { + var requests []reconcile.Request + for _, finalizer := range obj.GetFinalizers() { + if strings.HasPrefix(finalizer, rabbitmqv1.TransportURLFinalizerPrefix) { + turlName := finalizer[len(rabbitmqv1.TransportURLFinalizerPrefix):] + requests = append(requests, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: turlName, + Namespace: obj.GetNamespace(), + }, + }) + } + } + return requests +} + +// deleteOrphanedCR deletes a shared user/vhost CR if no active consumer finalizers remain. +// Consumer finalizers are per-TransportURL (turl.openstack.org/t-*) and per-user on vhosts +// (rmquser.openstack.org/u-*). External finalizers do NOT block the Delete call — the +// user/vhost controllers handle waiting for external finalizers during their own reconcileDelete. +func (r *TransportURLReconciler) deleteOrphanedCR(ctx context.Context, obj client.Object, name, namespace string) error { + Log := r.GetLogger(ctx) + if err := r.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, obj); err != nil { + if k8s_errors.IsNotFound(err) { + return nil + } + return fmt.Errorf("failed to get %s: %w", name, err) + } + if !obj.GetDeletionTimestamp().IsZero() { + return nil + } + for _, f := range obj.GetFinalizers() { + if strings.HasPrefix(f, rabbitmqv1.TransportURLFinalizerPrefix) { + return nil + } + if strings.HasPrefix(f, rabbitmqv1.UserVhostFinalizerPrefix) { + Log.Info("Skipping deletion of vhost CR still referenced by user", "name", name, "finalizer", f) + return nil + } + } + Log.Info("Deleting orphaned CR (no remaining consumers)", "name", name) + if err := r.Delete(ctx, obj); err != nil && !k8s_errors.IsNotFound(err) { + return fmt.Errorf("failed to delete orphaned CR %s: %w", name, err) + } + return nil +} + +// markUserOrphaned labels a shared RabbitMQUser CR as orphaned when no active consumer finalizers remain. +// Unlike deleteOrphanedCR (which issues a Delete), this sets a label so the CR stays reclaimable +// by new consumers. The user controller handles actual deletion once an admin removes the +// cleanup-blocked finalizer. +func (r *TransportURLReconciler) markUserOrphaned(ctx context.Context, name, namespace string) error { + Log := r.GetLogger(ctx) + user := &rabbitmqv1.RabbitMQUser{} + if err := r.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, user); err != nil { + if k8s_errors.IsNotFound(err) { + return nil + } + return fmt.Errorf("failed to get user %s: %w", name, err) + } + if !user.DeletionTimestamp.IsZero() { + return nil + } + for _, f := range user.GetFinalizers() { + if strings.HasPrefix(f, rabbitmqv1.TransportURLFinalizerPrefix) { + return nil + } + } + labels := user.GetLabels() + if labels == nil { + labels = map[string]string{} + } + if labels[rabbitmqv1.RabbitMQUserOrphanedLabel] == "true" { + return nil + } + labels[rabbitmqv1.RabbitMQUserOrphanedLabel] = "true" + user.SetLabels(labels) + Log.Info("Marking user CR as orphaned (no remaining consumers)", "name", name) + if err := r.Update(ctx, user); err != nil && !k8s_errors.IsNotFound(err) { + return fmt.Errorf("failed to mark user %s as orphaned: %w", name, err) + } + return nil +} + +// removeFinalizerFromCR removes a finalizer from a CR by name. NotFound is not an error. +func (r *TransportURLReconciler) removeFinalizerFromCR(ctx context.Context, obj client.Object, name, namespace, finalizer string) error { + if err := r.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, obj); err != nil { + if k8s_errors.IsNotFound(err) { + return nil + } + return fmt.Errorf("failed to get %s for finalizer removal: %w", name, err) + } + if controllerutil.RemoveFinalizer(obj, finalizer) { + if err := r.Update(ctx, obj); err != nil && !k8s_errors.IsNotFound(err) { + return fmt.Errorf("failed to remove finalizer %s from %s: %w", finalizer, name, err) + } + } + return nil +} + +// cleanupLegacyOwnedResources removes legacy TransportURL finalizers from CRs that were +// created with the old per-TransportURL naming scheme (owner-ref based). +func (r *TransportURLReconciler) cleanupLegacyOwnedResources(ctx context.Context, instance *rabbitmqv1.TransportURL) (bool, error) { + Log := r.GetLogger(ctx) + cleaned := false + + userList := &rabbitmqv1.RabbitMQUserList{} + if err := r.List(ctx, userList, client.InNamespace(instance.Namespace)); err != nil { + return false, fmt.Errorf("failed to list users for legacy cleanup: %w", err) + } + for i := range userList.Items { + user := &userList.Items[i] + if !object.CheckOwnerRefExist(instance.GetUID(), user.GetOwnerReferences()) { + continue + } + updated := false + if controllerutil.RemoveFinalizer(user, rabbitmqv1.TransportURLFinalizer) { + updated = true + } + if controllerutil.RemoveFinalizer(user, rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer) { + updated = true + } + if updated { + Log.Info("Cleaning up legacy owned user", "user", user.Name) + if err := r.Update(ctx, user); err != nil && !k8s_errors.IsNotFound(err) { + return false, fmt.Errorf("failed to clean up legacy user %s: %w", user.Name, err) + } + cleaned = true + } + + // Migrate credentials: copy the legacy user's secret to the canonical secret name + // so the canonical CR uses the same password and doesn't break existing connections. + if user.Status.SecretName != "" { + vhostName := instance.Spec.Vhost + if vhostName == "" { + vhostName = "/" + } + canonicalUserRef := rabbitmqv1.CanonicalUserName(instance.Spec.RabbitmqClusterName, vhostName, user.Spec.Username) + canonicalSecretName := fmt.Sprintf("rabbitmq-user-%s", canonicalUserRef) + if user.Status.SecretName != canonicalSecretName { + legacySecret := &corev1.Secret{} + if err := r.Get(ctx, types.NamespacedName{Name: user.Status.SecretName, Namespace: instance.Namespace}, legacySecret); err == nil { + canonicalSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: canonicalSecretName, + Namespace: instance.Namespace, + }, + } + legacyUsername, uOk := legacySecret.Data["username"] + legacyPassword, pOk := legacySecret.Data["password"] + if uOk && pOk { + if err := r.Get(ctx, types.NamespacedName{Name: canonicalSecretName, Namespace: instance.Namespace}, canonicalSecret); err != nil { + if k8s_errors.IsNotFound(err) { + canonicalSecret.Data = map[string][]byte{ + "username": legacyUsername, + "password": legacyPassword, + } + if err := r.Create(ctx, canonicalSecret); err != nil && !k8s_errors.IsAlreadyExists(err) { + Log.Error(err, "Failed to pre-create canonical secret with legacy credentials", "secret", canonicalSecretName) + } else { + Log.Info("Pre-created canonical secret with legacy credentials", "legacy", user.Status.SecretName, "canonical", canonicalSecretName) + } + } + } + } else { + Log.Info("Skipping credential migration - legacy secret missing username/password keys", "secret", user.Status.SecretName) + } + } + } + } + } + + vhostList := &rabbitmqv1.RabbitMQVhostList{} + if err := r.List(ctx, vhostList, client.InNamespace(instance.Namespace)); err != nil { + return false, fmt.Errorf("failed to list vhosts for legacy cleanup: %w", err) + } + for i := range vhostList.Items { + vhost := &vhostList.Items[i] + if !object.CheckOwnerRefExist(instance.GetUID(), vhost.GetOwnerReferences()) { + continue + } + if controllerutil.RemoveFinalizer(vhost, rabbitmqv1.TransportURLFinalizer) { + Log.Info("Cleaning up legacy owned vhost", "vhost", vhost.Name) + if err := r.Update(ctx, vhost); err != nil && !k8s_errors.IsNotFound(err) { + return false, fmt.Errorf("failed to clean up legacy vhost %s: %w", vhost.Name, err) + } + cleaned = true + } + } + + return cleaned, nil +} + +// deleteLegacyOwnedResources explicitly deletes legacy per-TransportURL owned CRs. +// This must be called AFTER the canonical CRs are created, so the user/vhost controller +// safety checks find the canonical CR and skip the RabbitMQ API delete. +func (r *TransportURLReconciler) deleteLegacyOwnedResources(ctx context.Context, instance *rabbitmqv1.TransportURL) error { + Log := r.GetLogger(ctx) + + userList := &rabbitmqv1.RabbitMQUserList{} + if err := r.List(ctx, userList, client.InNamespace(instance.Namespace)); err != nil { + return fmt.Errorf("failed to list users for legacy deletion: %w", err) + } + for i := range userList.Items { + user := &userList.Items[i] + if !object.CheckOwnerRefExist(instance.GetUID(), user.GetOwnerReferences()) { + continue + } + if user.DeletionTimestamp.IsZero() { + Log.Info("Deleting legacy owned user", "user", user.Name) + if err := r.Delete(ctx, user); err != nil && !k8s_errors.IsNotFound(err) { + return fmt.Errorf("failed to delete legacy user %s: %w", user.Name, err) + } + } + } + + vhostList := &rabbitmqv1.RabbitMQVhostList{} + if err := r.List(ctx, vhostList, client.InNamespace(instance.Namespace)); err != nil { + return fmt.Errorf("failed to list vhosts for legacy deletion: %w", err) + } + for i := range vhostList.Items { + vhost := &vhostList.Items[i] + if !object.CheckOwnerRefExist(instance.GetUID(), vhost.GetOwnerReferences()) { + continue + } + if vhost.DeletionTimestamp.IsZero() { + Log.Info("Deleting legacy owned vhost", "vhost", vhost.Name) + if err := r.Delete(ctx, vhost); err != nil && !k8s_errors.IsNotFound(err) { + return fmt.Errorf("failed to delete legacy vhost %s: %w", vhost.Name, err) + } + } + } + + return nil +} + // GetRabbitmqCluster - get RabbitMq object in namespace func getRabbitmqCluster( ctx context.Context, diff --git a/internal/webhook/rabbitmq/v1beta1/rabbitmquser_webhook.go b/internal/webhook/rabbitmq/v1beta1/rabbitmquser_webhook.go index b099af18..06df1a2f 100644 --- a/internal/webhook/rabbitmq/v1beta1/rabbitmquser_webhook.go +++ b/internal/webhook/rabbitmq/v1beta1/rabbitmquser_webhook.go @@ -59,14 +59,14 @@ type RabbitMQUserCustomDefaulter struct { var _ webhook.CustomDefaulter = &RabbitMQUserCustomDefaulter{} // Default implements webhook.CustomDefaulter so a webhook will be registered for the type RabbitMQUser. -func (d *RabbitMQUserCustomDefaulter) Default(_ context.Context, obj runtime.Object) error { +func (d *RabbitMQUserCustomDefaulter) Default(ctx context.Context, obj runtime.Object) error { rabbitmquser, ok := obj.(*rabbitmqv1beta1.RabbitMQUser) if !ok { return fmt.Errorf("expected a RabbitMQUser object but got %T", obj) } log.Info("Defaulting for RabbitMQUser", "name", rabbitmquser.GetName()) - rabbitmquser.Default(d.Client) + rabbitmquser.Default(ctx, d.Client) return nil } diff --git a/internal/webhook/rabbitmq/v1beta1/rabbitmquser_webhook_test.go b/internal/webhook/rabbitmq/v1beta1/rabbitmquser_webhook_test.go index eca4df47..92639f7c 100644 --- a/internal/webhook/rabbitmq/v1beta1/rabbitmquser_webhook_test.go +++ b/internal/webhook/rabbitmq/v1beta1/rabbitmquser_webhook_test.go @@ -39,7 +39,7 @@ var _ = Describe("RabbitMQUser webhook", func() { }, } - user.Default(k8sClient) + user.Default(ctx, k8sClient) Expect(user.Spec.Username).To(Equal("test-user")) }) @@ -56,7 +56,7 @@ var _ = Describe("RabbitMQUser webhook", func() { }, } - user.Default(k8sClient) + user.Default(ctx, k8sClient) Expect(user.Spec.Username).To(Equal("custom-username")) }) diff --git a/pkg/rabbitmq/api/client.go b/pkg/rabbitmq/api/client.go index 387c5685..8291224f 100644 --- a/pkg/rabbitmq/api/client.go +++ b/pkg/rabbitmq/api/client.go @@ -21,6 +21,7 @@ package api import ( "bytes" + "context" "crypto/tls" "crypto/x509" "encoding/json" @@ -47,6 +48,9 @@ const ( // DeleteTimeout is the timeout for delete operations which may take longer // (e.g., deleting vhosts with many queues) DeleteTimeout = 60 * time.Second + + // maxResponseBodySize limits how much of an error response body we read (1 MB) + maxResponseBodySize = 1 << 20 ) // User represents a RabbitMQ user @@ -80,9 +84,7 @@ type Policy struct { // NewClient creates a new RabbitMQ Management API client func NewClient(baseURL, username, password string, tlsEnabled bool, caCert []byte) (*Client, error) { - httpClient := &http.Client{ - Timeout: DefaultAPITimeout, - } + httpClient := &http.Client{} if tlsEnabled && len(caCert) > 0 { caCertPool := x509.NewCertPool() @@ -107,12 +109,15 @@ func NewClient(baseURL, username, password string, tlsEnabled bool, caCert []byt } // doRequest performs an HTTP request with authentication using the default timeout -func (c *Client) doRequest(method, path string, body interface{}) (*http.Response, error) { - return c.doRequestWithTimeout(method, path, body, DefaultAPITimeout) +func (c *Client) doRequest(ctx context.Context, method, path string, body interface{}) (*http.Response, error) { + return c.doRequestWithTimeout(ctx, method, path, body, DefaultAPITimeout) } // doRequestWithTimeout performs an HTTP request with authentication using a custom timeout -func (c *Client) doRequestWithTimeout(method, path string, body interface{}, timeout time.Duration) (*http.Response, error) { +func (c *Client) doRequestWithTimeout(ctx context.Context, method, path string, body interface{}, timeout time.Duration) (*http.Response, error) { + ctx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + var reqBody io.Reader if body != nil { jsonData, err := json.Marshal(body) @@ -123,7 +128,7 @@ func (c *Client) doRequestWithTimeout(method, path string, body interface{}, tim } apiURL := fmt.Sprintf("%s%s", c.baseURL, path) - req, err := http.NewRequest(method, apiURL, reqBody) + req, err := http.NewRequestWithContext(ctx, method, apiURL, reqBody) if err != nil { return nil, fmt.Errorf("failed to create request: %w", err) } @@ -131,15 +136,7 @@ func (c *Client) doRequestWithTimeout(method, path string, body interface{}, tim req.SetBasicAuth(c.username, c.password) req.Header.Set("Content-Type", "application/json") - httpClient := c.httpClient - if timeout != DefaultAPITimeout { - httpClient = &http.Client{ - Timeout: timeout, - Transport: c.httpClient.Transport, - } - } - - resp, err := httpClient.Do(req) + resp, err := c.httpClient.Do(req) if err != nil { return nil, fmt.Errorf("request failed: %w", err) } @@ -147,8 +144,21 @@ func (c *Client) doRequestWithTimeout(method, path string, body interface{}, tim return resp, nil } +// readAndCloseBody drains and closes a response body, reading up to maxResponseBodySize +// on error status codes for diagnostic messages +func readErrorBody(resp *http.Response) string { + body, _ := io.ReadAll(io.LimitReader(resp.Body, maxResponseBodySize)) + return string(body) +} + +// drainAndClose fully drains and closes resp.Body to allow HTTP connection reuse +func drainAndClose(resp *http.Response) { + _, _ = io.Copy(io.Discard, resp.Body) + _ = resp.Body.Close() +} + // CreateOrUpdateUser creates or updates a RabbitMQ user -func (c *Client) CreateOrUpdateUser(name, password string, tags []string) error { +func (c *Client) CreateOrUpdateUser(ctx context.Context, name, password string, tags []string) error { if tags == nil { tags = []string{} } @@ -160,85 +170,73 @@ func (c *Client) CreateOrUpdateUser(name, password string, tags []string) error } encodedName := url.PathEscape(name) - resp, err := c.doRequest("PUT", fmt.Sprintf("/api/users/%s", encodedName), user) + resp, err := c.doRequest(ctx, "PUT", fmt.Sprintf("/api/users/%s", encodedName), user) if err != nil { return err } - defer func() { - _ = resp.Body.Close() - }() + defer drainAndClose(resp) if resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusNoContent { - body, _ := io.ReadAll(resp.Body) - return fmt.Errorf("failed to create/update user %s: status %d, body: %s", name, resp.StatusCode, string(body)) + return fmt.Errorf("failed to create/update user %s: status %d, body: %s", name, resp.StatusCode, readErrorBody(resp)) } return nil } // DeleteUser deletes a RabbitMQ user -func (c *Client) DeleteUser(name string) error { +func (c *Client) DeleteUser(ctx context.Context, name string) error { encodedName := url.PathEscape(name) - resp, err := c.doRequestWithTimeout("DELETE", fmt.Sprintf("/api/users/%s", encodedName), nil, DeleteTimeout) + resp, err := c.doRequestWithTimeout(ctx, "DELETE", fmt.Sprintf("/api/users/%s", encodedName), nil, DeleteTimeout) if err != nil { return err } - defer func() { - _ = resp.Body.Close() - }() + defer drainAndClose(resp) if resp.StatusCode != http.StatusNoContent && resp.StatusCode != http.StatusNotFound { - body, _ := io.ReadAll(resp.Body) - return fmt.Errorf("failed to delete user %s: status %d, body: %s", name, resp.StatusCode, string(body)) + return fmt.Errorf("failed to delete user %s: status %d, body: %s", name, resp.StatusCode, readErrorBody(resp)) } return nil } // CreateOrUpdateVhost creates or updates a RabbitMQ vhost -func (c *Client) CreateOrUpdateVhost(name string) error { +func (c *Client) CreateOrUpdateVhost(ctx context.Context, name string) error { vhost := Vhost{ Name: name, } encodedName := url.PathEscape(name) - resp, err := c.doRequest("PUT", fmt.Sprintf("/api/vhosts/%s", encodedName), vhost) + resp, err := c.doRequest(ctx, "PUT", fmt.Sprintf("/api/vhosts/%s", encodedName), vhost) if err != nil { return err } - defer func() { - _ = resp.Body.Close() - }() + defer drainAndClose(resp) if resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusNoContent { - body, _ := io.ReadAll(resp.Body) - return fmt.Errorf("failed to create/update vhost %s: status %d, body: %s", name, resp.StatusCode, string(body)) + return fmt.Errorf("failed to create/update vhost %s: status %d, body: %s", name, resp.StatusCode, readErrorBody(resp)) } return nil } // DeleteVhost deletes a RabbitMQ vhost -func (c *Client) DeleteVhost(name string) error { +func (c *Client) DeleteVhost(ctx context.Context, name string) error { encodedName := url.PathEscape(name) - resp, err := c.doRequestWithTimeout("DELETE", fmt.Sprintf("/api/vhosts/%s", encodedName), nil, DeleteTimeout) + resp, err := c.doRequestWithTimeout(ctx, "DELETE", fmt.Sprintf("/api/vhosts/%s", encodedName), nil, DeleteTimeout) if err != nil { return err } - defer func() { - _ = resp.Body.Close() - }() + defer drainAndClose(resp) if resp.StatusCode != http.StatusNoContent && resp.StatusCode != http.StatusNotFound { - body, _ := io.ReadAll(resp.Body) - return fmt.Errorf("failed to delete vhost %s: status %d, body: %s", name, resp.StatusCode, string(body)) + return fmt.Errorf("failed to delete vhost %s: status %d, body: %s", name, resp.StatusCode, readErrorBody(resp)) } return nil } // SetPermissions sets permissions for a user on a vhost -func (c *Client) SetPermissions(vhost, user, configure, write, read string) error { +func (c *Client) SetPermissions(ctx context.Context, vhost, user, configure, write, read string) error { // The request body should only contain the permission fields, not user/vhost perm := map[string]string{ "configure": configure, @@ -250,44 +248,38 @@ func (c *Client) SetPermissions(vhost, user, configure, write, read string) erro encodedVhost := url.PathEscape(vhost) encodedUser := url.PathEscape(user) - resp, err := c.doRequest("PUT", fmt.Sprintf("/api/permissions/%s/%s", encodedVhost, encodedUser), perm) + resp, err := c.doRequest(ctx, "PUT", fmt.Sprintf("/api/permissions/%s/%s", encodedVhost, encodedUser), perm) if err != nil { return err } - defer func() { - _ = resp.Body.Close() - }() + defer drainAndClose(resp) if resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusNoContent { - body, _ := io.ReadAll(resp.Body) - return fmt.Errorf("failed to set permissions for user %s on vhost %s: status %d, body: %s", user, vhost, resp.StatusCode, string(body)) + return fmt.Errorf("failed to set permissions for user %s on vhost %s: status %d, body: %s", user, vhost, resp.StatusCode, readErrorBody(resp)) } return nil } // DeletePermissions deletes permissions for a user on a vhost -func (c *Client) DeletePermissions(vhost, user string) error { +func (c *Client) DeletePermissions(ctx context.Context, vhost, user string) error { encodedVhost := url.PathEscape(vhost) encodedUser := url.PathEscape(user) - resp, err := c.doRequestWithTimeout("DELETE", fmt.Sprintf("/api/permissions/%s/%s", encodedVhost, encodedUser), nil, DeleteTimeout) + resp, err := c.doRequestWithTimeout(ctx, "DELETE", fmt.Sprintf("/api/permissions/%s/%s", encodedVhost, encodedUser), nil, DeleteTimeout) if err != nil { return err } - defer func() { - _ = resp.Body.Close() - }() + defer drainAndClose(resp) if resp.StatusCode != http.StatusNoContent && resp.StatusCode != http.StatusNotFound { - body, _ := io.ReadAll(resp.Body) - return fmt.Errorf("failed to delete permissions for user %s on vhost %s: status %d, body: %s", user, vhost, resp.StatusCode, string(body)) + return fmt.Errorf("failed to delete permissions for user %s on vhost %s: status %d, body: %s", user, vhost, resp.StatusCode, readErrorBody(resp)) } return nil } // CreateOrUpdatePolicy creates or updates a RabbitMQ policy -func (c *Client) CreateOrUpdatePolicy(vhost, name, pattern string, definition map[string]interface{}, priority int, applyTo string) error { +func (c *Client) CreateOrUpdatePolicy(ctx context.Context, vhost, name, pattern string, definition map[string]interface{}, priority int, applyTo string) error { if applyTo == "" { applyTo = "all" } @@ -301,37 +293,31 @@ func (c *Client) CreateOrUpdatePolicy(vhost, name, pattern string, definition ma encodedVhost := url.PathEscape(vhost) encodedName := url.PathEscape(name) - resp, err := c.doRequest("PUT", fmt.Sprintf("/api/policies/%s/%s", encodedVhost, encodedName), policy) + resp, err := c.doRequest(ctx, "PUT", fmt.Sprintf("/api/policies/%s/%s", encodedVhost, encodedName), policy) if err != nil { return err } - defer func() { - _ = resp.Body.Close() - }() + defer drainAndClose(resp) if resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusNoContent { - body, _ := io.ReadAll(resp.Body) - return fmt.Errorf("failed to create/update policy %s on vhost %s: status %d, body: %s", name, vhost, resp.StatusCode, string(body)) + return fmt.Errorf("failed to create/update policy %s on vhost %s: status %d, body: %s", name, vhost, resp.StatusCode, readErrorBody(resp)) } return nil } // DeletePolicy deletes a RabbitMQ policy -func (c *Client) DeletePolicy(vhost, name string) error { +func (c *Client) DeletePolicy(ctx context.Context, vhost, name string) error { encodedVhost := url.PathEscape(vhost) encodedName := url.PathEscape(name) - resp, err := c.doRequestWithTimeout("DELETE", fmt.Sprintf("/api/policies/%s/%s", encodedVhost, encodedName), nil, DeleteTimeout) + resp, err := c.doRequestWithTimeout(ctx, "DELETE", fmt.Sprintf("/api/policies/%s/%s", encodedVhost, encodedName), nil, DeleteTimeout) if err != nil { return err } - defer func() { - _ = resp.Body.Close() - }() + defer drainAndClose(resp) if resp.StatusCode != http.StatusNoContent && resp.StatusCode != http.StatusNotFound { - body, _ := io.ReadAll(resp.Body) - return fmt.Errorf("failed to delete policy %s on vhost %s: status %d, body: %s", name, vhost, resp.StatusCode, string(body)) + return fmt.Errorf("failed to delete policy %s on vhost %s: status %d, body: %s", name, vhost, resp.StatusCode, readErrorBody(resp)) } return nil diff --git a/pkg/rabbitmq/api/client_test.go b/pkg/rabbitmq/api/client_test.go index 981f5ae8..5ca20009 100644 --- a/pkg/rabbitmq/api/client_test.go +++ b/pkg/rabbitmq/api/client_test.go @@ -18,6 +18,7 @@ limitations under the License. package api import ( + "context" "encoding/json" "net/http" "net/http/httptest" @@ -80,7 +81,7 @@ func TestCreateOrUpdateUser(t *testing.T) { if err != nil { t.Fatalf("NewClient failed: %v", err) } - err = client.CreateOrUpdateUser("testuser", "testpass", []string{"monitoring"}) + err = client.CreateOrUpdateUser(context.Background(), "testuser", "testpass", []string{"monitoring"}) if err != nil { t.Errorf("CreateOrUpdateUser failed: %v", err) } @@ -102,7 +103,7 @@ func TestDeleteUser(t *testing.T) { if err != nil { t.Fatalf("NewClient failed: %v", err) } - err = client.DeleteUser("testuser") + err = client.DeleteUser(context.Background(), "testuser") if err != nil { t.Errorf("DeleteUser failed: %v", err) } @@ -124,7 +125,7 @@ func TestCreateOrUpdateVhost(t *testing.T) { if err != nil { t.Fatalf("NewClient failed: %v", err) } - err = client.CreateOrUpdateVhost("testvhost") + err = client.CreateOrUpdateVhost(context.Background(), "testvhost") if err != nil { t.Errorf("CreateOrUpdateVhost failed: %v", err) } @@ -146,7 +147,7 @@ func TestDeleteVhost(t *testing.T) { if err != nil { t.Fatalf("NewClient failed: %v", err) } - err = client.DeleteVhost("testvhost") + err = client.DeleteVhost(context.Background(), "testvhost") if err != nil { t.Errorf("DeleteVhost failed: %v", err) } @@ -177,7 +178,7 @@ func TestSetPermissions(t *testing.T) { if err != nil { t.Fatalf("NewClient failed: %v", err) } - err = client.SetPermissions("/", "testuser", ".*", ".*", ".*") + err = client.SetPermissions(context.Background(), "/", "testuser", ".*", ".*", ".*") if err != nil { t.Errorf("SetPermissions failed: %v", err) } @@ -199,7 +200,7 @@ func TestDeletePermissions(t *testing.T) { if err != nil { t.Fatalf("NewClient failed: %v", err) } - err = client.DeletePermissions("/", "testuser") + err = client.DeletePermissions(context.Background(), "/", "testuser") if err != nil { t.Errorf("DeletePermissions failed: %v", err) } @@ -231,7 +232,7 @@ func TestCreateOrUpdatePolicy(t *testing.T) { t.Fatalf("NewClient failed: %v", err) } definition := map[string]interface{}{"max-length": 10000} - err = client.CreateOrUpdatePolicy("/", "testpolicy", ".*", definition, 1, "all") + err = client.CreateOrUpdatePolicy(context.Background(), "/", "testpolicy", ".*", definition, 1, "all") if err != nil { t.Errorf("CreateOrUpdatePolicy failed: %v", err) } @@ -253,7 +254,7 @@ func TestDeletePolicy(t *testing.T) { if err != nil { t.Fatalf("NewClient failed: %v", err) } - err = client.DeletePolicy("/", "testpolicy") + err = client.DeletePolicy(context.Background(), "/", "testpolicy") if err != nil { t.Errorf("DeletePolicy failed: %v", err) } diff --git a/test/functional/rabbitmquser_controller_test.go b/test/functional/rabbitmquser_controller_test.go index 8618d8cd..d24e553e 100644 --- a/test/functional/rabbitmquser_controller_test.go +++ b/test/functional/rabbitmquser_controller_test.go @@ -164,11 +164,11 @@ var _ = Describe("RabbitMQUser controller", func() { DeferCleanup(th.DeleteInstance, transportURL) // Wait for user to be created by TransportURL - ownedUserName = types.NamespacedName{Name: transportURLName.Name + "-testuser-user", Namespace: namespace} + ownedUserName = types.NamespacedName{Name: rabbitmqv1.CanonicalUserName(rabbitmqClusterName.Name, "/", "testuser"), Namespace: namespace} Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} g.Expect(th.K8sClient.Get(th.Ctx, ownedUserName, user)).To(Succeed()) - g.Expect(user.Finalizers).To(ContainElement(rabbitmqv1.TransportURLFinalizer)) + g.Expect(user.Finalizers).To(ContainElement(rabbitmqv1.TransportURLFinalizerFor(transportURLName.Name))) }, timeout, interval).Should(Succeed()) }) @@ -184,7 +184,7 @@ var _ = Describe("RabbitMQUser controller", func() { u := &rabbitmqv1.RabbitMQUser{} g.Expect(th.K8sClient.Get(th.Ctx, ownedUserName, u)).To(Succeed()) g.Expect(u.DeletionTimestamp).NotTo(BeNil()) - g.Expect(u.Finalizers).To(ContainElement(rabbitmqv1.TransportURLFinalizer)) + g.Expect(u.Finalizers).To(ContainElement(rabbitmqv1.TransportURLFinalizerFor(transportURLName.Name))) }, "2s", interval).Should(Succeed()) }) @@ -616,11 +616,11 @@ var _ = Describe("RabbitMQUser controller", func() { DeferCleanup(th.DeleteInstance, transportURL) // Wait for user to be created by TransportURL - ownedUserName := types.NamespacedName{Name: transportURLName.Name + "-testuser-status-user", Namespace: namespace} + ownedUserName := types.NamespacedName{Name: rabbitmqv1.CanonicalUserName(rabbitmqClusterName.Name, "/", "testuser-status"), Namespace: namespace} Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} g.Expect(th.K8sClient.Get(th.Ctx, ownedUserName, user)).To(Succeed()) - g.Expect(user.Finalizers).To(ContainElement(rabbitmqv1.TransportURLFinalizer)) + g.Expect(user.Finalizers).To(ContainElement(rabbitmqv1.TransportURLFinalizerFor(transportURLName.Name))) }, timeout, interval).Should(Succeed()) // Mark cluster for deletion to avoid RabbitMQ API call failures @@ -642,7 +642,7 @@ var _ = Describe("RabbitMQUser controller", func() { g.Expect(cond).NotTo(BeNil()) g.Expect(cond.Status).To(Equal(corev1.ConditionFalse)) g.Expect(string(cond.Reason)).To(Equal(string(condition.DeletingReason))) - g.Expect(cond.Message).To(ContainSubstring("Waiting for TransportURL to release user")) + g.Expect(cond.Message).To(ContainSubstring("Waiting for TransportURL " + transportURLName.Name + " to release user")) }, timeout, interval).Should(Succeed()) }) diff --git a/test/functional/rabbitmqvhost_controller_test.go b/test/functional/rabbitmqvhost_controller_test.go index d3a64c70..48dff8af 100644 --- a/test/functional/rabbitmqvhost_controller_test.go +++ b/test/functional/rabbitmqvhost_controller_test.go @@ -17,11 +17,18 @@ limitations under the License. package functional_test import ( + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + . "github.com/onsi/ginkgo/v2" //nolint:revive . "github.com/onsi/gomega" //nolint:revive rabbitmqv1 "github.com/openstack-k8s-operators/infra-operator/apis/rabbitmq/v1beta1" condition "github.com/openstack-k8s-operators/lib-common/modules/common/condition" + k8s_errors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) var _ = Describe("RabbitMQVhost controller", func() { @@ -121,11 +128,11 @@ var _ = Describe("RabbitMQVhost controller", func() { DeferCleanup(th.DeleteInstance, transportURL) // Wait for vhost to be created by TransportURL - ownedVhostName = types.NamespacedName{Name: transportURLName.Name + "-testvhost-vhost", Namespace: namespace} + ownedVhostName = types.NamespacedName{Name: rabbitmqv1.CanonicalVhostName(rabbitmqClusterName.Name, "testvhost"), Namespace: namespace} Eventually(func(g Gomega) { vhost := &rabbitmqv1.RabbitMQVhost{} g.Expect(th.K8sClient.Get(th.Ctx, ownedVhostName, vhost)).To(Succeed()) - g.Expect(vhost.Finalizers).To(ContainElement(rabbitmqv1.TransportURLFinalizer)) + g.Expect(vhost.Finalizers).To(ContainElement(rabbitmqv1.TransportURLFinalizerFor(transportURLName.Name))) }, timeout, interval).Should(Succeed()) }) @@ -141,7 +148,7 @@ var _ = Describe("RabbitMQVhost controller", func() { v := &rabbitmqv1.RabbitMQVhost{} g.Expect(th.K8sClient.Get(th.Ctx, ownedVhostName, v)).To(Succeed()) g.Expect(v.DeletionTimestamp).NotTo(BeNil()) - g.Expect(v.Finalizers).To(ContainElement(rabbitmqv1.TransportURLFinalizer)) + g.Expect(v.Finalizers).To(ContainElement(rabbitmqv1.TransportURLFinalizerFor(transportURLName.Name))) }, "2s", interval).Should(Succeed()) }) }) @@ -535,4 +542,93 @@ var _ = Describe("RabbitMQVhost controller", func() { }, timeout, interval).Should(Succeed()) }) }) + + When("a duplicate RabbitMQVhost CR references the same vhost name", func() { + var safetyClusterName types.NamespacedName + var canonicalVhostName types.NamespacedName + var duplicateVhostName types.NamespacedName + var vhostDeleteCount atomic.Int32 + + BeforeEach(func() { + safetyClusterName = types.NamespacedName{Name: "rabbitmq-vhost-safety", Namespace: namespace} + canonicalVhostName = types.NamespacedName{Name: "canonical-vhost", Namespace: namespace} + duplicateVhostName = types.NamespacedName{Name: "duplicate-vhost", Namespace: namespace} + vhostDeleteCount.Store(0) + + // Custom mock that tracks DELETE /api/vhosts/ calls + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodDelete && strings.HasPrefix(r.URL.Path, "/api/vhosts/") { + vhostDeleteCount.Add(1) + } + w.WriteHeader(http.StatusNoContent) + })) + hostPort := strings.TrimPrefix(server.URL, "http://") + parts := strings.Split(hostPort, ":") + mockRabbitMQServer = server + mockRabbitMQHost = parts[0] + mockRabbitMQPort = parts[1] + DeferCleanup(StopMockRabbitMQAPI) + + CreateRabbitMQCluster(safetyClusterName, GetDefaultRabbitMQClusterSpec(false)) + SimulateRabbitMQClusterReady(safetyClusterName) + DeferCleanup(DeleteRabbitMQCluster, safetyClusterName) + + // Create the canonical vhost CR + CreateRabbitMQVhost(canonicalVhostName, map[string]any{ + "rabbitmqClusterName": safetyClusterName.Name, + "name": "shared-vhost", + }) + + // Wait for it to become ready + Eventually(func(g Gomega) { + v := GetRabbitMQVhost(canonicalVhostName) + g.Expect(v.Status.Conditions.IsTrue(rabbitmqv1.RabbitMQVhostReadyCondition)).To(BeTrue()) + }, timeout, interval).Should(Succeed()) + + // Create a duplicate CR pointing to the same vhost + CreateRabbitMQVhost(duplicateVhostName, map[string]any{ + "rabbitmqClusterName": safetyClusterName.Name, + "name": "shared-vhost", + }) + + // Wait for it to become ready + Eventually(func(g Gomega) { + v := GetRabbitMQVhost(duplicateVhostName) + g.Expect(v.Status.Conditions.IsTrue(rabbitmqv1.RabbitMQVhostReadyCondition)).To(BeTrue()) + }, timeout, interval).Should(Succeed()) + + // Reset counter after setup (creation calls PUT, not DELETE) + vhostDeleteCount.Store(0) + }) + + It("should not call the RabbitMQ API to delete the vhost when the duplicate CR is deleted", func() { + // Verify both CRs reference the same vhost name + canonical := GetRabbitMQVhost(canonicalVhostName) + duplicate := GetRabbitMQVhost(duplicateVhostName) + Expect(canonical.Spec.Name).To(Equal("shared-vhost")) + Expect(duplicate.Spec.Name).To(Equal("shared-vhost")) + + // Delete the duplicate CR + Expect(th.K8sClient.Delete(th.Ctx, duplicate)).To(Succeed()) + + // Verify the duplicate CR is fully deleted + Eventually(func(g Gomega) { + v := &rabbitmqv1.RabbitMQVhost{} + err := th.K8sClient.Get(th.Ctx, duplicateVhostName, v) + g.Expect(k8s_errors.IsNotFound(err)).To(BeTrue()) + }, timeout, interval).Should(Succeed()) + + // Verify no DELETE /api/vhosts/ call was made + Expect(vhostDeleteCount.Load()).To(Equal(int32(0)), + "Safety check should have prevented the DELETE vhost API call") + + // Verify the canonical CR is still present and ready + Eventually(func(g Gomega) { + v := GetRabbitMQVhost(canonicalVhostName) + g.Expect(v.Status.Conditions.IsTrue(rabbitmqv1.RabbitMQVhostReadyCondition)).To(BeTrue()) + g.Expect(v.DeletionTimestamp).To(BeNil()) + g.Expect(controllerutil.ContainsFinalizer(v, "rabbitmqvhost.openstack.org/finalizer")).To(BeTrue()) + }, timeout, interval).Should(Succeed()) + }) + }) }) diff --git a/test/functional/transporturl_controller_test.go b/test/functional/transporturl_controller_test.go index 79597517..6d9b5283 100644 --- a/test/functional/transporturl_controller_test.go +++ b/test/functional/transporturl_controller_test.go @@ -18,7 +18,6 @@ package functional_test import ( "fmt" - "time" . "github.com/onsi/ginkgo/v2" //revive:disable:dot-imports . "github.com/onsi/gomega" //revive:disable:dot-imports @@ -29,8 +28,6 @@ import ( 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/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -360,7 +357,7 @@ var _ = Describe("TransportURL controller", func() { // Wait for and simulate RabbitMQVhost being ready vhostCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-%s-vhost", transportURLName.Name, vhostName), + Name: rabbitmqv1.CanonicalVhostName(rabbitmqName.Name, vhostName), Namespace: namespace, } // First wait for the vhost CR to be created by the TransportURL controller @@ -373,7 +370,7 @@ var _ = Describe("TransportURL controller", func() { // Wait for RabbitMQUser to be created userCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-nova-user-user", transportURLName.Name), + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, vhostName, "nova-user"), Namespace: namespace, } Eventually(func(g Gomega) { @@ -470,7 +467,7 @@ var _ = Describe("TransportURL controller", func() { // Wait for initial RabbitMQVhost to be created initialVhostCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-initial-vhost-vhost", transportURLName.Name), + Name: rabbitmqv1.CanonicalVhostName(rabbitmqName.Name, "initial-vhost"), Namespace: namespace, } Eventually(func(g Gomega) { @@ -483,7 +480,7 @@ var _ = Describe("TransportURL controller", func() { // Wait for initial RabbitMQUser to be created initialUserCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-initial-user-user", transportURLName.Name), + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "initial-vhost", "initial-user"), Namespace: namespace, } var initialUserPassword string @@ -539,7 +536,7 @@ var _ = Describe("TransportURL controller", func() { // Wait for new RabbitMQVhost to be created updatedVhostCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-updated-vhost-vhost", transportURLName.Name), + Name: rabbitmqv1.CanonicalVhostName(rabbitmqName.Name, "updated-vhost"), Namespace: namespace, } Eventually(func(g Gomega) { @@ -551,15 +548,20 @@ var _ = Describe("TransportURL controller", func() { // Simulate updated vhost being ready SimulateRabbitMQVhostReady(updatedVhostCRName) - // Wait for RabbitMQUser to be updated with new vhost + // In the shared singleton model, changing vhost creates a new canonical user CR + // (since the canonical name includes the vhost) + updatedUserCRName := types.NamespacedName{ + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "updated-vhost", "initial-user"), + Namespace: namespace, + } Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} - g.Expect(k8sClient.Get(ctx, initialUserCRName, user)).Should(Succeed()) + g.Expect(k8sClient.Get(ctx, updatedUserCRName, user)).Should(Succeed()) g.Expect(user.Spec.VhostRef).To(Equal(updatedVhostCRName.Name)) }, timeout, interval).Should(Succeed()) - // Simulate user being ready with updated vhost - SimulateRabbitMQUserReady(initialUserCRName, "updated-vhost") + // Simulate new user being ready with updated vhost + SimulateRabbitMQUserReady(updatedUserCRName, "updated-vhost") // Verify transport URL secret is updated with new vhost Eventually(func(g Gomega) { @@ -599,7 +601,7 @@ var _ = Describe("TransportURL controller", func() { // Wait for RabbitMQUser to be created (no vhost, so no RabbitMQVhost CR) initialUserCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-test-user-user", transportURLNoVhost.Name), + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "test-user"), Namespace: namespace, } Eventually(func(g Gomega) { @@ -636,7 +638,7 @@ var _ = Describe("TransportURL controller", func() { // Wait for RabbitMQVhost to be created newVhostCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-new-vhost-vhost", transportURLNoVhost.Name), + Name: rabbitmqv1.CanonicalVhostName(rabbitmqName.Name, "new-vhost"), Namespace: namespace, } Eventually(func(g Gomega) { @@ -648,15 +650,20 @@ var _ = Describe("TransportURL controller", func() { // Simulate new vhost being ready SimulateRabbitMQVhostReady(newVhostCRName) - // Wait for RabbitMQUser to be updated with new vhost reference + // In the shared singleton model, adding a vhost creates a new canonical user CR + // (since the canonical name includes the vhost) + updatedUserCRName := types.NamespacedName{ + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "new-vhost", "test-user"), + Namespace: namespace, + } Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} - g.Expect(k8sClient.Get(ctx, initialUserCRName, user)).Should(Succeed()) + g.Expect(k8sClient.Get(ctx, updatedUserCRName, user)).Should(Succeed()) g.Expect(user.Spec.VhostRef).To(Equal(newVhostCRName.Name)) }, timeout, interval).Should(Succeed()) // Simulate user being ready with NEW vhost - SimulateRabbitMQUserReady(initialUserCRName, "new-vhost") + SimulateRabbitMQUserReady(updatedUserCRName, "new-vhost") // Verify transport URL secret is UPDATED with new vhost Eventually(func(g Gomega) { @@ -681,7 +688,7 @@ var _ = Describe("TransportURL controller", func() { // Wait for initial RabbitMQVhost to be created vhostCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-initial-vhost-vhost", transportURLName.Name), + Name: rabbitmqv1.CanonicalVhostName(rabbitmqName.Name, "initial-vhost"), Namespace: namespace, } Eventually(func(g Gomega) { @@ -694,7 +701,7 @@ var _ = Describe("TransportURL controller", func() { // Wait for initial RabbitMQUser to be created initialUserCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-initial-user-user", transportURLName.Name), + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "initial-vhost", "initial-user"), Namespace: namespace, } Eventually(func(g Gomega) { @@ -734,7 +741,7 @@ var _ = Describe("TransportURL controller", func() { // Wait for new RabbitMQUser to be created updatedUserCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-updated-user-user", transportURLName.Name), + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "initial-vhost", "updated-user"), Namespace: namespace, } Eventually(func(g Gomega) { @@ -775,6 +782,85 @@ var _ = Describe("TransportURL controller", func() { }) }) + When("two TransportURLs share the same user CR", func() { + var rabbitmqName types.NamespacedName + var transportURL1Name types.NamespacedName + var transportURL2Name types.NamespacedName + + BeforeEach(func() { + rabbitmqName = types.NamespacedName{ + Name: "rabbitmq-shared", + Namespace: namespace, + } + + transportURL1Name = types.NamespacedName{ + Name: "turl-shared-1", + Namespace: namespace, + } + + transportURL2Name = types.NamespacedName{ + Name: "turl-shared-2", + Namespace: namespace, + } + + SetupMockRabbitMQAPI() + DeferCleanup(StopMockRabbitMQAPI) + + CreateRabbitMQCluster(rabbitmqName, GetDefaultRabbitMQClusterSpec(false)) + DeferCleanup(DeleteRabbitMQCluster, rabbitmqName) + + spec := GetDefaultRabbitMQSpec() + rabbitmq := CreateRabbitMQ(rabbitmqName, spec) + DeferCleanup(th.DeleteInstance, rabbitmq) + + tuSpec := map[string]any{ + "rabbitmqClusterName": rabbitmqName.Name, + "username": "shared-user", + } + DeferCleanup(th.DeleteInstance, CreateTransportURL(transportURL1Name, tuSpec)) + DeferCleanup(th.DeleteInstance, CreateTransportURL(transportURL2Name, tuSpec)) + }) + + It("should keep the shared user alive when one TransportURL is deleted", func() { + SimulateRabbitMQClusterReady(rabbitmqName) + + userCRName := types.NamespacedName{ + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "shared-user"), + Namespace: namespace, + } + + // Wait for user CR to be created with both per-consumer finalizers + Eventually(func(g Gomega) { + user := &rabbitmqv1.RabbitMQUser{} + g.Expect(k8sClient.Get(ctx, userCRName, user)).Should(Succeed()) + g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizerFor(transportURL1Name.Name))).To(BeTrue()) + g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizerFor(transportURL2Name.Name))).To(BeTrue()) + }, timeout, interval).Should(Succeed()) + + SimulateRabbitMQUserReady(userCRName, "/") + + // Delete TransportURL 1 + Eventually(func(g Gomega) { + tr := &rabbitmqv1.TransportURL{} + g.Expect(k8sClient.Get(ctx, transportURL1Name, tr)).Should(Succeed()) + g.Expect(k8sClient.Delete(ctx, tr)).Should(Succeed()) + }, timeout, interval).Should(Succeed()) + + // Verify: user CR still exists, TransportURL 1's finalizer removed, TransportURL 2's stays + Eventually(func(g Gomega) { + user := &rabbitmqv1.RabbitMQUser{} + g.Expect(k8sClient.Get(ctx, userCRName, user)).Should(Succeed()) + g.Expect(user.DeletionTimestamp.IsZero()).To(BeTrue(), "User should NOT be deleted") + g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizerFor(transportURL1Name.Name))).To(BeFalse(), + "Deleted TransportURL's finalizer should be removed") + g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizerFor(transportURL2Name.Name))).To(BeTrue(), + "Active TransportURL's finalizer should remain") + g.Expect(user.Labels[rabbitmqv1.RabbitMQUserOrphanedLabel]).ToNot(Equal("true"), + "User should NOT be marked orphaned while another consumer exists") + }, timeout, interval).Should(Succeed()) + }) + }) + When("username is changed, old RabbitMQUser should be automatically deleted", func() { var rabbitmqName types.NamespacedName @@ -810,7 +896,7 @@ var _ = Describe("TransportURL controller", func() { // Wait for initial RabbitMQUser to be created oldUserCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-olduser-user", transportURLName.Name), + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "olduser"), Namespace: namespace, } Eventually(func(g Gomega) { @@ -826,7 +912,7 @@ var _ = Describe("TransportURL controller", func() { Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} g.Expect(k8sClient.Get(ctx, oldUserCRName, user)).Should(Succeed()) - g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizer)).To(BeTrue()) + g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizerFor(transportURLName.Name))).To(BeTrue()) g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer)).To(BeTrue(), "User should have cleanup-blocked finalizer to prevent automatic deletion") }, timeout, interval).Should(Succeed()) @@ -840,7 +926,7 @@ var _ = Describe("TransportURL controller", func() { // Wait for new RabbitMQUser to be created newUserCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-newuser-user", transportURLName.Name), + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "newuser"), Namespace: namespace, } Eventually(func(g Gomega) { @@ -852,40 +938,20 @@ var _ = Describe("TransportURL controller", func() { // Simulate new user being ready SimulateRabbitMQUserReady(newUserCRName, "/") - // Verify old user's TransportURL finalizer was removed + // Verify old user's per-consumer finalizer was removed and user is marked as orphaned + // (shared singleton model: old user is labeled orphaned, not deleted — reclaimable by new consumers) Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} - err := k8sClient.Get(ctx, oldUserCRName, user) - // User might still exist but should not have the TransportURL finalizer - if err == nil { - g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizer)).To(BeFalse()) - } - }, timeout, interval).Should(Succeed()) - - // Manually remove the cleanup-blocked finalizer to allow deletion to proceed - // (simulating admin/operator manually approving the deletion) - Eventually(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - err := k8sClient.Get(ctx, oldUserCRName, user) - if err == nil && controllerutil.ContainsFinalizer(user, rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer) { - controllerutil.RemoveFinalizer(user, rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer) - g.Expect(k8sClient.Update(ctx, user)).Should(Succeed()) - } + g.Expect(k8sClient.Get(ctx, oldUserCRName, user)).Should(Succeed()) + g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizerFor(transportURLName.Name))).To(BeFalse()) + g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer)).To(BeTrue(), + "Cleanup-blocked finalizer should still be present") + g.Expect(user.Labels[rabbitmqv1.RabbitMQUserOrphanedLabel]).To(Equal("true"), + "Old user should be labeled as orphaned") + g.Expect(user.DeletionTimestamp.IsZero()).To(BeTrue(), + "Old user should NOT have DeletionTimestamp — label is used instead of Delete()") }, timeout, interval).Should(Succeed()) - // Verify old user resource is deleted (or being deleted) - // With 30s grace period - deletion should happen after grace period elapses - Eventually(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - err := k8sClient.Get(ctx, oldUserCRName, user) - // Either NotFound or has DeletionTimestamp set - if err == nil { - g.Expect(user.DeletionTimestamp.IsZero()).To(BeFalse(), "Old user should have DeletionTimestamp set") - } else { - g.Expect(k8s_errors.IsNotFound(err)).To(BeTrue(), "Old user should be NotFound") - } - }, time.Second*45, interval).Should(Succeed()) // Allow time for 30s grace period - // Verify new user is still present and ready Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} @@ -899,7 +965,7 @@ var _ = Describe("TransportURL controller", func() { // Wait for initial RabbitMQUser to be created oldUserCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-olduser-user", transportURLName.Name), + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "olduser"), Namespace: namespace, } Eventually(func(g Gomega) { @@ -943,7 +1009,7 @@ var _ = Describe("TransportURL controller", func() { // Wait for new RabbitMQUser to be created newUserCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-newuser-protected-user", transportURLName.Name), + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "newuser-protected"), Namespace: namespace, } Eventually(func(g Gomega) { @@ -958,632 +1024,36 @@ var _ = Describe("TransportURL controller", func() { Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} g.Expect(k8sClient.Get(ctx, oldUserCRName, user)).Should(Succeed()) - g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizer)).To(BeFalse()) + g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizerFor(transportURLName.Name))).To(BeFalse()) }, timeout, interval).Should(Succeed()) - // Verify old user is STILL present (not deleted due to external finalizers) - // No grace period - external finalizers immediately block deletion + // Verify old user is marked as orphaned (label-based) but NOT deleted Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} g.Expect(k8sClient.Get(ctx, oldUserCRName, user)).Should(Succeed()) - // Should NOT have DeletionTimestamp + // Should NOT have DeletionTimestamp — orphaned label is used instead g.Expect(user.DeletionTimestamp.IsZero()).To(BeTrue(), "Old user should NOT be marked for deletion") + // Should have the orphaned label + g.Expect(user.Labels[rabbitmqv1.RabbitMQUserOrphanedLabel]).To(Equal("true"), "Old user should have orphaned label") // Should still have both the nodeset finalizer and cleanup-blocked finalizer g.Expect(controllerutil.ContainsFinalizer(user, nodesetFinalizer)).To(BeTrue()) g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer)).To(BeTrue()) }, timeout, interval).Should(Succeed()) - // Cleanup: Remove both finalizers to allow cleanup + // Cleanup: Remove both finalizers and orphaned label to allow cleanup Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} g.Expect(k8sClient.Get(ctx, oldUserCRName, user)).Should(Succeed()) controllerutil.RemoveFinalizer(user, nodesetFinalizer) controllerutil.RemoveFinalizer(user, rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer) + labels := user.GetLabels() + delete(labels, rabbitmqv1.RabbitMQUserOrphanedLabel) + user.SetLabels(labels) g.Expect(k8sClient.Update(ctx, user)).Should(Succeed()) }, timeout, interval).Should(Succeed()) }) }) - When("username is changed with an owner service, deletion waits for owner to reconcile", func() { - var rabbitmqName types.NamespacedName - var ownerName types.NamespacedName - - BeforeEach(func() { - rabbitmqName = types.NamespacedName{ - Name: "rabbitmq-owner-gen-test", - Namespace: namespace, - } - ownerName = types.NamespacedName{ - Name: "test-owner-deployment", - Namespace: namespace, - } - - // Create RabbitMQCluster first - CreateRabbitMQCluster(rabbitmqName, GetDefaultRabbitMQClusterSpec(false)) - DeferCleanup(DeleteRabbitMQCluster, rabbitmqName) - - // Create RabbitMq CR - spec := GetDefaultRabbitMQSpec() - rabbitmq := CreateRabbitMQ(rabbitmqName, spec) - DeferCleanup(th.DeleteInstance, rabbitmq) - - // Create a mock owner service using a StatefulSet (simulating Cinder, Nova, etc.) - // StatefulSet has status.observedGeneration and conditions - owner := &unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": "apps/v1", - "kind": "StatefulSet", - "metadata": map[string]any{ - "name": ownerName.Name, - "namespace": ownerName.Namespace, - }, - "spec": map[string]any{ - "replicas": int64(1), - "selector": map[string]any{ - "matchLabels": map[string]any{ - "app": "test", - }, - }, - "template": map[string]any{ - "metadata": map[string]any{ - "labels": map[string]any{ - "app": "test", - }, - }, - "spec": map[string]any{ - "containers": []any{ - map[string]any{ - "name": "test", - "image": "test:latest", - }, - }, - }, - }, - }, - }, - } - owner.SetGroupVersionKind(schema.GroupVersionKind{ - Group: "apps", - Version: "v1", - Kind: "StatefulSet", - }) - Expect(k8sClient.Create(ctx, owner)).Should(Succeed()) - DeferCleanup(k8sClient.Delete, owner) - - // Verify the owner was created with generation=1 - Eventually(func(g Gomega) { - g.Expect(k8sClient.Get(ctx, ownerName, owner)).Should(Succeed()) - generation, found, err := unstructured.NestedInt64(owner.Object, "metadata", "generation") - g.Expect(err).ShouldNot(HaveOccurred()) - g.Expect(found).To(BeTrue(), "metadata.generation should be set") - g.Expect(generation).To(Equal(int64(1)), "metadata.generation should be 1") - }, timeout, interval).Should(Succeed()) - - // Set the status to match generation (observedGeneration=1, Ready=True) - Eventually(func(g Gomega) { - g.Expect(k8sClient.Get(ctx, ownerName, owner)).Should(Succeed()) - g.Expect(unstructured.SetNestedField(owner.Object, int64(1), "status", "observedGeneration")).Should(Succeed()) - g.Expect(unstructured.SetNestedSlice(owner.Object, []any{ - map[string]any{ - "type": "Ready", - "status": "True", - }, - }, "status", "conditions")).Should(Succeed()) - g.Expect(k8sClient.Status().Update(ctx, owner)).Should(Succeed()) - }, timeout, interval).Should(Succeed()) - - // Verify the owner status was properly set - Eventually(func(g Gomega) { - g.Expect(k8sClient.Get(ctx, ownerName, owner)).Should(Succeed()) - observedGen, found, err := unstructured.NestedInt64(owner.Object, "status", "observedGeneration") - g.Expect(err).ShouldNot(HaveOccurred()) - g.Expect(found).To(BeTrue(), "status.observedGeneration should be set") - g.Expect(observedGen).To(Equal(int64(1)), "status.observedGeneration should be 1") - - conditions, found, err := unstructured.NestedSlice(owner.Object, "status", "conditions") - g.Expect(err).ShouldNot(HaveOccurred()) - g.Expect(found).To(BeTrue(), "status.conditions should be set") - g.Expect(conditions).To(HaveLen(1)) - }, timeout, interval).Should(Succeed()) - - // Create TransportURL with owner reference - tu := &rabbitmqv1.TransportURL{ - ObjectMeta: metav1.ObjectMeta{ - Name: transportURLName.Name, - Namespace: transportURLName.Namespace, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: "apps/v1", - Kind: "StatefulSet", - Name: ownerName.Name, - UID: owner.GetUID(), - Controller: func() *bool { b := true; return &b }(), - }, - }, - }, - Spec: rabbitmqv1.TransportURLSpec{ - RabbitmqClusterName: rabbitmqName.Name, - Username: "gentest-olduser", - }, - } - Expect(k8sClient.Create(ctx, tu)).Should(Succeed()) - DeferCleanup(th.DeleteInstance, tu) - }) - - It("should wait for owner observedGeneration to increase before deleting old user", func() { - SimulateRabbitMQClusterReady(rabbitmqName) - - // Wait for initial RabbitMQUser to be created - oldUserCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-gentest-olduser-user", transportURLName.Name), - Namespace: namespace, - } - Eventually(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - g.Expect(k8sClient.Get(ctx, oldUserCRName, user)).Should(Succeed()) - g.Expect(user.Spec.Username).To(Equal("gentest-olduser")) - }, timeout, interval).Should(Succeed()) - - // Simulate user being ready - SimulateRabbitMQUserReady(oldUserCRName, "/") - - // Update the TransportURL spec with new username - Eventually(func(g Gomega) { - tr := th.GetTransportURL(transportURLName) - tr.Spec.Username = "gentest-newuser" - g.Expect(k8sClient.Update(ctx, tr)).Should(Succeed()) - }, timeout, interval).Should(Succeed()) - - // Wait for new RabbitMQUser to be created - newUserCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-gentest-newuser-user", transportURLName.Name), - Namespace: namespace, - } - Eventually(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - g.Expect(k8sClient.Get(ctx, newUserCRName, user)).Should(Succeed()) - g.Expect(user.Spec.Username).To(Equal("gentest-newuser")) - }, timeout, interval).Should(Succeed()) - - // Simulate new user being ready - SimulateRabbitMQUserReady(newUserCRName, "/") - - // Verify old user's TransportURL finalizer was removed and timestamp annotation was set - Eventually(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - g.Expect(k8sClient.Get(ctx, oldUserCRName, user)).Should(Succeed()) - g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizer)).To(BeFalse()) - // Verify the finalizer-removed-at timestamp annotation was set - g.Expect(user.Annotations).To(HaveKey("rabbitmq.openstack.org/finalizer-removed-at")) - }, timeout, interval).Should(Succeed()) - - // Wait a bit and verify old user is NOT deleted yet (grace period hasn't elapsed) - Consistently(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - g.Expect(k8sClient.Get(ctx, oldUserCRName, user)).Should(Succeed()) - // User should still exist and not have DeletionTimestamp - g.Expect(user.DeletionTimestamp).To(BeNil(), "Old user should NOT be deleted during grace period") - }, time.Second*3, interval).Should(Succeed()) - - // Manually remove the cleanup-blocked finalizer to allow deletion to proceed - Eventually(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - err := k8sClient.Get(ctx, oldUserCRName, user) - if err == nil && controllerutil.ContainsFinalizer(user, rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer) { - controllerutil.RemoveFinalizer(user, rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer) - g.Expect(k8sClient.Update(ctx, user)).Should(Succeed()) - } - }, timeout, interval).Should(Succeed()) - - // Now verify old user gets deleted after grace period (30s + buffer) - Eventually(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - err := k8sClient.Get(ctx, oldUserCRName, user) - // Either NotFound or has DeletionTimestamp set - if err == nil { - g.Expect(user.DeletionTimestamp.IsZero()).To(BeFalse(), "Old user should have DeletionTimestamp set") - } else { - g.Expect(k8s_errors.IsNotFound(err)).To(BeTrue(), "Old user should be NotFound") - } - }, time.Second*45, interval).Should(Succeed()) - }) - }) - - When("username is changed but owner service is not ready", func() { - var rabbitmqName types.NamespacedName - var transportURLName types.NamespacedName - var ownerName types.NamespacedName - - BeforeEach(func() { - rabbitmqName = types.NamespacedName{ - Name: "rabbitmq-owner-notready", - Namespace: namespace, - } - transportURLName = types.NamespacedName{ - Name: "transporturl-owner-notready", - Namespace: namespace, - } - ownerName = types.NamespacedName{ - Name: "owner-notready-deployment", - Namespace: namespace, - } - - // Create RabbitMQCluster first - CreateRabbitMQCluster(rabbitmqName, GetDefaultRabbitMQClusterSpec(false)) - DeferCleanup(DeleteRabbitMQCluster, rabbitmqName) - - // Create RabbitMq CR - spec := GetDefaultRabbitMQSpec() - rabbitmq := CreateRabbitMQ(rabbitmqName, spec) - DeferCleanup(th.DeleteInstance, rabbitmq) - - // Create a fake owner service (StatefulSet) - owner := &unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": "apps/v1", - "kind": "StatefulSet", - "metadata": map[string]any{ - "name": ownerName.Name, - "namespace": ownerName.Namespace, - }, - "spec": map[string]any{ - "replicas": int64(1), - "selector": map[string]any{ - "matchLabels": map[string]any{ - "app": "test", - }, - }, - "template": map[string]any{ - "metadata": map[string]any{ - "labels": map[string]any{ - "app": "test", - }, - }, - "spec": map[string]any{ - "containers": []any{ - map[string]any{ - "name": "test", - "image": "test:latest", - }, - }, - }, - }, - }, - }, - } - owner.SetGroupVersionKind(schema.GroupVersionKind{ - Group: "apps", - Version: "v1", - Kind: "StatefulSet", - }) - Expect(k8sClient.Create(ctx, owner)).Should(Succeed()) - DeferCleanup(k8sClient.Delete, owner) - - // Set owner status with Ready=True initially - Eventually(func(g Gomega) { - g.Expect(k8sClient.Get(ctx, ownerName, owner)).Should(Succeed()) - g.Expect(unstructured.SetNestedField(owner.Object, int64(1), "metadata", "generation")).Should(Succeed()) - g.Expect(unstructured.SetNestedField(owner.Object, int64(1), "status", "observedGeneration")).Should(Succeed()) - - conditions := []any{ - map[string]any{ - "type": "Ready", - "status": "True", - }, - } - g.Expect(unstructured.SetNestedSlice(owner.Object, conditions, "status", "conditions")).Should(Succeed()) - g.Expect(k8sClient.Status().Update(ctx, owner)).Should(Succeed()) - }, timeout, interval).Should(Succeed()) - - // Create TransportURL with owner reference - tu := &rabbitmqv1.TransportURL{ - ObjectMeta: metav1.ObjectMeta{ - Name: transportURLName.Name, - Namespace: transportURLName.Namespace, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: "apps/v1", - Kind: "StatefulSet", - Name: ownerName.Name, - UID: owner.GetUID(), - Controller: func() *bool { b := true; return &b }(), - }, - }, - }, - Spec: rabbitmqv1.TransportURLSpec{ - RabbitmqClusterName: rabbitmqName.Name, - Username: "notready-olduser", - }, - } - Expect(k8sClient.Create(ctx, tu)).Should(Succeed()) - DeferCleanup(th.DeleteInstance, tu) - }) - - It("should wait for owner to be ready before removing finalizer", func() { - SimulateRabbitMQClusterReady(rabbitmqName) - - // Wait for initial RabbitMQUser to be created - oldUserCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-notready-olduser-user", transportURLName.Name), - Namespace: namespace, - } - Eventually(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - g.Expect(k8sClient.Get(ctx, oldUserCRName, user)).Should(Succeed()) - g.Expect(user.Spec.Username).To(Equal("notready-olduser")) - }, timeout, interval).Should(Succeed()) - - // Simulate user being ready - SimulateRabbitMQUserReady(oldUserCRName, "/") - - // Verify old user has the TransportURL finalizer - Eventually(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - g.Expect(k8sClient.Get(ctx, oldUserCRName, user)).Should(Succeed()) - g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizer)).To(BeTrue()) - }, timeout, interval).Should(Succeed()) - - // Set owner to NOT ready - Eventually(func(g Gomega) { - owner := &unstructured.Unstructured{} - owner.SetGroupVersionKind(schema.GroupVersionKind{ - Group: "apps", - Version: "v1", - Kind: "StatefulSet", - }) - g.Expect(k8sClient.Get(ctx, ownerName, owner)).Should(Succeed()) - - conditions := []any{ - map[string]any{ - "type": "Ready", - "status": "False", - "reason": "Reconciling", - }, - } - g.Expect(unstructured.SetNestedSlice(owner.Object, conditions, "status", "conditions")).Should(Succeed()) - g.Expect(k8sClient.Status().Update(ctx, owner)).Should(Succeed()) - }, timeout, interval).Should(Succeed()) - - // Change username - Eventually(func(g Gomega) { - tr := th.GetTransportURL(transportURLName) - tr.Spec.Username = "notready-newuser" - g.Expect(k8sClient.Update(ctx, tr)).Should(Succeed()) - }, timeout, interval).Should(Succeed()) - - // Wait for new RabbitMQUser to be created - newUserCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-notready-newuser-user", transportURLName.Name), - Namespace: namespace, - } - Eventually(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - g.Expect(k8sClient.Get(ctx, newUserCRName, user)).Should(Succeed()) - }, timeout, interval).Should(Succeed()) - - // Simulate new user being ready - SimulateRabbitMQUserReady(newUserCRName, "/") - - // Verify old user finalizer is NOT removed (owner not ready) - Consistently(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - g.Expect(k8sClient.Get(ctx, oldUserCRName, user)).Should(Succeed()) - g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizer)).To(BeTrue(), - "TransportURL finalizer should NOT be removed while owner is not ready") - }, time.Second*5, interval).Should(Succeed()) - - // Set owner to Ready - Eventually(func(g Gomega) { - owner := &unstructured.Unstructured{} - owner.SetGroupVersionKind(schema.GroupVersionKind{ - Group: "apps", - Version: "v1", - Kind: "StatefulSet", - }) - g.Expect(k8sClient.Get(ctx, ownerName, owner)).Should(Succeed()) - - conditions := []any{ - map[string]any{ - "type": "Ready", - "status": "True", - }, - } - g.Expect(unstructured.SetNestedSlice(owner.Object, conditions, "status", "conditions")).Should(Succeed()) - g.Expect(k8sClient.Status().Update(ctx, owner)).Should(Succeed()) - }, timeout, interval).Should(Succeed()) - - // Now verify finalizer is removed - Eventually(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - g.Expect(k8sClient.Get(ctx, oldUserCRName, user)).Should(Succeed()) - g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizer)).To(BeFalse(), - "TransportURL finalizer should be removed after owner becomes ready") - }, timeout, interval).Should(Succeed()) - }) - }) - - When("username is changed and owner service is deleted", func() { - var rabbitmqName types.NamespacedName - var transportURLName types.NamespacedName - var ownerName types.NamespacedName - - BeforeEach(func() { - rabbitmqName = types.NamespacedName{ - Name: "rabbitmq-owner-deleted", - Namespace: namespace, - } - transportURLName = types.NamespacedName{ - Name: "transporturl-owner-deleted", - Namespace: namespace, - } - ownerName = types.NamespacedName{ - Name: "owner-deleted-deployment", - Namespace: namespace, - } - - // Create RabbitMQCluster first - CreateRabbitMQCluster(rabbitmqName, GetDefaultRabbitMQClusterSpec(false)) - DeferCleanup(DeleteRabbitMQCluster, rabbitmqName) - - // Create RabbitMq CR - spec := GetDefaultRabbitMQSpec() - rabbitmq := CreateRabbitMQ(rabbitmqName, spec) - DeferCleanup(th.DeleteInstance, rabbitmq) - - // Create a fake owner service (StatefulSet) - owner := &unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": "apps/v1", - "kind": "StatefulSet", - "metadata": map[string]any{ - "name": ownerName.Name, - "namespace": ownerName.Namespace, - }, - "spec": map[string]any{ - "replicas": int64(1), - "selector": map[string]any{ - "matchLabels": map[string]any{ - "app": "test", - }, - }, - "template": map[string]any{ - "metadata": map[string]any{ - "labels": map[string]any{ - "app": "test", - }, - }, - "spec": map[string]any{ - "containers": []any{ - map[string]any{ - "name": "test", - "image": "test:latest", - }, - }, - }, - }, - }, - }, - } - owner.SetGroupVersionKind(schema.GroupVersionKind{ - Group: "apps", - Version: "v1", - Kind: "StatefulSet", - }) - Expect(k8sClient.Create(ctx, owner)).Should(Succeed()) - - // Set owner status with Ready=True - Eventually(func(g Gomega) { - g.Expect(k8sClient.Get(ctx, ownerName, owner)).Should(Succeed()) - g.Expect(unstructured.SetNestedField(owner.Object, int64(1), "metadata", "generation")).Should(Succeed()) - g.Expect(unstructured.SetNestedField(owner.Object, int64(1), "status", "observedGeneration")).Should(Succeed()) - - conditions := []any{ - map[string]any{ - "type": "Ready", - "status": "True", - }, - } - g.Expect(unstructured.SetNestedSlice(owner.Object, conditions, "status", "conditions")).Should(Succeed()) - g.Expect(k8sClient.Status().Update(ctx, owner)).Should(Succeed()) - }, timeout, interval).Should(Succeed()) - - // Create TransportURL with owner reference - tu := &rabbitmqv1.TransportURL{ - ObjectMeta: metav1.ObjectMeta{ - Name: transportURLName.Name, - Namespace: transportURLName.Namespace, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: "apps/v1", - Kind: "StatefulSet", - Name: ownerName.Name, - UID: owner.GetUID(), - Controller: func() *bool { b := true; return &b }(), - }, - }, - }, - Spec: rabbitmqv1.TransportURLSpec{ - RabbitmqClusterName: rabbitmqName.Name, - Username: "deleted-olduser", - }, - } - Expect(k8sClient.Create(ctx, tu)).Should(Succeed()) - DeferCleanup(th.DeleteInstance, tu) - }) - - It("should proceed with cleanup when owner is deleted", func() { - SimulateRabbitMQClusterReady(rabbitmqName) - - // Wait for initial RabbitMQUser to be created - oldUserCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-deleted-olduser-user", transportURLName.Name), - Namespace: namespace, - } - Eventually(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - g.Expect(k8sClient.Get(ctx, oldUserCRName, user)).Should(Succeed()) - }, timeout, interval).Should(Succeed()) - - // Simulate user being ready - SimulateRabbitMQUserReady(oldUserCRName, "/") - - // Change username - Eventually(func(g Gomega) { - tr := th.GetTransportURL(transportURLName) - tr.Spec.Username = "deleted-newuser" - g.Expect(k8sClient.Update(ctx, tr)).Should(Succeed()) - }, timeout, interval).Should(Succeed()) - - // Wait for new RabbitMQUser to be created - newUserCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-deleted-newuser-user", transportURLName.Name), - Namespace: namespace, - } - Eventually(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - g.Expect(k8sClient.Get(ctx, newUserCRName, user)).Should(Succeed()) - }, timeout, interval).Should(Succeed()) - - // Simulate new user being ready - SimulateRabbitMQUserReady(newUserCRName, "/") - - // Delete the owner service - Eventually(func(g Gomega) { - owner := &unstructured.Unstructured{} - owner.SetGroupVersionKind(schema.GroupVersionKind{ - Group: "apps", - Version: "v1", - Kind: "StatefulSet", - }) - g.Expect(k8sClient.Get(ctx, ownerName, owner)).Should(Succeed()) - g.Expect(k8sClient.Delete(ctx, owner)).Should(Succeed()) - }, timeout, interval).Should(Succeed()) - - // Verify owner is deleted - Eventually(func(g Gomega) { - owner := &unstructured.Unstructured{} - owner.SetGroupVersionKind(schema.GroupVersionKind{ - Group: "apps", - Version: "v1", - Kind: "StatefulSet", - }) - err := k8sClient.Get(ctx, ownerName, owner) - g.Expect(k8s_errors.IsNotFound(err)).To(BeTrue(), "Owner should be deleted") - }, timeout, interval).Should(Succeed()) - - // Verify old user's TransportURL finalizer is removed (cleanup proceeds) - Eventually(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - err := k8sClient.Get(ctx, oldUserCRName, user) - // User might still exist but should not have the TransportURL finalizer - if err == nil { - g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizer)).To(BeFalse(), - "TransportURL finalizer should be removed even when owner is deleted") - } - }, timeout, interval).Should(Succeed()) - }) - }) - When("username is changed for standalone TransportURL without owner", func() { var rabbitmqName types.NamespacedName var transportURLName types.NamespacedName @@ -1627,7 +1097,7 @@ var _ = Describe("TransportURL controller", func() { // Wait for initial RabbitMQUser to be created oldUserCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-noowner-olduser-user", transportURLName.Name), + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "noowner-olduser"), Namespace: namespace, } Eventually(func(g Gomega) { @@ -1653,7 +1123,7 @@ var _ = Describe("TransportURL controller", func() { // Wait for new RabbitMQUser to be created newUserCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-noowner-newuser-user", transportURLName.Name), + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "noowner-newuser"), Namespace: namespace, } Eventually(func(g Gomega) { @@ -1664,38 +1134,15 @@ var _ = Describe("TransportURL controller", func() { // Simulate new user being ready SimulateRabbitMQUserReady(newUserCRName, "/") - // Verify old user's TransportURL finalizer is removed promptly (no owner to wait for) + // Verify old user's per-consumer finalizer is removed but user persists Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} - err := k8sClient.Get(ctx, oldUserCRName, user) - // User might still exist but should not have the TransportURL finalizer - if err == nil { - g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizer)).To(BeFalse(), - "TransportURL finalizer should be removed immediately when there is no owner") - } - }, timeout, interval).Should(Succeed()) - - // Manually remove the cleanup-blocked finalizer to allow deletion to proceed - Eventually(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - err := k8sClient.Get(ctx, oldUserCRName, user) - if err == nil && controllerutil.ContainsFinalizer(user, rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer) { - controllerutil.RemoveFinalizer(user, rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer) - g.Expect(k8sClient.Update(ctx, user)).Should(Succeed()) - } + g.Expect(k8sClient.Get(ctx, oldUserCRName, user)).Should(Succeed()) + g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizerFor(transportURLName.Name))).To(BeFalse(), + "TransportURL finalizer should be removed") + g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer)).To(BeTrue(), + "Cleanup-blocked finalizer should still be present") }, timeout, interval).Should(Succeed()) - - // Verify cleanup proceeds and user is eventually deleted - Eventually(func(g Gomega) { - user := &rabbitmqv1.RabbitMQUser{} - err := k8sClient.Get(ctx, oldUserCRName, user) - // Either NotFound or has DeletionTimestamp set - if err == nil { - g.Expect(user.DeletionTimestamp.IsZero()).To(BeFalse(), "Old user should have DeletionTimestamp set") - } else { - g.Expect(k8s_errors.IsNotFound(err)).To(BeTrue(), "Old user should be NotFound") - } - }, time.Second*45, interval).Should(Succeed()) }) }) @@ -1965,24 +1412,24 @@ var _ = Describe("TransportURL controller", func() { // Wait for RabbitMQVhost and RabbitMQUser to be created vhostCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-delete-test-vhost-vhost", transportURLName.Name), + Name: rabbitmqv1.CanonicalVhostName(rabbitmqName.Name, "delete-test-vhost"), Namespace: namespace, } userCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-delete-test-user-user", transportURLName.Name), + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "delete-test-vhost", "delete-test-user"), Namespace: namespace, } Eventually(func(g Gomega) { vhost := &rabbitmqv1.RabbitMQVhost{} g.Expect(k8sClient.Get(ctx, vhostCRName, vhost)).Should(Succeed()) - g.Expect(controllerutil.ContainsFinalizer(vhost, rabbitmqv1.TransportURLFinalizer)).To(BeTrue()) + g.Expect(controllerutil.ContainsFinalizer(vhost, rabbitmqv1.TransportURLFinalizerFor(transportURLName.Name))).To(BeTrue()) }, timeout, interval).Should(Succeed()) Eventually(func(g Gomega) { user := &rabbitmqv1.RabbitMQUser{} g.Expect(k8sClient.Get(ctx, userCRName, user)).Should(Succeed()) - g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizer)).To(BeTrue()) + g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizerFor(transportURLName.Name))).To(BeTrue()) g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer)).To(BeTrue()) }, timeout, interval).Should(Succeed()) @@ -2007,7 +1454,7 @@ var _ = Describe("TransportURL controller", func() { user := &rabbitmqv1.RabbitMQUser{} err := k8sClient.Get(ctx, userCRName, user) if err == nil { - g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizer)).To(BeFalse(), + g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizerFor(transportURLName.Name))).To(BeFalse(), "TransportURL finalizer should be removed from user") g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.RabbitMQUserCleanupBlockedFinalizer)).To(BeFalse(), "Cleanup-blocked finalizer should be removed from user") @@ -2018,7 +1465,7 @@ var _ = Describe("TransportURL controller", func() { vhost := &rabbitmqv1.RabbitMQVhost{} err := k8sClient.Get(ctx, vhostCRName, vhost) if err == nil { - g.Expect(controllerutil.ContainsFinalizer(vhost, rabbitmqv1.TransportURLFinalizer)).To(BeFalse(), + g.Expect(controllerutil.ContainsFinalizer(vhost, rabbitmqv1.TransportURLFinalizerFor(transportURLName.Name))).To(BeFalse(), "TransportURL finalizer should be removed from vhost") } }, timeout, interval).Should(Succeed()) @@ -2067,7 +1514,7 @@ var _ = Describe("TransportURL controller", func() { // Wait for RabbitMQUser to be created customUserCRName := types.NamespacedName{ - Name: fmt.Sprintf("%s-custom-user-user", transportURLName.Name), + Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "custom-user"), Namespace: namespace, } Eventually(func(g Gomega) { @@ -2121,7 +1568,7 @@ var _ = Describe("TransportURL controller", func() { user := &rabbitmqv1.RabbitMQUser{} err := k8sClient.Get(ctx, customUserCRName, user) if err == nil { - g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizer)).To(BeFalse(), + g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizerFor(transportURLName.Name))).To(BeFalse(), "TransportURL finalizer should be removed from old user") } }, timeout, interval).Should(Succeed()) @@ -2134,4 +1581,111 @@ var _ = Describe("TransportURL controller", func() { ) }) }) + + When("TransportURL is upgraded from legacy per-TransportURL user CRs to shared singletons", func() { + var migrationClusterName types.NamespacedName + var migrationTransportURLName types.NamespacedName + + BeforeEach(func() { + migrationClusterName = types.NamespacedName{Name: "rabbitmq-migration", Namespace: namespace} + migrationTransportURLName = types.NamespacedName{Name: "migration-transport", Namespace: namespace} + + CreateRabbitMQCluster(migrationClusterName, GetDefaultRabbitMQClusterSpec(false)) + SimulateRabbitMQClusterReady(migrationClusterName) + DeferCleanup(DeleteRabbitMQCluster, migrationClusterName) + }) + + It("should clean up the legacy user CR and create the canonical one", func() { + // Step 1: Create the legacy user CR first (simulating pre-upgrade state). + // At this point no TransportURL exists yet, so no webhook conflict. + legacyUserName := types.NamespacedName{ + Name: migrationTransportURLName.Name + "-migrate-user-user", + Namespace: namespace, + } + legacyUser := CreateRabbitMQUser(legacyUserName, map[string]any{ + "rabbitmqClusterName": migrationClusterName.Name, + "username": "migrate-user", + }) + DeferCleanup(func(_ types.NamespacedName) { + _ = k8sClient.Delete(ctx, legacyUser) + }, legacyUserName) + + // Add legacy finalizer + Eventually(func(g Gomega) { + user := &rabbitmqv1.RabbitMQUser{} + g.Expect(k8sClient.Get(ctx, legacyUserName, user)).Should(Succeed()) + controllerutil.AddFinalizer(user, rabbitmqv1.TransportURLFinalizer) + g.Expect(k8sClient.Update(ctx, user)).Should(Succeed()) + }, timeout, interval).Should(Succeed()) + + SimulateRabbitMQUserReady(legacyUserName, "/") + + // Step 2: Create the TransportURL (simulating the new code running after upgrade) + spec := map[string]any{ + "rabbitmqClusterName": migrationClusterName.Name, + "username": "migrate-user", + } + turl := CreateTransportURL(migrationTransportURLName, spec) + DeferCleanup(th.DeleteInstance, turl) + + // Add the owner reference from TransportURL to legacy user CR + // (simulating what the old controller would have set up) + turlObj := th.GetTransportURL(migrationTransportURLName) + Eventually(func(g Gomega) { + user := &rabbitmqv1.RabbitMQUser{} + g.Expect(k8sClient.Get(ctx, legacyUserName, user)).Should(Succeed()) + + isTrue := true + user.SetOwnerReferences([]metav1.OwnerReference{ + { + APIVersion: "rabbitmq.openstack.org/v1beta1", + Kind: "TransportURL", + Name: migrationTransportURLName.Name, + UID: turlObj.UID, + Controller: &isTrue, + BlockOwnerDeletion: &isTrue, + }, + }) + g.Expect(k8sClient.Update(ctx, user)).Should(Succeed()) + }, timeout, interval).Should(Succeed()) + + // Step 3: The controller should: + // 1. Find the legacy CR (owner ref match), remove its legacy finalizer + // 2. Create the canonical user CR (webhook skips legacy CRs with TransportURL owner refs) + // 3. Legacy CR stays alive until TransportURL is deleted (GC via owner ref) + + canonicalUserName := types.NamespacedName{ + Name: rabbitmqv1.CanonicalUserName(migrationClusterName.Name, "/", "migrate-user"), + Namespace: namespace, + } + + // Wait for the canonical user CR to be created + Eventually(func(g Gomega) { + user := &rabbitmqv1.RabbitMQUser{} + g.Expect(k8sClient.Get(ctx, canonicalUserName, user)).Should(Succeed()) + g.Expect(user.Spec.Username).To(Equal("migrate-user")) + g.Expect(user.Spec.RabbitmqClusterName).To(Equal(migrationClusterName.Name)) + g.Expect(controllerutil.ContainsFinalizer(user, rabbitmqv1.TransportURLFinalizerFor(migrationTransportURLName.Name))).To(BeTrue()) + }, timeout, interval).Should(Succeed()) + + // Simulate the canonical user becoming ready + SimulateRabbitMQUserReady(canonicalUserName, "/") + + // Wait for the legacy user CR to be deleted + Eventually(func(g Gomega) { + user := &rabbitmqv1.RabbitMQUser{} + err := k8sClient.Get(ctx, legacyUserName, user) + g.Expect(k8s_errors.IsNotFound(err)).To(BeTrue(), + "Legacy user CR should be deleted after canonical CR exists") + }, timeout, interval).Should(Succeed()) + + // TransportURL should eventually become ready + th.ExpectCondition( + migrationTransportURLName, + ConditionGetterFunc(TransportURLConditionGetter), + rabbitmqv1.TransportURLReadyCondition, + corev1.ConditionTrue, + ) + }) + }) }) diff --git a/test/kuttl/tests/rabbitmq-credential-rotation/01-assert.yaml b/test/kuttl/tests/rabbitmq-credential-rotation/01-assert.yaml index 65be8b7f..f5eb4ee1 100644 --- a/test/kuttl/tests/rabbitmq-credential-rotation/01-assert.yaml +++ b/test/kuttl/tests/rabbitmq-credential-rotation/01-assert.yaml @@ -25,13 +25,13 @@ commands: echo "PASS: Transport URL contains nova username" - script: | set -e - # Verify the RabbitMQUser was created (naming: --user) - READY=$(oc get rabbitmquser -n $NAMESPACE cred-rot-url-nova-user -o jsonpath='{.status.conditions[?(@.type=="Ready")].status}') + # Verify the RabbitMQUser was created (canonical naming: -user-) + READY=$(oc get rabbitmquser -n $NAMESPACE rabbitmq-cred-rot-user-nova -o jsonpath='{.status.conditions[?(@.type=="Ready")].status}') if [ "$READY" != "True" ]; then echo "FAIL: RabbitMQUser not ready" exit 1 fi - USERNAME=$(oc get rabbitmquser -n $NAMESPACE cred-rot-url-nova-user -o jsonpath='{.status.username}') + USERNAME=$(oc get rabbitmquser -n $NAMESPACE rabbitmq-cred-rot-user-nova -o jsonpath='{.status.username}') if [ "$USERNAME" != "nova" ]; then echo "FAIL: Expected username 'nova', got: '$USERNAME'" exit 1 diff --git a/test/kuttl/tests/rabbitmq-credential-rotation/02-assert.yaml b/test/kuttl/tests/rabbitmq-credential-rotation/02-assert.yaml index 3387bdda..2a8c2d66 100644 --- a/test/kuttl/tests/rabbitmq-credential-rotation/02-assert.yaml +++ b/test/kuttl/tests/rabbitmq-credential-rotation/02-assert.yaml @@ -40,11 +40,23 @@ commands: echo "PASS: User 'glance' exists in RabbitMQ" - script: | set -e - # Old user CR should still exist (blocked by cleanup-blocked finalizer) - # but TransportURL finalizer should have been removed by the controller - FINALIZERS=$(oc get rabbitmquser -n $NAMESPACE cred-rot-url-nova-user -o jsonpath='{.metadata.finalizers}') - if echo "$FINALIZERS" | grep -q "transporturl.rabbitmq.openstack.org/finalizer"; then - echo "FAIL: TransportURL finalizer still present on old user" + # Old user CR should be marked as orphaned (label set) but NOT deleted, + # because cleanup-blocked finalizer requires admin approval before deletion. + # The per-consumer finalizer should have been removed by the controller. + ORPHANED=$(oc get rabbitmquser -n $NAMESPACE rabbitmq-cred-rot-user-nova -o jsonpath='{.metadata.labels.rabbitmq\.openstack\.org/orphaned}') + if [ "$ORPHANED" != "true" ]; then + echo "FAIL: Old user CR not marked as orphaned" + oc get rabbitmquser -n $NAMESPACE rabbitmq-cred-rot-user-nova -o yaml + exit 1 + fi + DELTS=$(oc get rabbitmquser -n $NAMESPACE rabbitmq-cred-rot-user-nova -o jsonpath='{.metadata.deletionTimestamp}') + if [ -n "$DELTS" ]; then + echo "FAIL: Old user CR should NOT have DeletionTimestamp (orphaned label design)" + exit 1 + fi + FINALIZERS=$(oc get rabbitmquser -n $NAMESPACE rabbitmq-cred-rot-user-nova -o jsonpath='{.metadata.finalizers}') + if echo "$FINALIZERS" | grep -q "turl.openstack.org/t-cred-rot-url"; then + echo "FAIL: Per-consumer finalizer still present on old user" echo "Finalizers: $FINALIZERS" exit 1 fi @@ -53,4 +65,4 @@ commands: echo "Finalizers: $FINALIZERS" exit 1 fi - echo "PASS: Old user has cleanup-blocked finalizer, TransportURL finalizer removed" + echo "PASS: Old user is orphaned, blocked by cleanup-blocked finalizer" diff --git a/test/kuttl/tests/rabbitmq-credential-rotation/03-assert.yaml b/test/kuttl/tests/rabbitmq-credential-rotation/03-assert.yaml index e52e5712..c866db67 100644 --- a/test/kuttl/tests/rabbitmq-credential-rotation/03-assert.yaml +++ b/test/kuttl/tests/rabbitmq-credential-rotation/03-assert.yaml @@ -8,11 +8,11 @@ commands: - script: | set -e # Verify old user CR is gone - if oc get rabbitmquser -n $NAMESPACE cred-rot-url-nova-user 2>/dev/null; then - echo "FAIL: Old user CR cred-rot-url-nova-user still exists" + if oc get rabbitmquser -n $NAMESPACE rabbitmq-cred-rot-user-nova 2>/dev/null; then + echo "FAIL: Old user CR rabbitmq-cred-rot-user-nova still exists" exit 1 fi - echo "PASS: Old user CR cred-rot-url-nova-user deleted" + echo "PASS: Old user CR rabbitmq-cred-rot-user-nova deleted" - script: | set -e # Verify old user was deleted from RabbitMQ diff --git a/test/kuttl/tests/rabbitmq-credential-rotation/03-remove-blocked-finalizer.yaml b/test/kuttl/tests/rabbitmq-credential-rotation/03-remove-blocked-finalizer.yaml index 312af872..2258a440 100644 --- a/test/kuttl/tests/rabbitmq-credential-rotation/03-remove-blocked-finalizer.yaml +++ b/test/kuttl/tests/rabbitmq-credential-rotation/03-remove-blocked-finalizer.yaml @@ -1,7 +1,7 @@ # -# Step 3: Remove the cleanup-blocked finalizer from the old user to unblock deletion. -# In production, an external controller or operator would do this after confirming -# the owning service has fully switched to the new credentials. +# Step 3: Remove the cleanup-blocked finalizer from the orphaned user to approve deletion. +# In production, an admin would do this after confirming the owning service has fully +# switched to the new credentials. The user controller will then auto-delete the CR. # apiVersion: kuttl.dev/v1beta1 kind: TestStep @@ -9,11 +9,11 @@ commands: - script: | set -e # Get current finalizers and remove only cleanup-blocked - FINALIZERS=$(oc get rabbitmquser cred-rot-url-nova-user -n $NAMESPACE -o jsonpath='{.metadata.finalizers}') + FINALIZERS=$(oc get rabbitmquser rabbitmq-cred-rot-user-nova -n $NAMESPACE -o jsonpath='{.metadata.finalizers}') echo "Current finalizers: $FINALIZERS" # Build new finalizer list excluding cleanup-blocked - NEW_FINALIZERS=$(oc get rabbitmquser cred-rot-url-nova-user -n $NAMESPACE -o json | \ + NEW_FINALIZERS=$(oc get rabbitmquser rabbitmq-cred-rot-user-nova -n $NAMESPACE -o json | \ python3 -c " import sys, json obj = json.load(sys.stdin) @@ -21,6 +21,6 @@ commands: print(json.dumps(finalizers)) ") - oc patch rabbitmquser cred-rot-url-nova-user -n $NAMESPACE --type=merge \ + oc patch rabbitmquser rabbitmq-cred-rot-user-nova -n $NAMESPACE --type=merge \ -p "{\"metadata\":{\"finalizers\":$NEW_FINALIZERS}}" echo "PASS: Removed cleanup-blocked finalizer from old user" diff --git a/test/kuttl/tests/rabbitmq-credential-rotation/04-cleanup.yaml b/test/kuttl/tests/rabbitmq-credential-rotation/04-cleanup.yaml index 6e8f7729..fcc45762 100644 --- a/test/kuttl/tests/rabbitmq-credential-rotation/04-cleanup.yaml +++ b/test/kuttl/tests/rabbitmq-credential-rotation/04-cleanup.yaml @@ -5,6 +5,9 @@ commands: oc delete transporturl cred-rot-url -n $NAMESPACE --ignore-not-found oc delete rabbitmquser -n $NAMESPACE -l app.kubernetes.io/managed-by=transporturl --ignore-not-found oc delete rabbitmqvhost -n $NAMESPACE -l app.kubernetes.io/managed-by=transporturl --ignore-not-found + # Clean up canonical CRs created by the shared singleton model + oc delete rabbitmquser rabbitmq-cred-rot-user-nova -n $NAMESPACE --ignore-not-found + oc delete rabbitmquser rabbitmq-cred-rot-user-glance -n $NAMESPACE --ignore-not-found oc delete rabbitmq.rabbitmq.openstack.org rabbitmq-cred-rot -n $NAMESPACE --ignore-not-found oc delete pvc -n $NAMESPACE -l app.kubernetes.io/name=rabbitmq-cred-rot --ignore-not-found # Clean up Released PVs to avoid stale data on re-runs