[BUG] 🐛 fix invalid delete of associate secret of s3user

Author: Eneman Donatien

internal/controller/user/finalizer.go

@@ -78,11 +78,11 @@ func (r *S3UserReconciler) handleDeletion(
 			)
 		}
 
-		err := r.deleteOldLinkedSecret(ctx, userResource)
+		userOwnedlinkedSecrets, err := r.getUserLinkedSecrets(ctx, userResource)
 		if err != nil {
 			logger.Error(
 				err,
-				"An error occurred when trying to clean old secret linked to user",
+				"An error occurred when trying to list secret linked to user",
 				"userResourceName",
 				userResource.Name,
 				"NamespacedName",
@@ -93,29 +93,32 @@ func (r *S3UserReconciler) handleDeletion(
 				req,
 				userResource,
 				s3v1alpha1.DeletionFailure,
-				"Deletion of old secret associated to user have failed",
+				"An error occurred when trying to list secret linked to user",
 				err,
 			)
 		}
+		for _, linkedSecret := range userOwnedlinkedSecrets {
+			if err := r.deleteSecret(ctx, &linkedSecret); err != nil {
+				logger.Error(
+					err,
+					"An error occurred when trying to list secret linked to user",
+					"userResourceName",
+					userResource.Name,
+					"NamespacedName",
+					req.NamespacedName.String(),
+					"secretName",
+					linkedSecret.Name,
+				)
+				return r.SetReconciledCondition(
+					ctx,
+					req,
+					userResource,
+					s3v1alpha1.DeletionFailure,
+					"Deletion of secret associated to user have failed",
+					err,
+				)
+			}
 
-		userOwnedSecret, _ := r.getUserSecret(ctx, userResource)
-		if err := r.deleteSecret(ctx, &userOwnedSecret); err != nil {
-			logger.Error(
-				err,
-				"An error occurred when trying to clean secret linked to user",
-				"userResourceName",
-				userResource.Name,
-				"NamespacedName",
-				req.NamespacedName.String(),
-			)
-			return r.SetReconciledCondition(
-				ctx,
-				req,
-				userResource,
-				s3v1alpha1.DeletionFailure,
-				"Deletion of secret associated to user have failed",
-				err,
-			)
 		}
 
 		//Remove userFinalizer. Once all finalizers have been removed, the object will be deleted.

internal/controller/user/reconcile.go

@@ -17,6 +17,7 @@ limitations under the License.
 package user_controller
 
 import (
+	"cmp"
 	"context"
 	"fmt"
 	"slices"
@@ -247,11 +248,11 @@ func (r *S3UserReconciler) handleUpdate(
 		)
 	}
 
-	err = r.deleteOldLinkedSecret(ctx, userResource)
+	userOwnedlinkedSecrets, err := r.getUserLinkedSecrets(ctx, userResource)
 	if err != nil {
 		logger.Error(
 			err,
-			"An error occurred when trying to clean old secret linked to user",
+			"An error occurred while listing the user's secret",
 			"userResourceName",
 			userResource.Name,
 			"NamespacedName",
@@ -262,45 +263,70 @@ func (r *S3UserReconciler) handleUpdate(
 			req,
 			userResource,
 			s3v1alpha1.Unreachable,
-			"Deletion of old secret associated to user have failed",
+			"Impossible to list the user's secret",
 			err,
 		)
 	}
+	currentUserSecret := corev1.Secret{}
+	if len(userOwnedlinkedSecrets) == 0 {
+		logger.Info(
+			"No Secret associated to user found, user will be deleted from the S3 backend, then recreated with a secret",
+			"userResourceName",
+			userResource.Name,
+			"NamespacedName",
+			req.NamespacedName.String(),
+		)
 
-	userOwnedSecret, err := r.getUserSecret(ctx, userResource)
-	if err != nil {
-		if err.Error() == "SecretListingFailed" {
-			logger.Error(
+		err = s3Client.DeleteUser(userResource.Spec.AccessKey)
+		if err != nil {
+			logger.Error(err, "Could not delete user on S3 server", "userResource",
+				userResource.Name,
+				"userResourceName",
+				userResource.Name,
+				"NamespacedName",
+				req.NamespacedName.String())
+			return r.SetReconciledCondition(
+				ctx,
+				req,
+				userResource,
+				s3v1alpha1.Unreachable,
+				fmt.Sprintf(
+					"Deletion of S3user %s on S3 server has failed",
+					userResource.Name,
+				),
 				err,
-				"An error occurred when trying to obtain the user's secret",
+			)
+		}
+		return r.handleCreate(ctx, req, userResource)
+	} else {
+		foundSecret := false
+		for _, linkedsecret := range userOwnedlinkedSecrets {
+			if linkedsecret.Name != cmp.Or(userResource.Spec.SecretName, userResource.Name) {
+				if err := r.deleteSecret(ctx, &linkedsecret); err != nil {
+					logger.Info("Failed to delete unused secret", "secretName", linkedsecret.Name)
+					return r.SetReconciledCondition(
+						ctx,
+						req,
+						userResource,
+						s3v1alpha1.Unreachable,
+						"Failed to delete old linkedSecret",
+						err,
+					)
+				}
+			} else {
+				foundSecret = true
+				currentUserSecret = linkedsecret
+			}
+		}
+		if !foundSecret {
+			logger.Info(
+				"No Secret associated to user found, user will be deleted from the S3 backend, then recreated with a secret",
 				"userResourceName",
 				userResource.Name,
 				"NamespacedName",
 				req.NamespacedName.String(),
 			)
 
-			err = r.deleteSecret(ctx, &userOwnedSecret)
-			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,
-				)
-
-			}
 			err = s3Client.DeleteUser(userResource.Spec.AccessKey)
 			if err != nil {
 				logger.Error(err, "Could not delete user on S3 server", "userResource",
@@ -320,63 +346,9 @@ func (r *S3UserReconciler) handleUpdate(
 					),
 					err,
 				)
-
 			}
 			return r.handleCreate(ctx, req, userResource)
-		} else if err.Error() == "S3UserSecretNameMismatch" {
-			logger.Info("A secret with owner reference to the user was found, but its name doesn't match the spec. This is probably due to the S3User's spec changing (specifically spec.secretName being added, changed or removed). The \"old\" secret will be deleted.", "userResource",
-				userResource.Name,
-				"NamespacedName",
-				req.NamespacedName.String())
-			err = r.deleteSecret(ctx, &userOwnedSecret)
-			if err != nil {
-				logger.Error(err, "Deletion of secret associated to user have failed", "userResourceName",
-					userResource.Name,
-					"NamespacedName",
-					req.NamespacedName.String())
-				return r.SetReconciledCondition(
-					ctx,
-					req,
-					userResource,
-					s3v1alpha1.Unreachable,
-					"Deletion of secret associated to user have failed",
-					err,
-				)
-
-			}
-		}
-	}
-
-	if userOwnedSecret.Name == "" {
-		logger.Info(
-			"Secret associated to user not found, user will be deleted from the S3 backend, then recreated with a secret",
-			"userResourceName",
-			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",
-				userResource.Name,
-				"userResourceName",
-				userResource.Name,
-				"NamespacedName",
-				req.NamespacedName.String())
-			return r.SetReconciledCondition(
-				ctx,
-				req,
-				userResource,
-				s3v1alpha1.Unreachable,
-				fmt.Sprintf(
-					"Deletion of S3user %s on S3 server has failed",
-					userResource.Name,
-				),
-				err,
-			)
 		}
-		return r.handleCreate(ctx, req, userResource)
 	}
 
 	logger.Info("Checking user policies", "userResource",
@@ -477,8 +449,8 @@ func (r *S3UserReconciler) handleUpdate(
 
 	credentialsValid, err := s3Client.CheckUserCredentialsValid(
 		userResource.Name,
-		string(userOwnedSecret.Data[userResource.Spec.SecretFieldNameAccessKey]),
-		string(userOwnedSecret.Data[userResource.Spec.SecretFieldNameSecretKey]),
+		string(currentUserSecret.Data[userResource.Spec.SecretFieldNameAccessKey]),
+		string(currentUserSecret.Data[userResource.Spec.SecretFieldNameSecretKey]),
 	)
 
 	if err != nil {
@@ -508,7 +480,7 @@ func (r *S3UserReconciler) handleUpdate(
 			"NamespacedName",
 			req.NamespacedName.String(),
 		)
-		err = r.deleteSecret(ctx, &userOwnedSecret)
+		err = r.deleteSecret(ctx, &currentUserSecret)
 		if err != nil {
 			logger.Error(err, "Deletion of secret associated to user have failed", "userResource",
 				userResource.Name,

internal/controller/user/utils.go

@@ -21,15 +21,12 @@ import (
 	"context"
 	"fmt"
 
+	s3v1alpha1 "github.com/InseeFrLab/s3-operator/api/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
-	k8sapierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
-
-	s3v1alpha1 "github.com/InseeFrLab/s3-operator/api/v1alpha1"
 )
 
 func (r *S3UserReconciler) addPoliciesToUser(
@@ -65,69 +62,46 @@ func (r *S3UserReconciler) addPoliciesToUser(
 	return nil
 }
 
-func (r *S3UserReconciler) deleteOldLinkedSecret(ctx context.Context, userResource *s3v1alpha1.S3User) error {
+func (r *S3UserReconciler) getUserLinkedSecrets(
+	ctx context.Context,
+	userResource *s3v1alpha1.S3User,
+) ([]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{}
 
-	// Define options with label selector and namespace
-	listOpts := []client.ListOption{
-		client.InNamespace(userResource.Namespace),                           // Filter by namespace
-		client.MatchingLabels{"app.kubernetes.io/created-by": "s3-operator"}, // Filter by label
-	}
+	userSecretList := []corev1.Secret{}
 
-	// List Secrets with the specified label in the given namespace
-	if err := r.List(ctx, secretsList, listOpts...); err != nil {
-		return fmt.Errorf("failed to list secrets in namespace %s: %w", userResource.Namespace, err)
+	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")
 	}
 
+	if len(secretsList.Items) == 0 {
+		logger.Info("The user's namespace doesn't appear to contain any secret")
+		return userSecretList, 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.
+	uid := userResource.GetUID()
+
+	// 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 == userResource.GetUID() {
-				if (userResource.Spec.SecretName != "" && secret.Name != userResource.Spec.SecretName) || (userResource.Spec.SecretName == "" && secret.Name != userResource.Name) {
-					if err := r.deleteSecret(ctx, &secret); err != nil {
-						logger.Info("Failed to delete unused secret", "secret", secret.Name)
-						return fmt.Errorf("failed to delete unused secret %s, err %w", secret.Name, err)
-					}
-				}
+			if ref.UID == uid {
+				userSecretList = append(userSecretList, secret)
 			}
 		}
 	}
 
-	return nil
-}
-
-func (r *S3UserReconciler) getUserSecret(
-	ctx context.Context,
-	userResource *s3v1alpha1.S3User,
-) (corev1.Secret, error) {
-	userSecret := &corev1.Secret{}
-	secretName := userResource.Spec.SecretName
-	if secretName == "" {
-		secretName = userResource.Name
-	}
-	err := r.Get(
-		ctx,
-		types.NamespacedName{Namespace: userResource.Namespace, Name: secretName},
-		userSecret,
-	)
-	if err != nil {
-		if k8sapierrors.IsNotFound(err) {
-			return *userSecret, fmt.Errorf(
-				"secret %s not found in namespace %s",
-				secretName,
-				userResource.Namespace,
-			)
-		}
-		return *userSecret, err
-	}
-
-	for _, ref := range userSecret.OwnerReferences {
-		if ref.UID == userResource.GetUID() {
-			return *userSecret, nil
-		}
-	}
-
-	return *userSecret, err
+	return userSecretList, nil
 }
 
 func (r *S3UserReconciler) deleteSecret(ctx context.Context, secret *corev1.Secret) error {