Merge pull request #843 from lmiccini/rabbitmq_nil_check

Fix nil pointer dereferences in NotificationsBus handling

Author: openshift-merge-bot[bot]

api/v1beta1/autoscaling_webhook.go

@@ -90,12 +90,17 @@ func (spec *AodhCore) Default() {
 
 // getDeprecatedFields returns the centralized list of deprecated fields for AodhCore
 func (spec *AodhCore) getDeprecatedFields(old *AodhCore) []common_webhook.DeprecatedFieldUpdate {
+	var newValue *string
+	if spec.NotificationsBus != nil {
+		newValue = &spec.NotificationsBus.Cluster
+	}
+
 	deprecatedFields := []common_webhook.DeprecatedFieldUpdate{
 		{
 			DeprecatedFieldName: "rabbitMqClusterName",
 			NewFieldPath:        []string{"notificationsBus", "cluster"},
 			NewDeprecatedValue:  &spec.RabbitMqClusterName,
-			NewValue:            &spec.NotificationsBus.Cluster,
+			NewValue:            newValue,
 		},
 	}
 

api/v1beta1/ceilometer_webhook.go

@@ -98,12 +98,17 @@ func (spec *CeilometerSpecCore) Default() {
 
 // getDeprecatedFields returns the centralized list of deprecated fields for CeilometerSpecCore
 func (spec *CeilometerSpecCore) getDeprecatedFields(old *CeilometerSpecCore) []common_webhook.DeprecatedFieldUpdate {
+	var newValue *string
+	if spec.NotificationsBus != nil {
+		newValue = &spec.NotificationsBus.Cluster
+	}
+
 	deprecatedFields := []common_webhook.DeprecatedFieldUpdate{
 		{
 			DeprecatedFieldName: "rabbitMqClusterName",
 			NewFieldPath:        []string{"notificationsBus", "cluster"},
 			NewDeprecatedValue:  &spec.RabbitMqClusterName,
-			NewValue:            &spec.NotificationsBus.Cluster,
+			NewValue:            newValue,
 		},
 	}
 

api/v1beta1/telemetry_webhook_test.go

@@ -93,3 +93,65 @@ func TestCloudKittySpecBaseDefault(t *testing.T) {
 	g.Expect(spec.MessagingBus.Cluster).To(Equal("rabbitmq"),
 		"CloudKitty messagingBus.cluster should be defaulted to rabbitmq")
 }
+
+// TestAodhCoreDefaultWithNilNotificationsBus tests that Default() doesn't panic with nil NotificationsBus
+func TestAodhCoreDefaultWithNilNotificationsBus(t *testing.T) {
+	g := NewWithT(t)
+
+	spec := &AodhCore{}
+	// NotificationsBus is nil (not set)
+
+	// Call Default() - should not panic
+	spec.Default()
+
+	// NotificationsBus should remain nil
+	g.Expect(spec.NotificationsBus).To(BeNil(),
+		"Aodh notificationsBus should remain nil when not configured")
+}
+
+// TestCeilometerSpecCoreDefaultWithNilNotificationsBus tests that Default() doesn't panic with nil NotificationsBus
+func TestCeilometerSpecCoreDefaultWithNilNotificationsBus(t *testing.T) {
+	g := NewWithT(t)
+
+	spec := &CeilometerSpecCore{}
+	// NotificationsBus is nil (not set)
+
+	// Call Default() - should not panic
+	spec.Default()
+
+	// NotificationsBus should remain nil
+	g.Expect(spec.NotificationsBus).To(BeNil(),
+		"Ceilometer notificationsBus should remain nil when not configured")
+}
+
+// TestAodhCoreValidateCreateWithNilNotificationsBus tests validation with nil NotificationsBus
+func TestAodhCoreValidateCreateWithNilNotificationsBus(t *testing.T) {
+	g := NewWithT(t)
+
+	spec := &AodhCore{}
+	// NotificationsBus is nil, but we have the deprecated field set
+	spec.RabbitMqClusterName = "old-rabbitmq"
+
+	// Call ValidateCreate - should not panic even with nil NotificationsBus
+	warns, errs := spec.ValidateCreate(nil, "default")
+
+	// Should have a warning about deprecated field
+	g.Expect(warns).ToNot(BeEmpty(), "Should have warning about deprecated rabbitMqClusterName field")
+	g.Expect(errs).To(BeEmpty(), "Should not have errors")
+}
+
+// TestCeilometerSpecCoreValidateCreateWithNilNotificationsBus tests validation with nil NotificationsBus
+func TestCeilometerSpecCoreValidateCreateWithNilNotificationsBus(t *testing.T) {
+	g := NewWithT(t)
+
+	spec := &CeilometerSpecCore{}
+	// NotificationsBus is nil, but we have the deprecated field set
+	spec.RabbitMqClusterName = "old-rabbitmq"
+
+	// Call ValidateCreate - should not panic even with nil NotificationsBus
+	warns, errs := spec.ValidateCreate(nil, "default")
+
+	// Should have a warning about deprecated field
+	g.Expect(warns).ToNot(BeEmpty(), "Should have warning about deprecated rabbitMqClusterName field")
+	g.Expect(errs).To(BeEmpty(), "Should not have errors")
+}

go.mod

@@ -9,6 +9,7 @@ require (
 	github.com/go-logr/logr v1.4.3
 	github.com/grafana/loki/operator/api/loki v0.0.0-20250910094332-a082b8a061ba
 	github.com/k8snetworkplumbingwg/network-attachment-definition-client v1.7.7
+	github.com/onsi/gomega v1.39.0
 	github.com/openstack-k8s-operators/heat-operator/api v0.6.1-0.20260126095359-39e97ca5471e
 	github.com/openstack-k8s-operators/infra-operator/apis v0.6.1-0.20260126091827-7758173fbb09
 	github.com/openstack-k8s-operators/keystone-operator/api v0.6.1-0.20260126094240-81b572e8f653
@@ -103,6 +104,7 @@ require (
 	google.golang.org/genproto/googleapis/rpc v0.0.0-20250115164207-1a7da9e5054f // indirect
 	google.golang.org/grpc v1.71.1 // indirect
 	google.golang.org/protobuf v1.36.7 // indirect
+	gopkg.in/evanphx/json-patch.v4 v4.12.0 // indirect
 	gopkg.in/inf.v0 v0.9.1 // indirect
 	gopkg.in/yaml.v2 v2.4.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect

internal/controller/autoscaling_controller.go

@@ -71,6 +71,9 @@ import (
 // ErrNotificationsURLSecretNotSet is returned when NotificationsURLSecret is not set in the instance status
 var ErrNotificationsURLSecretNotSet = errors.New("NotificationsURLSecret is not set")
 
+// ErrRabbitMQConfigNil is returned when rabbitmqConfig parameter is nil
+var ErrRabbitMQConfigNil = errors.New("rabbitmqConfig is nil - NotificationsBus must be configured")
+
 // AutoscalingReconciler reconciles a Autoscaling object
 type AutoscalingReconciler struct {
 	client.Client
@@ -903,6 +906,11 @@ func (r *AutoscalingReconciler) transportURLCreateOrUpdate(
 ) (*rabbitmqv1.TransportURL, controllerutil.OperationResult, error) {
 	// Aodh only uses NotificationsBus TransportURL
 	rmqName := fmt.Sprintf("%s-notifications-transport", autoscaling.ServiceName)
+
+	if rabbitmqConfig == nil {
+		return nil, controllerutil.OperationResultNone, ErrRabbitMQConfigNil
+	}
+
 	config := rabbitmqConfig
 
 	// Prepare the spec values before CreateOrUpdate so webhooks see them during CREATE

internal/controller/autoscaling_controller_test.go

@@ -0,0 +1,66 @@
+package controller
+
+import (
+	"context"
+	"testing"
+
+	"github.com/onsi/gomega"
+	telemetryv1 "github.com/openstack-k8s-operators/telemetry-operator/api/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+// TestTransportURLCreateOrUpdateWithNilConfig tests that transportURLCreateOrUpdate
+// handles nil rabbitmqConfig gracefully without panicking
+func TestTransportURLCreateOrUpdateWithNilConfig(t *testing.T) {
+	g := gomega.NewWithT(t)
+
+	// Create a fake client
+	scheme := runtime.NewScheme()
+	_ = telemetryv1.AddToScheme(scheme)
+
+	client := fake.NewClientBuilder().WithScheme(scheme).Build()
+
+	reconciler := &AutoscalingReconciler{
+		Client: client,
+		Scheme: scheme,
+	}
+
+	instance := &telemetryv1.Autoscaling{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-autoscaling",
+			Namespace: "test-namespace",
+		},
+		Spec: telemetryv1.AutoscalingSpec{
+			Aodh: telemetryv1.Aodh{
+				AodhCore: telemetryv1.AodhCore{
+					// NotificationsBus is nil
+					NotificationsBus: nil,
+				},
+			},
+		},
+	}
+
+	serviceLabels := map[string]string{
+		"app": "test",
+	}
+
+	// Call transportURLCreateOrUpdate with nil rabbitmqConfig
+	// This should return an error, not panic
+	transportURL, op, err := reconciler.transportURLCreateOrUpdate(
+		context.Background(),
+		instance,
+		serviceLabels,
+		nil, // nil rabbitmqConfig
+	)
+
+	// Should return an error
+	g.Expect(err).To(gomega.HaveOccurred(), "Should return error when rabbitmqConfig is nil")
+	g.Expect(err.Error()).To(gomega.ContainSubstring("rabbitmqConfig is nil"),
+		"Error message should indicate rabbitmqConfig is nil")
+
+	// Should return nil transportURL and OperationResultNone
+	g.Expect(transportURL).To(gomega.BeNil(), "TransportURL should be nil when config is nil")
+	g.Expect(string(op)).To(gomega.Equal("unchanged"), "Operation should be OperationResultNone when config is nil")
+}