feat: significantly reduce api calls and introduce partial secret cache (#4086)

* feat: reduce api calls and introduce partial secret cache

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

* updates from review 1

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

* updates from review 2

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

* fix updating CreationPolicy after secret creation

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

* updates from review 3

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

* prevent loop when two ES claim Owner on the same target secret

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

* updates from review 4

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

* fix ClusterSecretStore not ready message

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

---------

Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>
Co-authored-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

apis/externalsecrets/v1beta1/externalsecret_types.go

@@ -427,6 +427,8 @@ const (
 	ConditionReasonSecretSyncedError = "SecretSyncedError"
 	// ConditionReasonSecretDeleted indicates that the secret has been deleted.
 	ConditionReasonSecretDeleted = "SecretDeleted"
+	// ConditionReasonSecretMissing indicates that the secret is missing.
+	ConditionReasonSecretMissing = "SecretMissing"
 
 	ReasonUpdateFailed = "UpdateFailed"
 	ReasonDeprecated   = "ParameterDeprecated"
@@ -470,10 +472,14 @@ type ExternalSecret struct {
 }
 
 const (
-	// AnnotationDataHash is used to ensure consistency.
+	// AnnotationDataHash all secrets managed by an ExternalSecret have this annotation with the hash of their data.
 	AnnotationDataHash = "reconcile.external-secrets.io/data-hash"
-	// LabelOwner points to the owning ExternalSecret resource
-	//  and is used to manage the lifecycle of a Secret
+
+	// LabelManaged all secrets managed by an ExternalSecret will have this label equal to "true".
+	LabelManaged      = "reconcile.external-secrets.io/managed"
+	LabelManagedValue = "true"
+
+	// LabelOwner points to the owning ExternalSecret resource when CreationPolicy=Owner.
 	LabelOwner = "reconcile.external-secrets.io/created-by"
 )
 

cmd/certcontroller.go

@@ -64,19 +64,27 @@ var certcontrollerCmd = &cobra.Command{
 		logger := zap.New(zap.UseFlagOptions(&opts))
 		ctrl.SetLogger(logger)
 
-		cacheOptions := cache.Options{}
+		// completely disable caching of Secrets and ConfigMaps to save memory
+		// see: https://github.com/external-secrets/external-secrets/issues/721
+		clientCacheDisableFor := make([]client.Object, 0)
+		clientCacheDisableFor = append(clientCacheDisableFor, &v1.Secret{}, &v1.ConfigMap{})
+
+		// in large clusters, the CRDs and ValidatingWebhookConfigurations can take up a lot of memory
+		// see: https://github.com/external-secrets/external-secrets/pull/3588
+		cacheByObject := make(map[client.Object]cache.ByObject)
 		if enablePartialCache {
-			cacheOptions.ByObject = map[client.Object]cache.ByObject{
-				&admissionregistration.ValidatingWebhookConfiguration{}: {
-					Label: labels.SelectorFromSet(map[string]string{
-						constants.WellKnownLabelKey: constants.WellKnownLabelValueWebhook,
-					}),
-				},
-				&apiextensions.CustomResourceDefinition{}: {
-					Label: labels.SelectorFromSet(map[string]string{
-						constants.WellKnownLabelKey: constants.WellKnownLabelValueController,
-					}),
-				},
+			// only cache ValidatingWebhookConfiguration with "external-secrets.io/component=webhook" label
+			cacheByObject[&admissionregistration.ValidatingWebhookConfiguration{}] = cache.ByObject{
+				Label: labels.SelectorFromSet(labels.Set{
+					constants.WellKnownLabelKey: constants.WellKnownLabelValueWebhook,
+				}),
+			}
+
+			// only cache CustomResourceDefinition with "external-secrets.io/component=controller" label
+			cacheByObject[&apiextensions.CustomResourceDefinition{}] = cache.ByObject{
+				Label: labels.SelectorFromSet(labels.Set{
+					constants.WellKnownLabelKey: constants.WellKnownLabelValueController,
+				}),
 			}
 		}
 
@@ -91,18 +99,12 @@ var certcontrollerCmd = &cobra.Command{
 			HealthProbeBindAddress: healthzAddr,
 			LeaderElection:         enableLeaderElection,
 			LeaderElectionID:       "crd-certs-controller",
-			Cache:                  cacheOptions,
+			Cache: cache.Options{
+				ByObject: cacheByObject,
+			},
 			Client: client.Options{
 				Cache: &client.CacheOptions{
-					DisableFor: []client.Object{
-						// the client creates a ListWatch for all resource kinds that
-						// are requested with .Get().
-						// We want to avoid to cache all secrets or configmaps in memory.
-						// The ES controller uses v1.PartialObjectMetadata for the secrets
-						// that he owns.
-						// see #721
-						&v1.Secret{},
-					},
+					DisableFor: clientCacheDisableFor,
 				},
 			},
 		})

cmd/root.go

@@ -64,6 +64,7 @@ var (
 	enableLeaderElection                  bool
 	enableSecretsCache                    bool
 	enableConfigMapsCache                 bool
+	enableManagedSecretsCache             bool
 	enablePartialCache                    bool
 	concurrent                            int
 	port                                  int
@@ -107,19 +108,6 @@ var rootCmd = &cobra.Command{
 	Run: func(cmd *cobra.Command, args []string) {
 		var lvl zapcore.Level
 		var enc zapcore.TimeEncoder
-		// the client creates a ListWatch for all resource kinds that
-		// are requested with .Get().
-		// We want to avoid to cache all secrets or configmaps in memory.
-		// The ES controller uses v1.PartialObjectMetadata for the secrets
-		// that he owns.
-		// see #721
-		cacheList := make([]client.Object, 0)
-		if !enableSecretsCache {
-			cacheList = append(cacheList, &v1.Secret{})
-		}
-		if !enableConfigMapsCache {
-			cacheList = append(cacheList, &v1.ConfigMap{})
-		}
 		lvlErr := lvl.UnmarshalText([]byte(loglevel))
 		if lvlErr != nil {
 			setupLog.Error(lvlErr, "error unmarshalling loglevel")
@@ -141,6 +129,21 @@ var rootCmd = &cobra.Command{
 		config := ctrl.GetConfigOrDie()
 		config.QPS = clientQPS
 		config.Burst = clientBurst
+
+		// the client creates a ListWatch for resources that are requested with .Get() or .List()
+		// some users might want to completely disable caching of Secrets and ConfigMaps
+		// to decrease memory usage at the expense of high Kubernetes API usage
+		// see: https://github.com/external-secrets/external-secrets/issues/721
+		clientCacheDisableFor := make([]client.Object, 0)
+		if !enableSecretsCache {
+			// dont cache any secrets
+			clientCacheDisableFor = append(clientCacheDisableFor, &v1.Secret{})
+		}
+		if !enableConfigMapsCache {
+			// dont cache any configmaps
+			clientCacheDisableFor = append(clientCacheDisableFor, &v1.ConfigMap{})
+		}
+
 		ctrlOpts := ctrl.Options{
 			Scheme: scheme,
 			Metrics: server.Options{
@@ -151,7 +154,7 @@ var rootCmd = &cobra.Command{
 			}),
 			Client: client.Options{
 				Cache: &client.CacheOptions{
-					DisableFor: cacheList,
+					DisableFor: clientCacheDisableFor,
 				},
 			},
 			LeaderElection:   enableLeaderElection,
@@ -168,6 +171,19 @@ var rootCmd = &cobra.Command{
 			os.Exit(1)
 		}
 
+		// we create a special client for accessing secrets in the ExternalSecret reconcile loop.
+		// by default, it is the same as the normal client, but if `--enable-managed-secrets-caching`
+		// is set, we use a special client that only caches secrets managed by an ExternalSecret.
+		// 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)
+			if err != nil {
+				setupLog.Error(err, "unable to create managed secret client")
+				os.Exit(1)
+			}
+		}
+
 		ssmetrics.SetUpMetrics()
 		if err = (&secretstore.StoreReconciler{
 			Client:          mgr.GetClient(),
@@ -198,6 +214,7 @@ var rootCmd = &cobra.Command{
 		}
 		if err = (&externalsecret.Reconciler{
 			Client:                    mgr.GetClient(),
+			SecretClient:              secretClient,
 			Log:                       ctrl.Log.WithName("controllers").WithName("ExternalSecret"),
 			Scheme:                    mgr.GetScheme(),
 			RestConfig:                mgr.GetConfig(),
@@ -277,8 +294,9 @@ func init() {
 	rootCmd.Flags().BoolVar(&enableClusterStoreReconciler, "enable-cluster-store-reconciler", true, "Enable cluster store reconciler.")
 	rootCmd.Flags().BoolVar(&enableClusterExternalSecretReconciler, "enable-cluster-external-secret-reconciler", true, "Enable cluster external secret reconciler.")
 	rootCmd.Flags().BoolVar(&enablePushSecretReconciler, "enable-push-secret-reconciler", true, "Enable push secret reconciler.")
-	rootCmd.Flags().BoolVar(&enableSecretsCache, "enable-secrets-caching", false, "Enable secrets caching for external-secrets pod.")
-	rootCmd.Flags().BoolVar(&enableConfigMapsCache, "enable-configmaps-caching", false, "Enable secrets caching for external-secrets pod.")
+	rootCmd.Flags().BoolVar(&enableSecretsCache, "enable-secrets-caching", false, "Enable secrets caching for ALL secrets in the cluster (WARNING: can increase memory usage).")
+	rootCmd.Flags().BoolVar(&enableConfigMapsCache, "enable-configmaps-caching", false, "Enable configmaps caching for ALL configmaps in the cluster (WARNING: can increase memory usage).")
+	rootCmd.Flags().BoolVar(&enableManagedSecretsCache, "enable-managed-secrets-caching", true, "Enable secrets caching for secrets managed by an ExternalSecret")
 	rootCmd.Flags().DurationVar(&storeRequeueInterval, "store-requeue-interval", time.Minute*5, "Default Time duration between reconciling (Cluster)SecretStores")
 	rootCmd.Flags().BoolVar(&enableFloodGate, "enable-flood-gate", true, "Enable flood gate. External secret will be reconciled only if the ClusterStore or Store have an healthy or unknown state.")
 	rootCmd.Flags().BoolVar(&enableExtendedMetricLabels, "enable-extended-metric-labels", false, "Enable recommended kubernetes annotations as labels in metrics.")

docs/api/controller-options.md

@@ -12,7 +12,7 @@ The external-secrets binary includes three components: `core controller`, `certc
 The core controller is invoked without a subcommand and can be configured with the following flags:
 
 | Name                                          | Type     | Default                       | Description                                                                                                                                                        |
-| --------------------------------------------- | -------- | ----------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
+|-----------------------------------------------|----------|-------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 | `--client-burst`                              | int      | uses rest client default (10) | Maximum Burst allowed to be passed to rest.Client                                                                                                                  |
 | `--client-qps`                                | float32  | uses rest client default (5)  | QPS configuration to be passed to rest.Client                                                                                                                      |
 | `--concurrent`                                | int      | 1                             | The number of concurrent reconciles.                                                                                                                               |
@@ -20,15 +20,16 @@ The core controller is invoked without a subcommand and can be configured with t
 | `--enable-cluster-external-secret-reconciler` | boolean  | true                          | Enables the cluster external secret reconciler.                                                                                                                    |
 | `--enable-cluster-store-reconciler`           | boolean  | true                          | Enables the cluster store reconciler.                                                                                                                              |
 | `--enable-push-secret-reconciler`             | boolean  | true                          | Enables the push secret reconciler.                                                                                                                                |
-| `--enable-secrets-caching`                    | boolean  | false                         | Enables the secrets caching for external-secrets pod.                                                                                                              |
-| `--enable-configmaps-caching`                 | boolean  | false                         | Enables the ConfigMap caching for external-secrets pod.                                                                                                            |
+| `--enable-secrets-caching`                    | boolean  | false                         | Enable secrets caching for ALL secrets in the cluster (WARNING: can increase memory usage).                                                                        |
+| `--enable-configmaps-caching`                 | boolean  | false                         | Enable configmaps caching for ALL configmaps in the cluster (WARNING: can increase memory usage).                                                                  |
+| `--enable-managed-secrets-caching`            | boolean  | true                          | Enable secrets caching for secrets managed by an ExternalSecret.                                                                                                   |
 | `--enable-flood-gate`                         | boolean  | true                          | Enable flood gate. External secret will be reconciled only if the ClusterStore or Store have an healthy or unknown state.                                          |
 | `--enable-extended-metric-labels`             | boolean  | true                          | Enable recommended kubernetes annotations as labels in metrics.                                                                                                    |
 | `--enable-leader-election`                    | boolean  | false                         | Enable leader election for controller manager. Enabling this will ensure there is only one active controller manager.                                              |
 | `--experimental-enable-aws-session-cache`     | boolean  | false                         | Enable experimental AWS session cache. External secret will reuse the AWS session without creating a new one on each request.                                      |
 | `--help`                                      |          |                               | help for external-secrets                                                                                                                                          |
 | `--loglevel`                                  | string   | info                          | loglevel to use, one of: debug, info, warn, error, dpanic, panic, fatal                                                                                            |
-| `--zap-time-encoding`                                  | string   | epoch                          | loglevel to use, one of: epoch, millis, nano, iso8601, rfc3339, rfc3339nano                                                                                            |
+| `--zap-time-encoding`                         | string   | epoch                         | loglevel to use, one of: epoch, millis, nano, iso8601, rfc3339, rfc3339nano                                                                                        |
 | `--metrics-addr`                              | string   | :8080                         | The address the metric endpoint binds to.                                                                                                                          |
 | `--namespace`                                 | string   | -                             | watch external secrets scoped in the provided namespace only. ClusterSecretStore can be used but only work if it doesn't reference resources from other namespaces |
 | `--store-requeue-interval`                    | duration | 5m0s                          | Default Time duration between reconciling (Cluster)SecretStores                                                                                                    |

e2e/framework/eso.go

@@ -105,8 +105,9 @@ func equalSecrets(exp, ts *v1.Secret) bool {
 		return false
 	}
 
-	// secret contains label owner which must be ignored
+	// secret contains labels which must be ignored
 	delete(ts.ObjectMeta.Labels, esv1beta1.LabelOwner)
+	delete(ts.ObjectMeta.Labels, esv1beta1.LabelManaged)
 	if len(ts.ObjectMeta.Labels) == 0 {
 		ts.ObjectMeta.Labels = nil
 	}

pkg/controllers/commontest/common.go

@@ -19,7 +19,6 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/google/go-cmp/cmp"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -62,14 +61,12 @@ func HasOwnerRef(meta metav1.ObjectMeta, kind, name string) bool {
 	return false
 }
 
-func HasFieldOwnership(meta metav1.ObjectMeta, mgr, expected string) string {
+// FirstManagedFieldForManager returns the JSON representation of the first `metadata.managedFields` entry for a given manager.
+func FirstManagedFieldForManager(meta metav1.ObjectMeta, managerName string) string {
 	for _, ref := range meta.ManagedFields {
-		if ref.Manager == mgr {
-			if diff := cmp.Diff(string(ref.FieldsV1.Raw), expected); diff != "" {
-				return fmt.Sprintf("(-got, +want)\n%s", diff)
-			}
-			return ""
+		if ref.Manager == managerName {
+			return ref.FieldsV1.String()
 		}
 	}
-	return fmt.Sprintf("No managed fields managed by %s", mgr)
+	return fmt.Sprintf("No managed fields managed by %s", managerName)
 }

pkg/controllers/externalsecret/externalsecret_controller_template.go

@@ -31,22 +31,37 @@ import (
 
 // merge template in the following order:
 // * template.Data (highest precedence)
-// * template.templateFrom
-// * secret via es.data or es.dataFrom.
+// * template.TemplateFrom
+// * secret via es.data or es.dataFrom (if template.MergePolicy is Merge, or there is no template)
+// * existing secret keys (if CreationPolicy is Merge).
 func (r *Reconciler) applyTemplate(ctx context.Context, es *esv1beta1.ExternalSecret, secret *v1.Secret, dataMap map[string][]byte) error {
+	// update metadata (labels, annotations) of the secret
 	if err := setMetadata(secret, es); err != nil {
 		return err
 	}
 
+	// we only keep existing keys if creation policy is Merge, otherwise we clear the secret
+	if es.Spec.Target.CreationPolicy != esv1beta1.CreatePolicyMerge {
+		secret.Data = make(map[string][]byte)
+	}
+
 	// no template: copy data and return
 	if es.Spec.Target.Template == nil {
-		secret.Data = dataMap
+		maps.Insert(secret.Data, maps.All(dataMap))
 		return nil
 	}
-	// Merge Policy should merge secrets
-	if es.Spec.Target.Template.MergePolicy == esv1beta1.MergePolicyMerge {
+
+	// set the secret type if it is defined in the template, otherwise keep the existing type
+	if es.Spec.Target.Template.Type != "" {
+		secret.Type = es.Spec.Target.Template.Type
+	}
+
+	// when TemplateMergePolicy is Merge, or there is no data template, we include the keys from `dataMap`
+	noTemplate := len(es.Spec.Target.Template.Data) == 0 && len(es.Spec.Target.Template.TemplateFrom) == 0
+	if es.Spec.Target.Template.MergePolicy == esv1beta1.MergePolicyMerge || noTemplate {
 		maps.Insert(secret.Data, maps.All(dataMap))
 	}
+
 	execute, err := template.EngineForVersion(es.Spec.Target.Template.EngineVersion)
 	if err != nil {
 		return err
@@ -58,6 +73,7 @@ func (r *Reconciler) applyTemplate(ctx context.Context, es *esv1beta1.ExternalSe
 		DataMap:      dataMap,
 		Exec:         execute,
 	}
+
 	// apply templates defined in template.templateFrom
 	err = p.MergeTemplateFrom(ctx, es.Namespace, es.Spec.Target.Template)
 	if err != nil {
@@ -79,24 +95,23 @@ func (r *Reconciler) applyTemplate(ctx context.Context, es *esv1beta1.ExternalSe
 	if err != nil {
 		return fmt.Errorf(errExecTpl, err)
 	}
-	// if no data was provided by template fallback
-	// to value from the provider
-	if len(es.Spec.Target.Template.Data) == 0 && len(es.Spec.Target.Template.TemplateFrom) == 0 {
-		secret.Data = dataMap
-	}
+
 	return nil
 }
 
 // setMetadata sets Labels and Annotations to the given secret.
 func setMetadata(secret *v1.Secret, es *esv1beta1.ExternalSecret) error {
+	// ensure that Labels and Annotations are not nil
+	// so it is safe to merge them
 	if secret.Labels == nil {
 		secret.Labels = make(map[string]string)
 	}
 	if secret.Annotations == nil {
 		secret.Annotations = make(map[string]string)
 	}
-	// Clean up Labels and Annotations added by the operator
-	// so that it won't leave outdated ones
+
+	// remove any existing labels managed by this external secret
+	// this is to ensure that we don't have any stale labels
 	labelKeys, err := templating.GetManagedLabelKeys(secret, es.Name)
 	if err != nil {
 		return err
@@ -104,7 +119,6 @@ func setMetadata(secret *v1.Secret, es *esv1beta1.ExternalSecret) error {
 	for _, key := range labelKeys {
 		delete(secret.ObjectMeta.Labels, key)
 	}
-
 	annotationKeys, err := templating.GetManagedAnnotationKeys(secret, es.Name)
 	if err != nil {
 		return err
@@ -113,13 +127,14 @@ func setMetadata(secret *v1.Secret, es *esv1beta1.ExternalSecret) error {
 		delete(secret.ObjectMeta.Annotations, key)
 	}
 
+	// if no template is defined, copy labels and annotations from the ExternalSecret
 	if es.Spec.Target.Template == nil {
 		utils.MergeStringMap(secret.ObjectMeta.Labels, es.ObjectMeta.Labels)
 		utils.MergeStringMap(secret.ObjectMeta.Annotations, es.ObjectMeta.Annotations)
 		return nil
 	}
 
-	secret.Type = es.Spec.Target.Template.Type
+	// copy labels and annotations from the template
 	utils.MergeStringMap(secret.ObjectMeta.Labels, es.Spec.Target.Template.Metadata.Labels)
 	utils.MergeStringMap(secret.ObjectMeta.Annotations, es.Spec.Target.Template.Metadata.Annotations)
 	return nil

pkg/controllers/externalsecret/externalsecret_controller_test.go

@@ -26,6 +26,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/onsi/gomega/format"
 	"github.com/prometheus/client_golang/prometheus"
 	dto "github.com/prometheus/client_model/go"
 	v1 "k8s.io/api/core/v1"
@@ -89,30 +90,23 @@ var _ = Describe("Kind=secret existence logic", func() {
 	}
 	type testCase struct {
 		Name           string
-		Input          v1.Secret
+		Input          *v1.Secret
 		ExpectedOutput bool
 	}
 	tests := []testCase{
 		{
 			Name:           "Should not be valid in case of missing uid",
-			Input:          v1.Secret{},
+			Input:          &v1.Secret{},
 			ExpectedOutput: false,
 		},
 		{
 			Name: "A nil annotation should not be valid",
-			Input: v1.Secret{
+			Input: &v1.Secret{
 				ObjectMeta: metav1.ObjectMeta{
-					UID:         "xxx",
-					Annotations: map[string]string{},
-				},
-			},
-			ExpectedOutput: false,
-		},
-		{
-			Name: "A nil annotation should not be valid",
-			Input: v1.Secret{
-				ObjectMeta: metav1.ObjectMeta{
-					UID:         "xxx",
+					UID: "xxx",
+					Labels: map[string]string{
+						esv1beta1.LabelManaged: esv1beta1.LabelManagedValue,
+					},
 					Annotations: map[string]string{},
 				},
 			},
@@ -120,9 +114,12 @@ var _ = Describe("Kind=secret existence logic", func() {
 		},
 		{
 			Name: "An invalid annotation hash should not be valid",
-			Input: v1.Secret{
+			Input: &v1.Secret{
 				ObjectMeta: metav1.ObjectMeta{
 					UID: "xxx",
+					Labels: map[string]string{
+						esv1beta1.LabelManaged: esv1beta1.LabelManagedValue,
+					},
 					Annotations: map[string]string{
 						esv1beta1.AnnotationDataHash: "xxxxxx",
 					},
@@ -131,10 +128,13 @@ var _ = Describe("Kind=secret existence logic", func() {
 			ExpectedOutput: false,
 		},
 		{
-			Name: "A valid config map should return true",
-			Input: v1.Secret{
+			Name: "A valid secret should return true",
+			Input: &v1.Secret{
 				ObjectMeta: metav1.ObjectMeta{
 					UID: "xxx",
+					Labels: map[string]string{
+						esv1beta1.LabelManaged: esv1beta1.LabelManagedValue,
+					},
 					Annotations: map[string]string{
 						esv1beta1.AnnotationDataHash: utils.ObjectHash(validData),
 					},
@@ -449,9 +449,10 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 			Expect(string(secret.Data[existingKey])).To(Equal(existingVal))
 			Expect(string(secret.Data[targetProp])).To(Equal(secretVal))
 
-			Expect(secret.ObjectMeta.Labels).To(HaveLen(2))
+			Expect(secret.ObjectMeta.Labels).To(HaveLen(3))
 			Expect(secret.ObjectMeta.Labels).To(HaveKeyWithValue("existing-label-key", "existing-label-value"))
 			Expect(secret.ObjectMeta.Labels).To(HaveKeyWithValue("es-label-key", "es-label-value"))
+			Expect(secret.ObjectMeta.Labels).To(HaveKeyWithValue(esv1beta1.LabelManaged, esv1beta1.LabelManagedValue))
 
 			Expect(secret.ObjectMeta.Annotations).To(HaveLen(3))
 			Expect(secret.ObjectMeta.Annotations).To(HaveKeyWithValue("existing-annotation-key", "existing-annotation-value"))
@@ -460,12 +461,15 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 
 			Expect(ctest.HasOwnerRef(secret.ObjectMeta, "ExternalSecret", ExternalSecretFQDN)).To(BeFalse())
 			Expect(secret.ObjectMeta.ManagedFields).To(HaveLen(2))
-			Expect(ctest.HasFieldOwnership(
-				secret.ObjectMeta,
-				ExternalSecretFQDN,
-				fmt.Sprintf(`{"f:data":{"f:targetProperty":{}},"f:immutable":{},"f:metadata":{"f:annotations":{"f:es-annotation-key":{},"f:%s":{}},"f:labels":{"f:es-label-key":{}}}}`, esv1beta1.AnnotationDataHash)),
-			).To(BeEmpty())
-			Expect(ctest.HasFieldOwnership(secret.ObjectMeta, FakeManager, `{"f:data":{".":{},"f:pre-existing-key":{}},"f:metadata":{"f:annotations":{".":{},"f:existing-annotation-key":{}},"f:labels":{".":{},"f:existing-label-key":{}}},"f:type":{}}`)).To(BeEmpty())
+			oldCharactersAroundMismatchToInclude := format.CharactersAroundMismatchToInclude
+			format.CharactersAroundMismatchToInclude = 10
+			Expect(ctest.FirstManagedFieldForManager(secret.ObjectMeta, ExternalSecretFQDN)).To(
+				Equal(fmt.Sprintf(`{"f:data":{"f:targetProperty":{}},"f:metadata":{"f:annotations":{"f:es-annotation-key":{},"f:%s":{}},"f:labels":{"f:es-label-key":{},"f:%s":{}}}}`, esv1beta1.AnnotationDataHash, esv1beta1.LabelManaged)),
+			)
+			Expect(ctest.FirstManagedFieldForManager(secret.ObjectMeta, FakeManager)).To(
+				Equal(`{"f:data":{".":{},"f:pre-existing-key":{}},"f:metadata":{"f:annotations":{".":{},"f:existing-annotation-key":{}},"f:labels":{".":{},"f:existing-label-key":{}}},"f:type":{}}`),
+			)
+			format.CharactersAroundMismatchToInclude = oldCharactersAroundMismatchToInclude
 		}
 	}
 
@@ -548,8 +552,18 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 
 		fakeProvider.WithGetSecret([]byte(secretVal), nil)
 		tc.checkCondition = func(es *esv1beta1.ExternalSecret) bool {
-			cond := GetExternalSecretCondition(es.Status, esv1beta1.ExternalSecretReady)
-			if cond == nil || cond.Status != v1.ConditionFalse || cond.Reason != esv1beta1.ConditionReasonSecretSyncedError {
+			expected := []esv1beta1.ExternalSecretStatusCondition{
+				{
+					Type:    esv1beta1.ExternalSecretReady,
+					Status:  v1.ConditionTrue,
+					Reason:  esv1beta1.ConditionReasonSecretMissing,
+					Message: msgMissing,
+				},
+			}
+
+			opts := cmpopts.IgnoreFields(esv1beta1.ExternalSecretStatusCondition{}, "LastTransitionTime")
+			if diff := cmp.Diff(expected, es.Status.Conditions, opts); diff != "" {
+				GinkgoLogr.Info("(-got, +want)\n%s", "diff", diff)
 				return false
 			}
 			return true
@@ -558,10 +572,10 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 			Eventually(func() bool {
 				Expect(testSyncCallsError.WithLabelValues(ExternalSecretName, ExternalSecretNamespace).Write(&metric)).To(Succeed())
 				Expect(testExternalSecretReconcileDuration.WithLabelValues(ExternalSecretName, ExternalSecretNamespace).Write(&metricDuration)).To(Succeed())
-				return metric.GetCounter().GetValue() >= 2.0 && metricDuration.GetGauge().GetValue() > 0.0
+				return metric.GetCounter().GetValue() == 0 && metricDuration.GetGauge().GetValue() > 0.0
 			}, timeout, interval).Should(BeTrue())
-			Expect(externalSecretConditionShouldBe(ExternalSecretName, ExternalSecretNamespace, esv1beta1.ExternalSecretReady, v1.ConditionFalse, 1.0)).To(BeTrue())
-			Expect(externalSecretConditionShouldBe(ExternalSecretName, ExternalSecretNamespace, esv1beta1.ExternalSecretReady, v1.ConditionTrue, 0.0)).To(BeTrue())
+			Expect(externalSecretConditionShouldBe(ExternalSecretName, ExternalSecretNamespace, esv1beta1.ExternalSecretReady, v1.ConditionFalse, 0.0)).To(BeTrue())
+			Expect(externalSecretConditionShouldBe(ExternalSecretName, ExternalSecretNamespace, esv1beta1.ExternalSecretReady, v1.ConditionTrue, 1.0)).To(BeTrue())
 		}
 	}
 
@@ -591,11 +605,12 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 			// check owner/managedFields
 			Expect(ctest.HasOwnerRef(secret.ObjectMeta, "ExternalSecret", ExternalSecretFQDN)).To(BeFalse())
 			Expect(secret.ObjectMeta.ManagedFields).To(HaveLen(2))
-			Expect(ctest.HasFieldOwnership(
-				secret.ObjectMeta,
-				ExternalSecretFQDN,
-				fmt.Sprintf("{\"f:data\":{\"f:targetProperty\":{}},\"f:immutable\":{},\"f:metadata\":{\"f:annotations\":{\"f:%s\":{}}}}", esv1beta1.AnnotationDataHash)),
-			).To(BeEmpty())
+			oldCharactersAroundMismatchToInclude := format.CharactersAroundMismatchToInclude
+			format.CharactersAroundMismatchToInclude = 10
+			Expect(ctest.FirstManagedFieldForManager(secret.ObjectMeta, ExternalSecretFQDN)).To(
+				Equal(fmt.Sprintf(`{"f:data":{"f:targetProperty":{}},"f:metadata":{"f:annotations":{".":{},"f:%s":{}},"f:labels":{".":{},"f:%s":{}}}}`, esv1beta1.AnnotationDataHash, esv1beta1.LabelManaged)),
+			)
+			format.CharactersAroundMismatchToInclude = oldCharactersAroundMismatchToInclude
 		}
 	}
 
@@ -1394,7 +1409,7 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 					Type:    esv1beta1.ExternalSecretReady,
 					Status:  v1.ConditionTrue,
 					Reason:  esv1beta1.ConditionReasonSecretSynced,
-					Message: "Secret was synced",
+					Message: msgSyncedRetain,
 				},
 			}
 
@@ -1841,7 +1856,7 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 			&Reconciler{
 				ClusterSecretStoreEnabled: false,
 			},
-			*tc.externalSecret,
+			tc.externalSecret,
 		)).To(BeTrue())
 
 		tc.checkCondition = func(es *esv1beta1.ExternalSecret) bool {
@@ -2315,14 +2330,17 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 var _ = Describe("ExternalSecret refresh logic", func() {
 	Context("secret refresh", func() {
 		It("should refresh when resource version does not match", func() {
-			Expect(shouldRefresh(esv1beta1.ExternalSecret{
+			Expect(shouldRefresh(&esv1beta1.ExternalSecret{
+				Spec: esv1beta1.ExternalSecretSpec{
+					RefreshInterval: &metav1.Duration{Duration: time.Minute},
+				},
 				Status: esv1beta1.ExternalSecretStatus{
 					SyncedResourceVersion: "some resource version",
 				},
 			})).To(BeTrue())
 		})
 		It("should refresh when labels change", func() {
-			es := esv1beta1.ExternalSecret{
+			es := &esv1beta1.ExternalSecret{
 				ObjectMeta: metav1.ObjectMeta{
 					Generation: 1,
 					Labels: map[string]string{
@@ -2346,7 +2364,7 @@ var _ = Describe("ExternalSecret refresh logic", func() {
 		})
 
 		It("should refresh when annotations change", func() {
-			es := esv1beta1.ExternalSecret{
+			es := &esv1beta1.ExternalSecret{
 				ObjectMeta: metav1.ObjectMeta{
 					Generation: 1,
 					Annotations: map[string]string{
@@ -2370,12 +2388,12 @@ var _ = Describe("ExternalSecret refresh logic", func() {
 		})
 
 		It("should refresh when generation has changed", func() {
-			es := esv1beta1.ExternalSecret{
+			es := &esv1beta1.ExternalSecret{
 				ObjectMeta: metav1.ObjectMeta{
 					Generation: 1,
 				},
 				Spec: esv1beta1.ExternalSecretSpec{
-					RefreshInterval: &metav1.Duration{Duration: 0},
+					RefreshInterval: &metav1.Duration{Duration: time.Minute},
 				},
 				Status: esv1beta1.ExternalSecretStatus{
 					RefreshTime: metav1.Now(),
@@ -2390,7 +2408,7 @@ var _ = Describe("ExternalSecret refresh logic", func() {
 		})
 
 		It("should skip refresh when refreshInterval is 0", func() {
-			es := esv1beta1.ExternalSecret{
+			es := &esv1beta1.ExternalSecret{
 				ObjectMeta: metav1.ObjectMeta{
 					Generation: 1,
 				},
@@ -2405,7 +2423,7 @@ var _ = Describe("ExternalSecret refresh logic", func() {
 		})
 
 		It("should refresh when refresh interval has passed", func() {
-			es := esv1beta1.ExternalSecret{
+			es := &esv1beta1.ExternalSecret{
 				ObjectMeta: metav1.ObjectMeta{
 					Generation: 1,
 				},
@@ -2422,7 +2440,7 @@ var _ = Describe("ExternalSecret refresh logic", func() {
 		})
 
 		It("should refresh when no refresh time was set", func() {
-			es := esv1beta1.ExternalSecret{
+			es := &esv1beta1.ExternalSecret{
 				ObjectMeta: metav1.ObjectMeta{
 					Generation: 1,
 				},
@@ -2502,43 +2520,6 @@ var _ = Describe("ExternalSecret refresh logic", func() {
 	})
 })
 
-var _ = Describe("Controller Reconcile logic", func() {
-	Context("controller reconcile", func() {
-		It("should reconcile when resource is not synced", func() {
-			Expect(shouldReconcile(esv1beta1.ExternalSecret{
-				Status: esv1beta1.ExternalSecretStatus{
-					SyncedResourceVersion: "some resource version",
-					Conditions:            []esv1beta1.ExternalSecretStatusCondition{{Reason: "NotASecretSynced"}},
-				},
-			})).To(BeTrue())
-		})
-
-		It("should reconcile when secret isn't immutable", func() {
-			Expect(shouldReconcile(esv1beta1.ExternalSecret{
-				Spec: esv1beta1.ExternalSecretSpec{
-					Target: esv1beta1.ExternalSecretTarget{
-						Immutable: false,
-					},
-				},
-			})).To(BeTrue())
-		})
-
-		It("should not reconcile if secret is immutable and has synced condition", func() {
-			Expect(shouldReconcile(esv1beta1.ExternalSecret{
-				Spec: esv1beta1.ExternalSecretSpec{
-					Target: esv1beta1.ExternalSecretTarget{
-						Immutable: true,
-					},
-				},
-				Status: esv1beta1.ExternalSecretStatus{
-					SyncedResourceVersion: "some resource version",
-					Conditions:            []esv1beta1.ExternalSecretStatusCondition{{Reason: "SecretSynced"}},
-				},
-			})).To(BeFalse())
-		})
-	})
-})
-
 func externalSecretConditionShouldBe(name, ns string, ct esv1beta1.ExternalSecretConditionType, cs v1.ConditionStatus, v float64) bool {
 	return Eventually(func() float64 {
 		Expect(testExternalSecretCondition.WithLabelValues(name, ns, string(ct), string(cs)).Write(&metric)).To(Succeed())

pkg/controllers/externalsecret/suite_test.go

@@ -83,7 +83,13 @@ var _ = BeforeSuite(func() {
 		},
 		Client: client.Options{
 			Cache: &client.CacheOptions{
-				DisableFor: []client.Object{&v1.Secret{}, &v1.ConfigMap{}},
+				// the client creates a ListWatch for resources that are requested with .Get() or .List()
+				// we disable caching in the production code, so we disable it here as well for consistency
+				// see: https://github.com/external-secrets/external-secrets/issues/721
+				DisableFor: []client.Object{
+					&v1.Secret{},
+					&v1.ConfigMap{},
+				},
 			},
 		},
 	})
@@ -95,8 +101,14 @@ var _ = BeforeSuite(func() {
 	Expect(k8sClient).ToNot(BeNil())
 	Expect(err).ToNot(HaveOccurred())
 
+	// 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)
+	Expect(err).ToNot(HaveOccurred())
+
 	err = (&Reconciler{
 		Client:                    k8sManager.GetClient(),
+		SecretClient:              secretClient,
 		RestConfig:                cfg,
 		Scheme:                    k8sManager.GetScheme(),
 		Log:                       ctrl.Log.WithName("controllers").WithName("ExternalSecrets"),

pkg/controllers/secretstore/client_manager.go

@@ -35,7 +35,7 @@ import (
 const (
 	errGetClusterSecretStore = "could not get ClusterSecretStore %q, %w"
 	errGetSecretStore        = "could not get SecretStore %q, %w"
-	errSecretStoreNotReady   = "the desired SecretStore %s is not ready"
+	errSecretStoreNotReady   = "%s %q is not ready"
 	errClusterStoreMismatch  = "using cluster store %q is not allowed from namespace %q: denied by spec.condition"
 )
 
@@ -271,7 +271,7 @@ func assertStoreIsUsable(store esv1beta1.GenericStore) error {
 	}
 	condition := GetSecretStoreCondition(store.GetStatus(), esv1beta1.SecretStoreReady)
 	if condition == nil || condition.Status != v1.ConditionTrue {
-		return fmt.Errorf(errSecretStoreNotReady, store.GetName())
+		return fmt.Errorf(errSecretStoreNotReady, store.GetKind(), store.GetName())
 	}
 	return nil
 }