Fix classic pushsecret omitted-kind template sync

Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>

pkg/controllers/pushsecret/pushsecret_controller.go

@@ -826,24 +826,7 @@ func statusRef(ref esv1.PushSecretData) string {
 // 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]esv1.GenericStore) (map[esapi.PushSecretStoreRef]esv1.GenericStore, error) {
-	for ref := range ss {
-		var store esv1.GenericStore
-		switch ref.Kind {
-		case esv1.SecretStoreKind:
-			store = &esv1.SecretStore{}
-		case esv1.ClusterSecretStoreKind:
-			store = &esv1.ClusterSecretStore{}
-			namespace = ""
-		}
-		err := r.Client.Get(ctx, types.NamespacedName{
-			Name:      ref.Name,
-			Namespace: namespace,
-		}, store)
-
-		if err != nil {
-			return ss, err
-		}
-
+	for ref, store := range ss {
 		class := store.GetSpec().Controller
 		if class != "" && class != r.ControllerClass {
 			delete(ss, ref)

pkg/controllers/pushsecret/pushsecret_controller_template.go

@@ -105,7 +105,9 @@ func setMetadata(secret *v1.Secret, ps *v1alpha1.PushSecret) error {
 		secret.Annotations = make(map[string]string)
 	}
 
-	secret.Type = ps.Spec.Template.Type
+	if ps.Spec.Template.Type != "" {
+		secret.Type = ps.Spec.Template.Type
+	}
 	esutils.MergeStringMap(secret.ObjectMeta.Labels, ps.Spec.Template.Metadata.Labels)
 	esutils.MergeStringMap(secret.ObjectMeta.Annotations, ps.Spec.Template.Metadata.Annotations)
 

pkg/controllers/pushsecret/pushsecret_controller_template_test.go

@@ -0,0 +1,41 @@
+package pushsecret
+
+import (
+	"context"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+
+	esv1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1"
+	esv1alpha1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1alpha1"
+)
+
+func TestApplyTemplatePreservesSourceSecretTypeWhenTemplateTypeUnset(t *testing.T) {
+	r := &Reconciler{}
+	secret := &corev1.Secret{
+		Type: corev1.SecretTypeOpaque,
+		Data: map[string][]byte{
+			"key": []byte("value"),
+		},
+	}
+	ps := &esv1alpha1.PushSecret{
+		Spec: esv1alpha1.PushSecretSpec{
+			Template: &esv1.ExternalSecretTemplate{
+				EngineVersion: esv1.TemplateEngineV2,
+				Data: map[string]string{
+					"key": "{{ .key | upper }}",
+				},
+			},
+		},
+	}
+
+	if err := r.applyTemplate(context.Background(), ps, secret); err != nil {
+		t.Fatalf("applyTemplate() error = %v", err)
+	}
+	if secret.Type != corev1.SecretTypeOpaque {
+		t.Fatalf("applyTemplate() changed secret type to %q", secret.Type)
+	}
+	if got := string(secret.Data["key"]); got != "VALUE" {
+		t.Fatalf("applyTemplate() templated value = %q, want %q", got, "VALUE")
+	}
+}

pkg/controllers/pushsecret/pushsecret_controller_test.go

@@ -534,6 +534,64 @@ var _ = Describe("PushSecret controller", func() {
 		}
 	}
 
+	syncSuccessfullyWithTemplateAndPropertyWithoutExplicitType := func(tc *testCase) {
+		fakeProvider.SetSecretFn = func() error {
+			return nil
+		}
+		tc.pushsecret.Spec.Data = []v1alpha1.PushSecretData{
+			{
+				Match: v1alpha1.PushSecretMatch{
+					SecretKey: defaultKey,
+					RemoteRef: v1alpha1.PushSecretRemoteRef{
+						RemoteKey: defaultPath,
+						Property:  "field",
+					},
+				},
+			},
+		}
+		tc.pushsecret.Spec.Template = &esv1.ExternalSecretTemplate{
+			EngineVersion: esv1.TemplateEngineV2,
+			Data: map[string]string{
+				defaultKey: "{{ .key | toString | upper }} was templated",
+			},
+		}
+
+		tc.assert = func(ps *v1alpha1.PushSecret, _ *v1.Secret) bool {
+			updatedPS := &v1alpha1.PushSecret{}
+			Eventually(func() bool {
+				By("checking if Provider value got updated for property-backed template data")
+				setSecretArgs := fakeProvider.GetPushSecretData()
+				providerValue, ok := setSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
+				if !ok {
+					return false
+				}
+				if !bytes.Equal(providerValue.Value, []byte("VALUE was templated")) {
+					return false
+				}
+
+				psKey := types.NamespacedName{Name: PushSecretName, Namespace: PushSecretNamespace}
+				err := k8sClient.Get(context.Background(), psKey, updatedPS)
+				if err != nil {
+					return false
+				}
+
+				expected := v1alpha1.PushSecretStatusCondition{
+					Type:    v1alpha1.PushSecretReady,
+					Status:  v1.ConditionTrue,
+					Reason:  v1alpha1.ReasonSynced,
+					Message: "PushSecret synced successfully",
+				}
+				if !checkCondition(updatedPS.Status, expected) {
+					return false
+				}
+
+				_, ok = updatedPS.Status.SyncedPushSecrets[fmt.Sprintf(storePrefixTemplate, PushSecretStore)][defaultPath+"/field"]
+				return ok
+			}, time.Second*10, time.Second).Should(BeTrue())
+			return true
+		}
+	}
+
 	// if target Secret name is not specified it should use the ExternalSecret name.
 	syncAndDeleteSuccessfully := func(tc *testCase) {
 		fakeProvider.SetSecretFn = func() error {
@@ -2488,6 +2546,7 @@ var _ = Describe("PushSecret controller", func() {
 		Entry("should update the PushSecret status correctly if UpdatePolicy=IfNotExists", updateIfNotExistsSyncStatus),
 		Entry("should fail if secret existence cannot be verified if UpdatePolicy=IfNotExists", updateIfNotExistsSyncFailed),
 		Entry("should sync with template", syncSuccessfullyWithTemplate),
+		Entry("should sync with template and property without explicit type", syncSuccessfullyWithTemplateAndPropertyWithoutExplicitType),
 		Entry("should sync with template reusing keys", syncSuccessfullyReusingKeys),
 		Entry("should sync with conversion strategy", syncSuccessfullyWithConversionStrategy),
 		Entry("should delete if DeletionPolicy=Delete", syncAndDeleteSuccessfully),

pkg/controllers/pushsecret/pushsecret_controller_v2.go

@@ -23,6 +23,7 @@ import (
 	"strings"
 
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -47,6 +48,20 @@ func (r *Reconciler) GetSecretStoresV2(ctx context.Context, ps esv1alpha1.PushSe
 			continue
 		}
 
+		if refStore.Kind == "" {
+			store, err := r.getSecretStoreFromName(ctx, refStore, ps.Namespace)
+			if err == nil {
+				stores[refStore] = store
+				continue
+			}
+			if !apierrors.IsNotFound(err) {
+				return nil, err
+			}
+			if !clientmanager.V2ProvidersEnabled() {
+				return nil, err
+			}
+		}
+
 		store, ok, err := r.resolveV2Store(ctx, refStore, ps.Namespace)
 		if err != nil {
 			return nil, err

pkg/controllers/pushsecret/pushsecret_controller_v2_test.go

@@ -702,6 +702,58 @@ func TestGetSecretStoresV2ResolvesClusterProviderWhenKindOmitted(t *testing.T) {
 	}
 }
 
+func TestGetSecretStoresV2PrefersSecretStoreWhenKindOmitted(t *testing.T) {
+	previous := clientmanager.V2ProvidersEnabled()
+	clientmanager.SetV2ProvidersEnabled(true)
+	t.Cleanup(func() {
+		clientmanager.SetV2ProvidersEnabled(previous)
+	})
+
+	scheme := newPushSecretTestScheme(t)
+	secretStore := &esv1.SecretStore{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "shared-name",
+			Namespace: "tenant-a",
+		},
+	}
+	clusterProvider := &esv1.ClusterProvider{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "shared-name",
+		},
+	}
+
+	kubeClient := fakeclient.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(secretStore, clusterProvider).
+		Build()
+
+	r := &Reconciler{Client: kubeClient, Log: logr.Discard()}
+	ps := esapi.PushSecret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "pushsecret",
+			Namespace: "tenant-a",
+		},
+		Spec: esapi.PushSecretSpec{
+			SecretStoreRefs: []esapi.PushSecretStoreRef{{
+				Name: "shared-name",
+			}},
+		},
+	}
+
+	stores, err := r.GetSecretStoresV2(context.Background(), ps)
+	if err != nil {
+		t.Fatalf("GetSecretStoresV2() error = %v", err)
+	}
+
+	store, ok := stores[esapi.PushSecretStoreRef{Name: "shared-name"}]
+	if !ok {
+		t.Fatalf("expected store to resolve, got %#v", stores)
+	}
+	if _, ok := store.(*esv1.SecretStore); !ok {
+		t.Fatalf("expected SecretStore to take precedence, got %T", store)
+	}
+}
+
 func TestGetSecretStoresV2SupportsSecretStoreLabelSelectors(t *testing.T) {
 	scheme := newPushSecretTestScheme(t)
 	selectedStore := &esv1.SecretStore{
@@ -758,6 +810,41 @@ func TestGetSecretStoresV2SupportsSecretStoreLabelSelectors(t *testing.T) {
 	}
 }
 
+func TestRemoveUnmanagedStoresSupportsOmittedKindRefs(t *testing.T) {
+	scheme := newPushSecretTestScheme(t)
+	secretStore := &esv1.SecretStore{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "shared-name",
+			Namespace: "tenant-a",
+		},
+		Spec: esv1.SecretStoreSpec{
+			Controller: "eso",
+		},
+	}
+
+	kubeClient := fakeclient.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(secretStore).
+		Build()
+
+	r := &Reconciler{
+		Client:          kubeClient,
+		ControllerClass: "eso",
+		Log:             logr.Discard(),
+	}
+
+	stores, err := removeUnmanagedStores(context.Background(), "tenant-a", r, map[esapi.PushSecretStoreRef]esv1.GenericStore{
+		{Name: "shared-name"}: secretStore,
+	})
+	if err != nil {
+		t.Fatalf("removeUnmanagedStores() error = %v", err)
+	}
+
+	if _, ok := stores[esapi.PushSecretStoreRef{Name: "shared-name"}]; !ok {
+		t.Fatalf("expected omitted-kind store ref to be retained, got %#v", stores)
+	}
+}
+
 func TestDeleteSecretFromProvidersV2DeletesRemovedStoreEvenWhenNoLongerReferenced(t *testing.T) {
 	previous := clientmanager.V2ProvidersEnabled()
 	clientmanager.SetV2ProvidersEnabled(true)