Problem Statement

This PR significantly improves the performance and stability of the ExternalSecrets controller.

While there are many changes (see below), I want to summarize the goals of this PR:

1. Reduce the number of Kubernetes API calls made by the ExternalSecrets controller.
2. Only make UPDATE calls to resources when it is necessary.

Related Issue

• Random dealy in creating or fetching new secrets in namespace (between 4m-7m) #2837
• ExternalSecret synchronization speed degradation #3258
• Container exits on etcd timeout #1725
• Client side throttling with many secrets and low refresh interval #1336
• closes Immutable field prevents secret from being re-created if it is deleted externally #3920
• closes Immutable target isn't respected on first creation when creationPolicy is "Orphan" #3919

Proposed Changes

I have tested the PR for most use-cases I can think of, and have ensured that the make test pass, but I can't run make test.e2e locally.

However, there may be subtle behavior changes that I have not considered, so I would appreciate a thorough review.

The following sections cover the specific changes made in this PR.

Changes to caching:

We have a new controller flag --enable-managed-secrets-caching which defaults to true.
Also, we have a new reconcile.external-secrets.io/managed=true label which is added to all target secrets managed by ExternalSecrets.

Previously, we were not caching any Secrets (when --enable-secrets-caching=false, which is the default).
This was causing a lot of unnecessary API calls during every reconcile of each ExternalSecret.
To reduce the number of API calls and still avoid high memory usage on clusters with many secrets, we now have a special cache that only caches secrets with the reconcile.external-secrets.io/managed=true label.

Users can disable this cache by setting --enable-managed-secrets-caching=false.
However, this is not recommended, as it's better to increase the memory of the controller to handle all the secrets you want to manage with ExternalSecrets.

Changes when updating target secrets:

We now only update the target secret if the data has changed.

We use the equality.Semantic.DeepEqual() to ensure we would be making a change before we do it.

This change is critical for systems like Kyverno, as they rely on admission events to trigger restarts of deployments when secrets are updated. For example, previously, if an ExternalSecret did not set spect.target.template.type, it would do an update call every reconcile because it would try and unset the Opaque default value.

Changes when spec.creationPolicy=Owner:

Previously, some keys might be retained, if they existed previously on the secret before the ExternalSecret took ownership of it.

Now, the ExternalSecret defines the full "desired state" of the target Secret, and any extraneous data keys will be removed. (WARNING: this might mean your target secret keys are "cleaned up" when updating to this PR).

Changes when data key is removed from target template:

Previously, there were strange situations where a data key being removed from an ExternalSecret target template would not result in the key being removed from the target Secret.

Now, the behavior is that any key which was previously managed by the ExternalSecret will be removed if it no longer in the template or data/dataFrom definitions. (WARNING: this might mean your target secret keys are "cleaned up" when updating to this PR)

Changes for immutable secrets:

There are two ways a secret could become immutable:

1. The user tells ExternalSecrets to make the target immutable by setting spec.target.template.immutable=true.
2. The target secret already existed and had the immutable field set to true.

We now correctly handle both cases, and we will not attempt to update the target secret's data, if it's immutable.

If the controller updated the data of an immutable secret, the ExternalSecret will put into an error status, with a message explaining why. However, we will still update the metadata of the target secret if it changes (for our tracking purposes).

Changes when two ExternalSecrets try to claim creationPolicy=Owner of the same target

Previously, we would loop forever in the case that another ExternalSecret (or some other object) was already the owner of the target Secret (because we returned an error on the reconcile loop).

Now we have a nice error condition in the status of the ExternalSecret, and just wait for the target secret to be updated again to see if it's now not owned.

Changes when spec.refreshInterval=0:

We now correctly handle the case where spec.refreshInterval=0 so it matches our documentation:

• We will only create the secret once, and we will not update it, even if the spec.target.template changes in the ExternalSecret.
• If the target secret is deleted, we will recreate it with the latest data/template.
• If the target secret data is manually updated, we will revert it back to the latest data/template from the ExternalSecret.

Changes when spec.creationPolicy=Merge and missing target secret:

We have introduced a new SecretMissing reason which we use to indicate that the target secret does not exist.
When an ExternalSecret is in this state, we will check again every refreshInterval to see if the target secret has been created.

Changes when spec.creationPolicy is changed away from Owner:

Previously, we would not clean up the metadata.ownerReferences on the target secret if the ExternalSecret had its creation policy changed away from Owner, this led to the possibility of the target secret being deleted when the ExternalSecret was deleted. We also did not clean up the reconcile.external-secrets.io/created-by label.

We now remove both of these things when the creation policy is changed away from Owner.

Changes to requeue logic:

We now correctly calculate how long we should wait before the next reconcile.

This was inconsistent previously, and sometimes we would wait way longer than refreshInterval between reconciles.
Now we always use our newly added getRequeueResult() to calculate the next reconcile time.

No longer use PATCH for updates:

We now always use Update() calls rather than Patch() to update target secrets.

The Patch function does not check if the resource has changed before updating it so can be problematic if multiple controllers are updating the same resource. If we get an Update conflict, we requeue immediately.

Removals

• I have removed the changes from fix: explicitly fetch status subresource due to inconsistencies #3608 which were causing needless get requests on the stauts of ExternalSecrets.
  • I could not reproduce the race condition after my updates, but I also fixed the underlying issue, and we now abandon the reconcile if the existing secret has the spec.immutable field set to true.
• I have removed some code from feat: do not modify the secret in case of a NotModified #3746 because it does nothing, as the err will never be non-nil when its checked:
  • Search for if errors.Is(err, esv1beta1.NotModifiedErr) { in the code to find what I removed.

Future work

• We should also cache secrets that are used in SecretStore provider credentials.
  Currently, these are still retrieved every loop, and are not currently cached when --enable-secrets-caching=false.

• We should apply the changes to how we patch status (using the deferred function) in the other controllers
  As they also do unnecessary updates and use Patch() calls rather than Update() calls.

Checklist

• I have read the contribution guidelines
• All commits are signed with git commit --signoff
• My changes have reasonable test coverage
• All tests pass with make test
• I ensured my PR is ready for review with make reviewable