Skip to content

Commit da69cd5

Browse files
graindcafeDonatien26
graindcafe
authored and
Donatien26
committed
Add tests for user creation with existing secret + test for user update with not owned secret (and fix previous commit)
1 parent 8b92ce3 commit da69cd5

File tree

3 files changed

+308
-10
lines changed

3 files changed

+308
-10
lines changed

‎internal/controller/user/reconcile.go‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,8 @@ func (r *S3UserReconciler) handleUpdate(
289289
if len(userOwnedlinkedSecrets) == 0 && userUnlinkedSecret == nil {
290290
logger.Info(
291291
"No Secret associated to user found, user will be deleted from the S3 backend, then recreated with a secret",
292+
"userResourceSpecSecretName",
293+
userResource.Spec.SecretName,
292294
"userResourceName",
293295
userResource.Name,
294296
"NamespacedName",
@@ -566,7 +568,7 @@ func (r *S3UserReconciler) handleUpdate(
566568
req,
567569
userResource,
568570
s3v1alpha1.Reconciled,
569-
"user reconciled",
571+
"User reconciled",
570572
err,
571573
)
572574
}
@@ -757,12 +759,14 @@ func (r *S3UserReconciler) handleCreate(
757759
err,
758760
)
759761
} else {
760-
// If a secret already exists, but has a different S3User owner reference, then the creation should
762+
// Case 3.1 : If a secret already exists, but has a different S3User owner reference, then the creation should
761763
// fail with no requeue, and use the status to inform that the spec should be changed
762764
for _, ref := range existingK8sSecret.OwnerReferences {
763765
if ref.Kind == "S3User" {
764766
if ref.UID != userResource.UID {
765-
logger.Error(fmt.Errorf(""), "The secret matching the new S3User's spec is owned by a different S3User.",
767+
err = fmt.Errorf("The secret matching the new S3User's spec is owned by a different S3User.")
768+
logger.Error(err,
769+
"S3User could not be created because of existing secret",
766770
"conflictingUser",
767771
ref.Name,
768772
"secretName",
@@ -874,16 +878,17 @@ func (r *S3UserReconciler) handleCreate(
874878
req,
875879
userResource,
876880
s3v1alpha1.Reconciled,
877-
"User Reconciled",
881+
"User reconciled",
878882
err,
879883
)
880884
}
881885

882886
// Case 3.3 : they are not valid, and the operator is configured keep the existing secret
883887
// The user will not be created, with no requeue and with two possible ways out : either toggle
884888
// OverrideExistingSecret on, or delete the S3User whose credentials are not working anyway.
885-
logger.Error(fmt.Errorf(""),
886-
"A secret with the same name already exists ; as the operator is configured to NOT override any pre-existing secrets, this user will not be created on S3 backend until spec change (to target new secret), or until the operator configuration is changed to override existing secrets",
889+
err = fmt.Errorf("A secret with the same name already exists ; as the operator is configured to NOT override nor read any pre-existing secrets, this user will not be created on S3 backend until spec change (to target new secret), or until the operator configuration is changed to override existing secrets")
890+
logger.Error(err,
891+
"S3User could not be created because of existing secret",
887892
"secretName",
888893
secret.Name,
889894
"userResource",

‎internal/controller/user/reconcile_test.go‎

Lines changed: 290 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,203 @@ import (
3333
"sigs.k8s.io/controller-runtime/pkg/log/zap"
3434
)
3535

36+
func TestExistingSecret(t *testing.T) {
37+
// Set up a logger before running tests
38+
log.SetLogger(zap.New(zap.UseDevMode(true)))
39+
40+
// Create a fake client with a sample CR
41+
s3UserResource := &s3v1alpha1.S3User{
42+
ObjectMeta: metav1.ObjectMeta{
43+
Name: "example-user",
44+
Namespace: "default",
45+
Generation: 1,
46+
},
47+
Spec: s3v1alpha1.S3UserSpec{
48+
S3InstanceRef: "s3-operator/default",
49+
AccessKey: "example-user",
50+
SecretName: "example-user-secret",
51+
Policies: []string{"admin"},
52+
SecretFieldNameAccessKey: "accessKey",
53+
SecretFieldNameSecretKey: "secretKey",
54+
},
55+
}
56+
thiefS3UserResource := &s3v1alpha1.S3User{
57+
ObjectMeta: metav1.ObjectMeta{
58+
Name: "example-thief-user",
59+
Namespace: "default",
60+
Generation: 1,
61+
},
62+
Spec: s3v1alpha1.S3UserSpec{
63+
S3InstanceRef: "s3-operator/default",
64+
AccessKey: "example-user",
65+
SecretName: "other-user-secret",
66+
Policies: []string{"admin"},
67+
SecretFieldNameAccessKey: "accessKey",
68+
SecretFieldNameSecretKey: "secretKey",
69+
},
70+
}
71+
blockOwnerDeletion := true
72+
controller := true
73+
74+
secretOtherS3UserResource := &corev1.Secret{
75+
ObjectMeta: metav1.ObjectMeta{
76+
Name: "other-user-secret",
77+
Namespace: "default",
78+
OwnerReferences: []metav1.OwnerReference{
79+
{
80+
APIVersion: s3UserResource.APIVersion,
81+
Kind: "S3User",
82+
Name: s3UserResource.Name,
83+
BlockOwnerDeletion: &blockOwnerDeletion,
84+
Controller: &controller,
85+
UID: "2c3d94ab-53f1-43f4-8f2b-33533be8d8e3", // chosen by fair dice roll
86+
},
87+
},
88+
},
89+
Data: map[string][]byte{
90+
"accessKey": []byte("example-user"),
91+
"secretKey": []byte("any-secret"),
92+
},
93+
}
94+
secretS3UserResource := &corev1.Secret{
95+
ObjectMeta: metav1.ObjectMeta{
96+
Name: "example-user-secret",
97+
Namespace: "default",
98+
},
99+
Data: map[string][]byte{
100+
"accessKey": []byte("example-user"),
101+
"secretKey": []byte("any-secret"),
102+
},
103+
}
104+
105+
// Add mock for s3Factory and client
106+
testUtils := TestUtils.NewTestUtils()
107+
testUtils.SetupMockedS3FactoryAndClient()
108+
s3instanceResource, secretResource := testUtils.GenerateBasicS3InstanceAndSecret()
109+
testUtils.SetupClient([]client.Object{s3instanceResource, secretResource, s3UserResource, secretS3UserResource, thiefS3UserResource, secretOtherS3UserResource})
110+
111+
// Create the reconciler
112+
reconciler := &user_controller.S3UserReconciler{
113+
Client: testUtils.Client,
114+
Scheme: testUtils.Client.Scheme(),
115+
S3factory: testUtils.S3Factory,
116+
}
117+
118+
reconciler.ReadExistingSecret = false
119+
reconciler.OverrideExistingSecret = false
120+
t.Run("error existing secret (case 3.3)", func(t *testing.T) {
121+
// Call Reconcile function
122+
req := ctrl.Request{NamespacedName: types.NamespacedName{Name: s3UserResource.Name, Namespace: s3UserResource.Namespace}}
123+
_, err := reconciler.Reconcile(context.TODO(), req)
124+
assert.NotNil(t, err)
125+
s3UserResourceUpdated := &s3v1alpha1.S3User{}
126+
err = testUtils.Client.Get(context.TODO(), client.ObjectKey{
127+
Namespace: "default",
128+
Name: "example-user",
129+
}, s3UserResourceUpdated)
130+
assert.NoError(t, err)
131+
assert.Equal(t, s3v1alpha1.CreationFailure, s3UserResourceUpdated.Status.Conditions[0].Reason)
132+
assert.Equal(t, metav1.ConditionFalse, s3UserResourceUpdated.Status.Conditions[0].Status)
133+
assert.Contains(t, s3UserResourceUpdated.Status.Conditions[0].Message, "Creation of user on S3 instance has failed because secret contains invalid credentials. The user's spec should be changed to target a different secret")
134+
})
135+
136+
reconciler.ReadExistingSecret = false
137+
reconciler.OverrideExistingSecret = true
138+
t.Run("no error override existing secret (case 3.2a)", func(t *testing.T) {
139+
140+
existingSecret := &corev1.Secret{}
141+
err := testUtils.Client.Get(context.TODO(), client.ObjectKey{
142+
Namespace: "default",
143+
Name: "example-user-secret",
144+
}, existingSecret)
145+
assert.NoError(t, err)
146+
147+
148+
// Call Reconcile function
149+
req := ctrl.Request{NamespacedName: types.NamespacedName{Name: s3UserResource.Name, Namespace: s3UserResource.Namespace}}
150+
_, err = reconciler.Reconcile(context.TODO(), req)
151+
assert.NoError(t, err)
152+
s3UserResourceUpdated := &s3v1alpha1.S3User{}
153+
err = testUtils.Client.Get(context.TODO(), client.ObjectKey{
154+
Namespace: "default",
155+
Name: "example-user",
156+
}, s3UserResourceUpdated)
157+
assert.NoError(t, err)
158+
assert.Equal(t, s3v1alpha1.Reconciled, s3UserResourceUpdated.Status.Conditions[0].Reason)
159+
assert.Equal(t, metav1.ConditionTrue, s3UserResourceUpdated.Status.Conditions[0].Status)
160+
assert.Contains(t, s3UserResourceUpdated.Status.Conditions[0].Message, "User reconciled")
161+
162+
163+
newSecret := &corev1.Secret{}
164+
err = testUtils.Client.Get(context.TODO(), client.ObjectKey{
165+
Namespace: "default",
166+
Name: "example-user-secret",
167+
}, newSecret)
168+
assert.NoError(t, err)
169+
assert.Equal(t, string(newSecret.Data["accessKey"]), string(existingSecret.Data["accessKey"]))
170+
assert.NotEqual(t, string(newSecret.Data["secretKey"]), string(existingSecret.Data["secretKey"]))
171+
172+
})
173+
reconciler.OverrideExistingSecret = false
174+
reconciler.ReadExistingSecret = true
175+
t.Run("no error read existing secret (case 3.2b)", func(t *testing.T) {
176+
existingSecret := &corev1.Secret{}
177+
err := testUtils.Client.Get(context.TODO(), client.ObjectKey{
178+
Namespace: "default",
179+
Name: "example-user-secret",
180+
}, existingSecret)
181+
assert.NoError(t, err)
182+
183+
// Call Reconcile function
184+
req := ctrl.Request{NamespacedName: types.NamespacedName{Name: s3UserResource.Name, Namespace: s3UserResource.Namespace}}
185+
_, err = reconciler.Reconcile(context.TODO(), req)
186+
assert.NoError(t, err)
187+
s3UserResourceUpdated := &s3v1alpha1.S3User{}
188+
err = testUtils.Client.Get(context.TODO(), client.ObjectKey{
189+
Namespace: "default",
190+
Name: "example-user",
191+
}, s3UserResourceUpdated)
192+
assert.NoError(t, err)
193+
assert.Equal(t, s3v1alpha1.Reconciled, s3UserResourceUpdated.Status.Conditions[0].Reason)
194+
assert.Equal(t, metav1.ConditionTrue, s3UserResourceUpdated.Status.Conditions[0].Status)
195+
assert.Contains(t, s3UserResourceUpdated.Status.Conditions[0].Message, "User reconciled")
196+
197+
newSecret := &corev1.Secret{}
198+
err = testUtils.Client.Get(context.TODO(), client.ObjectKey{
199+
Namespace: "default",
200+
Name: "example-user-secret",
201+
}, newSecret)
202+
assert.NoError(t, err)
203+
assert.Equal(t, string(newSecret.Data["accessKey"]), string(existingSecret.Data["accessKey"]))
204+
assert.Equal(t, string(newSecret.Data["secretKey"]), string(existingSecret.Data["secretKey"]))
205+
})
206+
207+
reconciler.ReadExistingSecret = false
208+
reconciler.OverrideExistingSecret = false
209+
t.Run("error existing secret owned by another one (case 3.1)", func(t *testing.T) {
210+
existingSecret := &corev1.Secret{}
211+
err := testUtils.Client.Get(context.TODO(), client.ObjectKey{
212+
Namespace: "default",
213+
Name: "other-user-secret",
214+
}, existingSecret)
215+
assert.NoError(t, err)
216+
assert.NotEqual(t, existingSecret.OwnerReferences[0].UID, thiefS3UserResource.UID)
217+
// Call Reconcile function
218+
req := ctrl.Request{NamespacedName: types.NamespacedName{Name: thiefS3UserResource.Name, Namespace: thiefS3UserResource.Namespace}}
219+
_, err = reconciler.Reconcile(context.TODO(), req)
220+
assert.NotNil(t, err)
221+
s3UserResourceUpdated := &s3v1alpha1.S3User{}
222+
err = testUtils.Client.Get(context.TODO(), client.ObjectKey{
223+
Namespace: "default",
224+
Name: "example-thief-user",
225+
}, s3UserResourceUpdated)
226+
assert.NoError(t, err)
227+
assert.Equal(t, s3v1alpha1.CreationFailure, s3UserResourceUpdated.Status.Conditions[0].Reason)
228+
assert.Equal(t, metav1.ConditionFalse, s3UserResourceUpdated.Status.Conditions[0].Status)
229+
assert.Contains(t, s3UserResourceUpdated.Status.Conditions[0].Message, "The secret matching the new S3User's spec is owned by a different, pre-existing S3User")
230+
})
231+
}
232+
36233
func TestHandleCreate(t *testing.T) {
37234
// Set up a logger before running tests
38235
log.SetLogger(zap.New(zap.UseDevMode(true)))
@@ -89,6 +286,15 @@ func TestHandleCreate(t *testing.T) {
89286
req := ctrl.Request{NamespacedName: types.NamespacedName{Name: s3UserResource.Name, Namespace: s3UserResource.Namespace}}
90287
_, err := reconciler.Reconcile(context.TODO(), req)
91288
assert.NoError(t, err)
289+
s3UserResourceUpdated := &s3v1alpha1.S3User{}
290+
err = testUtils.Client.Get(context.TODO(), client.ObjectKey{
291+
Namespace: "default",
292+
Name: "example-user",
293+
}, s3UserResourceUpdated)
294+
assert.NoError(t, err)
295+
assert.Equal(t, s3v1alpha1.Reconciled, s3UserResourceUpdated.Status.Conditions[0].Reason)
296+
assert.Equal(t, metav1.ConditionTrue, s3UserResourceUpdated.Status.Conditions[0].Status)
297+
assert.Contains(t, s3UserResourceUpdated.Status.Conditions[0].Message, "User reconciled")
92298
})
93299

94300
t.Run("error if using invalidS3Instance", func(t *testing.T) {
@@ -99,9 +305,6 @@ func TestHandleCreate(t *testing.T) {
99305
})
100306

101307
t.Run("secret is created", func(t *testing.T) {
102-
// Call Reconcile function
103-
req := ctrl.Request{NamespacedName: types.NamespacedName{Name: s3UserResource.Name, Namespace: s3UserResource.Namespace}}
104-
reconciler.Reconcile(context.TODO(), req)
105308

106309
secretCreated := &corev1.Secret{}
107310
err := testUtils.Client.Get(context.TODO(), client.ObjectKey{
@@ -113,6 +316,7 @@ func TestHandleCreate(t *testing.T) {
113316
assert.GreaterOrEqual(t, len(string(secretCreated.Data["secretKey"])), 20)
114317

115318
})
319+
116320
}
117321

118322
func TestHandleUpdate(t *testing.T) {
@@ -138,6 +342,79 @@ func TestHandleUpdate(t *testing.T) {
138342
},
139343
}
140344

345+
blockOwnerDeletion := true
346+
controller := true
347+
secretS3UserResource := &corev1.Secret{
348+
ObjectMeta: metav1.ObjectMeta{
349+
Name: "existing-valid-user-secret",
350+
Namespace: "default",
351+
OwnerReferences: []metav1.OwnerReference{
352+
{
353+
APIVersion: s3UserResource.APIVersion,
354+
Kind: s3UserResource.Kind,
355+
Name: s3UserResource.Name,
356+
BlockOwnerDeletion: &blockOwnerDeletion,
357+
Controller: &controller,
358+
UID: s3UserResource.UID,
359+
},
360+
},
361+
},
362+
Data: map[string][]byte{
363+
"accessKey": []byte("existing-valid-user"),
364+
"secretKey": []byte("validSecret"),
365+
},
366+
}
367+
368+
369+
// Add mock for s3Factory and client
370+
testUtils := TestUtils.NewTestUtils()
371+
testUtils.SetupMockedS3FactoryAndClient()
372+
s3instanceResource, secretResource := testUtils.GenerateBasicS3InstanceAndSecret()
373+
testUtils.SetupClient([]client.Object{s3instanceResource, secretResource, s3UserResource, secretS3UserResource})
374+
375+
// Create the reconciler
376+
reconciler := &user_controller.S3UserReconciler{
377+
Client: testUtils.Client,
378+
Scheme: testUtils.Client.Scheme(),
379+
S3factory: testUtils.S3Factory,
380+
}
381+
382+
t.Run("no error", func(t *testing.T) {
383+
// Call Reconcile function
384+
req := ctrl.Request{NamespacedName: types.NamespacedName{Name: s3UserResource.Name, Namespace: s3UserResource.Namespace}}
385+
_, err := reconciler.Reconcile(context.TODO(), req)
386+
assert.NoError(t, err)
387+
s3UserResourceUpdated := &s3v1alpha1.S3User{}
388+
err = testUtils.Client.Get(context.TODO(), client.ObjectKey{
389+
Namespace: "default",
390+
Name: "existing-valid-user",
391+
}, s3UserResourceUpdated)
392+
assert.NoError(t, err)
393+
assert.Equal(t, s3v1alpha1.Reconciled, s3UserResourceUpdated.Status.Conditions[0].Reason)
394+
assert.Equal(t, metav1.ConditionTrue, s3UserResourceUpdated.Status.Conditions[0].Status)
395+
assert.Contains(t, s3UserResourceUpdated.Status.Conditions[0].Message, "User reconciled")
396+
})
397+
})
398+
399+
t.Run("valid user unlinked secret", func(t *testing.T) {
400+
// Create a fake client with a sample CR
401+
s3UserResource := &s3v1alpha1.S3User{
402+
ObjectMeta: metav1.ObjectMeta{
403+
Name: "existing-valid-user",
404+
Namespace: "default",
405+
Generation: 1,
406+
Finalizers: []string{"s3.onyxia.sh/userFinalizer"},
407+
},
408+
Spec: s3v1alpha1.S3UserSpec{
409+
S3InstanceRef: "s3-operator/default",
410+
AccessKey: "existing-valid-user",
411+
SecretName: "existing-valid-user-secret",
412+
Policies: []string{"admin"},
413+
SecretFieldNameAccessKey: "accessKey",
414+
SecretFieldNameSecretKey: "secretKey",
415+
},
416+
}
417+
141418
secretS3UserResource := &corev1.Secret{
142419
ObjectMeta: metav1.ObjectMeta{
143420
Name: "existing-valid-user-secret",
@@ -149,6 +426,7 @@ func TestHandleUpdate(t *testing.T) {
149426
},
150427
}
151428

429+
152430
// Add mock for s3Factory and client
153431
testUtils := TestUtils.NewTestUtils()
154432
testUtils.SetupMockedS3FactoryAndClient()
@@ -167,6 +445,15 @@ func TestHandleUpdate(t *testing.T) {
167445
req := ctrl.Request{NamespacedName: types.NamespacedName{Name: s3UserResource.Name, Namespace: s3UserResource.Namespace}}
168446
_, err := reconciler.Reconcile(context.TODO(), req)
169447
assert.NoError(t, err)
448+
s3UserResourceUpdated := &s3v1alpha1.S3User{}
449+
err = testUtils.Client.Get(context.TODO(), client.ObjectKey{
450+
Namespace: "default",
451+
Name: "existing-valid-user",
452+
}, s3UserResourceUpdated)
453+
assert.NoError(t, err)
454+
assert.Equal(t, s3v1alpha1.Reconciled, s3UserResourceUpdated.Status.Conditions[0].Reason)
455+
assert.Equal(t, metav1.ConditionTrue, s3UserResourceUpdated.Status.Conditions[0].Status)
456+
assert.Contains(t, s3UserResourceUpdated.Status.Conditions[0].Message, "User reconciled")
170457
})
171458
})
172459

0 commit comments

Comments
 (0)