Merge pull request #577 from lmiccini/shared-singleton-vhost-user

Use shared singleton vhost/user CRs with per-consumer finalizers

Author: openshift-merge-bot[bot]

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

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,52 @@ 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
+
+	// MaxTransportURLDirectName is the maximum TransportURL name length that
+	// can be embedded directly (without hashing) into a per-consumer finalizer.
+	// Names longer than this are truncated+hashed, which breaks the watch-based
+	// reverse mapping and requires a periodic requeue fallback.
+	MaxTransportURLDirectName = maxFinalizerNameSegment - len("t-") // 61
+
+	// 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 ()
 

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)
+	}
+}

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

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,10 +97,29 @@ func (r *RabbitMQPolicy) ValidateUpdate(_ client.Client, old runtime.Object) (ad
 		)
 	}
 
+	if err := r.validatePattern(); err != nil {
+		return nil, err
+	}
+
 	return nil, nil
 }
 
 // ValidateDelete validates the RabbitMQPolicy on deletion
 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
+}

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)
 }