From ac26166ac9eb413ab60bccef512978704e831bb9 Mon Sep 17 00:00:00 2001 From: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com> Date: Sun, 24 Nov 2024 13:53:53 -0800 Subject: [PATCH] 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> --- .../v1beta1/externalsecret_types.go | 12 +- cmd/certcontroller.go | 46 +- cmd/root.go | 50 +- docs/api/controller-options.md | 9 +- e2e/framework/eso.go | 3 +- pkg/controllers/commontest/common.go | 13 +- .../externalsecret_controller.go | 903 ++++++++++++------ .../externalsecret_controller_template.go | 43 +- .../externalsecret_controller_test.go | 143 ++- pkg/controllers/externalsecret/suite_test.go | 14 +- pkg/controllers/secretstore/client_manager.go | 4 +- 11 files changed, 791 insertions(+), 449 deletions(-) diff --git a/apis/externalsecrets/v1beta1/externalsecret_types.go b/apis/externalsecrets/v1beta1/externalsecret_types.go index 6df9d538d..b59a5eced 100644 --- a/apis/externalsecrets/v1beta1/externalsecret_types.go +++ b/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" ) diff --git a/cmd/certcontroller.go b/cmd/certcontroller.go index 01d1f18bf..b61ac51be 100644 --- a/cmd/certcontroller.go +++ b/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, }, }, }) diff --git a/cmd/root.go b/cmd/root.go index 88aed014c..a8a856816 100644 --- a/cmd/root.go +++ b/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.") diff --git a/docs/api/controller-options.md b/docs/api/controller-options.md index 61ea9ad91..5911f6a1b 100644 --- a/docs/api/controller-options.md +++ b/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 | diff --git a/e2e/framework/eso.go b/e2e/framework/eso.go index 3003f72a3..baa50c5cb 100644 --- a/e2e/framework/eso.go +++ b/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 } diff --git a/pkg/controllers/commontest/common.go b/pkg/controllers/commontest/common.go index 72aa47bc9..b1332deb8 100644 --- a/pkg/controllers/commontest/common.go +++ b/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) } diff --git a/pkg/controllers/externalsecret/externalsecret_controller.go b/pkg/controllers/externalsecret/externalsecret_controller.go index c800a9bd0..d7d016dc3 100644 --- a/pkg/controllers/externalsecret/externalsecret_controller.go +++ b/pkg/controllers/externalsecret/externalsecret_controller.go @@ -30,12 +30,18 @@ import ( "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/fields" + "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" @@ -57,34 +63,65 @@ import ( ) const ( - fieldOwnerTemplate = "externalsecrets.external-secrets.io/%v" - errGetES = "could not get ExternalSecret" - errConvert = "could not apply conversion strategy to keys: %v" - errDecode = "could not apply decoding strategy to %v[%d]: %v" - errGenerate = "could not generate [%d]: %w" - errRewrite = "could not rewrite spec.dataFrom[%d]: %v" - errInvalidKeys = "secret keys from spec.dataFrom.%v[%d] can only have alphanumeric,'-', '_' or '.' characters. Convert them using rewrite (https://external-secrets.io/latest/guides-datafrom-rewrite)" - errUpdateSecret = "could not update Secret" - errPatchStatus = "unable to patch status" - errGetExistingSecret = "could not get existing secret: %w" - errSetCtrlReference = "could not set ExternalSecret controller reference: %w" - errFetchTplFrom = "error fetching templateFrom data: %w" - errGetSecretData = "could not get secret data from provider" - errDeleteSecret = "could not delete secret" - errApplyTemplate = "could not apply template: %w" - errExecTpl = "could not execute template: %w" - errInvalidCreatePolicy = "invalid creationPolicy=%s. Can not delete secret i do not own" - errPolicyMergeNotFound = "the desired secret %s was not found. With creationPolicy=Merge the secret won't be created" - errPolicyMergeGetSecret = "unable to get secret %s: %w" - errPolicyMergeMutate = "unable to mutate secret %s: %w" - errPolicyMergePatch = "unable to patch secret %s: %w" + fieldOwnerTemplate = "externalsecrets.external-secrets.io/%v" + + // condition messages for "SecretSynced" reason. + msgSynced = "secret synced" + msgSyncedRetain = "secret retained due to DeletionPolicy=Retain" + + // condition messages for "SecretDeleted" reason. + msgDeleted = "secret deleted due to DeletionPolicy=Delete" + + // condition messages for "SecretMissing" reason. + msgMissing = "secret will not be created due to CreationPolicy=Merge" + + // condition messages for "SecretSyncedError" reason. + msgErrorGetSecretData = "could not get secret data from provider" + msgErrorDeleteSecret = "could not delete secret" + msgErrorDeleteOrphaned = "could not delete orphaned secrets" + msgErrorUpdateSecret = "could not update secret" + msgErrorUpdateImmutable = "could not update secret, target is immutable" + msgErrorBecomeOwner = "failed to take ownership of target secret" + msgErrorIsOwned = "target is owned by another ExternalSecret" + + // log messages. + logErrorGetES = "unable to get ExternalSecret" + logErrorUpdateESStatus = "unable to update ExternalSecret status" + logErrorGetSecret = "unable to get Secret" + logErrorPatchSecret = "unable to patch Secret" + logErrorSecretCacheNotSynced = "controller caches for Secret are not in sync" + logErrorUnmanagedStore = "unable to determine if store is managed" + + // error formats. + errConvert = "could not apply conversion strategy to keys: %v" + errDecode = "could not apply decoding strategy to %v[%d]: %v" + errGenerate = "could not generate [%d]: %w" + errRewrite = "could not rewrite spec.dataFrom[%d]: %v" + errInvalidKeys = "secret keys from spec.dataFrom.%v[%d] can only have alphanumeric, '-', '_' or '.' characters. Convert them using rewrite (https://external-secrets.io/latest/guides/datafrom-rewrite/)" + errFetchTplFrom = "error fetching templateFrom data: %w" + errApplyTemplate = "could not apply template: %w" + errExecTpl = "could not execute template: %w" + errMutate = "unable to mutate secret %s: %w" + errUpdate = "unable to update secret %s: %w" + errUpdateNotFound = "unable to update secret %s: not found" + errDeleteCreatePolicy = "unable to delete secret %s: creationPolicy=%s is not Owner" + errSecretCachesNotSynced = "controller caches for secret %s are not in sync" ) -const externalSecretSecretNameKey = ".spec.target.name" +// these errors are explicitly defined so we can detect them with `errors.Is()`. +var ( + ErrSecretImmutable = fmt.Errorf("secret is immutable") + ErrSecretIsOwned = fmt.Errorf("secret is owned by another ExternalSecret") + ErrSecretSetCtrlRef = fmt.Errorf("could not set controller reference on secret") + ErrSecretRemoveCtrlRef = fmt.Errorf("could not remove controller reference on secret") +) + +const indexESTargetSecretNameField = ".metadata.targetSecretName" // Reconciler reconciles a ExternalSecret object. type Reconciler struct { client.Client + SecretClient client.Client Log logr.Logger Scheme *runtime.Scheme RestConfig *rest.Config @@ -98,7 +135,7 @@ type Reconciler struct { // Reconcile implements the main reconciliation loop // for watched objects (ExternalSecret, ClusterSecretStore and SecretStore), // and updates/creates a Kubernetes secret based on them. -func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, err error) { log := r.Log.WithValues("ExternalSecret", req.NamespacedName) resourceLabels := ctrlmetrics.RefineNonConditionMetricLabels(map[string]string{"name": req.Name, "namespace": req.Namespace}) @@ -112,11 +149,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu esmetrics.GetCounterVec(esmetrics.SyncCallsKey).With(resourceLabels).Inc() }() - var externalSecret esv1beta1.ExternalSecret - err := r.Get(ctx, req.NamespacedName, &externalSecret) - + externalSecret := &esv1beta1.ExternalSecret{} + err = r.Get(ctx, req.NamespacedName, externalSecret) if err != nil { if apierrors.IsNotFound(err) { + // NOTE: this does not actually set the condition on the ExternalSecret, because it does not exist + // this is a hack to disable metrics for deleted ExternalSecrets, see: + // https://github.com/external-secrets/external-secrets/pull/612 conditionSynced := NewExternalSecretCondition(esv1beta1.ExternalSecretDeleted, v1.ConditionFalse, esv1beta1.ConditionReasonSecretDeleted, "Secret was deleted") SetExternalSecretCondition(&esv1beta1.ExternalSecret{ ObjectMeta: metav1.ObjectMeta{ @@ -128,345 +167,553 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{}, nil } - log.Error(err, errGetES) + log.Error(err, logErrorGetES) syncCallsError.With(resourceLabels).Inc() - return ctrl.Result{}, err } - // See https://github.com/external-secrets/external-secrets/issues/3604 - // We fetch the ExternalSecret resource above, however the status subresource is inconsistent. - // We have to explicitly fetch it, otherwise it may be missing and will cause - // unexpected side effects. - err = r.SubResource("status").Get(ctx, &externalSecret, &externalSecret) - if err != nil { - log.Error(err, "failed to get status subresource") - return ctrl.Result{}, err - } - - timeSinceLastRefresh := 0 * time.Second - if !externalSecret.Status.RefreshTime.IsZero() { - timeSinceLastRefresh = time.Since(externalSecret.Status.RefreshTime.Time) - } - // skip reconciliation if deletion timestamp is set on external secret - if externalSecret.DeletionTimestamp != nil { - log.Info("skipping as it is in deletion") + if !externalSecret.GetDeletionTimestamp().IsZero() { + log.V(1).Info("skipping ExternalSecret, it is marked for deletion") return ctrl.Result{}, nil } // if extended metrics is enabled, refine the time series vector resourceLabels = ctrlmetrics.RefineLabels(resourceLabels, externalSecret.Labels) + // skip this ExternalSecret if it uses a ClusterSecretStore and the feature is disabled if shouldSkipClusterSecretStore(r, externalSecret) { - log.Info("skipping cluster secret store as it is disabled") + log.V(1).Info("skipping ExternalSecret, ClusterSecretStore feature is disabled") return ctrl.Result{}, nil } - // skip when pointing to an unmanaged store + // skip this ExternalSecret if it uses any SecretStore not managed by this controller skip, err := shouldSkipUnmanagedStore(ctx, req.Namespace, r, externalSecret) + if err != nil { + log.Error(err, logErrorUnmanagedStore) + syncCallsError.With(resourceLabels).Inc() + return ctrl.Result{}, err + } if skip { - log.Info("skipping unmanaged store as it points to a unmanaged controllerClass") + log.V(1).Info("skipping ExternalSecret, uses unmanaged SecretStore") return ctrl.Result{}, nil } - refreshInt := r.RequeueInterval - if externalSecret.Spec.RefreshInterval != nil { - refreshInt = externalSecret.Spec.RefreshInterval.Duration - } - - // Target Secret Name should default to the ExternalSecret name if not explicitly specified + // the target secret name defaults to the ExternalSecret name, if not explicitly set secretName := externalSecret.Spec.Target.Name if secretName == "" { - secretName = externalSecret.ObjectMeta.Name + secretName = externalSecret.Name } - // fetch external secret, we need to ensure that it exists, and it's hashmap corresponds - var existingSecret v1.Secret - err = r.Get(ctx, types.NamespacedName{ - Name: secretName, - Namespace: externalSecret.Namespace, - }, &existingSecret) + // fetch the existing secret (from the partial cache) + // - please note that the ~partial cache~ is different from the ~full cache~ + // so there can be race conditions between the two caches + // - the WatchesMetadata(v1.Secret{}) in SetupWithManager() is using the partial cache + // so we might receive a reconcile request before the full cache is updated + // - furthermore, when `--enable-managed-secrets-caching` is true, the full cache + // will ONLY include secrets with the "managed" label, so we cant use the full cache + // to reliably determine if a secret exists or not + secretPartial := &metav1.PartialObjectMetadata{} + secretPartial.SetGroupVersionKind(v1.SchemeGroupVersion.WithKind("Secret")) + err = r.Get(ctx, client.ObjectKey{Name: secretName, Namespace: externalSecret.Namespace}, secretPartial) if err != nil && !apierrors.IsNotFound(err) { - log.Error(err, errGetExistingSecret) + log.Error(err, logErrorGetSecret, "secretName", secretName, "secretNamespace", externalSecret.Namespace) + syncCallsError.With(resourceLabels).Inc() return ctrl.Result{}, err } - // refresh should be skipped if - // 1. resource generation hasn't changed - // 2. refresh interval is 0 - // 3. if we're still within refresh-interval - if !shouldRefresh(externalSecret) && isSecretValid(existingSecret) { - refreshInt = (externalSecret.Spec.RefreshInterval.Duration - timeSinceLastRefresh) + 5*time.Second - log.V(1).Info("skipping refresh", "rv", getResourceVersion(externalSecret), "nr", refreshInt.Seconds()) - return ctrl.Result{RequeueAfter: refreshInt}, nil - } - if !shouldReconcile(externalSecret) { - log.V(1).Info("stopping reconciling", "rv", getResourceVersion(externalSecret)) - return ctrl.Result{}, nil + // if the secret exists but does not have the "managed" label, add the label + // using a PATCH so it is visible in the cache, then requeue immediately + if secretPartial.UID != "" && secretPartial.Labels[esv1beta1.LabelManaged] != esv1beta1.LabelManagedValue { + fqdn := fmt.Sprintf(fieldOwnerTemplate, externalSecret.Name) + patch := client.MergeFrom(secretPartial.DeepCopy()) + if secretPartial.Labels == nil { + secretPartial.Labels = make(map[string]string) + } + secretPartial.Labels[esv1beta1.LabelManaged] = esv1beta1.LabelManagedValue + err = r.Patch(ctx, secretPartial, patch, client.FieldOwner(fqdn)) + if err != nil { + log.Error(err, logErrorPatchSecret, "secretName", secretName, "secretNamespace", externalSecret.Namespace) + syncCallsError.With(resourceLabels).Inc() + return ctrl.Result{}, err + } + return ctrl.Result{Requeue: true}, nil } - // patch status when done processing - p := client.MergeFrom(externalSecret.DeepCopy()) + // fetch existing secret (from the full cache) + // NOTE: we are using the `r.SecretClient` which we only use for managed secrets. + // when `enableManagedSecretsCache` is true, this is a cached client that only sees our managed secrets, + // otherwise it will be the normal controller-runtime client which may be cached or make direct API calls, + // depending on if `enabledSecretCache` is true or false. + existingSecret := &v1.Secret{} + err = r.SecretClient.Get(ctx, client.ObjectKey{Name: secretName, Namespace: externalSecret.Namespace}, existingSecret) + if err != nil && !apierrors.IsNotFound(err) { + log.Error(err, logErrorGetSecret, "secretName", secretName, "secretNamespace", externalSecret.Namespace) + syncCallsError.With(resourceLabels).Inc() + return ctrl.Result{}, err + } + + // ensure the full cache is up-to-date + // NOTE: this prevents race conditions between the partial and full cache. + // we return an error so we get an exponential backoff if we end up looping, + // for example, during high cluster load and frequent updates to the target secret by other controllers. + if secretPartial.UID != existingSecret.UID || secretPartial.ResourceVersion != existingSecret.ResourceVersion { + err = fmt.Errorf(errSecretCachesNotSynced, secretName) + log.Error(err, logErrorSecretCacheNotSynced, "secretName", secretName, "secretNamespace", externalSecret.Namespace) + syncCallsError.With(resourceLabels).Inc() + return ctrl.Result{}, err + } + + // refresh will be skipped if ALL the following conditions are met: + // 1. refresh interval is not 0 + // 2. resource generation of the ExternalSecret has not changed + // 3. the last refresh time of the ExternalSecret is within the refresh interval + // 4. the target secret is valid: + // - it exists + // - it has the correct "managed" label + // - it has the correct "data-hash" annotation + if !shouldRefresh(externalSecret) && isSecretValid(existingSecret) { + log.V(1).Info("skipping refresh") + return r.getRequeueResult(externalSecret), nil + } + + // update status of the ExternalSecret when this function returns, if needed. + // NOTE: we use the ability of deferred functions to update named return values `result` and `err` + // NOTE: we dereference the DeepCopy of the status field because status fields are NOT pointers, + // so otherwise the `equality.Semantic.DeepEqual` will always return false. + currentStatus := *externalSecret.Status.DeepCopy() defer func() { - err = r.Status().Patch(ctx, &externalSecret, p) - if err != nil { - log.Error(err, errPatchStatus) + // if the status has not changed, we don't need to update it + if equality.Semantic.DeepEqual(currentStatus, externalSecret.Status) { + return + } + + // update the status of the ExternalSecret, storing any error in a new variable + // if there was no new error, we don't need to change the `result` or `err` values + updateErr := r.Status().Update(ctx, externalSecret) + if updateErr == nil { + return + } + + // if we got an update conflict, we should requeue immediately + if apierrors.IsConflict(updateErr) { + log.V(1).Info("conflict while updating status, will requeue") + + // we only explicitly request a requeue if the main function did not return an `err`. + // otherwise, we get an annoying log saying that results are ignored when there is an error, + // as errors are always retried. + if err == nil { + result = ctrl.Result{Requeue: true} + } + return + } + + // for other errors, log and update the `err` variable if there is no error already + // so the reconciler will requeue the request + log.Error(updateErr, logErrorUpdateESStatus) + if err == nil { + err = updateErr } }() - secret := &v1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: secretName, - Namespace: externalSecret.Namespace, - }, - Immutable: &externalSecret.Spec.Target.Immutable, - Data: make(map[string][]byte), - } - - dataMap, err := r.getProviderSecretData(ctx, &externalSecret) + // retrieve the provider secret data. + dataMap, err := r.getProviderSecretData(ctx, externalSecret) if err != nil { - r.markAsFailed(log, errGetSecretData, err, &externalSecret, syncCallsError.With(resourceLabels)) + r.markAsFailed(msgErrorGetSecretData, err, externalSecret, syncCallsError.With(resourceLabels)) return ctrl.Result{}, err } - // secret data was not modified. - if errors.Is(err, esv1beta1.NotModifiedErr) { - log.Info("secret was not modified as a NotModified was returned by the provider") - r.markAsDone(&externalSecret, start, log) - - return ctrl.Result{}, nil - } - // if no data was found we can delete the secret if needed. if len(dataMap) == 0 { switch externalSecret.Spec.Target.DeletionPolicy { // delete secret and return early. case esv1beta1.DeletionPolicyDelete: - // safeguard that we only can delete secrets we own - // this is also implemented in the es validation webhook - if externalSecret.Spec.Target.CreationPolicy != esv1beta1.CreatePolicyOwner { - err := fmt.Errorf(errInvalidCreatePolicy, externalSecret.Spec.Target.CreationPolicy) - r.markAsFailed(log, errDeleteSecret, err, &externalSecret, syncCallsError.With(resourceLabels)) - return ctrl.Result{}, err + // safeguard that we only can delete secrets we own. + // this is also implemented in the es validation webhook. + // NOTE: this error cant be fixed by retrying so we don't return an error (which would requeue immediately) + creationPolicy := externalSecret.Spec.Target.CreationPolicy + if creationPolicy != esv1beta1.CreatePolicyOwner { + err := fmt.Errorf(errDeleteCreatePolicy, secretName, creationPolicy) + r.markAsFailed(msgErrorDeleteSecret, err, externalSecret, syncCallsError.With(resourceLabels)) + return ctrl.Result{}, nil } - if err := r.Delete(ctx, secret); err != nil && !apierrors.IsNotFound(err) { - r.markAsFailed(log, errDeleteSecret, err, &externalSecret, syncCallsError.With(resourceLabels)) - return ctrl.Result{}, err + // delete the secret, if it exists + if existingSecret.UID != "" { + if err := r.Delete(ctx, existingSecret); err != nil && !apierrors.IsNotFound(err) { + r.markAsFailed(msgErrorDeleteSecret, err, externalSecret, syncCallsError.With(resourceLabels)) + return ctrl.Result{}, err + } } - conditionSynced := NewExternalSecretCondition(esv1beta1.ExternalSecretReady, v1.ConditionTrue, esv1beta1.ConditionReasonSecretDeleted, "secret deleted due to DeletionPolicy") - SetExternalSecretCondition(&externalSecret, *conditionSynced) - return ctrl.Result{RequeueAfter: refreshInt}, nil + r.markAsDone(externalSecret, start, log, esv1beta1.ConditionReasonSecretDeleted, msgDeleted) + return r.getRequeueResult(externalSecret), nil // In case provider secrets don't exist the kubernetes secret will be kept as-is. case esv1beta1.DeletionPolicyRetain: - r.markAsDone(&externalSecret, start, log) - return ctrl.Result{RequeueAfter: refreshInt}, nil + r.markAsDone(externalSecret, start, log, esv1beta1.ConditionReasonSecretSynced, msgSyncedRetain) + return r.getRequeueResult(externalSecret), nil // noop, handled below case esv1beta1.DeletionPolicyMerge: } } - mutationFunc := func() error { + // mutationFunc is a function which can be applied to a secret to make it match the desired state. + mutationFunc := func(secret *v1.Secret) error { + // get information about the current owner of the secret + // - we ignore the API version as it can change over time + // - we ignore the UID for consistency with the SetControllerReference function + currentOwner := metav1.GetControllerOf(secret) + ownerIsESKind := false + ownerIsCurrentES := false + if currentOwner != nil { + currentOwnerGK := schema.FromAPIVersionAndKind(currentOwner.APIVersion, currentOwner.Kind).GroupKind() + ownerIsESKind = currentOwnerGK.String() == esv1beta1.ExtSecretGroupKind + ownerIsCurrentES = ownerIsESKind && currentOwner.Name == externalSecret.Name + } + + // if another ExternalSecret is the owner, we should return an error + // otherwise the controller will fight with itself to update the secret. + // note, this does not prevent other controllers from owning the secret. + if ownerIsESKind && !ownerIsCurrentES { + return fmt.Errorf("%w: %s", ErrSecretIsOwned, currentOwner.Name) + } + + // if the CreationPolicy is Owner, we should set ourselves as the owner of the secret if externalSecret.Spec.Target.CreationPolicy == esv1beta1.CreatePolicyOwner { - err = controllerutil.SetControllerReference(&externalSecret, &secret.ObjectMeta, r.Scheme) + err = controllerutil.SetControllerReference(externalSecret, secret, r.Scheme) if err != nil { - return fmt.Errorf(errSetCtrlReference, err) + return fmt.Errorf("%w: %w", ErrSecretSetCtrlRef, err) } } + + // if the creation policy is not Owner, we should remove ourselves as the owner + // this could happen if the creation policy was changed after the secret was created + if externalSecret.Spec.Target.CreationPolicy != esv1beta1.CreatePolicyOwner && ownerIsCurrentES { + err = controllerutil.RemoveControllerReference(externalSecret, secret, r.Scheme) + if err != nil { + return fmt.Errorf("%w: %w", ErrSecretRemoveCtrlRef, err) + } + } + + // initialize maps within the secret so it's safe to set values + if secret.Annotations == nil { + secret.Annotations = make(map[string]string) + } + if secret.Labels == nil { + secret.Labels = make(map[string]string) + } if secret.Data == nil { secret.Data = make(map[string][]byte) } - // diff existing keys - keys, err := getManagedDataKeys(&existingSecret, externalSecret.Name) + + // get the list of keys that are managed by this ExternalSecret + keys, err := getManagedDataKeys(secret, externalSecret.Name) if err != nil { return err } - // Sanitize data map for any updates on the ES + + // remove any data keys that are managed by this ExternalSecret, so we can re-add them + // this ensures keys added by templates are not left behind when they are removed from the template for _, key := range keys { - if dataMap[key] == nil { - secret.Data[key] = nil - // Sanitizing any templated / updated keys - delete(secret.Data, key) - } + delete(secret.Data, key) } - err = r.applyTemplate(ctx, &externalSecret, secret, dataMap) + + // WARNING: this will remove any labels or annotations managed by this ExternalSecret + // so any updates to labels and annotations should be done AFTER this point + err = r.applyTemplate(ctx, externalSecret, secret, dataMap) if err != nil { return fmt.Errorf(errApplyTemplate, err) } + + // set the immutable flag on the secret if requested by the ExternalSecret + if externalSecret.Spec.Target.Immutable { + secret.Immutable = ptr.To(true) + } + + // we also use a label to keep track of the owner of the secret + // this lets us remove secrets that are no longer needed if the target secret name changes if externalSecret.Spec.Target.CreationPolicy == esv1beta1.CreatePolicyOwner { lblValue := utils.ObjectHash(fmt.Sprintf("%v/%v", externalSecret.Namespace, externalSecret.Name)) secret.Labels[esv1beta1.LabelOwner] = lblValue + } else { + // the label should not be set if the creation policy is not Owner + delete(secret.Labels, esv1beta1.LabelOwner) } - secret.Annotations[esv1beta1.AnnotationDataHash] = r.computeDataHashAnnotation(&existingSecret, secret) + secret.Labels[esv1beta1.LabelManaged] = esv1beta1.LabelManagedValue + secret.Annotations[esv1beta1.AnnotationDataHash] = utils.ObjectHash(secret.Data) return nil } switch externalSecret.Spec.Target.CreationPolicy { //nolint:exhaustive case esv1beta1.CreatePolicyMerge: - err = r.patchSecret(ctx, secret, mutationFunc, &externalSecret) - if err == nil { - externalSecret.Status.Binding = v1.LocalObjectReference{Name: secret.Name} + // update the secret, if it exists + if existingSecret.UID != "" { + err = r.updateSecret(ctx, existingSecret, mutationFunc, externalSecret, secretName) + } else { + // if the secret does not exist, we wait until the next refresh interval + // rather than returning an error which would requeue immediately + r.markAsDone(externalSecret, start, log, esv1beta1.ConditionReasonSecretMissing, msgMissing) + return r.getRequeueResult(externalSecret), nil } case esv1beta1.CreatePolicyNone: log.V(1).Info("secret creation skipped due to creationPolicy=None") err = nil default: - var created bool - created, err = r.createOrUpdateSecret(ctx, secret, mutationFunc, &externalSecret) - if err == nil { - externalSecret.Status.Binding = v1.LocalObjectReference{Name: secret.Name} - } - // cleanup orphaned secrets - if created { - delErr := deleteOrphanedSecrets(ctx, r.Client, &externalSecret) - if delErr != nil { - msg := fmt.Sprintf("failed to clean up orphaned secrets: %v", delErr) - r.markAsFailed(log, msg, delErr, &externalSecret, syncCallsError.With(resourceLabels)) - return ctrl.Result{}, delErr + // create the secret, if it does not exist + if existingSecret.UID == "" { + err = r.createSecret(ctx, mutationFunc, externalSecret, secretName) + + // we may have orphaned secrets to clean up, + // for example, if the target secret name was changed + if err == nil { + delErr := deleteOrphanedSecrets(ctx, r.Client, externalSecret, secretName) + if delErr != nil { + r.markAsFailed(msgErrorDeleteOrphaned, delErr, externalSecret, syncCallsError.With(resourceLabels)) + return ctrl.Result{}, delErr + } } + } else { + // update the secret, if it exists + err = r.updateSecret(ctx, existingSecret, mutationFunc, externalSecret, secretName) } } - if err != nil { - r.markAsFailed(log, errUpdateSecret, err, &externalSecret, syncCallsError.With(resourceLabels)) + // if we got an update conflict, we should requeue immediately + if apierrors.IsConflict(err) { + log.V(1).Info("conflict while updating secret, will requeue") + return ctrl.Result{Requeue: true}, nil + } + + // detect errors indicating that we failed to set ourselves as the owner of the secret + // NOTE: this error cant be fixed by retrying so we don't return an error (which would requeue immediately) + if errors.Is(err, ErrSecretSetCtrlRef) { + r.markAsFailed(msgErrorBecomeOwner, err, externalSecret, syncCallsError.With(resourceLabels)) + return ctrl.Result{}, nil + } + + // detect errors indicating that the secret has another ExternalSecret as owner + // NOTE: this error cant be fixed by retrying so we don't return an error (which would requeue immediately) + if errors.Is(err, ErrSecretIsOwned) { + r.markAsFailed(msgErrorIsOwned, err, externalSecret, syncCallsError.With(resourceLabels)) + return ctrl.Result{}, nil + } + + // detect errors indicating that the secret is immutable + // NOTE: this error cant be fixed by retrying so we don't return an error (which would requeue immediately) + if errors.Is(err, ErrSecretImmutable) { + r.markAsFailed(msgErrorUpdateImmutable, err, externalSecret, syncCallsError.With(resourceLabels)) + return ctrl.Result{}, nil + } + + r.markAsFailed(msgErrorUpdateSecret, err, externalSecret, syncCallsError.With(resourceLabels)) return ctrl.Result{}, err } - r.markAsDone(&externalSecret, start, log) - - return ctrl.Result{ - RequeueAfter: refreshInt, - }, nil + r.markAsDone(externalSecret, start, log, esv1beta1.ConditionReasonSecretSynced, msgSynced) + return r.getRequeueResult(externalSecret), nil } -func (r *Reconciler) markAsDone(externalSecret *esv1beta1.ExternalSecret, start time.Time, log logr.Logger) { - conditionSynced := NewExternalSecretCondition(esv1beta1.ExternalSecretReady, v1.ConditionTrue, esv1beta1.ConditionReasonSecretSynced, "Secret was synced") - currCond := GetExternalSecretCondition(externalSecret.Status, esv1beta1.ExternalSecretReady) - SetExternalSecretCondition(externalSecret, *conditionSynced) +// getRequeueResult create a result with requeueAfter based on the ExternalSecret refresh interval. +func (r *Reconciler) getRequeueResult(externalSecret *esv1beta1.ExternalSecret) ctrl.Result { + // default to the global requeue interval + // note, this will never be used because the CRD has a default value of 1 hour + refreshInterval := r.RequeueInterval + if externalSecret.Spec.RefreshInterval != nil { + refreshInterval = externalSecret.Spec.RefreshInterval.Duration + } + + // if the refresh interval is <= 0, we should not requeue + if refreshInterval <= 0 { + return ctrl.Result{} + } + + // if the last refresh time is not set, requeue after the refresh interval + // note, this should not happen, as we only call this function on ExternalSecrets + // that have been reconciled at least once + if externalSecret.Status.RefreshTime.IsZero() { + return ctrl.Result{RequeueAfter: refreshInterval} + } + + timeSinceLastRefresh := time.Since(externalSecret.Status.RefreshTime.Time) + + // if the last refresh time is in the future, we should requeue immediately + // note, this should not happen, as we always refresh an ExternalSecret + // that has a last refresh time in the future + if timeSinceLastRefresh < 0 { + return ctrl.Result{Requeue: true} + } + + // if there is time remaining, requeue after the remaining time + if timeSinceLastRefresh < refreshInterval { + return ctrl.Result{RequeueAfter: refreshInterval - timeSinceLastRefresh} + } + + // otherwise, requeue immediately + return ctrl.Result{Requeue: true} +} + +func (r *Reconciler) markAsDone(externalSecret *esv1beta1.ExternalSecret, start time.Time, log logr.Logger, reason, msg string) { + oldReadyCondition := GetExternalSecretCondition(externalSecret.Status, esv1beta1.ExternalSecretReady) + newReadyCondition := NewExternalSecretCondition(esv1beta1.ExternalSecretReady, v1.ConditionTrue, reason, msg) + SetExternalSecretCondition(externalSecret, *newReadyCondition) + externalSecret.Status.RefreshTime = metav1.NewTime(start) - externalSecret.Status.SyncedResourceVersion = getResourceVersion(*externalSecret) - if currCond == nil || currCond.Status != conditionSynced.Status { - log.Info("reconciled secret") // Log once if on success in any verbosity + externalSecret.Status.SyncedResourceVersion = getResourceVersion(externalSecret) + + // if the status or reason has changed, log at the appropriate verbosity level + if oldReadyCondition == nil || oldReadyCondition.Status != newReadyCondition.Status || oldReadyCondition.Reason != newReadyCondition.Reason { + if newReadyCondition.Reason == esv1beta1.ConditionReasonSecretDeleted { + log.Info("deleted secret") + } else { + log.Info("reconciled secret") + } } else { - log.V(1).Info("reconciled secret") // Log all reconciliation cycles if higher verbosity applied + log.V(1).Info("reconciled secret") } } -func (r *Reconciler) markAsFailed(log logr.Logger, msg string, err error, externalSecret *esv1beta1.ExternalSecret, counter prometheus.Counter) { - log.Error(err, msg) +func (r *Reconciler) markAsFailed(msg string, err error, externalSecret *esv1beta1.ExternalSecret, counter prometheus.Counter) { r.recorder.Event(externalSecret, v1.EventTypeWarning, esv1beta1.ReasonUpdateFailed, err.Error()) conditionSynced := NewExternalSecretCondition(esv1beta1.ExternalSecretReady, v1.ConditionFalse, esv1beta1.ConditionReasonSecretSyncedError, msg) SetExternalSecretCondition(externalSecret, *conditionSynced) counter.Inc() } -func deleteOrphanedSecrets(ctx context.Context, cl client.Client, externalSecret *esv1beta1.ExternalSecret) error { - secretList := v1.SecretList{} - lblValue := utils.ObjectHash(fmt.Sprintf("%v/%v", externalSecret.Namespace, externalSecret.Name)) - ls := &metav1.LabelSelector{ - MatchLabels: map[string]string{ - esv1beta1.LabelOwner: lblValue, - }, +func deleteOrphanedSecrets(ctx context.Context, cl client.Client, externalSecret *esv1beta1.ExternalSecret, secretName string) error { + ownerLabel := utils.ObjectHash(fmt.Sprintf("%v/%v", externalSecret.Namespace, externalSecret.Name)) + + secretListPartial := &metav1.PartialObjectMetadataList{} + secretListPartial.SetGroupVersionKind(v1.SchemeGroupVersion.WithKind("SecretList")) + listOpts := &client.ListOptions{ + LabelSelector: labels.SelectorFromSet(map[string]string{ + esv1beta1.LabelOwner: ownerLabel, + }), + Namespace: externalSecret.Namespace, } - labelSelector, err := metav1.LabelSelectorAsSelector(ls) - if err != nil { + if err := cl.List(ctx, secretListPartial, listOpts); err != nil { return err } - err = cl.List(ctx, &secretList, &client.ListOptions{LabelSelector: labelSelector}) - if err != nil { - return err - } - for key, secret := range secretList.Items { - if externalSecret.Spec.Target.Name != "" && secret.Name != externalSecret.Spec.Target.Name { - err = cl.Delete(ctx, &secretList.Items[key]) - if err != nil { + + // delete all secrets that are not the target secret + for _, secretPartial := range secretListPartial.Items { + if secretPartial.GetName() != secretName { + if err := cl.Delete(ctx, &secretPartial); err != nil { return err } } } + return nil } -func (r *Reconciler) createOrUpdateSecret(ctx context.Context, secret *v1.Secret, mutationFunc func() error, es *esv1beta1.ExternalSecret) (bool, error) { +// createSecret creates a new secret with the given mutation function. +func (r *Reconciler) createSecret(ctx context.Context, mutationFunc func(secret *v1.Secret) error, es *esv1beta1.ExternalSecret, secretName string) error { fqdn := fmt.Sprintf(fieldOwnerTemplate, es.Name) - key := client.ObjectKeyFromObject(secret) - if err := r.Client.Get(ctx, key, secret); err != nil { - if !apierrors.IsNotFound(err) { - return false, err - } - if err := mutationFunc(); err != nil { - return false, err - } - // Setting Field Owner even for CreationPolicy==Create - if err := r.Client.Create(ctx, secret, client.FieldOwner(fqdn)); err != nil { - return false, err - } - r.recorder.Event(es, v1.EventTypeNormal, esv1beta1.ReasonCreated, "Created Secret") - return true, nil - } - existing := secret.DeepCopyObject() - if err := mutationFunc(); err != nil { - return false, err + // define and mutate the new secret + newSecret := &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: es.Namespace, + }, + Data: make(map[string][]byte), } - - if equality.Semantic.DeepEqual(existing, secret) { - return false, nil - } - - if err := r.Client.Update(ctx, secret, client.FieldOwner(fqdn)); err != nil { - return false, err - } - r.recorder.Event(es, v1.EventTypeNormal, esv1beta1.ReasonUpdated, "Updated Secret") - return false, nil -} - -func (r *Reconciler) patchSecret(ctx context.Context, secret *v1.Secret, mutationFunc func() error, es *esv1beta1.ExternalSecret) error { - fqdn := fmt.Sprintf(fieldOwnerTemplate, es.Name) - err := r.Client.Get(ctx, client.ObjectKeyFromObject(secret), secret.DeepCopy()) - if apierrors.IsNotFound(err) { - return fmt.Errorf(errPolicyMergeNotFound, secret.Name) - } - if err != nil { - return fmt.Errorf(errPolicyMergeGetSecret, secret.Name, err) - } - - existing := secret.DeepCopyObject() - if err = mutationFunc(); err != nil { - return fmt.Errorf(errPolicyMergeMutate, secret.Name, err) - } - - // GVK is missing in the Secret, see: - // https://github.com/kubernetes-sigs/controller-runtime/issues/526 - // https://github.com/kubernetes-sigs/controller-runtime/issues/1517 - // https://github.com/kubernetes/kubernetes/issues/80609 - // we need to manually set it before doing a Patch() as it depends on the GVK - gvks, unversioned, err := r.Scheme.ObjectKinds(secret) - if err != nil { + if err := mutationFunc(newSecret); err != nil { return err } - if !unversioned && len(gvks) == 1 { - secret.SetGroupVersionKind(gvks[0]) + + // note, we set field owner even for Create + if err := r.Create(ctx, newSecret, client.FieldOwner(fqdn)); err != nil { + return err } - if equality.Semantic.DeepEqual(existing, secret) { + // set the binding reference to the secret + // https://github.com/external-secrets/external-secrets/pull/2263 + es.Status.Binding = v1.LocalObjectReference{Name: newSecret.Name} + + r.recorder.Event(es, v1.EventTypeNormal, esv1beta1.ReasonCreated, "Created Secret") + return nil +} + +func (r *Reconciler) updateSecret(ctx context.Context, existingSecret *v1.Secret, mutationFunc func(secret *v1.Secret) error, es *esv1beta1.ExternalSecret, secretName string) error { + fqdn := fmt.Sprintf(fieldOwnerTemplate, es.Name) + + // fail if the secret does not exist + // this should never happen because we check this before calling this function + if existingSecret.UID == "" { + return fmt.Errorf(errUpdateNotFound, secretName) + } + + // set the binding reference to the secret + // https://github.com/external-secrets/external-secrets/pull/2263 + es.Status.Binding = v1.LocalObjectReference{Name: secretName} + + // mutate a copy of the existing secret with the mutation function + updatedSecret := existingSecret.DeepCopy() + if err := mutationFunc(updatedSecret); err != nil { + return fmt.Errorf(errMutate, updatedSecret.Name, err) + } + + // if the secret does not need to be updated, return early + if equality.Semantic.DeepEqual(existingSecret, updatedSecret) { return nil } - // Cleaning up Managed fields manually as to keep patch coherence - secret.ObjectMeta.ManagedFields = nil - // we're not able to resolve conflicts so we force ownership - // see: https://kubernetes.io/docs/reference/using-api/server-side-apply/#using-server-side-apply-in-a-controller - if err := r.Client.Patch(ctx, secret, client.Apply, client.FieldOwner(fqdn), client.ForceOwnership); err != nil { - return fmt.Errorf(errPolicyMergePatch, secret.Name, err) + + // if the existing secret is immutable, we can only update the object metadata + if ptr.Deref(existingSecret.Immutable, false) { + // check if the metadata was changed + metadataChanged := !equality.Semantic.DeepEqual(existingSecret.ObjectMeta, updatedSecret.ObjectMeta) + + // check if the immutable data/type was changed + var dataChanged bool + if metadataChanged { + // update the `existingSecret` object with the metadata from `updatedSecret` + // this lets us compare the objects to see if the immutable data/type was changed + existingSecret.ObjectMeta = *updatedSecret.ObjectMeta.DeepCopy() + dataChanged = !equality.Semantic.DeepEqual(existingSecret, updatedSecret) + + // because we use labels and annotations to keep track of the secret, + // we need to update the metadata, regardless of if the immutable data was changed + // NOTE: we are using the `existingSecret` object here, as we ONLY want to update the metadata, + // and we previously copied the metadata from the `updatedSecret` object + if err := r.Update(ctx, existingSecret, client.FieldOwner(fqdn)); err != nil { + // if we get a conflict, we should return early to requeue immediately + // note, we don't wrap this error so we can handle it in the caller + if apierrors.IsConflict(err) { + return err + } + return fmt.Errorf(errUpdate, existingSecret.Name, err) + } + } else { + // we know there was some change in the secret (or we would have returned early) + // we know the metadata was NOT changed (metadataChanged == false) + // so, the only thing that could have changed is the immutable data/type fields + dataChanged = true + } + + // if the immutable data was changed, we should return an error + if dataChanged { + return fmt.Errorf(errUpdate, existingSecret.Name, ErrSecretImmutable) + } } + + // update the secret + if err := r.Update(ctx, updatedSecret, client.FieldOwner(fqdn)); err != nil { + // if we get a conflict, we should return early to requeue immediately + // note, we don't wrap this error so we can handle it in the caller + if apierrors.IsConflict(err) { + return err + } + return fmt.Errorf(errUpdate, updatedSecret.Name, err) + } + r.recorder.Event(es, v1.EventTypeNormal, esv1beta1.ReasonUpdated, "Updated Secret") return nil } +// getManagedDataKeys returns the list of data keys in a secret which are managed by a specified owner. func getManagedDataKeys(secret *v1.Secret, fieldOwner string) ([]string, error) { return getManagedFieldKeys(secret, fieldOwner, func(fields map[string]any) []string { dataFields := fields["f:data"] @@ -508,29 +755,31 @@ func getManagedFieldKeys( return keys, nil } -func getResourceVersion(es esv1beta1.ExternalSecret) string { +func getResourceVersion(es *esv1beta1.ExternalSecret) string { return fmt.Sprintf("%d-%s", es.ObjectMeta.GetGeneration(), hashMeta(es.ObjectMeta)) } +// hashMeta returns a consistent hash of the `metadata.labels` and `metadata.annotations` fields of the given object. func hashMeta(m metav1.ObjectMeta) string { type meta struct { annotations map[string]string labels map[string]string } - return utils.ObjectHash(meta{ + objectMeta := meta{ annotations: m.Annotations, labels: m.Labels, - }) + } + return utils.ObjectHash(objectMeta) } -func shouldSkipClusterSecretStore(r *Reconciler, es esv1beta1.ExternalSecret) bool { +func shouldSkipClusterSecretStore(r *Reconciler, es *esv1beta1.ExternalSecret) bool { return !r.ClusterSecretStoreEnabled && es.Spec.SecretStoreRef.Kind == esv1beta1.ClusterSecretStoreKind } // shouldSkipUnmanagedStore iterates over all secretStore references in the externalSecret spec, // fetches the store and evaluates the controllerClass property. // Returns true if any storeRef points to store with a non-matching controllerClass. -func shouldSkipUnmanagedStore(ctx context.Context, namespace string, r *Reconciler, es esv1beta1.ExternalSecret) (bool, error) { +func shouldSkipUnmanagedStore(ctx context.Context, namespace string, r *Reconciler, es *esv1beta1.ExternalSecret) (bool, error) { var storeList []esv1beta1.SecretStoreRef if es.Spec.SecretStoreRef.Name != "" { @@ -552,6 +801,10 @@ func shouldSkipUnmanagedStore(ctx context.Context, namespace string, r *Reconcil if ref.SourceRef != nil && ref.SourceRef.GeneratorRef != nil { _, obj, err := resolvers.GeneratorRef(ctx, r.RestConfig, namespace, ref.SourceRef.GeneratorRef) if err != nil { + if apierrors.IsNotFound(err) { + // skip non-existent generators + continue + } return false, err } skipGenerator, err := shouldSkipGenerator(r, obj) @@ -575,11 +828,15 @@ func shouldSkipUnmanagedStore(ctx context.Context, namespace string, r *Reconcil namespace = "" } - err := r.Client.Get(ctx, types.NamespacedName{ + err := r.Get(ctx, types.NamespacedName{ Name: ref.Name, Namespace: namespace, }, store) if err != nil { + if apierrors.IsNotFound(err) { + // skip non-existent stores + continue + } return false, err } class := store.GetSpec().Controller @@ -590,110 +847,162 @@ func shouldSkipUnmanagedStore(ctx context.Context, namespace string, r *Reconcil return false, nil } -func shouldRefresh(es esv1beta1.ExternalSecret) bool { - // refresh if resource version changed +func shouldRefresh(es *esv1beta1.ExternalSecret) bool { + // if the refresh interval is 0, and we have synced previously, we should not refresh + if es.Spec.RefreshInterval.Duration <= 0 && es.Status.SyncedResourceVersion != "" { + return false + } + + // if the ExternalSecret has been updated, we should refresh if es.Status.SyncedResourceVersion != getResourceVersion(es) { return true } - // skip refresh if refresh interval is 0 - if es.Spec.RefreshInterval.Duration == 0 && es.Status.SyncedResourceVersion != "" { - return false - } + // if the last refresh time is zero, we should refresh if es.Status.RefreshTime.IsZero() { return true } + + // if the last refresh time is in the future, we should refresh + if es.Status.RefreshTime.Time.After(time.Now()) { + return true + } + + // if the last refresh time + refresh interval is before now, we should refresh return es.Status.RefreshTime.Add(es.Spec.RefreshInterval.Duration).Before(time.Now()) } -func shouldReconcile(es esv1beta1.ExternalSecret) bool { - if es.Spec.Target.Immutable && hasSyncedCondition(es) { - return false - } - return true -} - -func hasSyncedCondition(es esv1beta1.ExternalSecret) bool { - for _, condition := range es.Status.Conditions { - if condition.Reason == "SecretSynced" { - return true - } - } - return false -} - // isSecretValid checks if the secret exists, and it's data is consistent with the calculated hash. -func isSecretValid(existingSecret v1.Secret) bool { - // if target secret doesn't exist, or annotations as not set, we need to refresh - if existingSecret.UID == "" || existingSecret.Annotations == nil { +func isSecretValid(existingSecret *v1.Secret) bool { + // if target secret doesn't exist, we need to refresh + if existingSecret.UID == "" { return false } - // if the calculated hash is different from the calculation, then it's invalid + // if the managed label is missing or incorrect, then it's invalid + if existingSecret.Labels[esv1beta1.LabelManaged] != esv1beta1.LabelManagedValue { + return false + } + + // if the data-hash annotation is missing or incorrect, then it's invalid + // this is how we know if the data has chanced since we last updated the secret if existingSecret.Annotations[esv1beta1.AnnotationDataHash] != utils.ObjectHash(existingSecret.Data) { return false } + return true } -// computeDataHashAnnotation generate a hash of the secret data combining the old key with the new keys to add or override. -func (r *Reconciler) computeDataHashAnnotation(existing, secret *v1.Secret) string { - data := make(map[string][]byte) - maps.Insert(data, maps.All(existing.Data)) - maps.Insert(data, maps.All(secret.Data)) - - return utils.ObjectHash(data) -} - // SetupWithManager returns a new controller builder that will be started by the provided Manager. func (r *Reconciler) SetupWithManager(mgr ctrl.Manager, opts controller.Options) error { r.recorder = mgr.GetEventRecorderFor("external-secrets") - // Index .Spec.Target.Name to reconcile ExternalSecrets effectively when secrets have changed - if err := mgr.GetFieldIndexer().IndexField(context.Background(), &esv1beta1.ExternalSecret{}, externalSecretSecretNameKey, func(obj client.Object) []string { + // index ExternalSecrets based on the target secret name, + // this lets us quickly find all ExternalSecrets which target a specific Secret + if err := mgr.GetFieldIndexer().IndexField(context.Background(), &esv1beta1.ExternalSecret{}, indexESTargetSecretNameField, func(obj client.Object) []string { es := obj.(*esv1beta1.ExternalSecret) - - if name := es.Spec.Target.Name; name != "" { - return []string{name} + // if the target name is set, use that as the index + if es.Spec.Target.Name != "" { + return []string{es.Spec.Target.Name} } + // otherwise, use the ExternalSecret name return []string{es.Name} }); err != nil { return err } + // predicate function to ignore secret events unless they have the "managed" label + secretHasESLabel := predicate.NewPredicateFuncs(func(object client.Object) bool { + value, hasLabel := object.GetLabels()[esv1beta1.LabelManaged] + return hasLabel && value == esv1beta1.LabelManagedValue + }) + return ctrl.NewControllerManagedBy(mgr). WithOptions(opts). For(&esv1beta1.ExternalSecret{}). - // Cannot use Owns since the controller does not set owner reference when creation policy is not Owner - Watches( + // we cant use Owns(), as we don't set ownerReferences when the creationPolicy is not Owner. + // we use WatchesMetadata() to reduce memory usage, as otherwise we have to process full secret objects. + WatchesMetadata( &v1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.findObjectsForSecret), - builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}), - builder.OnlyMetadata, + builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}, secretHasESLabel), ). Complete(r) } func (r *Reconciler) findObjectsForSecret(ctx context.Context, secret client.Object) []reconcile.Request { - var externalSecrets esv1beta1.ExternalSecretList - err := r.List( - ctx, - &externalSecrets, - client.InNamespace(secret.GetNamespace()), - client.MatchingFields{externalSecretSecretNameKey: secret.GetName()}, - ) + externalSecretsList := &esv1beta1.ExternalSecretList{} + listOps := &client.ListOptions{ + FieldSelector: fields.OneTermEqualSelector(indexESTargetSecretNameField, secret.GetName()), + Namespace: secret.GetNamespace(), + } + err := r.List(ctx, externalSecretsList, listOps) if err != nil { return []reconcile.Request{} } - requests := make([]reconcile.Request, len(externalSecrets.Items)) - for i := range externalSecrets.Items { + requests := make([]reconcile.Request, len(externalSecretsList.Items)) + for i, item := range externalSecretsList.Items { requests[i] = reconcile.Request{ NamespacedName: types.NamespacedName{ - Name: externalSecrets.Items[i].GetName(), - Namespace: externalSecrets.Items[i].GetNamespace(), + Name: item.GetName(), + Namespace: item.GetNamespace(), }, } } 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 +} diff --git a/pkg/controllers/externalsecret/externalsecret_controller_template.go b/pkg/controllers/externalsecret/externalsecret_controller_template.go index 73fb35edd..19eb8c48a 100644 --- a/pkg/controllers/externalsecret/externalsecret_controller_template.go +++ b/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 diff --git a/pkg/controllers/externalsecret/externalsecret_controller_test.go b/pkg/controllers/externalsecret/externalsecret_controller_test.go index 99b2d2b22..525e49795 100644 --- a/pkg/controllers/externalsecret/externalsecret_controller_test.go +++ b/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()) diff --git a/pkg/controllers/externalsecret/suite_test.go b/pkg/controllers/externalsecret/suite_test.go index d99e0b1e5..b8faa46c8 100644 --- a/pkg/controllers/externalsecret/suite_test.go +++ b/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"), diff --git a/pkg/controllers/secretstore/client_manager.go b/pkg/controllers/secretstore/client_manager.go index dc8d5bee4..077380e92 100644 --- a/pkg/controllers/secretstore/client_manager.go +++ b/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 }