feat: Include remove orphans logic (#1389)

* feat: Include remove orphans logic

Signed-off-by: Daniel Campos Olivares <dacamposol@gmail.com>

* chore: Introduce deletion based on CR Status

Signed-off-by: Daniel Campos Olivares <dacamposol@gmail.com>

* chore: Simplify exit condition

Signed-off-by: Daniel Campos Olivares <dacamposol@gmail.com>

* fix: Check-diff and Unit Test

Signed-off-by: Daniel Campos Olivares <dacamposol@gmail.com>

* fix: Consume PR comments

Signed-off-by: Daniel Campos Olivares <dacamposol@gmail.com>

* chore: Change test string value for JSON

Signed-off-by: Daniel Campos Olivares <dacamposol@gmail.com>

* fix: New secret requires new name

Signed-off-by: Daniel Campos Olivares <dacamposol@gmail.com>

* bumping docs

Signed-off-by: Gustavo Carvalho <gusfcarvalho@gmail.com>

* Adding unit test instead of e2e test for orphaned secrets compatibility

Signed-off-by: Gustavo Carvalho <gusfcarvalho@gmail.com>

* Improving readability

Signed-off-by: Gustavo Carvalho <gusfcarvalho@gmail.com>

* Using Label approach

Signed-off-by: Gustavo Carvalho <gusfcarvalho@gmail.com>

* fixing lint

Signed-off-by: Gustavo Carvalho <gusfcarvalho@gmail.com>

* bumping docs

Signed-off-by: Gustavo Carvalho <gusfcarvalho@gmail.com>

* Update apis/externalsecrets/v1beta1/externalsecret_types.go

Signed-off-by: Moritz Johner <moolen@users.noreply.github.com>

---------

Signed-off-by: Daniel Campos Olivares <dacamposol@gmail.com>
Signed-off-by: Gustavo Carvalho <gusfcarvalho@gmail.com>
Signed-off-by: Moritz Johner <moolen@users.noreply.github.com>
Co-authored-by: Daniel Campos Olivares <daniel.campos.olivares@sap.com>
Co-authored-by: Gustavo Carvalho <gusfcarvalho@gmail.com>
Co-authored-by: Moritz Johner <moolen@users.noreply.github.com>

apis/externalsecrets/v1beta1/externalsecret_types.go

@@ -436,6 +436,9 @@ type ExternalSecret struct {
 const (
 	// AnnotationDataHash is used to ensure consistency.
 	AnnotationDataHash = "reconcile.external-secrets.io/data-hash"
+	// LabelOwner points to the owning ExternalSecret resource
+	//  and is used to manage the lifecycle of a Secret
+	LabelOwner = "reconcile.external-secrets.io/created-by"
 )
 
 // +kubebuilder:object:root=true

pkg/controllers/externalsecret/externalsecret_controller.go

@@ -288,10 +288,23 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		log.V(1).Info("secret creation skipped due to creationPolicy=None")
 		err = nil
 	default:
-		err = createOrUpdate(ctx, r.Client, secret, mutationFunc, externalSecret.Name)
+		created, err := createOrUpdate(ctx, r.Client, secret, mutationFunc, externalSecret.Name)
 		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)
+				log.Error(err, msg)
+				r.recorder.Event(&externalSecret, v1.EventTypeWarning, esv1beta1.ReasonUpdateFailed, err.Error())
+				conditionSynced := NewExternalSecretCondition(esv1beta1.ExternalSecretReady, v1.ConditionFalse, esv1beta1.ConditionReasonSecretSyncedError, msg)
+				SetExternalSecretCondition(&externalSecret, *conditionSynced)
+				syncCallsError.With(resourceLabels).Inc()
+				return ctrl.Result{}, err
+			}
+		}
 	}
 
 	if err != nil {
@@ -320,30 +333,63 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	}, nil
 }
 
-func createOrUpdate(ctx context.Context, c client.Client, obj client.Object, f func() error, fieldOwner string) error {
+func deleteOrphanedSecrets(ctx context.Context, cl client.Client, externalSecret *esv1beta1.ExternalSecret) error {
+	secretList := v1.SecretList{}
+	label := fmt.Sprintf("%v_%v", externalSecret.ObjectMeta.Namespace, externalSecret.ObjectMeta.Name)
+	ls := &metav1.LabelSelector{
+		MatchLabels: map[string]string{
+			esv1beta1.LabelOwner: label,
+		},
+	}
+	labelSelector, err := metav1.LabelSelectorAsSelector(ls)
+	if 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 secret.Name != externalSecret.Spec.Target.Name {
+			err = cl.Delete(ctx, &secretList.Items[key])
+			if err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
+func createOrUpdate(ctx context.Context, c client.Client, obj client.Object, f func() error, fieldOwner string) (bool, error) {
 	fqdn := fmt.Sprintf(fieldOwnerTemplate, fieldOwner)
 	key := client.ObjectKeyFromObject(obj)
 	if err := c.Get(ctx, key, obj); err != nil {
 		if !apierrors.IsNotFound(err) {
-			return err
+			return false, err
 		}
 		if err := f(); err != nil {
-			return err
+			return false, err
 		}
 		// Setting Field Owner even for CreationPolicy==Create
-		return c.Create(ctx, obj, client.FieldOwner(fqdn))
+		if err := c.Create(ctx, obj, client.FieldOwner(fqdn)); err != nil {
+			return false, err
+		}
+		return true, nil
 	}
 
 	existing := obj.DeepCopyObject()
 	if err := f(); err != nil {
-		return err
+		return false, err
 	}
 
 	if equality.Semantic.DeepEqual(existing, obj) {
-		return nil
+		return false, nil
 	}
 
-	return c.Update(ctx, obj, client.FieldOwner(fqdn))
+	if err := c.Update(ctx, obj, client.FieldOwner(fqdn)); err != nil {
+		return false, err
+	}
+	return false, nil
 }
 
 func patchSecret(ctx context.Context, c client.Client, scheme *runtime.Scheme, secret *v1.Secret, mutationFunc func() error, fieldOwner string) error {

pkg/controllers/externalsecret/externalsecret_controller_template.go

@@ -153,6 +153,7 @@ func (r *Reconciler) applyTemplate(ctx context.Context, es *esv1beta1.ExternalSe
 	if es.Spec.Target.Template == nil {
 		secret.Data = dataMap
 		secret.Annotations[esv1beta1.AnnotationDataHash] = utils.ObjectHash(secret.Data)
+		secret.Labels[esv1beta1.LabelOwner] = fmt.Sprintf("%v_%v", es.Namespace, es.Name)
 		return nil
 	}
 	// Merge Policy should merge secrets
@@ -199,6 +200,7 @@ func (r *Reconciler) applyTemplate(ctx context.Context, es *esv1beta1.ExternalSe
 		secret.Data = dataMap
 	}
 	secret.Annotations[esv1beta1.AnnotationDataHash] = utils.ObjectHash(secret.Data)
+	secret.Labels[esv1beta1.LabelOwner] = fmt.Sprintf("%v_%v", es.Namespace, es.Name)
 
 	return nil
 }

pkg/controllers/externalsecret/externalsecret_controller_test.go

@@ -311,7 +311,9 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 			Expect(string(secret.Data[targetProp])).To(Equal(secretVal))
 
 			// check labels & annotations
-			Expect(secret.ObjectMeta.Labels).To(BeEquivalentTo(es.ObjectMeta.Labels))
+			for k, v := range es.ObjectMeta.Labels {
+				Expect(secret.ObjectMeta.Labels).To(HaveKeyWithValue(k, v))
+			}
 			for k, v := range es.ObjectMeta.Annotations {
 				Expect(secret.ObjectMeta.Annotations).To(HaveKeyWithValue(k, v))
 			}
@@ -362,7 +364,9 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 			Expect(string(secret.Data[targetProp])).To(Equal(secretVal))
 
 			// check labels & annotations
-			Expect(secret.ObjectMeta.Labels).To(BeEquivalentTo(es.ObjectMeta.Labels))
+			for k, v := range es.ObjectMeta.Labels {
+				Expect(secret.ObjectMeta.Labels).To(HaveKeyWithValue(k, v))
+			}
 			for k, v := range es.ObjectMeta.Annotations {
 				Expect(secret.ObjectMeta.Annotations).To(HaveKeyWithValue(k, v))
 			}
@@ -371,7 +375,7 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 			Expect(ctest.HasFieldOwnership(
 				secret.ObjectMeta,
 				ExternalSecretFQDN,
-				fmt.Sprintf("{\"f:data\":{\"f:targetProperty\":{}},\"f:immutable\":{},\"f:metadata\":{\"f:annotations\":{\"f:%s\":{}}}}", esv1beta1.AnnotationDataHash)),
+				fmt.Sprintf("{\"f:data\":{\"f:targetProperty\":{}},\"f:immutable\":{},\"f:metadata\":{\"f:annotations\":{\"f:%s\":{}},\"f:labels\":{\"f:%s\":{}}}}", esv1beta1.AnnotationDataHash, esv1beta1.LabelOwner)),
 			).To(BeTrue())
 			Expect(ctest.HasFieldOwnership(secret.ObjectMeta, FakeManager, "{\"f:data\":{\".\":{},\"f:pre-existing-key\":{}},\"f:type\":{}}")).To(BeTrue())
 		}
@@ -469,7 +473,11 @@ 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, "{\"f:data\":{\"f:targetProperty\":{}},\"f:immutable\":{},\"f:metadata\":{\"f:annotations\":{\"f:reconcile.external-secrets.io/data-hash\":{}}}}")).To(BeTrue())
+			Expect(ctest.HasFieldOwnership(
+				secret.ObjectMeta,
+				ExternalSecretFQDN,
+				fmt.Sprintf("{\"f:data\":{\"f:targetProperty\":{}},\"f:immutable\":{},\"f:metadata\":{\"f:annotations\":{\"f:%s\":{}},\"f:labels\":{\"f:%s\":{}}}}", esv1beta1.AnnotationDataHash, esv1beta1.LabelOwner)),
+			).To(BeTrue())
 		}
 	}
 
@@ -510,6 +518,36 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 		}
 	}
 
+	deleteOrphanedSecrets := func(tc *testCase) {
+
+		tc.checkSecret = func(es *esv1beta1.ExternalSecret, secret *v1.Secret) {
+			cleanEs := es.DeepCopy()
+			oldSecret := v1.Secret{}
+			oldSecretName := types.NamespacedName{
+				Name:      "test-secret",
+				Namespace: secret.Namespace,
+			}
+			newSecret := v1.Secret{}
+			secretName := types.NamespacedName{
+				Name:      "new-foo",
+				Namespace: secret.Namespace,
+			}
+			Eventually(func() bool {
+				err := k8sClient.Get(context.Background(), oldSecretName, &oldSecret)
+				return err == nil
+			}, time.Second*10, time.Millisecond*200).Should(BeTrue())
+			es.Spec.Target.Name = "new-foo"
+			Expect(k8sClient.Patch(context.Background(), es, client.MergeFrom(cleanEs))).To(Succeed())
+			Eventually(func() bool {
+				err := k8sClient.Get(context.Background(), secretName, &newSecret)
+				return err == nil
+			}, time.Second*10, time.Millisecond*200).Should(BeTrue())
+			Eventually(func() bool {
+				err := k8sClient.Get(context.Background(), oldSecretName, &oldSecret)
+				return apierrors.IsNotFound(err)
+			}, time.Second*10, time.Millisecond*200).Should(BeTrue())
+		}
+	}
 	ignoreMismatchControllerForGeneratorRef := func(tc *testCase) {
 		const secretKey = "somekey"
 		const secretVal = "someValue"
@@ -653,7 +691,10 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 			Expect(string(secret.Data[tplStaticKey])).To(Equal(tplStaticVal))
 
 			// labels/annotations should be taken from the template
-			Expect(secret.ObjectMeta.Labels).To(BeEquivalentTo(es.Spec.Target.Template.Metadata.Labels))
+			for k, v := range es.Spec.Target.Template.Metadata.Labels {
+				Expect(secret.ObjectMeta.Labels).To(HaveKeyWithValue(k, v))
+
+			}
 			for k, v := range es.Spec.Target.Template.Metadata.Annotations {
 				Expect(secret.ObjectMeta.Annotations).To(HaveKeyWithValue(k, v))
 			}
@@ -911,7 +952,10 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 			Expect(string(secret.Data[tplStaticKey])).To(Equal(tplStaticVal))
 
 			// labels/annotations should be taken from the template
-			Expect(secret.ObjectMeta.Labels).To(BeEquivalentTo(es.Spec.Target.Template.Metadata.Labels))
+			for k, v := range es.Spec.Target.Template.Metadata.Labels {
+				Expect(secret.ObjectMeta.Labels).To(HaveKeyWithValue(k, v))
+
+			}
 
 			// a secret will always have some extra annotations (i.e. hashmap check), so we only check for specific
 			// source annotations
@@ -943,7 +987,10 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 			}, time.Second*10, time.Millisecond*200).Should(BeTrue())
 
 			// also check labels/annotations have been updated
-			Expect(secret.ObjectMeta.Labels).To(BeEquivalentTo(es.Spec.Target.Template.Metadata.Labels))
+			for k, v := range es.Spec.Target.Template.Metadata.Labels {
+				Expect(secret.ObjectMeta.Labels).To(HaveKeyWithValue(k, v))
+
+			}
 			for k, v := range es.Spec.Target.Template.Metadata.Annotations {
 				Expect(secret.ObjectMeta.Annotations).To(HaveKeyWithValue(k, v))
 			}
@@ -965,7 +1012,10 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 			Expect(string(secret.Data[targetProp])).To(Equal(secretVal))
 
 			// labels/annotations should be taken from the template
-			Expect(secret.ObjectMeta.Labels).To(BeEquivalentTo(es.Spec.Target.Template.Metadata.Labels))
+			for k, v := range es.Spec.Target.Template.Metadata.Labels {
+				Expect(secret.ObjectMeta.Labels).To(HaveKeyWithValue(k, v))
+			}
+
 			for k, v := range es.Spec.Target.Template.Metadata.Annotations {
 				Expect(secret.ObjectMeta.Annotations).To(HaveKeyWithValue(k, v))
 			}
@@ -1853,7 +1903,6 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 			Expect(string(secret.Data[targetProp])).To(Equal(secretVal))
 		}
 	}
-
 	// Secret is created when ClusterSecretStore has a multiple string conditions, one matching
 	secretCreatedWhenNamespaceMatchesMultipleStringConditions := func(tc *testCase) {
 		tc.secretStore.GetSpec().Conditions = []esv1beta1.ClusterSecretStoreCondition{
@@ -2010,6 +2059,7 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 		},
 		Entry("should recreate deleted secret", checkDeletion),
 		Entry("should create proper hash annotation for the external secret", checkSecretDataHashAnnotation),
+		Entry("es deletes orphaned secrets", deleteOrphanedSecrets),
 		Entry("should refresh when the hash annotation doesn't correspond to secret data", checkSecretDataHashAnnotationChange),
 		Entry("should use external secret name if target secret name isn't defined", syncWithoutTargetName),
 		Entry("should expose the secret as a provisioned service binding secret", syncBindingSecret),