feat: refactor provider / secretsclient interface (#55)

* feat: refactor provider / secretsclient interface

see #53

pkg/controllers/externalsecret/externalsecret_controller.go

@@ -94,7 +94,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		return ctrl.Result{RequeueAfter: requeueAfter}, nil
 	}
 
-	providerClient, err := storeProvider.New(ctx, store, r.Client, req.Namespace)
+	secretClient, err := storeProvider.NewClient(ctx, store, r.Client, req.Namespace)
 	if err != nil {
 		log.Error(err, "could not get provider client")
 		conditionSynced := NewExternalSecretCondition(esv1alpha1.ExternalSecretReady, corev1.ConditionFalse, esv1alpha1.ConditionReasonSecretSyncedError, err.Error())
@@ -112,7 +112,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		secret.Labels = externalSecret.Labels
 		secret.Annotations = externalSecret.Annotations
 
-		secret.Data, err = r.getProviderSecretData(ctx, providerClient, &externalSecret)
+		secret.Data, err = r.getProviderSecretData(ctx, secretClient, &externalSecret)
 		if err != nil {
 			return fmt.Errorf("could not get secret data from provider: %w", err)
 		}
@@ -173,7 +173,7 @@ func (r *Reconciler) getStore(ctx context.Context, externalSecret *esv1alpha1.Ex
 	return &secretStore, nil
 }
 
-func (r *Reconciler) getProviderSecretData(ctx context.Context, providerClient provider.Provider, externalSecret *esv1alpha1.ExternalSecret) (map[string][]byte, error) {
+func (r *Reconciler) getProviderSecretData(ctx context.Context, providerClient provider.SecretsClient, externalSecret *esv1alpha1.ExternalSecret) (map[string][]byte, error) {
 	providerData := make(map[string][]byte)
 
 	for _, remoteRef := range externalSecret.Spec.DataFrom {

pkg/controllers/externalsecret/externalsecret_controller_test.go

@@ -393,7 +393,7 @@ var _ = Describe("ExternalSecret controller", func() {
 			}
 
 			fakeProvider.WithNew(func(context.Context, esv1alpha1.GenericStore, client.Client,
-				string) (provider.Provider, error) {
+				string) (provider.SecretsClient, error) {
 				return nil, fmt.Errorf("artificial constructor error")
 			})
 			Expect(k8sClient.Create(ctx, es)).Should(Succeed())

pkg/provider/aws/provider.go

@@ -1,57 +1,41 @@
 package aws
 
 import (
+	"context"
 	"fmt"
 
-	"github.com/aws/aws-sdk-go/aws"
-	"github.com/aws/aws-sdk-go/aws/credentials"
-	"github.com/aws/aws-sdk-go/aws/credentials/stscreds"
-	"github.com/aws/aws-sdk-go/aws/request"
-	"github.com/aws/aws-sdk-go/aws/session"
-	"github.com/aws/aws-sdk-go/service/sts"
-	ctrl "sigs.k8s.io/controller-runtime"
-)
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
-// Config contains configuration to create a new AWS provider.
-type Config struct {
-	AssumeRole string
-	Region     string
-	APIRetries int
-}
+	esv1alpha1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1alpha1"
+	"github.com/external-secrets/external-secrets/pkg/provider"
+	"github.com/external-secrets/external-secrets/pkg/provider/aws/secretsmanager"
+	awssess "github.com/external-secrets/external-secrets/pkg/provider/aws/session"
+	"github.com/external-secrets/external-secrets/pkg/provider/schema"
+)
 
-var log = ctrl.Log.WithName("provider").WithName("aws")
+// Provider satisfies the provider interface.
+type Provider struct{}
 
-// NewSession creates a new aws session based on the supported input methods.
-// https://docs.aws.amazon.com/sdk-for-go/v1/developer-guide/configuring-sdk.html#specifying-credentials
-func NewSession(sak, aks, region, role string, stsprovider STSProvider) (*session.Session, error) {
-	config := aws.NewConfig()
-	sessionOpts := session.Options{
-		Config: *config,
+// NewClient constructs a new secrets client based on the provided store.
+func (p *Provider) NewClient(ctx context.Context, store esv1alpha1.GenericStore, kube client.Client, namespace string) (provider.SecretsClient, error) {
+	if store == nil {
+		return nil, fmt.Errorf("store is nil")
 	}
-	if sak != "" && aks != "" {
-		sessionOpts.Config.Credentials = credentials.NewStaticCredentials(aks, sak, "")
-		sessionOpts.SharedConfigState = session.SharedConfigDisable
+	spec := store.GetSpec()
+	if spec == nil {
+		return nil, fmt.Errorf("store is missing spec")
 	}
-	sess, err := session.NewSessionWithOptions(sessionOpts)
-	if err != nil {
-		return nil, fmt.Errorf("unable to create aws session: %w", err)
+	if spec.Provider == nil {
+		return nil, fmt.Errorf("storeSpec is missing provider")
 	}
-	if region != "" {
-		log.V(1).Info("using region", "region", region)
-		sess.Config.WithRegion(region)
+	if spec.Provider.AWSSM != nil {
+		return secretsmanager.New(ctx, store, kube, namespace, awssess.DefaultSTSProvider)
 	}
-
-	if role != "" {
-		log.V(1).Info("assuming role", "role", role)
-		stsclient := stsprovider(sess)
-		sess.Config.WithCredentials(stscreds.NewCredentialsWithClient(stsclient, role))
-	}
-	sess.Handlers.Build.PushBack(request.WithAppendUserAgent("external-secrets"))
-	return sess, nil
+	return nil, fmt.Errorf("AWS Provider spec missing")
 }
 
-type STSProvider func(*session.Session) stscreds.AssumeRoler
-
-func DefaultSTSProvider(sess *session.Session) stscreds.AssumeRoler {
-	return sts.New(sess)
+func init() {
+	schema.Register(&Provider{}, &esv1alpha1.SecretStoreProvider{
+		AWSSM: &esv1alpha1.AWSSMProvider{},
+	})
 }

pkg/provider/aws/provider_test.go

@@ -0,0 +1,69 @@
+package aws
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	clientfake "sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	esv1alpha1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1alpha1"
+)
+
+func TestProvider(t *testing.T) {
+	cl := clientfake.NewClientBuilder().Build()
+	p := Provider{}
+
+	tbl := []struct {
+		test   string
+		store  esv1alpha1.GenericStore
+		expErr bool
+	}{
+		{
+			test:   "should not create provider due to nil store",
+			store:  nil,
+			expErr: true,
+		},
+		{
+			test:   "should not create provider due to missing provider",
+			expErr: true,
+			store: &esv1alpha1.SecretStore{
+				Spec: esv1alpha1.SecretStoreSpec{},
+			},
+		},
+		{
+			test:   "should not create provider due to missing provider field",
+			expErr: true,
+			store: &esv1alpha1.SecretStore{
+				Spec: esv1alpha1.SecretStoreSpec{
+					Provider: &esv1alpha1.SecretStoreProvider{},
+				},
+			},
+		},
+
+		{
+			test:   "should create provider",
+			expErr: false,
+			store: &esv1alpha1.SecretStore{
+				Spec: esv1alpha1.SecretStoreSpec{
+					Provider: &esv1alpha1.SecretStoreProvider{
+						AWSSM: &esv1alpha1.AWSSMProvider{},
+					},
+				},
+			},
+		},
+	}
+	for i := range tbl {
+		row := tbl[i]
+		t.Run(row.test, func(t *testing.T) {
+			sc, err := p.NewClient(context.TODO(), row.store, cl, "foo")
+			if row.expErr {
+				assert.Error(t, err)
+				assert.Nil(t, sc)
+			} else {
+				assert.Nil(t, err)
+				assert.NotNil(t, sc)
+			}
+		})
+	}
+}

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

@@ -21,7 +21,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 )
 
-// Client implements the provider interface.
+// Client implements the aws secretsmanager interface.
 type Client struct {
 	valFn func(*awssm.GetSecretValueInput) (*awssm.GetSecretValueOutput, error)
 }

pkg/provider/aws/secretsmanager/secretsmanager.go

@@ -27,14 +27,13 @@ import (
 
 	esv1alpha1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1alpha1"
 	"github.com/external-secrets/external-secrets/pkg/provider"
-	"github.com/external-secrets/external-secrets/pkg/provider/aws"
-	"github.com/external-secrets/external-secrets/pkg/provider/schema"
+	awssess "github.com/external-secrets/external-secrets/pkg/provider/aws/session"
 )
 
 // SecretsManager is a provider for AWS SecretsManager.
 type SecretsManager struct {
 	session     *session.Session
-	stsProvider aws.STSProvider
+	stsProvider awssess.STSProvider
 	client      SMInterface
 }
 
@@ -47,7 +46,10 @@ type SMInterface interface {
 var log = ctrl.Log.WithName("provider").WithName("aws").WithName("secretsmanager")
 
 // New constructs a SecretsManager Provider that is specific to a store.
-func (sm *SecretsManager) New(ctx context.Context, store esv1alpha1.GenericStore, kube client.Client, namespace string) (provider.Provider, error) {
+func New(ctx context.Context, store esv1alpha1.GenericStore, kube client.Client, namespace string, stsProvider awssess.STSProvider) (provider.SecretsClient, error) {
+	sm := &SecretsManager{
+		stsProvider: stsProvider,
+	}
 	if store == nil {
 		return nil, fmt.Errorf("found nil store")
 	}
@@ -107,10 +109,7 @@ func (sm *SecretsManager) New(ctx context.Context, store esv1alpha1.GenericStore
 			return nil, fmt.Errorf("missing AccessKeyID")
 		}
 	}
-	if sm.stsProvider == nil {
-		sm.stsProvider = aws.DefaultSTSProvider
-	}
-	sess, err := aws.NewSession(sak, aks, smProvider.Region, smProvider.Role, sm.stsProvider)
+	sess, err := awssess.New(sak, aks, smProvider.Region, smProvider.Role, sm.stsProvider)
 	if err != nil {
 		return nil, err
 	}
@@ -174,9 +173,3 @@ func (sm *SecretsManager) GetSecretMap(ctx context.Context, ref esv1alpha1.Exter
 	}
 	return secretData, nil
 }
-
-func init() {
-	schema.Register(&SecretsManager{}, &esv1alpha1.SecretStoreProvider{
-		AWSSM: &esv1alpha1.AWSSMProvider{},
-	})
-}

pkg/provider/aws/secretsmanager/secretsmanager_test.go

@@ -34,8 +34,8 @@ import (
 
 	esv1alpha1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1alpha1"
 	esmeta "github.com/external-secrets/external-secrets/apis/meta/v1"
-	awsprovider "github.com/external-secrets/external-secrets/pkg/provider/aws"
 	fakesm "github.com/external-secrets/external-secrets/pkg/provider/aws/secretsmanager/fake"
+	awssess "github.com/external-secrets/external-secrets/pkg/provider/aws/session"
 )
 
 func TestConstructor(t *testing.T) {
@@ -348,7 +348,7 @@ type ConstructorRow struct {
 	store             esv1alpha1.GenericStore
 	secrets           []v1.Secret
 	namespace         string
-	stsProvider       awsprovider.STSProvider
+	stsProvider       awssess.STSProvider
 	expectProvider    bool
 	expectErr         string
 	expectedKeyID     string
@@ -370,10 +370,7 @@ func testRow(t *testing.T, row ConstructorRow) {
 			os.Unsetenv(k)
 		}
 	}()
-	sm := SecretsManager{
-		stsProvider: row.stsProvider,
-	}
-	newsm, err := sm.New(context.Background(), row.store, kc, row.namespace)
+	newsm, err := New(context.Background(), row.store, kc, row.namespace, row.stsProvider)
 	if !ErrorContains(err, row.expectErr) {
 		t.Errorf("expected error %s but found %s", row.expectErr, err.Error())
 	}
@@ -392,22 +389,22 @@ func testRow(t *testing.T, row ConstructorRow) {
 
 func TestSMEnvCredentials(t *testing.T) {
 	k8sClient := clientfake.NewClientBuilder().Build()
-	sm := &SecretsManager{}
 	os.Setenv("AWS_SECRET_ACCESS_KEY", "1111")
 	os.Setenv("AWS_ACCESS_KEY_ID", "2222")
 	defer os.Unsetenv("AWS_SECRET_ACCESS_KEY")
 	defer os.Unsetenv("AWS_ACCESS_KEY_ID")
-	smi, err := sm.New(context.Background(), &esv1alpha1.SecretStore{
+	smi, err := New(context.Background(), &esv1alpha1.SecretStore{
 		Spec: esv1alpha1.SecretStoreSpec{
 			Provider: &esv1alpha1.SecretStoreProvider{
 				// defaults
 				AWSSM: &esv1alpha1.AWSSMProvider{},
 			},
 		},
-	}, k8sClient, "example-ns")
+	}, k8sClient, "example-ns", awssess.DefaultSTSProvider)
 	assert.Nil(t, err)
 	assert.NotNil(t, smi)
-
+	sm, ok := smi.(*SecretsManager)
+	assert.True(t, ok)
 	creds, err := sm.session.Config.Credentials.Get()
 	assert.Nil(t, err)
 	assert.Equal(t, creds.AccessKeyID, "2222")
@@ -434,21 +431,11 @@ func TestSMAssumeRole(t *testing.T) {
 			}, nil
 		},
 	}
-	sm := &SecretsManager{
-		stsProvider: func(se *session.Session) stscreds.AssumeRoler {
-			// check if the correct temporary credentials were used
-			creds, err := se.Config.Credentials.Get()
-			assert.Nil(t, err)
-			assert.Equal(t, creds.AccessKeyID, "2222")
-			assert.Equal(t, creds.SecretAccessKey, "1111")
-			return sts
-		},
-	}
 	os.Setenv("AWS_SECRET_ACCESS_KEY", "1111")
 	os.Setenv("AWS_ACCESS_KEY_ID", "2222")
 	defer os.Unsetenv("AWS_SECRET_ACCESS_KEY")
 	defer os.Unsetenv("AWS_ACCESS_KEY_ID")
-	smi, err := sm.New(context.Background(), &esv1alpha1.SecretStore{
+	smi, err := New(context.Background(), &esv1alpha1.SecretStore{
 		Spec: esv1alpha1.SecretStoreSpec{
 			Provider: &esv1alpha1.SecretStoreProvider{
 				// do assume role!
@@ -457,10 +444,19 @@ func TestSMAssumeRole(t *testing.T) {
 				},
 			},
 		},
-	}, k8sClient, "example-ns")
+	}, k8sClient, "example-ns", func(se *session.Session) stscreds.AssumeRoler {
+		// check if the correct temporary credentials were used
+		creds, err := se.Config.Credentials.Get()
+		assert.Nil(t, err)
+		assert.Equal(t, creds.AccessKeyID, "2222")
+		assert.Equal(t, creds.SecretAccessKey, "1111")
+		return sts
+	})
 	assert.Nil(t, err)
 	assert.NotNil(t, smi)
 
+	sm, ok := smi.(*SecretsManager)
+	assert.True(t, ok)
 	creds, err := sm.session.Config.Credentials.Get()
 	assert.Nil(t, err)
 	assert.Equal(t, creds.AccessKeyID, "3333")

pkg/provider/aws/session/session.go

@@ -0,0 +1,57 @@
+package session
+
+import (
+	"fmt"
+
+	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/credentials"
+	"github.com/aws/aws-sdk-go/aws/credentials/stscreds"
+	"github.com/aws/aws-sdk-go/aws/request"
+	awssess "github.com/aws/aws-sdk-go/aws/session"
+	"github.com/aws/aws-sdk-go/service/sts"
+	ctrl "sigs.k8s.io/controller-runtime"
+)
+
+// Config contains configuration to create a new AWS provider.
+type Config struct {
+	AssumeRole string
+	Region     string
+	APIRetries int
+}
+
+var log = ctrl.Log.WithName("provider").WithName("aws")
+
+// New creates a new aws session based on the supported input methods.
+// https://docs.aws.amazon.com/sdk-for-go/v1/developer-guide/configuring-sdk.html#specifying-credentials
+func New(sak, aks, region, role string, stsprovider STSProvider) (*awssess.Session, error) {
+	config := aws.NewConfig()
+	sessionOpts := awssess.Options{
+		Config: *config,
+	}
+	if sak != "" && aks != "" {
+		sessionOpts.Config.Credentials = credentials.NewStaticCredentials(aks, sak, "")
+		sessionOpts.SharedConfigState = awssess.SharedConfigDisable
+	}
+	sess, err := awssess.NewSessionWithOptions(sessionOpts)
+	if err != nil {
+		return nil, fmt.Errorf("unable to create aws session: %w", err)
+	}
+	if region != "" {
+		log.V(1).Info("using region", "region", region)
+		sess.Config.WithRegion(region)
+	}
+
+	if role != "" {
+		log.V(1).Info("assuming role", "role", role)
+		stsclient := stsprovider(sess)
+		sess.Config.WithCredentials(stscreds.NewCredentialsWithClient(stsclient, role))
+	}
+	sess.Handlers.Build.PushBack(request.WithAppendUserAgent("external-secrets"))
+	return sess, nil
+}
+
+type STSProvider func(*awssess.Session) stscreds.AssumeRoler
+
+func DefaultSTSProvider(sess *awssess.Session) stscreds.AssumeRoler {
+	return sts.New(sess)
+}

pkg/provider/aws/session/session_test.go

@@ -0,0 +1,77 @@
+package session
+
+import (
+	"testing"
+	"time"
+
+	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/credentials/stscreds"
+	"github.com/aws/aws-sdk-go/aws/session"
+	"github.com/aws/aws-sdk-go/service/sts"
+	"github.com/stretchr/testify/assert"
+
+	fakesm "github.com/external-secrets/external-secrets/pkg/provider/aws/secretsmanager/fake"
+)
+
+func TestSession(t *testing.T) {
+	tbl := []struct {
+		test              string
+		aks               string
+		sak               string
+		region            string
+		role              string
+		sts               STSProvider
+		expectedKeyID     string
+		expectedSecretKey string
+	}{
+		{
+			test:              "test default role provider",
+			aks:               "2222",
+			sak:               "1111",
+			region:            "xxxxx",
+			role:              "",
+			sts:               DefaultSTSProvider,
+			expectedSecretKey: "1111",
+			expectedKeyID:     "2222",
+		},
+		{
+			test:   "test custom sts provider",
+			aks:    "1111",
+			sak:    "2222",
+			region: "xxxxx",
+			role:   "zzzzz",
+			sts: func(*session.Session) stscreds.AssumeRoler {
+				return &fakesm.AssumeRoler{
+					AssumeRoleFunc: func(input *sts.AssumeRoleInput) (*sts.AssumeRoleOutput, error) {
+						assert.Equal(t, *input.RoleArn, "zzzzz")
+						return &sts.AssumeRoleOutput{
+							AssumedRoleUser: &sts.AssumedRoleUser{
+								Arn:           aws.String("1123132"),
+								AssumedRoleId: aws.String("xxxxx"),
+							},
+							Credentials: &sts.Credentials{
+								SecretAccessKey: aws.String("3333"),
+								AccessKeyId:     aws.String("4444"),
+								Expiration:      aws.Time(time.Now().Add(time.Hour)),
+								SessionToken:    aws.String("6666"),
+							},
+						}, nil
+					},
+				}
+			},
+			expectedSecretKey: "3333",
+			expectedKeyID:     "4444",
+		},
+	}
+	for i := range tbl {
+		row := tbl[i]
+		t.Run(row.test, func(t *testing.T) {
+			sess, err := New(row.sak, row.aks, row.region, row.role, row.sts)
+			assert.Nil(t, err)
+			creds, err := sess.Config.Credentials.Get()
+			assert.Nil(t, err)
+			assert.Equal(t, row.expectedKeyID, creds.AccessKeyID)
+			assert.Equal(t, row.expectedSecretKey, creds.SecretAccessKey)
+		})
+	}
+}

pkg/provider/fake/fake.go

@@ -29,12 +29,12 @@ var _ provider.Provider = &Client{}
 // Client is a fake client for testing.
 type Client struct {
 	NewFn func(context.Context, esv1alpha1.GenericStore, client.Client,
-		string) (provider.Provider, error)
+		string) (provider.SecretsClient, error)
 	GetSecretFn    func(context.Context, esv1alpha1.ExternalSecretDataRemoteRef) ([]byte, error)
 	GetSecretMapFn func(context.Context, esv1alpha1.ExternalSecretDataRemoteRef) (map[string][]byte, error)
 }
 
-// New returns a fake client.
+// New returns a fake provider/client.
 func New() *Client {
 	v := &Client{
 		GetSecretFn: func(context.Context, esv1alpha1.ExternalSecretDataRemoteRef) ([]byte, error) {
@@ -45,7 +45,7 @@ func New() *Client {
 		},
 	}
 
-	v.NewFn = func(context.Context, esv1alpha1.GenericStore, client.Client, string) (provider.Provider, error) {
+	v.NewFn = func(context.Context, esv1alpha1.GenericStore, client.Client, string) (provider.SecretsClient, error) {
 		return v, nil
 	}
 
@@ -85,16 +85,16 @@ func (v *Client) WithGetSecretMap(secData map[string][]byte, err error) *Client
 
 // WithNew wraps the fake provider factory function.
 func (v *Client) WithNew(f func(context.Context, esv1alpha1.GenericStore, client.Client,
-	string) (provider.Provider, error)) *Client {
+	string) (provider.SecretsClient, error)) *Client {
 	v.NewFn = f
 	return v
 }
 
-// New returns a new fake provider.
-func (v *Client) New(ctx context.Context, store esv1alpha1.GenericStore, kube client.Client, namespace string) (provider.Provider, error) {
-	client, err := v.NewFn(ctx, store, kube, namespace)
+// NewClient returns a new fake provider.
+func (v *Client) NewClient(ctx context.Context, store esv1alpha1.GenericStore, kube client.Client, namespace string) (provider.SecretsClient, error) {
+	c, err := v.NewFn(ctx, store, kube, namespace)
 	if err != nil {
 		return nil, err
 	}
-	return client, nil
+	return c, nil
 }

pkg/provider/provider.go

@@ -24,9 +24,12 @@ import (
 
 // Provider is a common interface for interacting with secret backends.
 type Provider interface {
-	// New constructs a SecretsManager Provider
-	New(ctx context.Context, store esv1alpha1.GenericStore, kube client.Client, namespace string) (Provider, error)
+	// NewClient constructs a SecretsManager Provider
+	NewClient(ctx context.Context, store esv1alpha1.GenericStore, kube client.Client, namespace string) (SecretsClient, error)
+}
 
+// SecretsClient provides access to secrets.
+type SecretsClient interface {
 	// GetSecret returns a single secret from the provider
 	GetSecret(ctx context.Context, ref esv1alpha1.ExternalSecretDataRemoteRef) ([]byte, error)
 

pkg/provider/register/register.go

@@ -18,5 +18,5 @@ package register
 import (
 
 	// register awssm provider.
-	_ "github.com/external-secrets/external-secrets/pkg/provider/aws/secretsmanager"
+	_ "github.com/external-secrets/external-secrets/pkg/provider/aws"
 )

pkg/provider/schema/schema_test.go

@@ -27,7 +27,7 @@ import (
 type PP struct{}
 
 // New constructs a SecretsManager Provider.
-func (p *PP) New(ctx context.Context, store esv1alpha1.GenericStore, kube client.Client, namespace string) (provider.Provider, error) {
+func (p *PP) NewClient(ctx context.Context, store esv1alpha1.GenericStore, kube client.Client, namespace string) (provider.SecretsClient, error) {
 	return p, nil
 }