From a8f904d17e2dd4e91e9b3d1f40c5580c9039fdff Mon Sep 17 00:00:00 2001 From: Quentin Godron <33049+graindcafe@users.noreply.github.com> Date: Tue, 23 Sep 2025 19:45:01 +0200 Subject: [PATCH 1/6] 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 3eb8fad368678ea0f54b3201407acbdda69c1800 Mon Sep 17 00:00:00 2001 From: Quentin Godron <33049+graindcafe@users.noreply.github.com> Date: Tue, 23 Sep 2025 19:45:19 +0200 Subject: [PATCH 2/6] 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 5f6e007e511729a4039eff18183637aebb43c58a Mon Sep 17 00:00:00 2001 From: Quentin Godron <33049+graindcafe@users.noreply.github.com> Date: Tue, 23 Sep 2025 19:45:22 +0200 Subject: [PATCH 3/6] 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 fad931bfe6d1957bd5137269316364b6ece18944 Mon Sep 17 00:00:00 2001 From: Quentin Godron <33049+graindcafe@users.noreply.github.com> Date: Tue, 23 Sep 2025 19:45:33 +0200 Subject: [PATCH 4/6] 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 From bc3bb4808dc73064e280a353526d4d1f09dbe215 Mon Sep 17 00:00:00 2001 From: Zadkiel AHARONIAN Date: Mon, 15 Sep 2025 13:16:10 +0200 Subject: [PATCH 5/6] docs: Fix Helm chart repository link and installation commands --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 8db24ba..74b907a 100644 --- a/README.md +++ b/README.md @@ -45,11 +45,11 @@ The S3 operator is provided either in source form through this repository, or al ### Helm -With this Docker image, the recommended way of installing S3 Operator on your cluster is to use the Helm chart provided in the dedicated repository : https://github.com/InseeFrLab/helm-charts/tree/master/charts/s3-operator. Among other things, the chart takes care of managing a (Kubernetes) ServiceAccount for the operator to run as. The most basic way of using this chart would be : +With this Docker image, the recommended way of installing S3 Operator on your cluster is to use the Helm chart provided in this repository [`deploy/charts/s3-operator` directory](./deploy/charts/s3-operator). Among other things, the chart takes care of managing a (Kubernetes) ServiceAccount for the operator to run as. The most basic way of using this chart would be: ```shell -helm repo add inseefrlab https://inseefrlab.github.io/helm-charts # or [helm repo update] if already available -helm install s3-operator --values # see below for the parameters +helm repo add inseefrlab-s3operator https://inseefrlab.github.io/s3-operator # or [helm repo update] if already available +helm install inseefrlab-s3operator/s3-operator --values # see below for the parameters ``` ### Running from source From 52fad45ab7c3dc4827e99a5c8915c9ea8beb23df Mon Sep 17 00:00:00 2001 From: Quentin GODRON <33049+graindcafe@users.noreply.github.com> Date: Tue, 23 Sep 2025 19:36:58 +0200 Subject: [PATCH 6/6] Improve status condition handling - Replace single status condition type with multiple ones to better reflect resource reconciliation states. - During reconciliation, update conditions instead of relying on explicit logs, ensuring status information is surfaced in the resource itself while still producing INFO/ERROR logs. - Add condition reasons to distinguish errors originating from S3, the operator, or Kubernetes. --- api/v1alpha1/types.go | 8 +- internal/controller/bucket/finalizer.go | 58 +- internal/controller/bucket/reconcile.go | 286 +++----- internal/controller/bucket/status.go | 72 ++- internal/controller/path/finalizer.go | 72 ++- internal/controller/path/reconcile.go | 160 ++--- internal/controller/path/status.go | 73 ++- internal/controller/policy/finalizer.go | 57 +- internal/controller/policy/reconcile.go | 201 +++--- internal/controller/policy/status.go | 72 ++- internal/controller/s3instance/finalizer.go | 61 +- .../controller/s3instance/finalizer_test.go | 8 +- internal/controller/s3instance/reconcile.go | 131 ++-- .../controller/s3instance/reconcile_test.go | 12 +- internal/controller/s3instance/status.go | 72 ++- internal/controller/user/finalizer.go | 86 ++- internal/controller/user/reconcile.go | 611 +++++++----------- internal/controller/user/reconcile_test.go | 56 +- internal/controller/user/status.go | 72 ++- internal/helpers/S3instance_test.go | 4 +- internal/helpers/controller.go | 34 +- internal/helpers/controller_test.go | 16 +- internal/helpers/s3instance.go | 6 +- test/utils/testUtils.go | 4 +- 24 files changed, 1100 insertions(+), 1132 deletions(-) diff --git a/api/v1alpha1/types.go b/api/v1alpha1/types.go index 8a44415..40a04a7 100644 --- a/api/v1alpha1/types.go +++ b/api/v1alpha1/types.go @@ -2,13 +2,17 @@ package v1alpha1 // Definitions to manage status condition types const ( - // ConditionReconciled represents the status of the resource reconciliation - ConditionReconciled = "Reconciled" + ConditionAvailable = "Available" // The resource is in sync with S3 + ConditionProgressing = "Progressing" // The resource is being synced with S3 + ConditionDegraded = "Degraded" // The sync with S3 has failed, resource state is unknown + ConditionRejected = "Rejected" // The resource cannot be sync with S3 and does not exists on S3 ) // Definitions to manage status condition reasons const ( Reconciling = "Reconciling" + InternalError = "OperatorError" + K8sApiError = "KubernetesAPIError" Unreachable = "Unreachable" CreationFailure = "CreationFailure" Reconciled = "Reconciled" diff --git a/internal/controller/bucket/finalizer.go b/internal/controller/bucket/finalizer.go index 3517ac1..4511f32 100644 --- a/internal/controller/bucket/finalizer.go +++ b/internal/controller/bucket/finalizer.go @@ -18,12 +18,13 @@ package bucket_controller import ( "context" + "fmt" s3v1alpha1 "github.com/InseeFrLab/s3-operator/api/v1alpha1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -32,36 +33,29 @@ func (r *BucketReconciler) handleDeletion( req reconcile.Request, bucketResource *s3v1alpha1.Bucket, ) (reconcile.Result, error) { - logger := log.FromContext(ctx) if controllerutil.ContainsFinalizer(bucketResource, bucketFinalizer) { - if err := r.finalizeBucket(ctx, bucketResource); err != nil { - logger.Error( - err, - "An error occurred while attempting to finalize the bucket", - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + if err := r.finalizeBucket(ctx, req, bucketResource); err != nil { + return r.SetDegradedCondition( ctx, req, bucketResource, + metav1.ConditionFalse, s3v1alpha1.DeletionFailure, - "Bucket deletion has failed", + fmt.Sprintf("Bucket %s deletion has failed", bucketResource.Spec.Name), err, ) } if ok := controllerutil.RemoveFinalizer(bucketResource, bucketFinalizer); !ok { - logger.Info( - "Failed to remove finalizer for bucketResource", - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), + r.SetProgressingCondition( + ctx, + req, + bucketResource, + metav1.ConditionFalse, + s3v1alpha1.InternalError, + fmt.Sprintf("Failed to remove finalizer for bucket %s", bucketResource.Spec.Name), ) return ctrl.Result{Requeue: true}, nil } @@ -72,14 +66,16 @@ func (r *BucketReconciler) handleDeletion( // your changes to the latest version and try again" which would re-trigger the reconciliation // if we try to update it again in the following operations if err := r.Update(ctx, bucketResource); err != nil { - logger.Error( + r.SetDegradedCondition( + ctx, + req, + bucketResource, + metav1.ConditionFalse, + s3v1alpha1.K8sApiError, + fmt.Sprintf("An error occured when removing finalizer from bucket %s", bucketResource.Spec.Name), err, - "Failed to remove finalizer for bucketResource", - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), ) + return ctrl.Result{}, err } @@ -89,9 +85,9 @@ func (r *BucketReconciler) handleDeletion( func (r *BucketReconciler) finalizeBucket( ctx context.Context, + req reconcile.Request, bucketResource *s3v1alpha1.Bucket, ) error { - logger := log.FromContext(ctx) s3Client, err := r.S3Instancehelper.GetS3ClientForRessource( ctx, @@ -102,7 +98,15 @@ func (r *BucketReconciler) finalizeBucket( bucketResource.Spec.S3InstanceRef, ) if err != nil { - logger.Error(err, "an error occurred while getting s3Client") + r.SetDegradedCondition( + ctx, + req, + bucketResource, + metav1.ConditionUnknown, + s3v1alpha1.Unreachable, + "Failed to generate s3client from instance", + err, + ) return err } if s3Client.GetConfig().BucketDeletionEnabled { diff --git a/internal/controller/bucket/reconcile.go b/internal/controller/bucket/reconcile.go index df7ba0e..52db48d 100644 --- a/internal/controller/bucket/reconcile.go +++ b/internal/controller/bucket/reconcile.go @@ -21,10 +21,9 @@ import ( "fmt" s3v1alpha1 "github.com/InseeFrLab/s3-operator/api/v1alpha1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" k8sapierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" @@ -60,28 +59,15 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // Let's just set the status as Unknown when no status are available if len(bucketResource.Status.Conditions) == 0 { - meta.SetStatusCondition( - &bucketResource.Status.Conditions, - metav1.Condition{ - Type: s3v1alpha1.ConditionReconciled, - Status: metav1.ConditionUnknown, - ObservedGeneration: bucketResource.Generation, - Reason: s3v1alpha1.Reconciling, - Message: "Starting reconciliation", - }, - ) - if err = r.Status().Update(ctx, bucketResource); err != nil { - logger.Error( - err, - "Failed to update bucketRessource status", - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) + _, err = r.SetProgressingCondition(ctx, + req, + bucketResource, + metav1.ConditionUnknown, + s3v1alpha1.Reconciling, + fmt.Sprintf("Newly discovered resource %s", bucketResource.Name)) + if err != nil { return ctrl.Result{}, err } - // Let's re-fetch the bucketResource Custom Resource after update the status // so that we have the latest state of the resource on the cluster and we will avoid // raise the issue "the object has been modified, please apply @@ -102,41 +88,44 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // Add finalizer for this CR if !controllerutil.ContainsFinalizer(bucketResource, bucketFinalizer) { - logger.Info("Adding finalizer to bucket resource", "bucketName", - bucketResource.Spec.Name, "NamespacedName", req.NamespacedName.String()) + r.SetProgressingCondition(ctx, + req, + bucketResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("Adding finalizer to bucket resource %s", bucketResource.Spec.Name)) if ok := controllerutil.AddFinalizer(bucketResource, bucketFinalizer); !ok { - logger.Error( - err, - "Failed to add finalizer into bucket resource", - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) + r.SetDegradedCondition(ctx, + req, + bucketResource, + metav1.ConditionFalse, + s3v1alpha1.InternalError, + fmt.Sprintf("Failed to add finalizer to bucket resource %s", bucketResource.Spec.Name), + err) return ctrl.Result{Requeue: true}, nil } if err := r.Update(ctx, bucketResource); err != nil { - logger.Error( - err, - "An error occurred when adding finalizer on bucketResource", - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) + r.SetDegradedCondition(ctx, + req, + bucketResource, + metav1.ConditionFalse, + s3v1alpha1.K8sApiError, + fmt.Sprintf("An error occurred when adding finalizer on bucket resource %s", bucketResource.Spec.Name), + err) + return ctrl.Result{}, err } if err := r.Get(ctx, req.NamespacedName, bucketResource); err != nil { - logger.Error( - err, - "Failed to re-fetch bucketResource", - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) + r.SetDegradedCondition(ctx, + req, + bucketResource, + metav1.ConditionFalse, + s3v1alpha1.K8sApiError, + fmt.Sprintf("Failed to re-fetch bucket resource %s", bucketResource.Spec.Name), + err) + return ctrl.Result{}, err } } @@ -144,13 +133,16 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // // Managing bucket deletion with a finalizer // // REF : https://sdk.operatorframework.io/docs/building-operators/golang/advanced-topics/#external-resources if bucketResource.GetDeletionTimestamp() != nil { - logger.Info("bucketResource have been marked for deletion", "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String()) + r.SetProgressingCondition(ctx, + req, + bucketResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("Bucket resource have been marked for deletion %s", bucketResource.Spec.Name)) return r.handleDeletion(ctx, req, bucketResource) } + r.SetProgressingCondition(ctx, req, bucketResource, metav1.ConditionTrue, s3v1alpha1.Reconciling, fmt.Sprintf("Starting reconciliation of bucket %s", bucketResource.Name)) return r.handleReconciliation(ctx, req, bucketResource) } @@ -160,7 +152,6 @@ func (r *BucketReconciler) handleReconciliation( req reconcile.Request, bucketResource *s3v1alpha1.Bucket, ) (reconcile.Result, error) { - logger := log.FromContext(ctx) s3Client, err := r.S3Instancehelper.GetS3ClientForRessource( ctx, @@ -171,11 +162,11 @@ func (r *BucketReconciler) handleReconciliation( bucketResource.Spec.S3InstanceRef, ) if err != nil { - logger.Error(err, "an error occurred while getting s3Client") - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, bucketResource, + metav1.ConditionUnknown, s3v1alpha1.Unreachable, "Failed to generate s3client from instance", err, @@ -186,20 +177,13 @@ func (r *BucketReconciler) handleReconciliation( // Check bucket existence on the S3 server found, err := s3Client.BucketExists(bucketResource.Spec.Name) if err != nil { - logger.Error( - err, - "An error occurred while checking the existence of a bucket", - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, bucketResource, + metav1.ConditionUnknown, s3v1alpha1.Unreachable, - "Error while checking if bucket already exist", + fmt.Sprintf("Error while checking if bucket %s already exist", bucketResource.Spec.Name), err, ) @@ -219,7 +203,6 @@ func (r *BucketReconciler) handleUpdate( req reconcile.Request, bucketResource *s3v1alpha1.Bucket, ) (reconcile.Result, error) { - logger := log.FromContext(ctx) s3Client, err := r.S3Instancehelper.GetS3ClientForRessource( ctx, @@ -230,18 +213,11 @@ func (r *BucketReconciler) handleUpdate( bucketResource.Spec.S3InstanceRef, ) if err != nil { - logger.Error( - err, - "An error occurred while getting s3Client for bucket ressource", - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, bucketResource, + metav1.ConditionUnknown, s3v1alpha1.Unreachable, "Failed to generate s3client from instance", err, @@ -254,20 +230,13 @@ func (r *BucketReconciler) handleUpdate( // Checking effectiveQuota existence on the bucket effectiveQuota, err := s3Client.GetQuota(bucketResource.Spec.Name) if err != nil { - logger.Error( - err, - "An error occurred while checking the quota for bucket ressource", - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, bucketResource, + metav1.ConditionUnknown, s3v1alpha1.Unreachable, - "Checking quota has failed", + fmt.Sprintf("Checking quota of bucket %s has failed", bucketResource.Spec.Name), err, ) } @@ -278,40 +247,26 @@ func (r *BucketReconciler) handleUpdate( // Choosing between override / default quotaToResetTo, convertionSucceed := bucketResource.Spec.Quota.Override.AsInt64() if !convertionSucceed { - logger.Error( - err, - "An error occurred while getting quotas override as int64 for ressource", - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, bucketResource, - s3v1alpha1.Unreachable, - "An error occurred while creating bucket", + metav1.ConditionFalse, + s3v1alpha1.InternalError, + fmt.Sprintf("An error occurred while getting overrided quotas as int64 for ressource %s", bucketResource.Spec.Name), err, ) } if quotaToResetTo == 0 { quotaToResetTo, convertionSucceed = bucketResource.Spec.Quota.Default.AsInt64() if !convertionSucceed { - logger.Error( - err, - "An error occurred while getting default quotas as int64 for ressource", - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, bucketResource, - s3v1alpha1.Unreachable, - "An error occurred while creating bucket", + metav1.ConditionFalse, + s3v1alpha1.InternalError, + fmt.Sprintf("An error occurred while getting default quotas as int64 for ressource %s", bucketResource.Spec.Name), err, ) } @@ -320,23 +275,18 @@ func (r *BucketReconciler) handleUpdate( if effectiveQuota != quotaToResetTo { err = s3Client.SetQuota(bucketResource.Spec.Name, quotaToResetTo) if err != nil { - logger.Error( - err, - "An error occurred while resetting the quota for bucket ressource", - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + + return r.SetDegradedCondition( ctx, req, bucketResource, + metav1.ConditionFalse, s3v1alpha1.Unreachable, fmt.Sprintf( - "The quota update (%v => %v) has failed", + "The quota update (%v => %v) has failed for bucket %s", effectiveQuota, quotaToResetTo, + bucketResource.Spec.Name, ), err, ) @@ -353,22 +303,14 @@ func (r *BucketReconciler) handleUpdate( for _, pathInCr := range bucketResource.Spec.Paths { pathExists, err := s3Client.PathExists(bucketResource.Spec.Name, pathInCr) if err != nil { - logger.Error( - err, - "An error occurred while checking a path's existence for bucket ressource", - "path", - pathInCr, - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + + return r.SetDegradedCondition( ctx, req, bucketResource, + metav1.ConditionFalse, s3v1alpha1.Unreachable, - fmt.Sprintf("The check for path [%s] in bucket has failed", pathInCr), + fmt.Sprintf("The check for path [%s] in bucket %s has failed", pathInCr, bucketResource.Spec.Name), err, ) } @@ -376,35 +318,25 @@ func (r *BucketReconciler) handleUpdate( if !pathExists { err = s3Client.CreatePath(bucketResource.Spec.Name, pathInCr) if err != nil { - logger.Error( - err, - "An error occurred while creating a path for bucket ressource", - "path", - pathInCr, - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, bucketResource, + metav1.ConditionFalse, s3v1alpha1.Unreachable, - fmt.Sprintf("The creation of path [%s] in bucket has failed", pathInCr), + fmt.Sprintf("The creation of path [%s] in bucket %s has failed", pathInCr, bucketResource.Spec.Name), err, ) } } } - return r.SetReconciledCondition( + return r.SetAvailableCondition( ctx, req, bucketResource, s3v1alpha1.Reconciled, "Bucket reconciled", - nil, ) } @@ -413,7 +345,6 @@ func (r *BucketReconciler) handleCreation( req reconcile.Request, bucketResource *s3v1alpha1.Bucket, ) (reconcile.Result, error) { - logger := log.FromContext(ctx) s3Client, err := r.S3Instancehelper.GetS3ClientForRessource( ctx, @@ -424,15 +355,7 @@ func (r *BucketReconciler) handleCreation( bucketResource.Spec.S3InstanceRef, ) if err != nil { - logger.Error( - err, - "An error occurred while getting s3Client for bucket ressource", - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetRejectedCondition( ctx, req, bucketResource, @@ -445,20 +368,13 @@ func (r *BucketReconciler) handleCreation( // Bucket creation err = s3Client.CreateBucket(bucketResource.Spec.Name) if err != nil { - logger.Error( - err, - "An error occurred while creating bucket ressource", - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + + return r.SetRejectedCondition( ctx, req, bucketResource, s3v1alpha1.CreationFailure, - "An error occurred while creating bucket", + fmt.Sprintf("An error occurred while creating bucket %s", bucketResource.Spec.Name), err, ) } @@ -466,37 +382,23 @@ func (r *BucketReconciler) handleCreation( // Setting quotas quotas, convertionSucceed := bucketResource.Spec.Quota.Default.AsInt64() if !convertionSucceed { - logger.Error( - err, - "An error occurred while getting quotas as int64 for ressource", - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, bucketResource, - s3v1alpha1.CreationFailure, - "An error occurred while creating bucket", + metav1.ConditionFalse, + s3v1alpha1.InternalError, + fmt.Sprintf("An error occurred while getting default quotas as int64 for ressource %s", bucketResource.Spec.Name), err, ) } err = s3Client.SetQuota(bucketResource.Spec.Name, quotas) if err != nil { - logger.Error( - err, - "An error occurred while setting quota for bucket ressource", - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, bucketResource, + metav1.ConditionFalse, s3v1alpha1.Unreachable, fmt.Sprintf( "Setting a quota of [%v] on bucket [%s] has failed", @@ -511,33 +413,23 @@ func (r *BucketReconciler) handleCreation( for _, pathInCr := range bucketResource.Spec.Paths { err = s3Client.CreatePath(bucketResource.Spec.Name, pathInCr) if err != nil { - logger.Error( - err, - "An error occurred while creating path for bucket ressource", - "path", - pathInCr, - "bucketName", - bucketResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, bucketResource, + metav1.ConditionFalse, s3v1alpha1.Unreachable, - fmt.Sprintf("Creation for path [%s] in bucket has failed", pathInCr), + fmt.Sprintf("Creation for path [%s] in bucket %s has failed", pathInCr, bucketResource.Spec.Name), err, ) } } - return r.SetReconciledCondition( + return r.SetAvailableCondition( ctx, req, bucketResource, s3v1alpha1.Reconciled, "Bucket reconciled", - nil, ) } diff --git a/internal/controller/bucket/status.go b/internal/controller/bucket/status.go index e4ff414..0820bac 100644 --- a/internal/controller/bucket/status.go +++ b/internal/controller/bucket/status.go @@ -19,12 +19,79 @@ package bucket_controller import ( "context" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" s3v1alpha1 "github.com/InseeFrLab/s3-operator/api/v1alpha1" ctrl "sigs.k8s.io/controller-runtime" ) -func (r *BucketReconciler) SetReconciledCondition( +func (r *BucketReconciler) SetProgressingCondition( + ctx context.Context, + req ctrl.Request, + bucketResource *s3v1alpha1.Bucket, + status metav1.ConditionStatus, + reason string, + message string, +) (ctrl.Result, error) { + return r.ControllerHelper.SetReconciledCondition( + ctx, + r.Status(), + req, + bucketResource, + &bucketResource.Status.Conditions, + s3v1alpha1.ConditionProgressing, + status, + reason, + message, + nil, + r.ReconcilePeriod, + ) +} +func (r *BucketReconciler) SetAvailableCondition( + ctx context.Context, + req ctrl.Request, + bucketResource *s3v1alpha1.Bucket, + reason string, + message string, +) (ctrl.Result, error) { + return r.ControllerHelper.SetReconciledCondition( + ctx, + r.Status(), + req, + bucketResource, + &bucketResource.Status.Conditions, + s3v1alpha1.ConditionAvailable, + metav1.ConditionTrue, + reason, + message, + nil, + r.ReconcilePeriod, + ) +} +func (r *BucketReconciler) SetDegradedCondition( + ctx context.Context, + req ctrl.Request, + bucketResource *s3v1alpha1.Bucket, + status metav1.ConditionStatus, + reason string, + message string, + err error, +) (ctrl.Result, error) { + return r.ControllerHelper.SetReconciledCondition( + ctx, + r.Status(), + req, + bucketResource, + &bucketResource.Status.Conditions, + s3v1alpha1.ConditionDegraded, + status, + reason, + message, + err, + r.ReconcilePeriod, + ) +} +func (r *BucketReconciler) SetRejectedCondition( ctx context.Context, req ctrl.Request, bucketResource *s3v1alpha1.Bucket, @@ -38,7 +105,8 @@ func (r *BucketReconciler) SetReconciledCondition( req, bucketResource, &bucketResource.Status.Conditions, - s3v1alpha1.ConditionReconciled, + s3v1alpha1.ConditionRejected, + metav1.ConditionFalse, reason, message, err, diff --git a/internal/controller/path/finalizer.go b/internal/controller/path/finalizer.go index 1f60c84..1cd5b21 100644 --- a/internal/controller/path/finalizer.go +++ b/internal/controller/path/finalizer.go @@ -20,9 +20,9 @@ import ( "context" "fmt" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" s3v1alpha1 "github.com/InseeFrLab/s3-operator/api/v1alpha1" @@ -33,24 +33,16 @@ func (r *PathReconciler) handleDeletion( req reconcile.Request, pathResource *s3v1alpha1.Path, ) (reconcile.Result, error) { - logger := log.FromContext(ctx) if controllerutil.ContainsFinalizer(pathResource, pathFinalizer) { - if err := r.finalizePath(ctx, pathResource); err != nil { - logger.Error( - err, - "An error occurred when attempting to finalize the path", - "path", - pathResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + if err := r.finalizePath(ctx, req, pathResource); err != nil { + return r.SetDegradedCondition( ctx, req, pathResource, + metav1.ConditionFalse, s3v1alpha1.DeletionFailure, - "Path deletion has failed", + fmt.Sprintf("Path %s deletion has failed", pathResource.Name), err, ) } @@ -59,12 +51,13 @@ func (r *PathReconciler) handleDeletion( // removed, the object will be deleted. if ok := controllerutil.RemoveFinalizer(pathResource, pathFinalizer); !ok { - logger.Info( - "Failed to remove finalizer for pathResource", - "pathResource", - pathResource.Name, - "NamespacedName", - req.NamespacedName.String(), + r.SetProgressingCondition( + ctx, + req, + pathResource, + metav1.ConditionFalse, + s3v1alpha1.InternalError, + fmt.Sprintf("Failed to remove finalizer for path %s", pathResource.Name), ) return ctrl.Result{Requeue: true}, nil } @@ -75,14 +68,16 @@ func (r *PathReconciler) handleDeletion( // your changes to the latest version and try again" which would re-trigger the reconciliation // if we try to update it again in the following operations if err := r.Update(ctx, pathResource); err != nil { - logger.Error( + r.SetDegradedCondition( + ctx, + req, + pathResource, + metav1.ConditionFalse, + s3v1alpha1.K8sApiError, + fmt.Sprintf("An error occured when removing finalizer from path %s", pathResource.Name), err, - "An error occurred when removing finalizer from pathResource", - "pathResource", - pathResource.Name, - "NamespacedName", - req.NamespacedName.String(), ) + return ctrl.Result{}, err } @@ -90,8 +85,7 @@ func (r *PathReconciler) handleDeletion( return ctrl.Result{}, nil } -func (r *PathReconciler) finalizePath(ctx context.Context, pathResource *s3v1alpha1.Path) error { - logger := log.FromContext(ctx) +func (r *PathReconciler) finalizePath(ctx context.Context, req reconcile.Request, pathResource *s3v1alpha1.Path) error { s3Client, err := r.S3Instancehelper.GetS3ClientForRessource( ctx, @@ -103,7 +97,15 @@ func (r *PathReconciler) finalizePath(ctx context.Context, pathResource *s3v1alp ) if err != nil { - logger.Error(err, "An error occurred while getting s3Client") + r.SetDegradedCondition( + ctx, + req, + pathResource, + metav1.ConditionUnknown, + s3v1alpha1.Unreachable, + "Failed to generate s3client from instance", + err, + ) return err } @@ -113,14 +115,16 @@ func (r *PathReconciler) finalizePath(ctx context.Context, pathResource *s3v1alp pathExists, err := s3Client.PathExists(pathResource.Spec.BucketName, path) if err != nil { - logger.Error( + r.SetDegradedCondition( + ctx, + req, + pathResource, + metav1.ConditionFalse, + s3v1alpha1.DeletionFailure, + fmt.Sprintf("An error occured while finalizing path %s, failed to check path's existence on bucket %s", pathResource.Name, pathResource.Spec.BucketName), err, - "finalize : an error occurred while checking a path's existence on a bucket", - "bucket", - pathResource.Spec.BucketName, - "path", - path, ) + } if pathExists { diff --git a/internal/controller/path/reconcile.go b/internal/controller/path/reconcile.go index e082e5f..baff6c0 100644 --- a/internal/controller/path/reconcile.go +++ b/internal/controller/path/reconcile.go @@ -21,7 +21,6 @@ import ( "fmt" k8sapierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -60,25 +59,13 @@ func (r *PathReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. // Let's just set the status as Unknown when no status are available if len(pathResource.Status.Conditions) == 0 { - meta.SetStatusCondition( - &pathResource.Status.Conditions, - metav1.Condition{ - Type: s3v1alpha1.ConditionReconciled, - Status: metav1.ConditionUnknown, - ObservedGeneration: pathResource.Generation, - Reason: s3v1alpha1.Reconciling, - Message: "Starting reconciliation", - }, - ) - if err = r.Status().Update(ctx, pathResource); err != nil { - logger.Error( - err, - "Failed to update pathResource status", - "pathResourceName", - pathResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) + _, err = r.SetProgressingCondition(ctx, + req, + pathResource, + metav1.ConditionUnknown, + s3v1alpha1.Reconciling, + fmt.Sprintf("Newly discovered resource %s", pathResource.Name)) + if err != nil { return ctrl.Result{}, err } @@ -88,48 +75,55 @@ func (r *PathReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. // your changes to the latest version and try again" which would re-trigger the reconciliation // if we try to update it again in the following operations if err := r.Get(ctx, req.NamespacedName, pathResource); err != nil { - logger.Error( - err, - "Failed to re-fetch pathResource", - "pathResourceName", - pathResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) + r.SetDegradedCondition(ctx, + req, + pathResource, + metav1.ConditionFalse, + s3v1alpha1.K8sApiError, + fmt.Sprintf("Failed to re-fetch path resource %s", pathResource.Name), + err) return ctrl.Result{}, err } } // Add finalizer for this CR if !controllerutil.ContainsFinalizer(pathResource, pathFinalizer) { - logger.Info("Adding finalizer to pathResource", "pathResourceName", - pathResource.Name, "NamespacedName", req.NamespacedName.String()) + r.SetProgressingCondition(ctx, + req, + pathResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("Adding finalizer to path resource %s", pathResource.Name)) if ok := controllerutil.AddFinalizer(pathResource, pathFinalizer); !ok { - logger.Error( - err, - "Failed to add finalizer into pathResource", - "pathResourceName", - pathResource.Name, "NamespacedName", req.NamespacedName.String()) + r.SetDegradedCondition(ctx, + req, + pathResource, + metav1.ConditionFalse, + s3v1alpha1.InternalError, + fmt.Sprintf("Failed to add finalizer to path resource %s", pathResource.Name), + err) return ctrl.Result{Requeue: true}, nil } if err := r.Update(ctx, pathResource); err != nil { - logger.Error( - err, - "an error occurred when adding finalizer on pathResource", - "pathResourceName", - pathResource.Name, "NamespacedName", req.NamespacedName.String(), - ) + r.SetDegradedCondition(ctx, + req, + pathResource, + metav1.ConditionFalse, + s3v1alpha1.K8sApiError, + fmt.Sprintf("An error occurred when adding finalizer on path resource %s", pathResource.Name), + err) return ctrl.Result{}, err } if err := r.Get(ctx, req.NamespacedName, pathResource); err != nil { - logger.Error( - err, - "Failed to re-fetch pathResource", - "pathResourceName", - pathResource.Name, "NamespacedName", req.NamespacedName.String(), - ) + r.SetDegradedCondition(ctx, + req, + pathResource, + metav1.ConditionFalse, + s3v1alpha1.K8sApiError, + fmt.Sprintf("Failed to re-fetch path resource %s", pathResource.Name), + err) return ctrl.Result{}, err } } @@ -137,13 +131,16 @@ func (r *PathReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. // Managing path deletion with a finalizer // REF : https://sdk.operatorframework.io/docs/building-operators/golang/advanced-topics/#external-resources if pathResource.GetDeletionTimestamp() != nil { - logger.Info("pathResource have been marked for deletion", "pathResourceName", - pathResource.Name, - "NamespacedName", - req.NamespacedName.String()) + r.SetProgressingCondition(ctx, + req, + pathResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("Path resource have been marked for deletion %s", pathResource.Name)) return r.handleDeletion(ctx, req, pathResource) } + r.SetProgressingCondition(ctx, req, pathResource, metav1.ConditionTrue, s3v1alpha1.Reconciling, fmt.Sprintf("Starting reconciliation of path %s", pathResource.Name)) return r.handleReconciliation(ctx, req, pathResource) } @@ -154,7 +151,6 @@ func (r *PathReconciler) handleReconciliation( pathResource *s3v1alpha1.Path, ) (reconcile.Result, error) { - logger := log.FromContext(ctx) // Create S3Client s3Client, err := r.S3Instancehelper.GetS3ClientForRessource( @@ -166,11 +162,11 @@ func (r *PathReconciler) handleReconciliation( pathResource.Spec.S3InstanceRef, ) if err != nil { - logger.Error(err, "an error occurred while getting s3Client") - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, pathResource, + metav1.ConditionUnknown, s3v1alpha1.Unreachable, "Failed to generate s3client from instance", err, @@ -182,36 +178,20 @@ func (r *PathReconciler) handleReconciliation( // Check bucket existence on the S3 server bucketFound, err := s3Client.BucketExists(pathResource.Spec.BucketName) if err != nil { - logger.Error( - err, - "an error occurred while checking the existence of a bucket", - "bucketName", - pathResource.Spec.BucketName, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, pathResource, + metav1.ConditionUnknown, s3v1alpha1.Unreachable, - "Error while checking if bucket already exist", + fmt.Sprintf("Error while checking if bucket %s already exist", pathResource.Spec.BucketName), err, ) } // If bucket does not exist, the Path CR should be in a failing state if !bucketFound { - errorBucketNotFound := fmt.Errorf( - "the path CR %s references a non-existing bucket : %s", - pathResource.Name, - pathResource.Spec.BucketName, - ) - logger.Error(errorBucketNotFound, errorBucketNotFound.Error(), "pathResourceName", - pathResource.Name, - "NamespacedName", - req.NamespacedName.String()) - return r.SetReconciledCondition( + return r.SetRejectedCondition( ctx, req, pathResource, @@ -236,22 +216,13 @@ func (r *PathReconciler) handleReconciliation( for _, pathInCr := range pathResource.Spec.Paths { pathExists, err := s3Client.PathExists(pathResource.Spec.BucketName, pathInCr) if err != nil { - logger.Error( - err, - "An error occurred while checking a path's existence for bucket ressource", - "path", - pathInCr, - "bucketName", - pathResource.Spec.BucketName, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, pathResource, + metav1.ConditionFalse, s3v1alpha1.Unreachable, - fmt.Sprintf("The check for path [%s] in bucket has failed", pathInCr), + fmt.Sprintf("The check for path [%s] in bucket %s has failed", pathInCr, pathResource.Spec.BucketName), err, ) } @@ -259,34 +230,23 @@ func (r *PathReconciler) handleReconciliation( if !pathExists { err = s3Client.CreatePath(pathResource.Spec.BucketName, pathInCr) if err != nil { - logger.Error( - err, - "An error occurred while creating a path for bucket ressource", - "path", - pathInCr, - "bucketName", - pathResource.Spec.BucketName, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetRejectedCondition( ctx, req, pathResource, s3v1alpha1.Unreachable, - fmt.Sprintf("The creation of path [%s] in bucket has failed", pathInCr), + fmt.Sprintf("The creation of path [%s] in bucket %s has failed", pathInCr, pathResource.Spec.BucketName), err, ) } } } - return r.SetReconciledCondition( + return r.SetAvailableCondition( ctx, req, pathResource, s3v1alpha1.Reconciled, "Path reconciled", - nil, ) } diff --git a/internal/controller/path/status.go b/internal/controller/path/status.go index 2456a8c..19d4262 100644 --- a/internal/controller/path/status.go +++ b/internal/controller/path/status.go @@ -19,15 +19,60 @@ package path_controller import ( "context" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" s3v1alpha1 "github.com/InseeFrLab/s3-operator/api/v1alpha1" ) -func (r *PathReconciler) SetReconciledCondition( +func (r *PathReconciler) SetProgressingCondition( ctx context.Context, req ctrl.Request, pathResource *s3v1alpha1.Path, + status metav1.ConditionStatus, + reason string, + message string, +) (ctrl.Result, error) { + return r.ControllerHelper.SetReconciledCondition( + ctx, + r.Status(), + req, + pathResource, + &pathResource.Status.Conditions, + s3v1alpha1.ConditionProgressing, + status, + reason, + message, + nil, + r.ReconcilePeriod, + ) +} +func (r *PathReconciler) SetAvailableCondition( + ctx context.Context, + req ctrl.Request, + pathResource *s3v1alpha1.Path, + reason string, + message string, +) (ctrl.Result, error) { + return r.ControllerHelper.SetReconciledCondition( + ctx, + r.Status(), + req, + pathResource, + &pathResource.Status.Conditions, + s3v1alpha1.ConditionAvailable, + metav1.ConditionTrue, + reason, + message, + nil, + r.ReconcilePeriod, + ) +} +func (r *PathReconciler) SetDegradedCondition( + ctx context.Context, + req ctrl.Request, + pathResource *s3v1alpha1.Path, + status metav1.ConditionStatus, reason string, message string, err error, @@ -38,11 +83,33 @@ func (r *PathReconciler) SetReconciledCondition( req, pathResource, &pathResource.Status.Conditions, - s3v1alpha1.ConditionReconciled, + s3v1alpha1.ConditionDegraded, + status, + reason, + message, + err, + r.ReconcilePeriod, + ) +} +func (r *PathReconciler) SetRejectedCondition( + ctx context.Context, + req ctrl.Request, + pathResource *s3v1alpha1.Path, + reason string, + message string, + err error, +) (ctrl.Result, error) { + return r.ControllerHelper.SetReconciledCondition( + ctx, + r.Status(), + req, + pathResource, + &pathResource.Status.Conditions, + s3v1alpha1.ConditionRejected, + metav1.ConditionFalse, reason, message, err, r.ReconcilePeriod, ) - } diff --git a/internal/controller/policy/finalizer.go b/internal/controller/policy/finalizer.go index 2adb995..6bee2ff 100644 --- a/internal/controller/policy/finalizer.go +++ b/internal/controller/policy/finalizer.go @@ -18,10 +18,11 @@ package policy_controller import ( "context" + "fmt" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" s3v1alpha1 "github.com/InseeFrLab/s3-operator/api/v1alpha1" @@ -29,9 +30,9 @@ import ( func (r *PolicyReconciler) finalizePolicy( ctx context.Context, + req reconcile.Request, policyResource *s3v1alpha1.Policy, ) error { - logger := log.FromContext(ctx) s3Client, err := r.S3Instancehelper.GetS3ClientForRessource( ctx, r.Client, @@ -41,7 +42,15 @@ func (r *PolicyReconciler) finalizePolicy( policyResource.Spec.S3InstanceRef, ) if err != nil { - logger.Error(err, "an error occurred while getting s3Client") + r.SetDegradedCondition( + ctx, + req, + policyResource, + metav1.ConditionUnknown, + s3v1alpha1.Unreachable, + "Failed to generate s3client from instance", + err, + ) return err } if s3Client.GetConfig().PolicyDeletionEnabled { @@ -55,49 +64,43 @@ func (r *PolicyReconciler) handleDeletion( req reconcile.Request, policyResource *s3v1alpha1.Policy, ) (reconcile.Result, error) { - logger := log.FromContext(ctx) if controllerutil.ContainsFinalizer(policyResource, policyFinalizer) { // Run finalization logic for policyFinalizer. If the // finalization logic fails, don't remove the finalizer so // that we can retry during the next reconciliation. - if err := r.finalizePolicy(ctx, policyResource); err != nil { - logger.Error( - err, - "An error occurred when attempting to finalize the policy", - "policyName", - policyResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + if err := r.finalizePolicy(ctx, req, policyResource); err != nil { + return r.SetDegradedCondition( ctx, req, policyResource, + metav1.ConditionFalse, s3v1alpha1.DeletionFailure, - "Policy deletion has failed", + fmt.Sprintf("Policy %s deletion has failed", policyResource.Spec.Name), err, ) } if ok := controllerutil.RemoveFinalizer(policyResource, policyFinalizer); !ok { - logger.Info( - "Failed to remove finalizer for policyResource", - "policyName", - policyResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), + r.SetProgressingCondition( + ctx, + req, + policyResource, + metav1.ConditionFalse, + s3v1alpha1.InternalError, + fmt.Sprintf("Failed to remove finalizer for policy %s", policyResource.Spec.Name), ) return ctrl.Result{Requeue: true}, nil } if err := r.Update(ctx, policyResource); err != nil { - logger.Error( + r.SetDegradedCondition( + ctx, + req, + policyResource, + metav1.ConditionFalse, + s3v1alpha1.K8sApiError, + fmt.Sprintf("An error occured when removing finalizer from policy %s", policyResource.Spec.Name), err, - "an error occurred when removing finalizer from policy", - "policyName", - policyResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), ) return ctrl.Result{}, err } diff --git a/internal/controller/policy/reconcile.go b/internal/controller/policy/reconcile.go index b01644c..a43ef12 100644 --- a/internal/controller/policy/reconcile.go +++ b/internal/controller/policy/reconcile.go @@ -23,7 +23,6 @@ import ( "fmt" k8sapierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -63,25 +62,13 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // Let's just set the status as Unknown when no status are available if len(policyResource.Status.Conditions) == 0 { - meta.SetStatusCondition( - &policyResource.Status.Conditions, - metav1.Condition{ - Type: s3v1alpha1.ConditionReconciled, - Status: metav1.ConditionUnknown, - ObservedGeneration: policyResource.Generation, - Reason: s3v1alpha1.Reconciling, - Message: "Starting reconciliation", - }, - ) - if err = r.Status().Update(ctx, policyResource); err != nil { - logger.Error( - err, - "Failed to update bucketRessource status", - "bucketName", - policyResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) + _, err = r.SetProgressingCondition(ctx, + req, + policyResource, + metav1.ConditionUnknown, + s3v1alpha1.Reconciling, + fmt.Sprintf("Newly discovered resource %s", policyResource.Name)) + if err != nil { return ctrl.Result{}, err } @@ -91,61 +78,60 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // your changes to the latest version and try again" which would re-trigger the reconciliation // if we try to update it again in the following operations if err := r.Get(ctx, req.NamespacedName, policyResource); err != nil { - logger.Error( - err, - "Failed to re-fetch policyResource", - "policyName", - policyResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) + + r.SetDegradedCondition(ctx, + req, + policyResource, + metav1.ConditionFalse, + s3v1alpha1.K8sApiError, + fmt.Sprintf("Failed to re-fetch policy resource %s", policyResource.Name), + err) return ctrl.Result{}, err } } // Add finalizer for this CR if !controllerutil.ContainsFinalizer(policyResource, policyFinalizer) { - logger.Info("Adding finalizer to policy resource", "PolicyName", - policyResource.Spec.Name, "NamespacedName", req.NamespacedName.String()) + r.SetProgressingCondition(ctx, + req, + policyResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("Adding finalizer to policy resource %s", policyResource.Name)) if ok := controllerutil.AddFinalizer(policyResource, policyFinalizer); !ok { - logger.Error( - err, - "Failed to add finalizer into policy resource", - "policyName", - policyResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) + r.SetDegradedCondition(ctx, + req, + policyResource, + metav1.ConditionFalse, + s3v1alpha1.InternalError, + fmt.Sprintf("Failed to add finalizer to policy resource %s", policyResource.Name), + err) return ctrl.Result{Requeue: true}, nil } - err = r.Update(ctx, policyResource) - if err != nil { - logger.Error( - err, - "An error occurred when adding finalizer from policyResource", - "policyResource", - policyResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) + if err := r.Update(ctx, policyResource); err != nil { + r.SetDegradedCondition(ctx, + req, + policyResource, + metav1.ConditionFalse, + s3v1alpha1.K8sApiError, + fmt.Sprintf("An error occurred when adding finalizer on policy resource %s", policyResource.Name), + err) return ctrl.Result{}, err } - // Let's re-fetch the policy Custom Resource after adding the finalizer // so that we have the latest state of the resource on the cluster and we will avoid // raise the issue "the object has been modified, please apply // your changes to the latest version and try again" which would re-trigger the reconciliation // if we try to update it again in the following operations if err := r.Get(ctx, req.NamespacedName, policyResource); err != nil { - logger.Error( - err, - "Failed to re-fetch policyResource", - "policyName", - policyResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) + r.SetDegradedCondition(ctx, + req, + policyResource, + metav1.ConditionFalse, + s3v1alpha1.K8sApiError, + fmt.Sprintf("Failed to re-fetch policy resource %s", policyResource.Name), + err) return ctrl.Result{}, err } } @@ -153,16 +139,18 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // Managing policy deletion with a finalizer // REF : https://sdk.operatorframework.io/docs/building-operators/golang/advanced-topics/#external-resources if policyResource.GetDeletionTimestamp() != nil { - logger.Info("policyResource have been marked for deletion", "policyName", - policyResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String()) + r.SetProgressingCondition(ctx, + req, + policyResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("policy resource have been marked for deletion %s", policyResource.Name)) return r.handleDeletion(ctx, req, policyResource) } // Policy lifecycle management (other than deletion) starts here + r.SetProgressingCondition(ctx, req, policyResource, metav1.ConditionTrue, s3v1alpha1.Reconciling, fmt.Sprintf("Starting reconciliation of policy %s", policyResource.Name)) return r.handleReconciliation(ctx, req, policyResource) - } func (r *PolicyReconciler) handleReconciliation( @@ -170,8 +158,9 @@ func (r *PolicyReconciler) handleReconciliation( req reconcile.Request, policyResource *s3v1alpha1.Policy, ) (reconcile.Result, error) { - logger := log.FromContext(ctx) + + // Create S3Client s3Client, err := r.S3Instancehelper.GetS3ClientForRessource( ctx, r.Client, @@ -181,34 +170,27 @@ func (r *PolicyReconciler) handleReconciliation( policyResource.Spec.S3InstanceRef, ) if err != nil { - logger.Error(err, "an error occurred while getting s3Client") - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, policyResource, + metav1.ConditionUnknown, s3v1alpha1.Unreachable, "Failed to generate s3client from instance", err, ) } - // Check policy existence on the S3 server + // check policy existence on the s3 server effectivePolicy, err := s3Client.GetPolicyInfo(policyResource.Spec.Name) if err != nil { - logger.Error( - err, - "An error occurred while checking the existence of a policy", - "policyName", - policyResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, policyResource, + metav1.ConditionUnknown, s3v1alpha1.Unreachable, - "Error while checking if policy already exist", + fmt.Sprintf("Error while checking if policy %s already exist", policyResource.Spec.Name), err, ) } @@ -226,7 +208,6 @@ func (r *PolicyReconciler) handleUpdate( req reconcile.Request, policyResource *s3v1alpha1.Policy, ) (reconcile.Result, error) { - logger := log.FromContext(ctx) s3Client, err := r.S3Instancehelper.GetS3ClientForRessource( ctx, @@ -238,11 +219,11 @@ func (r *PolicyReconciler) handleUpdate( ) if err != nil { - logger.Error(err, "an error occurred while getting s3Client") - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, policyResource, + metav1.ConditionUnknown, s3v1alpha1.Unreachable, "Failed to generate s3client from instance", err, @@ -252,42 +233,29 @@ func (r *PolicyReconciler) handleUpdate( // Check policy existence on the S3 server effectivePolicy, err := s3Client.GetPolicyInfo(policyResource.Spec.Name) if err != nil { - logger.Error( - err, - "An error occurred while checking the existence of a policy", - "policyName", - policyResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, policyResource, + metav1.ConditionUnknown, s3v1alpha1.Unreachable, - "Error while checking if policy already exist", + fmt.Sprintf("Error while checking if policy %s already exist", policyResource.Spec.Name), err, ) } matching, err := r.isPolicyMatchingWithCustomResource(policyResource, effectivePolicy) if err != nil { - logger.Error( - err, - "An error occurred while comparing actual and expected configuration for the policy", - "policyName", - policyResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, policyResource, + metav1.ConditionFalse, s3v1alpha1.Unreachable, fmt.Sprintf( - "The comparison between the effective policy [%s] on S3 and its corresponding custom resource on K8S has failed", + "The comparison between the effective policy [%s] on S3 and its corresponding custom resource %s on K8S has failed", policyResource.Spec.Name, + policyResource.Name, ), err, ) @@ -300,41 +268,33 @@ func (r *PolicyReconciler) handleUpdate( policyResource.Spec.PolicyContent, ) if err != nil { - logger.Error( - err, - "An error occurred while updating the policy", - "policyName", - policyResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - r.SetReconciledCondition( + r.SetDegradedCondition( ctx, req, policyResource, + metav1.ConditionFalse, s3v1alpha1.Unreachable, fmt.Sprintf( - "The comparison between the effective policy [%s] on S3 and its corresponding custom resource on K8S has failed", + "The comparison between the effective policy [%s] on S3 and its corresponding custom resource %s on K8S has failed", policyResource.Spec.Name, + policyResource.Name, ), err, ) } } - return r.SetReconciledCondition( + return r.SetAvailableCondition( ctx, req, policyResource, s3v1alpha1.Reconciled, "Policy reconciled", - nil, ) } func (r *PolicyReconciler) handleCreation(ctx context.Context, req reconcile.Request, policyResource *s3v1alpha1.Policy) (reconcile.Result, error) { - logger := log.FromContext(ctx) s3Client, err := r.S3Instancehelper.GetS3ClientForRessource( ctx, @@ -346,8 +306,7 @@ func (r *PolicyReconciler) handleCreation(ctx context.Context, req reconcile.Req ) if err != nil { - logger.Error(err, "An error occurred while getting s3Client") - return r.SetReconciledCondition( + return r.SetRejectedCondition( ctx, req, policyResource, @@ -363,15 +322,8 @@ func (r *PolicyReconciler) handleCreation(ctx context.Context, req reconcile.Req ) if err != nil { - logger.Error( - err, - "An error occurred while creating the policy", - "policyName", - policyResource.Spec.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + + return r.SetRejectedCondition( ctx, req, policyResource, @@ -381,13 +333,12 @@ func (r *PolicyReconciler) handleCreation(ctx context.Context, req reconcile.Req ) } - return r.SetReconciledCondition( + return r.SetAvailableCondition( ctx, req, policyResource, s3v1alpha1.Reconciled, "Policy reconciled", - err, ) } diff --git a/internal/controller/policy/status.go b/internal/controller/policy/status.go index ba780b7..8bf8b93 100644 --- a/internal/controller/policy/status.go +++ b/internal/controller/policy/status.go @@ -19,12 +19,79 @@ package policy_controller import ( "context" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" s3v1alpha1 "github.com/InseeFrLab/s3-operator/api/v1alpha1" ) -func (r *PolicyReconciler) SetReconciledCondition( +func (r *PolicyReconciler) SetProgressingCondition( + ctx context.Context, + req ctrl.Request, + policyResource *s3v1alpha1.Policy, + status metav1.ConditionStatus, + reason string, + message string, +) (ctrl.Result, error) { + return r.ControllerHelper.SetReconciledCondition( + ctx, + r.Status(), + req, + policyResource, + &policyResource.Status.Conditions, + s3v1alpha1.ConditionProgressing, + status, + reason, + message, + nil, + r.ReconcilePeriod, + ) +} +func (r *PolicyReconciler) SetAvailableCondition( + ctx context.Context, + req ctrl.Request, + policyResource *s3v1alpha1.Policy, + reason string, + message string, +) (ctrl.Result, error) { + return r.ControllerHelper.SetReconciledCondition( + ctx, + r.Status(), + req, + policyResource, + &policyResource.Status.Conditions, + s3v1alpha1.ConditionAvailable, + metav1.ConditionTrue, + reason, + message, + nil, + r.ReconcilePeriod, + ) +} +func (r *PolicyReconciler) SetDegradedCondition( + ctx context.Context, + req ctrl.Request, + policyResource *s3v1alpha1.Policy, + status metav1.ConditionStatus, + reason string, + message string, + err error, +) (ctrl.Result, error) { + return r.ControllerHelper.SetReconciledCondition( + ctx, + r.Status(), + req, + policyResource, + &policyResource.Status.Conditions, + s3v1alpha1.ConditionDegraded, + status, + reason, + message, + err, + r.ReconcilePeriod, + ) +} +func (r *PolicyReconciler) SetRejectedCondition( ctx context.Context, req ctrl.Request, policyResource *s3v1alpha1.Policy, @@ -38,7 +105,8 @@ func (r *PolicyReconciler) SetReconciledCondition( req, policyResource, &policyResource.Status.Conditions, - s3v1alpha1.ConditionReconciled, + s3v1alpha1.ConditionRejected, + metav1.ConditionFalse, reason, message, err, diff --git a/internal/controller/s3instance/finalizer.go b/internal/controller/s3instance/finalizer.go index 2a5dae4..619c35b 100644 --- a/internal/controller/s3instance/finalizer.go +++ b/internal/controller/s3instance/finalizer.go @@ -18,12 +18,13 @@ package s3instance_controller import ( "context" + "strings" "fmt" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" s3v1alpha1 "github.com/InseeFrLab/s3-operator/api/v1alpha1" @@ -34,28 +35,40 @@ func (r *S3InstanceReconciler) handleS3InstanceDeletion( req ctrl.Request, s3InstanceResource *s3v1alpha1.S3Instance, ) (reconcile.Result, error) { - logger := log.FromContext(ctx) if controllerutil.ContainsFinalizer(s3InstanceResource, s3InstanceFinalizer) { - logger.Info( - "Performing Finalizer Operations for S3Instance before delete CR", - "Namespace", - s3InstanceResource.GetNamespace(), - "Name", - s3InstanceResource.GetName(), + r.SetProgressingCondition( + ctx, + req, + s3InstanceResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("Performing Finalizer Operations for S3Instance %s before delete CR", s3InstanceResource.GetName()), ) // Vérifier les références existantes if err := r.checkS3InstanceReferences(ctx, s3InstanceResource); err != nil { + r.SetDegradedCondition( + ctx, + req, + s3InstanceResource, + metav1.ConditionFalse, + s3v1alpha1.DeletionBlocked, + fmt.Sprintf("S3Instance %s is still referenced by other resources", s3InstanceResource.Name), + err, + ) return ctrl.Result{}, err } //Remove s3InstanceFinalizer. Once all finalizers have been removed, the object will be deleted. if ok := controllerutil.RemoveFinalizer(s3InstanceResource, s3InstanceFinalizer); !ok { - logger.Info( - "Failed to remove finalizer for S3Instance", - "NamespacedName", - req.NamespacedName.String(), + r.SetProgressingCondition( + ctx, + req, + s3InstanceResource, + metav1.ConditionFalse, + s3v1alpha1.InternalError, + fmt.Sprintf("Failed to remove finalizer for S3Instance %s", s3InstanceResource.Name), ) return ctrl.Result{Requeue: true}, nil } @@ -66,11 +79,14 @@ func (r *S3InstanceReconciler) handleS3InstanceDeletion( // your changes to the latest version and try again" which would re-trigger the reconciliation // if we try to update it again in the following operations if err := r.Update(ctx, s3InstanceResource); err != nil { - logger.Error( + r.SetDegradedCondition( + ctx, + req, + s3InstanceResource, + metav1.ConditionFalse, + s3v1alpha1.K8sApiError, + fmt.Sprintf("An error occured when removing finalizer from S3Instance %s", s3InstanceResource.Name), err, - "Failed to remove finalizer for S3Instance", - "NamespacedName", - req.NamespacedName.String(), ) return ctrl.Result{}, err } @@ -78,25 +94,28 @@ func (r *S3InstanceReconciler) handleS3InstanceDeletion( return ctrl.Result{}, nil } -// checkS3InstanceReferences vérifie si l'instance S3 est encore utilisée +// checkS3InstanceReferences make sure the instance is not used anymore func (r *S3InstanceReconciler) checkS3InstanceReferences(ctx context.Context, s3Instance *s3v1alpha1.S3Instance) error { - // Liste des types de ressources à vérifier + // CR to be checked for existence references := map[string]client.ObjectList{ "Buckets": &s3v1alpha1.BucketList{}, "Policies": &s3v1alpha1.PolicyList{}, "Paths": &s3v1alpha1.PathList{}, "S3Users": &s3v1alpha1.S3UserList{}, } - + var errors []string for name, list := range references { if err := r.List(ctx, list); err != nil { - return fmt.Errorf("échec de la récupération des %s : %w", name, err) + return fmt.Errorf("Failed to retrieve %s : %w", name, err) } if found := r.countReferences(list, s3Instance); found > 0 { - return fmt.Errorf("impossible de supprimer S3Instance, %d %s utilisent cette instance", found, name) + errors = append(errors, fmt.Sprintf("Cannot delete s3Instance as %d %s are used on this instance", found, name)) } } + if len(errors) >0 { + return fmt.Errorf(strings.Join(errors, "\n")) + } return nil } diff --git a/internal/controller/s3instance/finalizer_test.go b/internal/controller/s3instance/finalizer_test.go index 3ef63d0..87a1676 100644 --- a/internal/controller/s3instance/finalizer_test.go +++ b/internal/controller/s3instance/finalizer_test.go @@ -147,7 +147,7 @@ func TestHandleDelete(t *testing.T) { req := ctrl.Request{NamespacedName: types.NamespacedName{Name: s3instanceResource.Name, Namespace: s3instanceResource.Namespace}} _, err := reconciler.Reconcile(context.TODO(), req) assert.Error(t, err) - assert.EqualErrorf(t, err, "impossible de supprimer S3Instance, 1 Buckets utilisent cette instance", err.Error()) + assert.EqualErrorf(t, err, "Cannot delete s3Instance as 1 Buckets are used on this instance", err.Error()) }) t.Run("error if one policy ressource still use it", func(t *testing.T) { @@ -208,7 +208,7 @@ func TestHandleDelete(t *testing.T) { req := ctrl.Request{NamespacedName: types.NamespacedName{Name: s3instanceResource.Name, Namespace: s3instanceResource.Namespace}} _, err := reconciler.Reconcile(context.TODO(), req) assert.Error(t, err) - assert.EqualErrorf(t, err, "impossible de supprimer S3Instance, 1 Policies utilisent cette instance", err.Error()) + assert.EqualErrorf(t, err, "Cannot delete s3Instance as 1 Policies are used on this instance", err.Error()) }) t.Run("error if one path ressource still use it", func(t *testing.T) { @@ -269,7 +269,7 @@ func TestHandleDelete(t *testing.T) { req := ctrl.Request{NamespacedName: types.NamespacedName{Name: s3instanceResource.Name, Namespace: s3instanceResource.Namespace}} _, err := reconciler.Reconcile(context.TODO(), req) assert.Error(t, err) - assert.EqualErrorf(t, err, "impossible de supprimer S3Instance, 1 Paths utilisent cette instance", err.Error()) + assert.EqualErrorf(t, err, "Cannot delete s3Instance as 1 Paths are used on this instance", err.Error()) }) t.Run("error if one user ressource still use it", func(t *testing.T) { @@ -330,6 +330,6 @@ func TestHandleDelete(t *testing.T) { req := ctrl.Request{NamespacedName: types.NamespacedName{Name: s3instanceResource.Name, Namespace: s3instanceResource.Namespace}} _, err := reconciler.Reconcile(context.TODO(), req) assert.Error(t, err) - assert.EqualErrorf(t, err, "impossible de supprimer S3Instance, 1 S3Users utilisent cette instance", err.Error()) + assert.EqualErrorf(t, err, "Cannot delete s3Instance as 1 S3Users are used on this instance", err.Error()) }) } diff --git a/internal/controller/s3instance/reconcile.go b/internal/controller/s3instance/reconcile.go index 548cf3e..cefdafb 100644 --- a/internal/controller/s3instance/reconcile.go +++ b/internal/controller/s3instance/reconcile.go @@ -21,7 +21,6 @@ import ( "fmt" k8sapierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -63,94 +62,62 @@ func (r *S3InstanceReconciler) Reconcile( return ctrl.Result{}, err } - // Let's just set the status as Unknown when no status are available - if len(s3InstanceResource.Status.Conditions) == 0 { - meta.SetStatusCondition( - &s3InstanceResource.Status.Conditions, - metav1.Condition{ - Type: s3v1alpha1.ConditionReconciled, - Status: metav1.ConditionUnknown, - ObservedGeneration: s3InstanceResource.Generation, - Reason: s3v1alpha1.Reconciling, - Message: "Starting reconciliation", - }, - ) - if err = r.Status().Update(ctx, s3InstanceResource); err != nil { - logger.Error( - err, - "Failed to update s3InstanceResource status", - "NamespacedName", - req.NamespacedName.String(), - ) - return ctrl.Result{}, err - } - - // Let's re-fetch the s3InstanceResource Custom Resource after update the status - // so that we have the latest state of the resource on the cluster and we will avoid - // raise the issue "the object has been modified, please apply - // your changes to the latest version and try again" which would re-trigger the reconciliation - // if we try to update it again in the following operations - if err := r.Get(ctx, req.NamespacedName, s3InstanceResource); err != nil { - logger.Error( - err, - "Failed to re-fetch s3Instance", - "NamespacedName", - req.NamespacedName.String(), - ) - return ctrl.Result{}, err - } - } - // Add finalizer for this CR if !controllerutil.ContainsFinalizer(s3InstanceResource, s3InstanceFinalizer) { - logger.Info("Adding finalizer to s3Instance", "NamespacedName", req.NamespacedName.String()) + r.SetProgressingCondition(ctx, + req, + s3InstanceResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("Adding finalizer to s3Instance resource %s", s3InstanceResource.Name)) if ok := controllerutil.AddFinalizer(s3InstanceResource, s3InstanceFinalizer); !ok { - logger.Error( - err, - "Failed to add finalizer into the s3Instance", - "NamespacedName", - req.NamespacedName.String(), - ) + r.SetDegradedCondition(ctx, + req, + s3InstanceResource, + metav1.ConditionFalse, + s3v1alpha1.InternalError, + fmt.Sprintf("Failed to add finalizer to s3Instance resource %s", s3InstanceResource.Name), + err) return ctrl.Result{Requeue: true}, nil } - if err = r.Update(ctx, s3InstanceResource); err != nil { - logger.Error( - err, - "an error occurred when adding finalizer on s3Instance", - "s3Instance", - s3InstanceResource.Name, - ) + if err := r.Update(ctx, s3InstanceResource); err != nil { + r.SetDegradedCondition(ctx, + req, + s3InstanceResource, + metav1.ConditionFalse, + s3v1alpha1.K8sApiError, + fmt.Sprintf("An error occurred when adding finalizer on s3Instance resource %s", s3InstanceResource.Name), + err) return ctrl.Result{}, err } - // Let's re-fetch the S3Instance Custom Resource after adding the finalizer - // so that we have the latest state of the resource on the cluster and we will avoid - // raise the issue "the object has been modified, please apply - // your changes to the latest version and try again" which would re-trigger the reconciliation - // if we try to update it again in the following operations if err := r.Get(ctx, req.NamespacedName, s3InstanceResource); err != nil { - logger.Error( - err, - "Failed to re-fetch s3Instance", - "NamespacedName", - req.NamespacedName.String(), - ) + r.SetDegradedCondition(ctx, + req, + s3InstanceResource, + metav1.ConditionFalse, + s3v1alpha1.K8sApiError, + fmt.Sprintf("Failed to re-fetch s3Instance resource %s", s3InstanceResource.Name), + err) return ctrl.Result{}, err } - } - // Check if the s3InstanceResource instance is marked to be deleted, which is // indicated by the deletion timestamp being set. The object will be deleted. if s3InstanceResource.GetDeletionTimestamp() != nil { - logger.Info("s3InstanceResource have been marked for deletion") + r.SetProgressingCondition(ctx, + req, + s3InstanceResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("S3Instance resource have been marked for deletion %s", s3InstanceResource.Name)) return r.handleS3InstanceDeletion(ctx, req, s3InstanceResource) } // Reconciliation starts here + r.SetProgressingCondition(ctx, req, s3InstanceResource, metav1.ConditionTrue, s3v1alpha1.Reconciling, fmt.Sprintf("Starting reconciliation of s3Instance %s", s3InstanceResource.Name)) return r.handleReconciliation(ctx, req, s3InstanceResource) - } func (r *S3InstanceReconciler) handleReconciliation( @@ -158,44 +125,26 @@ func (r *S3InstanceReconciler) handleReconciliation( req reconcile.Request, s3InstanceResource *s3v1alpha1.S3Instance, ) (reconcile.Result, error) { - logger := log.FromContext(ctx) s3Client, err := r.S3Instancehelper.GetS3ClientFromS3Instance(ctx, r.Client, r.S3factory, s3InstanceResource) if err != nil { - logger.Error( - err, - "Could not generate s3Instance", - "s3InstanceSecretRefName", - s3InstanceResource.Spec.SecretRef, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition(ctx, req, s3InstanceResource, s3v1alpha1.Unreachable, - "Failed to generate S3Instance ", err) + return r.SetDegradedCondition(ctx, req, s3InstanceResource, metav1.ConditionFalse, s3v1alpha1.Unreachable, + fmt.Sprintf("Failed to generate S3Instance %s using secret %s", s3InstanceResource.Name, s3InstanceResource.Spec.SecretRef), err) } _, err = s3Client.ListBuckets() if err != nil { - logger.Error( - err, - "Could not generate s3Instance", - "s3InstanceName", - s3InstanceResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition(ctx, req, s3InstanceResource, s3v1alpha1.CreationFailure, - "Failed to generate S3Instance ", err) + return r.SetDegradedCondition(ctx, req, s3InstanceResource, metav1.ConditionFalse, s3v1alpha1.CreationFailure, + fmt.Sprintf("Failed to generate S3Instance %s, listing buckets failed, using secret %s", s3InstanceResource.Name, s3InstanceResource.Spec.SecretRef), err) } - return r.SetReconciledCondition( + return r.SetAvailableCondition( ctx, req, s3InstanceResource, s3v1alpha1.Reconciled, "S3Instance instance reconciled", - nil, ) } diff --git a/internal/controller/s3instance/reconcile_test.go b/internal/controller/s3instance/reconcile_test.go index 94ee103..a7df4bd 100644 --- a/internal/controller/s3instance/reconcile_test.go +++ b/internal/controller/s3instance/reconcile_test.go @@ -134,8 +134,10 @@ func TestHandleCreate(t *testing.T) { Namespace: "s3-operator", Name: "default", }, reconciledInstance) - - assert.Equal(t, "Reconciled", reconciledInstance.Status.Conditions[0].Reason) + // Conditions[0] = Progressing ; Conditions[1] = Reconciled + var conditions = reconciledInstance.Status.Conditions + var lastCondition = conditions[len(conditions) - 1] + assert.Equal(t, s3v1alpha1.Reconciled, lastCondition.Reason) }) t.Run("reason is creation failure because of invalid client", func(t *testing.T) { @@ -150,7 +152,9 @@ func TestHandleCreate(t *testing.T) { Namespace: "s3-operator", Name: "invalid-instance", }, reconciledInstance) - - assert.Equal(t, "CreationFailure", reconciledInstance.Status.Conditions[0].Reason) + // Conditions[0] = Progressing ; Conditions[1] = CreationFailure + var conditions = reconciledInstance.Status.Conditions + var lastCondition = conditions[len(conditions) - 1] + assert.Equal(t, s3v1alpha1.CreationFailure, lastCondition.Reason) }) } diff --git a/internal/controller/s3instance/status.go b/internal/controller/s3instance/status.go index ad82e68..a5b1560 100644 --- a/internal/controller/s3instance/status.go +++ b/internal/controller/s3instance/status.go @@ -19,12 +19,79 @@ package s3instance_controller import ( "context" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" s3v1alpha1 "github.com/InseeFrLab/s3-operator/api/v1alpha1" ) -func (r *S3InstanceReconciler) SetReconciledCondition( +func (r *S3InstanceReconciler) SetProgressingCondition( + ctx context.Context, + req ctrl.Request, + s3InstanceResource *s3v1alpha1.S3Instance, + status metav1.ConditionStatus, + reason string, + message string, +) (ctrl.Result, error) { + return r.ControllerHelper.SetReconciledCondition( + ctx, + r.Status(), + req, + s3InstanceResource, + &s3InstanceResource.Status.Conditions, + s3v1alpha1.ConditionProgressing, + status, + reason, + message, + nil, + r.ReconcilePeriod, + ) +} +func (r *S3InstanceReconciler) SetAvailableCondition( + ctx context.Context, + req ctrl.Request, + s3InstanceResource *s3v1alpha1.S3Instance, + reason string, + message string, +) (ctrl.Result, error) { + return r.ControllerHelper.SetReconciledCondition( + ctx, + r.Status(), + req, + s3InstanceResource, + &s3InstanceResource.Status.Conditions, + s3v1alpha1.ConditionAvailable, + metav1.ConditionTrue, + reason, + message, + nil, + r.ReconcilePeriod, + ) +} +func (r *S3InstanceReconciler) SetDegradedCondition( + ctx context.Context, + req ctrl.Request, + s3InstanceResource *s3v1alpha1.S3Instance, + status metav1.ConditionStatus, + reason string, + message string, + err error, +) (ctrl.Result, error) { + return r.ControllerHelper.SetReconciledCondition( + ctx, + r.Status(), + req, + s3InstanceResource, + &s3InstanceResource.Status.Conditions, + s3v1alpha1.ConditionDegraded, + status, + reason, + message, + err, + r.ReconcilePeriod, + ) +} +func (r *S3InstanceReconciler) SetRejectedCondition( ctx context.Context, req ctrl.Request, s3InstanceResource *s3v1alpha1.S3Instance, @@ -38,7 +105,8 @@ func (r *S3InstanceReconciler) SetReconciledCondition( req, s3InstanceResource, &s3InstanceResource.Status.Conditions, - s3v1alpha1.ConditionReconciled, + s3v1alpha1.ConditionRejected, + metav1.ConditionFalse, reason, message, err, diff --git a/internal/controller/user/finalizer.go b/internal/controller/user/finalizer.go index d9389e4..9f55690 100644 --- a/internal/controller/user/finalizer.go +++ b/internal/controller/user/finalizer.go @@ -18,10 +18,11 @@ package user_controller import ( "context" + "fmt" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" s3v1alpha1 "github.com/InseeFrLab/s3-operator/api/v1alpha1" @@ -29,9 +30,9 @@ import ( func (r *S3UserReconciler) finalizeS3User( ctx context.Context, + req reconcile.Request, userResource *s3v1alpha1.S3User, ) error { - logger := log.FromContext(ctx) s3Client, err := r.S3Instancehelper.GetS3ClientForRessource( ctx, r.Client, @@ -41,7 +42,15 @@ func (r *S3UserReconciler) finalizeS3User( userResource.Spec.S3InstanceRef, ) if err != nil { - logger.Error(err, "An error occurred while getting s3Client") + r.SetDegradedCondition( + ctx, + req, + userResource, + metav1.ConditionUnknown, + s3v1alpha1.Unreachable, + "Failed to generate s3client from instance", + err, + ) return err } if s3Client.GetConfig().S3UserDeletionEnabled { @@ -55,66 +64,43 @@ func (r *S3UserReconciler) handleDeletion( req reconcile.Request, userResource *s3v1alpha1.S3User, ) (reconcile.Result, error) { - logger := log.FromContext(ctx) if controllerutil.ContainsFinalizer(userResource, userFinalizer) { // Run finalization logic for S3UserFinalizer. If the finalization logic fails, don't remove the finalizer so that we can retry during the next reconciliation. - if err := r.finalizeS3User(ctx, userResource); err != nil { - logger.Error( - err, - "An error occurred when attempting to finalize the user", - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + if err := r.finalizeS3User(ctx, req, userResource); err != nil { + return r.SetDegradedCondition( ctx, req, userResource, + metav1.ConditionFalse, s3v1alpha1.DeletionFailure, - "user deletion has failed", + fmt.Sprintf("User %s deletion has failed", userResource.Name), err, ) } userOwnedlinkedSecrets, _, err := r.getUserLinkedSecrets(ctx, userResource) if err != nil { - logger.Error( - err, - "An error occurred when trying to list secret linked to user", - "userResourceName", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + + return r.SetDegradedCondition( ctx, req, userResource, + metav1.ConditionFalse, s3v1alpha1.DeletionFailure, - "An error occurred when trying to list secret linked to user", + fmt.Sprintf("An error occurred when trying to list secret linked to user %s", userResource.Name), 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( + return r.SetDegradedCondition( ctx, req, userResource, + metav1.ConditionFalse, s3v1alpha1.DeletionFailure, - "Deletion of secret associated to user have failed", + fmt.Sprintf("Deletion of secret %s associated to user %s have failed", linkedSecret.Name, userResource.Name), err, ) } @@ -123,12 +109,13 @@ func (r *S3UserReconciler) handleDeletion( //Remove userFinalizer. Once all finalizers have been removed, the object will be deleted. if ok := controllerutil.RemoveFinalizer(userResource, userFinalizer); !ok { - logger.Info( - "Failed to remove finalizer for user resource", - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), + r.SetProgressingCondition( + ctx, + req, + userResource, + metav1.ConditionFalse, + s3v1alpha1.InternalError, + fmt.Sprintf("Failed to remove finalizer for user %s", userResource.Name), ) return ctrl.Result{Requeue: true}, nil } @@ -139,13 +126,14 @@ func (r *S3UserReconciler) handleDeletion( // "the object has been modified; please apply your changes to the latest version and try again" error) err = r.Update(ctx, userResource) if err != nil { - logger.Error( + r.SetDegradedCondition( + ctx, + req, + userResource, + metav1.ConditionFalse, + s3v1alpha1.K8sApiError, + fmt.Sprintf("An error occured when removing finalizer from user %s", userResource.Name), err, - "An error occurred when removing finalizer from policy", - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), ) return ctrl.Result{}, err } diff --git a/internal/controller/user/reconcile.go b/internal/controller/user/reconcile.go index 7538fa1..d8d34e4 100644 --- a/internal/controller/user/reconcile.go +++ b/internal/controller/user/reconcile.go @@ -20,11 +20,11 @@ import ( "cmp" "context" "fmt" + "strings" "slices" corev1 "k8s.io/api/core/v1" k8sapierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" @@ -62,25 +62,13 @@ func (r *S3UserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // Let's just set the status as Unknown when no status are available if len(userResource.Status.Conditions) == 0 { - meta.SetStatusCondition( - &userResource.Status.Conditions, - metav1.Condition{ - Type: s3v1alpha1.ConditionReconciled, - Status: metav1.ConditionUnknown, - ObservedGeneration: userResource.Generation, - Reason: s3v1alpha1.Reconciling, - Message: "Starting reconciliation", - }, - ) - if err = r.Status().Update(ctx, userResource); err != nil { - logger.Error( - err, - "Failed to update userRessource status", - "userResourceName", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) + _, err = r.SetProgressingCondition(ctx, + req, + userResource, + metav1.ConditionUnknown, + s3v1alpha1.Reconciling, + fmt.Sprintf("Newly discovered resource %s", userResource.Name)) + if err != nil { return ctrl.Result{}, err } @@ -90,47 +78,45 @@ func (r *S3UserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // your changes to the latest version and try again" which would re-trigger the reconciliation // if we try to update it again in the following operations if err := r.Get(ctx, req.NamespacedName, userResource); err != nil { - logger.Error( - err, - "Failed to re-fetch userResource", - "userResourceName", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) + r.SetDegradedCondition(ctx, + req, + userResource, + metav1.ConditionFalse, + s3v1alpha1.K8sApiError, + fmt.Sprintf("Failed to re-fetch user resource %s", userResource.Name), + err) return ctrl.Result{}, err } } // Add finalizer for this CR if !controllerutil.ContainsFinalizer(userResource, userFinalizer) { - logger.Info("Adding finalizer to user resource", - "userResourceName", - userResource.Name, - "NamespacedName", - req.NamespacedName.String()) + r.SetProgressingCondition(ctx, + req, + userResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("Adding finalizer to user resource %s", userResource.Name)) if ok := controllerutil.AddFinalizer(userResource, userFinalizer); !ok { - logger.Error( - err, - "Failed to add finalizer into user resource", - "userResourceName", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) + r.SetDegradedCondition(ctx, + req, + userResource, + metav1.ConditionFalse, + s3v1alpha1.InternalError, + fmt.Sprintf("Failed to add finalizer to user resource %s", userResource.Name), + err) return ctrl.Result{Requeue: true}, nil } if err = r.Update(ctx, userResource); err != nil { - logger.Error( - err, - "An error occurred when adding finalizer from user", - "userResourceName", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) + r.SetDegradedCondition(ctx, + req, + userResource, + metav1.ConditionFalse, + s3v1alpha1.K8sApiError, + fmt.Sprintf("An error occurred when adding finalizer on user resource %s", userResource.Name), + err) return ctrl.Result{}, err } @@ -140,14 +126,13 @@ func (r *S3UserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // your changes to the latest version and try again" which would re-trigger the reconciliation // if we try to update it again in the following operations if err := r.Get(ctx, req.NamespacedName, userResource); err != nil { - logger.Error( - err, - "Failed to re-fetch userResource", - "userResourceName", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) + r.SetDegradedCondition(ctx, + req, + userResource, + metav1.ConditionFalse, + s3v1alpha1.K8sApiError, + fmt.Sprintf("Failed to re-fetch user resource %s", userResource.Name), + err) return ctrl.Result{}, err } } @@ -155,14 +140,16 @@ func (r *S3UserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // Check if the userResource instance is marked to be deleted, which is // indicated by the deletion timestamp being set. The object will be deleted. if userResource.GetDeletionTimestamp() != nil { - logger.Info("userResource have been marked for deletion", - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String()) + r.SetProgressingCondition(ctx, + req, + userResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("User resource have been marked for deletion %s", userResource.Name)) return r.handleDeletion(ctx, req, userResource) } + r.SetProgressingCondition(ctx, req, userResource, metav1.ConditionTrue, s3v1alpha1.Reconciling, fmt.Sprintf("Starting reconciliation of user %s", userResource.Name)) return r.handleReconciliation(ctx, req, userResource) } @@ -172,7 +159,6 @@ func (r *S3UserReconciler) handleReconciliation( req reconcile.Request, userResource *s3v1alpha1.S3User, ) (reconcile.Result, error) { - logger := log.FromContext(ctx) s3Client, err := r.S3Instancehelper.GetS3ClientForRessource( ctx, @@ -183,11 +169,11 @@ func (r *S3UserReconciler) handleReconciliation( userResource.Spec.S3InstanceRef, ) if err != nil { - logger.Error(err, "An error occurred while getting s3Client") - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, userResource, + metav1.ConditionUnknown, s3v1alpha1.Unreachable, "Failed to generate s3client from instance", err, @@ -196,20 +182,13 @@ func (r *S3UserReconciler) handleReconciliation( found, err := s3Client.UserExist(userResource.Spec.AccessKey) if err != nil { - logger.Error( - err, - "An error occurred while checking the existence of a user", - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, userResource, + metav1.ConditionUnknown, s3v1alpha1.Unreachable, - "Fail to check existence", + fmt.Sprintf("Error while checking if user %s already exist", userResource.Name), err, ) } @@ -225,7 +204,6 @@ func (r *S3UserReconciler) handleUpdate( req reconcile.Request, userResource *s3v1alpha1.S3User, ) (reconcile.Result, error) { - logger := log.FromContext(ctx) // Create S3Client s3Client, err := r.S3Instancehelper.GetS3ClientForRessource( @@ -237,11 +215,11 @@ func (r *S3UserReconciler) handleUpdate( userResource.Spec.S3InstanceRef, ) if err != nil { - logger.Error(err, "An error occurred while getting s3Client") - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, userResource, + metav1.ConditionUnknown, s3v1alpha1.Unreachable, "Failed to generate s3client from instance", err, @@ -250,65 +228,32 @@ func (r *S3UserReconciler) handleUpdate( 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( + return r.SetDegradedCondition( ctx, req, userResource, + metav1.ConditionUnknown, s3v1alpha1.Unreachable, - "Impossible to list the user's secret", - err, - ) - } - 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", + fmt.Sprintf("Impossible to list the secrets of user %s", userResource.Name), err, ) } currentUserSecret := corev1.Secret{} 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", - req.NamespacedName.String(), - ) + r.SetProgressingCondition(ctx, + req, + userResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("No Secret associated to user %s found, user will be deleted from the S3 backend, then recreated with a secret", userResource.Name)) 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( + return r.SetDegradedCondition( ctx, req, userResource, + metav1.ConditionFalse, s3v1alpha1.Unreachable, fmt.Sprintf( "Deletion of S3user %s on S3 server has failed", @@ -326,13 +271,13 @@ func (r *S3UserReconciler) handleUpdate( 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( + return r.SetDegradedCondition( ctx, req, userResource, + metav1.ConditionFalse, s3v1alpha1.Unreachable, - "Failed to delete old linkedSecret", + fmt.Sprintf("Failed to delete old linkedSecret %s for user %s", linkedsecret.Name, userResource.Name), err, ) } @@ -342,26 +287,21 @@ func (r *S3UserReconciler) handleUpdate( } } 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(), - ) + r.SetProgressingCondition(ctx, + req, + userResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + "No Secret associated to user found, user will be deleted from the S3 backend, then recreated with a secret") + 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( + return r.SetDegradedCondition( ctx, req, userResource, + metav1.ConditionFalse, s3v1alpha1.Unreachable, fmt.Sprintf( "Deletion of S3user %s on S3 server has failed", @@ -373,23 +313,23 @@ func (r *S3UserReconciler) handleUpdate( return r.handleCreate(ctx, req, userResource) } } + r.SetProgressingCondition(ctx, + req, + userResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("Checking user %s policies", userResource.Name)) + - logger.Info("Checking user policies", "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String()) userPolicies, err := s3Client.GetUserPolicies(userResource.Spec.AccessKey) if err != nil { - logger.Error(err, "Could not check the user's policies", "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String()) - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, userResource, + metav1.ConditionUnknown, s3v1alpha1.Unreachable, - "Checking the S3user policies has failed", + fmt.Sprintf("Checking the S3user policies has failed for user %s", userResource.Name), err, ) } @@ -399,13 +339,6 @@ func (r *S3UserReconciler) handleUpdate( for _, policy := range userPolicies { policyFound := slices.Contains(userResource.Spec.Policies, policy) if !policyFound { - logger.Info( - fmt.Sprintf("S3User has unexpected policy not in definition: %s", policy), - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) policyToDelete = append(policyToDelete, policy) } } @@ -413,60 +346,55 @@ func (r *S3UserReconciler) handleUpdate( for _, policy := range userResource.Spec.Policies { policyFound := slices.Contains(userPolicies, policy) if !policyFound { - logger.Info( - fmt.Sprintf("S3User is missing policy from definition: %s", policy), - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) policyToAdd = append(policyToAdd, policy) } } - if len(policyToDelete) > 0 { - err = s3Client.RemovePoliciesFromUser(userResource.Spec.AccessKey, policyToDelete) - if err != nil { - logger.Error( - err, - "An error occurred while removing policy to user", - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( - ctx, - req, - userResource, - s3v1alpha1.Unreachable, - "Error while updating policies of user", - err, - ) + if len(policyToDelete) > 0 || len(policyToAdd) > 0 { + var message string = "" + if len(policyToDelete) > 0 { + message = fmt.Sprintf("S3User %s has unexpected policy not in definition: %s", userResource.Name, strings.Join(policyToDelete, ", ")) + } + if len(policyToAdd) > 0 { + message += fmt.Sprintf("S3User %s is missing policy from definition: %s", userResource.Name, strings.Join(policyToAdd, ", ")) + } + // Update the condition (+ prints a log) before doing anything + r.SetProgressingCondition(ctx, + req, + userResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + message) + + if len(policyToDelete) > 0 { + err = s3Client.RemovePoliciesFromUser(userResource.Spec.AccessKey, policyToDelete) + if err != nil { + return r.SetDegradedCondition( + ctx, + req, + userResource, + metav1.ConditionFalse, + s3v1alpha1.Unreachable, + fmt.Sprintf("Error while removing policies of user %s", userResource.Name), + err, + ) + } } - } - - if len(policyToAdd) > 0 { - err := s3Client.AddPoliciesToUser(userResource.Spec.AccessKey, policyToAdd) - if err != nil { - logger.Error( - err, - "An error occurred while adding policy to user", - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( - ctx, - req, - userResource, - s3v1alpha1.Unreachable, - "Error while updating policies of user", - err, - ) + if len(policyToAdd) > 0 { + err := s3Client.AddPoliciesToUser(userResource.Spec.AccessKey, policyToAdd) + if err != nil { + return r.SetDegradedCondition( + ctx, + req, + userResource, + metav1.ConditionFalse, + s3v1alpha1.Unreachable, + fmt.Sprintf("Error while adding policies of user %s", userResource.Name), + err, + ) + } } } @@ -477,73 +405,56 @@ func (r *S3UserReconciler) handleUpdate( ) if err != nil { - logger.Error( - err, - "An error occurred when checking if user credentials were valid", - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, userResource, + metav1.ConditionFalse, s3v1alpha1.Unreachable, - "Checking credentials on S3 server has failed", + fmt.Sprintf("Checking credentials on S3 server has failed for user %s", userResource.Name), err, ) } if !credentialsValid { 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(), - ) + r.SetProgressingCondition(ctx, + req, + userResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("The secret %s containing the credentials will be deleted, and the user %s will be deleted from the S3 backend, then recreated (through another reconcile)", currentUserSecret.Name, userResource.Name)) + 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( + return r.SetDegradedCondition( ctx, req, userResource, + metav1.ConditionFalse, s3v1alpha1.Unreachable, - "Deletion of secret associated to user have failed", + fmt.Sprintf("Deletion of secret associated to user %s have failed", userResource.Name), 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(), - ) + r.SetProgressingCondition(ctx, + req, + userResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("The user %s will be deleted from the S3 backend, then recreated (through another reconcile), the secret %s will be kept.", userResource.Name, currentUserSecret.Name)) + } 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( + return r.SetDegradedCondition( ctx, req, userResource, + metav1.ConditionFalse, s3v1alpha1.Unreachable, fmt.Sprintf( "Deletion of S3user %s on S3 server has failed", @@ -556,20 +467,12 @@ func (r *S3UserReconciler) handleUpdate( return r.handleCreate(ctx, req, userResource) } - logger.Info("User was reconciled without error", - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - - return r.SetReconciledCondition( + return r.SetAvailableCondition( ctx, req, userResource, s3v1alpha1.Reconciled, - "User reconciled", - err, + fmt.Sprintf("User %s reconciled", userResource.Name), ) } @@ -578,7 +481,6 @@ func (r *S3UserReconciler) handleCreate( req reconcile.Request, userResource *s3v1alpha1.S3User, ) (reconcile.Result, error) { - logger := log.FromContext(ctx) // Create S3Client s3Client, err := r.S3Instancehelper.GetS3ClientForRessource( @@ -590,12 +492,12 @@ func (r *S3UserReconciler) handleCreate( userResource.Spec.S3InstanceRef, ) if err != nil { - logger.Error(err, "An error occurred while getting s3Client") - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, userResource, - s3v1alpha1.Unreachable, + metav1.ConditionUnknown, + s3v1alpha1.InternalError, "Failed to generate s3client from instance", err, ) @@ -605,19 +507,12 @@ func (r *S3UserReconciler) handleCreate( secretKey, err := r.PasswordGeneratorHelper.Generate(20, true, false, true) if err != nil { - logger.Error(err, fmt.Sprintf("Fail to generate password for user %s", userResource.Name), - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - - return r.SetReconciledCondition( + return r.SetRejectedCondition( ctx, req, userResource, - s3v1alpha1.Unreachable, - "An error occurred when attempting to generate password for user", + s3v1alpha1.InternalError, + fmt.Sprintf("Fail to generate password for user %s", userResource.Name), err, ) } @@ -632,17 +527,11 @@ func (r *S3UserReconciler) handleCreate( ) if err != nil { // Error while creating the Kubernetes secret - requeue the request. - logger.Error(err, "Could not generate Kubernetes secret", - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetRejectedCondition( ctx, req, userResource, - s3v1alpha1.Unreachable, + s3v1alpha1.K8sApiError, "Generation of associated k8s Secret has failed", err, ) @@ -659,62 +548,45 @@ func (r *S3UserReconciler) handleCreate( // If none exist : we create the user, then the secret if err != nil && k8sapierrors.IsNotFound(err) { - logger.Info( - "No secret found ; creating a new Secret", - "Secret.Namespace", - secret.Namespace, - "Secret.Name", - secret.Name, - ) + r.SetProgressingCondition(ctx, + req, + userResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("No secret found for user %s creating a new Secret", userResource.Name)) // Creating the user err = s3Client.CreateUser(userResource.Spec.AccessKey, secretKey) if err != nil { - logger.Error( - err, - "An error occurred while creating user on S3 server", - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + + return r.SetRejectedCondition( ctx, req, userResource, s3v1alpha1.Unreachable, - "Creation of user on S3 instance has failed", + fmt.Sprintf("Creation of user %s on S3 instance has failed", userResource.Name), err, ) } // Creating the secret - logger.Info( - "Creating a new secret to store the user's credentials", - "secretName", - secret.Name, - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) + r.SetProgressingCondition(ctx, + req, + userResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("Creating a new secret (%s) to store the credentials of user %s", secret.Name, userResource.Name)) + err = r.Create(ctx, secret) if err != nil { - logger.Error(err, "Could not create secret for user", - "secretName", - secret.Name, - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, userResource, + metav1.ConditionFalse, s3v1alpha1.Unreachable, - "Creation of secret for user has failed", + fmt.Sprintf("Creation of secret %s for user %s has failed", secret.Name, userResource.Name), err, ) } @@ -722,40 +594,32 @@ func (r *S3UserReconciler) handleCreate( // Add policies err = r.addPoliciesToUser(ctx, userResource) if err != nil { - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, userResource, + metav1.ConditionFalse, s3v1alpha1.Unreachable, "Error while updating policies of user on S3 instance", err, ) } - return r.SetReconciledCondition( + return r.SetAvailableCondition( ctx, req, userResource, s3v1alpha1.Reconciled, - "User reconciled", - err, + fmt.Sprintf("User %s reconciled", userResource.Name), ) } else if err != nil { - logger.Error(err, "Couldn't check secret existence", - "secretName", - secret.Name, - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetRejectedCondition( ctx, req, userResource, - s3v1alpha1.Unreachable, - "Fail to check if an existing secret already exist", + s3v1alpha1.K8sApiError, + fmt.Sprintf("Fail to check if an existing secret %s already exist for user %s", secret.Name, userResource.Name), err, ) } else { @@ -765,17 +629,7 @@ func (r *S3UserReconciler) handleCreate( if ref.Kind == "S3User" { if ref.UID != userResource.UID { 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", - secret.Name, - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String()) - return r.SetReconciledCondition( + return r.SetRejectedCondition( ctx, req, userResource, @@ -790,12 +644,13 @@ func (r *S3UserReconciler) handleCreate( 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()) + r.SetProgressingCondition(ctx, + req, + userResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("The secret key of user %s will be retrieved from the secret named %s.", userResource.Name, secret.Name)) + var cpData = *&existingK8sSecret.Data for k, v := range cpData { if k == userResource.Spec.SecretFieldNameSecretKey { @@ -804,82 +659,69 @@ func (r *S3UserReconciler) handleCreate( } } 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()) + r.SetProgressingCondition(ctx, + req, + userResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("A secret with the name %s already exists ; it will be overwritten for user %s because of operator configuration", secret.Name, userResource.Name)) } // Creating the user err = s3Client.CreateUser(userResource.Spec.AccessKey, secretKey) if err != nil { - logger.Error( - err, - "An error occurred while creating user on S3 server", - "userResource", - userResource.Name, - "NamespacedName", - req.NamespacedName.String(), - ) - return r.SetReconciledCondition( + return r.SetRejectedCondition( ctx, req, userResource, s3v1alpha1.Unreachable, - "Creation of user on S3 instance has failed", + fmt.Sprintf("Creation of user %s on S3 instance has failed", userResource.Name), err, ) } 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(), - ) + r.SetProgressingCondition(ctx, + req, + userResource, + metav1.ConditionTrue, + s3v1alpha1.Reconciling, + fmt.Sprintf("Updating the pre-existing secret %s with new credentials for user %s", secret.Name, userResource.Name)) + 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( + return r.SetDegradedCondition( ctx, req, userResource, + metav1.ConditionFalse, s3v1alpha1.Unreachable, - "Update of secret have failed", + fmt.Sprintf("Update of secret %s have failed for user %s", secret.Name, userResource.Name), err, ) + } } // Add policies err = r.addPoliciesToUser(ctx, userResource) if err != nil { - return r.SetReconciledCondition( + return r.SetDegradedCondition( ctx, req, userResource, + metav1.ConditionFalse, s3v1alpha1.Unreachable, "Error while updating associated policy", err, ) } - return r.SetReconciledCondition( + return r.SetAvailableCondition( ctx, req, userResource, s3v1alpha1.Reconciled, - "User reconciled", - err, + fmt.Sprintf("User %s reconciled", userResource.Name), ) } @@ -887,20 +729,13 @@ func (r *S3UserReconciler) handleCreate( // 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. 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", - userResource.Name, - "NamespacedName", - req.NamespacedName.String()) - return r.SetReconciledCondition( + + return r.SetRejectedCondition( ctx, req, userResource, s3v1alpha1.CreationFailure, - "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", + fmt.Sprintf("Creation of user %s on S3 instance has failed because a secret named %s already exists. The user's spec should be changed to target a different secret", userResource.Name, secret.Name), err, ) } diff --git a/internal/controller/user/reconcile_test.go b/internal/controller/user/reconcile_test.go index ff05232..0d4e8b5 100644 --- a/internal/controller/user/reconcile_test.go +++ b/internal/controller/user/reconcile_test.go @@ -128,9 +128,11 @@ func TestExistingSecret(t *testing.T) { 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") + var conditions = s3UserResourceUpdated.Status.Conditions + var lastCondition = conditions[len(conditions) - 1] + assert.Equal(t, s3v1alpha1.CreationFailure, lastCondition.Reason) + assert.Equal(t, metav1.ConditionFalse, lastCondition.Status) + assert.Contains(t, lastCondition.Message, "Creation of user example-user on S3 instance has failed because a secret named example-user-secret already exists") }) reconciler.ReadExistingSecret = false @@ -155,9 +157,11 @@ func TestExistingSecret(t *testing.T) { 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") + var conditions = s3UserResourceUpdated.Status.Conditions + var lastCondition = conditions[len(conditions) - 1] + assert.Equal(t, s3v1alpha1.Reconciled, lastCondition.Reason) + assert.Equal(t, metav1.ConditionTrue, lastCondition.Status) + assert.Contains(t, lastCondition.Message, "User example-user reconciled") newSecret := &corev1.Secret{} @@ -190,9 +194,11 @@ func TestExistingSecret(t *testing.T) { 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") + var conditions = s3UserResourceUpdated.Status.Conditions + var lastCondition = conditions[len(conditions) - 1] + assert.Equal(t, s3v1alpha1.Reconciled, lastCondition.Reason) + assert.Equal(t, metav1.ConditionTrue, lastCondition.Status) + assert.Contains(t, lastCondition.Message, "User example-user reconciled") newSecret := &corev1.Secret{} err = testUtils.Client.Get(context.TODO(), client.ObjectKey{ @@ -224,9 +230,11 @@ func TestExistingSecret(t *testing.T) { 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") + var conditions = s3UserResourceUpdated.Status.Conditions + var lastCondition = conditions[len(conditions) - 1] + assert.Equal(t, s3v1alpha1.CreationFailure, lastCondition.Reason) + assert.Equal(t, metav1.ConditionFalse, lastCondition.Status) + assert.Contains(t, lastCondition.Message, "The secret matching the new S3User's spec is owned by a different, pre-existing S3User") }) } @@ -292,9 +300,11 @@ func TestHandleCreate(t *testing.T) { 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") + var conditions = s3UserResourceUpdated.Status.Conditions + var lastCondition = conditions[len(conditions) - 1] + assert.Equal(t, s3v1alpha1.Reconciled, lastCondition.Reason) + assert.Equal(t, metav1.ConditionTrue, lastCondition.Status) + assert.Contains(t, lastCondition.Message, "User example-user reconciled") }) t.Run("error if using invalidS3Instance", func(t *testing.T) { @@ -390,9 +400,11 @@ func TestHandleUpdate(t *testing.T) { 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") + var conditions = s3UserResourceUpdated.Status.Conditions + var lastCondition = conditions[len(conditions) - 1] + assert.Equal(t, s3v1alpha1.Reconciled, lastCondition.Reason) + assert.Equal(t, metav1.ConditionTrue, lastCondition.Status) + assert.Contains(t, lastCondition.Message, "User existing-valid-user reconciled") }) }) @@ -451,9 +463,11 @@ func TestHandleUpdate(t *testing.T) { 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") + var conditions = s3UserResourceUpdated.Status.Conditions + var lastCondition = conditions[len(conditions) - 1] + assert.Equal(t, s3v1alpha1.Reconciled, lastCondition.Reason) + assert.Equal(t, metav1.ConditionTrue, lastCondition.Status) + assert.Contains(t, lastCondition.Message, "User existing-valid-user reconciled") }) }) diff --git a/internal/controller/user/status.go b/internal/controller/user/status.go index 990a159..0911daf 100644 --- a/internal/controller/user/status.go +++ b/internal/controller/user/status.go @@ -19,12 +19,79 @@ package user_controller import ( "context" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" s3v1alpha1 "github.com/InseeFrLab/s3-operator/api/v1alpha1" ) -func (r *S3UserReconciler) SetReconciledCondition( +func (r *S3UserReconciler) SetProgressingCondition( + ctx context.Context, + req ctrl.Request, + userResource *s3v1alpha1.S3User, + status metav1.ConditionStatus, + reason string, + message string, +) (ctrl.Result, error) { + return r.ControllerHelper.SetReconciledCondition( + ctx, + r.Status(), + req, + userResource, + &userResource.Status.Conditions, + s3v1alpha1.ConditionProgressing, + status, + reason, + message, + nil, + r.ReconcilePeriod, + ) +} +func (r *S3UserReconciler) SetAvailableCondition( + ctx context.Context, + req ctrl.Request, + userResource *s3v1alpha1.S3User, + reason string, + message string, +) (ctrl.Result, error) { + return r.ControllerHelper.SetReconciledCondition( + ctx, + r.Status(), + req, + userResource, + &userResource.Status.Conditions, + s3v1alpha1.ConditionAvailable, + metav1.ConditionTrue, + reason, + message, + nil, + r.ReconcilePeriod, + ) +} +func (r *S3UserReconciler) SetDegradedCondition( + ctx context.Context, + req ctrl.Request, + userResource *s3v1alpha1.S3User, + status metav1.ConditionStatus, + reason string, + message string, + err error, +) (ctrl.Result, error) { + return r.ControllerHelper.SetReconciledCondition( + ctx, + r.Status(), + req, + userResource, + &userResource.Status.Conditions, + s3v1alpha1.ConditionDegraded, + status, + reason, + message, + err, + r.ReconcilePeriod, + ) +} +func (r *S3UserReconciler) SetRejectedCondition( ctx context.Context, req ctrl.Request, userResource *s3v1alpha1.S3User, @@ -38,7 +105,8 @@ func (r *S3UserReconciler) SetReconciledCondition( req, userResource, &userResource.Status.Conditions, - s3v1alpha1.ConditionReconciled, + s3v1alpha1.ConditionRejected, + metav1.ConditionFalse, reason, message, err, diff --git a/internal/helpers/S3instance_test.go b/internal/helpers/S3instance_test.go index 678416a..7de612d 100644 --- a/internal/helpers/S3instance_test.go +++ b/internal/helpers/S3instance_test.go @@ -60,7 +60,7 @@ func TestGetS3ClientForRessource(t *testing.T) { Name: "default", Namespace: "s3-operator", }, - Status: s3v1alpha1.S3InstanceStatus{Conditions: []metav1.Condition{{Reason: s3v1alpha1.Reconciled}}}, + Status: s3v1alpha1.S3InstanceStatus{Conditions: []metav1.Condition{{Type: s3v1alpha1.ConditionAvailable, Reason: s3v1alpha1.Reconciled}}}, } s3Instance_not_ready := &s3v1alpha1.S3Instance{ @@ -112,7 +112,7 @@ func TestGetS3ClientForRessource(t *testing.T) { t.Run("error because instance not ready", func(t *testing.T) { s3instanceHelper := helpers.NewS3InstanceHelper() _, err := s3instanceHelper.GetS3ClientForRessource(context.TODO(), client, testUtils.S3Factory, "bucket-example", "default", "s3-operator/not-ready") - assert.Equal(t, "S3instance is not in a ready state", err.Error()) + assert.Equal(t, "S3instance is not in a ready state: ", err.Error()) }) } diff --git a/internal/helpers/controller.go b/internal/helpers/controller.go index a243ada..80d922a 100644 --- a/internal/helpers/controller.go +++ b/internal/helpers/controller.go @@ -44,6 +44,7 @@ func (c *ControllerHelper) SetReconciledCondition( resource client.Object, // Accepts any Kubernetes object with conditions conditions *[]metav1.Condition, // Conditions field reference (must be a pointer) conditionType string, // The type of condition to set + status metav1.ConditionStatus, reason string, message string, err error, @@ -55,30 +56,21 @@ func (c *ControllerHelper) SetReconciledCondition( if err != nil { logger.Error(err, message, "NamespacedName", req.NamespacedName.String()) - changed = meta.SetStatusCondition( - conditions, - metav1.Condition{ - Type: conditionType, - Status: metav1.ConditionFalse, - ObservedGeneration: resource.GetGeneration(), - Reason: reason, - Message: fmt.Sprintf("%s: %s", message, err), - }, - ) + message = fmt.Sprintf("%s: %s", message, err) + } else { logger.Info(message, "NamespacedName", req.NamespacedName.String()) - changed = meta.SetStatusCondition( - conditions, - metav1.Condition{ - Type: conditionType, - Status: metav1.ConditionTrue, - ObservedGeneration: resource.GetGeneration(), - Reason: reason, - Message: message, - }, - ) } - + changed = meta.SetStatusCondition( + conditions, + metav1.Condition{ + Type: conditionType, + Status: status, + ObservedGeneration: resource.GetGeneration(), + Reason: reason, + Message: message, + }, + ) if changed { if errStatusUpdate := statusWriter.Update(ctx, resource); errStatusUpdate != nil { logger.Error(errStatusUpdate, "Failed to update resource status", "ObjectKind", resource.GetObjectKind(), "NamespacedName", req.NamespacedName.String()) diff --git a/internal/helpers/controller_test.go b/internal/helpers/controller_test.go index a71c4db..86b5f7b 100644 --- a/internal/helpers/controller_test.go +++ b/internal/helpers/controller_test.go @@ -70,9 +70,10 @@ func TestSetReconciledCondition(t *testing.T) { ctrl.Request{NamespacedName: types.NamespacedName{Name: s3instanceResource.Name, Namespace: s3instanceResource.Namespace}}, s3instanceResource, &s3instanceResource.Status.Conditions, + s3v1alpha1.ConditionAvailable, + metav1.ConditionTrue, s3v1alpha1.Reconciled, "s3Instance reconciled", - "s3Instance reconciled", nil, time.Duration(10), ) assert.NoError(t, err) @@ -85,7 +86,10 @@ func TestSetReconciledCondition(t *testing.T) { Name: "default", }, s3instanceResourceUpdated) assert.NoError(t, err) - assert.Equal(t, s3v1alpha1.Reconciled, s3instanceResourceUpdated.Status.Conditions[0].Type) + var conditions = s3instanceResourceUpdated.Status.Conditions + var lastCondition = conditions[len(conditions) - 1] + assert.Equal(t, s3v1alpha1.ConditionAvailable, lastCondition.Type) + assert.Equal(t, s3v1alpha1.Reconciled, lastCondition.Reason) assert.Equal(t, "s3Instance reconciled", s3instanceResourceUpdated.Status.Conditions[0].Message) }) @@ -96,9 +100,10 @@ func TestSetReconciledCondition(t *testing.T) { ctrl.Request{NamespacedName: types.NamespacedName{Name: s3instanceResource.Name, Namespace: s3instanceResource.Namespace}}, s3instanceResource, &s3instanceResource.Status.Conditions, + s3v1alpha1.ConditionDegraded, + metav1.ConditionFalse, s3v1alpha1.CreationFailure, "s3Instance reconciled", - "s3Instance reconciled", fmt.Errorf("Something wrong have happened"), time.Duration(10), ) @@ -113,7 +118,10 @@ func TestSetReconciledCondition(t *testing.T) { Name: "default", }, s3instanceResourceUpdated) assert.NoError(t, err) - assert.Equal(t, s3v1alpha1.CreationFailure, s3instanceResourceUpdated.Status.Conditions[1].Type) + var conditions = s3instanceResourceUpdated.Status.Conditions + var lastCondition = conditions[len(conditions) - 1] + assert.Equal(t, s3v1alpha1.ConditionDegraded, lastCondition.Type) + assert.Equal(t, s3v1alpha1.CreationFailure, lastCondition.Reason) assert.Contains(t, s3instanceResourceUpdated.Status.Conditions[1].Message, "Something wrong have happened") }) } diff --git a/internal/helpers/s3instance.go b/internal/helpers/s3instance.go index c74ebe7..672827f 100644 --- a/internal/helpers/s3instance.go +++ b/internal/helpers/s3instance.go @@ -69,8 +69,10 @@ func (s3InstanceHelper *S3InstanceHelper) GetS3ClientForRessource( return nil, fmt.Errorf("S3Instance %s not found", s3InstanceInfo.name) } - if s3Instance.Status.Conditions[0].Reason != s3v1alpha1.Reconciled { - return nil, fmt.Errorf("S3instance is not in a ready state") + var conditions = s3Instance.Status.Conditions + var lastCondition = conditions[len(conditions) - 1] + if lastCondition.Type != s3v1alpha1.ConditionAvailable { + return nil, fmt.Errorf("S3instance is not in a ready state: %s", lastCondition.Type) } return s3InstanceHelper.GetS3ClientFromS3Instance(ctx, client, s3factory, s3Instance) diff --git a/test/utils/testUtils.go b/test/utils/testUtils.go index b899496..c70cfb8 100644 --- a/test/utils/testUtils.go +++ b/test/utils/testUtils.go @@ -132,7 +132,7 @@ func (t *TestUtils) SetupDefaultS3instance() *s3v1alpha1.S3Instance { Name: "default", Namespace: "s3-operator", }, - Status: s3v1alpha1.S3InstanceStatus{Conditions: []metav1.Condition{{Reason: s3v1alpha1.Reconciled}}}, + Status: s3v1alpha1.S3InstanceStatus{Conditions: []metav1.Condition{{Type: s3v1alpha1.ConditionAvailable, Reason: s3v1alpha1.Reconciled}}}, } return s3Instance @@ -155,7 +155,7 @@ func (t *TestUtils) GenerateBasicS3InstanceAndSecret() (*s3v1alpha1.S3Instance, Name: "default", Namespace: "s3-operator", }, - Status: s3v1alpha1.S3InstanceStatus{Conditions: []metav1.Condition{{Reason: s3v1alpha1.Reconciled}}}, + Status: s3v1alpha1.S3InstanceStatus{Conditions: []metav1.Condition{{Type: s3v1alpha1.ConditionAvailable, Reason: s3v1alpha1.Reconciled}}}, } secretResource := &corev1.Secret{