fix: creation policy orphan now does not react to secret updates (#4956)

Signed-off-by: Gustavo Carvalho <gustavo@externalsecrets.com>
Co-authored-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

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.

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
 	}

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))
 		})
 	}
 })