Implementation of acl grants #3728

Chhed13 · 2018-03-09T22:27:44Z

It implements ACL policy grants requested in #989.
Type AmazonCustomerByEmail is not implemented due to it can only be set, but get is only in form of CannonicalUser.
Canned acl also can only be set, but not get and it also add some conditionals.

Chhed13 · 2018-08-29T06:24:30Z

@antonbabenko could you please take a look

antonbabenko · 2018-09-05T08:55:07Z

Hi Andrey!

@bflad is the best person who can take a look at the code and prioritize this feature over others.

The feature you are proposing makes perfect sense to be added, but since it is not so small and entirely obvious, it may take some time to be reviewed and merged.

Best regards,
Anton.

Chhed13 · 2018-09-05T19:56:26Z

Hi Anton,

Thanks, let's wait. Just didn't think that the queue is so long

antonbabenko · 2018-09-05T20:04:16Z

Luckily (not unfortunately), there are a lot of people who are using Terraform.

Here you can get a basic understanding of the priorities - https://github.com/terraform-providers/terraform-provider-aws/pulls?q=is%3Apr+is%3Aopen+sort%3Areactions-%2B1-desc

Remember that this is one of at least 3 priority queues which I know a team is picking from.

https://www.hashicorp.com/jobs/1288355 - I will just leave it here, if you want to join. :)

eredi93 · 2018-10-08T14:36:01Z

@bflad can you give an ETA for the review of this PR? setting the current ACL (provate, log-delivery-write etc) is overriding CloudFront c4c1ede66af53448b93c283ce9448c4ba468c9432aa01d700d3878632f77d2d0 ACL https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/AccessLogs.html#AccessLogsBucketAndFileOwnership
adding ACL grants would solve this

bflad · 2018-10-08T20:12:55Z

Hi @eredi93 👋 Sorry this one is not on my personal short list, but I cannot speak for other maintainers. We encourage upvoting via 👍 reactions on the original PR though to help prioritize. As mentioned above, S3 ACL management is non-trivial and may require some design cycles to get correct.

pauldraper · 2018-11-01T13:39:16Z

Alright @bflad, by number of votes this PR is currently #5 of 198 open PRs.

Hope to see that status reflected in the review/merge priority.

pauldraper · 2018-11-22T06:21:55Z

It is now #4 of 207 open PRs. Twenty-five PRs -- all with less than a quarter of this PRs upvotes -- have been merged in the last week alone.

I fear the maintainers do not actually sort by @antonbabenko's link when reviewing and this 300 line will live for well over a year before being addressed. Very slow pace.

tdmalone · 2018-12-13T00:13:14Z

@pauldraper Anton did say that is one of 3 different priority lists. Also - this is both a size/L PR and a complicated part of AWS due to S3 permissions being a hotpotch of legacy and newer controls. I wouldn't be surprised if smaller PRs got merged first simply because they're easier to 'review in a break between tasks' rather than needing a full session of getting in the zone to sufficiently understand the scope of the PR. Having said that, I'd love for this to get merged too... but we gotta be patient with a free, open source tool 😄

bflad

Hi @Chhed13 👋 Thanks for submitting this and sorry for the lengthy delays in getting a review. Please see the initial feedback and let us know if you have any questions or do not have time to implement the items.

bflad · 2019-03-26T19:42:53Z

aws/resource_aws_s3_bucket.go

+				ConflictsWith: []string{"grant"},
+			},
+
+			"grant": {


To prevent issues with setting the state during Read and other unforeseen issues with the initial implementation, I would recommend adding Computed: true to this attribute for now. We can always adjust this in the future and add a note in the documentation for now, e.g. To enable drift detection for this configuration, it must be configured.

Computed: true leads to non-tacking in case of removal from TF resource, is it really needed?
It will work only if it was set explicitly, I don't see the way to track it always now (answered blow).
Of course I can miss some, but on tests Computed: true make the situation worse.

Day passed and I understand the idea of using Computed: true and always save the state, it will work. And if you'll say that it is better way - I change the PR to this version.
The only thing I miss - that it will no the way back from computed, because it will make a lot of changes in state files

aws/resource_aws_s3_bucket.go

bflad · 2019-03-28T01:00:12Z

aws/resource_aws_s3_bucket.go

+
+		log.Printf("[DEBUG] S3 bucket: %s, read ACL grants policy before set: %+v", d.Id(), grants)
+		//if ACL grants contains bucket owner FULL_CONTROL only - it's "private" acl, skip the state
+		if len(grants) == 1 && grants[0]["id"].(string) == *ap.Owner.ID &&


I believe this conditional can be removed if we prefer to always save to state

aws/resource_aws_s3_bucket.go

bflad · 2019-03-28T01:02:43Z

aws/resource_aws_s3_bucket.go

+			grantMap := rawGrant.(map[string]interface{})
+			for _, rawPermission := range grantMap["permission"].([]interface{}) {
+				ge := &s3.Grantee{}
+				if i := grantMap["id"].(string); i != "" {


To prevent potential panics, we should perform a type assertion check here and below:

Suggested change

if i := grantMap["id"].(string); i != "" {

if i, ok := grantMap["id"].(string); ok && i != "" {

website/docs/r/s3_bucket.html.markdown

upstream

Chhed13 · 2019-03-28T19:38:30Z

Hi @bflad, thank you for the review, I thought that it hang.
So, I have time to work it out and thank you for the great comments

# Conflicts: # aws/validators.go

…or comments

gangadhar01a · 2019-06-19T17:15:38Z

Any updates on this request?

bostrowski13 · 2019-07-23T13:21:56Z

be super nice to have this.

gdavison · 2019-09-10T19:49:41Z

We could really make use of this and get rid of some local-exec. Is there anything I can do to help unblock this?

Also, @Chhed13 @bflad, I think I know how to address the AmazonCustomerByEmail issue. Essentially, accept the email as an input, then store both the email and canonical ID in the state. Then when updating, check for changes in either value.

Chhed13 · 2019-10-30T15:28:51Z

I'm still waiting feedback from maintainers. @bflad is there any chance that you answer?
@gdavison it is a good idea, but in case of change in cannonical ID and when initial config were by email - we will easily get state flapping.

C1PendletonJones · 2020-02-07T16:04:37Z

Any updates for this? It would be very useful to have for enterprise use cases.

gdavison · 2020-02-12T01:27:17Z

@Chhed13 there are merge conflicts on this PR, can you please resolve them? It also looks like there are some acceptance test failures.

Chhed13 · 2020-02-12T05:35:05Z

@gdavison ok, I'll take a look

…-acl-policies # Conflicts: # aws/resource_aws_s3_bucket_test.go # website/docs/r/s3_bucket.html.markdown

gdavison

I've added a number of changes to make.

Also, can you run the full set of S3 acceptance tests, please?

You can ignore failures with "After applying this step, the plan was not empty", since they're related to eventual consistency problems with S3.
You'll get a number of "ImportStateVerify attributes not equivalent" errors. I think most of these are related to our default test cases mostly defining acl = "public-read" for some reason; this can be changed to the default, IMO 😄

aws/resource_aws_s3_bucket_test.go

website/docs/r/s3_bucket.html.markdown

aws/resource_aws_s3_bucket_test.go

website/docs/r/s3_bucket.html.markdown

aws/resource_aws_s3_bucket_test.go

Chhed13 · 2020-02-26T18:39:17Z

@gdavison thanks for the review.
I tried to fix them all. And of course I passed all the tests for the s3_bucket

gdavison

It's looking great! Just a couple styling changes

aws/resource_aws_s3_bucket_test.go

gdavison

LGTM 🚀

--- PASS: TestAccAWSS3BucketAnalyticsConfiguration_WithFilter_Empty (5.05s)
--- PASS: TestAccAWSS3BucketAnalyticsConfiguration_WithStorageClassAnalysis_Empty (5.34s)
--- PASS: TestAccAWSS3BucketMetric_WithEmptyFilter (23.88s)
--- PASS: TestAccAWSS3BucketAnalyticsConfiguration_WithStorageClassAnalysis_Full (25.69s)
--- PASS: TestAccAWSS3BucketInventory_encryptWithSSES3 (25.87s)
--- PASS: TestAccAWSS3BucketMetric_basic (26.39s)
--- PASS: TestAccAWSS3BucketAnalyticsConfiguration_basic (26.78s)
--- PASS: TestAccAWSS3BucketInventory_basic (27.48s)
--- PASS: TestAccAWSS3BucketAnalyticsConfiguration_WithStorageClassAnalysis_Default (27.61s)
--- PASS: TestAccAWSS3BucketObject_noNameNoKey (7.64s)
--- PASS: TestAccAWSS3BucketAnalyticsConfiguration_removed (37.39s)
--- PASS: TestAccAWSS3BucketMetric_WithFilterPrefixAndSingleTag (39.30s)
--- PASS: TestAccAWSS3BucketMetric_WithFilterPrefix (39.08s)
--- PASS: TestAccAWSS3BucketAnalyticsConfiguration_WithFilter_Remove (39.24s)
--- PASS: TestAccAWSS3BucketAnalyticsConfiguration_WithFilter_SingleTag (39.43s)
--- PASS: TestAccAWSS3BucketAnalyticsConfiguration_WithFilter_PrefixAndTags (39.32s)
--- PASS: TestAccAWSS3BucketMetric_WithFilterPrefixAndMultipleTags (39.54s)
--- PASS: TestAccAWSS3BucketAnalyticsConfiguration_WithFilter_MultipleTags (39.85s)
--- PASS: TestAccAWSS3BucketAnalyticsConfiguration_WithFilter_Prefix (39.73s)
--- PASS: TestAccAWSS3BucketMetric_WithFilterMultipleTags (36.44s)
--- PASS: TestAccAWSS3BucketMetric_WithFilterSingleTag (36.30s)
--- PASS: TestAccAWSS3BucketInventory_encryptWithSSEKMS (44.41s)
--- PASS: TestAccAWSS3BucketNotification_Queue (24.65s)
--- PASS: TestAccAWSS3BucketObject_empty (17.83s)
--- PASS: TestAccAWSS3BucketNotification_Topic_Multiple (27.11s)
--- PASS: TestAccAWSS3BucketAnalyticsConfiguration_updateBasic (54.21s)
--- PASS: TestAccAWSS3BucketNotification_Topic (27.58s)
--- PASS: TestAccAWSS3BucketObject_source (19.51s)
--- PASS: TestAccAWSS3BucketObject_content (20.88s)
--- PASS: TestAccAWSS3BucketObject_withContentCharacteristics (21.33s)
--- PASS: TestAccAWSS3BucketObject_etagEncryption (21.77s)
--- PASS: TestAccAWSS3BucketObject_NonVersioned (21.67s)
--- PASS: TestAccAWSS3BucketObject_contentBase64 (22.38s)
--- PASS: TestAccAWSS3BucketObject_sse (22.37s)
--- PASS: TestAccAWSS3BucketNotification_update (39.35s)
--- PASS: TestAccAWSS3BucketNotification_LambdaFunction (45.93s)
--- PASS: TestAccAWSS3BucketNotification_LambdaFunction_LambdaFunctionArn_Alias (49.77s)
--- PASS: TestAccAWSS3BucketObject_updateSameFile (35.97s)
--- PASS: TestAccAWSS3BucketObject_updatesWithVersioning (36.27s)
--- PASS: TestAccAWSS3BucketObject_updates (36.46s)
--- PASS: TestAccAWSS3BucketObject_kms (37.58s)
--- PASS: TestAccAWSS3BucketObject_updatesWithVersioningViaAccessPoint (38.37s)
--- PASS: TestAccAWSS3BucketPolicy_basic (26.20s)
--- PASS: TestAccAWSS3BucketPublicAccessBlock_bucketDisappears (18.48s)
--- PASS: TestAccAWSS3BucketPublicAccessBlock_disappears (22.42s)
--- PASS: TestAccAWSS3BucketPublicAccessBlock_basic (26.80s)
--- PASS: TestAccAWSS3BucketObject_ObjectLockLegalHoldStartWithOn (39.81s)
--- PASS: TestAccAWSS3BucketObject_acl (50.40s)
--- PASS: TestAccAWSS3BucketObject_metadata (48.90s)
--- PASS: TestAccAWSS3BucketPolicy_policyUpdate (41.59s)
--- PASS: TestAccAWSS3Bucket_basic (24.52s)
--- PASS: TestAccAWSS3Bucket_Bucket_EmptyString (25.96s)
--- PASS: TestAccAWSS3BucketObject_ObjectLockLegalHoldStartWithNone (53.22s)
--- PASS: TestAccAWSS3BucketObject_ObjectLockRetentionStartWithNone (52.64s)
--- PASS: TestAccAWSS3Bucket_namePrefix (24.11s)
--- PASS: TestAccAWSS3MultiBucket_withTags (28.72s)
--- PASS: TestAccAWSS3BucketObject_tags (67.07s)
--- PASS: TestAccAWSS3BucketObject_tagsLeadingSlash (66.80s)
--- PASS: TestAccAWSS3Bucket_generatedName (23.94s)
--- PASS: TestAccAWSS3BucketObject_ObjectLockRetentionStartWithSet (64.50s)
--- PASS: TestAccAWSS3BucketPublicAccessBlock_BlockPublicAcls (53.95s)
--- PASS: TestAccAWSS3BucketPublicAccessBlock_IgnorePublicAcls (53.71s)
--- PASS: TestAccAWSS3BucketObject_storageClass (76.53s)
--- PASS: TestAccAWSS3BucketPublicAccessBlock_BlockPublicPolicy (55.08s)
--- PASS: TestAccAWSS3BucketPublicAccessBlock_RestrictPublicBuckets (54.49s)
--- PASS: TestAccAWSS3Bucket_RequestPayer (39.06s)
--- PASS: TestAccAWSS3Bucket_acceleration (42.97s)
--- PASS: TestAccAWSS3Bucket_UpdateAcl (39.30s)
--- PASS: TestAccAWSS3Bucket_shouldFailNotFound (19.73s)
--- PASS: TestAccAWSS3Bucket_enableDefaultEncryption_whenAES256IsUsed (24.57s)
--- PASS: TestAccAWSS3Bucket_AclToGrant (40.27s)
--- PASS: TestAccAWSS3Bucket_tagsWithNoSystemTags (67.02s)
--- PASS: TestAccAWSS3Bucket_Cors_Delete (23.43s)
--- PASS: TestAccAWSS3Bucket_Policy (52.54s)
--- PASS: TestAccAWSS3Bucket_GrantToAcl (41.98s)
--- PASS: TestAccAWSS3Bucket_WebsiteRoutingRules (40.11s)
--- PASS: TestAccAWSS3Bucket_region (60.21s)
--- PASS: TestAccAWSS3Bucket_UpdateGrant (56.40s)
--- PASS: TestAccAWSS3Bucket_disableDefaultEncryption_whenDefaultEncryptionIsEnabled (38.36s)
--- PASS: TestAccAWSS3Bucket_Cors_EmptyOrigin (27.17s)
--- PASS: TestAWSS3BucketName (0.00s)
--- PASS: TestAccAWSS3Bucket_enableDefaultEncryption_whenTypical (45.79s)
--- PASS: TestBucketRegionalDomainName (0.00s)
--- PASS: TestAccAWSS3Bucket_Cors_Update (41.58s)
--- PASS: TestAccAWSS3Bucket_Website_Simple (53.36s)
--- PASS: TestAccAWSS3Bucket_WebsiteRedirect (52.89s)
--- PASS: TestAccAWSS3Bucket_Logging (29.42s)
--- PASS: TestAccAWSS3Bucket_Versioning (46.22s)
--- PASS: TestAccAWSS3Bucket_forceDestroy (18.01s)
--- PASS: TestAccAWSS3Bucket_forceDestroyWithEmptyPrefixes (14.46s)
--- PASS: TestAccAWSS3Bucket_LifecycleExpireMarkerOnly (32.52s)
--- PASS: TestAccAWSS3Bucket_forceDestroyWithObjectLockEnabled (16.24s)
--- PASS: TestAccAWSS3Bucket_objectLock (26.73s)
--- PASS: TestAccAWSS3Bucket_LifecycleBasic (43.35s)
--- PASS: TestAccAWSS3Bucket_tagsWithSystemTags (105.05s)
--- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (43.45s)
--- PASS: TestAccAWSS3Bucket_ReplicationWithoutStorageClass (65.29s)
--- PASS: TestAccAWSS3Bucket_ReplicationWithoutPrefix (73.67s)
--- PASS: TestAccAWSS3Bucket_ReplicationConfiguration_Rule_Destination_AccessControlTranslation (127.63s)
--- PASS: TestAccAWSS3Bucket_ReplicationSchemaV2 (165.38s)
--- PASS: TestAccAWSS3Bucket_Replication (187.17s)

ghost · 2020-03-06T19:26:03Z

This has been released in version 2.52.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-04-04T17:10:17Z

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!

Implementation of acl grants and update in docs

6549e72

ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 9, 2018

bflad added enhancement Requests to existing resources that expand the functionality or scope. service/s3 Issues and PRs that pertain to the s3 service. labels Mar 12, 2018

Chhed13 mentioned this pull request Mar 13, 2018

Support bucket ACLs. #3757

Closed

eredi93 mentioned this pull request Oct 11, 2018

[feature request] Support for complete ACL on S3 buckets #989

Closed

bford21 approved these changes Jan 28, 2019

View reviewed changes

Merge branch 'master' into support-s3-bucket-acl-policies

62694d8

Error check on fallback

844ef9b

bflad reviewed Mar 28, 2019

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Mar 28, 2019

Merge pull request #1 from terraform-providers/master

8d95f64

upstream

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Mar 28, 2019

Merge branch 'master' into support-s3-bucket-acl-policies

4c25528

# Conflicts: # aws/validators.go

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Mar 29, 2019

change grant to schema.HashSet type, re-evaluate grant logic, fix min…

85cb44f

…or comments

jaspervdj-luminal mentioned this pull request Apr 18, 2019

[RM-175] Implementation of acl grants LuminalHQ/terraform-provider-aws#13

Merged

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Jun 19, 2019

aeschright requested a review from a team June 25, 2019 21:34

gdavison self-assigned this Jan 14, 2020

Merge remote-tracking branch 'upstream/master' into support-s3-bucket…

51717d4

…-acl-policies # Conflicts: # aws/resource_aws_s3_bucket_test.go # website/docs/r/s3_bucket.html.markdown

gdavison requested changes Feb 12, 2020

View reviewed changes

s3 bucket grant implementation: fix docs and tests

a5638e4

Chhed13 requested a review from gdavison February 26, 2020 09:26

gdavison requested changes Mar 3, 2020

View reviewed changes

aws/resource_aws_s3_bucket_test.go Outdated Show resolved Hide resolved

aws/resource_aws_s3_bucket_test.go Outdated Show resolved Hide resolved

aws/resource_aws_s3_bucket_test.go Outdated Show resolved Hide resolved

aws/resource_aws_s3_bucket_test.go Outdated Show resolved Hide resolved

s3 bucket grant implementation: fix fmt

b0c2939

gdavison approved these changes Mar 4, 2020

View reviewed changes

gdavison added this to the v2.52.0 milestone Mar 4, 2020

gdavison merged commit fad73a2 into hashicorp:master Mar 4, 2020

gdavison added a commit that referenced this pull request Mar 4, 2020

Update CHANGELOG for #3728

f85b29e

ewbankkit mentioned this pull request Mar 11, 2020

Breaking change for several resources from 2.51.0 → 2.52.0 #12332

Closed

antonbabenko mentioned this pull request Mar 13, 2020

Feature Request - S3 ACL Policy Grant terraform-aws-modules/terraform-aws-s3-bucket#20

Closed

This was referenced Mar 19, 2020

Fixed S3 Permissions incorrect references and typos #12455

Closed

S3 bucket grantee #247

Closed

Support multiple canned ACLs for AWS S3 buckets #144

Open

ghost locked and limited conversation to collaborators Apr 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of acl grants #3728

Implementation of acl grants #3728

Chhed13 commented Mar 9, 2018

Chhed13 commented Aug 29, 2018

antonbabenko commented Sep 5, 2018

Chhed13 commented Sep 5, 2018

antonbabenko commented Sep 5, 2018

eredi93 commented Oct 8, 2018

bflad commented Oct 8, 2018

pauldraper commented Nov 1, 2018 •

edited

Loading

pauldraper commented Nov 22, 2018 •

edited

Loading

tdmalone commented Dec 13, 2018

bflad left a comment

bflad Mar 26, 2019

Chhed13 Mar 31, 2019

Chhed13 Apr 1, 2019

bflad Mar 28, 2019

bflad Mar 28, 2019

Chhed13 Mar 31, 2019

Chhed13 commented Mar 28, 2019

gangadhar01a commented Jun 19, 2019

bostrowski13 commented Jul 23, 2019

gdavison commented Sep 10, 2019 •

edited

Loading

Chhed13 commented Oct 30, 2019

C1PendletonJones commented Feb 7, 2020

gdavison commented Feb 12, 2020 •

edited

Loading

Chhed13 commented Feb 12, 2020

gdavison left a comment

Chhed13 commented Feb 26, 2020

gdavison left a comment

gdavison left a comment

ghost commented Mar 6, 2020

ghost commented Apr 4, 2020

	if i := grantMap["id"].(string); i != "" {
	if i, ok := grantMap["id"].(string); ok && i != "" {

Implementation of acl grants #3728

Implementation of acl grants #3728

Conversation

Chhed13 commented Mar 9, 2018

Chhed13 commented Aug 29, 2018

antonbabenko commented Sep 5, 2018

Chhed13 commented Sep 5, 2018

antonbabenko commented Sep 5, 2018

eredi93 commented Oct 8, 2018

bflad commented Oct 8, 2018

pauldraper commented Nov 1, 2018 • edited Loading

pauldraper commented Nov 22, 2018 • edited Loading

tdmalone commented Dec 13, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad Mar 26, 2019

Choose a reason for hiding this comment

Chhed13 Mar 31, 2019

Choose a reason for hiding this comment

Chhed13 Apr 1, 2019

Choose a reason for hiding this comment

bflad Mar 28, 2019

Choose a reason for hiding this comment

bflad Mar 28, 2019

Choose a reason for hiding this comment

Chhed13 Mar 31, 2019

Choose a reason for hiding this comment

Chhed13 commented Mar 28, 2019

gangadhar01a commented Jun 19, 2019

bostrowski13 commented Jul 23, 2019

gdavison commented Sep 10, 2019 • edited Loading

Chhed13 commented Oct 30, 2019

C1PendletonJones commented Feb 7, 2020

gdavison commented Feb 12, 2020 • edited Loading

Chhed13 commented Feb 12, 2020

gdavison left a comment

Choose a reason for hiding this comment

Chhed13 commented Feb 26, 2020

gdavison left a comment

Choose a reason for hiding this comment

gdavison left a comment

Choose a reason for hiding this comment

ghost commented Mar 6, 2020

ghost commented Apr 4, 2020

pauldraper commented Nov 1, 2018 •

edited

Loading

pauldraper commented Nov 22, 2018 •

edited

Loading

gdavison commented Sep 10, 2019 •

edited

Loading

gdavison commented Feb 12, 2020 •

edited

Loading