From 3a67380b343d44e9a1cb44df6b7395dede16128c Mon Sep 17 00:00:00 2001 From: Quentin GODRON Date: Thu, 11 Sep 2025 13:49:53 +0200 Subject: [PATCH 1/4] Add the capability to read existing secret for an S3User --- cmd/main.go | 8 + deploy/charts/s3-operator/README.md | 4 +- .../s3-operator/templates/deployment.yaml | 1 + deploy/charts/s3-operator/values.yaml | 1 + internal/controller/user/controller.go | 1 + internal/controller/user/reconcile.go | 151 ++++++++++++------ internal/controller/user/utils.go | 33 ++++ 7 files changed, 146 insertions(+), 53 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 37c86fb..85e3f48 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..7db5ada 100644 --- a/deploy/charts/s3-operator/README.md +++ b/deploy/charts/s3-operator/README.md @@ -24,6 +24,6 @@ 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..35f4b23 100644 --- a/deploy/charts/s3-operator/values.yaml +++ b/deploy/charts/s3-operator/values.yaml @@ -48,6 +48,7 @@ controllerManager: kubernetes: clusterDomain: cluster.local overrideExistingSecret: false + 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/reconcile.go b/internal/controller/user/reconcile.go index d6b4ffe..5c38a70 100644 --- a/internal/controller/user/reconcile.go +++ b/internal/controller/user/reconcile.go @@ -247,7 +247,7 @@ func (r *S3UserReconciler) handleUpdate( err, ) } - + ownedSecret := true userOwnedlinkedSecrets, err := r.getUserLinkedSecrets(ctx, userResource) if err != nil { logger.Error( @@ -267,8 +267,27 @@ func (r *S3UserReconciler) handleUpdate( err, ) } + userUnlinkedSecret, err := r.getUserUnlinkedSecret(ctx, userResource.Namespace, userResource.Spec.SecretName, userResource.Name) + 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, + ) + } 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", "userResourceName", @@ -298,6 +317,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 +495,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", @@ -751,15 +784,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", + 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 +828,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 diff --git a/internal/controller/user/utils.go b/internal/controller/user/utils.go index 21574d9..5306953 100644 --- a/internal/controller/user/utils.go +++ b/internal/controller/user/utils.go @@ -104,6 +104,39 @@ func (r *S3UserReconciler) getUserLinkedSecrets( return userSecretList, nil } + +func (r *S3UserReconciler) getUserUnlinkedSecret( + ctx context.Context, + namespace string, + secretNameA string, + secretNameB string, +) (*corev1.Secret, error) { + logger := log.FromContext(ctx) + // Listing every secrets in the S3User's namespace, as a first step + // to get the actual secret matching the S3User proper. + // TODO : proper label matching ? + secretsList := &corev1.SecretList{} + err := r.List(ctx, secretsList, client.InNamespace(namespace)) + if err != nil { + logger.Error(err, "An error occurred while listing the secrets in user's namespace") + return nil, fmt.Errorf("SecretListingFailed") + } + if len(secretsList.Items) == 0 { + logger.Info("The user's namespace doesn't appear to contain any secret") + return nil, nil + } + + var secretB *corev1.Secret + for _, secret := range secretsList.Items { + if secret.Name == secretNameA { + return &secret, nil + } else if secret.Name == secretNameB { + secretB = &secret + } + } + return secretB, nil +} + func (r *S3UserReconciler) deleteSecret(ctx context.Context, secret *corev1.Secret) error { logger := log.FromContext(ctx) logger.Info("the secret named " + secret.Name + " will be deleted") From 24cbdbad11faf5d65c92a11c39a835febc9a8714 Mon Sep 17 00:00:00 2001 From: Quentin Godron Date: Mon, 22 Sep 2025 15:49:49 +0200 Subject: [PATCH 2/4] Fix S3user update reconciliation with resource-owned secret: the secret was not deleted & recreated as needed --- cmd/main.go | 2 +- internal/controller/user/finalizer.go | 2 +- internal/controller/user/reconcile.go | 9 ++--- internal/controller/user/utils.go | 55 ++++++++------------------- 4 files changed, 22 insertions(+), 46 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 85e3f48..fa4843c 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -210,7 +210,7 @@ func main() { Client: mgr.GetClient(), Scheme: mgr.GetScheme(), OverrideExistingSecret: overrideExistingSecret, - ReadExistingSecret: readExistingSecret, + ReadExistingSecret: readExistingSecret, ReconcilePeriod: reconcilePeriod, S3factory: s3Factory, ControllerHelper: 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 5c38a70..2fa63ee 100644 --- a/internal/controller/user/reconcile.go +++ b/internal/controller/user/reconcile.go @@ -248,7 +248,7 @@ func (r *S3UserReconciler) handleUpdate( ) } ownedSecret := true - userOwnedlinkedSecrets, err := r.getUserLinkedSecrets(ctx, userResource) + userOwnedlinkedSecrets, userUnlinkedSecret, err := r.getUserLinkedSecrets(ctx, userResource) if err != nil { logger.Error( err, @@ -267,7 +267,6 @@ func (r *S3UserReconciler) handleUpdate( err, ) } - userUnlinkedSecret, err := r.getUserUnlinkedSecret(ctx, userResource.Namespace, userResource.Spec.SecretName, userResource.Name) if err != nil { logger.Error( err, @@ -287,7 +286,7 @@ func (r *S3UserReconciler) handleUpdate( ) } currentUserSecret := corev1.Secret{} - if len(userOwnedlinkedSecrets) == 0 && userUnlinkedSecret == nil { + 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", "userResourceName", @@ -792,7 +791,7 @@ func (r *S3UserReconciler) handleCreate( "userResource", userResource.Name, "NamespacedName", - req.NamespacedName.String()) + req.NamespacedName.String()) var cpData = *&existingK8sSecret.Data for k, v := range cpData { if k == userResource.Spec.SecretFieldNameSecretKey { @@ -896,7 +895,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/utils.go b/internal/controller/user/utils.go index 5306953..bd05833 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,68 +73,45 @@ 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 { for _, ref := range secret.OwnerReferences { if ref.UID == uid { - userSecretList = append(userSecretList, secret) + userOwnedSecretList = append(userOwnedSecretList, secret) + } else if secret.Name == secretConfiguredName { + notOwnedConfiguredSecret = &secret + } else if secret.Name == secretDefaultName && notOwnedConfiguredSecret == nil { + notOwnedConfiguredSecret = &secret } } } - return userSecretList, nil -} - - -func (r *S3UserReconciler) getUserUnlinkedSecret( - ctx context.Context, - namespace string, - secretNameA string, - secretNameB string, -) (*corev1.Secret, error) { - logger := log.FromContext(ctx) - // Listing every secrets in the S3User's namespace, as a first step - // to get the actual secret matching the S3User proper. - // TODO : proper label matching ? - secretsList := &corev1.SecretList{} - err := r.List(ctx, secretsList, client.InNamespace(namespace)) - if err != nil { - logger.Error(err, "An error occurred while listing the secrets in user's namespace") - return nil, fmt.Errorf("SecretListingFailed") - } - if len(secretsList.Items) == 0 { - logger.Info("The user's namespace doesn't appear to contain any secret") - return nil, nil - } - - var secretB *corev1.Secret - for _, secret := range secretsList.Items { - if secret.Name == secretNameA { - return &secret, nil - } else if secret.Name == secretNameB { - secretB = &secret - } - } - return secretB, nil + return userOwnedSecretList, notOwnedConfiguredSecret, nil } func (r *S3UserReconciler) deleteSecret(ctx context.Context, secret *corev1.Secret) error { From 9ac86d1d071d87f7d7773410b575faa0babc172f Mon Sep 17 00:00:00 2001 From: Quentin Godron Date: Mon, 22 Sep 2025 15:54:20 +0200 Subject: [PATCH 3/4] helm-doc: correctly add the description of values --- deploy/charts/s3-operator/README.md | 1 + deploy/charts/s3-operator/values.yaml | 2 ++ 2 files changed, 3 insertions(+) diff --git a/deploy/charts/s3-operator/README.md b/deploy/charts/s3-operator/README.md index 7db5ada..2d11b59 100644 --- a/deploy/charts/s3-operator/README.md +++ b/deploy/charts/s3-operator/README.md @@ -27,3 +27,4 @@ A Helm chart for deploying an operator to manage S3 resources (eg buckets, polic | 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/values.yaml b/deploy/charts/s3-operator/values.yaml index 35f4b23..25c6105 100644 --- a/deploy/charts/s3-operator/values.yaml +++ b/deploy/charts/s3-operator/values.yaml @@ -47,7 +47,9 @@ 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 From 2e076d13a35fda276fc05c49e3362b3ae78d5f3a Mon Sep 17 00:00:00 2001 From: Quentin Godron Date: Mon, 22 Sep 2025 20:17:03 +0200 Subject: [PATCH 4/4] Add tests for user creation with existing secret + test for user update with not owned secret (and fix previous commit) --- internal/controller/user/reconcile.go | 17 +- internal/controller/user/reconcile_test.go | 293 ++++++++++++++++++++- internal/controller/user/utils.go | 8 +- 3 files changed, 308 insertions(+), 10 deletions(-) diff --git a/internal/controller/user/reconcile.go b/internal/controller/user/reconcile.go index 2fa63ee..7538fa1 100644 --- a/internal/controller/user/reconcile.go +++ b/internal/controller/user/reconcile.go @@ -289,6 +289,8 @@ func (r *S3UserReconciler) handleUpdate( 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", @@ -566,7 +568,7 @@ func (r *S3UserReconciler) handleUpdate( req, userResource, s3v1alpha1.Reconciled, - "user reconciled", + "User reconciled", err, ) } @@ -757,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", @@ -874,7 +878,7 @@ func (r *S3UserReconciler) handleCreate( req, userResource, s3v1alpha1.Reconciled, - "User Reconciled", + "User reconciled", err, ) } @@ -882,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", 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 bd05833..f53007d 100644 --- a/internal/controller/user/utils.go +++ b/internal/controller/user/utils.go @@ -100,15 +100,21 @@ func (r *S3UserReconciler) getUserLinkedSecrets( 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 { userOwnedSecretList = append(userOwnedSecretList, secret) - } else if secret.Name == secretConfiguredName { + secretLinked = true + } + } + if (!secretLinked) { + if secret.Name == secretConfiguredName { notOwnedConfiguredSecret = &secret } else if secret.Name == secretDefaultName && notOwnedConfiguredSecret == nil { notOwnedConfiguredSecret = &secret } } + } return userOwnedSecretList, notOwnedConfiguredSecret, nil