chore(aws): parameterstore unit tests improvement (#4986)

* chore(aws): parameterstore unit tests improvement

Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>

* chore(aws): parameterstore unit tests improvement

Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>

---------

Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>

pkg/provider/aws/parameterstore/fake/fake.go

@@ -52,8 +52,13 @@ func (sm *Client) ListTagsForResource(ctx context.Context, input *ssm.ListTagsFo
 	return sm.ListTagsForResourceFn(ctx, input, options...)
 }
 
-func NewListTagsForResourceFn(output *ssm.ListTagsForResourceOutput, err error) ListTagsForResourceFn {
-	return func(context.Context, *ssm.ListTagsForResourceInput, ...func(*ssm.Options)) (*ssm.ListTagsForResourceOutput, error) {
+func NewListTagsForResourceFn(output *ssm.ListTagsForResourceOutput, err error, aFunc ...func(input *ssm.ListTagsForResourceInput)) ListTagsForResourceFn {
+	return func(_ context.Context, params *ssm.ListTagsForResourceInput, _ ...func(*ssm.Options)) (*ssm.ListTagsForResourceOutput, error) {
+		if len(aFunc) > 0 {
+			for _, f := range aFunc {
+				f(params)
+			}
+		}
 		return output, err
 	}
 }
@@ -98,8 +103,13 @@ func (sm *Client) PutParameter(ctx context.Context, input *ssm.PutParameterInput
 	return sm.PutParameterFn(ctx, input, options...)
 }
 
-func NewPutParameterFn(output *ssm.PutParameterOutput, err error) PutParameterFn {
-	return func(context.Context, *ssm.PutParameterInput, ...func(*ssm.Options)) (*ssm.PutParameterOutput, error) {
+func NewPutParameterFn(output *ssm.PutParameterOutput, err error, aFunc ...func(input *ssm.PutParameterInput)) PutParameterFn {
+	return func(_ context.Context, params *ssm.PutParameterInput, _ ...func(*ssm.Options)) (*ssm.PutParameterOutput, error) {
+		if len(aFunc) > 0 {
+			for _, f := range aFunc {
+				f(params)
+			}
+		}
 		return output, err
 	}
 }
@@ -113,22 +123,32 @@ func (sm *Client) WithValue(in *ssm.GetParameterInput, val *ssm.GetParameterOutp
 	}
 }
 
-func (sm *Client) RemoveTagsFromResource(_ context.Context, params *ssm.RemoveTagsFromResourceInput, optFns ...func(*ssm.Options)) (*ssm.RemoveTagsFromResourceOutput, error) {
-	return nil, errors.New("RemoveTagsFromResource is not implemented in fake client")
+func (sm *Client) RemoveTagsFromResource(ctx context.Context, params *ssm.RemoveTagsFromResourceInput, optFns ...func(*ssm.Options)) (*ssm.RemoveTagsFromResourceOutput, error) {
+	return sm.RemoveTagsFromResourceFn(ctx, params, optFns...)
 }
 
-func NewRemoveTagsFromResourceFn(output *ssm.RemoveTagsFromResourceOutput, err error) RemoveTagsFromResourceFn {
+func NewRemoveTagsFromResourceFn(output *ssm.RemoveTagsFromResourceOutput, err error, aFunc ...func(input *ssm.RemoveTagsFromResourceInput)) RemoveTagsFromResourceFn {
 	return func(ctx context.Context, params *ssm.RemoveTagsFromResourceInput, optFns ...func(*ssm.Options)) (*ssm.RemoveTagsFromResourceOutput, error) {
+		if len(aFunc) > 0 {
+			for _, f := range aFunc {
+				f(params)
+			}
+		}
 		return output, err
 	}
 }
 
-func (sm *Client) AddTagsToResource(_ context.Context, params *ssm.AddTagsToResourceInput, optFns ...func(*ssm.Options)) (*ssm.AddTagsToResourceOutput, error) {
-	return nil, errors.New("AddTagsToResource is not implemented in fake client")
+func (sm *Client) AddTagsToResource(ctx context.Context, params *ssm.AddTagsToResourceInput, optFns ...func(*ssm.Options)) (*ssm.AddTagsToResourceOutput, error) {
+	return sm.AddTagsToResourceFn(ctx, params, optFns...)
 }
 
-func NewAddTagsToResourceFn(output *ssm.AddTagsToResourceOutput, err error) AddTagsToResourceFn {
+func NewAddTagsToResourceFn(output *ssm.AddTagsToResourceOutput, err error, aFunc ...func(input *ssm.AddTagsToResourceInput)) AddTagsToResourceFn {
 	return func(ctx context.Context, params *ssm.AddTagsToResourceInput, optFns ...func(*ssm.Options)) (*ssm.AddTagsToResourceOutput, error) {
+		if len(aFunc) > 0 {
+			for _, f := range aFunc {
+				f(params)
+			}
+		}
 		return output, err
 	}
 }

pkg/provider/aws/parameterstore/parameterstore.go

@@ -302,7 +302,7 @@ func (pm *ParameterStore) setExisting(ctx context.Context, existing *ssm.GetPara
 		return err
 	}
 
-	tagKeysToRemove := findTagKeysToRemove(tags, metaTags)
+	tagKeysToRemove := util.FindTagKeysToRemove(tags, metaTags)
 	if len(tagKeysToRemove) > 0 {
 		_, err = pm.client.RemoveTagsFromResource(ctx, &ssm.RemoveTagsFromResourceInput{
 			ResourceId:   existing.Parameter.Name,
@@ -678,19 +678,6 @@ func (pm *ParameterStore) constructMetadataWithDefaults(data *apiextensionsv1.JS
 	return meta, nil
 }
 
-// findTagKeysToRemove returns a slice of tag keys that exist in the current tags
-// but are not present in the desired metaTags. These keys should be removed to
-// synchronize the tags with the desired state.
-func findTagKeysToRemove(tags, metaTags map[string]string) []string {
-	var diff []string
-	for key, _ := range tags {
-		if _, ok := metaTags[key]; !ok {
-			diff = append(diff, key)
-		}
-	}
-	return diff
-}
-
 // computeTagsToUpdate compares the current tags with the desired metaTags and returns a slice of ssmTypes.Tag
 // that should be set on the resource. It also returns a boolean indicating if any tag was added or modified.
 func computeTagsToUpdate(tags, metaTags map[string]string) ([]ssmTypes.Tag, bool) {

pkg/provider/aws/parameterstore/parameterstore_test.go

@@ -17,7 +17,6 @@ package parameterstore
 import (
 	"context"
 	"errors"
-	"fmt"
 	"strings"
 	"testing"
 
@@ -342,10 +341,15 @@ func TestPushSecret(t *testing.T) {
 			args: args{
 				store: makeValidParameterStore().Spec.Provider.AWS,
 				client: fakeps.Client{
-					PutParameterFn:           fakeps.NewPutParameterFn(putParameterOutput, nil),
-					GetParameterFn:           fakeps.NewGetParameterFn(getParameterOutput, nil),
-					DescribeParametersFn:     fakeps.NewDescribeParametersFn(describeParameterOutput, nil),
-					ListTagsForResourceFn:    fakeps.NewListTagsForResourceFn(validListTagsForResourceOutput, nil),
+					PutParameterFn: fakeps.NewPutParameterFn(putParameterOutput, nil, func(input *ssm.PutParameterInput) {
+						assert.Len(t, input.Tags, 1)
+						assert.Contains(t, input.Tags, managedByESO)
+					}),
+					GetParameterFn:       fakeps.NewGetParameterFn(getParameterOutput, nil),
+					DescribeParametersFn: fakeps.NewDescribeParametersFn(describeParameterOutput, nil),
+					ListTagsForResourceFn: fakeps.NewListTagsForResourceFn(validListTagsForResourceOutput, nil, func(input *ssm.ListTagsForResourceInput) {
+						assert.Equal(t, "/external-secrets/parameters/fake-key", input.ResourceId)
+					}),
 					RemoveTagsFromResourceFn: fakeps.NewRemoveTagsFromResourceFn(&ssm.RemoveTagsFromResourceOutput{}, nil),
 					AddTagsToResourceFn:      fakeps.NewAddTagsToResourceFn(&ssm.AddTagsToResourceOutput{}, nil),
 				},
@@ -591,7 +595,7 @@ func TestPushSecret(t *testing.T) {
 				err: errors.New("unable to compare 'sensitive' result, ensure to request a decrypted value"),
 			},
 		},
-		"SecretWithTags": {
+		"SecretPatchTags": {
 			reason: "test if we can configure tags for the secret",
 			args: args{
 				store: makeValidParameterStore().Spec.Provider.AWS,
@@ -601,18 +605,38 @@ func TestPushSecret(t *testing.T) {
 						"kind": "PushSecretMetadata",
 						"spec": {
 							"tags": {
-								"tagname1": "value1"
+								"env": "sandbox",
+								"rotation": "1h"
 							},
 						}
 					}`),
 				},
 				client: fakeps.Client{
-					PutParameterFn:       fakeps.NewPutParameterFn(putParameterOutput, nil),
-					GetParameterFn:       fakeps.NewGetParameterFn(sameGetParameterOutput, nil),
-					DescribeParametersFn: fakeps.NewDescribeParametersFn(describeParameterOutput, nil),
+					PutParameterFn: fakeps.NewPutParameterFn(putParameterOutput, nil, func(input *ssm.PutParameterInput) {
+						assert.Len(t, input.Tags, 0)
+					}),
+					GetParameterFn: fakeps.NewGetParameterFn(&ssm.GetParameterOutput{
+						Parameter: &ssmtypes.Parameter{
+							Value: aws.String("some-value"),
+						},
+					}, nil),
+					DescribeParametersFn: fakeps.NewDescribeParametersFn(&ssm.DescribeParametersOutput{}, nil),
 					ListTagsForResourceFn: fakeps.NewListTagsForResourceFn(&ssm.ListTagsForResourceOutput{
-						TagList: []ssmtypes.Tag{managedByESO, {Key: ptr.To("tagname1"), Value: ptr.To("value1")}},
+						TagList: []ssmtypes.Tag{managedByESO,
+							{Key: ptr.To("team"), Value: ptr.To("no-longer-needed")},
+							{Key: ptr.To("rotation"), Value: ptr.To("10m")},
+						},
 					}, nil),
+					RemoveTagsFromResourceFn: fakeps.NewRemoveTagsFromResourceFn(&ssm.RemoveTagsFromResourceOutput{}, nil, func(input *ssm.RemoveTagsFromResourceInput) {
+						assert.Len(t, input.TagKeys, 1)
+						assert.Equal(t, []string{"team"}, input.TagKeys)
+					}),
+					AddTagsToResourceFn: fakeps.NewAddTagsToResourceFn(&ssm.AddTagsToResourceOutput{}, nil, func(input *ssm.AddTagsToResourceInput) {
+						assert.Len(t, input.Tags, 3)
+						assert.Contains(t, input.Tags, ssmtypes.Tag{Key: &managedBy, Value: &externalSecrets})
+						assert.Contains(t, input.Tags, ssmtypes.Tag{Key: ptr.To("env"), Value: ptr.To("sandbox")})
+						assert.Contains(t, input.Tags, ssmtypes.Tag{Key: ptr.To("rotation"), Value: ptr.To("1h")})
+					}),
 				},
 			},
 			want: want{
@@ -624,7 +648,6 @@ func TestPushSecret(t *testing.T) {
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
 			psd := fake.PushSecretData{SecretKey: fakeSecretKey, RemoteKey: remoteKey}
-			fmt.Println("line 635:", psd, tc.args.metadata)
 			if tc.args.metadata != nil {
 				psd.Metadata = tc.args.metadata
 			}
@@ -633,8 +656,6 @@ func TestPushSecret(t *testing.T) {
 			}
 			err := ps.PushSecret(context.TODO(), fakeSecret, psd)
 
-			fmt.Println("line 644:", err)
-
 			// Error nil XOR tc.want.err nil
 			if ((err == nil) || (tc.want.err == nil)) && !((err == nil) && (tc.want.err == nil)) {
 				t.Errorf("\nTesting SetSecret:\nName: %v\nReason: %v\nWant error: %v\nGot error: %v", name, tc.reason, tc.want.err, err)
@@ -1184,71 +1205,6 @@ func TestConstructMetadataWithDefaults(t *testing.T) {
 	}
 }
 
-func TestFindTagKeysToRemove(t *testing.T) {
-	tests := []struct {
-		name     string
-		tags     map[string]string
-		metaTags map[string]string
-		expected []string
-	}{
-		{
-			name: "No tags to remove",
-			tags: map[string]string{
-				"key1": "value1",
-				"key2": "value2",
-			},
-			metaTags: map[string]string{
-				"key1": "value1",
-				"key2": "value2",
-			},
-			expected: []string{},
-		},
-		{
-			name: "Some tags to remove",
-			tags: map[string]string{
-				"key1": "value1",
-				"key2": "value2",
-				"key3": "value3",
-			},
-			metaTags: map[string]string{
-				"key1": "value1",
-				"key2": "value2",
-			},
-			expected: []string{"key3"},
-		},
-		{
-			name: "All tags to remove",
-			tags: map[string]string{
-				"key1": "value1",
-				"key2": "value2",
-			},
-			metaTags: map[string]string{},
-			expected: []string{"key1", "key2"},
-		},
-		{
-			name:     "Empty tags and metaTags",
-			tags:     map[string]string{},
-			metaTags: map[string]string{},
-			expected: []string{},
-		},
-		{
-			name: "Empty metaTags with non-empty tags",
-			tags: map[string]string{
-				"key1": "value1",
-			},
-			metaTags: map[string]string{},
-			expected: []string{"key1"},
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			result := findTagKeysToRemove(tt.tags, tt.metaTags)
-			assert.ElementsMatch(t, tt.expected, result)
-		})
-	}
-}
-
 func TestComputeTagsToUpdate(t *testing.T) {
 	tests := []struct {
 		name     string

pkg/provider/aws/util/provider.go

@@ -86,3 +86,16 @@ func ParameterTagsToJSONString(tags map[string]string) (string, error) {
 
 	return string(byteArr), nil
 }
+
+// FindTagKeysToRemove returns a slice of tag keys that exist in the current tags
+// but are not present in the desired metaTags. These keys should be removed to
+// synchronize the tags with the desired state.
+func FindTagKeysToRemove(tags, metaTags map[string]string) []string {
+	var diff []string
+	for key, _ := range tags {
+		if _, ok := metaTags[key]; !ok {
+			diff = append(diff, key)
+		}
+	}
+	return diff
+}

pkg/provider/aws/util/provider_test.go

@@ -66,3 +66,68 @@ func TestParameterTagsToJSONString(t *testing.T) {
 		})
 	}
 }
+
+func TestFindTagKeysToRemove(t *testing.T) {
+	tests := []struct {
+		name     string
+		tags     map[string]string
+		metaTags map[string]string
+		expected []string
+	}{
+		{
+			name: "No tags to remove",
+			tags: map[string]string{
+				"key1": "value1",
+				"key2": "value2",
+			},
+			metaTags: map[string]string{
+				"key1": "value1",
+				"key2": "value2",
+			},
+			expected: []string{},
+		},
+		{
+			name: "Some tags to remove",
+			tags: map[string]string{
+				"key1": "value1",
+				"key2": "value2",
+				"key3": "value3",
+			},
+			metaTags: map[string]string{
+				"key1": "value1",
+				"key2": "value2",
+			},
+			expected: []string{"key3"},
+		},
+		{
+			name: "All tags to remove",
+			tags: map[string]string{
+				"key1": "value1",
+				"key2": "value2",
+			},
+			metaTags: map[string]string{},
+			expected: []string{"key1", "key2"},
+		},
+		{
+			name:     "Empty tags and metaTags",
+			tags:     map[string]string{},
+			metaTags: map[string]string{},
+			expected: []string{},
+		},
+		{
+			name: "Empty metaTags with non-empty tags",
+			tags: map[string]string{
+				"key1": "value1",
+			},
+			metaTags: map[string]string{},
+			expected: []string{"key1"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := FindTagKeysToRemove(tt.tags, tt.metaTags)
+			assert.ElementsMatch(t, tt.expected, result)
+		})
+	}
+}