add acl validator to aws s3 bucket resource #15327

NikolaeVarius · 2020-09-24T01:48:20Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Release note for CHANGELOG:


- Adds in canned acls that apply to buckets documented in https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html#canned-acl specifically "aws-exec-read" and "log-delivery-write" as valid acls

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

github-actions · 2020-09-24T01:48:46Z

Thank you for your contribution! 🚀

Please note that typically Go dependency changes are handled in this repository by Renovate Bot or the maintainers. This is to prevent pull request merge conflicts and further delay reviews of contributions. Remove any changes to the go.mod, go.sum, and vendor/ files and commit them into this pull request.

Additional details:

Check open pull requests with the dependencies label to view other dependency updates.
If this pull request includes an update the AWS Go SDK (or any other dependency) version, only updates submitted via Renovate Bot will be merged. This pull request will need to remove these changes and will need to be rebased after the existing dependency update via Renovate Bot has been merged for this pull request to be reviewed.
If this pull request is for supporting a new AWS service:
- Ensure the new AWS service changes are following the Contributing Guide section on new services, in particular that the dependency addition and initial provider support are in a separate pull request from other changes (e.g. new resources). Contributions not following this item will not be reviewed until the changes are split.
- If this pull request is already a separate pull request from the above item, you can ignore this message.

reedloden · 2020-09-26T16:47:24Z

aws/resource_aws_s3_bucket.go

@@ -76,6 +76,14 @@ func resourceAwsS3Bucket() *schema.Resource {
 				Default:       "private",
 				Optional:      true,
 				ConflictsWith: []string{"grant"},
+				ValidateFunc: validation.StringInSlice([]string{


Instead of hardcoding them, how about just using s3.BucketCannedACL_Values()?

I dont really have a preference. But, looking at other validation functions, they seem to list out all the values. I dont think I want to go against the style?

https://github.com/terraform-providers/terraform-provider-aws/blob/0544ae1fe0f788616cc483df8d0e68d3dd36a0ed/aws/resource_aws_s3_bucket.go#L120
https://github.com/terraform-providers/terraform-provider-aws/blob/0544ae1fe0f788616cc483df8d0e68d3dd36a0ed/aws/resource_aws_s3_bucket.go#L406
https://github.com/terraform-providers/terraform-provider-aws/blob/0544ae1fe0f788616cc483df8d0e68d3dd36a0ed/aws/resource_aws_s3_bucket.go#L453

Hi @NikolaeVarius. We've actually been changing over to the _Values() functions so that new values are automatically picked up when we update the AWS SDK. We're tracking this in #14601

The _Values() functions were actually added by one of our maintainers in aws/aws-sdk-go#3447 🙂

reedloden · 2020-09-26T16:47:34Z

aws/resource_aws_s3_bucket.go

+					s3.BucketCannedACLPublicReadWrite,
+					s3.BucketCannedACLAWSExecRead,
+					s3.BucketCannedACLAuthenticatedRead,
+					s3.BucketCannedACLLogDeliveryWrite,


This doesn't seem to exist?

as in the other comment, yeah you're correct

reedloden · 2020-09-26T16:48:11Z

aws/resource_aws_s3_bucket.go

+					s3.BucketCannedACLPrivate,
+					s3.BucketCannedACLPublicRead,
+					s3.BucketCannedACLPublicReadWrite,
+					s3.BucketCannedACLAWSExecRead,


I don't think this exists for buckets... For objects, there's ObjectCannedACLAwsExecRead.

https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html#canned-acl

It seems to say it works on both

The SDK doesn't seem to support it. See https://docs.aws.amazon.com/sdk-for-go/api/service/s3/#pkg-constants. Or if you look at the code directly (https://raw.githubusercontent.com/aws/aws-sdk-go/master/service/s3/api.go), you can take a look at the BucketCannedACL_Values() function.

I see you're correct. Those values are exist at the ObjectCannedACL_Values() function. I dont know the behavior just yet, and would need to test, but in this case, would it be wise to add this validator in if the upstream SDK doesn't support all possible values for a bucket? Wouldn't want to return a validation error when the ACL is perfectly valid.

// ObjectCannedACL_Values returns all elements of the ObjectCannedACL enum func ObjectCannedACL_Values() []string { return []string{ ObjectCannedACLPrivate, ObjectCannedACLPublicRead, ObjectCannedACLPublicReadWrite, ObjectCannedACLAuthenticatedRead, ObjectCannedACLAwsExecRead, ObjectCannedACLBucketOwnerRead, ObjectCannedACLBucketOwnerFullControl, } }

NikolaeVarius · 2020-09-30T23:51:18Z

It seems the outdated canned acls in the go SDK are a known issue

aws/aws-sdk-go#2683

Further linked report terraform-linters/tflint#341 (comment)

gdavison · 2020-10-16T22:23:20Z

vendor/github.com/aws/aws-sdk-go/service/s3/api.go

+	// BucketCannedACLAWSExecRead is a BucketCannedACL enum value
+	BucketCannedACLAWSExecRead = "aws-exec-read"
+
 	// BucketCannedACLAuthenticatedRead is a BucketCannedACL enum value
 	BucketCannedACLAuthenticatedRead = "authenticated-read"
+
+	// BucketCannedACLLogDeliveryWrite is a BucketCannedACL enum value
+	BucketCannedACLLogDeliveryWrite = "log-delivery-write"


Unfortunately, we can't make changes in the vendored dependencies, because they'll be wiped out the next time we update the AWS SDK. When we run into missing constants, the first thing to do is double-check that they haven't been added in a new version of the SDK. If they haven't, then we can define our own constants in the directory aws/internal/service/s3 and also file an issue with AWS either on the SDK or with support

gdavison · 2020-10-16T22:28:31Z

HI @NikolaeVarius, this PR looks like a good start. Is there anything you need from us to move it forward?

ghost · 2020-11-12T23:42:32Z

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ghost · 2020-12-11T17:10:07Z

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!

ghost added size/XS Managed by automation to categorize the size of a PR. service/s3 Issues and PRs that pertain to the s3 service. dependencies Used to indicate dependency changes. labels Sep 24, 2020

github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Sep 24, 2020

NikolaeVarius added 2 commits September 23, 2020 19:53

add in currently documented s3 bucket canned acls to validator

ca6c281

add validator for s3 bucket acl

0544ae1

NikolaeVarius force-pushed the 15304 branch from d88cc16 to 0544ae1 Compare September 24, 2020 01:53

reedloden suggested changes Sep 26, 2020

View reviewed changes

gdavison reviewed Oct 16, 2020

View reviewed changes

gdavison marked this pull request as ready for review November 10, 2020 20:33

gdavison requested a review from a team as a code owner November 10, 2020 20:33

gdavison added a commit that referenced this pull request Nov 10, 2020

Merge pull request #15327 from NikolaeVarius/15304

fc9e520

gdavison added a commit that referenced this pull request Nov 10, 2020

Update CHANGELOG for #15327

c19d246

gdavison merged commit 36c2a5c into hashicorp:master Nov 10, 2020

gdavison removed the needs-triage Waiting for first response or review from a maintainer. label Nov 10, 2020

gdavison added this to the v3.15.0 milestone Nov 10, 2020

james-skinner-deltatre mentioned this pull request Nov 16, 2020

S3 bucket ACL validation is missing valid Canned ACLs #16212

Closed

eyablonowitz pushed a commit to PatientPing/terraform-aws-bastion that referenced this pull request Dec 2, 2020

fix s3 bucket acl broken by hashicorp/terraform-provider-aws#15327

896ebd0

eyablonowitz mentioned this pull request Dec 2, 2020

fix s3 bucket acl PatientPing/terraform-aws-bastion#10

Merged

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add acl validator to aws s3 bucket resource #15327

add acl validator to aws s3 bucket resource #15327

NikolaeVarius commented Sep 24, 2020 •

edited

Loading

github-actions bot commented Sep 24, 2020

reedloden Sep 26, 2020 •

edited

Loading

NikolaeVarius Sep 30, 2020

gdavison Oct 16, 2020

reedloden Sep 26, 2020

NikolaeVarius Sep 27, 2020

NikolaeVarius Oct 1, 2020

reedloden Sep 26, 2020

NikolaeVarius Sep 27, 2020

reedloden Sep 27, 2020

NikolaeVarius Sep 30, 2020

NikolaeVarius commented Sep 30, 2020 •

edited

Loading

gdavison Oct 16, 2020

gdavison commented Oct 16, 2020

ghost commented Nov 12, 2020

ghost commented Dec 11, 2020

add acl validator to aws s3 bucket resource #15327

add acl validator to aws s3 bucket resource #15327

Conversation

NikolaeVarius commented Sep 24, 2020 • edited Loading

Community Note

github-actions bot commented Sep 24, 2020

reedloden Sep 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikolaeVarius commented Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

gdavison commented Oct 16, 2020

ghost commented Nov 12, 2020

ghost commented Dec 11, 2020

NikolaeVarius commented Sep 24, 2020 •

edited

Loading

reedloden Sep 26, 2020 •

edited

Loading

NikolaeVarius commented Sep 30, 2020 •

edited

Loading