move notificationsbusinstance migration to controller

Author: lmiccini

api/core/v1beta1/openstackcontrolplane_webhook.go

@@ -872,13 +872,9 @@ func setOverrideSpec(override **route.OverrideSpec, anno map[string]string) {
 
 // DefaultServices - common function for calling individual services' defaulting functions
 func (r *OpenStackControlPlane) DefaultServices() {
-	// Default NotificationsBus if NotificationsBusInstance is specified
-	if r.Spec.NotificationsBusInstance != nil && *r.Spec.NotificationsBusInstance != "" {
-		if r.Spec.NotificationsBus == nil {
-			r.Spec.NotificationsBus = &rabbitmqv1.RabbitMqConfig{}
-		}
-		rabbitmqv1.DefaultRabbitMqConfig(r.Spec.NotificationsBus, *r.Spec.NotificationsBusInstance)
-	}
+	// Top-level NotificationsBusInstance migration is handled in the reconcile loop
+	// to properly support migration from the deprecated field. The webhook doesn't
+	// have access to the old object to distinguish between user overrides and defaults.
 
 	// Cinder
 	if r.Spec.Cinder.Enabled || r.Spec.Cinder.Template != nil {

internal/controller/core/openstackcontrolplane_controller.go

@@ -402,6 +402,19 @@ func (r *OpenStackControlPlaneReconciler) reconcileNormal(ctx context.Context, i
 		instance.Status.Conditions.MarkTrue(condition.TopologyReadyCondition, condition.TopologyReadyMessage)
 	}
 
+	// Migration: Migrate top-level deprecated NotificationsBusInstance to NotificationsBus
+	// This must happen before service reconciliation so services can inherit the migrated value
+	if instance.Spec.NotificationsBusInstance != nil && *instance.Spec.NotificationsBusInstance != "" {
+		if instance.Spec.NotificationsBus == nil {
+			instance.Spec.NotificationsBus = &rabbitmqv1.RabbitMqConfig{}
+		}
+		if instance.Spec.NotificationsBus.Cluster == "" {
+			instance.Spec.NotificationsBus.Cluster = *instance.Spec.NotificationsBusInstance
+		}
+		// Clear deprecated field once migrated
+		instance.Spec.NotificationsBusInstance = nil
+	}
+
 	ctrlResult, err := openstack.ReconcileCAs(ctx, instance, helper)
 	if err != nil {
 		return ctrl.Result{}, err

internal/openstack/migration_logic_test.go

@@ -370,6 +370,96 @@ func TestEdgeCases(t *testing.T) {
 	})
 }
 
+// TestTopLevelNotificationsBusInstanceMigration tests the top-level NotificationsBusInstance
+// migration that happens in the controller's reconcileNormal function
+func TestTopLevelNotificationsBusInstanceMigration(t *testing.T) {
+	g := NewWithT(t)
+
+	testCases := []struct {
+		name                     string
+		notificationsBusInstance *string
+		notificationsBus         *rabbitmqv1.RabbitMqConfig
+		expectedCluster          string
+		expectedInstanceNil      bool
+		description              string
+	}{
+		{
+			name:                     "Migrate from NotificationsBusInstance",
+			notificationsBusInstance: stringPtr("rabbitmq-notifications"),
+			notificationsBus:         nil,
+			expectedCluster:          "rabbitmq-notifications",
+			expectedInstanceNil:      true,
+			description:              "Should create NotificationsBus and migrate value",
+		},
+		{
+			name:                     "Migrate to existing empty NotificationsBus",
+			notificationsBusInstance: stringPtr("rabbitmq-notifications"),
+			notificationsBus:         &rabbitmqv1.RabbitMqConfig{Cluster: ""},
+			expectedCluster:          "rabbitmq-notifications",
+			expectedInstanceNil:      true,
+			description:              "Should populate existing NotificationsBus",
+		},
+		{
+			name:                     "New field takes precedence",
+			notificationsBusInstance: stringPtr("old-value"),
+			notificationsBus:         &rabbitmqv1.RabbitMqConfig{Cluster: "new-value"},
+			expectedCluster:          "new-value",
+			expectedInstanceNil:      true,
+			description:              "Should keep new field value and clear deprecated",
+		},
+		{
+			name:                     "Already migrated",
+			notificationsBusInstance: nil,
+			notificationsBus:         &rabbitmqv1.RabbitMqConfig{Cluster: "rabbitmq-notifications"},
+			expectedCluster:          "rabbitmq-notifications",
+			expectedInstanceNil:      true,
+			description:              "Should leave already-migrated config unchanged",
+		},
+		{
+			name:                     "Empty deprecated field",
+			notificationsBusInstance: stringPtr(""),
+			notificationsBus:         nil,
+			expectedCluster:          "",
+			expectedInstanceNil:      false,
+			description:              "Empty string in deprecated field should not trigger migration",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			notificationsBusInstance := tc.notificationsBusInstance
+			notificationsBus := tc.notificationsBus
+
+			// Simulate the controller migration logic
+			if notificationsBusInstance != nil && *notificationsBusInstance != "" {
+				if notificationsBus == nil {
+					notificationsBus = &rabbitmqv1.RabbitMqConfig{}
+				}
+				if notificationsBus.Cluster == "" {
+					notificationsBus.Cluster = *notificationsBusInstance
+				}
+				// Clear deprecated field once migrated
+				notificationsBusInstance = nil
+			}
+
+			// Verify results
+			if tc.expectedCluster != "" {
+				g.Expect(notificationsBus).ToNot(BeNil(), tc.description+" - NotificationsBus should not be nil")
+				g.Expect(notificationsBus.Cluster).To(Equal(tc.expectedCluster),
+					tc.description+" - Cluster value mismatch")
+			} else if tc.notificationsBus == nil && tc.expectedCluster == "" {
+				// For the empty deprecated field case, NotificationsBus should remain nil
+				g.Expect(notificationsBus).To(BeNil(), tc.description+" - NotificationsBus should be nil")
+			}
+
+			if tc.expectedInstanceNil {
+				g.Expect(notificationsBusInstance).To(BeNil(),
+					tc.description+" - Deprecated field should be nil after migration")
+			}
+		})
+	}
+}
+
 // Helper function to create string pointers
 func stringPtr(s string) *string {
 	return &s