Skip to content

Commit 005037c

Browse files
s-vitaliys-vitaliy
authored
Fix secret reference error handling (#210)
* Implement proper logging if secret ref not found * Fix * Fix * Fix unit tests * Remove outdated test
1 parent e7c5d04 commit 005037c

File tree

9 files changed

+140
-92
lines changed

9 files changed

+140
-92
lines changed

‎generate.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,3 +4,4 @@ package main
44
//go:generate mockgen -destination=./tests/mocks/unmanaged_reconciler.go -package=mocks github.com/SneaksAndData/arcane-operator/services/controllers UnmanagedReconciler
55
//go:generate mockgen -destination=./tests/mocks/controller.go -package=mocks sigs.k8s.io/controller-runtime/pkg/controller Controller
66
//go:generate mockgen -destination=./tests/mocks/job_builder.go -package=mocks github.com/SneaksAndData/arcane-operator/services/controllers/stream JobBuilder
7+
//go:generate mockgen -destination=./tests/mocks/job_mock/secret_reference_provider.go -package=mocks github.com/SneaksAndData/arcane-operator/services/job SecretReferenceProvider

‎services/controllers/stream/stream_metadata_service.go‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package stream
22

33
import (
4-
"fmt"
54
v1 "github.com/SneaksAndData/arcane-operator/pkg/apis/streaming/v1"
65
"github.com/SneaksAndData/arcane-operator/services/job"
76
)
@@ -16,12 +15,8 @@ type streamClassMetadataService struct {
1615
func (s streamClassMetadataService) JobConfigurator() (job.Configurator, error) {
1716
builder := job.NewConfiguratorChainBuilder()
1817

19-
for _, secretName := range s.streamClass.Spec.SecretRefs {
20-
name, err := s.streamDefinition.GetReferenceForSecret(secretName)
21-
if err != nil {
22-
return nil, fmt.Errorf("error getting secret reference: %w", err)
23-
}
24-
builder = builder.WithConfigurator(job.NewSecretReferenceConfigurator(name))
18+
for _, referenceFieldName := range s.streamClass.Spec.SecretRefs {
19+
builder = builder.WithConfigurator(job.NewSecretReferenceConfigurator(referenceFieldName, s.streamDefinition))
2520
}
2621

2722
return builder.Build(), nil

‎services/controllers/stream/stream_metadata_service_test.go‎

Lines changed: 0 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -124,39 +124,6 @@ func Test_StreamMetadataService_JobConfigurator_SingleSecretRef(t *testing.T) {
124124
require.Equal(t, "databaseCredentials", job.Spec.Template.Spec.Containers[0].EnvFrom[0].SecretRef.Name)
125125
}
126126

127-
func Test_StreamMetadataService_JobConfigurator_MissingSecretField(t *testing.T) {
128-
// Arrange
129-
streamClass := &v1.StreamClass{
130-
ObjectMeta: metav1.ObjectMeta{
131-
Name: "test-stream-class",
132-
},
133-
Spec: v1.StreamClassSpec{
134-
APIGroupRef: "streaming.sneaksanddata.com",
135-
APIVersion: "v1",
136-
KindRef: "MockStreamDefinition",
137-
PluralName: "mockstreamdefinitions",
138-
SecretRefs: []string{"nonExistentSecret"},
139-
},
140-
}
141-
142-
fakeClient := setupFakeClient(nil)
143-
unstructuredObj, err := getUnstructured(t, fakeClient)
144-
require.NoError(t, err)
145-
146-
streamDefinition, err := fromUnstructured(&unstructuredObj)
147-
require.NoError(t, err)
148-
149-
service := NewStreamMetadataService(streamClass, streamDefinition)
150-
151-
// Act
152-
configurator, err := service.JobConfigurator()
153-
154-
// Assert
155-
require.Error(t, err)
156-
require.Nil(t, configurator)
157-
require.ErrorContains(t, err, "error getting secret reference")
158-
}
159-
160127
func Test_StreamMetadataService_JobConfigurator_NilSecretRefs(t *testing.T) {
161128
// Arrange
162129
streamClass := &v1.StreamClass{
@@ -343,40 +310,6 @@ func Test_StreamMetadataService_JobConfigurator_PreservesExistingEnvFrom(t *test
343310
require.Equal(t, "my-secret", job.Spec.Template.Spec.Containers[0].EnvFrom[1].SecretRef.Name)
344311
}
345312

346-
func Test_StreamMetadataService_JobConfigurator_PartialFailure(t *testing.T) {
347-
// Arrange
348-
streamClass := &v1.StreamClass{
349-
ObjectMeta: metav1.ObjectMeta{
350-
Name: "test-stream-class",
351-
},
352-
Spec: v1.StreamClassSpec{
353-
APIGroupRef: "streaming.sneaksanddata.com",
354-
APIVersion: "v1",
355-
KindRef: "MockStreamDefinition",
356-
PluralName: "mockstreamdefinitions",
357-
SecretRefs: []string{"secretRef", "nonExistentSecret"},
358-
},
359-
}
360-
361-
fakeClient := setupFakeClientWithSecrets("databaseCredentials")
362-
unstructuredObj, err := getUnstructured(t, fakeClient)
363-
require.NoError(t, err)
364-
365-
streamDefinition, err := fromUnstructured(&unstructuredObj)
366-
require.NoError(t, err)
367-
368-
service := NewStreamMetadataService(streamClass, streamDefinition)
369-
370-
// Act
371-
configurator, err := service.JobConfigurator()
372-
373-
// Assert - should fail on the first missing secret
374-
require.Error(t, err)
375-
require.Nil(t, configurator)
376-
require.ErrorContains(t, err, "error getting secret reference")
377-
require.ErrorContains(t, err, "nonExistentSecret")
378-
}
379-
380313
// setupFakeClientWithSecrets creates a fake client with a MockStreamDefinition that has secret references
381314
func setupFakeClientWithSecrets(secrets string) client.WithWatch {
382315
sd := testv1.MockStreamDefinition{

‎services/controllers/stream/stream_reconciler.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"github.com/SneaksAndData/arcane-operator/services/controllers"
88
"github.com/SneaksAndData/arcane-operator/services/job"
99
batchv1 "k8s.io/api/batch/v1"
10+
corev1 "k8s.io/api/core/v1"
1011
"k8s.io/apimachinery/pkg/api/errors"
1112
"k8s.io/apimachinery/pkg/api/meta"
1213
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -457,6 +458,10 @@ func (s *streamReconciler) startNewJob(ctx context.Context, definition Definitio
457458
j, err := s.jobBuilder.BuildJob(ctx, templateReference, combinedConfigurator)
458459
if err != nil { // coverage-ignore
459460
logger.V(0).Error(err, "failed to build job")
461+
s.eventRecorder.Eventf(definition.ToUnstructured(),
462+
corev1.EventTypeWarning,
463+
"FailedCreateJob",
464+
"failed to create job: %v", err)
460465
return err
461466
}
462467

‎services/controllers/stream/unstructured_wrapper.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,9 @@ import (
1515
)
1616

1717
var (
18-
_ Definition = (*unstructuredWrapper)(nil)
19-
_ job.ConfiguratorProvider = (*unstructuredWrapper)(nil)
18+
_ Definition = (*unstructuredWrapper)(nil)
19+
_ job.ConfiguratorProvider = (*unstructuredWrapper)(nil)
20+
_ job.SecretReferenceProvider = (*unstructuredWrapper)(nil)
2021
)
2122

2223
type unstructuredWrapper struct {

‎services/job/secret_reference_configurator.go‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,16 @@ import (
99
var _ Configurator = &secretReferenceConfigurator{}
1010

1111
type secretReferenceConfigurator struct {
12-
reference *corev1.LocalObjectReference
12+
referenceFieldName string
13+
streamDefinition SecretReferenceProvider
1314
}
1415

1516
func (s secretReferenceConfigurator) ConfigureJob(job *batchv1.Job) error {
16-
if s.reference == nil {
17-
return fmt.Errorf("secretReferenceConfigurator reference is nil")
17+
reference, err := s.streamDefinition.GetReferenceForSecret(s.referenceFieldName)
18+
if err != nil {
19+
return fmt.Errorf("error getting secret reference: %w", err)
1820
}
19-
if s.reference.Name == "" {
21+
if reference.Name == "" {
2022
return fmt.Errorf("secretReferenceConfigurator reference name is empty")
2123
}
2224
if len(job.Spec.Template.Spec.Containers) == 0 {
@@ -31,16 +33,17 @@ func (s secretReferenceConfigurator) ConfigureJob(job *batchv1.Job) error {
3133

3234
job.Spec.Template.Spec.Containers[i].EnvFrom = append(job.Spec.Template.Spec.Containers[i].EnvFrom, corev1.EnvFromSource{
3335
SecretRef: &corev1.SecretEnvSource{
34-
LocalObjectReference: *s.reference,
36+
LocalObjectReference: *reference,
3537
},
3638
})
3739
}
3840

3941
return nil
4042
}
4143

42-
func NewSecretReferenceConfigurator(reference *corev1.LocalObjectReference) Configurator {
44+
func NewSecretReferenceConfigurator(referenceFieldName string, sd SecretReferenceProvider) Configurator {
4345
return &secretReferenceConfigurator{
44-
reference: reference,
46+
referenceFieldName: referenceFieldName,
47+
streamDefinition: sd,
4548
}
4649
}

‎services/job/secret_reference_configurator_test.go‎

Lines changed: 53 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package job
22

33
import (
4+
"github.com/SneaksAndData/arcane-operator/tests/mocks/job_mock"
5+
"go.uber.org/mock/gomock"
46
"testing"
57

68
"github.com/stretchr/testify/require"
@@ -24,7 +26,12 @@ func Test_SecretReferenceConfigurator_Add_SecretRef_To_Empty_Job(t *testing.T) {
2426
},
2527
}
2628

27-
configurator := NewSecretReferenceConfigurator(&corev1.LocalObjectReference{Name: "my-secret"})
29+
mockCtrl := gomock.NewController(t)
30+
defer mockCtrl.Finish()
31+
mockProvider := job_mock.NewMockSecretReferenceProvider(mockCtrl)
32+
mockProvider.EXPECT().GetReferenceForSecret("test-secret").Return(&corev1.LocalObjectReference{Name: "my-secret"}, nil)
33+
34+
configurator := NewSecretReferenceConfigurator("test-secret", mockProvider)
2835
err := configurator.ConfigureJob(job)
2936
require.NoError(t, err)
3037
require.NotNil(t, job.Spec.Template.Spec.Containers[0].EnvFrom)
@@ -58,7 +65,12 @@ func Test_SecretReferenceConfigurator_Append_To_Existing_EnvFrom(t *testing.T) {
5865
},
5966
}
6067

61-
configurator := NewSecretReferenceConfigurator(&corev1.LocalObjectReference{Name: "new-secret"})
68+
mockCtrl := gomock.NewController(t)
69+
defer mockCtrl.Finish()
70+
mockProvider := job_mock.NewMockSecretReferenceProvider(mockCtrl)
71+
mockProvider.EXPECT().GetReferenceForSecret("test-secret").Return(&corev1.LocalObjectReference{Name: "new-secret"}, nil)
72+
73+
configurator := NewSecretReferenceConfigurator("test-secret", mockProvider)
6274
err := configurator.ConfigureJob(job)
6375
require.NoError(t, err)
6476
require.Len(t, job.Spec.Template.Spec.Containers[0].EnvFrom, 2)
@@ -79,7 +91,12 @@ func Test_SecretReferenceConfigurator_No_Containers(t *testing.T) {
7991
},
8092
}
8193

82-
configurator := NewSecretReferenceConfigurator(&corev1.LocalObjectReference{Name: "my-secret"})
94+
mockCtrl := gomock.NewController(t)
95+
defer mockCtrl.Finish()
96+
mockProvider := job_mock.NewMockSecretReferenceProvider(mockCtrl)
97+
mockProvider.EXPECT().GetReferenceForSecret("test-secret").Return(&corev1.LocalObjectReference{Name: "my-secret"}, nil)
98+
99+
configurator := NewSecretReferenceConfigurator("test-secret", mockProvider)
83100
err := configurator.ConfigureJob(job)
84101
require.NoError(t, err)
85102
require.Empty(t, job.Spec.Template.Spec.Containers)
@@ -96,7 +113,12 @@ func Test_SecretReferenceConfigurator_Nil_Containers(t *testing.T) {
96113
},
97114
}
98115

99-
configurator := NewSecretReferenceConfigurator(&corev1.LocalObjectReference{Name: "my-secret"})
116+
mockCtrl := gomock.NewController(t)
117+
defer mockCtrl.Finish()
118+
mockProvider := job_mock.NewMockSecretReferenceProvider(mockCtrl)
119+
mockProvider.EXPECT().GetReferenceForSecret("test-secret").Return(&corev1.LocalObjectReference{Name: "my-secret"}, nil)
120+
121+
configurator := NewSecretReferenceConfigurator("test-secret", mockProvider)
100122
err := configurator.ConfigureJob(job)
101123
require.NoError(t, err)
102124
require.Nil(t, job.Spec.Template.Spec.Containers)
@@ -118,11 +140,18 @@ func Test_SecretReferenceConfigurator_Multiple_Secrets(t *testing.T) {
118140
},
119141
}
120142

121-
configurator1 := NewSecretReferenceConfigurator(&corev1.LocalObjectReference{Name: "first-secret"})
143+
mockCtrl := gomock.NewController(t)
144+
defer mockCtrl.Finish()
145+
146+
mockProvider1 := job_mock.NewMockSecretReferenceProvider(mockCtrl)
147+
mockProvider1.EXPECT().GetReferenceForSecret("secret1").Return(&corev1.LocalObjectReference{Name: "first-secret"}, nil)
148+
configurator1 := NewSecretReferenceConfigurator("secret1", mockProvider1)
122149
err := configurator1.ConfigureJob(job)
123150
require.NoError(t, err)
124151

125-
configurator2 := NewSecretReferenceConfigurator(&corev1.LocalObjectReference{Name: "second-secret"})
152+
mockProvider2 := job_mock.NewMockSecretReferenceProvider(mockCtrl)
153+
mockProvider2.EXPECT().GetReferenceForSecret("secret2").Return(&corev1.LocalObjectReference{Name: "second-secret"}, nil)
154+
configurator2 := NewSecretReferenceConfigurator("secret2", mockProvider2)
126155
err = configurator2.ConfigureJob(job)
127156
require.NoError(t, err)
128157

@@ -147,7 +176,12 @@ func Test_SecretReferenceConfigurator_Empty_Secret_Name(t *testing.T) {
147176
},
148177
}
149178

150-
configurator := NewSecretReferenceConfigurator(&corev1.LocalObjectReference{Name: ""})
179+
mockCtrl := gomock.NewController(t)
180+
defer mockCtrl.Finish()
181+
mockProvider := job_mock.NewMockSecretReferenceProvider(mockCtrl)
182+
mockProvider.EXPECT().GetReferenceForSecret("test-secret").Return(&corev1.LocalObjectReference{Name: ""}, nil)
183+
184+
configurator := NewSecretReferenceConfigurator("test-secret", mockProvider)
151185
err := configurator.ConfigureJob(job)
152186
require.EqualError(t, err, "secretReferenceConfigurator reference name is empty")
153187
}
@@ -176,7 +210,12 @@ func Test_SecretReferenceConfigurator_Affects_All_Containers(t *testing.T) {
176210
},
177211
}
178212

179-
configurator := NewSecretReferenceConfigurator(&corev1.LocalObjectReference{Name: "my-secret"})
213+
mockCtrl := gomock.NewController(t)
214+
defer mockCtrl.Finish()
215+
mockProvider := job_mock.NewMockSecretReferenceProvider(mockCtrl)
216+
mockProvider.EXPECT().GetReferenceForSecret("test-secret").Return(&corev1.LocalObjectReference{Name: "my-secret"}, nil)
217+
218+
configurator := NewSecretReferenceConfigurator("test-secret", mockProvider)
180219
err := configurator.ConfigureJob(job)
181220
require.NoError(t, err)
182221

@@ -216,7 +255,12 @@ func Test_SecretReferenceConfigurator_With_Existing_SecretRef(t *testing.T) {
216255
},
217256
}
218257

219-
configurator := NewSecretReferenceConfigurator(&corev1.LocalObjectReference{Name: "new-secret"})
258+
mockCtrl := gomock.NewController(t)
259+
defer mockCtrl.Finish()
260+
mockProvider := job_mock.NewMockSecretReferenceProvider(mockCtrl)
261+
mockProvider.EXPECT().GetReferenceForSecret("test-secret").Return(&corev1.LocalObjectReference{Name: "new-secret"}, nil)
262+
263+
configurator := NewSecretReferenceConfigurator("test-secret", mockProvider)
220264
err := configurator.ConfigureJob(job)
221265
require.NoError(t, err)
222266

‎services/job/secret_reference_provider.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
package job
2+
3+
import corev1 "k8s.io/api/core/v1"
4+
5+
// SecretReferenceProvider defines an interface for types that can provide a reference to a Kubernetes Secret.
6+
type SecretReferenceProvider interface {
7+
8+
// GetReferenceForSecret retrieves a LocalObjectReference for the specified secret name.
9+
GetReferenceForSecret(name string) (*corev1.LocalObjectReference, error)
10+
}

‎tests/mocks/job_mock/secret_reference_provider.go‎

Lines changed: 56 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)