Add secrets manager resource policy support #5290
Conversation
ghost
added
the
bflad
added
enhancement
There was a problem hiding this comment.
Thanks so much for this, @ts-tek! 🚀 Left a few minor comments which I will adjust on merge since they are more nitpicks than anything else. 👍
7 tests passed (all tests)
=== RUN TestAccAwsSecretsManagerSecret_policy
--- PASS: TestAccAwsSecretsManagerSecret_policy (6.85s)
=== RUN TestAccAwsSecretsManagerSecret_Basic
--- PASS: TestAccAwsSecretsManagerSecret_Basic (7.49s)
=== RUN TestAccAwsSecretsManagerSecret_Description
--- PASS: TestAccAwsSecretsManagerSecret_Description (12.25s)
=== RUN TestAccAwsSecretsManagerSecret_Tags
--- PASS: TestAccAwsSecretsManagerSecret_Tags (21.34s)
=== RUN TestAccAwsSecretsManagerSecret_KmsKeyID
--- PASS: TestAccAwsSecretsManagerSecret_KmsKeyID (31.77s)
=== RUN TestAccAwsSecretsManagerSecret_RotationLambdaARN
--- PASS: TestAccAwsSecretsManagerSecret_RotationLambdaARN (31.94s)
=== RUN TestAccAwsSecretsManagerSecret_RotationRules
--- PASS: TestAccAwsSecretsManagerSecret_RotationRules (32.01s)
| } | ||
|
|
||
| if pOut.ResourcePolicy != nil { | ||
| policy, err := structure.NormalizeJsonString(*pOut.ResourcePolicy) |
There was a problem hiding this comment.
To prevent potential panics, we should prefer to use the SDK provided function for dereferencing values (aws.StringValue(pOut.ResourcePolicy)) instead of directly dereferencing via *.
| if pOut.ResourcePolicy != nil { | ||
| policy, err := structure.NormalizeJsonString(*pOut.ResourcePolicy) | ||
| if err != nil { | ||
| return errwrap.Wrapf("policy contains an invalid JSON: {{err}}", err) |
There was a problem hiding this comment.
Nitpick: This is likely copy paste from other code, but we prefer to use the standard library fmt.Errorf() for returning error messages instead of errwrap.Wrapf() which has functionality we do not need in this case. 👍 At some point we'll get around to cleaning up all the other references to errwrap.Wrapf().
| if v, ok := d.GetOk("policy"); ok && v.(string) != "" { | ||
| policy, err := structure.NormalizeJsonString(v.(string)) | ||
| if err != nil { | ||
| return errwrap.Wrapf("policy contains an invalid JSON: {{err}}", err) |
There was a problem hiding this comment.
Same here, regarding fmt.Errorf() versus errwrap.Wrapf()
|
This has been released in version 1.29.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
|
Can you also update docs: This change is not documented there |
|
@mikolajprzybysz this change in this pull request only occurred on the resource, not the data source, which is why you do not see the documentation there. The feature request for the data source can be found here: #5329 |
|
My point is that with the merge of this pull request now there is new attribute "policy" available in resource aws_secretsmanager_secret, unfortunately even though I can see docs have been update with commit 215668c this is not visible here https://www.terraform.io/docs/providers/aws/d/secretsmanager_secret.html in section "Attributes Reference" which I would expect it to be. Furthermore it would be helpful to also add information in docs what is the version this attribute is available since. I hope you find my feedback helpful :) |
|
Note the If the data source page doesn't have
Versioned documentation is on the long-term roadmap, but not currently a focus. |
|
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! |
Fixes #4997
Changes proposed in this pull request:
policyproperty on aws_secretsmanager_secretOutput from acceptance testing: