diff --git a/cmd/main.go b/cmd/main.go index 37c86fb..fa4843c 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -80,6 +80,7 @@ func main() { //K8S related variable var overrideExistingSecret bool + var readExistingSecret bool flag.StringVar( &metricsAddr, @@ -106,6 +107,12 @@ func main() { false, "Override existing secret associated to user in case of the secret already exist", ) + flag.BoolVar( + &readExistingSecret, + "read-existing-secret", + false, + "Read existing secret associated to user in case of the secret already exist", + ) opts := zap.Options{ Development: true, @@ -203,6 +210,7 @@ func main() { Client: mgr.GetClient(), Scheme: mgr.GetScheme(), OverrideExistingSecret: overrideExistingSecret, + ReadExistingSecret: readExistingSecret, ReconcilePeriod: reconcilePeriod, S3factory: s3Factory, ControllerHelper: controllerHelper, diff --git a/deploy/charts/s3-operator/README.md b/deploy/charts/s3-operator/README.md index 26713a9..2d11b59 100644 --- a/deploy/charts/s3-operator/README.md +++ b/deploy/charts/s3-operator/README.md @@ -24,6 +24,7 @@ A Helm chart for deploying an operator to manage S3 resources (eg buckets, polic | crds.install | bool | `true` | Install and upgrade CRDs | | crds.keep | bool | `true` | Keep CRDs on chart uninstall | | kubernetes.clusterDomain | string | `"cluster.local"` | | -| kubernetes.overrideExistingSecret | bool | `false` | | +| kubernetes.overrideExistingSecret | bool | `false` | When creating an S3User, update existing secret with the generated secret key | +| kubernetes.readExistingSecret | bool | `false` | When creating an S3User, read existing secret to retrieve the secret key | | s3 | object | `{"default":{"accessKey":"accessKey","createNamespace":true,"deletion":{"bucket":true,"path":false,"policy":false,"s3user":false},"enabled":false,"namespace":"s3-operator","region":"us-east-1","s3Provider":"minio","secretKey":"secretKey","url":"https://localhost:9000"}}` | Default S3 Instance | diff --git a/deploy/charts/s3-operator/templates/deployment.yaml b/deploy/charts/s3-operator/templates/deployment.yaml index a5a2755..bdefb10 100644 --- a/deploy/charts/s3-operator/templates/deployment.yaml +++ b/deploy/charts/s3-operator/templates/deployment.yaml @@ -38,6 +38,7 @@ spec: - --metrics-bind-address=127.0.0.1:8080 - --leader-elect - --override-existing-secret={{ .Values.kubernetes.overrideExistingSecret }} + - --read-existing-secret={{ .Values.kubernetes.readExistingSecret }} {{- if .Values.controllerManager.manager.extraArgs }} {{- toYaml .Values.controllerManager.manager.extraArgs | nindent 8 }} {{- end }} diff --git a/deploy/charts/s3-operator/values.yaml b/deploy/charts/s3-operator/values.yaml index cb8aa50..25c6105 100644 --- a/deploy/charts/s3-operator/values.yaml +++ b/deploy/charts/s3-operator/values.yaml @@ -47,7 +47,10 @@ controllerManager: kubernetes: clusterDomain: cluster.local + # -- When creating an S3User, update existing secret with the generated secret key overrideExistingSecret: false + # -- When creating an S3User, read existing secret to retrieve the secret key + readExistingSecret: false # -- Default S3 Instance s3: diff --git a/internal/controller/user/controller.go b/internal/controller/user/controller.go index 717ceb4..52ed853 100644 --- a/internal/controller/user/controller.go +++ b/internal/controller/user/controller.go @@ -44,6 +44,7 @@ type S3UserReconciler struct { client.Client Scheme *runtime.Scheme OverrideExistingSecret bool + ReadExistingSecret bool ReconcilePeriod time.Duration S3factory s3factory.S3Factory ControllerHelper *helpers.ControllerHelper diff --git a/internal/controller/user/finalizer.go b/internal/controller/user/finalizer.go index aab520a..d9389e4 100644 --- a/internal/controller/user/finalizer.go +++ b/internal/controller/user/finalizer.go @@ -78,7 +78,7 @@ func (r *S3UserReconciler) handleDeletion( ) } - userOwnedlinkedSecrets, err := r.getUserLinkedSecrets(ctx, userResource) + userOwnedlinkedSecrets, _, err := r.getUserLinkedSecrets(ctx, userResource) if err != nil { logger.Error( err, diff --git a/internal/controller/user/reconcile.go b/internal/controller/user/reconcile.go index d6b4ffe..7538fa1 100644 --- a/internal/controller/user/reconcile.go +++ b/internal/controller/user/reconcile.go @@ -247,8 +247,26 @@ func (r *S3UserReconciler) handleUpdate( err, ) } - - userOwnedlinkedSecrets, err := r.getUserLinkedSecrets(ctx, userResource) + ownedSecret := true + userOwnedlinkedSecrets, userUnlinkedSecret, err := r.getUserLinkedSecrets(ctx, userResource) + if err != nil { + logger.Error( + err, + "An error occurred while listing the user's secret", + "userResourceName", + userResource.Name, + "NamespacedName", + req.NamespacedName.String(), + ) + return r.SetReconciledCondition( + ctx, + req, + userResource, + s3v1alpha1.Unreachable, + "Impossible to list the user's secret", + err, + ) + } if err != nil { logger.Error( err, @@ -268,9 +286,11 @@ func (r *S3UserReconciler) handleUpdate( ) } currentUserSecret := corev1.Secret{} - if len(userOwnedlinkedSecrets) == 0 { + if len(userOwnedlinkedSecrets) == 0 && userUnlinkedSecret == nil { logger.Info( "No Secret associated to user found, user will be deleted from the S3 backend, then recreated with a secret", + "userResourceSpecSecretName", + userResource.Spec.SecretName, "userResourceName", userResource.Name, "NamespacedName", @@ -298,6 +318,9 @@ func (r *S3UserReconciler) handleUpdate( ) } return r.handleCreate(ctx, req, userResource) + } else if userUnlinkedSecret != nil { + currentUserSecret = *userUnlinkedSecret + ownedSecret = false } else { foundSecret := false for _, linkedsecret := range userOwnedlinkedSecrets { @@ -473,31 +496,42 @@ func (r *S3UserReconciler) handleUpdate( } if !credentialsValid { - logger.Info( - "The secret containing the credentials will be deleted, and the user will be deleted from the S3 backend, then recreated (through another reconcile)", - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - err = r.deleteSecret(ctx, ¤tUserSecret) - if err != nil { - logger.Error(err, "Deletion of secret associated to user have failed", "userResource", - userResource.Name, - "userResourceName", + if ownedSecret { + logger.Info( + "The secret containing the credentials will be deleted, and the user will be deleted from the S3 backend, then recreated (through another reconcile)", + "userResource", userResource.Name, "NamespacedName", - req.NamespacedName.String()) - return r.SetReconciledCondition( - ctx, - req, - userResource, - s3v1alpha1.Unreachable, - "Deletion of secret associated to user have failed", - err, + req.NamespacedName.String(), ) + err = r.deleteSecret(ctx, ¤tUserSecret) + if err != nil { + logger.Error(err, "Deletion of secret associated to user have failed", "userResource", + userResource.Name, + "userResourceName", + userResource.Name, + "NamespacedName", + req.NamespacedName.String()) + return r.SetReconciledCondition( + ctx, + req, + userResource, + s3v1alpha1.Unreachable, + "Deletion of secret associated to user have failed", + err, + ) + } + } else { + logger.Info( + "The user will be deleted from the S3 backend, then recreated (through another reconcile), the secret will be kept.", + "userResource", + userResource.Name, + "NamespacedName", + req.NamespacedName.String(), + ) } + err = s3Client.DeleteUser(userResource.Spec.AccessKey) if err != nil { logger.Error(err, "Could not delete user on S3 server", "userResource", @@ -534,7 +568,7 @@ func (r *S3UserReconciler) handleUpdate( req, userResource, s3v1alpha1.Reconciled, - "user reconciled", + "User reconciled", err, ) } @@ -725,12 +759,14 @@ func (r *S3UserReconciler) handleCreate( err, ) } else { - // If a secret already exists, but has a different S3User owner reference, then the creation should + // Case 3.1 : If a secret already exists, but has a different S3User owner reference, then the creation should // fail with no requeue, and use the status to inform that the spec should be changed for _, ref := range existingK8sSecret.OwnerReferences { if ref.Kind == "S3User" { if ref.UID != userResource.UID { - logger.Error(fmt.Errorf(""), "The secret matching the new S3User's spec is owned by a different S3User.", + err = fmt.Errorf("The secret matching the new S3User's spec is owned by a different S3User.") + logger.Error(err, + "S3User could not be created because of existing secret", "conflictingUser", ref.Name, "secretName", @@ -751,15 +787,30 @@ func (r *S3UserReconciler) handleCreate( } } - if r.OverrideExistingSecret { - // Case 3.2 : they are not valid, but the operator is configured to overwrite it - logger.Info(fmt.Sprintf("A secret with the name %s already exists ; it will be overwritten because of operator configuration", secret.Name), "secretName", - secret.Name, - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String()) - + if r.OverrideExistingSecret || r.ReadExistingSecret { + if r.ReadExistingSecret { + // Case 3.2a : read existing secret instead of updating it + logger.Info(fmt.Sprintf("The secret key will be retrieved from the secret named %s.", secret.Name), "secretName", + secret.Name, + "userResource", + userResource.Name, + "NamespacedName", + req.NamespacedName.String()) + var cpData = *&existingK8sSecret.Data + for k, v := range cpData { + if k == userResource.Spec.SecretFieldNameSecretKey { + secretKey = string(v) + } + } + } else { + // Case 3.2b : they are not valid, but the operator is configured to overwrite it + logger.Info(fmt.Sprintf("A secret with the name %s already exists ; it will be overwritten because of operator configuration", secret.Name), "secretName", + secret.Name, + "userResource", + userResource.Name, + "NamespacedName", + req.NamespacedName.String()) + } // Creating the user err = s3Client.CreateUser(userResource.Spec.AccessKey, secretKey) if err != nil { @@ -780,32 +831,33 @@ func (r *S3UserReconciler) handleCreate( err, ) } - - // Updating the secret - logger.Info("Updating the pre-existing secret with new credentials", - "secretName", - secret.Name, - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - err = r.Update(ctx, secret) - if err != nil { - logger.Error(err, "Could not update secret", "secretName", + if r.OverrideExistingSecret { + // Updating the secret + logger.Info("Updating the pre-existing secret with new credentials", + "secretName", secret.Name, "userResource", userResource.Name, "NamespacedName", - req.NamespacedName.String()) - return r.SetReconciledCondition( - ctx, - req, - userResource, - s3v1alpha1.Unreachable, - "Update of secret have failed", - err, + req.NamespacedName.String(), ) + err = r.Update(ctx, secret) + if err != nil { + logger.Error(err, "Could not update secret", "secretName", + secret.Name, + "userResource", + userResource.Name, + "NamespacedName", + req.NamespacedName.String()) + return r.SetReconciledCondition( + ctx, + req, + userResource, + s3v1alpha1.Unreachable, + "Update of secret have failed", + err, + ) + } } // Add policies @@ -826,7 +878,7 @@ func (r *S3UserReconciler) handleCreate( req, userResource, s3v1alpha1.Reconciled, - "User Reconciled", + "User reconciled", err, ) } @@ -834,8 +886,9 @@ func (r *S3UserReconciler) handleCreate( // Case 3.3 : they are not valid, and the operator is configured keep the existing secret // The user will not be created, with no requeue and with two possible ways out : either toggle // OverrideExistingSecret on, or delete the S3User whose credentials are not working anyway. - logger.Error(fmt.Errorf(""), - "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", + 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") + logger.Error(err, + "S3User could not be created because of existing secret", "secretName", secret.Name, "userResource", @@ -847,7 +900,7 @@ func (r *S3UserReconciler) handleCreate( req, userResource, s3v1alpha1.CreationFailure, - "Creation of user on S3 instance has failed necause secret contains invalid credentials. The user's spec should be changed to target a different secret", + "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", err, ) } diff --git a/internal/controller/user/reconcile_test.go b/internal/controller/user/reconcile_test.go index 9ba3c83..ff05232 100644 --- a/internal/controller/user/reconcile_test.go +++ b/internal/controller/user/reconcile_test.go @@ -33,6 +33,203 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" ) +func TestExistingSecret(t *testing.T) { + // Set up a logger before running tests + log.SetLogger(zap.New(zap.UseDevMode(true))) + + // Create a fake client with a sample CR + s3UserResource := &s3v1alpha1.S3User{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example-user", + Namespace: "default", + Generation: 1, + }, + Spec: s3v1alpha1.S3UserSpec{ + S3InstanceRef: "s3-operator/default", + AccessKey: "example-user", + SecretName: "example-user-secret", + Policies: []string{"admin"}, + SecretFieldNameAccessKey: "accessKey", + SecretFieldNameSecretKey: "secretKey", + }, + } + thiefS3UserResource := &s3v1alpha1.S3User{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example-thief-user", + Namespace: "default", + Generation: 1, + }, + Spec: s3v1alpha1.S3UserSpec{ + S3InstanceRef: "s3-operator/default", + AccessKey: "example-user", + SecretName: "other-user-secret", + Policies: []string{"admin"}, + SecretFieldNameAccessKey: "accessKey", + SecretFieldNameSecretKey: "secretKey", + }, + } + blockOwnerDeletion := true + controller := true + + secretOtherS3UserResource := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "other-user-secret", + Namespace: "default", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: s3UserResource.APIVersion, + Kind: "S3User", + Name: s3UserResource.Name, + BlockOwnerDeletion: &blockOwnerDeletion, + Controller: &controller, + UID: "2c3d94ab-53f1-43f4-8f2b-33533be8d8e3", // chosen by fair dice roll + }, + }, + }, + Data: map[string][]byte{ + "accessKey": []byte("example-user"), + "secretKey": []byte("any-secret"), + }, + } + secretS3UserResource := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example-user-secret", + Namespace: "default", + }, + Data: map[string][]byte{ + "accessKey": []byte("example-user"), + "secretKey": []byte("any-secret"), + }, + } + + // Add mock for s3Factory and client + testUtils := TestUtils.NewTestUtils() + testUtils.SetupMockedS3FactoryAndClient() + s3instanceResource, secretResource := testUtils.GenerateBasicS3InstanceAndSecret() + testUtils.SetupClient([]client.Object{s3instanceResource, secretResource, s3UserResource, secretS3UserResource, thiefS3UserResource, secretOtherS3UserResource}) + + // Create the reconciler + reconciler := &user_controller.S3UserReconciler{ + Client: testUtils.Client, + Scheme: testUtils.Client.Scheme(), + S3factory: testUtils.S3Factory, + } + + reconciler.ReadExistingSecret = false + reconciler.OverrideExistingSecret = false + t.Run("error existing secret (case 3.3)", func(t *testing.T) { + // Call Reconcile function + req := ctrl.Request{NamespacedName: types.NamespacedName{Name: s3UserResource.Name, Namespace: s3UserResource.Namespace}} + _, err := reconciler.Reconcile(context.TODO(), req) + assert.NotNil(t, err) + s3UserResourceUpdated := &s3v1alpha1.S3User{} + err = testUtils.Client.Get(context.TODO(), client.ObjectKey{ + Namespace: "default", + Name: "example-user", + }, s3UserResourceUpdated) + assert.NoError(t, err) + assert.Equal(t, s3v1alpha1.CreationFailure, s3UserResourceUpdated.Status.Conditions[0].Reason) + assert.Equal(t, metav1.ConditionFalse, s3UserResourceUpdated.Status.Conditions[0].Status) + 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") + }) + + reconciler.ReadExistingSecret = false + reconciler.OverrideExistingSecret = true + t.Run("no error override existing secret (case 3.2a)", func(t *testing.T) { + + existingSecret := &corev1.Secret{} + err := testUtils.Client.Get(context.TODO(), client.ObjectKey{ + Namespace: "default", + Name: "example-user-secret", + }, existingSecret) + assert.NoError(t, err) + + + // Call Reconcile function + req := ctrl.Request{NamespacedName: types.NamespacedName{Name: s3UserResource.Name, Namespace: s3UserResource.Namespace}} + _, err = reconciler.Reconcile(context.TODO(), req) + assert.NoError(t, err) + s3UserResourceUpdated := &s3v1alpha1.S3User{} + err = testUtils.Client.Get(context.TODO(), client.ObjectKey{ + Namespace: "default", + Name: "example-user", + }, s3UserResourceUpdated) + assert.NoError(t, err) + assert.Equal(t, s3v1alpha1.Reconciled, s3UserResourceUpdated.Status.Conditions[0].Reason) + assert.Equal(t, metav1.ConditionTrue, s3UserResourceUpdated.Status.Conditions[0].Status) + assert.Contains(t, s3UserResourceUpdated.Status.Conditions[0].Message, "User reconciled") + + + newSecret := &corev1.Secret{} + err = testUtils.Client.Get(context.TODO(), client.ObjectKey{ + Namespace: "default", + Name: "example-user-secret", + }, newSecret) + assert.NoError(t, err) + assert.Equal(t, string(newSecret.Data["accessKey"]), string(existingSecret.Data["accessKey"])) + assert.NotEqual(t, string(newSecret.Data["secretKey"]), string(existingSecret.Data["secretKey"])) + + }) + reconciler.OverrideExistingSecret = false + reconciler.ReadExistingSecret = true + t.Run("no error read existing secret (case 3.2b)", func(t *testing.T) { + existingSecret := &corev1.Secret{} + err := testUtils.Client.Get(context.TODO(), client.ObjectKey{ + Namespace: "default", + Name: "example-user-secret", + }, existingSecret) + assert.NoError(t, err) + + // Call Reconcile function + req := ctrl.Request{NamespacedName: types.NamespacedName{Name: s3UserResource.Name, Namespace: s3UserResource.Namespace}} + _, err = reconciler.Reconcile(context.TODO(), req) + assert.NoError(t, err) + s3UserResourceUpdated := &s3v1alpha1.S3User{} + err = testUtils.Client.Get(context.TODO(), client.ObjectKey{ + Namespace: "default", + Name: "example-user", + }, s3UserResourceUpdated) + assert.NoError(t, err) + assert.Equal(t, s3v1alpha1.Reconciled, s3UserResourceUpdated.Status.Conditions[0].Reason) + assert.Equal(t, metav1.ConditionTrue, s3UserResourceUpdated.Status.Conditions[0].Status) + assert.Contains(t, s3UserResourceUpdated.Status.Conditions[0].Message, "User reconciled") + + newSecret := &corev1.Secret{} + err = testUtils.Client.Get(context.TODO(), client.ObjectKey{ + Namespace: "default", + Name: "example-user-secret", + }, newSecret) + assert.NoError(t, err) + assert.Equal(t, string(newSecret.Data["accessKey"]), string(existingSecret.Data["accessKey"])) + assert.Equal(t, string(newSecret.Data["secretKey"]), string(existingSecret.Data["secretKey"])) + }) + + reconciler.ReadExistingSecret = false + reconciler.OverrideExistingSecret = false + t.Run("error existing secret owned by another one (case 3.1)", func(t *testing.T) { + existingSecret := &corev1.Secret{} + err := testUtils.Client.Get(context.TODO(), client.ObjectKey{ + Namespace: "default", + Name: "other-user-secret", + }, existingSecret) + assert.NoError(t, err) + assert.NotEqual(t, existingSecret.OwnerReferences[0].UID, thiefS3UserResource.UID) + // Call Reconcile function + req := ctrl.Request{NamespacedName: types.NamespacedName{Name: thiefS3UserResource.Name, Namespace: thiefS3UserResource.Namespace}} + _, err = reconciler.Reconcile(context.TODO(), req) + assert.NotNil(t, err) + s3UserResourceUpdated := &s3v1alpha1.S3User{} + err = testUtils.Client.Get(context.TODO(), client.ObjectKey{ + Namespace: "default", + Name: "example-thief-user", + }, s3UserResourceUpdated) + assert.NoError(t, err) + assert.Equal(t, s3v1alpha1.CreationFailure, s3UserResourceUpdated.Status.Conditions[0].Reason) + assert.Equal(t, metav1.ConditionFalse, s3UserResourceUpdated.Status.Conditions[0].Status) + assert.Contains(t, s3UserResourceUpdated.Status.Conditions[0].Message, "The secret matching the new S3User's spec is owned by a different, pre-existing S3User") + }) +} + func TestHandleCreate(t *testing.T) { // Set up a logger before running tests log.SetLogger(zap.New(zap.UseDevMode(true))) @@ -89,6 +286,15 @@ func TestHandleCreate(t *testing.T) { req := ctrl.Request{NamespacedName: types.NamespacedName{Name: s3UserResource.Name, Namespace: s3UserResource.Namespace}} _, err := reconciler.Reconcile(context.TODO(), req) assert.NoError(t, err) + s3UserResourceUpdated := &s3v1alpha1.S3User{} + err = testUtils.Client.Get(context.TODO(), client.ObjectKey{ + Namespace: "default", + Name: "example-user", + }, s3UserResourceUpdated) + assert.NoError(t, err) + assert.Equal(t, s3v1alpha1.Reconciled, s3UserResourceUpdated.Status.Conditions[0].Reason) + assert.Equal(t, metav1.ConditionTrue, s3UserResourceUpdated.Status.Conditions[0].Status) + assert.Contains(t, s3UserResourceUpdated.Status.Conditions[0].Message, "User reconciled") }) t.Run("error if using invalidS3Instance", func(t *testing.T) { @@ -99,9 +305,6 @@ func TestHandleCreate(t *testing.T) { }) t.Run("secret is created", func(t *testing.T) { - // Call Reconcile function - req := ctrl.Request{NamespacedName: types.NamespacedName{Name: s3UserResource.Name, Namespace: s3UserResource.Namespace}} - reconciler.Reconcile(context.TODO(), req) secretCreated := &corev1.Secret{} err := testUtils.Client.Get(context.TODO(), client.ObjectKey{ @@ -113,6 +316,7 @@ func TestHandleCreate(t *testing.T) { assert.GreaterOrEqual(t, len(string(secretCreated.Data["secretKey"])), 20) }) + } func TestHandleUpdate(t *testing.T) { @@ -138,6 +342,79 @@ func TestHandleUpdate(t *testing.T) { }, } + blockOwnerDeletion := true + controller := true + secretS3UserResource := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "existing-valid-user-secret", + Namespace: "default", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: s3UserResource.APIVersion, + Kind: s3UserResource.Kind, + Name: s3UserResource.Name, + BlockOwnerDeletion: &blockOwnerDeletion, + Controller: &controller, + UID: s3UserResource.UID, + }, + }, + }, + Data: map[string][]byte{ + "accessKey": []byte("existing-valid-user"), + "secretKey": []byte("validSecret"), + }, + } + + + // Add mock for s3Factory and client + testUtils := TestUtils.NewTestUtils() + testUtils.SetupMockedS3FactoryAndClient() + s3instanceResource, secretResource := testUtils.GenerateBasicS3InstanceAndSecret() + testUtils.SetupClient([]client.Object{s3instanceResource, secretResource, s3UserResource, secretS3UserResource}) + + // Create the reconciler + reconciler := &user_controller.S3UserReconciler{ + Client: testUtils.Client, + Scheme: testUtils.Client.Scheme(), + S3factory: testUtils.S3Factory, + } + + t.Run("no error", func(t *testing.T) { + // Call Reconcile function + req := ctrl.Request{NamespacedName: types.NamespacedName{Name: s3UserResource.Name, Namespace: s3UserResource.Namespace}} + _, err := reconciler.Reconcile(context.TODO(), req) + assert.NoError(t, err) + s3UserResourceUpdated := &s3v1alpha1.S3User{} + err = testUtils.Client.Get(context.TODO(), client.ObjectKey{ + Namespace: "default", + Name: "existing-valid-user", + }, s3UserResourceUpdated) + assert.NoError(t, err) + assert.Equal(t, s3v1alpha1.Reconciled, s3UserResourceUpdated.Status.Conditions[0].Reason) + assert.Equal(t, metav1.ConditionTrue, s3UserResourceUpdated.Status.Conditions[0].Status) + assert.Contains(t, s3UserResourceUpdated.Status.Conditions[0].Message, "User reconciled") + }) + }) + + t.Run("valid user unlinked secret", func(t *testing.T) { + // Create a fake client with a sample CR + s3UserResource := &s3v1alpha1.S3User{ + ObjectMeta: metav1.ObjectMeta{ + Name: "existing-valid-user", + Namespace: "default", + Generation: 1, + Finalizers: []string{"s3.onyxia.sh/userFinalizer"}, + }, + Spec: s3v1alpha1.S3UserSpec{ + S3InstanceRef: "s3-operator/default", + AccessKey: "existing-valid-user", + SecretName: "existing-valid-user-secret", + Policies: []string{"admin"}, + SecretFieldNameAccessKey: "accessKey", + SecretFieldNameSecretKey: "secretKey", + }, + } + secretS3UserResource := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "existing-valid-user-secret", @@ -149,6 +426,7 @@ func TestHandleUpdate(t *testing.T) { }, } + // Add mock for s3Factory and client testUtils := TestUtils.NewTestUtils() testUtils.SetupMockedS3FactoryAndClient() @@ -167,6 +445,15 @@ func TestHandleUpdate(t *testing.T) { req := ctrl.Request{NamespacedName: types.NamespacedName{Name: s3UserResource.Name, Namespace: s3UserResource.Namespace}} _, err := reconciler.Reconcile(context.TODO(), req) assert.NoError(t, err) + s3UserResourceUpdated := &s3v1alpha1.S3User{} + err = testUtils.Client.Get(context.TODO(), client.ObjectKey{ + Namespace: "default", + Name: "existing-valid-user", + }, s3UserResourceUpdated) + assert.NoError(t, err) + assert.Equal(t, s3v1alpha1.Reconciled, s3UserResourceUpdated.Status.Conditions[0].Reason) + assert.Equal(t, metav1.ConditionTrue, s3UserResourceUpdated.Status.Conditions[0].Status) + assert.Contains(t, s3UserResourceUpdated.Status.Conditions[0].Message, "User reconciled") }) }) diff --git a/internal/controller/user/utils.go b/internal/controller/user/utils.go index 21574d9..f53007d 100644 --- a/internal/controller/user/utils.go +++ b/internal/controller/user/utils.go @@ -65,7 +65,7 @@ func (r *S3UserReconciler) addPoliciesToUser( func (r *S3UserReconciler) getUserLinkedSecrets( ctx context.Context, userResource *s3v1alpha1.S3User, -) ([]corev1.Secret, error) { +) ([]corev1.Secret, *corev1.Secret, error) { logger := log.FromContext(ctx) // Listing every secrets in the S3User's namespace, as a first step @@ -73,35 +73,51 @@ func (r *S3UserReconciler) getUserLinkedSecrets( // TODO : proper label matching ? secretsList := &corev1.SecretList{} - userSecretList := []corev1.Secret{} + userOwnedSecretList := []corev1.Secret{} err := r.List(ctx, secretsList, client.InNamespace(userResource.Namespace)) if err != nil { logger.Error(err, "An error occurred while listing the secrets in user's namespace") - return userSecretList, fmt.Errorf("SecretListingFailed") + return userOwnedSecretList, nil, fmt.Errorf("SecretListingFailed") } if len(secretsList.Items) == 0 { logger.Info("The user's namespace doesn't appear to contain any secret") - return userSecretList, nil + return userOwnedSecretList, nil, nil } // In all the secrets inside the S3User's namespace, one should have an owner reference // pointing to the S3User. For that specific secret, we check if its name matches the one from // the S3User, whether explicit (userResource.Spec.SecretName) or implicit (userResource.Name) // In case of mismatch, that secret is deleted (and will be recreated) ; if there is a match, // it will be used for state comparison. + // We also check for secret not owned by the resource but with a name matching the configured + // or default one. If such a secret is found it will be returned separately as it is to be + // handled differently. uid := userResource.GetUID() + var secretConfiguredName string = userResource.Spec.SecretName + var secretDefaultName string = userResource.Name + var notOwnedConfiguredSecret *corev1.Secret // cmp.Or takes the first non "zero" value, see https://pkg.go.dev/cmp#Or for _, secret := range secretsList.Items { + var secretLinked = false for _, ref := range secret.OwnerReferences { if ref.UID == uid { - userSecretList = append(userSecretList, secret) + userOwnedSecretList = append(userOwnedSecretList, secret) + secretLinked = true } } + if (!secretLinked) { + if secret.Name == secretConfiguredName { + notOwnedConfiguredSecret = &secret + } else if secret.Name == secretDefaultName && notOwnedConfiguredSecret == nil { + notOwnedConfiguredSecret = &secret + } + } + } - return userSecretList, nil + return userOwnedSecretList, notOwnedConfiguredSecret, nil } func (r *S3UserReconciler) deleteSecret(ctx context.Context, secret *corev1.Secret) error {