Fix pushing to an AWS Secrets Manager Secret when there are no secret values (#4878)

* Remove unnecessary stubbings

Signed-off-by: Niraj Sapkota <n.sapkota@outlook.com.au>

* Update tests

Signed-off-by: Niraj Sapkota <n.sapkota@outlook.com.au>

* Update implementation

Signed-off-by: Niraj Sapkota <n.sapkota@outlook.com.au>

* Lint

Signed-off-by: Niraj Sapkota <n.sapkota@outlook.com.au>

---------

Signed-off-by: Niraj Sapkota <n.sapkota@outlook.com.au>
Co-authored-by: Gergely Brautigam <skarlso777@gmail.com>

pkg/provider/aws/secretsmanager/secretsmanager.go

@@ -215,39 +215,64 @@ func (sm *SecretsManager) PushSecret(ctx context.Context, secret *corev1.Secret,
 	}
 
 	secretName := sm.prefix + psd.GetRemoteKey()
-	secretValue := awssm.GetSecretValueInput{
-		SecretId: &secretName,
+	describeSecretInput := awssm.DescribeSecretInput{SecretId: &secretName}
+	describeSecretOutput, err := sm.client.DescribeSecret(ctx, &describeSecretInput)
+	metrics.ObserveAPICall(constants.ProviderAWSSM, constants.CallAWSSMDescribeSecret, err)
+	var aerr smithy.APIError
+	if err != nil {
+		if ok := errors.As(err, &aerr); !ok {
+			return err
+		}
+		if aerr.ErrorCode() == ResourceNotFoundException {
+			finalValue, err := sm.getNewSecretValue(value, psd.GetProperty(), nil)
+			if err != nil {
+				return err
+			}
+			return sm.createSecretWithContext(ctx, secretName, psd, finalValue)
+		}
+		return err
+	} else if !isManagedByESO(describeSecretOutput) {
+		return errors.New("secret not managed by external-secrets")
 	}
 
-	secretInput := awssm.DescribeSecretInput{
-		SecretId: &secretName,
+	if len(describeSecretOutput.VersionIdsToStages) == 0 {
+		finalValue, err := sm.getNewSecretValue(value, psd.GetProperty(), nil)
+		if err != nil {
+			return err
+		}
+		return sm.putSecretValueWithContext(ctx, secretName, nil, psd, finalValue, describeSecretOutput.Tags)
 	}
 
-	awsSecret, err := sm.client.GetSecretValue(ctx, &secretValue)
+	getSecretValueInput := awssm.GetSecretValueInput{SecretId: &secretName}
+	getSecretValueOutput, err := sm.client.GetSecretValue(ctx, &getSecretValueInput)
 	metrics.ObserveAPICall(constants.ProviderAWSSM, constants.CallAWSSMGetSecretValue, err)
-
-	if psd.GetProperty() != "" {
-		currentSecret := sm.retrievePayload(awsSecret)
-		if currentSecret != "" && !gjson.Valid(currentSecret) {
-			return errors.New("PushSecret for aws secrets manager with a pushSecretData property requires a json secret")
-		}
-		value, _ = sjson.SetBytes([]byte(currentSecret), psd.GetProperty(), value)
+	if err != nil {
+		return err
 	}
 
-	var aerr smithy.APIError
+	finalValue, err := sm.getNewSecretValue(value, psd.GetProperty(), getSecretValueOutput)
 	if err != nil {
-		if ok := errors.As(err, &aerr); !ok {
-			return err
-		}
+		return err
+	}
+	return sm.putSecretValueWithContext(ctx, secretName, getSecretValueOutput, psd, finalValue, describeSecretOutput.Tags)
+}
 
-		if aerr.ErrorCode() == ResourceNotFoundException {
-			return sm.createSecretWithContext(ctx, secretName, psd, value)
-		}
+func (sm *SecretsManager) getNewSecretValue(value []byte, property string, existingSecret *awssm.GetSecretValueOutput) ([]byte, error) {
+	if property == "" {
+		return value, nil
+	}
 
-		return err
+	if existingSecret == nil {
+		value, _ = sjson.SetBytes([]byte{}, property, value)
+		return value, nil
 	}
 
-	return sm.putSecretValueWithContext(ctx, secretInput, awsSecret, psd, value)
+	currentSecret := sm.retrievePayload(existingSecret)
+	if currentSecret != "" && !gjson.Valid(currentSecret) {
+		return nil, errors.New("PushSecret for aws secrets manager with a pushSecretData property requires a json secret")
+	}
+	value, _ = sjson.SetBytes([]byte(currentSecret), property, value)
+	return value, nil
 }
 
 func padOrTrim(b []byte) []byte {
@@ -543,27 +568,23 @@ func (sm *SecretsManager) createSecretWithContext(ctx context.Context, secretNam
 	return err
 }
 
-func (sm *SecretsManager) putSecretValueWithContext(ctx context.Context, secretInput awssm.DescribeSecretInput, awsSecret *awssm.GetSecretValueOutput, psd esv1.PushSecretData, value []byte) error {
-	data, err := sm.client.DescribeSecret(ctx, &secretInput)
-	metrics.ObserveAPICall(constants.ProviderAWSSM, constants.CallAWSSMDescribeSecret, err)
-	if err != nil {
-		return err
-	}
-	if !isManagedByESO(data) {
-		return errors.New("secret not managed by external-secrets")
-	}
-	if awsSecret != nil && bytes.Equal(awsSecret.SecretBinary, value) || utils.CompareStringAndByteSlices(awsSecret.SecretString, value) {
+func (sm *SecretsManager) putSecretValueWithContext(ctx context.Context, secretArn string, awsSecret *awssm.GetSecretValueOutput, psd esv1.PushSecretData, value []byte, tags []types.Tag) error {
+	if awsSecret != nil && (bytes.Equal(awsSecret.SecretBinary, value) || utils.CompareStringAndByteSlices(awsSecret.SecretString, value)) {
 		return nil
 	}
 
-	newVersionNumber, err := bumpVersionNumber(awsSecret.VersionId)
-	if err != nil {
-		return err
+	newVersionNumber := initialVersion
+	if awsSecret != nil {
+		bumpedVersionNumber, err := bumpVersionNumber(awsSecret.VersionId)
+		if err != nil {
+			return err
+		}
+		newVersionNumber = *bumpedVersionNumber
 	}
 	input := &awssm.PutSecretValueInput{
-		SecretId:           awsSecret.ARN,
+		SecretId:           &secretArn,
 		SecretBinary:       value,
-		ClientRequestToken: newVersionNumber,
+		ClientRequestToken: aws.String(newVersionNumber),
 	}
 	secretPushFormat, err := utils.FetchValueFromMetadata(SecretPushFormatKey, psd.GetMetadata(), SecretPushFormatBinary)
 	if err != nil {
@@ -580,11 +601,11 @@ func (sm *SecretsManager) putSecretValueWithContext(ctx context.Context, secretI
 		return err
 	}
 
-	currentTags := make(map[string]string, len(data.Tags))
-	for _, tag := range data.Tags {
+	currentTags := make(map[string]string, len(tags))
+	for _, tag := range tags {
 		currentTags[*tag.Key] = *tag.Value
 	}
-	return sm.patchTags(ctx, psd.GetMetadata(), awsSecret.ARN, currentTags)
+	return sm.patchTags(ctx, psd.GetMetadata(), &secretArn, currentTags)
 }
 
 func (sm *SecretsManager) patchTags(ctx context.Context, metadata *apiextensionsv1.JSON, secretId *string, tags map[string]string) error {

pkg/provider/aws/secretsmanager/secretsmanager_test.go

@@ -441,9 +441,19 @@ func TestSetSecret(t *testing.T) {
 		},
 	}
 
+	tagSecretOutputNoVersions := &awssm.DescribeSecretOutput{
+		ARN:  &arn,
+		Tags: externalSecretsTag,
+	}
+
+	defaultVersion := "00000000-0000-0000-0000-000000000002"
+
 	tagSecretOutput := &awssm.DescribeSecretOutput{
 		ARN:  &arn,
 		Tags: externalSecretsTag,
+		VersionIdsToStages: map[string][]string{
+			defaultVersion: {"AWSCURRENT"},
+		},
 	}
 
 	tagSecretOutputFaulty := &awssm.DescribeSecretOutput{
@@ -451,8 +461,17 @@ func TestSetSecret(t *testing.T) {
 		Tags: externalSecretsTagFaulty,
 	}
 
+	tagSecretOutputFrom := func(versionId string) *awssm.DescribeSecretOutput {
+		return &awssm.DescribeSecretOutput{
+			ARN:  &arn,
+			Tags: externalSecretsTag,
+			VersionIdsToStages: map[string][]string{
+				versionId: {"AWSCURRENT"},
+			},
+		}
+	}
+
 	initialVersion := "00000000-0000-0000-0000-000000000001"
-	defaultVersion := "00000000-0000-0000-0000-000000000002"
 	defaultUpdatedVersion := "00000000-0000-0000-0000-000000000003"
 	randomUUIDVersion := "c2812e8d-84ce-4858-abec-7163d8ab312b"
 	randomUUIDVersionIncremented := "c2812e8d-84ce-4858-abec-7163d8ab312c"
@@ -468,6 +487,7 @@ func TestSetSecret(t *testing.T) {
 		SecretBinary: secretValue,
 		VersionId:    &defaultVersion,
 	}
+	blankDescribeSecretOutput := &awssm.DescribeSecretOutput{}
 
 	type params struct {
 		s       string
@@ -489,7 +509,6 @@ func TestSetSecret(t *testing.T) {
 			VersionId:    version,
 		}
 	}
-	blankSecretValueOutput := &awssm.GetSecretValueOutput{}
 
 	putSecretOutput := &awssm.PutSecretValueOutput{
 		ARN: &arn,
@@ -527,7 +546,6 @@ func TestSetSecret(t *testing.T) {
 				store: makeValidSecretStore().Spec.Provider.AWS,
 				client: fakesm.Client{
 					GetSecretValueFn: fakesm.NewGetSecretValueFn(secretValueOutput, nil),
-					CreateSecretFn:   fakesm.NewCreateSecretFn(secretOutput, nil),
 					PutSecretValueFn: fakesm.NewPutSecretValueFn(putSecretOutput, nil),
 					DescribeSecretFn: fakesm.NewDescribeSecretFn(tagSecretOutput, nil),
 					TagResourceFn:    fakesm.NewTagResourceFn(&awssm.TagResourceOutput{}, nil),
@@ -539,13 +557,50 @@ func TestSetSecret(t *testing.T) {
 				err: nil,
 			},
 		},
+		"SetSecretSucceedsWithExistingSecretButNoSecretVersionsWithoutProperty": {
+			reason: "a secret can be pushed to aws secrets manager when it already exists but has no secret versions",
+			args: args{
+				store: makeValidSecretStore().Spec.Provider.AWS,
+				client: fakesm.Client{
+					DescribeSecretFn: fakesm.NewDescribeSecretFn(tagSecretOutputNoVersions, nil),
+					PutSecretValueFn: fakesm.NewPutSecretValueFn(putSecretOutput, nil, fakesm.ExpectedPutSecretValueInput{
+						SecretBinary: []byte(`fake-value`),
+						Version:      aws.String(initialVersion),
+					}),
+					TagResourceFn:   fakesm.NewTagResourceFn(&awssm.TagResourceOutput{}, nil),
+					UntagResourceFn: fakesm.NewUntagResourceFn(&awssm.UntagResourceOutput{}, nil),
+				},
+				pushSecretData: pushSecretDataWithoutProperty,
+			},
+			want: want{
+				err: nil,
+			},
+		},
+		"SetSecretSucceedsWithExistingSecretButNoSecretVersionsWithProperty": {
+			reason: "a secret can be pushed to aws secrets manager when it already exists but has no secret versions",
+			args: args{
+				store: makeValidSecretStore().Spec.Provider.AWS,
+				client: fakesm.Client{
+					DescribeSecretFn: fakesm.NewDescribeSecretFn(tagSecretOutputNoVersions, nil),
+					PutSecretValueFn: fakesm.NewPutSecretValueFn(putSecretOutput, nil, fakesm.ExpectedPutSecretValueInput{
+						SecretBinary: []byte(`{"other-fake-property":"fake-value"}`),
+						Version:      aws.String(initialVersion),
+					}),
+					TagResourceFn:   fakesm.NewTagResourceFn(&awssm.TagResourceOutput{}, nil),
+					UntagResourceFn: fakesm.NewUntagResourceFn(&awssm.UntagResourceOutput{}, nil),
+				},
+				pushSecretData: pushSecretDataWithProperty,
+			},
+			want: want{
+				err: nil,
+			},
+		},
 		"SetSecretSucceedsWithoutSecretKey": {
 			reason: "a secret can be pushed to aws secrets manager without secret key",
 			args: args{
 				store: makeValidSecretStore().Spec.Provider.AWS,
 				client: fakesm.Client{
 					GetSecretValueFn: fakesm.NewGetSecretValueFn(secretValueOutput, nil),
-					CreateSecretFn:   fakesm.NewCreateSecretFn(secretOutput, nil),
 					PutSecretValueFn: fakesm.NewPutSecretValueFn(putSecretOutput, nil),
 					DescribeSecretFn: fakesm.NewDescribeSecretFn(tagSecretOutput, nil),
 					TagResourceFn:    fakesm.NewTagResourceFn(&awssm.TagResourceOutput{}, nil),
@@ -563,7 +618,6 @@ func TestSetSecret(t *testing.T) {
 				store: makeValidSecretStore().Spec.Provider.AWS,
 				client: fakesm.Client{
 					GetSecretValueFn: fakesm.NewGetSecretValueFn(secretValueOutput, nil),
-					CreateSecretFn:   fakesm.NewCreateSecretFn(secretOutput, nil),
 					PutSecretValueFn: fakesm.NewPutSecretValueFn(putSecretOutput, nil),
 					DescribeSecretFn: fakesm.NewDescribeSecretFn(tagSecretOutput, nil),
 					TagResourceFn:    fakesm.NewTagResourceFn(&awssm.TagResourceOutput{}, nil),
@@ -581,7 +635,6 @@ func TestSetSecret(t *testing.T) {
 				store: makeValidSecretStore().Spec.Provider.AWS,
 				client: fakesm.Client{
 					GetSecretValueFn: fakesm.NewGetSecretValueFn(secretValueOutput, &getSecretCorrectErr),
-					CreateSecretFn:   fakesm.NewCreateSecretFn(secretOutput, nil),
 					PutSecretValueFn: fakesm.NewPutSecretValueFn(putSecretOutput, nil),
 					DescribeSecretFn: fakesm.NewDescribeSecretFn(tagSecretOutput, nil),
 				},
@@ -596,7 +649,7 @@ func TestSetSecret(t *testing.T) {
 						}`)}},
 			},
 			want: want{
-				err: nil,
+				err: &getSecretCorrectErr,
 			},
 		},
 		"SetSecretSucceedsWithExistingSecretAndAdditionalTags": {
@@ -605,7 +658,6 @@ func TestSetSecret(t *testing.T) {
 				store: makeValidSecretStore().Spec.Provider.AWS,
 				client: fakesm.Client{
 					GetSecretValueFn: fakesm.NewGetSecretValueFn(secretValueOutput, nil),
-					CreateSecretFn:   fakesm.NewCreateSecretFn(secretOutput, nil),
 					PutSecretValueFn: fakesm.NewPutSecretValueFn(putSecretOutput, nil),
 					DescribeSecretFn: fakesm.NewDescribeSecretFn(tagSecretOutput, nil),
 					TagResourceFn:    fakesm.NewTagResourceFn(&awssm.TagResourceOutput{}, nil),
@@ -629,7 +681,7 @@ func TestSetSecret(t *testing.T) {
 			args: args{
 				store: makeValidSecretStore().Spec.Provider.AWS,
 				client: fakesm.Client{
-					GetSecretValueFn: fakesm.NewGetSecretValueFn(blankSecretValueOutput, &getSecretCorrectErr),
+					DescribeSecretFn: fakesm.NewDescribeSecretFn(blankDescribeSecretOutput, &getSecretCorrectErr),
 					CreateSecretFn:   fakesm.NewCreateSecretFn(secretOutput, nil),
 				},
 				pushSecretData: pushSecretDataWithoutProperty,
@@ -643,7 +695,7 @@ func TestSetSecret(t *testing.T) {
 			args: args{
 				store: makeValidSecretStore().Spec.Provider.AWS,
 				client: fakesm.Client{
-					GetSecretValueFn: fakesm.NewGetSecretValueFn(blankSecretValueOutput, &getSecretCorrectErr),
+					DescribeSecretFn: fakesm.NewDescribeSecretFn(blankDescribeSecretOutput, &getSecretCorrectErr),
 					CreateSecretFn:   fakesm.NewCreateSecretFn(secretOutput, nil, []byte(`{"other-fake-property":"fake-value"}`)),
 				},
 				pushSecretData: pushSecretDataWithProperty,
@@ -681,7 +733,7 @@ func TestSetSecret(t *testing.T) {
 						b:       []byte((`{"fake-property":"fake-value"}`)),
 						version: &randomUUIDVersion,
 					}), nil),
-					DescribeSecretFn: fakesm.NewDescribeSecretFn(tagSecretOutput, nil),
+					DescribeSecretFn: fakesm.NewDescribeSecretFn(tagSecretOutputFrom(randomUUIDVersion), nil),
 					PutSecretValueFn: fakesm.NewPutSecretValueFn(putSecretOutput, nil, fakesm.ExpectedPutSecretValueInput{
 						SecretBinary: []byte(`{"fake-property":"fake-value","other-fake-property":"fake-value"}`),
 						Version:      &randomUUIDVersionIncremented,
@@ -705,10 +757,6 @@ func TestSetSecret(t *testing.T) {
 						version: &unparsableVersion,
 					}), nil),
 					DescribeSecretFn: fakesm.NewDescribeSecretFn(tagSecretOutput, nil),
-					PutSecretValueFn: fakesm.NewPutSecretValueFn(putSecretOutput, nil, fakesm.ExpectedPutSecretValueInput{
-						SecretBinary: []byte(`{"fake-property":"fake-value","other-fake-property":"fake-value"}`),
-						Version:      &initialVersion,
-					}),
 				},
 				pushSecretData: pushSecretDataWithProperty,
 			},
@@ -798,7 +846,7 @@ func TestSetSecret(t *testing.T) {
 			args: args{
 				store: makeValidSecretStore().Spec.Provider.AWS,
 				client: fakesm.Client{
-					GetSecretValueFn: fakesm.NewGetSecretValueFn(blankSecretValueOutput, &getSecretCorrectErr),
+					DescribeSecretFn: fakesm.NewDescribeSecretFn(blankDescribeSecretOutput, &getSecretCorrectErr),
 					CreateSecretFn:   fakesm.NewCreateSecretFn(nil, noPermission),
 				},
 				pushSecretData: pushSecretDataWithoutProperty,
@@ -812,7 +860,7 @@ func TestSetSecret(t *testing.T) {
 			args: args{
 				store: makeValidSecretStore().Spec.Provider.AWS,
 				client: fakesm.Client{
-					GetSecretValueFn: fakesm.NewGetSecretValueFn(blankSecretValueOutput, noPermission),
+					DescribeSecretFn: fakesm.NewDescribeSecretFn(blankDescribeSecretOutput, noPermission),
 				},
 				pushSecretData: pushSecretDataWithoutProperty,
 			},
@@ -852,11 +900,11 @@ func TestSetSecret(t *testing.T) {
 			},
 		},
 		"SetSecretWrongGetSecretErrFails": {
-			reason: "GetSecretValueWithContext errors out when anything except awssm.ErrCodeResourceNotFoundException",
+			reason: "DescribeSecret errors out when anything except awssm.ErrCodeResourceNotFoundException",
 			args: args{
 				store: makeValidSecretStore().Spec.Provider.AWS,
 				client: fakesm.Client{
-					GetSecretValueFn: fakesm.NewGetSecretValueFn(blankSecretValueOutput, &getSecretWrongErr),
+					DescribeSecretFn: fakesm.NewDescribeSecretFn(blankDescribeSecretOutput, &getSecretWrongErr),
 				},
 				pushSecretData: pushSecretDataWithoutProperty,
 			},