fix: retry failed reconciles much less aggressively (#4339)

Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>

cmd/controller/certcontroller.go

@@ -35,6 +35,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 
 	"github.com/external-secrets/external-secrets/pkg/constants"
+	ctrlcommon "github.com/external-secrets/external-secrets/pkg/controllers/common"
 	"github.com/external-secrets/external-secrets/pkg/controllers/crds"
 	"github.com/external-secrets/external-secrets/pkg/controllers/webhookconfig"
 )
@@ -108,6 +109,7 @@ var certcontrollerCmd = &cobra.Command{
 			})
 		if err := crdctrl.SetupWithManager(mgr, controller.Options{
 			MaxConcurrentReconciles: concurrent,
+			RateLimiter:             ctrlcommon.BuildRateLimiter(),
 		}); err != nil {
 			setupLog.Error(err, errCreateController, "controller", "CustomResourceDefinition")
 			os.Exit(1)
@@ -124,6 +126,7 @@ var certcontrollerCmd = &cobra.Command{
 			})
 		if err := whc.SetupWithManager(mgr, controller.Options{
 			MaxConcurrentReconciles: concurrent,
+			RateLimiter:             ctrlcommon.BuildRateLimiter(),
 		}); err != nil {
 			setupLog.Error(err, errCreateController, "controller", "WebhookConfig")
 			os.Exit(1)

cmd/controller/root.go

@@ -38,6 +38,7 @@ import (
 	genv1alpha1 "github.com/external-secrets/external-secrets/apis/generators/v1alpha1"
 	"github.com/external-secrets/external-secrets/pkg/controllers/clusterexternalsecret"
 	"github.com/external-secrets/external-secrets/pkg/controllers/clusterexternalsecret/cesmetrics"
+	ctrlcommon "github.com/external-secrets/external-secrets/pkg/controllers/common"
 	"github.com/external-secrets/external-secrets/pkg/controllers/externalsecret"
 	"github.com/external-secrets/external-secrets/pkg/controllers/externalsecret/esmetrics"
 	ctrlmetrics "github.com/external-secrets/external-secrets/pkg/controllers/metrics"
@@ -163,7 +164,7 @@ var rootCmd = &cobra.Command{
 		// if we are already caching all secrets, we don't need to use the special client.
 		secretClient := mgr.GetClient()
 		if enableManagedSecretsCache && !enableSecretsCache {
-			secretClient, err = externalsecret.BuildManagedSecretClient(mgr)
+			secretClient, err = ctrlcommon.BuildManagedSecretClient(mgr)
 			if err != nil {
 				setupLog.Error(err, "unable to create managed secret client")
 				os.Exit(1)
@@ -179,6 +180,7 @@ var rootCmd = &cobra.Command{
 			RequeueInterval: storeRequeueInterval,
 		}).SetupWithManager(mgr, controller.Options{
 			MaxConcurrentReconciles: concurrent,
+			RateLimiter:             ctrlcommon.BuildRateLimiter(),
 		}); err != nil {
 			setupLog.Error(err, errCreateController, "controller", "SecretStore")
 			os.Exit(1)
@@ -193,6 +195,7 @@ var rootCmd = &cobra.Command{
 				RequeueInterval: storeRequeueInterval,
 			}).SetupWithManager(mgr, controller.Options{
 				MaxConcurrentReconciles: concurrent,
+				RateLimiter:             ctrlcommon.BuildRateLimiter(),
 			}); err != nil {
 				setupLog.Error(err, errCreateController, "controller", "ClusterSecretStore")
 				os.Exit(1)
@@ -210,6 +213,7 @@ var rootCmd = &cobra.Command{
 			EnableFloodGate:           enableFloodGate,
 		}).SetupWithManager(mgr, controller.Options{
 			MaxConcurrentReconciles: concurrent,
+			RateLimiter:             ctrlcommon.BuildRateLimiter(),
 		}); err != nil {
 			setupLog.Error(err, errCreateController, "controller", "ExternalSecret")
 			os.Exit(1)
@@ -225,6 +229,7 @@ var rootCmd = &cobra.Command{
 				RequeueInterval: time.Hour,
 			}).SetupWithManager(mgr, controller.Options{
 				MaxConcurrentReconciles: concurrent,
+				RateLimiter:             ctrlcommon.BuildRateLimiter(),
 			}); err != nil {
 				setupLog.Error(err, errCreateController, "controller", "PushSecret")
 				os.Exit(1)
@@ -240,6 +245,7 @@ var rootCmd = &cobra.Command{
 				RequeueInterval: time.Hour,
 			}).SetupWithManager(mgr, controller.Options{
 				MaxConcurrentReconciles: concurrent,
+				RateLimiter:             ctrlcommon.BuildRateLimiter(),
 			}); err != nil {
 				setupLog.Error(err, errCreateController, "controller", "ClusterExternalSecret")
 				os.Exit(1)

go.mod

@@ -253,7 +253,7 @@ require (
 	golang.org/x/sys v0.29.0 // indirect
 	golang.org/x/term v0.28.0 // indirect
 	golang.org/x/text v0.21.0 // indirect
-	golang.org/x/time v0.9.0 // indirect
+	golang.org/x/time v0.9.0
 	golang.org/x/tools v0.29.0 // indirect
 	gomodules.xyz/jsonpatch/v2 v2.4.0 // indirect
 	google.golang.org/protobuf v1.36.2 // indirect

pkg/controllers/clusterexternalsecret/suite_test.go

@@ -31,6 +31,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/metrics/server"
 
 	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
+	ctrlcommon "github.com/external-secrets/external-secrets/pkg/controllers/common"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -90,6 +91,7 @@ var _ = BeforeSuite(func() {
 		RequeueInterval: time.Second,
 	}).SetupWithManager(k8sManager, controller.Options{
 		MaxConcurrentReconciles: 1,
+		RateLimiter:             ctrlcommon.BuildRateLimiter(),
 	})
 	Expect(err).ToNot(HaveOccurred())
 

pkg/controllers/common/common.go

@@ -0,0 +1,112 @@
+/*
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package common
+
+import (
+	"context"
+	"time"
+
+	"golang.org/x/time/rate"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/selection"
+	"k8s.io/client-go/util/workqueue"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/cache"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+
+	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
+)
+
+// BuildManagedSecretClient creates a new client that only sees secrets with the "managed" label.
+func BuildManagedSecretClient(mgr ctrl.Manager) (client.Client, error) {
+	// secrets we manage will have the `reconcile.external-secrets.io/managed=true` label
+	managedLabelReq, _ := labels.NewRequirement(esv1beta1.LabelManaged, selection.Equals, []string{esv1beta1.LabelManagedValue})
+	managedLabelSelector := labels.NewSelector().Add(*managedLabelReq)
+
+	// create a new cache with a label selector for managed secrets
+	// NOTE: this means that the cache/client will be unable to see secrets without the "managed" label
+	secretCacheOpts := cache.Options{
+		HTTPClient: mgr.GetHTTPClient(),
+		Scheme:     mgr.GetScheme(),
+		Mapper:     mgr.GetRESTMapper(),
+		ByObject: map[client.Object]cache.ByObject{
+			&corev1.Secret{}: {
+				Label: managedLabelSelector,
+			},
+		},
+		// this requires us to explicitly start an informer for each object type
+		// and helps avoid people mistakenly using the secret client for other resources
+		ReaderFailOnMissingInformer: true,
+	}
+	secretCache, err := cache.New(mgr.GetConfig(), secretCacheOpts)
+	if err != nil {
+		return nil, err
+	}
+
+	// start an informer for secrets
+	// this is required because we set ReaderFailOnMissingInformer to true
+	_, err = secretCache.GetInformer(context.Background(), &corev1.Secret{})
+	if err != nil {
+		return nil, err
+	}
+
+	// add the secret cache to the manager, so that it starts at the same time
+	err = mgr.Add(secretCache)
+	if err != nil {
+		return nil, err
+	}
+
+	// create a new client that uses the secret cache
+	secretClient, err := client.New(mgr.GetConfig(), client.Options{
+		HTTPClient: mgr.GetHTTPClient(),
+		Scheme:     mgr.GetScheme(),
+		Mapper:     mgr.GetRESTMapper(),
+		Cache: &client.CacheOptions{
+			Reader: secretCache,
+		},
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	return secretClient, nil
+}
+
+// BuildRateLimiter creates a new rate limiter for our controllers.
+// NOTE: we dont use `DefaultTypedControllerRateLimiter` because it retries very aggressively, starting at 5ms!
+func BuildRateLimiter() workqueue.TypedRateLimiter[reconcile.Request] {
+	// exponential backoff rate limiter
+	//  - this handles per-item rate limiting for ~failures~
+	//  - it uses an exponential backoff strategy were: delay = baseDelay * 2^failures
+	//  - graph visualization: https://www.desmos.com/calculator/fexlpdmiti
+	failureBaseDelay := 1 * time.Second
+	failureMaxDelay := 7 * time.Minute
+	failureRateLimiter := workqueue.NewTypedItemExponentialFailureRateLimiter[reconcile.Request](failureBaseDelay, failureMaxDelay)
+
+	// overall rate limiter
+	//  - this handles overall rate limiting, ignoring individual items and only considering the overall rate
+	//  - it implements a "token bucket" of size totalMaxBurst that is initially full,
+	//    and which is refilled at rate totalEventsPerSecond tokens per second.
+	totalEventsPerSecond := 10
+	totalMaxBurst := 100
+	totalRateLimiter := &workqueue.TypedBucketRateLimiter[reconcile.Request]{
+		Limiter: rate.NewLimiter(rate.Limit(totalEventsPerSecond), totalMaxBurst),
+	}
+
+	// return the worst-case (longest) of the rate limiters for a given item
+	return workqueue.NewTypedMaxOfRateLimiter[reconcile.Request](failureRateLimiter, totalRateLimiter)
+}

pkg/controllers/crds/suite_test.go

@@ -31,6 +31,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/metrics/server"
 
 	esapi "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
+	ctrlcommon "github.com/external-secrets/external-secrets/pkg/controllers/common"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -90,7 +91,10 @@ var _ = BeforeSuite(func() {
 				"secretstores.test.io",
 			},
 		})
-	rec.SetupWithManager(k8sManager, controller.Options{})
+	err = rec.SetupWithManager(k8sManager, controller.Options{
+		MaxConcurrentReconciles: 1,
+		RateLimiter:             ctrlcommon.BuildRateLimiter(),
+	})
 	Expect(err).ToNot(HaveOccurred())
 
 	go func() {

pkg/controllers/externalsecret/externalsecret_controller.go

@@ -34,14 +34,12 @@ import (
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/apimachinery/pkg/selection"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
-	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -976,57 +974,3 @@ func (r *Reconciler) findObjectsForSecret(ctx context.Context, secret client.Obj
 	}
 	return requests
 }
-
-func BuildManagedSecretClient(mgr ctrl.Manager) (client.Client, error) {
-	// secrets we manage will have the `reconcile.external-secrets.io/managed=true` label
-	managedLabelReq, _ := labels.NewRequirement(esv1beta1.LabelManaged, selection.Equals, []string{esv1beta1.LabelManagedValue})
-	managedLabelSelector := labels.NewSelector().Add(*managedLabelReq)
-
-	// create a new cache with a label selector for managed secrets
-	// NOTE: this means that the cache/client will be unable to see secrets without the "managed" label
-	secretCacheOpts := cache.Options{
-		HTTPClient: mgr.GetHTTPClient(),
-		Scheme:     mgr.GetScheme(),
-		Mapper:     mgr.GetRESTMapper(),
-		ByObject: map[client.Object]cache.ByObject{
-			&v1.Secret{}: {
-				Label: managedLabelSelector,
-			},
-		},
-		// this requires us to explicitly start an informer for each object type
-		// and helps avoid people mistakenly using the secret client for other resources
-		ReaderFailOnMissingInformer: true,
-	}
-	secretCache, err := cache.New(mgr.GetConfig(), secretCacheOpts)
-	if err != nil {
-		return nil, err
-	}
-
-	// start an informer for secrets
-	// this is required because we set ReaderFailOnMissingInformer to true
-	_, err = secretCache.GetInformer(context.Background(), &v1.Secret{})
-	if err != nil {
-		return nil, err
-	}
-
-	// add the secret cache to the manager, so that it starts at the same time
-	err = mgr.Add(secretCache)
-	if err != nil {
-		return nil, err
-	}
-
-	// create a new client that uses the secret cache
-	secretClient, err := client.New(mgr.GetConfig(), client.Options{
-		HTTPClient: mgr.GetHTTPClient(),
-		Scheme:     mgr.GetScheme(),
-		Mapper:     mgr.GetRESTMapper(),
-		Cache: &client.CacheOptions{
-			Reader: secretCache,
-		},
-	})
-	if err != nil {
-		return nil, err
-	}
-
-	return secretClient, nil
-}

pkg/controllers/externalsecret/suite_test.go

@@ -34,6 +34,7 @@ import (
 
 	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
 	genv1alpha1 "github.com/external-secrets/external-secrets/apis/generators/v1alpha1"
+	ctrlcommon "github.com/external-secrets/external-secrets/pkg/controllers/common"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -103,7 +104,7 @@ var _ = BeforeSuite(func() {
 
 	// by default, we use a separate cached client for secrets that are managed by the controller
 	// so we should test under the same conditions
-	secretClient, err := BuildManagedSecretClient(k8sManager)
+	secretClient, err := ctrlcommon.BuildManagedSecretClient(k8sManager)
 	Expect(err).ToNot(HaveOccurred())
 
 	err = (&Reconciler{
@@ -116,6 +117,7 @@ var _ = BeforeSuite(func() {
 		ClusterSecretStoreEnabled: true,
 	}).SetupWithManager(k8sManager, controller.Options{
 		MaxConcurrentReconciles: 1,
+		RateLimiter:             ctrlcommon.BuildRateLimiter(),
 	})
 	Expect(err).ToNot(HaveOccurred())
 

pkg/controllers/pushsecret/suite_test.go

@@ -34,6 +34,7 @@ import (
 	esv1alpha1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1alpha1"
 	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
 	genv1alpha1 "github.com/external-secrets/external-secrets/apis/generators/v1alpha1"
+	ctrlcommon "github.com/external-secrets/external-secrets/pkg/controllers/common"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -99,6 +100,7 @@ var _ = BeforeSuite(func() {
 		RequeueInterval: time.Second,
 	}).SetupWithManager(k8sManager, controller.Options{
 		MaxConcurrentReconciles: 1,
+		RateLimiter:             ctrlcommon.BuildRateLimiter(),
 	})
 	Expect(err).ToNot(HaveOccurred())
 

pkg/controllers/secretstore/suite_test.go

@@ -30,6 +30,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/metrics/server"
 
 	esapi "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
+	ctrlcommon "github.com/external-secrets/external-secrets/pkg/controllers/common"
 	ctrlmetrics "github.com/external-secrets/external-secrets/pkg/controllers/metrics"
 	"github.com/external-secrets/external-secrets/pkg/controllers/secretstore/cssmetrics"
 	"github.com/external-secrets/external-secrets/pkg/controllers/secretstore/ssmetrics"
@@ -85,7 +86,10 @@ var _ = BeforeSuite(func() {
 		Scheme:          k8sManager.GetScheme(),
 		Log:             ctrl.Log.WithName("controllers").WithName("SecretStore"),
 		ControllerClass: defaultControllerClass,
-	}).SetupWithManager(k8sManager, controller.Options{})
+	}).SetupWithManager(k8sManager, controller.Options{
+		MaxConcurrentReconciles: 1,
+		RateLimiter:             ctrlcommon.BuildRateLimiter(),
+	})
 	Expect(err).ToNot(HaveOccurred())
 
 	err = (&ClusterStoreReconciler{
@@ -93,7 +97,10 @@ var _ = BeforeSuite(func() {
 		Scheme:          k8sManager.GetScheme(),
 		ControllerClass: defaultControllerClass,
 		Log:             ctrl.Log.WithName("controllers").WithName("ClusterSecretStore"),
-	}).SetupWithManager(k8sManager, controller.Options{})
+	}).SetupWithManager(k8sManager, controller.Options{
+		MaxConcurrentReconciles: 1,
+		RateLimiter:             ctrlcommon.BuildRateLimiter(),
+	})
 	Expect(err).ToNot(HaveOccurred())
 
 	go func() {

pkg/controllers/webhookconfig/suite_test.go

@@ -31,6 +31,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/metrics/server"
 
 	esapi "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
+	ctrlcommon "github.com/external-secrets/external-secrets/pkg/controllers/common"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -94,7 +95,10 @@ var _ = BeforeSuite(func() {
 		SecretNamespace: ctrlSecretNamespace,
 		RequeueInterval: time.Second,
 	})
-	reconciler.SetupWithManager(k8sManager, controller.Options{})
+	err = reconciler.SetupWithManager(k8sManager, controller.Options{
+		MaxConcurrentReconciles: 1,
+		RateLimiter:             ctrlcommon.BuildRateLimiter(),
+	})
 	Expect(err).ToNot(HaveOccurred())
 
 	go func() {