Start reconciliation when a secret has changed (#3459)

* Start reconciliation when a secret has changed

Signed-off-by: shuheiktgw <s-kitagawa@mercari.com>

* Prolong the test timeout

Signed-off-by: shuheiktgw <s-kitagawa@mercari.com>

* Use predicate.ResourceVersionChangedPredicate instead

Signed-off-by: shuheiktgw <s-kitagawa@mercari.com>

---------

Signed-off-by: shuheiktgw <s-kitagawa@mercari.com>

pkg/controllers/externalsecret/externalsecret_controller.go

@@ -36,6 +36,9 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+	"sigs.k8s.io/controller-runtime/pkg/handler"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
 	// Metrics.
@@ -73,6 +76,8 @@ const (
 	errPolicyMergePatch     = "unable to patch secret %s: %w"
 )
 
+const externalSecretSecretNameKey = ".spec.target.name"
+
 // Reconciler reconciles a ExternalSecret object.
 type Reconciler struct {
 	client.Client
@@ -628,9 +633,51 @@ func (r *Reconciler) computeDataHashAnnotation(existing, secret *v1.Secret) stri
 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 {
+		es := obj.(*esv1beta1.ExternalSecret)
+
+		if name := es.Spec.Target.Name; name != "" {
+			return []string{name}
+		}
+		return []string{es.Name}
+	}); err != nil {
+		return err
+	}
+
 	return ctrl.NewControllerManagedBy(mgr).
 		WithOptions(opts).
 		For(&esv1beta1.ExternalSecret{}).
-		Owns(&v1.Secret{}, builder.OnlyMetadata).
+		// Cannot use Owns since the controller does not set owner reference when creation policy is not Owner
+		Watches(
+			&v1.Secret{},
+			handler.EnqueueRequestsFromMapFunc(r.findObjectsForSecret),
+			builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}),
+			builder.OnlyMetadata,
+		).
 		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()},
+	)
+	if err != nil {
+		return []reconcile.Request{}
+	}
+
+	requests := make([]reconcile.Request, len(externalSecrets.Items))
+	for i := range externalSecrets.Items {
+		requests[i] = reconcile.Request{
+			NamespacedName: types.NamespacedName{
+				Name:      externalSecrets.Items[i].GetName(),
+				Namespace: externalSecrets.Items[i].GetNamespace(),
+			},
+		}
+	}
+	return requests
+}

pkg/controllers/externalsecret/externalsecret_controller_test.go

@@ -50,7 +50,7 @@ var (
 	fakeProvider   *fake.Client
 	metric         dto.Metric
 	metricDuration dto.Metric
-	timeout        = time.Second * 10
+	timeout        = time.Second * 20
 	interval       = time.Millisecond * 250
 )
 
@@ -468,6 +468,39 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 		}
 	}
 
+	mergeWithSecretUpdate := func(tc *testCase) {
+		const secretVal = "someValue"
+		tc.externalSecret.Spec.Target.CreationPolicy = esv1beta1.CreatePolicyMerge
+		tc.externalSecret.Spec.RefreshInterval = &metav1.Duration{Duration: time.Hour}
+
+		Expect(k8sClient.Create(context.Background(), &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      ExternalSecretTargetSecretName,
+				Namespace: ExternalSecretNamespace,
+			},
+			Data: map[string][]byte{
+				existingKey: []byte(existingVal),
+			},
+		}, client.FieldOwner(FakeManager))).To(Succeed())
+
+		fakeProvider.WithGetSecret([]byte(secretVal), nil)
+		tc.checkSecret = func(es *esv1beta1.ExternalSecret, secret *v1.Secret) {
+			// Overwrite the secret value to check if the change kicks reconciliation and overwrites it again
+			Expect(k8sClient.Update(context.Background(), &v1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      ExternalSecretTargetSecretName,
+					Namespace: ExternalSecretNamespace,
+				},
+				Data: map[string][]byte{
+					existingKey: []byte("differentValue"),
+				},
+			}, client.FieldOwner(FakeManager))).To(Succeed())
+
+			Expect(string(secret.Data[existingKey])).To(Equal(existingVal))
+			Expect(string(secret.Data[targetProp])).To(Equal(secretVal))
+		}
+	}
+
 	// should not update if no changes
 	mergeWithSecretNoChange := func(tc *testCase) {
 		tc.externalSecret.Spec.Target.CreationPolicy = esv1beta1.CreatePolicyMerge
@@ -2226,6 +2259,7 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 		Entry("should removed outdated labels and annotations", removeOutdatedLabelsAnnotations),
 		Entry("should set prometheus counters", checkPrometheusCounters),
 		Entry("should merge with existing secret using creationPolicy=Merge", mergeWithSecret),
+		Entry("should kick reconciliation when secret changes using creationPolicy=Merge", mergeWithSecretUpdate),
 		Entry("should error if secret doesn't exist when using creationPolicy=Merge", mergeWithSecretErr),
 		Entry("should not resolve conflicts with creationPolicy=Merge", mergeWithConflict),
 		Entry("should not update unchanged secret using creationPolicy=Merge", mergeWithSecretNoChange),

pkg/controllers/externalsecret/suite_test.go

@@ -90,7 +90,7 @@ var _ = BeforeSuite(func() {
 	Expect(err).ToNot(HaveOccurred())
 
 	err = (&Reconciler{
-		Client:                    k8sClient,
+		Client:                    k8sManager.GetClient(),
 		RestConfig:                cfg,
 		Scheme:                    k8sManager.GetScheme(),
 		Log:                       ctrl.Log.WithName("controllers").WithName("ExternalSecrets"),