Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use exponential backoffs on secret source errors. #732

Merged
merged 5 commits into from
May 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 24 additions & 0 deletions chart/templates/_helpers.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -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 -}}
3 changes: 3 additions & 0 deletions chart/templates/deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Expand Down
16 changes: 16 additions & 0 deletions chart/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
14 changes: 12 additions & 2 deletions controllers/hcpvaultsecretsapp_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)).
Expand Down Expand Up @@ -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) {
Expand Down
68 changes: 68 additions & 0 deletions controllers/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ package controllers

import (
"sync"
"time"

"github.com/cenkalti/backoff/v4"
"sigs.k8s.io/controller-runtime/pkg/client"
)

Expand Down Expand Up @@ -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,
}
}
117 changes: 117 additions & 0 deletions controllers/registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
})
}
}