feat: Add AWSProvider.prefix to aws secrets manager (#4612)

Signed-off-by: Justin Walz <justin.walz10@gmail.com>
Co-authored-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

pkg/provider/aws/provider.go

@@ -154,7 +154,7 @@ func newClient(ctx context.Context, store esv1beta1.GenericStore, kube client.Cl
 		sess := &session.Session{Config: cfg}
 		switch prov.Service {
 		case esv1beta1.AWSServiceSecretsManager:
-			return secretsmanager.New(sess, cfg, prov.SecretsManager, true)
+			return secretsmanager.New(sess, cfg, prov.SecretsManager, storeSpec.Provider.AWS.Prefix, true)
 		case esv1beta1.AWSServiceParameterStore:
 			return parameterstore.New(sess, cfg, storeSpec.Provider.AWS.Prefix, true)
 		}
@@ -193,7 +193,7 @@ func newClient(ctx context.Context, store esv1beta1.GenericStore, kube client.Cl
 
 	switch prov.Service {
 	case esv1beta1.AWSServiceSecretsManager:
-		return secretsmanager.New(sess, cfg, prov.SecretsManager, false)
+		return secretsmanager.New(sess, cfg, prov.SecretsManager, storeSpec.Provider.AWS.Prefix, false)
 	case esv1beta1.AWSServiceParameterStore:
 		return parameterstore.New(sess, cfg, storeSpec.Provider.AWS.Prefix, false)
 	}

pkg/provider/aws/secretsmanager/secretsmanager.go

@@ -60,6 +60,7 @@ type SecretsManager struct {
 	referentAuth bool
 	cache        map[string]*awssm.GetSecretValueOutput
 	config       *esv1beta1.SecretsManager
+	prefix       string
 }
 
 // SMInterface is a subset of the smiface api.
@@ -85,13 +86,14 @@ const (
 var log = ctrl.Log.WithName("provider").WithName("aws").WithName("secretsmanager")
 
 // New creates a new SecretsManager client.
-func New(sess *session.Session, cfg *aws.Config, secretsManagerCfg *esv1beta1.SecretsManager, referentAuth bool) (*SecretsManager, error) {
+func New(sess *session.Session, cfg *aws.Config, secretsManagerCfg *esv1beta1.SecretsManager, prefix string, referentAuth bool) (*SecretsManager, error) {
 	return &SecretsManager{
 		sess:         sess,
 		client:       awssm.New(sess, cfg),
 		referentAuth: referentAuth,
 		cache:        make(map[string]*awssm.GetSecretValueOutput),
 		config:       secretsManagerCfg,
+		prefix:       prefix,
 	}, nil
 }
 
@@ -105,15 +107,16 @@ func (sm *SecretsManager) fetch(ctx context.Context, ref esv1beta1.ExternalSecre
 		valueFrom = "TAG"
 	}
 
-	log.Info("fetching secret value", "key", ref.Key, "version", ver, "value", valueFrom)
+	key := sm.prefix + ref.Key
+	log.Info("fetching secret value", "key", key, "version", ver, "value", valueFrom)
 
-	cacheKey := fmt.Sprintf("%s#%s#%s", ref.Key, ver, valueFrom)
+	cacheKey := fmt.Sprintf("%s#%s#%s", key, ver, valueFrom)
 	if secretOut, found := sm.cache[cacheKey]; found {
-		log.Info("found secret in cache", "key", ref.Key, "version", ver)
+		log.Info("found secret in cache", "key", key, "version", ver)
 		return secretOut, nil
 	}
 
-	secretOut, err := sm.constructSecretValue(ctx, ref, ver)
+	secretOut, err := sm.constructSecretValue(ctx, key, ver, ref.MetadataPolicy)
 	if err != nil {
 		return nil, err
 	}
@@ -124,7 +127,7 @@ func (sm *SecretsManager) fetch(ctx context.Context, ref esv1beta1.ExternalSecre
 }
 
 func (sm *SecretsManager) DeleteSecret(ctx context.Context, remoteRef esv1beta1.PushSecretRemoteRef) error {
-	secretName := remoteRef.GetRemoteKey()
+	secretName := sm.prefix + remoteRef.GetRemoteKey()
 	secretValue := awssm.GetSecretValueInput{
 		SecretId: &secretName,
 	}
@@ -170,7 +173,7 @@ func (sm *SecretsManager) DeleteSecret(ctx context.Context, remoteRef esv1beta1.
 }
 
 func (sm *SecretsManager) SecretExists(ctx context.Context, pushSecretRef esv1beta1.PushSecretRemoteRef) (bool, error) {
-	secretName := pushSecretRef.GetRemoteKey()
+	secretName := sm.prefix + pushSecretRef.GetRemoteKey()
 	secretValue := awssm.GetSecretValueInput{
 		SecretId: &secretName,
 	}
@@ -198,7 +201,7 @@ func (sm *SecretsManager) PushSecret(ctx context.Context, secret *corev1.Secret,
 		return fmt.Errorf("failed to extract secret data: %w", err)
 	}
 
-	secretName := psd.GetRemoteKey()
+	secretName := sm.prefix + psd.GetRemoteKey()
 	secretValue := awssm.GetSecretValueInput{
 		SecretId: &secretName,
 	}
@@ -591,17 +594,17 @@ func (sm *SecretsManager) setSecretValues(secret *awssm.SecretValueEntry, data m
 	}
 }
 
-func (sm *SecretsManager) constructSecretValue(ctx context.Context, ref esv1beta1.ExternalSecretDataRemoteRef, ver string) (*awssm.GetSecretValueOutput, error) {
-	if ref.MetadataPolicy == esv1beta1.ExternalSecretMetadataPolicyFetch {
+func (sm *SecretsManager) constructSecretValue(ctx context.Context, key, ver string, metadataPolicy esv1beta1.ExternalSecretMetadataPolicy) (*awssm.GetSecretValueOutput, error) {
+	if metadataPolicy == esv1beta1.ExternalSecretMetadataPolicyFetch {
 		describeSecretInput := &awssm.DescribeSecretInput{
-			SecretId: &ref.Key,
+			SecretId: &key,
 		}
 
 		descOutput, err := sm.client.DescribeSecretWithContext(ctx, describeSecretInput)
 		if err != nil {
 			return nil, err
 		}
-		log.Info("found metadata secret", "key", ref.Key, "output", descOutput)
+		log.Info("found metadata secret", "key", key, "output", descOutput)
 
 		jsonTags, err := util.SecretTagsToJSONString(descOutput.Tags)
 		if err != nil {
@@ -620,12 +623,12 @@ func (sm *SecretsManager) constructSecretValue(ctx context.Context, ref esv1beta
 	if strings.HasPrefix(ver, "uuid/") {
 		versionID := strings.TrimPrefix(ver, "uuid/")
 		getSecretValueInput = &awssm.GetSecretValueInput{
-			SecretId:  &ref.Key,
+			SecretId:  &key,
 			VersionId: &versionID,
 		}
 	} else {
 		getSecretValueInput = &awssm.GetSecretValueInput{
-			SecretId:     &ref.Key,
+			SecretId:     &key,
 			VersionStage: &ver,
 		}
 	}

pkg/provider/aws/secretsmanager/secretsmanager_test.go

@@ -54,6 +54,7 @@ type secretsManagerTestCase struct {
 	expectedData map[string][]byte
 	// for testing caching
 	expectedCounter *int
+	prefix          string
 }
 
 const unexpectedErrorString = "[%d] unexpected error: %s, expected: '%s'"
@@ -126,6 +127,16 @@ func TestSecretsManagerGetSecret(t *testing.T) {
 		smtc.expectedSecret = "testtesttest"
 	}
 
+	// good case: key is passed in with prefix
+	setSecretStringWithPrefix := func(smtc *secretsManagerTestCase) {
+		smtc.remoteRef.Key = "secret-key"
+		smtc.apiInput = &awssm.GetSecretValueInput{
+			SecretId:     aws.String("my-prefix/secret-key"),
+			VersionStage: aws.String("AWSCURRENT"),
+		}
+		smtc.prefix = "my-prefix/"
+	}
+
 	// good case: extract property
 	// Testing that the property exists in the SecretString
 	setRemoteRefPropertyExistsInKey := func(smtc *secretsManagerTestCase) {
@@ -231,6 +242,7 @@ func TestSecretsManagerGetSecret(t *testing.T) {
 	successCases := []*secretsManagerTestCase{
 		makeValidSecretsManagerTestCase(),
 		makeValidSecretsManagerTestCaseCustom(setSecretString),
+		makeValidSecretsManagerTestCaseCustom(setSecretStringWithPrefix),
 		makeValidSecretsManagerTestCaseCustom(setRemoteRefPropertyExistsInKey),
 		makeValidSecretsManagerTestCaseCustom(setRemoteRefMissingProperty),
 		makeValidSecretsManagerTestCaseCustom(setRemoteRefMissingPropertyInvalidJSON),
@@ -250,6 +262,7 @@ func TestSecretsManagerGetSecret(t *testing.T) {
 		sm := SecretsManager{
 			cache:  make(map[string]*awssm.GetSecretValueOutput),
 			client: v.fakeClient,
+			prefix: v.prefix,
 		}
 		out, err := sm.GetSecret(context.Background(), *v.remoteRef)
 		if !ErrorContains(err, v.expectError) {
@@ -790,13 +803,40 @@ func TestSetSecret(t *testing.T) {
 				err: errors.New("secret not managed by external-secrets"),
 			},
 		},
+		"SetSecretWithPrefix": {
+			reason: "secret key is properly prefixed when creating a new secret",
+			args: args{
+
+				store: &esv1beta1.AWSProvider{
+					Service: esv1beta1.AWSServiceSecretsManager,
+					Region:  "eu-west-2",
+					Prefix:  "prefix-",
+				},
+				client: fakesm.Client{
+					GetSecretValueWithContextFn: fakesm.NewGetSecretValueWithContextFn(blankSecretValueOutput, &getSecretCorrectErr),
+					CreateSecretWithContextFn: func(ctx aws.Context, input *awssm.CreateSecretInput, opts ...request.Option) (*awssm.CreateSecretOutput, error) {
+						// Verify that the input name has the prefix applied
+						if *input.Name != "prefix-"+fakeKey {
+							return nil, fmt.Errorf("expected secret name to be prefixed with 'prefix-', got %s", *input.Name)
+						}
+						return secretOutput, nil
+					},
+				},
+				pushSecretData: pushSecretDataWithoutProperty,
+			},
+			want: want{
+				err: nil,
+			},
+		},
 	}
 
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
 			sm := SecretsManager{
 				client: &tc.args.client,
+				prefix: tc.args.store.Prefix,
 			}
+
 			err := sm.PushSecret(context.Background(), fakeSecret, tc.args.pushSecretData)
 
 			// Error nil XOR tc.want.err nil
@@ -825,6 +865,7 @@ func TestDeleteSecret(t *testing.T) {
 	type args struct {
 		client               fakesm.Client
 		config               esv1beta1.SecretsManager
+		prefix               string
 		getSecretOutput      *awssm.GetSecretValueOutput
 		describeSecretOutput *awssm.DescribeSecretOutput
 		deleteSecretOutput   *awssm.DeleteSecretOutput
@@ -1007,6 +1048,43 @@ func TestDeleteSecret(t *testing.T) {
 				err: errors.New("timeout"),
 			},
 		},
+		"DeleteWithPrefix": {
+			args: args{
+				client: fakesm.Client{
+					GetSecretValueWithContextFn: func(ctx aws.Context, input *awssm.GetSecretValueInput, opts ...request.Option) (*awssm.GetSecretValueOutput, error) {
+						// Verify that the input secret ID has the prefix applied
+						if *input.SecretId != "my-prefix-"+fakeKey {
+							return nil, fmt.Errorf("expected secret name to be prefixed with 'my-prefix-', got %s", *input.SecretId)
+						}
+						return &awssm.GetSecretValueOutput{}, nil
+					},
+					DescribeSecretWithContextFn: func(ctx aws.Context, input *awssm.DescribeSecretInput, opts ...request.Option) (*awssm.DescribeSecretOutput, error) {
+						// Verify that the input secret ID has the prefix applied
+						if *input.SecretId != "my-prefix-"+fakeKey {
+							return nil, fmt.Errorf("expected secret name to be prefixed with 'my-prefix-', got %s", *input.SecretId)
+						}
+						return &awssm.DescribeSecretOutput{
+							Tags: []*awssm.Tag{&secretTag},
+						}, nil
+					},
+					DeleteSecretWithContextFn: func(ctx aws.Context, input *awssm.DeleteSecretInput, opts ...request.Option) (*awssm.DeleteSecretOutput, error) {
+						return &awssm.DeleteSecretOutput{}, nil
+					},
+				},
+				config:               esv1beta1.SecretsManager{},
+				prefix:               "my-prefix-",
+				getSecretOutput:      nil,
+				describeSecretOutput: nil,
+				deleteSecretOutput:   nil,
+				getSecretErr:         nil,
+				describeSecretErr:    nil,
+				deleteSecretErr:      nil,
+			},
+			want: want{
+				err: nil,
+			},
+			reason: "Verifies that the prefix is correctly applied when deleting a secret",
+		},
 	}
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
@@ -1014,10 +1092,19 @@ func TestDeleteSecret(t *testing.T) {
 			sm := SecretsManager{
 				client: &tc.args.client,
 				config: &tc.args.config,
+				prefix: tc.args.prefix,
 			}
-			tc.args.client.GetSecretValueWithContextFn = fakesm.NewGetSecretValueWithContextFn(tc.args.getSecretOutput, tc.args.getSecretErr)
-			tc.args.client.DescribeSecretWithContextFn = fakesm.NewDescribeSecretWithContextFn(tc.args.describeSecretOutput, tc.args.describeSecretErr)
-			tc.args.client.DeleteSecretWithContextFn = fakesm.NewDeleteSecretWithContextFn(tc.args.deleteSecretOutput, tc.args.deleteSecretErr)
+
+			if tc.args.client.GetSecretValueWithContextFn == nil {
+				tc.args.client.GetSecretValueWithContextFn = fakesm.NewGetSecretValueWithContextFn(tc.args.getSecretOutput, tc.args.getSecretErr)
+			}
+			if tc.args.client.DescribeSecretWithContextFn == nil {
+				tc.args.client.DescribeSecretWithContextFn = fakesm.NewDescribeSecretWithContextFn(tc.args.describeSecretOutput, tc.args.describeSecretErr)
+			}
+			if tc.args.client.DeleteSecretWithContextFn == nil {
+				tc.args.client.DeleteSecretWithContextFn = fakesm.NewDeleteSecretWithContextFn(tc.args.deleteSecretOutput, tc.args.deleteSecretErr)
+			}
+
 			err := sm.DeleteSecret(context.TODO(), ref)
 			t.Logf("DeleteSecret error: %v", err)