pr feedback 1

Author: lmiccini

apis/bases/rabbitmq.openstack.org_rabbitmqvhosts.yaml

@@ -60,6 +60,7 @@ spec:
               name:
                 default: /
                 description: Name - the vhost name in RabbitMQ (defaults to "/")
+                minLength: 1
                 type: string
               rabbitmqClusterName:
                 description: RabbitmqClusterName - the name of the RabbitMQ cluster

apis/rabbitmq/v1beta1/conditions.go

@@ -23,6 +23,9 @@ import (
 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 = "transporturl.rabbitmq.openstack.org/finalizer"
 )
 
 // TransportURL Reasons used by API objects.

apis/rabbitmq/v1beta1/rabbitmq_types.go

@@ -43,6 +43,14 @@ const (
 	CrMaxLengthCorrection   = 11
 	errInvalidOverride      = "invalid spec override (%s)"
 	warnOverrideStatefulSet = "%s: is deprecated and will be removed in a future API version"
+
+	// Queue types
+	// QueueTypeMirrored - mirrored queue type
+	QueueTypeMirrored = "Mirrored"
+	// QueueTypeQuorum - quorum queue type
+	QueueTypeQuorum = "Quorum"
+	// QueueTypeNone - no special queue type
+	QueueTypeNone = "None"
 )
 
 // PodOverride defines per-pod service configurations

apis/rabbitmq/v1beta1/rabbitmqpolicy_types.go

@@ -104,15 +104,15 @@ func (instance RabbitMQPolicy) IsReady() bool {
 }
 
 const (
-	// PolicyReadyCondition indicates that the policy is ready
-	PolicyReadyCondition condition.Type = "PolicyReady"
+	// RabbitMQPolicyReadyCondition indicates that the policy is ready
+	RabbitMQPolicyReadyCondition condition.Type = "RabbitMQPolicyReady"
 
-	// PolicyReadyMessage is the message for the PolicyReady condition
-	PolicyReadyMessage = "RabbitMQ policy is ready"
+	// RabbitMQPolicyReadyMessage is the message for the RabbitMQPolicyReady condition
+	RabbitMQPolicyReadyMessage = "RabbitMQ policy is ready"
 
-	// PolicyReadyInitMessage is the message for the PolicyReady condition when not started
-	PolicyReadyInitMessage = "RabbitMQ policy not started"
+	// RabbitMQPolicyReadyInitMessage is the message for the RabbitMQPolicyReady condition when not started
+	RabbitMQPolicyReadyInitMessage = "RabbitMQ policy not started"
 
-	// PolicyReadyErrorMessage is the message format for the PolicyReady condition when an error occurs
-	PolicyReadyErrorMessage = "RabbitMQ policy error occurred %s"
+	// RabbitMQPolicyReadyErrorMessage is the message format for the RabbitMQPolicyReady condition when an error occurs
+	RabbitMQPolicyReadyErrorMessage = "RabbitMQ policy error occurred %s"
 )

apis/rabbitmq/v1beta1/rabbitmquser_types.go

@@ -120,15 +120,15 @@ const (
 	// UserFinalizer - finalizer to protect user from deletion when owned by TransportURL
 	UserFinalizer = "rabbitmquser.rabbitmq.openstack.org/finalizer"
 
-	// UserReadyCondition indicates that the user is ready
-	UserReadyCondition condition.Type = "UserReady"
+	// RabbitMQUserReadyCondition indicates that the user is ready
+	RabbitMQUserReadyCondition condition.Type = "RabbitMQUserReady"
 
-	// UserReadyMessage is the message for the UserReady condition
-	UserReadyMessage = "RabbitMQ user is ready"
+	// RabbitMQUserReadyMessage is the message for the RabbitMQUserReady condition
+	RabbitMQUserReadyMessage = "RabbitMQ user is ready"
 
-	// UserReadyInitMessage is the message for the UserReady condition when not started
-	UserReadyInitMessage = "RabbitMQ user not started"
+	// RabbitMQUserReadyInitMessage is the message for the RabbitMQUserReady condition when not started
+	RabbitMQUserReadyInitMessage = "RabbitMQ user not started"
 
-	// UserReadyErrorMessage is the message format for the UserReady condition when an error occurs
-	UserReadyErrorMessage = "RabbitMQ user error occurred %s"
+	// RabbitMQUserReadyErrorMessage is the message format for the RabbitMQUserReady condition when an error occurs
+	RabbitMQUserReadyErrorMessage = "RabbitMQ user error occurred %s"
 )

apis/rabbitmq/v1beta1/rabbitmqvhost_types.go

@@ -28,6 +28,7 @@ type RabbitMQVhostSpec struct {
 	RabbitmqClusterName string `json:"rabbitmqClusterName"`
 
 	// +kubebuilder:validation:Optional
+	// +kubebuilder:validation:MinLength=1
 	// +kubebuilder:default="/"
 	// Name - the vhost name in RabbitMQ (defaults to "/")
 	Name string `json:"name"`
@@ -81,15 +82,15 @@ const (
 	// VhostFinalizer - finalizer to protect vhost from deletion when owned by TransportURL
 	VhostFinalizer = "rabbitmqvhost.rabbitmq.openstack.org/finalizer"
 
-	// VhostReadyCondition indicates that the vhost is ready
-	VhostReadyCondition condition.Type = "VhostReady"
+	// RabbitMQVhostReadyCondition indicates that the vhost is ready
+	RabbitMQVhostReadyCondition condition.Type = "RabbitMQVhostReady"
 
-	// VhostReadyMessage is the message for the VhostReady condition
-	VhostReadyMessage = "RabbitMQ vhost is ready"
+	// RabbitMQVhostReadyMessage is the message for the RabbitMQVhostReady condition
+	RabbitMQVhostReadyMessage = "RabbitMQ vhost is ready"
 
-	// VhostReadyInitMessage is the message for the VhostReady condition when not started
-	VhostReadyInitMessage = "RabbitMQ vhost not started"
+	// RabbitMQVhostReadyInitMessage is the message for the RabbitMQVhostReady condition when not started
+	RabbitMQVhostReadyInitMessage = "RabbitMQ vhost not started"
 
-	// VhostReadyErrorMessage is the message format for the VhostReady condition when an error occurs
-	VhostReadyErrorMessage = "RabbitMQ vhost error occurred %s"
+	// RabbitMQVhostReadyErrorMessage is the message format for the RabbitMQVhostReady condition when an error occurs
+	RabbitMQVhostReadyErrorMessage = "RabbitMQ vhost error occurred %s"
 )

config/crd/bases/rabbitmq.openstack.org_rabbitmqvhosts.yaml

@@ -60,6 +60,7 @@ spec:
               name:
                 default: /
                 description: Name - the vhost name in RabbitMQ (defaults to "/")
+                minLength: 1
                 type: string
               rabbitmqClusterName:
                 description: RabbitmqClusterName - the name of the RabbitMQ cluster

internal/controller/rabbitmq/rabbitmq_controller.go

@@ -466,7 +466,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ct
 		// Let's wait DeploymentReadyCondition=True to apply the policy
 		// QueueType should never be nil due to webhook defaulting, but add safety check
 		if instance.Spec.QueueType != nil {
-			if *instance.Spec.QueueType == "Mirrored" && *instance.Spec.Replicas > 1 && instance.Status.QueueType != "Mirrored" {
+			if *instance.Spec.QueueType == rabbitmqv1beta1.QueueTypeMirrored && *instance.Spec.Replicas > 1 && instance.Status.QueueType != rabbitmqv1beta1.QueueTypeMirrored {
 				Log.Info("ha-all policy not present. Applying.")
 				err := updateMirroredPolicy(ctx, helper, instance, true)
 				if err != nil {
@@ -478,8 +478,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ct
 						condition.DeploymentReadyErrorMessage, err.Error()))
 					return ctrl.Result{}, err
 				}
-				instance.Status.QueueType = "Mirrored"
-			} else if *instance.Spec.QueueType != "Mirrored" && instance.Status.QueueType == "Mirrored" {
+				instance.Status.QueueType = rabbitmqv1beta1.QueueTypeMirrored
+			} else if *instance.Spec.QueueType != rabbitmqv1beta1.QueueTypeMirrored && instance.Status.QueueType == rabbitmqv1beta1.QueueTypeMirrored {
 				Log.Info("QueueType changed from Mirrored. Removing ha-all policy")
 				err := updateMirroredPolicy(ctx, helper, instance, false)
 				if err != nil {
@@ -495,16 +495,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ct
 			}
 
 			// Update status for Quorum queue type
-			if *instance.Spec.QueueType == "Quorum" && instance.Status.QueueType != "Quorum" {
+			if *instance.Spec.QueueType == rabbitmqv1beta1.QueueTypeQuorum && instance.Status.QueueType != rabbitmqv1beta1.QueueTypeQuorum {
 				Log.Info("Setting queue type status to quorum")
-				instance.Status.QueueType = "Quorum"
-			} else if *instance.Spec.QueueType != "Quorum" && instance.Status.QueueType == "Quorum" {
+				instance.Status.QueueType = rabbitmqv1beta1.QueueTypeQuorum
+			} else if *instance.Spec.QueueType != rabbitmqv1beta1.QueueTypeQuorum && instance.Status.QueueType == rabbitmqv1beta1.QueueTypeQuorum {
 				Log.Info("Removing quorum queue type status")
 				instance.Status.QueueType = ""
 			}
 
 			// Update status for None queue type
-			if *instance.Spec.QueueType == "None" && instance.Status.QueueType != "" {
+			if *instance.Spec.QueueType == rabbitmqv1beta1.QueueTypeNone && instance.Status.QueueType != "" {
 				Log.Info("Setting queue type status to None (clearing)")
 				instance.Status.QueueType = ""
 			}

internal/controller/rabbitmq/rabbitmqpolicy_controller.go

@@ -68,18 +68,22 @@ func (r *RabbitMQPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 
 	h, _ := helper.NewHelper(instance, r.Client, r.Kclient, r.Scheme, Log)
 
-	// Initialize status
-	if instance.Status.Conditions == nil {
-		instance.Status.Conditions = condition.Conditions{}
-		cl := condition.CreateList(
-			condition.UnknownCondition(condition.ReadyCondition, condition.InitReason, condition.ReadyInitMessage),
-			condition.UnknownCondition(rabbitmqv1.PolicyReadyCondition, condition.InitReason, rabbitmqv1.PolicyReadyInitMessage),
-		)
-		instance.Status.Conditions.Init(&cl)
-		instance.Status.ObservedGeneration = instance.Generation
-	}
+	// Save a copy of the conditions so that we can restore the LastTransitionTime
+	// when a condition's state doesn't change
+	savedConditions := instance.Status.Conditions.DeepCopy()
+
+	// Initialize status conditions
+	cl := condition.CreateList(
+		condition.UnknownCondition(condition.ReadyCondition, condition.InitReason, condition.ReadyInitMessage),
+		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
+		condition.RestoreLastTransitionTimes(&instance.Status.Conditions, savedConditions)
+
 		if instance.Status.Conditions.IsUnknown(condition.ReadyCondition) {
 			instance.Status.Conditions.Set(instance.Status.Conditions.Mirror(condition.ReadyCondition))
 		}
@@ -93,10 +97,11 @@ func (r *RabbitMQPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return r.reconcileDelete(ctx, instance, h)
 	}
 
-	// Add finalizer
+	// Add finalizer if not being deleted
 	if !controllerutil.ContainsFinalizer(instance, policyFinalizer) {
 		controllerutil.AddFinalizer(instance, policyFinalizer)
-		return ctrl.Result{Requeue: true}, nil
+		// No need to requeue, the update will trigger a reconcile
+		return ctrl.Result{}, nil
 	}
 
 	return r.reconcileNormal(ctx, instance, h)
@@ -115,7 +120,7 @@ func (r *RabbitMQPolicyReconciler) reconcileNormal(ctx context.Context, instance
 		vhost := &rabbitmqv1.RabbitMQVhost{}
 		err := r.Get(ctx, types.NamespacedName{Name: instance.Spec.VhostRef, Namespace: instance.Namespace}, vhost)
 		if err != nil {
-			instance.Status.Conditions.Set(condition.FalseCondition(rabbitmqv1.PolicyReadyCondition, condition.ErrorReason, condition.SeverityWarning, rabbitmqv1.PolicyReadyErrorMessage, err.Error()))
+			instance.Status.Conditions.Set(condition.FalseCondition(rabbitmqv1.RabbitMQPolicyReadyCondition, condition.ErrorReason, condition.SeverityWarning, rabbitmqv1.RabbitMQPolicyReadyErrorMessage, err.Error()))
 			return ctrl.Result{}, err
 		}
 		vhostName = vhost.Spec.Name
@@ -128,14 +133,14 @@ func (r *RabbitMQPolicyReconciler) reconcileNormal(ctx context.Context, instance
 	rabbit := &rabbitmqclusterv2.RabbitmqCluster{}
 	err := r.Get(ctx, types.NamespacedName{Name: instance.Spec.RabbitmqClusterName, Namespace: instance.Namespace}, rabbit)
 	if err != nil {
-		instance.Status.Conditions.Set(condition.FalseCondition(rabbitmqv1.PolicyReadyCondition, condition.ErrorReason, condition.SeverityWarning, rabbitmqv1.PolicyReadyErrorMessage, err.Error()))
+		instance.Status.Conditions.Set(condition.FalseCondition(rabbitmqv1.RabbitMQPolicyReadyCondition, condition.ErrorReason, condition.SeverityWarning, rabbitmqv1.RabbitMQPolicyReadyErrorMessage, err.Error()))
 		return ctrl.Result{}, err
 	}
 
 	// Get admin credentials
 	rabbitSecret, _, err := oko_secret.GetSecret(ctx, h, rabbit.Status.DefaultUser.SecretReference.Name, instance.Namespace)
 	if err != nil {
-		instance.Status.Conditions.Set(condition.FalseCondition(rabbitmqv1.PolicyReadyCondition, condition.ErrorReason, condition.SeverityWarning, rabbitmqv1.PolicyReadyErrorMessage, err.Error()))
+		instance.Status.Conditions.Set(condition.FalseCondition(rabbitmqv1.RabbitMQPolicyReadyCondition, condition.ErrorReason, condition.SeverityWarning, rabbitmqv1.RabbitMQPolicyReadyErrorMessage, err.Error()))
 		return ctrl.Result{}, err
 	}
 
@@ -157,16 +162,16 @@ func (r *RabbitMQPolicyReconciler) reconcileNormal(ctx context.Context, instance
 	}
 	var definition map[string]interface{}
 	if err := json.Unmarshal(instance.Spec.Definition.Raw, &definition); err != nil {
-		instance.Status.Conditions.Set(condition.FalseCondition(rabbitmqv1.PolicyReadyCondition, condition.ErrorReason, condition.SeverityWarning, rabbitmqv1.PolicyReadyErrorMessage, err.Error()))
+		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, applyTo)
 	if err != nil {
-		instance.Status.Conditions.Set(condition.FalseCondition(rabbitmqv1.PolicyReadyCondition, condition.ErrorReason, condition.SeverityWarning, rabbitmqv1.PolicyReadyErrorMessage, err.Error()))
+		instance.Status.Conditions.Set(condition.FalseCondition(rabbitmqv1.RabbitMQPolicyReadyCondition, condition.ErrorReason, condition.SeverityWarning, rabbitmqv1.RabbitMQPolicyReadyErrorMessage, err.Error()))
 		return ctrl.Result{}, err
 	}
 
-	instance.Status.Conditions.MarkTrue(rabbitmqv1.PolicyReadyCondition, rabbitmqv1.PolicyReadyMessage)
+	instance.Status.Conditions.MarkTrue(rabbitmqv1.RabbitMQPolicyReadyCondition, rabbitmqv1.RabbitMQPolicyReadyMessage)
 	instance.Status.Conditions.MarkTrue(condition.ReadyCondition, condition.ReadyMessage)
 
 	return ctrl.Result{}, nil
@@ -182,7 +187,10 @@ func (r *RabbitMQPolicyReconciler) reconcileDelete(ctx context.Context, instance
 	if instance.Spec.VhostRef != "" {
 		vhost := &rabbitmqv1.RabbitMQVhost{}
 		err := r.Get(ctx, types.NamespacedName{Name: instance.Spec.VhostRef, Namespace: instance.Namespace}, vhost)
-		if err == nil {
+		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)
+		} else if err == nil {
 			vhostName = vhost.Spec.Name
 			if vhostName == "" {
 				vhostName = "/"
@@ -193,10 +201,16 @@ func (r *RabbitMQPolicyReconciler) reconcileDelete(ctx context.Context, instance
 	// Get RabbitMQ cluster
 	rabbit := &rabbitmqclusterv2.RabbitmqCluster{}
 	err := r.Get(ctx, types.NamespacedName{Name: instance.Spec.RabbitmqClusterName, Namespace: instance.Namespace}, rabbit)
-	if err == nil {
+	if err != nil && !k8s_errors.IsNotFound(err) {
+		// Log non-NotFound errors but continue with deletion
+		log.FromContext(ctx).Error(err, "Failed to get RabbitMQ cluster", "cluster", instance.Spec.RabbitmqClusterName)
+	} else if err == nil {
 		// Get admin credentials
 		rabbitSecret, _, err := oko_secret.GetSecret(ctx, h, rabbit.Status.DefaultUser.SecretReference.Name, instance.Namespace)
-		if err == nil {
+		if err != nil && !k8s_errors.IsNotFound(err) {
+			// Log non-NotFound errors but continue with deletion
+			log.FromContext(ctx).Error(err, "Failed to get admin secret")
+		} else if err == nil {
 			// Create API client
 			tlsEnabled := rabbit.Spec.TLS.SecretName != ""
 			protocol := "http"
@@ -208,7 +222,7 @@ func (r *RabbitMQPolicyReconciler) reconcileDelete(ctx context.Context, instance
 			baseURL := fmt.Sprintf("%s://%s:%s", protocol, string(rabbitSecret.Data["host"]), managementPort)
 			apiClient := rabbitmqapi.NewClient(baseURL, string(rabbitSecret.Data["username"]), string(rabbitSecret.Data["password"]), tlsEnabled)
 
-			// Delete policy
+			// Delete policy - ignore NotFound errors
 			if err := apiClient.DeletePolicy(vhostName, policyName); err != nil {
 				// Log error but don't fail deletion - the policy may already be gone
 				log.FromContext(ctx).Error(err, "Failed to delete policy from RabbitMQ", "policy", policyName, "vhost", vhostName)