Adding secret_binary to aws_secretsmanager_secret_version

[eraac]

Fixes #4571

Changes proposed in this pull request:

• Adding data secret_binary for data "aws_secretsmanager_secret_version"
• Adding input secret_binary for resource "aws_secretsmanager_secret_version"

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAwsSecretsManagerSecretVersion'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsSecretsManagerSecretVersion -timeout 120m
=== RUN   TestAccAwsSecretsManagerSecretVersion_BasicString
--- PASS: TestAccAwsSecretsManagerSecretVersion_BasicString (12.46s)
=== RUN   TestAccAwsSecretsManagerSecretVersion_BasicBinary
--- PASS: TestAccAwsSecretsManagerSecretVersion_BasicBinary (10.40s)
=== RUN   TestAccAwsSecretsManagerSecretVersion_VersionStages
--- PASS: TestAccAwsSecretsManagerSecretVersion_VersionStages (26.18s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	49.086s
...

First time i contribute to a terraform provider, i hope to do well.

I have just few doubt about the code for the resource (for the cast []byte <-> string). For the test i only copy/paste the test for secret_string

[bflad]

Hi @eraac 👋 Thanks for submitting this! When working with binary data in Terraform, it should be base64 encoded and decoded to prevent Terraform state corruption. Are you able to update the handling to include that? Otherwise this is looking pretty good so far. 👍

[eraac]

@bflad https://docs.aws.amazon.com/sdk-for-go/api/service/secretsmanager/#GetSecretValueOutput according to documentation is automatically base64 encoded/decoded by the SDK

[eraac]

I probably misunderstood, you mean for the terraform himself ?

So user should insert data as base64 and I decode it in the plugin ? What about output, should be decoded or not ?

[bflad]

So user should insert data as base64 and I decode it in the plugin ?

Yes, sorry if I was unclear! Basically, we should require something like "${base64encode(file("path/to/binaryfile"))}" or "${base64gzip(file("path/to/binaryfile"))}" in configurations so that there are no issues with parsing configurations or saving the binary data in the Terraform state as a UTF-8 string.

If the AWS SDK already handles the conversion correct, that's great! We'll just want to make sure the acceptance test is updated to perform the above on some binary content (there are some zip files in aws/test-fixtures that can be used here) and that the documentation notes the required base64 input and likely mentioning the base64encode() function available in Terraform.

What about output, should be decoded or not ?

It should be base64 encoded. If people need to get the actual binary contents, they can use base64decode() function

[eraac]

I take a look to other resources using base64 and your response confirm. Thanks for the precision

I push new commit with all the changes needed (code, test and documentation), should be good now :proud:

[bflad]

LGTM, thanks @eraac! 🚀

--- PASS: TestAccAwsSecretsManagerSecretVersion_BasicString (14.55s)
--- PASS: TestAccAwsSecretsManagerSecretVersion_Base64Binary (13.49s)
--- PASS: TestAccAwsSecretsManagerSecretVersion_VersionStages (33.27s)

[bflad]

We should note this is base64 encoded. I'll make the very minor edit post-merge. 😄

[bflad]

Not a blocker for merging, but I believe we could enhance this check to actually be done during plan with a ValidateFunc on the secret_binary attribute:

ValidateFunc: func(v interface{}, name string) (warns []string, errs []error) {
	s := v.(string)
	if !isBase64Encoded([]byte(s)) {
		errs = append(errs, fmt.Errorf("%s: must be base64-encoded", name))
	}
	return
},

At some point we can migrate the existing attributes that use this (or a similar) ValidateFunc for ensure Base64 content into its own, easy to use common function like validateBase64String. 👍

[bflad]

This has been released in version 1.40.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!