Merge branch 'beach-team' of github.com:external-secrets/external-secrets into beach-team

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

apis/externalsecrets/v1alpha1/pushsecret_types.go

@@ -20,9 +20,8 @@ import (
 )
 
 const (
-	ReasonSynced    = "Synced"
-	ReasonNotSynced = "NotSynced"
-	ReasonErrored   = "Errored"
+	ReasonSynced  = "Synced"
+	ReasonErrored = "Errored"
 )
 
 type PushSecretStoreRef struct {
@@ -65,7 +64,7 @@ type PushSecretMatch struct {
 }
 
 type PushSecretData struct {
-	Match []PushSecretMatch `json:"match"`
+	Match PushSecretMatch `json:"match"`
 }
 
 // PushSecretConditionType indicates the condition of the PushSecret.

apis/externalsecrets/v1alpha1/zz_generated.deepcopy.go

@@ -994,13 +994,7 @@ func (in *PushSecret) DeepCopyObject() runtime.Object {
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *PushSecretData) DeepCopyInto(out *PushSecretData) {
 	*out = *in
-	if in.Match != nil {
-		in, out := &in.Match, &out.Match
-		*out = make([]PushSecretMatch, len(*in))
-		for i := range *in {
-			(*in)[i].DeepCopyInto(&(*out)[i])
-		}
-	}
+	in.Match.DeepCopyInto(&out.Match)
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PushSecretData.

config/crds/bases/external-secrets.io_clustersecretstores.yaml

@@ -1392,6 +1392,7 @@ spec:
       type: string
     - jsonPath: .status.capabilities
       name: Capabilities
+      type: string
     - jsonPath: .status.conditions[?(@.type=="Ready")].status
       name: Ready
       type: string

config/crds/bases/external-secrets.io_pushsecrets.yaml

@@ -2,7 +2,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.9.0
+    controller-gen.kubebuilder.io/version: v0.9.2
   creationTimestamp: null
   name: pushsecrets.external-secrets.io
 spec:
@@ -46,24 +46,22 @@ spec:
                 items:
                   properties:
                     match:
-                      items:
-                        properties:
-                          remoteRefs:
-                            items:
-                              properties:
-                                remoteKey:
-                                  type: string
-                              required:
-                              - remoteKey
-                              type: object
-                            type: array
-                          secretKey:
-                            type: string
-                        required:
-                        - remoteRefs
-                        - secretKey
-                        type: object
-                      type: array
+                      properties:
+                        remoteRefs:
+                          items:
+                            properties:
+                              remoteKey:
+                                type: string
+                            required:
+                            - remoteKey
+                            type: object
+                          type: array
+                        secretKey:
+                          type: string
+                      required:
+                      - remoteRefs
+                      - secretKey
+                      type: object
                   required:
                   - match
                   type: object

config/crds/bases/external-secrets.io_secretstores.yaml

@@ -1392,6 +1392,7 @@ spec:
       type: string
     - jsonPath: .status.capabilities
       name: Capabilities
+      type: string
     - jsonPath: .status.conditions[?(@.type=="Ready")].status
       name: Ready
       type: string

deploy/crds/bundle.yaml

@@ -1407,6 +1407,9 @@ spec:
         - jsonPath: .status.conditions[?(@.type=="Ready")].reason
           name: Status
           type: string
+        - jsonPath: .status.capabilities
+          name: Capabilities
+          type: string
         - jsonPath: .status.conditions[?(@.type=="Ready")].status
           name: Ready
           type: string
@@ -3143,24 +3146,22 @@ spec:
                   items:
                     properties:
                       match:
-                        items:
-                          properties:
-                            remoteRefs:
-                              items:
-                                properties:
-                                  remoteKey:
-                                    type: string
-                                required:
-                                  - remoteKey
-                                type: object
-                              type: array
-                            secretKey:
-                              type: string
-                          required:
-                            - remoteRefs
-                            - secretKey
-                          type: object
-                        type: array
+                        properties:
+                          remoteRefs:
+                            items:
+                              properties:
+                                remoteKey:
+                                  type: string
+                              required:
+                                - remoteKey
+                              type: object
+                            type: array
+                          secretKey:
+                            type: string
+                        required:
+                          - remoteRefs
+                          - secretKey
+                        type: object
                     required:
                       - match
                     type: object
@@ -3249,7 +3250,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.9.0
+    controller-gen.kubebuilder.io/version: v0.9.2
   creationTimestamp: null
   name: secretstores.external-secrets.io
 spec:
@@ -4278,6 +4279,7 @@ spec:
           type: string
         - jsonPath: .status.capabilities
           name: Capabilities
+          type: string
         - jsonPath: .status.conditions[?(@.type=="Ready")].status
           name: Ready
           type: string

design/002-secretsink.md → design/002-pushsecret.md

@@ -2,7 +2,7 @@
 ---
 title: PushSecret
 version: v1alpha1
-authors: 
+authors:
 creation-date: 2022-01-25
 status: draft
 ---
@@ -26,7 +26,7 @@ Secret Sink allows some inCluster generated secrets to also be available on a gi
 ### Goals
 - CRD Design for the PushSecret
 - Define the need for a SinkStore
-- 
+-
 ### Non-Goals
 Do not implement full compatibility mechanisms with each provider (we are not Terraform neither Crossplane)
 
@@ -130,17 +130,17 @@ spec:
     secret:
       name: foobar
   data:
-    match:
-    - secretKey: foobar
+  - match:
+      secretKey: foobar
       remoteRefs:
-      - remoteKey: my/path/foobar 
+      - remoteKey: my/path/foobar
         property: my-property #optional. To allow coming back from a 'dataFrom'
       - remoteKey: secret/my-path-foobar
         property: another-property
     rewrite:
-    - secretKey: game-(.+).(.+)
+      secretKey: game-(.+).(.+)
       remoteRefs:
-      - remoteKey: my/path/($1) 
+      - remoteKey: my/path/($1)
         property: prop-($2)
       - remoteKey: my-path-($1)-($2) #Applies this way to all other secretStores
 
@@ -148,7 +148,7 @@ status:
   refreshTime: "2019-08-12T12:33:02Z"
   conditions:
   - type: Ready
-    status: "True" 
+    status: "True"
     reason: "SecretSynced"
     message: "Secret was synced" #Fully synced
     lastTransitionTime: "2019-08-12T12:33:02Z"

e2e/framework/addon/eso.go

@@ -17,7 +17,7 @@ import (
 	"os"
 
 	// nolint
-	ginkgo "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/ginkgo/v2"
 )
 
 type ESO struct {
@@ -155,7 +155,7 @@ func WithCRDs() MutationFunc {
 }
 
 func (l *ESO) Install() error {
-	ginkgo.By("Installing eso\n")
+	By("Installing eso\n")
 	err := l.HelmChart.Install()
 	if err != nil {
 		return err
@@ -165,7 +165,7 @@ func (l *ESO) Install() error {
 }
 
 func (l *ESO) Uninstall() error {
-	ginkgo.By("Uninstalling eso")
+	By("Uninstalling eso")
 	err := l.HelmChart.Uninstall()
 	if err != nil {
 		return err

go.mod

@@ -91,7 +91,7 @@ require (
 	k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9
 	sigs.k8s.io/controller-runtime v0.11.2
 	sigs.k8s.io/controller-tools v0.9.2
-	software.sslmate.com/src/go-pkcs12 v0.0.0-20210415151418-c5206de65a78
+	software.sslmate.com/src/go-pkcs12 v0.2.0
 )
 
 require github.com/1Password/connect-sdk-go v1.5.0

pkg/controllers/pushsecret/pushsecret_controller.go

@@ -134,16 +134,14 @@ func (r *Reconciler) SetSecretToProviders(ctx context.Context, stores []v1beta1.
 			}
 		}()
 		for _, ref := range ps.Spec.Data {
-			for _, match := range ref.Match {
-				secretValue, ok := secret.Data[match.SecretKey]
-				if !ok {
-					return fmt.Errorf("secret key %v does not exist", match.SecretKey)
-				}
-				for _, rK := range match.RemoteRefs {
-					err := client.SetSecret(ctx, secretValue, rK)
-					if err != nil {
-						return fmt.Errorf(errSetSecretFailed, match.SecretKey, store.GetName(), err)
-					}
+			secretValue, ok := secret.Data[ref.Match.SecretKey]
+			if !ok {
+				return fmt.Errorf("secret key %v does not exist", ref.Match.SecretKey)
+			}
+			for _, rK := range ref.Match.RemoteRefs {
+				err := client.SetSecret(ctx, secretValue, rK)
+				if err != nil {
+					return fmt.Errorf(errSetSecretFailed, ref.Match.SecretKey, store.GetName(), err)
 				}
 			}
 		}

pkg/controllers/pushsecret/pushsecret_controller_test.go

@@ -361,13 +361,11 @@ var _ = Describe("pushsecret", func() {
 				},
 				Data: []esapi.PushSecretData{
 					{
-						Match: []esapi.PushSecretMatch{
-							{
-								SecretKey: "foo",
-								RemoteRefs: []esapi.PushSecretRemoteRefs{
-									{
-										RemoteKey: "bar",
-									},
+						Match: esapi.PushSecretMatch{
+							SecretKey: "foo",
+							RemoteRefs: []esapi.PushSecretRemoteRefs{
+								{
+									RemoteKey: "bar",
 								},
 							},
 						},

pkg/provider/aws/secretsmanager/fake/fake.go

@@ -38,7 +38,7 @@ func NewClient() *Client {
 
 func (sm *Client) CreateSecretWithContext(aws.Context, *awssm.CreateSecretInput, ...request.Option) (*awssm.CreateSecretOutput, error) {
 	value := "I'm a key"
-	output := awssm.CreateSecretOutput {
+	output := awssm.CreateSecretOutput{
 		Name: &value,
 	}
 	return &output, nil

pkg/provider/gcp/secretmanager/fake/fake.go

@@ -31,16 +31,49 @@ type MockSMClient struct {
 	addSecretFn    func(ctx context.Context, req *secretmanagerpb.AddSecretVersionRequest, opts ...gax.CallOption) (*secretmanagerpb.SecretVersion, error)
 	createSecretFn func(ctx context.Context, req *secretmanagerpb.CreateSecretRequest, opts ...gax.CallOption) (*secretmanagerpb.Secret, error)
 	closeFn        func() error
+	GetSecretFn    func(ctx context.Context, req *secretmanagerpb.GetSecretRequest, opts ...gax.CallOption) (*secretmanagerpb.Secret, error)
+}
+
+type AccessSecretVersionMockReturn struct {
+	Res *secretmanagerpb.AccessSecretVersionResponse
+	Err error
+}
+
+type AddSecretVersionMockReturn struct {
+	SecretVersion *secretmanagerpb.SecretVersion
+	Err           error
+}
+
+type GetSecretMockReturn struct {
+	Secret *secretmanagerpb.Secret
+	Err    error
+}
+
+type CreateSecretMockReturn struct {
+	Secret *secretmanagerpb.Secret
+	Err    error
 }
 
 func (mc *MockSMClient) GetSecret(ctx context.Context, req *secretmanagerpb.GetSecretRequest, opts ...gax.CallOption) (*secretmanagerpb.Secret, error) {
-	return nil, nil
+	return mc.GetSecretFn(ctx, req)
+}
+
+func (mc *MockSMClient) NewGetSecretFn(mock GetSecretMockReturn) {
+	mc.GetSecretFn = func(ctx context.Context, req *secretmanagerpb.GetSecretRequest, opts ...gax.CallOption) (*secretmanagerpb.Secret, error) {
+		return mock.Secret, mock.Err
+	}
 }
 
 func (mc *MockSMClient) AccessSecretVersion(ctx context.Context, req *secretmanagerpb.AccessSecretVersionRequest, opts ...gax.CallOption) (*secretmanagerpb.AccessSecretVersionResponse, error) {
 	return mc.accessSecretFn(ctx, req)
 }
 
+func (mc *MockSMClient) NewAccessSecretVersionFn(mock AccessSecretVersionMockReturn) {
+	mc.accessSecretFn = func(ctx context.Context, req *secretmanagerpb.AccessSecretVersionRequest, opts ...gax.CallOption) (*secretmanagerpb.AccessSecretVersionResponse, error) {
+		return mock.Res, mock.Err
+	}
+}
+
 func (mc *MockSMClient) ListSecrets(ctx context.Context, req *secretmanagerpb.ListSecretsRequest, opts ...gax.CallOption) *secretmanager.SecretIterator {
 	return mc.ListSecretsFn(ctx, req)
 }
@@ -52,10 +85,22 @@ func (mc *MockSMClient) AddSecretVersion(ctx context.Context, req *secretmanager
 	return mc.addSecretFn(ctx, req)
 }
 
+func (mc *MockSMClient) NewAddSecretVersionFn(mock AddSecretVersionMockReturn) {
+	mc.addSecretFn = func(ctx context.Context, req *secretmanagerpb.AddSecretVersionRequest, opts ...gax.CallOption) (*secretmanagerpb.SecretVersion, error) {
+		return mock.SecretVersion, mock.Err
+	}
+}
+
 func (mc *MockSMClient) CreateSecret(ctx context.Context, req *secretmanagerpb.CreateSecretRequest, opts ...gax.CallOption) (*secretmanagerpb.Secret, error) {
 	return mc.createSecretFn(ctx, req)
 }
 
+func (mc *MockSMClient) NewCreateSecretFn(mock CreateSecretMockReturn) {
+	mc.createSecretFn = func(ctx context.Context, req *secretmanagerpb.CreateSecretRequest, opts ...gax.CallOption) (*secretmanagerpb.Secret, error) {
+		return mock.Secret, mock.Err
+	}
+}
+
 func (mc *MockSMClient) NilClose() {
 	mc.closeFn = func() error {
 		return nil

pkg/provider/gcp/secretmanager/secretsmanager.go

@@ -251,15 +251,13 @@ func (sm *ProviderGCP) SetSecret(ctx context.Context, payload []byte, remoteRef
 
 	var gErr *apierror.APIError
 
-	if errors.As(err, &gErr) {
-		if err != nil && gErr.GRPCStatus().Code() == codes.NotFound {
+	if err != nil && errors.As(err, &gErr) {
+		if gErr.GRPCStatus().Code() == codes.NotFound {
 			gcpSecret, err = sm.SecretManagerClient.CreateSecret(ctx, createSecretReq)
 			if err != nil {
 				return err
 			}
-		}
-
-		if err != nil {
+		} else {
 			return err
 		}
 	}
@@ -278,6 +276,8 @@ func (sm *ProviderGCP) SetSecret(ctx context.Context, payload []byte, remoteRef
 		if err != nil && gErr.GRPCStatus().Code() != codes.NotFound {
 			return err
 		}
+	} else if err != nil {
+		return err
 	}
 
 	if gcpVersion != nil && gcpVersion.Payload != nil && string(payload) == string(gcpVersion.Payload.Data) {

pkg/provider/gcp/secretmanager/secretsmanager_test.go

@@ -20,19 +20,18 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/crossplane/crossplane-runtime/pkg/test"
+	"github.com/google/go-cmp/cmp"
 	"github.com/googleapis/gax-go/v2/apierror"
-	"github.com/stretchr/testify/assert"
 	secretmanagerpb "google.golang.org/genproto/googleapis/cloud/secretmanager/v1"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"k8s.io/utils/pointer"
 
 	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
-	fakeprr "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1/fakes"
 	v1 "github.com/external-secrets/external-secrets/apis/meta/v1"
 	"github.com/external-secrets/external-secrets/pkg/provider/gcp/secretmanager"
 	fakesm "github.com/external-secrets/external-secrets/pkg/provider/gcp/secretmanager/fake"
-	"github.com/external-secrets/external-secrets/pkg/provider/gcp/secretmanager/internal/fakes"
 )
 
 type secretManagerTestCase struct {
@@ -188,84 +187,38 @@ func TestSecretManagerGetSecret(t *testing.T) {
 	}
 }
 
-func TestSetSecret(t *testing.T) {
-	client := newClient()
-	pushRemoteRef := newPushRemoteRef()
-	secret := newSecret()
-	p := newProvider(client)
-
-	client.GetSecretReturns(&secret, nil)
-
-	err := p.SetSecret(context.Background(), nil, pushRemoteRef)
-	assert.Equal(t, err, nil)
+type fakeRef struct {
+	key string
 }
 
-func TestSetSecretAddSecretVersion(t *testing.T) {
-	client := newClient()
-	pushRemoteRef := newPushRemoteRef()
-	secret := newSecret()
-	p := newProvider(client)
-
-	expectedErr := "rpc error: code = Aborted desc = failed"
-	newStatus := status.Error(codes.Aborted, "failed")
-	err, _ := apierror.FromError(newStatus)
-
-	client.GetSecretReturns(&secret, nil)
-	client.AddSecretVersionReturns(nil, err)
-
-	expect := p.SetSecret(context.TODO(), nil, pushRemoteRef)
-	if assert.Error(t, expect) {
-		assert.Equal(t, expect.Error(), expectedErr)
-	}
+func (f fakeRef) GetRemoteKey() string {
+	return f.key
 }
 
-func TestSetSecretAccessSecretVersion(t *testing.T) {
-	client := newClient()
-	pushRemoteRef := newPushRemoteRef()
-	secret := newSecret()
-	p := newProvider(client)
-
-	expectedErr := "rpc error: code = Aborted desc = failed"
-	newStatus := status.Error(codes.Aborted, "failed")
-	err, _ := apierror.FromError(newStatus)
-
-	client.AccessSecretVersionReturns(nil, err)
-	client.GetSecretReturns(nil, err)
-	client.CreateSecretReturns(&secret, nil)
-
-	expect := p.SetSecret(context.Background(), nil, pushRemoteRef)
-	if assert.Error(t, expect) {
-		assert.Equal(t, expect.Error(), expectedErr)
-	}
-}
+func TestSetSecret(t *testing.T) {
+	ref := fakeRef{key: "/baz"}
 
-func TestSetSecretGetSecret404(t *testing.T) {
-	client := newClient()
-	pushRemoteRef := newPushRemoteRef()
-	secret := newSecret()
-	p := newProvider(client)
+	notFoundError := status.Error(codes.NotFound, "failed")
+	notFoundError, _ = apierror.FromError(notFoundError)
 
-	newStatus := status.Error(codes.NotFound, "failed")
-	err, _ := apierror.FromError(newStatus)
+	canceledError := status.Error(codes.Canceled, "canceled")
+	canceledError, _ = apierror.FromError(canceledError)
 
-	client.GetSecretReturns(nil, err)
-	client.CreateSecretReturns(&secret, nil)
-	client.AccessSecretVersionReturns(nil, err)
+	APIerror := fmt.Errorf("API Error")
+	labelError := fmt.Errorf("secret %v is not managed by external secrets", ref.GetRemoteKey())
 
-	p.SetSecret(context.Background(), nil, pushRemoteRef)
-	if client.AddSecretVersionCallCount() != 1 {
-		t.Error("expected addSecretVersion to be called")
-	}
-	if client.CreateSecretCallCount() != 1 {
-		t.Error("expected CreateSecret to be called")
-	}
-}
-
-func TestSetSecretWrongLabel(t *testing.T) {
-	client := newClient()
-	pushRemoteRef := newPushRemoteRef()
-	p := newProvider(client)
 	secret := secretmanagerpb.Secret{
+		Name: "projects/default/secrets/baz",
+		Replication: &secretmanagerpb.Replication{
+			Replication: &secretmanagerpb.Replication_Automatic_{
+				Automatic: &secretmanagerpb.Replication_Automatic{},
+			},
+		},
+		Labels: map[string]string{
+			"managed-by": "external-secrets",
+		},
+	}
+	wrongLabelSecret := secretmanagerpb.Secret{
 		Name: "projects/default/secrets/foo-bar",
 		Replication: &secretmanagerpb.Replication{
 			Replication: &secretmanagerpb.Replication_Automatic_{
@@ -277,36 +230,170 @@ func TestSetSecretWrongLabel(t *testing.T) {
 		},
 	}
 
-	pushRemoteRef.GetRemoteKeyReturns("foo-bar")
-	client.GetSecretReturns(&secret, nil)
-
-	expectedErr := "secret foo-bar is not managed by external secrets"
-	err := p.SetSecret(context.Background(), nil, pushRemoteRef)
+	smtc := secretManagerTestCase{
+		mockClient:     &fakesm.MockSMClient{},
+		apiInput:       makeValidAPIInput(),
+		ref:            makeValidRef(),
+		apiOutput:      makeValidAPIOutput(),
+		projectID:      "default",
+		apiErr:         nil,
+		expectError:    "",
+		expectedSecret: "",
+		expectedData:   map[string][]byte{},
+	}
 
-	if assert.Error(t, err) {
-		assert.Equal(t, err.Error(), expectedErr)
+	var payload = secretmanagerpb.SecretPayload{
+		Data: []byte("payload"),
 	}
-}
 
-func TestSetSecretAlreadyExists(t *testing.T) {
-	client := newClient()
-	pushRemoteRef := newPushRemoteRef()
-	secret := newSecret()
-	p := newProvider(client)
-	payload := &secretmanagerpb.SecretPayload{Data: []byte("bar")}
+	var payload2 = secretmanagerpb.SecretPayload{
+		Data: []byte("fake-value"),
+	}
 
-	client.AccessSecretVersionReturns(&secretmanagerpb.AccessSecretVersionResponse{
+	var res = secretmanagerpb.AccessSecretVersionResponse{
 		Name:    "projects/default/secrets/foo-bar",
-		Payload: payload,
-	}, nil)
-	client.GetSecretReturns(&secret, nil)
+		Payload: &payload,
+	}
+
+	var res2 = secretmanagerpb.AccessSecretVersionResponse{
+		Name:    "projects/default/secrets/baz",
+		Payload: &payload2,
+	}
+
+	var secretVersion = secretmanagerpb.SecretVersion{}
+
+	type args struct {
+		mock                          *fakesm.MockSMClient
+		GetSecretMockReturn           fakesm.GetSecretMockReturn
+		AccessSecretVersionMockReturn fakesm.AccessSecretVersionMockReturn
+		AddSecretVersionMockReturn    fakesm.AddSecretVersionMockReturn
+		CreateSecretMockReturn        fakesm.CreateSecretMockReturn
+	}
 
-	err := p.SetSecret(context.TODO(), []byte("bar"), pushRemoteRef)
-	if client.AddSecretVersionCallCount() != 0 {
-		t.Error("expected addSecretVersion to not be called")
+	type want struct {
+		err error
+	}
+	tests := map[string]struct {
+		reason string
+		args   args
+		want   want
+	}{
+		"SetSecret": {
+			reason: "SetSecret successfully pushes a secret",
+			args: args{
+				mock:                          smtc.mockClient,
+				GetSecretMockReturn:           fakesm.GetSecretMockReturn{Secret: &secret, Err: nil},
+				AccessSecretVersionMockReturn: fakesm.AccessSecretVersionMockReturn{Res: &res, Err: nil},
+				AddSecretVersionMockReturn:    fakesm.AddSecretVersionMockReturn{SecretVersion: &secretVersion, Err: nil}},
+			want: want{
+				err: nil,
+			},
+		},
+		"AddSecretVersion": {
+			reason: "secret not pushed if AddSecretVersion errors",
+			args: args{
+				mock:                          smtc.mockClient,
+				GetSecretMockReturn:           fakesm.GetSecretMockReturn{Secret: &secret, Err: nil},
+				AccessSecretVersionMockReturn: fakesm.AccessSecretVersionMockReturn{Res: &res, Err: nil},
+				AddSecretVersionMockReturn:    fakesm.AddSecretVersionMockReturn{SecretVersion: nil, Err: APIerror},
+			},
+			want: want{
+				err: APIerror,
+			},
+		},
+		"AccessSecretVersion": {
+			reason: "secret not pushed if AccessSecretVersion errors",
+			args: args{
+				mock:                          smtc.mockClient,
+				GetSecretMockReturn:           fakesm.GetSecretMockReturn{Secret: &secret, Err: nil},
+				AccessSecretVersionMockReturn: fakesm.AccessSecretVersionMockReturn{Res: nil, Err: APIerror},
+			},
+			want: want{
+				err: APIerror,
+			},
+		},
+		"NotManagedByESO": {
+			reason: "secret not pushed if not managed-by external-secrets",
+			args: args{
+				mock:                smtc.mockClient,
+				GetSecretMockReturn: fakesm.GetSecretMockReturn{Secret: &wrongLabelSecret, Err: nil},
+			},
+			want: want{
+				err: labelError,
+			},
+		},
+		"SecretAlreadyExists": {
+			reason: "don't push a secret with the same key and value",
+			args: args{
+				mock:                          smtc.mockClient,
+				AccessSecretVersionMockReturn: fakesm.AccessSecretVersionMockReturn{Res: &res2, Err: nil},
+				GetSecretMockReturn:           fakesm.GetSecretMockReturn{Secret: &secret, Err: nil},
+			},
+			want: want{
+				err: nil,
+			},
+		},
+		"GetSecretNotFound": {
+			reason: "secret is created if one doesn't already exist",
+			args: args{
+				mock:                          smtc.mockClient,
+				GetSecretMockReturn:           fakesm.GetSecretMockReturn{Secret: nil, Err: notFoundError},
+				AccessSecretVersionMockReturn: fakesm.AccessSecretVersionMockReturn{Res: nil, Err: notFoundError},
+				AddSecretVersionMockReturn:    fakesm.AddSecretVersionMockReturn{SecretVersion: &secretVersion, Err: nil},
+				CreateSecretMockReturn:        fakesm.CreateSecretMockReturn{Secret: &secret, Err: nil},
+			},
+			want: want{
+				err: nil,
+			},
+		},
+		"CreateSecretReturnsNotFoundError": {
+			reason: "secret not created if CreateSecret returns not found error",
+			args: args{
+				mock:                   smtc.mockClient,
+				GetSecretMockReturn:    fakesm.GetSecretMockReturn{Secret: nil, Err: notFoundError},
+				CreateSecretMockReturn: fakesm.CreateSecretMockReturn{Secret: &secret, Err: notFoundError},
+			},
+			want: want{
+				err: notFoundError,
+			},
+		},
+		"CreateSecretReturnsError": {
+			reason: "secret not created if CreateSecret returns error",
+			args: args{
+				mock:                smtc.mockClient,
+				GetSecretMockReturn: fakesm.GetSecretMockReturn{Secret: nil, Err: canceledError},
+			},
+			want: want{
+				err: canceledError,
+			},
+		},
+		"AccessSecretVersionReturnsError": {
+			reason: "access secret version for an existing secret returns error",
+			args: args{
+				mock:                          smtc.mockClient,
+				GetSecretMockReturn:           fakesm.GetSecretMockReturn{Secret: &secret, Err: nil},
+				AccessSecretVersionMockReturn: fakesm.AccessSecretVersionMockReturn{Res: nil, Err: canceledError},
+			},
+			want: want{
+				err: canceledError,
+			},
+		},
 	}
-	if err != nil {
-		t.Errorf("expected nil got error")
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			tc.args.mock.NewGetSecretFn(tc.args.GetSecretMockReturn)
+			tc.args.mock.NewCreateSecretFn(tc.args.CreateSecretMockReturn)
+			tc.args.mock.NewAccessSecretVersionFn(tc.args.AccessSecretVersionMockReturn)
+			tc.args.mock.NewAddSecretVersionFn(tc.args.AddSecretVersionMockReturn)
+
+			p := secretmanager.ProviderGCP{
+				SecretManagerClient: tc.args.mock,
+			}
+			err := p.SetSecret(context.Background(), []byte("fake-value"), ref)
+			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
+				t.Errorf("\nTesting SetSecret:\nName: %v\nReason: %v\nWant error: %v\nGot error: %v", name, tc.reason, tc.want.err, diff)
+			}
+		})
 	}
 }
 
@@ -423,33 +510,3 @@ func TestValidateStore(t *testing.T) {
 		})
 	}
 }
-
-// counterfeiter helper methods.
-func newClient() *fakes.GoogleSecretManagerClient {
-	return new(fakes.GoogleSecretManagerClient)
-}
-
-func newPushRemoteRef() *fakeprr.PushRemoteRef {
-	return new(fakeprr.PushRemoteRef)
-}
-
-func newSecret() secretmanagerpb.Secret {
-	return secretmanagerpb.Secret{
-		Name: "projects/default/secrets/foo-bar",
-		Replication: &secretmanagerpb.Replication{
-			Replication: &secretmanagerpb.Replication_Automatic_{
-				Automatic: &secretmanagerpb.Replication_Automatic{},
-			},
-		},
-		Labels: map[string]string{
-			"managed-by": "external-secrets",
-		},
-	}
-}
-
-func newProvider(client secretmanager.GoogleSecretManagerClient) secretmanager.ProviderGCP {
-	return secretmanager.ProviderGCP{
-		SecretManagerClient: client,
-		ProjectID:           "default",
-	}
-}

pkg/provider/vault/vault.go

@@ -385,6 +385,14 @@ func (v *client) SetSecret(ctx context.Context, value []byte, remoteRef esv1beta
 	if err != nil && !strings.Contains(err.Error(), "secret not found") {
 		return err
 	}
+
+	// Retrieve the secret value to be pushed and convert it to string form.
+	pushSecretValue := string(value)
+
+	if vaultSecretValue == pushSecretValue {
+		return nil
+	}
+
 	// If the secret exists (err == nil), we should check if it is managed by external-secrets
 	if err == nil {
 		metadata, err := v.readSecretMetadata(ctx, remoteRef.GetRemoteKey())
@@ -396,13 +404,6 @@ func (v *client) SetSecret(ctx context.Context, value []byte, remoteRef esv1beta
 			return fmt.Errorf("secret not managed by external-secrets")
 		}
 	}
-
-	// Retrieve the secret value to be pushed and convert it to string form.
-	pushSecretValue := string(value)
-
-	if vaultSecretValue == pushSecretValue {
-		return nil
-	}
 	_, err = v.logical.WriteWithContext(ctx, metaPath, label)
 	if err != nil {
 		return err
@@ -749,7 +750,6 @@ func (v *client) readSecret(ctx context.Context, path, version string) (map[stri
 		// Vault KV2 has data embedded within sub-field
 		// reference - https://www.vaultproject.io/api/secret/kv/kv-v2#read-secret-version
 		dataInt, ok := vaultSecret.Data["data"]
-
 		if !ok {
 			return nil, errors.New(errDataField)
 		}

pkg/provider/vault/vault_test.go

@@ -24,7 +24,6 @@ import (
 	"github.com/crossplane/crossplane-runtime/pkg/test"
 	"github.com/google/go-cmp/cmp"
 	vault "github.com/hashicorp/vault/api"
-	"gotest.tools/v3/assert"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/pointer"
@@ -38,7 +37,6 @@ import (
 const (
 	tokenSecretName  = "example-secret-token"
 	secretDataString = "some-creds"
-	secretPath       = "secret"
 )
 
 var (
@@ -1410,129 +1408,114 @@ func (f fakeRef) GetRemoteKey() string {
 	return f.key
 }
 
-func TestSetSecretUpdateSecretNotFound(t *testing.T) {
-	path := secretPath
-	secretData := map[string]interface{}{
-		"data": map[string]interface{}{
-			"fake key": "fake value",
-		},
-	}
-	f := fake.Logical{
-		ReadWithDataWithContextFn: fake.NewReadWithContextFn(nil, fmt.Errorf("secret not found")),
-	}
-	f.WriteWithContextFn = fake.WriteChangingReadContext(secretData, f)
-	client := client{
-		store: &esv1beta1.VaultProvider{
-			Path: &path,
-		},
-		logical: f,
-	}
-	ref := fakeRef{key: "I'm a key"}
+func TestSetSecret(t *testing.T) {
+	noPermission := errors.New("no permission")
+	secretNotFound := errors.New("secret not found")
 
-	err := client.SetSecret(context.Background(), []byte("HI"), ref)
-	assert.Equal(t, err, nil)
-}
-
-func TestSetSecretUpdateSecretNotFoundWithError(t *testing.T) {
-	path := secretPath
-	f := fake.Logical{
-		ReadWithDataWithContextFn: fake.NewReadWithContextFn(nil, fmt.Errorf("secret not found")),
-	}
-	f.WriteWithContextFn = fake.NewWriteWithContextFn(nil, fmt.Errorf("no permissions"))
-	client := client{
-		store: &esv1beta1.VaultProvider{
-			Path: &path,
-		},
-		logical: f,
+	type args struct {
+		store    *esv1beta1.VaultProvider
+		vLogical Logical
 	}
-	ref := fakeRef{key: "I'm a key"}
 
-	err := client.SetSecret(context.Background(), []byte("HI"), ref)
-	assert.Equal(t, err.Error(), "no permissions")
-}
-func TestSetSecretEqualsPushSecret(t *testing.T) {
-	path := secretPath
-	f := fake.Logical{
-		ReadWithDataWithContextFn: fake.NewReadWithContextFn(map[string]interface{}{
-			"key": "fake value",
-		}, nil),
+	type want struct {
+		err error
 	}
-	f.WriteWithContextFn = fake.NewWriteWithContextFn(nil, nil)
-	client := client{
-		store: &esv1beta1.VaultProvider{
-			Path: &path,
+	tests := map[string]struct {
+		reason string
+		args   args
+		want   want
+	}{
+		"SetSecret": {
+			reason: "secret is successfully set, with no existing vault secret",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(nil, secretNotFound),
+					WriteWithContextFn:        fake.NewWriteWithContextFn(nil, nil),
+				},
+			},
+			want: want{
+				err: nil,
+			},
 		},
-		logical: f,
-	}
-	ref := fakeRef{key: "key"}
-
-	err := client.SetSecret(context.Background(), []byte("fake value"), ref)
 
-	assert.Equal(t, err, nil)
-}
-
-func TestSetSecretEqualsPushSecretWithError(t *testing.T) {
-	path := secretPath
-	f := fake.Logical{
-		ReadWithDataWithContextFn: fake.NewReadWithContextFn(map[string]interface{}{
-			"key": "wrong-key",
-		}, nil),
-	}
-	f.WriteWithContextFn = fake.NewWriteWithContextFn(nil, fmt.Errorf("boom"))
-	client := client{
-		store: &esv1beta1.VaultProvider{
-			Path: &path,
+		"SetSecretWithWriteError": {
+			reason: "secret cannot be pushed if write fails",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(nil, secretNotFound),
+					WriteWithContextFn:        fake.NewWriteWithContextFn(nil, noPermission),
+				},
+			},
+			want: want{
+				err: noPermission,
+			},
 		},
-		logical: f,
-	}
-	ref := fakeRef{key: "key"}
 
-	err := client.SetSecret(context.Background(), []byte("fake value"), ref)
-	assert.Equal(t, err.Error(), "boom")
-}
-func TestSetSecretErrorReadingSecret(t *testing.T) {
-	path := secretPath
-	f := fake.Logical{
-		ReadWithDataWithContextFn: fake.NewReadWithContextFn(nil, fmt.Errorf("you shall not pass")),
-	}
-	f.WriteWithContextFn = fake.NewWriteWithContextFn(nil, nil)
-	client := client{
-		store: &esv1beta1.VaultProvider{
-			Path: &path,
+		"SetSecretEqualsPushSecret": {
+			reason: "vault secret kv equals secret to push kv",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(map[string]interface{}{
+						"data": map[string]interface{}{
+							"fake-key": "fake-value",
+						},
+					}, nil),
+				},
+			},
+			want: want{
+				err: nil,
+			},
 		},
-		logical: f,
-	}
-	ref := fakeRef{key: "key"}
-
-	err := client.SetSecret(context.Background(), []byte("fake value"), ref)
-	assert.ErrorContains(t, err, "you shall not pass")
-}
-
-// Test if secret is managed by eso.
-func TestSetSecretNotManagedByESO(t *testing.T) {
-	path := secretPath
-
-	f := fake.Logical{
-		ReadWithDataWithContextFn: fake.NewReadWithContextFn(map[string]interface{}{
-			"key": "fake value",
-		}, nil),
-	}
 
-	f.WriteWithContextFn = fake.NewWriteWithContextFn(map[string]interface{}{
-		"custom_metadata": map[string]string{
-			"managed-by": "not-external-secrets",
+		"SetSecretErrorReadingSecret": {
+			reason: "error occurs if secret cannot be read",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(nil, noPermission),
+				},
+			},
+			want: want{
+				err: fmt.Errorf(errReadSecret, noPermission),
+			},
 		},
-	}, nil)
 
-	client := client{
-		store: &esv1beta1.VaultProvider{
-			Path: &path,
+		"SetSecretNotManagedByESO": {
+			reason: "a secret not managed by ESO cannot be updated",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(map[string]interface{}{
+						"data": map[string]interface{}{
+							"fake-key": "fake-value2",
+							"custom_metadata": map[string]interface{}{
+								"managed-by": "not-external-secrets",
+							},
+						},
+					}, nil),
+				},
+			},
+			want: want{
+				err: errors.New("secret not managed by external-secrets"),
+			},
 		},
-		logical: f,
 	}
-	ref := fakeRef{key: "key"}
 
-	err := client.SetSecret(context.Background(), []byte("fake value"), ref)
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			ref := fakeRef{key: "fake-key"}
+			client := &client{
+				logical: tc.args.vLogical,
+				store:   tc.args.store,
+			}
+			err := client.SetSecret(context.Background(), []byte("fake-value"), ref)
 
-	assert.Equal(t, err.Error(), "secret not managed by external-secrets")
+			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
+				t.Errorf("\nTesting SetSecret:\nName: %v\nReason: %v\nWant error: %v\nGot error: %v", name, tc.reason, tc.want.err, diff)
+			}
+		})
+	}
 }