Skip to content

Commit

Permalink
Always add finalizers after status updates (#609)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@benashz
benashz authored Feb 20, 2024
1 parent 6170409 commit e8ea84e
Show file tree
Hide file tree
Showing 8 changed files with 250 additions and 107 deletions.
26 changes: 26 additions & 0 deletions controllers/common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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
}
178 changes: 178 additions & 0 deletions controllers/common_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
}
30 changes: 13 additions & 17 deletions controllers/hcpvaultsecretsapp_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
}

Expand All @@ -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()
Expand Down Expand Up @@ -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))
Expand Down
25 changes: 6 additions & 19 deletions controllers/vaultauth_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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) {
Expand Down
21 changes: 4 additions & 17 deletions controllers/vaultconnection_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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) {
Expand Down
Loading

0 comments on commit e8ea84e

Please sign in to comment.