Always add finalizers after status updates (#609)

Make updating a resource's status more consistent for all secret
controllers, by factoring out the Status.Update() calls to controller
methods, and always setting the lastGeneraton from the reconciled
object.

Always call addFinalizer() after updateStatus() to avoid schema 
validation errors.

Author: benashz

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
+}

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)
+}

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))

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) {

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,25 +120,16 @@ 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)
 	if err := r.Status().Update(ctx, o); err != nil {
 		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) {