Added unit tests for SetSecret AWS parameter store

Signed-off-by: Marcus Dantas <marcus.dantas@engineerbetter.com>
Co-authored-by: Nick Ruffles <nick.ruffles@engineerbetter.com>
Co-authored-by: Gustavo Carvalho <gustavo.carvalho@container-solutions.com>

pkg/provider/aws/parameterstore/parameterstore.go

@@ -111,8 +111,7 @@ func (pm *ParameterStore) SetSecret(ctx context.Context, value []byte, remoteRef
 	var awsError awserr.Error
 	ok := errors.As(err, &awsError)
 	if err != nil && (!ok || awsError.Code() != ssm.ErrCodeParameterNotFound) {
-		// TODO: give some more context to the error
-		return err
+		return fmt.Errorf("unexpected error getting parameter %v: %w", secretName, err)
 	}
 
 	// If we have a valid parameter returned to us, check its tags
@@ -120,7 +119,7 @@ func (pm *ParameterStore) SetSecret(ctx context.Context, value []byte, remoteRef
 		fmt.Println("The existing value contains data:", existing.String())
 		tags, err := pm.getTagsByName(ctx, existing)
 		if err != nil {
-			return fmt.Errorf("error getting the existing tags for the parameter: %w", err)
+			return fmt.Errorf("error getting the existing tags for the parameter %v: %w", secretName, err)
 		}
 
 		isManaged := isManagedByESO(tags)
@@ -165,7 +164,10 @@ func (pm *ParameterStore) setManagedRemoteParameter(ctx context.Context, secretR
 	}
 
 	_, err := pm.client.PutParameterWithContext(ctx, &secretRequest)
-	return err
+	if err != nil {
+		return fmt.Errorf("unexpected error pushing parameter %v: %w", secretRequest.Name, err)
+	}
+	return nil
 }
 
 // GetAllSecrets fetches information from multiple secrets into a single kubernetes secret.

pkg/provider/aws/parameterstore/parameterstore_test.go

@@ -22,7 +22,6 @@ import (
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ssm"
-	"github.com/crossplane/crossplane-runtime/pkg/test"
 	"github.com/google/go-cmp/cmp"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
@@ -95,6 +94,7 @@ func makeValidParameterStoreTestCaseCustom(tweaks ...func(pstc *parameterstoreTe
 func TestPushSecret(t *testing.T) {
 	invalidParameters := errors.New(ssm.ErrCodeInvalidParameters)
 	alreadyExistsError := errors.New(ssm.ErrCodeAlreadyExistsException)
+	fakeValue := "fakeValue"
 
 	managedByESO := ssm.Tag{
 		Key:   &managedBy,
@@ -107,6 +107,7 @@ func TestPushSecret(t *testing.T) {
 	validListTagsForResourceOutput := &ssm.ListTagsForResourceOutput{
 		TagList: []*ssm.Tag{&managedByESO},
 	}
+	noTagsResourceOutput := &ssm.ListTagsForResourceOutput{}
 
 	validGetParameterOutput := &ssm.GetParameterOutput{
 		Parameter: &ssm.Parameter{
@@ -122,6 +123,12 @@ func TestPushSecret(t *testing.T) {
 		},
 	}
 
+	sameGetParameterOutput := &ssm.GetParameterOutput{
+		Parameter: &ssm.Parameter{
+			Value: &fakeValue,
+		},
+	}
+
 	type args struct {
 		store  *esv1beta1.AWSProvider
 		client fakeps.Client
@@ -196,6 +203,51 @@ func TestPushSecret(t *testing.T) {
 				err: nil,
 			},
 		},
+		"SetSecretNotManagedByESO": {
+			reason: "SetSecret to the parameter store but tags are not managed by ESO",
+			args: args{
+				store: makeValidParameterStore().Spec.Provider.AWS,
+				client: fakeps.Client{
+					PutParameterWithContextFn:        fakeps.NewPutParameterWithContextFn(putParameterOutput, nil),
+					GetParameterWithContextFn:        fakeps.NewGetParameterWithContextFn(validGetParameterOutput, nil),
+					DescribeParametersWithContextFn:  fakeps.NewDescribeParametersWithContextFn(describeParameterOutput, nil),
+					ListTagsForResourceWithContextFn: fakeps.NewListTagsForResourceWithContextFn(noTagsResourceOutput, nil),
+				},
+			},
+			want: want{
+				err: fmt.Errorf("secret not managed by external-secrets"),
+			},
+		},
+		"SetSecretGetTagsError": {
+			reason: "SetSecret to the parameter store returns error while obtaining tags",
+			args: args{
+				store: makeValidParameterStore().Spec.Provider.AWS,
+				client: fakeps.Client{
+					PutParameterWithContextFn:        fakeps.NewPutParameterWithContextFn(putParameterOutput, nil),
+					GetParameterWithContextFn:        fakeps.NewGetParameterWithContextFn(validGetParameterOutput, nil),
+					DescribeParametersWithContextFn:  fakeps.NewDescribeParametersWithContextFn(describeParameterOutput, nil),
+					ListTagsForResourceWithContextFn: fakeps.NewListTagsForResourceWithContextFn(nil, fmt.Errorf("you shall not tag")),
+				},
+			},
+			want: want{
+				err: fmt.Errorf("you shall not tag"),
+			},
+		},
+		"SetSecretContentMatches": {
+			reason: "No ops",
+			args: args{
+				store: makeValidParameterStore().Spec.Provider.AWS,
+				client: fakeps.Client{
+					PutParameterWithContextFn:        fakeps.NewPutParameterWithContextFn(putParameterOutput, nil),
+					GetParameterWithContextFn:        fakeps.NewGetParameterWithContextFn(sameGetParameterOutput, nil),
+					DescribeParametersWithContextFn:  fakeps.NewDescribeParametersWithContextFn(describeParameterOutput, nil),
+					ListTagsForResourceWithContextFn: fakeps.NewListTagsForResourceWithContextFn(validListTagsForResourceOutput, nil),
+				},
+			},
+			want: want{
+				err: nil,
+			},
+		},
 	}
 	//
 	for name, tc := range tests {
@@ -204,10 +256,18 @@ func TestPushSecret(t *testing.T) {
 			ps := ParameterStore{
 				client: &tc.args.client,
 			}
-			err := ps.SetSecret(context.TODO(), []byte("fakeValue"), ref)
+			err := ps.SetSecret(context.TODO(), []byte(fakeValue), ref)
+
+			// 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)
+			}
 
-			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)
+			// if errors are the same type but their contents do not match
+			if err != nil && tc.want.err != nil {
+				if !strings.Contains(err.Error(), tc.want.err.Error()) {
+					t.Errorf("\nTesting SetSecret:\nName: %v\nReason: %v\nWant error: %v\nGot error got nil", name, tc.reason, tc.want.err)
+				}
 			}
 		})
 	}