diff --git a/controllers/common.go b/controllers/common.go index 45c40f8a..a23cca4d 100644 --- a/controllers/common.go +++ b/controllers/common.go @@ -12,9 +12,11 @@ import ( "github.com/go-logr/logr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/log" secretsv1beta1 "github.com/hashicorp/vault-secrets-operator/api/v1beta1" "github.com/hashicorp/vault-secrets-operator/internal/common" + "github.com/hashicorp/vault-secrets-operator/internal/consts" ) var ( @@ -233,3 +235,27 @@ func parseDurationString(duration, path string, min time.Duration) (time.Duratio func isInWindow(t1, t2 time.Time) bool { return t1.After(t2) || t1.Equal(t2) } + +// maybeAddFinalizer updates client.Object with finalizer if it is not already +// set. Return true if the object was updated, in which case the object's +// ResourceVersion will have changed. This update should be handled by in the +// caller. +func maybeAddFinalizer(ctx context.Context, c client.Client, o client.Object, finalizer string) (bool, error) { + if o.GetDeletionTimestamp() == nil && !controllerutil.ContainsFinalizer(o, finalizer) { + // always call maybeAddFinalizer() after client.Client.Status.Update() to avoid + // API validation errors due to changes to the status schema. + logger := log.FromContext(ctx).WithValues("finalizer", finalizer) + logger.V(consts.LogLevelTrace).Info("Adding finalizer", + "finalizer", finalizer) + controllerutil.AddFinalizer(o, finalizer) + if err := c.Update(ctx, o); err != nil { + logger.Error(err, "Failed to add finalizer") + controllerutil.RemoveFinalizer(o, finalizer) + return false, err + } + + return true, nil + } + + return false, nil +} diff --git a/controllers/common_test.go b/controllers/common_test.go index 1163c2ee..283bfa97 100644 --- a/controllers/common_test.go +++ b/controllers/common_test.go @@ -4,12 +4,22 @@ package controllers import ( + "context" "fmt" "testing" "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + secretsv1beta1 "github.com/hashicorp/vault-secrets-operator/api/v1beta1" ) func Test_dynamicHorizon(t *testing.T) { @@ -202,3 +212,171 @@ func Test_isInWindow(t *testing.T) { }) } } + +func Test_maybeAddFinalizer(t *testing.T) { + t.Parallel() + + ctx := context.Background() + clientBuilder := newClientBuilder() + deletionTimestamp := metav1.NewTime(time.Now()) + + tests := []struct { + name string + o client.Object + create bool + finalizer string + want bool + wantFinalizers []string + wantErr assert.ErrorAssertionFunc + }{ + { + name: "updated", + o: &secretsv1beta1.VaultAuth{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "updated", + }, + Spec: secretsv1beta1.VaultAuthSpec{ + Method: "kubernetes", + }, + }, + create: true, + finalizer: vaultAuthFinalizer, + want: true, + wantFinalizers: []string{vaultAuthFinalizer}, + wantErr: assert.NoError, + }, + { + name: "updated-with-multiple", + o: &secretsv1beta1.VaultAuth{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "updated", + Finalizers: []string{ + "other", + }, + }, + Spec: secretsv1beta1.VaultAuthSpec{ + Method: "kubernetes", + }, + }, + create: true, + finalizer: vaultAuthFinalizer, + want: true, + wantFinalizers: []string{"other", vaultAuthFinalizer}, + wantErr: assert.NoError, + }, + { + name: "not-updated-exists-with-finalizer", + o: &secretsv1beta1.VaultAuth{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "updated", + Finalizers: []string{ + vaultAuthFinalizer, + }, + }, + Spec: secretsv1beta1.VaultAuthSpec{ + Method: "kubernetes", + }, + }, + create: true, + finalizer: vaultAuthFinalizer, + want: false, + wantFinalizers: []string{vaultAuthFinalizer}, + wantErr: assert.NoError, + }, + { + name: "not-updated-inexistent-with-finalizer", + o: &secretsv1beta1.VaultAuth{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "updated", + Finalizers: []string{ + vaultAuthFinalizer, + }, + }, + Spec: secretsv1beta1.VaultAuthSpec{ + Method: "kubernetes", + }, + }, + finalizer: vaultAuthFinalizer, + want: false, + wantFinalizers: []string{vaultAuthFinalizer}, + wantErr: assert.NoError, + }, + { + name: "not-updated-has-deletion-timestamp", + o: &secretsv1beta1.VaultAuth{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "updated", + DeletionTimestamp: &deletionTimestamp, + }, + Spec: secretsv1beta1.VaultAuthSpec{ + Method: "kubernetes", + }, + }, + finalizer: vaultAuthFinalizer, + want: false, + wantFinalizers: []string(nil), + wantErr: assert.NoError, + }, + { + name: "invalid-not-found", + o: &secretsv1beta1.VaultAuth{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "updated", + }, + Spec: secretsv1beta1.VaultAuthSpec{ + Method: "kubernetes", + }, + }, + finalizer: vaultAuthFinalizer, + want: false, + wantFinalizers: []string{}, + wantErr: func(t assert.TestingT, err error, _ ...interface{}) bool { + return assert.True(t, apierrors.IsNotFound(err)) + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := clientBuilder.Build() + var origResourceVersion string + if tt.create { + require.NoError(t, c.Create(ctx, tt.o)) + origResourceVersion = tt.o.GetResourceVersion() + } + + got, err := maybeAddFinalizer(ctx, c, tt.o, tt.finalizer) + if !tt.wantErr(t, err, fmt.Sprintf("maybeAddFinalizer(%v, %v, %v, %v)", ctx, c, tt.o, tt.finalizer)) { + return + } + + assert.Equalf(t, tt.want, got, "maybeAddFinalizer(%v, %v, %v, %v)", ctx, c, tt.o, tt.finalizer) + assert.Equalf(t, tt.wantFinalizers, tt.o.GetFinalizers(), "maybeAddFinalizer(%v, %v, %v, %v)", ctx, c, tt.o, tt.finalizer) + + if tt.create { + var updated secretsv1beta1.VaultAuth + if assert.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(tt.o), &updated)) { + if tt.want { + assert.NotEqual(t, origResourceVersion, tt.o.GetResourceVersion()) + } else { + // ensure that the object was not updated. + assert.Equal(t, origResourceVersion, tt.o.GetResourceVersion()) + } + } + } + }) + } +} + +// newClientBuilder copied from helpers. +func newClientBuilder() *fake.ClientBuilder { + scheme := runtime.NewScheme() + utilruntime.Must(clientgoscheme.AddToScheme(scheme)) + utilruntime.Must(secretsv1beta1.AddToScheme(scheme)) + return fake.NewClientBuilder().WithScheme(scheme) +} diff --git a/controllers/hcpvaultsecretsapp_controller.go b/controllers/hcpvaultsecretsapp_controller.go index c53e2700..7df74cb0 100644 --- a/controllers/hcpvaultsecretsapp_controller.go +++ b/controllers/hcpvaultsecretsapp_controller.go @@ -77,11 +77,7 @@ func (r *HCPVaultSecretsAppReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, err } - if o.GetDeletionTimestamp() == nil { - if err := r.addFinalizer(ctx, o); err != nil { - return ctrl.Result{}, err - } - } else { + if o.GetDeletionTimestamp() != nil { logger.Info("Got deletion timestamp", "obj", o) return ctrl.Result{}, r.handleDeletion(ctx, o) } @@ -185,8 +181,7 @@ func (r *HCPVaultSecretsAppReconciler) Reconcile(ctx context.Context, req ctrl.R r.Recorder.Event(o, corev1.EventTypeNormal, consts.ReasonSecretSync, "Secret sync not required") } - o.Status.LastGeneration = o.GetGeneration() - if err := r.Status().Update(ctx, o); err != nil { + if err := r.updateStatus(ctx, o); err != nil { return ctrl.Result{}, err } @@ -195,6 +190,17 @@ func (r *HCPVaultSecretsAppReconciler) Reconcile(ctx context.Context, req ctrl.R }, nil } +func (r *HCPVaultSecretsAppReconciler) updateStatus(ctx context.Context, o *secretsv1beta1.HCPVaultSecretsApp) error { + o.Status.LastGeneration = o.GetGeneration() + if err := r.Status().Update(ctx, o); err != nil { + r.Recorder.Eventf(o, corev1.EventTypeWarning, consts.ReasonStatusUpdateError, + "Failed to update the resource's status, err=%s", err) + } + + _, err := maybeAddFinalizer(ctx, r.Client, o, hcpVaultSecretsAppFinalizer) + return err +} + // SetupWithManager sets up the controller with the Manager. func (r *HCPVaultSecretsAppReconciler) SetupWithManager(mgr ctrl.Manager, opts controller.Options) error { r.referenceCache = newResourceReferenceCache() @@ -258,16 +264,6 @@ func (r *HCPVaultSecretsAppReconciler) hvsClient(ctx context.Context, o *secrets return hvsclient.New(cl, nil), nil } -func (r *HCPVaultSecretsAppReconciler) addFinalizer(ctx context.Context, o client.Object) error { - if !controllerutil.ContainsFinalizer(o, hcpVaultSecretsAppFinalizer) { - controllerutil.AddFinalizer(o, hcpVaultSecretsAppFinalizer) - if err := r.Client.Update(ctx, o); err != nil { - return err - } - } - return nil -} - func (r *HCPVaultSecretsAppReconciler) handleDeletion(ctx context.Context, o client.Object) error { logger := log.FromContext(ctx) r.referenceCache.Remove(SecretTransformation, client.ObjectKeyFromObject(o)) diff --git a/controllers/vaultauth_controller.go b/controllers/vaultauth_controller.go index ba92721b..987fd24e 100644 --- a/controllers/vaultauth_controller.go +++ b/controllers/vaultauth_controller.go @@ -62,11 +62,7 @@ func (r *VaultAuthReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, err } - if o.GetDeletionTimestamp() == nil { - if err := r.addFinalizer(ctx, o); err != nil { - return ctrl.Result{}, err - } - } else { + if o.GetDeletionTimestamp() != nil { logger.Info("Got deletion timestamp", "obj", o) return r.handleFinalizer(ctx, o) } @@ -131,25 +127,16 @@ func (r *VaultAuthReconciler) recordEvent(a *secretsv1beta1.VaultAuth, reason, m r.Recorder.Eventf(a, eventType, reason, msg, i...) } -func (r *VaultAuthReconciler) updateStatus(ctx context.Context, a *secretsv1beta1.VaultAuth) error { +func (r *VaultAuthReconciler) updateStatus(ctx context.Context, o *secretsv1beta1.VaultAuth) error { logger := log.FromContext(ctx) - metrics.SetResourceStatus("vaultauth", a, a.Status.Valid) - if err := r.Status().Update(ctx, a); err != nil { + metrics.SetResourceStatus("vaultauth", o, o.Status.Valid) + if err := r.Status().Update(ctx, o); err != nil { logger.Error(err, "Failed to update the resource's status") return err } - return nil -} - -func (r *VaultAuthReconciler) addFinalizer(ctx context.Context, o *secretsv1beta1.VaultAuth) error { - if !controllerutil.ContainsFinalizer(o, vaultAuthFinalizer) { - controllerutil.AddFinalizer(o, vaultAuthFinalizer) - if err := r.Client.Update(ctx, o); err != nil { - return err - } - } - return nil + _, err := maybeAddFinalizer(ctx, r.Client, o, vaultAuthFinalizer) + return err } func (r *VaultAuthReconciler) handleFinalizer(ctx context.Context, o *secretsv1beta1.VaultAuth) (ctrl.Result, error) { diff --git a/controllers/vaultconnection_controller.go b/controllers/vaultconnection_controller.go index 8388ea1c..a0191749 100644 --- a/controllers/vaultconnection_controller.go +++ b/controllers/vaultconnection_controller.go @@ -59,11 +59,7 @@ func (r *VaultConnectionReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, err } - if o.GetDeletionTimestamp() == nil { - if err := r.addFinalizer(ctx, o); err != nil { - return ctrl.Result{}, err - } - } else { + if o.GetDeletionTimestamp() != nil { logger.Info("Got deletion timestamp", "obj", o) return r.handleFinalizer(ctx, o) } @@ -124,17 +120,6 @@ func (r *VaultConnectionReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, nil } -func (r *VaultConnectionReconciler) addFinalizer(ctx context.Context, o *secretsv1beta1.VaultConnection) error { - if !controllerutil.ContainsFinalizer(o, vaultConnectionFinalizer) { - controllerutil.AddFinalizer(o, vaultConnectionFinalizer) - if err := r.Client.Update(ctx, o); err != nil { - return err - } - } - - return nil -} - func (r *VaultConnectionReconciler) updateStatus(ctx context.Context, o *secretsv1beta1.VaultConnection) error { logger := log.FromContext(ctx) metrics.SetResourceStatus("vaultconnection", o, o.Status.Valid) @@ -142,7 +127,9 @@ func (r *VaultConnectionReconciler) updateStatus(ctx context.Context, o *secrets logger.Error(err, "Failed to update the resource's status") return err } - return nil + + _, err := maybeAddFinalizer(ctx, r.Client, o, vaultConnectionFinalizer) + return err } func (r *VaultConnectionReconciler) handleFinalizer(ctx context.Context, o *secretsv1beta1.VaultConnection) (ctrl.Result, error) { diff --git a/controllers/vaultdynamicsecret_controller.go b/controllers/vaultdynamicsecret_controller.go index 8b2b181f..fd062ef5 100644 --- a/controllers/vaultdynamicsecret_controller.go +++ b/controllers/vaultdynamicsecret_controller.go @@ -102,11 +102,7 @@ func (r *VaultDynamicSecretReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, err } - if o.GetDeletionTimestamp() == nil { - if err := r.addFinalizer(ctx, o); err != nil { - return ctrl.Result{}, err - } - } else { + if o.GetDeletionTimestamp() != nil { logger.Info("Got deletion timestamp", "obj", o) return ctrl.Result{}, r.handleDeletion(ctx, o) } @@ -242,7 +238,6 @@ func (r *VaultDynamicSecretReconciler) Reconcile(ctx context.Context, req ctrl.R doRolloutRestart := (doSync && o.Status.LastGeneration > 1) || staticCredsUpdated o.Status.SecretLease = *secretLease o.Status.LastRenewalTime = nowFunc().Unix() - o.Status.LastGeneration = o.GetGeneration() if err := r.updateStatus(ctx, o); err != nil { return ctrl.Result{}, err } @@ -402,12 +397,15 @@ func (r *VaultDynamicSecretReconciler) updateStatus(ctx context.Context, o *secr if r.runtimePodUID != "" { o.Status.LastRuntimePodUID = r.runtimePodUID } + + o.Status.LastGeneration = o.GetGeneration() if err := r.Status().Update(ctx, o); err != nil { r.Recorder.Eventf(o, corev1.EventTypeWarning, consts.ReasonStatusUpdateError, "Failed to update the resource's status, err=%s", err) } - return nil + _, err := maybeAddFinalizer(ctx, r.Client, o, vaultDynamicSecretFinalizer) + return err } func (r *VaultDynamicSecretReconciler) getVaultSecretLease(resp *api.Secret) *secretsv1beta1.VaultSecretLease { @@ -442,16 +440,6 @@ func (r *VaultDynamicSecretReconciler) renewLease( return r.getVaultSecretLease(resp.Secret()), nil } -func (r *VaultDynamicSecretReconciler) addFinalizer(ctx context.Context, o *secretsv1beta1.VaultDynamicSecret) error { - if !controllerutil.ContainsFinalizer(o, vaultDynamicSecretFinalizer) { - controllerutil.AddFinalizer(o, vaultDynamicSecretFinalizer) - if err := r.Client.Update(ctx, o); err != nil { - return err - } - } - return nil -} - // SetupWithManager sets up the controller with the Manager. func (r *VaultDynamicSecretReconciler) SetupWithManager(mgr ctrl.Manager, opts controller.Options) error { r.referenceCache = newResourceReferenceCache() diff --git a/controllers/vaultpkisecret_controller.go b/controllers/vaultpkisecret_controller.go index 1c417ff9..eadd4007 100644 --- a/controllers/vaultpkisecret_controller.go +++ b/controllers/vaultpkisecret_controller.go @@ -77,11 +77,7 @@ func (r *VaultPKISecretReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, err } - if o.GetDeletionTimestamp() == nil { - if err := r.addFinalizer(ctx, o); err != nil { - return ctrl.Result{}, err - } - } else { + if o.GetDeletionTimestamp() != nil { logger.Info("Got deletion timestamp", "obj", o) return ctrl.Result{}, r.handleDeletion(ctx, o) } @@ -299,7 +295,6 @@ func (r *VaultPKISecretReconciler) Reconcile(ctx context.Context, req ctrl.Reque o.Status.SerialNumber = certResp.SerialNumber o.Status.Expiration = certResp.Expiration o.Status.LastRotation = time.Now().Unix() - o.Status.LastGeneration = o.GetGeneration() if err := r.updateStatus(ctx, o); err != nil { logger.Error(err, "Failed to update the status") return ctrl.Result{}, err @@ -343,22 +338,6 @@ func (r *VaultPKISecretReconciler) handleDeletion(ctx context.Context, o *secret return nil } -func (r *VaultPKISecretReconciler) addFinalizer(ctx context.Context, s *secretsv1beta1.VaultPKISecret) error { - logger := log.FromContext(ctx).WithValues("finalizer", vaultPKIFinalizer) - if !controllerutil.ContainsFinalizer(s, vaultPKIFinalizer) { - controllerutil.AddFinalizer(s, vaultPKIFinalizer) - logger.V(consts.LogLevelDebug).Info("Adding finalizer") - if err := r.Client.Update(ctx, s); err != nil { - logger.Error(err, "Adding finalizer") - return err - } - return nil - } else { - logger.V(consts.LogLevelDebug).Info("Finalizer already added") - return nil - } -} - func (r *VaultPKISecretReconciler) SetupWithManager(mgr ctrl.Manager, opts controller.Options) error { r.referenceCache = newResourceReferenceCache() return ctrl.NewControllerManagedBy(mgr). @@ -441,7 +420,10 @@ func (r *VaultPKISecretReconciler) recordEvent(p *secretsv1beta1.VaultPKISecret, func (r *VaultPKISecretReconciler) updateStatus(ctx context.Context, o *secretsv1beta1.VaultPKISecret) error { logger := log.FromContext(ctx) logger.V(consts.LogLevelTrace).Info("Update status called") + metrics.SetResourceStatus("vaultpkisecret", o, o.Status.Valid) + + o.Status.LastGeneration = o.GetGeneration() if err := r.Status().Update(ctx, o); err != nil { msg := "Failed to update the resource's status" r.recordEvent(o, consts.ReasonStatusUpdateError, "%s: %s", msg, err) @@ -449,7 +431,8 @@ func (r *VaultPKISecretReconciler) updateStatus(ctx context.Context, o *secretsv return err } - return nil + _, err := maybeAddFinalizer(ctx, r.Client, o, vaultPKIFinalizer) + return err } func computeExpirationTimePKI(o *secretsv1beta1.VaultPKISecret, offset int64) time.Time { diff --git a/controllers/vaultstaticsecret_controller.go b/controllers/vaultstaticsecret_controller.go index c3be7973..d2a8cd86 100644 --- a/controllers/vaultstaticsecret_controller.go +++ b/controllers/vaultstaticsecret_controller.go @@ -65,11 +65,7 @@ func (r *VaultStaticSecretReconciler) Reconcile(ctx context.Context, req ctrl.Re return ctrl.Result{}, err } - if o.GetDeletionTimestamp() == nil { - if err := r.addFinalizer(ctx, o); err != nil { - return ctrl.Result{}, err - } - } else { + if o.GetDeletionTimestamp() != nil { logger.Info("Got deletion timestamp", "obj", o) return ctrl.Result{}, r.handleDeletion(ctx, o) } @@ -173,8 +169,7 @@ func (r *VaultStaticSecretReconciler) Reconcile(ctx context.Context, req ctrl.Re logger.V(consts.LogLevelDebug).Info("Secret sync not required") } - o.Status.LastGeneration = o.GetGeneration() - if err := r.Status().Update(ctx, o); err != nil { + if err := r.updateStatus(ctx, o); err != nil { return ctrl.Result{}, err } @@ -183,14 +178,17 @@ func (r *VaultStaticSecretReconciler) Reconcile(ctx context.Context, req ctrl.Re }, nil } -func (r *VaultStaticSecretReconciler) addFinalizer(ctx context.Context, o client.Object) error { - if !controllerutil.ContainsFinalizer(o, vaultStaticSecretFinalizer) { - controllerutil.AddFinalizer(o, vaultStaticSecretFinalizer) - if err := r.Client.Update(ctx, o); err != nil { - return err - } +func (r *VaultStaticSecretReconciler) updateStatus(ctx context.Context, o *secretsv1beta1.VaultStaticSecret) error { + logger := log.FromContext(ctx) + logger.V(consts.LogLevelDebug).Info("Updating status") + o.Status.LastGeneration = o.GetGeneration() + if err := r.Status().Update(ctx, o); err != nil { + r.Recorder.Eventf(o, corev1.EventTypeWarning, consts.ReasonStatusUpdateError, + "Failed to update the resource's status, err=%s", err) } - return nil + + _, err := maybeAddFinalizer(ctx, r.Client, o, vaultStaticSecretFinalizer) + return err } func (r *VaultStaticSecretReconciler) handleDeletion(ctx context.Context, o client.Object) error {