Repository: external-secrets/external-secrets (go)

Issue:
Unable to change secret value when `refreshInterval: 0` and `creationPolicy: Orphan`
**Describe the bug**

I would like to update a secrets (created from an externalsecret) data values. From what I gathered this should be possible by setting `refreshInterval: 0` and `creationPolicy: Orphan`

**To Reproduce**

```yaml
apiVersion: external-secrets.io/v1beta1
kind: ExternalSecret
metadata:
  name: test
spec:
  refreshInterval: "0"
  secretStoreRef:
    kind: ClusterSecretStore
    name: onepassword-connect
  target:
    name: test
    creationPolicy: Orphan
    template:
      engineVersion: v2
      type: kubernetes.io/tls
      metadata:
        annotations:
          testAnnotation: annotation 
        labels:
          testLabel: label
  dataFrom:
    - extract:
        key: test
        decodingStrategy: Auto
```

Now try to edit a field in the generated secret, it will get overridden on save.

**Expected behavior**

I expect to edit the data values in the generated secret and have them persistent.

**Additional context**

I am trying to automate pushing my LE certificate to my eso provider (onepassword) and then when the cluster is re-provisioned have the certificate be "imported" from onepassword but this doesn't work because when cert-manager renews the cert it writes to the `*-tls` secret but ESO just reverts it.

Here's my implementation using Flux:

https://github.com/onedr0p/home-ops/tree/e3b1bda79ef408d497d649f166a92fc04ee0bb4c/kubernetes/main/apps/cert-manager/certificates

- `export` folder creates the `certificate` and `pushsecret` resources
- `import` folder creates the `externalsecret` that tries to pull down the `cert` into a secret that cert-manager will read.

**Related**

https://github.com/external-secrets/external-secrets/issues/2245#issuecomment-1519000742

https://github.com/external-secrets/external-secrets/issues/4029

https://github.com/external-secrets/external-secrets/discussions/3148

Pull request description:
fix: creation policy orphan now does not react to secret updates
Fixes #4099

Patch:
diff --git a/docs/guides/ownership-deletion-policy.md b/docs/guides/ownership-deletion-policy.md
index 1024c9ea2..6a3bb29a5 100644
--- a/docs/guides/ownership-deletion-policy.md
+++ b/docs/guides/ownership-deletion-policy.md
@@ -1,5 +1,5 @@
 # Lifecycle
-The External Secrets Operator manages the lifecycle of secrets in Kubernetes. With `creationPolicy` and `deletionPolicy` you get fine-grained control of its lifecycle.
+The External Secrets Operator manages the lifecycle of secrets in Kubernetes. With `refreshPolicy`,   `creationPolicy` and `deletionPolicy` you get fine-grained control of its lifecycle.
 
 !!! note "Creation/Deletion Policy Combinations"
     Some combinations of creationPolicy/deletionPolicy are not allowed as they would delete existing secrets:
@@ -7,6 +7,18 @@ The External Secrets Operator manages the lifecycle of secrets in Kubernetes. Wi
     <br/>- `deletionPolicy=Delete` & `creationPolicy=None`
     <br/>- `deletionPolicy=Merge` & `creationPolicy=None`
 
+## Refresh Policy
+The field `spec.refreshPolicy` defines how the operator refreshes the a secret.
+
+### Periodic (default) 
+Refreshes the secret at a fixed interval via `spec.refreshInterval`. Due to backwards compatibility, setting a refresh interval of 0 will result in the same behavior as `CreatedOnce`.
+
+### OnChange
+Refreshes the secret only when the ExternalSecret is updated.  
+
+### CreatedOnce
+Refreshes the secret only once, when the ExternalSecret is created.
+
 ## Creation Policy
 The field `spec.target.creationPolicy` defines how the operator creates the a secret.
 
@@ -17,7 +29,16 @@ The External Secret Operator creates secret and sets the `ownerReference` field
     If the secret exists and the ownerReference field is not found, the controller treats this secret as orphaned. It will take ownership of this secret by adding an `ownerReference` field and updating it.
 
 ### Orphan
-The operator creates the secret but does not set the `ownerReference` on the Secret. That means the Secret will not be subject to garbage collection. If a secret with the same name already exists it will be updated.
+Whenever triggered via `RefreshPolicy` conditions, the operator creates/updates 
+the target Secret according to the provider available information. 
+However, the operator will not watch on Secret Changes (delete/updates), nor trigger 
+[garbage collection](https://kubernetes.io/docs/concepts/architecture/garbage-collection/) when the `ExternalSecret` object is deleted.
+
+!!! warning "Unwanted reverts of manual changes"
+    If you set `spec.refreshPolicy` to `Periodic` or `OnChange` and `spec.target.creationPolicy` to `Orphan`,
+    any changes manually done to the Secret will eventually be replaced on the next sync interval
+    or on the next update to `ExternalSecret` object. That manual change is then lost forever.
+    Use `creationPolicy=Orphan` with caution.
 
 ### Merge
 The operator does not create a secret. Instead, it expects the secret to already exist. Values from the secret provider will be merged into the existing secret. Note: the controller takes ownership of a field even if it is owned by a different entity. Multiple ExternalSecrets can use `creationPolicy=Merge` with a single secret as long as the fields don't collide - otherwise you end up in an oscillating state.
diff --git a/pkg/controllers/externalsecret/externalsecret_controller.go b/pkg/controllers/externalsecret/externalsecret_controller.go
index c2789a111..d85d64aff 100644
--- a/pkg/controllers/externalsecret/externalsecret_controller.go
+++ b/pkg/controllers/externalsecret/externalsecret_controller.go
@@ -281,7 +281,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ct
 	//     - it exists
 	//     - it has the correct "managed" label
 	//     - it has the correct "data-hash" annotation
-	if !shouldRefresh(externalSecret) && isSecretValid(existingSecret) {
+	if !shouldRefresh(externalSecret) && isSecretValid(existingSecret, externalSecret) {
 		log.V(1).Info("skipping refresh")
 		return r.getRequeueResult(externalSecret), nil
 	}
@@ -906,8 +906,12 @@ func shouldRefreshPeriodic(es *esv1.ExternalSecret) bool {
 }
 
 // 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, we need to refresh
+func isSecretValid(existingSecret *v1.Secret, es *esv1.ExternalSecret) bool {
+	// Secret is always valid with `CreationPolicy=Orphan`
+	if es.Spec.Target.CreationPolicy == esv1.CreatePolicyOrphan {
+		return true
+	}
+
 	if existingSecret.UID == "" {
 		return false
 	}


Test patch:
diff --git a/pkg/controllers/externalsecret/externalsecret_controller_test.go b/pkg/controllers/externalsecret/externalsecret_controller_test.go
index 1361bc26d..d65b2f145 100644
--- a/pkg/controllers/externalsecret/externalsecret_controller_test.go
+++ b/pkg/controllers/externalsecret/externalsecret_controller_test.go
@@ -91,6 +91,16 @@ type testCase struct {
 	checkSecret func(*esv1.ExternalSecret, *v1.Secret)
 }
 
+func makeExternalSecret(policy esv1.ExternalSecretCreationPolicy) *esv1.ExternalSecret {
+	return &esv1.ExternalSecret{
+		Spec: esv1.ExternalSecretSpec{
+			Target: esv1.ExternalSecretTarget{
+				CreationPolicy: policy,
+			},
+		},
+	}
+}
+
 type testTweaks func(*testCase)
 
 var _ = Describe("Kind=secret existence logic", func() {
@@ -101,12 +111,14 @@ var _ = Describe("Kind=secret existence logic", func() {
 	type testCase struct {
 		Name           string
 		Input          *v1.Secret
+		ExternalSecret *esv1.ExternalSecret
 		ExpectedOutput bool
 	}
 	tests := []testCase{
 		{
 			Name:           "Should not be valid in case of missing uid",
 			Input:          &v1.Secret{},
+			ExternalSecret: &esv1.ExternalSecret{},
 			ExpectedOutput: false,
 		},
 		{
@@ -120,6 +132,7 @@ var _ = Describe("Kind=secret existence logic", func() {
 					Annotations: map[string]string{},
 				},
 			},
+			ExternalSecret: makeExternalSecret(esv1.CreatePolicyOwner),
 			ExpectedOutput: false,
 		},
 		{
@@ -135,6 +148,7 @@ var _ = Describe("Kind=secret existence logic", func() {
 					},
 				},
 			},
+			ExternalSecret: makeExternalSecret(esv1.CreatePolicyOwner),
 			ExpectedOutput: false,
 		},
 		{
@@ -151,13 +165,36 @@ var _ = Describe("Kind=secret existence logic", func() {
 				},
 				Data: validData,
 			},
+			ExternalSecret: makeExternalSecret(esv1.CreatePolicyOwner),
+			ExpectedOutput: true,
+		},
+		{
+			Name: "Ignore Annotations if creation policy is Orphan",
+			Input: &v1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					UID: "xxx",
+					Labels: map[string]string{
+						esv1.LabelManaged: esv1.LabelManagedValue,
+					},
+					Annotations: map[string]string{
+						esv1.AnnotationDataHash: "xxxxxx",
+					},
+				},
+			},
+			ExternalSecret: makeExternalSecret(esv1.CreatePolicyOrphan),
+			ExpectedOutput: true,
+		},
+		{
+			Name:           "Ignore missing UID Secret if creation policy is Orphan",
+			Input:          &v1.Secret{},
+			ExternalSecret: makeExternalSecret(esv1.CreatePolicyOrphan),
 			ExpectedOutput: true,
 		},
 	}
 
 	for _, tt := range tests {
 		It(tt.Name, func() {
-			Expect(isSecretValid(tt.Input)).To(BeEquivalentTo(tt.ExpectedOutput))
+			Expect(isSecretValid(tt.Input, tt.ExternalSecret)).To(BeEquivalentTo(tt.ExpectedOutput))
 		})
 	}
 })