Add logic to skip multiple stores. Add tests for multiple un/managed stores (#3123)

Signed-off-by: Bude8 <henryblee8@gmail.com>

pkg/controllers/pushsecret/pushsecret_controller.go

@@ -49,6 +49,7 @@ const (
 	errSetSecretFailed       = "could not write remote ref %v to target secretstore %v: %v"
 	errFailedSetSecret       = "set secret failed: %v"
 	errConvert               = "could not apply conversion strategy to keys: %v"
+	errUnmanagedStores       = "PushSecret %q has no managed stores to push to"
 	pushSecretFinalizer      = "pushsecret.externalsecrets.io/finalizer"
 )
 
@@ -157,6 +158,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		return ctrl.Result{}, err
 	}
 
+	secretStores, err = removeUnmanagedStores(ctx, req.Namespace, r, secretStores)
+	if err != nil {
+		r.markAsFailed(err.Error(), &ps, nil)
+		return ctrl.Result{}, err
+	}
+	// if no stores are managed by this controller
+	if len(secretStores) == 0 {
+		return ctrl.Result{}, nil
+	}
+
 	syncedSecrets, err := r.PushSecretToProviders(ctx, secretStores, ps, secret, mgr)
 	if err != nil {
 		if errors.Is(err, locks.ErrConflict) {
@@ -465,3 +476,32 @@ func statusRef(ref v1beta1.PushSecretData) string {
 	}
 	return ref.GetRemoteKey()
 }
+
+// removeUnmanagedStores iterates over all SecretStore references and evaluates the controllerClass property.
+// Returns a map containing only managed stores.
+func removeUnmanagedStores(ctx context.Context, namespace string, r *Reconciler, ss map[esapi.PushSecretStoreRef]v1beta1.GenericStore) (map[esapi.PushSecretStoreRef]v1beta1.GenericStore, error) {
+	for ref := range ss {
+		var store v1beta1.GenericStore
+		switch ref.Kind {
+		case v1beta1.SecretStoreKind:
+			store = &v1beta1.SecretStore{}
+		case v1beta1.ClusterSecretStoreKind:
+			store = &v1beta1.ClusterSecretStore{}
+			namespace = ""
+		}
+		err := r.Client.Get(ctx, types.NamespacedName{
+			Name:      ref.Name,
+			Namespace: namespace,
+		}, store)
+
+		if err != nil {
+			return ss, err
+		}
+
+		class := store.GetSpec().Controller
+		if class != "" && class != r.ControllerClass {
+			delete(ss, ref)
+		}
+	}
+	return ss, nil
+}

pkg/controllers/pushsecret/pushsecret_controller_test.go

@@ -44,10 +44,14 @@ var (
 )
 
 type testCase struct {
-	store      v1beta1.GenericStore
-	pushsecret *v1alpha1.PushSecret
-	secret     *v1.Secret
-	assert     func(pushsecret *v1alpha1.PushSecret, secret *v1.Secret) bool
+	store           v1beta1.GenericStore
+	managedStore1   v1beta1.GenericStore
+	managedStore2   v1beta1.GenericStore
+	unmanagedStore1 v1beta1.GenericStore
+	unmanagedStore2 v1beta1.GenericStore
+	pushsecret      *v1alpha1.PushSecret
+	secret          *v1.Secret
+	assert          func(pushsecret *v1alpha1.PushSecret, secret *v1.Secret) bool
 }
 
 func init() {
@@ -59,6 +63,7 @@ func init() {
 }
 
 func checkCondition(status v1alpha1.PushSecretStatus, cond v1alpha1.PushSecretStatusCondition) bool {
+	fmt.Printf("status: %+v\ncond: %+v\n", status.Conditions, cond)
 	for _, condition := range status.Conditions {
 		if condition.Message == cond.Message &&
 			condition.Reason == cond.Reason &&
@@ -72,9 +77,9 @@ func checkCondition(status v1alpha1.PushSecretStatus, cond v1alpha1.PushSecretSt
 
 type testTweaks func(*testCase)
 
-var _ = Describe("ExternalSecret controller", func() {
+var _ = Describe("PushSecret controller", func() {
 	const (
-		PushSecretName  = "test-es"
+		PushSecretName  = "test-ps"
 		PushSecretStore = "test-store"
 		SecretName      = "test-secret"
 	)
@@ -718,6 +723,7 @@ var _ = Describe("ExternalSecret controller", func() {
 							},
 						},
 						Kind: "SecretStore",
+						Name: PushSecretStore,
 					},
 				},
 				Selector: v1alpha1.PushSecretSelector{
@@ -819,6 +825,7 @@ var _ = Describe("ExternalSecret controller", func() {
 							},
 						},
 						Kind: "ClusterSecretStore",
+						Name: PushSecretStore,
 					},
 				},
 				Selector: v1alpha1.PushSecretSelector{
@@ -930,7 +937,8 @@ var _ = Describe("ExternalSecret controller", func() {
 			}
 			return checkCondition(ps.Status, expected)
 		}
-	} // if target Secret name is not specified it should use the ExternalSecret name.
+	}
+	// if target Secret name is not specified it should use the ExternalSecret name.
 	setSecretFail := func(tc *testCase) {
 		fakeProvider.SetSecretFn = func() error {
 			return fmt.Errorf("boom")
@@ -960,6 +968,7 @@ var _ = Describe("ExternalSecret controller", func() {
 			return checkCondition(ps.Status, expected)
 		}
 	}
+
 	DescribeTable("When reconciling a PushSecret",
 		func(tweaks ...testTweaks) {
 			tc := makeDefaultTestcase()
@@ -977,7 +986,7 @@ var _ = Describe("ExternalSecret controller", func() {
 			if tc.pushsecret != nil {
 				Expect(k8sClient.Create(ctx, tc.pushsecret)).Should(Succeed())
 			}
-			time.Sleep(2 * time.Second)
+			time.Sleep(2 * time.Second) // prevents race conditions during tests causing failures
 			psKey := types.NamespacedName{Name: PushSecretName, Namespace: PushSecretNamespace}
 			createdPS := &v1alpha1.PushSecret{}
 			By("checking the pushSecret condition")
@@ -1012,3 +1021,315 @@ var _ = Describe("ExternalSecret controller", func() {
 		Entry("should fail if NewClient fails", newClientFail),
 	)
 })
+
+var _ = Describe("PushSecret Controller Un/Managed Stores", func() {
+	const (
+		PushSecretName            = "test-ps"
+		ManagedPushSecretStore1   = "test-managed-store-1"
+		ManagedPushSecretStore2   = "test-managed-store-2"
+		UnmanagedPushSecretStore1 = "test-unmanaged-store-1"
+		UnmanagedPushSecretStore2 = "test-unmanaged-store-2"
+		SecretName                = "test-secret"
+	)
+
+	var PushSecretNamespace string
+	PushSecretStores := []string{ManagedPushSecretStore1, ManagedPushSecretStore2, UnmanagedPushSecretStore1, UnmanagedPushSecretStore2}
+
+	// if we are in debug and need to increase the timeout for testing, we can do so by using an env var
+	if customTimeout := os.Getenv("TEST_CUSTOM_TIMEOUT_SEC"); customTimeout != "" {
+		if t, err := strconv.Atoi(customTimeout); err == nil {
+			timeout = time.Second * time.Duration(t)
+		}
+	}
+
+	BeforeEach(func() {
+		var err error
+		PushSecretNamespace, err = ctest.CreateNamespace("test-ns", k8sClient)
+		Expect(err).ToNot(HaveOccurred())
+		fakeProvider.Reset()
+	})
+
+	AfterEach(func() {
+		k8sClient.Delete(context.Background(), &v1alpha1.PushSecret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      PushSecretName,
+				Namespace: PushSecretNamespace,
+			},
+		})
+		// give a time for reconciler to remove finalizers before removing SecretStores
+		// TODO: Secret Stores should have finalizers bound to PushSecrets if DeletionPolicy == Delete
+		time.Sleep(2 * time.Second)
+		for _, psstore := range PushSecretStores {
+			k8sClient.Delete(context.Background(), &v1beta1.SecretStore{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      psstore,
+					Namespace: PushSecretNamespace,
+				},
+			})
+			k8sClient.Delete(context.Background(), &v1beta1.ClusterSecretStore{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: psstore,
+				},
+			})
+		}
+		k8sClient.Delete(context.Background(), &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      SecretName,
+				Namespace: PushSecretNamespace,
+			},
+		})
+		Expect(k8sClient.Delete(context.Background(), &v1.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: PushSecretNamespace,
+			},
+		})).To(Succeed())
+	})
+
+	const (
+		defaultKey          = "key"
+		defaultVal          = "value"
+		defaultPath         = "path/to/key"
+		otherKey            = "other-key"
+		otherVal            = "other-value"
+		otherPath           = "path/to/other-key"
+		newKey              = "new-key"
+		newVal              = "new-value"
+		storePrefixTemplate = "SecretStore/%v"
+	)
+
+	makeDefaultTestcase := func() *testCase {
+		return &testCase{
+			pushsecret: &v1alpha1.PushSecret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      PushSecretName,
+					Namespace: PushSecretNamespace,
+				},
+				Spec: v1alpha1.PushSecretSpec{
+					SecretStoreRefs: []v1alpha1.PushSecretStoreRef{
+						{
+							Name: ManagedPushSecretStore1,
+							Kind: "SecretStore",
+						},
+					},
+					Selector: v1alpha1.PushSecretSelector{
+						Secret: v1alpha1.PushSecretSecret{
+							Name: SecretName,
+						},
+					},
+					Data: []v1alpha1.PushSecretData{
+						{
+							Match: v1alpha1.PushSecretMatch{
+								SecretKey: defaultKey,
+								RemoteRef: v1alpha1.PushSecretRemoteRef{
+									RemoteKey: defaultPath,
+								},
+							},
+						},
+					},
+				},
+			},
+			secret: &v1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      SecretName,
+					Namespace: PushSecretNamespace,
+				},
+				Data: map[string][]byte{
+					defaultKey: []byte(defaultVal),
+				},
+			},
+			managedStore1: &v1beta1.SecretStore{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      ManagedPushSecretStore1,
+					Namespace: PushSecretNamespace,
+				},
+				TypeMeta: metav1.TypeMeta{
+					Kind: "SecretStore",
+				},
+				Spec: v1beta1.SecretStoreSpec{
+					Provider: &v1beta1.SecretStoreProvider{
+						Fake: &v1beta1.FakeProvider{
+							Data: []v1beta1.FakeProviderData{},
+						},
+					},
+				},
+			},
+			managedStore2: &v1beta1.SecretStore{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      ManagedPushSecretStore2,
+					Namespace: PushSecretNamespace,
+				},
+				TypeMeta: metav1.TypeMeta{
+					Kind: "SecretStore",
+				},
+				Spec: v1beta1.SecretStoreSpec{
+					Provider: &v1beta1.SecretStoreProvider{
+						Fake: &v1beta1.FakeProvider{
+							Data: []v1beta1.FakeProviderData{},
+						},
+					},
+				},
+			},
+			unmanagedStore1: &v1beta1.SecretStore{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      UnmanagedPushSecretStore1,
+					Namespace: PushSecretNamespace,
+				},
+				TypeMeta: metav1.TypeMeta{
+					Kind: "SecretStore",
+				},
+				Spec: v1beta1.SecretStoreSpec{
+					Provider: &v1beta1.SecretStoreProvider{
+						Fake: &v1beta1.FakeProvider{
+							Data: []v1beta1.FakeProviderData{},
+						},
+					},
+					Controller: "not-managed",
+				},
+			},
+			unmanagedStore2: &v1beta1.SecretStore{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      UnmanagedPushSecretStore2,
+					Namespace: PushSecretNamespace,
+				},
+				TypeMeta: metav1.TypeMeta{
+					Kind: "SecretStore",
+				},
+				Spec: v1beta1.SecretStoreSpec{
+					Provider: &v1beta1.SecretStoreProvider{
+						Fake: &v1beta1.FakeProvider{
+							Data: []v1beta1.FakeProviderData{},
+						},
+					},
+					Controller: "not-managed",
+				},
+			},
+		}
+	}
+
+	multipleManagedStoresSyncsSuccessfully := func(tc *testCase) {
+		fakeProvider.SetSecretFn = func() error {
+			return nil
+		}
+
+		tc.pushsecret.Spec.SecretStoreRefs = append(tc.pushsecret.Spec.SecretStoreRefs,
+			v1alpha1.PushSecretStoreRef{
+				Name: ManagedPushSecretStore2,
+				Kind: "SecretStore",
+			},
+		)
+
+		tc.assert = func(ps *v1alpha1.PushSecret, secret *v1.Secret) bool {
+			Eventually(func() bool {
+				By("checking if Provider value got updated")
+				secretValue := secret.Data[defaultKey]
+				providerValue, ok := fakeProvider.SetSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
+				if !ok {
+					return false
+				}
+				got := providerValue.Value
+				return bytes.Equal(got, secretValue)
+			}, time.Second*10, time.Second).Should(BeTrue())
+			return true
+		}
+	}
+
+	skipUnmanagedStores := func(tc *testCase) {
+		tc.pushsecret.Spec.SecretStoreRefs = []v1alpha1.PushSecretStoreRef{
+			{
+				Name: UnmanagedPushSecretStore1,
+				Kind: "SecretStore",
+			},
+			{
+				Name: UnmanagedPushSecretStore2,
+				Kind: "SecretStore",
+			},
+		}
+
+		tc.assert = func(ps *v1alpha1.PushSecret, secret *v1.Secret) bool {
+			return len(ps.Status.Conditions) == 0
+		}
+	}
+
+	warnUnmanagedStoresAndSyncManagedStores := func(tc *testCase) {
+		fakeProvider.SetSecretFn = func() error {
+			return nil
+		}
+
+		tc.pushsecret.Spec.SecretStoreRefs = []v1alpha1.PushSecretStoreRef{
+			{
+				Name: ManagedPushSecretStore1,
+				Kind: "SecretStore",
+			},
+			{
+				Name: ManagedPushSecretStore2,
+				Kind: "SecretStore",
+			},
+			{
+				Name: UnmanagedPushSecretStore1,
+				Kind: "SecretStore",
+			},
+			{
+				Name: UnmanagedPushSecretStore2,
+				Kind: "SecretStore",
+			},
+		}
+
+		tc.assert = func(ps *v1alpha1.PushSecret, secret *v1.Secret) bool {
+			Eventually(func() bool {
+				By("checking if Provider value got updated")
+				secretValue := secret.Data[defaultKey]
+				providerValue, ok := fakeProvider.SetSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
+				if !ok {
+					return false
+				}
+				got := providerValue.Value
+				return bytes.Equal(got, secretValue)
+			}, time.Second*10, time.Second).Should(BeTrue())
+			return true
+		}
+	}
+
+	DescribeTable("When reconciling a PushSecret with multiple secret stores",
+		func(tweaks ...testTweaks) {
+			tc := makeDefaultTestcase()
+			for _, tweak := range tweaks {
+				tweak(tc)
+			}
+			ctx := context.Background()
+			By("creating secret stores, a secret and a pushsecret")
+			if tc.managedStore1 != nil {
+				Expect(k8sClient.Create(ctx, tc.managedStore1)).To(Succeed())
+			}
+			if tc.managedStore2 != nil {
+				Expect(k8sClient.Create(ctx, tc.managedStore2)).To(Succeed())
+			}
+			if tc.unmanagedStore1 != nil {
+				Expect(k8sClient.Create(ctx, tc.unmanagedStore1)).To(Succeed())
+			}
+			if tc.unmanagedStore2 != nil {
+				Expect(k8sClient.Create(ctx, tc.unmanagedStore2)).To(Succeed())
+			}
+			if tc.secret != nil {
+				Expect(k8sClient.Create(ctx, tc.secret)).To(Succeed())
+			}
+			if tc.pushsecret != nil {
+				Expect(k8sClient.Create(ctx, tc.pushsecret)).Should(Succeed())
+			}
+			time.Sleep(2 * time.Second) // prevents race conditions during tests causing failures
+			psKey := types.NamespacedName{Name: PushSecretName, Namespace: PushSecretNamespace}
+			createdPS := &v1alpha1.PushSecret{}
+			By("checking the pushSecret condition")
+			Eventually(func() bool {
+				err := k8sClient.Get(ctx, psKey, createdPS)
+				if err != nil {
+					return false
+				}
+				return tc.assert(createdPS, tc.secret)
+			}, timeout, interval).Should(BeTrue())
+			// this must be optional so we can test faulty es configuration
+		},
+		Entry("should sync successfully if there are multiple managed stores", multipleManagedStoresSyncsSuccessfully),
+		Entry("should skip unmanaged stores", skipUnmanagedStores),
+		Entry("should skip unmanaged stores and sync managed stores", warnUnmanagedStoresAndSyncManagedStores),
+	)
+})