fix: scope secret list call to the namespace the push secret was created (#5133)

pkg/controllers/pushsecret/pushsecret_controller.go

@@ -429,7 +429,7 @@ func (r *Reconciler) resolveSecrets(ctx context.Context, ps *esapi.PushSecret) (
 		}
 
 		var secretList v1.SecretList
-		err = r.List(ctx, &secretList, &client.ListOptions{LabelSelector: labelSelector})
+		err = r.List(ctx, &secretList, &client.ListOptions{LabelSelector: labelSelector, Namespace: ps.Namespace})
 		if err != nil {
 			return nil, err
 		}

pkg/controllers/pushsecret/pushsecret_controller_test.go

@@ -65,7 +65,6 @@ 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 &&
@@ -232,7 +231,8 @@ var _ = Describe("PushSecret controller", func() {
 			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]
+				setSecretArgs := fakeProvider.GetPushSecretData()
+				providerValue, ok := setSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
 				if !ok {
 					return false
 				}
@@ -248,23 +248,26 @@ var _ = Describe("PushSecret controller", func() {
 			return nil
 		}
 		fakeProvider.SecretExistsFn = func(ctx context.Context, ref esv1.PushSecretRemoteRef) (bool, error) {
-			_, ok := fakeProvider.SetSecretArgs[ref.GetRemoteKey()]
+			setSecretArgs := fakeProvider.GetPushSecretData()
+			_, ok := setSecretArgs[ref.GetRemoteKey()]
 			return ok, nil
 		}
 		tc.pushsecret.Spec.UpdatePolicy = v1alpha1.PushSecretUpdatePolicyIfNotExists
-		initialValue := fakeProvider.SetSecretArgs[tc.pushsecret.Spec.Data[0].Match.RemoteRef.RemoteKey].Value
-		tc.secret.Data[defaultKey] = []byte(newVal)
 
 		tc.assert = func(ps *v1alpha1.PushSecret, secret *v1.Secret) bool {
-			Eventually(func() bool {
-				By("checking if Provider value did not get updated")
+			Consistently(func() bool {
+				By("updating the secret value")
+				tc.secret.Data[defaultKey] = []byte(newVal)
 				Expect(k8sClient.Update(context.Background(), secret, &client.UpdateOptions{})).Should(Succeed())
-				providerValue, ok := fakeProvider.SetSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
+
+				By("checking if Provider value does not get updated")
+				setSecretArgs := fakeProvider.GetPushSecretData()
+				providerValue, ok := setSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
 				if !ok {
 					return false
 				}
 				got := providerValue.Value
-				return bytes.Equal(got, initialValue)
+				return bytes.Equal(got, []byte(defaultVal))
 			}, time.Second*10, time.Second).Should(BeTrue())
 			return true
 		}
@@ -275,7 +278,8 @@ var _ = Describe("PushSecret controller", func() {
 			return nil
 		}
 		fakeProvider.SecretExistsFn = func(ctx context.Context, ref esv1.PushSecretRemoteRef) (bool, error) {
-			_, ok := fakeProvider.SetSecretArgs[ref.GetRemoteKey()]
+			setSecretArgs := fakeProvider.GetPushSecretData()
+			_, ok := setSecretArgs[ref.GetRemoteKey()]
 			return ok, nil
 		}
 		tc.pushsecret.Spec.UpdatePolicy = v1alpha1.PushSecretUpdatePolicyIfNotExists
@@ -288,26 +292,25 @@ var _ = Describe("PushSecret controller", func() {
 			},
 		})
 
-		initialValue := fakeProvider.SetSecretArgs[tc.pushsecret.Spec.Data[0].Match.RemoteRef.RemoteKey].Value
-		tc.secret.Data[defaultKey] = []byte(newVal) // change initial value in secret
-		tc.secret.Data[otherKey] = []byte(otherVal)
-
 		tc.assert = func(ps *v1alpha1.PushSecret, secret *v1.Secret) bool {
 			Eventually(func() bool {
+				tc.secret.Data[defaultKey] = []byte(newVal) // change initial value in secret
+				tc.secret.Data[otherKey] = []byte(otherVal)
+
 				By("checking if only not existing Provider value got updated")
 				Expect(k8sClient.Update(context.Background(), secret, &client.UpdateOptions{})).Should(Succeed())
-				providerValue, ok := fakeProvider.SetSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
+				setSecretArgs := fakeProvider.GetPushSecretData()
+				providerValue, ok := setSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
 				if !ok {
 					return false
 				}
 				got := providerValue.Value
-				otherProviderValue, ok := fakeProvider.SetSecretArgs[ps.Spec.Data[1].Match.RemoteRef.RemoteKey]
+				otherProviderValue, ok := setSecretArgs[ps.Spec.Data[1].Match.RemoteRef.RemoteKey]
 				if !ok {
 					return false
 				}
 				gotOther := otherProviderValue.Value
-
-				return bytes.Equal(gotOther, tc.secret.Data[otherKey]) && bytes.Equal(got, initialValue)
+				return bytes.Equal(gotOther, tc.secret.Data[otherKey]) && bytes.Equal(got, []byte(defaultVal))
 			}, time.Second*10, time.Second).Should(BeTrue())
 			return true
 		}
@@ -318,7 +321,8 @@ var _ = Describe("PushSecret controller", func() {
 			return nil
 		}
 		fakeProvider.SecretExistsFn = func(ctx context.Context, ref esv1.PushSecretRemoteRef) (bool, error) {
-			_, ok := fakeProvider.SetSecretArgs[ref.GetRemoteKey()]
+			setSecretArgs := fakeProvider.GetPushSecretData()
+			_, ok := setSecretArgs[ref.GetRemoteKey()]
 			return ok, nil
 		}
 		tc.pushsecret.Spec.UpdatePolicy = v1alpha1.PushSecretUpdatePolicyIfNotExists
@@ -371,24 +375,17 @@ var _ = Describe("PushSecret controller", func() {
 			return false, errors.New("don't know")
 		}
 		tc.pushsecret.Spec.UpdatePolicy = v1alpha1.PushSecretUpdatePolicyIfNotExists
-		initialValue := fakeProvider.SetSecretArgs[tc.pushsecret.Spec.Data[0].Match.RemoteRef.RemoteKey].Value
-		tc.secret.Data[defaultKey] = []byte(newVal)
 
 		tc.assert = func(ps *v1alpha1.PushSecret, secret *v1.Secret) bool {
 			Eventually(func() bool {
 				By("checking if sync failed if secret existence cannot be verified in Provider")
-				providerValue, ok := fakeProvider.SetSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
-				if !ok {
-					return false
-				}
-				got := providerValue.Value
 				expected := v1alpha1.PushSecretStatusCondition{
 					Type:    v1alpha1.PushSecretReady,
 					Status:  v1.ConditionFalse,
 					Reason:  v1alpha1.ReasonErrored,
 					Message: "set secret failed: could not verify if secret exists in store: don't know",
 				}
-				return checkCondition(ps.Status, expected) && bytes.Equal(got, initialValue)
+				return checkCondition(ps.Status, expected)
 			}, time.Second*10, time.Second).Should(BeTrue())
 			return true
 		}
@@ -445,7 +442,8 @@ var _ = Describe("PushSecret controller", func() {
 		tc.assert = func(ps *v1alpha1.PushSecret, secret *v1.Secret) bool {
 			Eventually(func() bool {
 				By("checking if Provider value got updated")
-				providerValue, ok := fakeProvider.SetSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
+				setSecretArgs := fakeProvider.GetPushSecretData()
+				providerValue, ok := setSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
 				if !ok {
 					return false
 				}
@@ -507,7 +505,8 @@ var _ = Describe("PushSecret controller", func() {
 		tc.assert = func(ps *v1alpha1.PushSecret, secret *v1.Secret) bool {
 			Eventually(func() bool {
 				By("checking if Provider value got updated")
-				providerValue, ok := fakeProvider.SetSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
+				setSecretArgs := fakeProvider.GetPushSecretData()
+				providerValue, ok := setSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
 				if !ok {
 					return false
 				}
@@ -824,7 +823,8 @@ var _ = Describe("PushSecret controller", func() {
 			Eventually(func() bool {
 				By("checking if Provider value got updated")
 				secretValue := secret.Data["some-array_U005b_0_U005d_.entity"]
-				providerValue, ok := fakeProvider.SetSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
+				setSecretArgs := fakeProvider.GetPushSecretData()
+				providerValue, ok := setSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
 				if !ok {
 					return false
 				}
@@ -897,7 +897,8 @@ var _ = Describe("PushSecret controller", func() {
 		}
 		tc.assert = func(ps *v1alpha1.PushSecret, secret *v1.Secret) bool {
 			secretValue := secret.Data[defaultKey]
-			providerValue := fakeProvider.SetSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey].Value
+			setSecretArgs := fakeProvider.GetPushSecretData()
+			providerValue := setSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey].Value
 			expected := v1alpha1.PushSecretStatusCondition{
 				Type:    v1alpha1.PushSecretReady,
 				Status:  v1.ConditionTrue,
@@ -929,7 +930,8 @@ var _ = Describe("PushSecret controller", func() {
 		tc.pushsecret.Spec.SecretStoreRefs[0].Kind = "ClusterSecretStore"
 		tc.assert = func(ps *v1alpha1.PushSecret, secret *v1.Secret) bool {
 			secretValue := secret.Data[defaultKey]
-			providerValue := fakeProvider.SetSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey].Value
+			setSecretArgs := fakeProvider.GetPushSecretData()
+			providerValue := setSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey].Value
 			expected := v1alpha1.PushSecretStatusCondition{
 				Type:    v1alpha1.PushSecretReady,
 				Status:  v1.ConditionTrue,
@@ -951,7 +953,8 @@ var _ = Describe("PushSecret controller", func() {
 			Name:       "test",
 		}
 		tc.assert = func(ps *v1alpha1.PushSecret, secret *v1.Secret) bool {
-			providerValue := fakeProvider.SetSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey].Value
+			setSecretArgs := fakeProvider.GetPushSecretData()
+			providerValue := setSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey].Value
 			expected := v1alpha1.PushSecretStatusCondition{
 				Type:    v1alpha1.PushSecretReady,
 				Status:  v1.ConditionTrue,
@@ -1017,7 +1020,8 @@ var _ = Describe("PushSecret controller", func() {
 		}
 		tc.assert = func(ps *v1alpha1.PushSecret, secret *v1.Secret) bool {
 			secretValue := secret.Data[defaultKey]
-			providerValue := fakeProvider.SetSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey].Value
+			setSecretArgs := fakeProvider.GetPushSecretData()
+			providerValue := setSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey].Value
 			expected := v1alpha1.PushSecretStatusCondition{
 				Type:    v1alpha1.PushSecretReady,
 				Status:  v1.ConditionTrue,
@@ -1174,6 +1178,44 @@ var _ = Describe("PushSecret controller", func() {
 		}
 	}
 
+	// Secrets in different namespace than PushSecret should not be selected.
+	secretDifferentNamespace := func(tc *testCase) {
+		fakeProvider.SetSecretFn = func() error {
+			return nil
+		}
+		// Create the Secret in a different namespace
+		tc.secret = &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      SecretName,
+				Namespace: OtherNamespace,
+				Labels: map[string]string{
+					"foo": "bar",
+				},
+			},
+			Data: map[string][]byte{
+				defaultKey: []byte(defaultVal),
+			},
+		}
+		// Use label selector to select Secrets
+		tc.pushsecret.Spec.Selector.Secret = &v1alpha1.PushSecretSecret{
+			Selector: &metav1.LabelSelector{
+				MatchLabels: map[string]string{
+					"foo": "bar",
+				},
+			},
+		}
+
+		tc.assert = func(ps *v1alpha1.PushSecret, secret *v1.Secret) bool {
+			Eventually(func() bool {
+				// We should not be able to reference a secret across namespaces,
+				// the map should be empty.
+				Expect(fakeProvider.GetPushSecretData()).To(BeEmpty())
+				return true
+			}, time.Second*10, time.Second).Should(BeTrue())
+			return true
+		}
+	}
+
 	DescribeTable("When reconciling a PushSecret",
 		func(tweaks ...testTweaks) {
 			tc := makeDefaultTestcase()
@@ -1228,6 +1270,7 @@ var _ = Describe("PushSecret controller", func() {
 		Entry("should fail if no valid ClusterSecretStore", failNoClusterStore),
 		Entry("should fail if NewClient fails", newClientFail),
 		Entry("should not sync to SecretStore in different namespace", secretStoreDifferentNamespace),
+		Entry("should not reference secret in different namespace", secretDifferentNamespace),
 	)
 })
 
@@ -1425,7 +1468,8 @@ var _ = Describe("PushSecret Controller Un/Managed Stores", func() {
 			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]
+				setSecretArgs := fakeProvider.GetPushSecretData()
+				providerValue, ok := setSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
 				if !ok {
 					return false
 				}
@@ -1481,7 +1525,8 @@ var _ = Describe("PushSecret Controller Un/Managed Stores", func() {
 			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]
+				setSecretArgs := fakeProvider.GetPushSecretData()
+				providerValue, ok := setSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
 				if !ok {
 					return false
 				}

pkg/provider/testing/fake/fake.go

@@ -16,6 +16,7 @@ package fake
 
 import (
 	"context"
+	"sync"
 
 	corev1 "k8s.io/api/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -33,7 +34,8 @@ type SetSecretCallArgs struct {
 
 // Client is a fake client for testing.
 type Client struct {
-	SetSecretArgs   map[string]SetSecretCallArgs
+	mu              *sync.RWMutex
+	pushSecretData  map[string]SetSecretCallArgs
 	NewFn           func(context.Context, esv1.GenericStore, client.Client, string) (esv1.SecretsClient, error)
 	GetSecretFn     func(context.Context, esv1.ExternalSecretDataRemoteRef) ([]byte, error)
 	GetSecretMapFn  func(context.Context, esv1.ExternalSecretDataRemoteRef) (map[string][]byte, error)
@@ -46,6 +48,7 @@ type Client struct {
 // New returns a fake provider/client.
 func New() *Client {
 	v := &Client{
+		mu: &sync.RWMutex{},
 		GetSecretFn: func(context.Context, esv1.ExternalSecretDataRemoteRef) ([]byte, error) {
 			return nil, nil
 		},
@@ -64,7 +67,7 @@ func New() *Client {
 		DeleteSecretFn: func() error {
 			return nil
 		},
-		SetSecretArgs: map[string]SetSecretCallArgs{},
+		pushSecretData: map[string]SetSecretCallArgs{},
 	}
 
 	v.NewFn = func(context.Context, esv1.GenericStore, client.Client, string) (esv1.SecretsClient, error) {
@@ -85,13 +88,27 @@ func (v *Client) GetAllSecrets(ctx context.Context, ref esv1.ExternalSecretFind)
 }
 
 func (v *Client) PushSecret(_ context.Context, secret *corev1.Secret, data esv1.PushSecretData) error {
-	v.SetSecretArgs[data.GetRemoteKey()] = SetSecretCallArgs{
+	v.mu.Lock()
+	defer v.mu.Unlock()
+	v.pushSecretData[data.GetRemoteKey()] = SetSecretCallArgs{
 		Value:     secret.Data[data.GetSecretKey()],
 		RemoteRef: data,
 	}
 	return v.SetSecretFn()
 }
 
+// GetPushSecretData safely retrieves the push secret data map for reading.
+func (v *Client) GetPushSecretData() map[string]SetSecretCallArgs {
+	v.mu.RLock()
+	defer v.mu.RUnlock()
+	// Create a copy to avoid race conditions
+	result := make(map[string]SetSecretCallArgs, len(v.pushSecretData))
+	for k, v := range v.pushSecretData {
+		result[k] = v
+	}
+	return result
+}
+
 func (v *Client) DeleteSecret(_ context.Context, _ esv1.PushSecretRemoteRef) error {
 	return v.DeleteSecretFn()
 }
@@ -180,4 +197,7 @@ func (v *Client) Reset() {
 		string) (esv1.SecretsClient, error) {
 		return v, nil
 	})
+	v.mu.Lock()
+	defer v.mu.Unlock()
+	v.pushSecretData = map[string]SetSecretCallArgs{}
 }