Use exponential backoffs on secret source errors. (#732)

Previously, the back off duration was based on a fixed duration + some
jitter. This PR introduces exponential back offs for all secret syncing
controllers. The back off will be calculated and honored whenever an
error is encountered while fetching from a secret source e.g: Vault,
HCPVS.

- add new metric runtime configuration gauge
- unify sync reasons between VDS and VPS controllers

Author: benashz

chart/templates/_helpers.tpl

@@ -175,3 +175,27 @@ globalTransformationOptions configures the manager's --global-transformation-opt
 {{- $opts | join "," -}}
 {{- end -}}
 {{- end -}}
+
+
+{{/*
+backOffOnSecretSourceError provides the back-off options for the manager when a
+secret source error occurs.
+*/}}
+{{- define "vso.backOffOnSecretSourceError" -}}
+{{- $opts := list -}}
+{{- with .Values.controller.manager.backOffOnSecretSourceError -}}
+{{- with .initialInterval -}}
+{{- $opts = mustAppend $opts (printf "--back-off-initial-interval=%s" .) -}}
+{{- end -}}
+{{- with .maxInterval -}}
+{{- $opts = mustAppend $opts (printf "--back-off-max-interval=%s" .) -}}
+{{- end -}}
+{{- with .multiplier -}}
+{{- $opts = mustAppend $opts (printf "--back-off-multiplier=%.2f"  (. | float64)) -}}
+{{- end -}}
+{{- with .randomizationFactor -}}
+{{- $opts = mustAppend $opts (printf "--back-off-randomization-factor=%.2f" (. | float64)) -}}
+{{- end -}}
+{{- $opts | toYaml | nindent 8 -}}
+{{- end -}}
+{{- end -}}

chart/templates/deployment.yaml

@@ -78,6 +78,9 @@ spec:
         {{- if $opts }}
         - --global-transformation-options={{ $opts }}
         {{- end }}
+        {{- with include "vso.backOffOnSecretSourceError" . }}
+        {{- . -}}
+        {{- end }}
         {{- if .Values.controller.manager.extraArgs }}
         {{- toYaml .Values.controller.manager.extraArgs | nindent 8 }}
         {{- end }}

chart/values.yaml

@@ -113,6 +113,22 @@ controller:
       # in the destination K8s Secret.
       excludeRaw: false
 
+    # Backoff settings for the controller manager. These settings control the backoff behavior
+    # when the controller encounters an error while fetching secrets from the SecretSource.
+    backOffOnSecretSourceError:
+      # Initial interval between retries.
+      # @type: duration
+      initialInterval: "5s"
+      # Maximum interval between retries.
+      # @type: duration
+      maxInterval: "60s"
+      # Randomization factor to add jitter to the interval between retries.
+      # @type: float
+      randomizationFactor: 0.5
+      # Sets the multiplier for increasing the interval between retries.
+      # @type: float
+      multiplier: 1.5
+
     # Configures the client cache which is used by the controller to cache (and potentially persist) vault tokens that
     # are the result of using the VaultAuthMethod. This enables re-use of Vault Tokens
     # throughout their TTLs as well as the ability to renew.

controllers/hcpvaultsecretsapp_controller.go

@@ -54,6 +54,7 @@ type HCPVaultSecretsAppReconciler struct {
 	MinRefreshAfter            time.Duration
 	referenceCache             ResourceReferenceCache
 	GlobalTransformationOption *helpers.GlobalTransformationOption
+	BackOffRegistry            *BackOffRegistry
 }
 
 //+kubebuilder:rbac:groups=secrets.hashicorp.com,resources=hcpvaultsecretsapps,verbs=get;list;watch;create;update;patch;delete
@@ -126,9 +127,12 @@ func (r *HCPVaultSecretsAppReconciler) Reconcile(ctx context.Context, req ctrl.R
 	resp, err := c.OpenAppSecrets(params, nil)
 	if err != nil {
 		logger.Error(err, "Get App Secret", "appName", o.Spec.AppName)
+		entry, _ := r.BackOffRegistry.Get(req.NamespacedName)
 		return ctrl.Result{
-			RequeueAfter: computeHorizonWithJitter(requeueDurationOnError),
+			RequeueAfter: entry.NextBackOff(),
 		}, nil
+	} else {
+		r.BackOffRegistry.Delete(req.NamespacedName)
 	}
 
 	r.referenceCache.Set(SecretTransformation, req.NamespacedName,
@@ -211,6 +215,10 @@ func (r *HCPVaultSecretsAppReconciler) updateStatus(ctx context.Context, o *secr
 // SetupWithManager sets up the controller with the Manager.
 func (r *HCPVaultSecretsAppReconciler) SetupWithManager(mgr ctrl.Manager, opts controller.Options) error {
 	r.referenceCache = newResourceReferenceCache()
+	if r.BackOffRegistry == nil {
+		r.BackOffRegistry = NewBackOffRegistry()
+	}
+
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&secretsv1beta1.HCPVaultSecretsApp{}).
 		WithEventFilter(syncableSecretPredicate(nil)).
@@ -273,7 +281,9 @@ func (r *HCPVaultSecretsAppReconciler) hvsClient(ctx context.Context, o *secrets
 
 func (r *HCPVaultSecretsAppReconciler) handleDeletion(ctx context.Context, o client.Object) error {
 	logger := log.FromContext(ctx)
-	r.referenceCache.Remove(SecretTransformation, client.ObjectKeyFromObject(o))
+	objKey := client.ObjectKeyFromObject(o)
+	r.referenceCache.Remove(SecretTransformation, objKey)
+	r.BackOffRegistry.Delete(objKey)
 	if controllerutil.ContainsFinalizer(o, hcpVaultSecretsAppFinalizer) {
 		logger.Info("Removing finalizer")
 		if controllerutil.RemoveFinalizer(o, hcpVaultSecretsAppFinalizer) {

controllers/registry.go

@@ -5,7 +5,9 @@ package controllers
 
 import (
 	"sync"
+	"time"
 
+	"github.com/cenkalti/backoff/v4"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -227,3 +229,69 @@ func (r *SyncRegistry) ObjectKeys() []client.ObjectKey {
 
 	return result
 }
+
+// BackOffRegistry is a registry that stores sync backoff for a client.Object.
+type BackOffRegistry struct {
+	m    map[client.ObjectKey]*BackOff
+	mu   sync.RWMutex
+	opts []backoff.ExponentialBackOffOpts
+}
+
+// Delete objKey to the set of registered objects.
+func (r *BackOffRegistry) Delete(objKey client.ObjectKey) bool {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+
+	_, ok := r.m[objKey]
+	delete(r.m, objKey)
+	return ok
+}
+
+// Get is a getter/setter that returns the BackOff for objKey.
+// If objKey is not in the set of registered objects, it will be added. Return
+// true if the sync backoff entry was created.
+func (r *BackOffRegistry) Get(objKey client.ObjectKey) (*BackOff, bool) {
+	r.mu.RLock()
+	defer r.mu.RUnlock()
+
+	entry, ok := r.m[objKey]
+	if !ok {
+		entry = &BackOff{
+			bo: backoff.NewExponentialBackOff(r.opts...),
+		}
+		r.m[objKey] = entry
+	}
+
+	return entry, !ok
+}
+
+// BackOff is a wrapper around backoff.BackOff that does not implement
+// BackOff.Reset, since elements in BackOffRegistry are meant to be ephemeral.
+type BackOff struct {
+	bo backoff.BackOff
+}
+
+// NextBackOff returns the next backoff duration.
+func (s *BackOff) NextBackOff() time.Duration {
+	return s.bo.NextBackOff()
+}
+
+// DefaultExponentialBackOffOpts returns the default exponential options for the
+func DefaultExponentialBackOffOpts() []backoff.ExponentialBackOffOpts {
+	return []backoff.ExponentialBackOffOpts{
+		backoff.WithInitialInterval(requeueDurationOnError),
+		backoff.WithMaxInterval(time.Second * 60),
+	}
+}
+
+// NewBackOffRegistry returns a BackOffRegistry.
+func NewBackOffRegistry(opts ...backoff.ExponentialBackOffOpts) *BackOffRegistry {
+	if len(opts) == 0 {
+		opts = DefaultExponentialBackOffOpts()
+	}
+
+	return &BackOffRegistry{
+		m:    map[client.ObjectKey]*BackOff{},
+		opts: opts,
+	}
+}

controllers/registry_test.go

@@ -6,7 +6,9 @@ package controllers
 import (
 	"sync"
 	"testing"
+	"time"
 
+	"github.com/cenkalti/backoff/v4"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -503,3 +505,118 @@ func TestSyncRegistry(t *testing.T) {
 		})
 	}
 }
+
+func TestBackOffRegistry_Get(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name   string
+		m      map[client.ObjectKey]*BackOff
+		opts   []backoff.ExponentialBackOffOpts
+		objKey client.ObjectKey
+		want   *BackOff
+		want1  bool
+	}{
+		{
+			name: "new",
+			m:    map[client.ObjectKey]*BackOff{},
+			objKey: client.ObjectKey{
+				Namespace: "foo",
+				Name:      "bar",
+			},
+			want: &BackOff{
+				bo: backoff.NewExponentialBackOff(
+					DefaultExponentialBackOffOpts()...,
+				),
+			},
+			want1: true,
+		},
+		{
+			name: "previous",
+			m: map[client.ObjectKey]*BackOff{
+				{
+					Namespace: "foo",
+					Name:      "bar",
+				}: {
+					bo: backoff.NewExponentialBackOff(
+						DefaultExponentialBackOffOpts()...,
+					),
+				},
+			},
+			objKey: client.ObjectKey{
+				Namespace: "foo",
+				Name:      "bar",
+			},
+			want: &BackOff{
+				bo: backoff.NewExponentialBackOff(
+					DefaultExponentialBackOffOpts()...,
+				),
+			},
+			want1: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			r := &BackOffRegistry{
+				m:    tt.m,
+				opts: tt.opts,
+			}
+			got, got1 := r.Get(tt.objKey)
+			assert.NotNilf(t, got, "Get(%v)", tt.objKey)
+			assert.Equalf(t, tt.want1, got1, "Get(%v)", tt.objKey)
+			last := got.bo.NextBackOff()
+			assert.Greaterf(t, last, time.Duration(0), "Get(%v)", tt.objKey)
+			assert.Greaterf(t, got.bo.NextBackOff(), last, "Get(%v)", tt.objKey)
+		})
+	}
+}
+
+func TestBackOffRegistry_Delete(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name   string
+		m      map[client.ObjectKey]*BackOff
+		opts   []backoff.ExponentialBackOffOpts
+		objKey client.ObjectKey
+		want   bool
+	}{
+		{
+			name: "not-found",
+			m:    map[client.ObjectKey]*BackOff{},
+			objKey: client.ObjectKey{
+				Namespace: "foo",
+				Name:      "bar",
+			},
+			want: false,
+		},
+		{
+			name: "deleted",
+			m: map[client.ObjectKey]*BackOff{
+				{
+					Namespace: "foo",
+					Name:      "bar",
+				}: {
+					bo: backoff.NewExponentialBackOff(
+						DefaultExponentialBackOffOpts()...,
+					),
+				},
+			},
+			objKey: client.ObjectKey{
+				Namespace: "foo",
+				Name:      "bar",
+			},
+			want: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			r := &BackOffRegistry{
+				m:    tt.m,
+				opts: tt.opts,
+			}
+			got := r.Delete(tt.objKey)
+			assert.Equalf(t, tt.want, got, "Delete(%v)", tt.objKey)
+		})
+	}
+}