Skip to content

Commit 45e1f93

Browse files
IshwarKanseIshwarKanse
committed
fix: handle nil pointer dereference when ObservabilityInstaller has empty spec
Assisted by Claude Code
1 parent eb33497 commit 45e1f93

3 files changed

Lines changed: 348 additions & 1 deletion

File tree

‎pkg/controllers/observability/observability_controller.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ func (o observabilityInstallerController) getInstance(ctx context.Context, req c
206206
func (o observabilityInstallerController) updateStatus(ctx context.Context, instance *obsv1alpha1.ObservabilityInstaller, reconcileErr error) reconcile.Result {
207207
if instance.Spec.Capabilities != nil {
208208
capabilities := instance.Spec.Capabilities
209-
if capabilities.Tracing.Enabled {
209+
if capabilities.Tracing != nil && capabilities.Tracing.Enabled {
210210
otelcol := &otelv1beta1.OpenTelemetryCollector{}
211211
err := o.client.Get(ctx, types.NamespacedName{
212212
Namespace: instance.Namespace,

‎pkg/controllers/observability/tempo_components_test.go‎

Lines changed: 264 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,264 @@
1+
package observability
2+
3+
import (
4+
"testing"
5+
6+
tempov1alpha1 "github.com/grafana/tempo-operator/api/tempo/v1alpha1"
7+
"github.com/stretchr/testify/assert"
8+
"github.com/stretchr/testify/require"
9+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
10+
11+
obsv1alpha1 "github.com/rhobs/observability-operator/pkg/apis/observability/v1alpha1"
12+
)
13+
14+
func TestTempoStack(t *testing.T) {
15+
tests := []struct {
16+
name string
17+
instance *obsv1alpha1.ObservabilityInstaller
18+
wantStorageType tempov1alpha1.ObjectStorageSecretType
19+
wantCredentialMode tempov1alpha1.CredentialMode
20+
wantTLSEnabled bool
21+
wantTLSCASet bool
22+
wantTLSCertSet bool
23+
}{
24+
{
25+
name: "nil capabilities - does not panic",
26+
instance: &obsv1alpha1.ObservabilityInstaller{
27+
ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "test-ns"},
28+
Spec: obsv1alpha1.ObservabilityInstallerSpec{},
29+
},
30+
},
31+
{
32+
name: "nil tracing - does not panic",
33+
instance: &obsv1alpha1.ObservabilityInstaller{
34+
ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "test-ns"},
35+
Spec: obsv1alpha1.ObservabilityInstallerSpec{
36+
Capabilities: &obsv1alpha1.CapabilitiesSpec{},
37+
},
38+
},
39+
},
40+
{
41+
name: "nil storage - does not panic",
42+
instance: &obsv1alpha1.ObservabilityInstaller{
43+
ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "test-ns"},
44+
Spec: obsv1alpha1.ObservabilityInstallerSpec{
45+
Capabilities: &obsv1alpha1.CapabilitiesSpec{
46+
Tracing: &obsv1alpha1.TracingSpec{},
47+
},
48+
},
49+
},
50+
},
51+
{
52+
name: "nil object storage spec - does not panic",
53+
instance: &obsv1alpha1.ObservabilityInstaller{
54+
ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "test-ns"},
55+
Spec: obsv1alpha1.ObservabilityInstallerSpec{
56+
Capabilities: &obsv1alpha1.CapabilitiesSpec{
57+
Tracing: &obsv1alpha1.TracingSpec{
58+
Storage: &obsv1alpha1.TracingStorageSpec{},
59+
},
60+
},
61+
},
62+
},
63+
},
64+
{
65+
name: "S3 storage - sets S3 type and static credential mode",
66+
instance: &obsv1alpha1.ObservabilityInstaller{
67+
ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "test-ns"},
68+
Spec: obsv1alpha1.ObservabilityInstallerSpec{
69+
Capabilities: &obsv1alpha1.CapabilitiesSpec{
70+
Tracing: &obsv1alpha1.TracingSpec{
71+
Storage: &obsv1alpha1.TracingStorageSpec{
72+
ObjectStorageSpec: &obsv1alpha1.TracingObjectStorageSpec{
73+
S3: &obsv1alpha1.S3Spec{
74+
Bucket: "test-bucket",
75+
Endpoint: "http://minio:9000",
76+
},
77+
},
78+
},
79+
},
80+
},
81+
},
82+
},
83+
wantStorageType: tempov1alpha1.ObjectStorageSecretS3,
84+
wantCredentialMode: tempov1alpha1.CredentialModeStatic,
85+
},
86+
{
87+
name: "S3STS storage - sets S3 type and token credential mode",
88+
instance: &obsv1alpha1.ObservabilityInstaller{
89+
ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "test-ns"},
90+
Spec: obsv1alpha1.ObservabilityInstallerSpec{
91+
Capabilities: &obsv1alpha1.CapabilitiesSpec{
92+
Tracing: &obsv1alpha1.TracingSpec{
93+
Storage: &obsv1alpha1.TracingStorageSpec{
94+
ObjectStorageSpec: &obsv1alpha1.TracingObjectStorageSpec{
95+
S3STS: &obsv1alpha1.S3STSpec{
96+
Bucket: "test-bucket",
97+
RoleARN: "arn:aws:iam::123:role/test",
98+
Region: "us-east-1",
99+
},
100+
},
101+
},
102+
},
103+
},
104+
},
105+
},
106+
wantStorageType: tempov1alpha1.ObjectStorageSecretS3,
107+
wantCredentialMode: tempov1alpha1.CredentialModeToken,
108+
},
109+
{
110+
name: "Azure storage - sets Azure type and static credential mode",
111+
instance: &obsv1alpha1.ObservabilityInstaller{
112+
ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "test-ns"},
113+
Spec: obsv1alpha1.ObservabilityInstallerSpec{
114+
Capabilities: &obsv1alpha1.CapabilitiesSpec{
115+
Tracing: &obsv1alpha1.TracingSpec{
116+
Storage: &obsv1alpha1.TracingStorageSpec{
117+
ObjectStorageSpec: &obsv1alpha1.TracingObjectStorageSpec{
118+
Azure: &obsv1alpha1.AzureSpec{
119+
Container: "test-container",
120+
AccountName: "test-account",
121+
AccountKeySecret: obsv1alpha1.SecretKeySelector{
122+
Name: "secret",
123+
Key: "key",
124+
},
125+
},
126+
},
127+
},
128+
},
129+
},
130+
},
131+
},
132+
wantStorageType: tempov1alpha1.ObjectStorageSecretAzure,
133+
wantCredentialMode: tempov1alpha1.CredentialModeStatic,
134+
},
135+
{
136+
name: "GCS storage - sets GCS type and static credential mode",
137+
instance: &obsv1alpha1.ObservabilityInstaller{
138+
ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "test-ns"},
139+
Spec: obsv1alpha1.ObservabilityInstallerSpec{
140+
Capabilities: &obsv1alpha1.CapabilitiesSpec{
141+
Tracing: &obsv1alpha1.TracingSpec{
142+
Storage: &obsv1alpha1.TracingStorageSpec{
143+
ObjectStorageSpec: &obsv1alpha1.TracingObjectStorageSpec{
144+
GCS: &obsv1alpha1.GCSSpec{
145+
Bucket: "test-bucket",
146+
KeyJSONSecret: obsv1alpha1.SecretKeySelector{
147+
Name: "secret",
148+
Key: "key.json",
149+
},
150+
},
151+
},
152+
},
153+
},
154+
},
155+
},
156+
},
157+
wantStorageType: tempov1alpha1.ObjectStorageSecretGCS,
158+
wantCredentialMode: tempov1alpha1.CredentialModeStatic,
159+
},
160+
{
161+
name: "S3 with HTTPS endpoint - enables TLS automatically",
162+
instance: &obsv1alpha1.ObservabilityInstaller{
163+
ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "test-ns"},
164+
Spec: obsv1alpha1.ObservabilityInstallerSpec{
165+
Capabilities: &obsv1alpha1.CapabilitiesSpec{
166+
Tracing: &obsv1alpha1.TracingSpec{
167+
Storage: &obsv1alpha1.TracingStorageSpec{
168+
ObjectStorageSpec: &obsv1alpha1.TracingObjectStorageSpec{
169+
S3: &obsv1alpha1.S3Spec{
170+
Bucket: "test-bucket",
171+
Endpoint: "https://s3.amazonaws.com",
172+
},
173+
},
174+
},
175+
},
176+
},
177+
},
178+
},
179+
wantStorageType: tempov1alpha1.ObjectStorageSecretS3,
180+
wantCredentialMode: tempov1alpha1.CredentialModeStatic,
181+
wantTLSEnabled: true,
182+
},
183+
{
184+
name: "S3 with explicit TLS CA configmap - sets TLS CA reference",
185+
instance: &obsv1alpha1.ObservabilityInstaller{
186+
ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "test-ns"},
187+
Spec: obsv1alpha1.ObservabilityInstallerSpec{
188+
Capabilities: &obsv1alpha1.CapabilitiesSpec{
189+
Tracing: &obsv1alpha1.TracingSpec{
190+
Storage: &obsv1alpha1.TracingStorageSpec{
191+
ObjectStorageSpec: &obsv1alpha1.TracingObjectStorageSpec{
192+
S3: &obsv1alpha1.S3Spec{
193+
Bucket: "test-bucket",
194+
Endpoint: "http://minio:9000",
195+
},
196+
TLS: &obsv1alpha1.TLSSpec{
197+
CAConfigMap: &obsv1alpha1.ConfigMapKeySelector{
198+
Name: "ca-configmap",
199+
Key: "ca.crt",
200+
},
201+
},
202+
},
203+
},
204+
},
205+
},
206+
},
207+
},
208+
wantStorageType: tempov1alpha1.ObjectStorageSecretS3,
209+
wantCredentialMode: tempov1alpha1.CredentialModeStatic,
210+
wantTLSEnabled: true,
211+
wantTLSCASet: true,
212+
},
213+
{
214+
name: "S3 with explicit TLS cert secret - sets TLS cert reference",
215+
instance: &obsv1alpha1.ObservabilityInstaller{
216+
ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "test-ns"},
217+
Spec: obsv1alpha1.ObservabilityInstallerSpec{
218+
Capabilities: &obsv1alpha1.CapabilitiesSpec{
219+
Tracing: &obsv1alpha1.TracingSpec{
220+
Storage: &obsv1alpha1.TracingStorageSpec{
221+
ObjectStorageSpec: &obsv1alpha1.TracingObjectStorageSpec{
222+
S3: &obsv1alpha1.S3Spec{
223+
Bucket: "test-bucket",
224+
Endpoint: "http://minio:9000",
225+
},
226+
TLS: &obsv1alpha1.TLSSpec{
227+
CertSecret: &obsv1alpha1.SecretKeySelector{
228+
Name: "cert-secret",
229+
Key: "tls.crt",
230+
},
231+
},
232+
},
233+
},
234+
},
235+
},
236+
},
237+
},
238+
wantStorageType: tempov1alpha1.ObjectStorageSecretS3,
239+
wantCredentialMode: tempov1alpha1.CredentialModeStatic,
240+
wantTLSEnabled: true,
241+
wantTLSCertSet: true,
242+
},
243+
}
244+
245+
for _, tt := range tests {
246+
t.Run(tt.name, func(t *testing.T) {
247+
result := tempoStack(tt.instance)
248+
249+
require.NotNil(t, result)
250+
assert.Equal(t, tt.instance.Name, result.Name)
251+
assert.Equal(t, tt.instance.Namespace, result.Namespace)
252+
assert.Equal(t, tt.wantStorageType, result.Spec.Storage.Secret.Type)
253+
assert.Equal(t, tt.wantCredentialMode, result.Spec.Storage.Secret.CredentialMode)
254+
assert.Equal(t, tt.wantTLSEnabled, result.Spec.Storage.TLS.Enabled)
255+
256+
if tt.wantTLSCASet {
257+
assert.NotEmpty(t, result.Spec.Storage.TLS.CA)
258+
}
259+
if tt.wantTLSCertSet {
260+
assert.NotEmpty(t, result.Spec.Storage.TLS.Cert)
261+
}
262+
})
263+
}
264+
}

‎test/e2e/observability_installer_test.go‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ func TestObservabilityInstallerController(t *testing.T) {
4242
assertCRDExists(t, "observabilityinstallers.observability.openshift.io")
4343

4444
t.Run("ObservabilityInstallerTracing", testObservabilityInstallerTracing)
45+
t.Run("ObservabilityInstallerEmptySpec", testObservabilityInstallerEmptySpec)
4546
}
4647

4748
func testObservabilityInstallerTracing(t *testing.T) {
@@ -224,6 +225,88 @@ func testObservabilityInstallerTracing(t *testing.T) {
224225
require.NoError(t, err)
225226
}
226227

228+
// testObservabilityInstallerEmptySpec verifies that the controller handles an
229+
// ObservabilityInstaller with spec: {} gracefully — no panic, no operands deployed.
230+
//
231+
// Regression test for: nil pointer dereference when Capabilities is nil.
232+
func testObservabilityInstallerEmptySpec(t *testing.T) {
233+
ctx := context.Background()
234+
235+
ns := &corev1.Namespace{
236+
ObjectMeta: metav1.ObjectMeta{GenerateName: "obs-empty-spec-test-"},
237+
}
238+
err := f.K8sClient.Create(ctx, ns)
239+
require.NoError(t, err)
240+
f.CleanUp(t, func() { f.K8sClient.Delete(ctx, ns) })
241+
242+
obsInstaller := &obsv1alpha1.ObservabilityInstaller{
243+
ObjectMeta: metav1.ObjectMeta{
244+
Name: "empty-spec",
245+
Namespace: ns.Name,
246+
},
247+
Spec: obsv1alpha1.ObservabilityInstallerSpec{},
248+
}
249+
f.CleanUp(t, func() { f.K8sClient.Delete(ctx, obsInstaller) })
250+
251+
err = f.K8sClient.Create(ctx, obsInstaller)
252+
require.NoError(t, err, "creating ObservabilityInstaller with empty spec should succeed")
253+
254+
// Give the controller time to reconcile. A panic would restart the pod;
255+
// a nil-pointer dereference in updateStatus would show up as a crash loop.
256+
time.Sleep(30 * time.Second)
257+
258+
// Operator deployment must still be available — if the controller panicked
259+
// it would be crash-looping and AvailableReplicas would drop to zero.
260+
var operatorDeploy appsv1.Deployment
261+
err = f.K8sClient.Get(ctx, types.NamespacedName{
262+
Name: "observability-operator",
263+
Namespace: f.OperatorNamespace,
264+
}, &operatorDeploy)
265+
require.NoError(t, err, "observability-operator deployment must exist")
266+
require.Greater(t, operatorDeploy.Status.AvailableReplicas, int32(0),
267+
"observability-operator must have available replicas (controller must not be crash-looping)")
268+
269+
// No TempoStack should exist in the test namespace — tracing is not enabled.
270+
var stsList appsv1.StatefulSetList
271+
err = f.K8sClient.List(ctx, &stsList, client.InNamespace(ns.Name))
272+
require.NoError(t, err)
273+
for _, sts := range stsList.Items {
274+
t.Errorf("unexpected StatefulSet %q in namespace %q — no operands should be deployed for empty spec", sts.Name, ns.Name)
275+
}
276+
277+
// No OpenTelemetryCollector Deployment should exist in the test namespace.
278+
var deployList appsv1.DeploymentList
279+
err = f.K8sClient.List(ctx, &deployList, client.InNamespace(ns.Name))
280+
require.NoError(t, err)
281+
for _, d := range deployList.Items {
282+
t.Errorf("unexpected Deployment %q in namespace %q — no operands should be deployed for empty spec", d.Name, ns.Name)
283+
}
284+
285+
// Status fields must remain empty — tracing.enabled is false.
286+
var current obsv1alpha1.ObservabilityInstaller
287+
err = f.K8sClient.Get(ctx, types.NamespacedName{Name: obsInstaller.Name, Namespace: ns.Name}, &current)
288+
require.NoError(t, err)
289+
assert.Equal(t, "", current.Status.Tempo, "Status.Tempo must be empty when tracing is not enabled")
290+
assert.Equal(t, "", current.Status.OpenTelemetry, "Status.OpenTelemetry must be empty when tracing is not enabled")
291+
292+
// Clean deletion must work without errors.
293+
err = f.K8sClient.Delete(ctx, obsInstaller)
294+
require.NoError(t, err, "deleting ObservabilityInstaller with empty spec should succeed")
295+
296+
err = wait.PollUntilContextTimeout(ctx, 1*time.Second, 30*time.Second, true, func(ctx context.Context) (bool, error) {
297+
var deleted obsv1alpha1.ObservabilityInstaller
298+
err := f.K8sClient.Get(ctx, types.NamespacedName{Name: obsInstaller.Name, Namespace: ns.Name}, &deleted)
299+
if err == nil {
300+
return false, nil
301+
}
302+
if apierrors.IsNotFound(err) {
303+
return true, nil
304+
}
305+
return false, err
306+
})
307+
require.NoError(t, err, "ObservabilityInstaller with empty spec should be fully deleted")
308+
}
309+
227310
func deployManifest(t *testing.T, manifest string) client.Object {
228311
// Create an unstructured object to decode the YAML into
229312
obj := &unstructured.Unstructured{}

0 commit comments

Comments
 (0)