Skip to content

Commit 32c6499

Browse files
lmiccinilmiccini
committed
Fix nil pointer dereferences in NotificationsBus handling
Add nil checks before accessing optional NotificationsBus fields to prevent panics when the field is not configured. Changes: - autoscaling_webhook.go: Check NotificationsBus != nil before accessing Cluster field in getDeprecatedFields() - ceilometer_webhook.go: Same nil check for Ceilometer webhook - autoscaling_controller.go: Return error when rabbitmqConfig is nil in transportURLCreateOrUpdate() - Add tests for nil NotificationsBus in webhook validation and controller reconciliation NotificationsBus is optional (*RabbitMqConfig with omitempty), so users can legitimately omit it. The code now handles this gracefully instead of panicking.
1 parent fe8dc27 commit 32c6499

6 files changed

Lines changed: 150 additions & 2 deletions

File tree

‎api/v1beta1/autoscaling_webhook.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,12 +90,17 @@ func (spec *AodhCore) Default() {
9090

9191
// getDeprecatedFields returns the centralized list of deprecated fields for AodhCore
9292
func (spec *AodhCore) getDeprecatedFields(old *AodhCore) []common_webhook.DeprecatedFieldUpdate {
93+
var newValue *string
94+
if spec.NotificationsBus != nil {
95+
newValue = &spec.NotificationsBus.Cluster
96+
}
97+
9398
deprecatedFields := []common_webhook.DeprecatedFieldUpdate{
9499
{
95100
DeprecatedFieldName: "rabbitMqClusterName",
96101
NewFieldPath: []string{"notificationsBus", "cluster"},
97102
NewDeprecatedValue: &spec.RabbitMqClusterName,
98-
NewValue: &spec.NotificationsBus.Cluster,
103+
NewValue: newValue,
99104
},
100105
}
101106

‎api/v1beta1/ceilometer_webhook.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,12 +98,17 @@ func (spec *CeilometerSpecCore) Default() {
9898

9999
// getDeprecatedFields returns the centralized list of deprecated fields for CeilometerSpecCore
100100
func (spec *CeilometerSpecCore) getDeprecatedFields(old *CeilometerSpecCore) []common_webhook.DeprecatedFieldUpdate {
101+
var newValue *string
102+
if spec.NotificationsBus != nil {
103+
newValue = &spec.NotificationsBus.Cluster
104+
}
105+
101106
deprecatedFields := []common_webhook.DeprecatedFieldUpdate{
102107
{
103108
DeprecatedFieldName: "rabbitMqClusterName",
104109
NewFieldPath: []string{"notificationsBus", "cluster"},
105110
NewDeprecatedValue: &spec.RabbitMqClusterName,
106-
NewValue: &spec.NotificationsBus.Cluster,
111+
NewValue: newValue,
107112
},
108113
}
109114

‎api/v1beta1/telemetry_webhook_test.go‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,3 +93,65 @@ func TestCloudKittySpecBaseDefault(t *testing.T) {
9393
g.Expect(spec.MessagingBus.Cluster).To(Equal("rabbitmq"),
9494
"CloudKitty messagingBus.cluster should be defaulted to rabbitmq")
9595
}
96+
97+
// TestAodhCoreDefaultWithNilNotificationsBus tests that Default() doesn't panic with nil NotificationsBus
98+
func TestAodhCoreDefaultWithNilNotificationsBus(t *testing.T) {
99+
g := NewWithT(t)
100+
101+
spec := &AodhCore{}
102+
// NotificationsBus is nil (not set)
103+
104+
// Call Default() - should not panic
105+
spec.Default()
106+
107+
// NotificationsBus should remain nil
108+
g.Expect(spec.NotificationsBus).To(BeNil(),
109+
"Aodh notificationsBus should remain nil when not configured")
110+
}
111+
112+
// TestCeilometerSpecCoreDefaultWithNilNotificationsBus tests that Default() doesn't panic with nil NotificationsBus
113+
func TestCeilometerSpecCoreDefaultWithNilNotificationsBus(t *testing.T) {
114+
g := NewWithT(t)
115+
116+
spec := &CeilometerSpecCore{}
117+
// NotificationsBus is nil (not set)
118+
119+
// Call Default() - should not panic
120+
spec.Default()
121+
122+
// NotificationsBus should remain nil
123+
g.Expect(spec.NotificationsBus).To(BeNil(),
124+
"Ceilometer notificationsBus should remain nil when not configured")
125+
}
126+
127+
// TestAodhCoreValidateCreateWithNilNotificationsBus tests validation with nil NotificationsBus
128+
func TestAodhCoreValidateCreateWithNilNotificationsBus(t *testing.T) {
129+
g := NewWithT(t)
130+
131+
spec := &AodhCore{}
132+
// NotificationsBus is nil, but we have the deprecated field set
133+
spec.RabbitMqClusterName = "old-rabbitmq"
134+
135+
// Call ValidateCreate - should not panic even with nil NotificationsBus
136+
warns, errs := spec.ValidateCreate(nil, "default")
137+
138+
// Should have a warning about deprecated field
139+
g.Expect(warns).ToNot(BeEmpty(), "Should have warning about deprecated rabbitMqClusterName field")
140+
g.Expect(errs).To(BeEmpty(), "Should not have errors")
141+
}
142+
143+
// TestCeilometerSpecCoreValidateCreateWithNilNotificationsBus tests validation with nil NotificationsBus
144+
func TestCeilometerSpecCoreValidateCreateWithNilNotificationsBus(t *testing.T) {
145+
g := NewWithT(t)
146+
147+
spec := &CeilometerSpecCore{}
148+
// NotificationsBus is nil, but we have the deprecated field set
149+
spec.RabbitMqClusterName = "old-rabbitmq"
150+
151+
// Call ValidateCreate - should not panic even with nil NotificationsBus
152+
warns, errs := spec.ValidateCreate(nil, "default")
153+
154+
// Should have a warning about deprecated field
155+
g.Expect(warns).ToNot(BeEmpty(), "Should have warning about deprecated rabbitMqClusterName field")
156+
g.Expect(errs).To(BeEmpty(), "Should not have errors")
157+
}

‎go.mod‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ require (
99
github.com/go-logr/logr v1.4.3
1010
github.com/grafana/loki/operator/api/loki v0.0.0-20250910094332-a082b8a061ba
1111
github.com/k8snetworkplumbingwg/network-attachment-definition-client v1.7.7
12+
github.com/onsi/gomega v1.39.0
1213
github.com/openstack-k8s-operators/heat-operator/api v0.6.1-0.20260126095359-39e97ca5471e
1314
github.com/openstack-k8s-operators/infra-operator/apis v0.6.1-0.20260126091827-7758173fbb09
1415
github.com/openstack-k8s-operators/keystone-operator/api v0.6.1-0.20260126094240-81b572e8f653
@@ -103,6 +104,7 @@ require (
103104
google.golang.org/genproto/googleapis/rpc v0.0.0-20250115164207-1a7da9e5054f // indirect
104105
google.golang.org/grpc v1.71.1 // indirect
105106
google.golang.org/protobuf v1.36.7 // indirect
107+
gopkg.in/evanphx/json-patch.v4 v4.12.0 // indirect
106108
gopkg.in/inf.v0 v0.9.1 // indirect
107109
gopkg.in/yaml.v2 v2.4.0 // indirect
108110
gopkg.in/yaml.v3 v3.0.1 // indirect

‎internal/controller/autoscaling_controller.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,9 @@ import (
7171
// ErrNotificationsURLSecretNotSet is returned when NotificationsURLSecret is not set in the instance status
7272
var ErrNotificationsURLSecretNotSet = errors.New("NotificationsURLSecret is not set")
7373

74+
// ErrRabbitMQConfigNil is returned when rabbitmqConfig parameter is nil
75+
var ErrRabbitMQConfigNil = errors.New("rabbitmqConfig is nil - NotificationsBus must be configured")
76+
7477
// AutoscalingReconciler reconciles a Autoscaling object
7578
type AutoscalingReconciler struct {
7679
client.Client
@@ -903,6 +906,11 @@ func (r *AutoscalingReconciler) transportURLCreateOrUpdate(
903906
) (*rabbitmqv1.TransportURL, controllerutil.OperationResult, error) {
904907
// Aodh only uses NotificationsBus TransportURL
905908
rmqName := fmt.Sprintf("%s-notifications-transport", autoscaling.ServiceName)
909+
910+
if rabbitmqConfig == nil {
911+
return nil, controllerutil.OperationResultNone, ErrRabbitMQConfigNil
912+
}
913+
906914
config := rabbitmqConfig
907915

908916
// Prepare the spec values before CreateOrUpdate so webhooks see them during CREATE

‎internal/controller/autoscaling_controller_test.go‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
package controller
2+
3+
import (
4+
"context"
5+
"testing"
6+
7+
"github.com/onsi/gomega"
8+
telemetryv1 "github.com/openstack-k8s-operators/telemetry-operator/api/v1beta1"
9+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
10+
"k8s.io/apimachinery/pkg/runtime"
11+
"sigs.k8s.io/controller-runtime/pkg/client/fake"
12+
)
13+
14+
// TestTransportURLCreateOrUpdateWithNilConfig tests that transportURLCreateOrUpdate
15+
// handles nil rabbitmqConfig gracefully without panicking
16+
func TestTransportURLCreateOrUpdateWithNilConfig(t *testing.T) {
17+
g := gomega.NewWithT(t)
18+
19+
// Create a fake client
20+
scheme := runtime.NewScheme()
21+
_ = telemetryv1.AddToScheme(scheme)
22+
23+
client := fake.NewClientBuilder().WithScheme(scheme).Build()
24+
25+
reconciler := &AutoscalingReconciler{
26+
Client: client,
27+
Scheme: scheme,
28+
}
29+
30+
instance := &telemetryv1.Autoscaling{
31+
ObjectMeta: metav1.ObjectMeta{
32+
Name: "test-autoscaling",
33+
Namespace: "test-namespace",
34+
},
35+
Spec: telemetryv1.AutoscalingSpec{
36+
Aodh: telemetryv1.Aodh{
37+
AodhCore: telemetryv1.AodhCore{
38+
// NotificationsBus is nil
39+
NotificationsBus: nil,
40+
},
41+
},
42+
},
43+
}
44+
45+
serviceLabels := map[string]string{
46+
"app": "test",
47+
}
48+
49+
// Call transportURLCreateOrUpdate with nil rabbitmqConfig
50+
// This should return an error, not panic
51+
transportURL, op, err := reconciler.transportURLCreateOrUpdate(
52+
context.Background(),
53+
instance,
54+
serviceLabels,
55+
nil, // nil rabbitmqConfig
56+
)
57+
58+
// Should return an error
59+
g.Expect(err).To(gomega.HaveOccurred(), "Should return error when rabbitmqConfig is nil")
60+
g.Expect(err.Error()).To(gomega.ContainSubstring("rabbitmqConfig is nil"),
61+
"Error message should indicate rabbitmqConfig is nil")
62+
63+
// Should return nil transportURL and OperationResultNone
64+
g.Expect(transportURL).To(gomega.BeNil(), "TransportURL should be nil when config is nil")
65+
g.Expect(string(op)).To(gomega.Equal("unchanged"), "Operation should be OperationResultNone when config is nil")
66+
}

0 commit comments

Comments
 (0)