fix: parameter store should be called only once

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
external-secrets · Jun 14, 2024 · 5f9122c · 5f9122c
1 parent d29c001
commit 5f9122c
Show file tree

Hide file tree

Showing 4 changed files with 53 additions and 20 deletions.
diff --git a/pkg/provider/aws/parameterstore/fake/fake.go b/pkg/provider/aws/parameterstore/fake/fake.go
@@ -29,6 +29,7 @@ type Client struct {
 	GetParameterWithContextFn        GetParameterWithContextFn
 	GetParametersByPathWithContextFn GetParametersByPathWithContextFn
 	PutParameterWithContextFn        PutParameterWithContextFn
+	PutParameterWithContextCalledN   int
 	DeleteParameterWithContextFn     DeleteParameterWithContextFn
 	DescribeParametersWithContextFn  DescribeParametersWithContextFn
 	ListTagsForResourceWithContextFn ListTagsForResourceWithContextFn
@@ -86,6 +87,7 @@ func NewDescribeParametersWithContextFn(output *ssm.DescribeParametersOutput, er
 }
 
 func (sm *Client) PutParameterWithContext(ctx aws.Context, input *ssm.PutParameterInput, options ...request.Option) (*ssm.PutParameterOutput, error) {
+	sm.PutParameterWithContextCalledN++
 	return sm.PutParameterWithContextFn(ctx, input, options...)
 }
 

diff --git a/pkg/provider/aws/parameterstore/parameterstore.go b/pkg/provider/aws/parameterstore/parameterstore.go
@@ -41,12 +41,10 @@ import (
 
 // Declares metadata information for pushing secrets to AWS Parameter Store.
 const (
-	PushSecretType                 = "parameterStoreType"
-	ParameterStoreTypeString       = "String"
-	ParameterStoreTypeStringList   = "StringList"
-	ParameterStoreTypeSecureString = "SecureString"
-	ParameterStoreKeyID            = "parameterStoreKeyID"
-	PushSecretKeyID                = "keyID"
+	PushSecretType  = "parameterStoreType"
+	StoreTypeString = "String"
+	StoreKeyID      = "parameterStoreKeyID"
+	PushSecretKeyID = "keyID"
 )
 
 // https://github.com/external-secrets/external-secrets/issues/644
@@ -153,12 +151,12 @@ func (pm *ParameterStore) PushSecret(ctx context.Context, secret *corev1.Secret,
 		err   error
 	)
 
-	parameterTypeFormat, err := utils.FetchValueFromMetadata(PushSecretType, data.GetMetadata(), ParameterStoreTypeString)
+	parameterTypeFormat, err := utils.FetchValueFromMetadata(PushSecretType, data.GetMetadata(), StoreTypeString)
 	if err != nil {
 		return fmt.Errorf("failed to parse metadata: %w", err)
 	}
 
-	parameterKeyIDFormat, err := utils.FetchValueFromMetadata(ParameterStoreKeyID, data.GetMetadata(), PushSecretKeyID)
+	parameterKeyIDFormat, err := utils.FetchValueFromMetadata(StoreKeyID, data.GetMetadata(), PushSecretKeyID)
 	if err != nil {
 		return fmt.Errorf("failed to parse metadata: %w", err)
 	}
@@ -208,7 +206,6 @@ func (pm *ParameterStore) PushSecret(ctx context.Context, secret *corev1.Secret,
 
 	// If we have a valid parameter returned to us, check its tags
 	if existing != nil && existing.Parameter != nil {
-		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 %v: %w", secretName, err)

diff --git a/pkg/provider/aws/parameterstore/parameterstore_test.go b/pkg/provider/aws/parameterstore/parameterstore_test.go
@@ -25,6 +25,8 @@ import (
 	"github.com/aws/aws-sdk-go/aws/awserr"
 	"github.com/aws/aws-sdk-go/service/ssm"
 	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -513,6 +515,48 @@ func TestPushSecret(t *testing.T) {
 	}
 }
 
+func TestPushSecretCalledOnlyOnce(t *testing.T) {
+	fakeSecretKey := "fakeSecretKey"
+	fakeValue := "fakeValue"
+	fakeSecret := &corev1.Secret{
+		Data: map[string][]byte{
+			fakeSecretKey: []byte(fakeValue),
+		},
+	}
+
+	managedByESO := ssm.Tag{
+		Key:   &managedBy,
+		Value: &externalSecrets,
+	}
+
+	putParameterOutput := &ssm.PutParameterOutput{}
+	validGetParameterOutput := &ssm.GetParameterOutput{
+		Parameter: &ssm.Parameter{
+			Value: &fakeValue,
+		},
+	}
+	describeParameterOutput := &ssm.DescribeParametersOutput{}
+	validListTagsForResourceOutput := &ssm.ListTagsForResourceOutput{
+		TagList: []*ssm.Tag{&managedByESO},
+	}
+
+	client := fakeps.Client{
+		PutParameterWithContextFn:        fakeps.NewPutParameterWithContextFn(putParameterOutput, nil),
+		GetParameterWithContextFn:        fakeps.NewGetParameterWithContextFn(validGetParameterOutput, nil),
+		DescribeParametersWithContextFn:  fakeps.NewDescribeParametersWithContextFn(describeParameterOutput, nil),
+		ListTagsForResourceWithContextFn: fakeps.NewListTagsForResourceWithContextFn(validListTagsForResourceOutput, nil),
+	}
+
+	psd := fake.PushSecretData{SecretKey: fakeSecretKey, RemoteKey: "fake-key"}
+	ps := ParameterStore{
+		client: &client,
+	}
+
+	require.NoError(t, ps.PushSecret(context.TODO(), fakeSecret, psd))
+
+	assert.Equal(t, 0, client.PutParameterWithContextCalledN)
+}
+
 // test the ssm<->aws interface
 // make sure correct values are passed and errors are handled accordingly.
 func TestGetSecret(t *testing.T) {

diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go
@@ -512,16 +512,6 @@ func CompareStringAndByteSlices(valueString *string, valueByte []byte) bool {
 	if valueString == nil {
 		return false
 	}
-	stringToByteSlice := []byte(*valueString)
-	if len(stringToByteSlice) != len(valueByte) {
-		return false
-	}
-
-	for sb := range valueByte {
-		if stringToByteSlice[sb] != valueByte[sb] {
-			return false
-		}
-	}
 
-	return true
+	return bytes.Equal(valueByte, []byte(*valueString))
 }