Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

No default value for acl attribute in s3 object #27197

Merged
merged 8 commits into from May 24, 2023
Merged

No default value for acl attribute in s3 object #27197

merged 8 commits into from May 24, 2023

Conversation

lomluca
Copy link
Contributor

@lomluca lomluca commented Oct 11, 2022

Description

Currently, the acl argument is optional for aws_s3_object but defaults to private. This default value could led to issue when using bucket with disabled ACL (BucketOwnerEnforced as object ownership configuration), that is an AWS best practice [1].

Indeed, if you try to upload an object with no specified ACL, a 400 error with the error code AccessControlListNotSupported is returned, because the provider actually tries to set ACL to private.

The workaround, as [1] suggests, is setting the acl attribute to bucket-owner-full-control. I think it's quite confusing to have to specify an ACL to use no ACL; if I don't want to use ACLs, I simply don't specify acl attribute at all.

Relations

References

[1] https://docs.aws.amazon.com/AmazonS3/latest/userguide/about-object-ownership.html

Output from Acceptance Testing

$ make testacc TESTS=TestAccS3Object PKG=s3
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/s3/... -v -count 1 -parallel 20 -run='TestAccS3Object'  -timeout 180m
=== RUN   TestAccS3ObjectCopy_basic
=== PAUSE TestAccS3ObjectCopy_basic
=== RUN   TestAccS3ObjectCopy_BucketKeyEnabled_bucket
=== PAUSE TestAccS3ObjectCopy_BucketKeyEnabled_bucket
=== RUN   TestAccS3ObjectCopy_BucketKeyEnabled_object
=== PAUSE TestAccS3ObjectCopy_BucketKeyEnabled_object
=== RUN   TestAccS3ObjectDataSource_basic
=== PAUSE TestAccS3ObjectDataSource_basic
=== RUN   TestAccS3ObjectDataSource_basicViaAccessPoint
=== PAUSE TestAccS3ObjectDataSource_basicViaAccessPoint
=== RUN   TestAccS3ObjectDataSource_readableBody
=== PAUSE TestAccS3ObjectDataSource_readableBody
=== RUN   TestAccS3ObjectDataSource_kmsEncrypted
=== PAUSE TestAccS3ObjectDataSource_kmsEncrypted
=== RUN   TestAccS3ObjectDataSource_bucketKeyEnabled
=== PAUSE TestAccS3ObjectDataSource_bucketKeyEnabled
=== RUN   TestAccS3ObjectDataSource_allParams
=== PAUSE TestAccS3ObjectDataSource_allParams
=== RUN   TestAccS3ObjectDataSource_objectLockLegalHoldOff
=== PAUSE TestAccS3ObjectDataSource_objectLockLegalHoldOff
=== RUN   TestAccS3ObjectDataSource_objectLockLegalHoldOn
=== PAUSE TestAccS3ObjectDataSource_objectLockLegalHoldOn
=== RUN   TestAccS3ObjectDataSource_leadingSlash
=== PAUSE TestAccS3ObjectDataSource_leadingSlash
=== RUN   TestAccS3ObjectDataSource_multipleSlashes
=== PAUSE TestAccS3ObjectDataSource_multipleSlashes
=== RUN   TestAccS3ObjectDataSource_singleSlashAsKey
=== PAUSE TestAccS3ObjectDataSource_singleSlashAsKey
=== RUN   TestAccS3Object_noNameNoKey
=== PAUSE TestAccS3Object_noNameNoKey
=== RUN   TestAccS3Object_empty
=== PAUSE TestAccS3Object_empty
=== RUN   TestAccS3Object_source
=== PAUSE TestAccS3Object_source
=== RUN   TestAccS3Object_content
=== PAUSE TestAccS3Object_content
=== RUN   TestAccS3Object_etagEncryption
=== PAUSE TestAccS3Object_etagEncryption
=== RUN   TestAccS3Object_contentBase64
=== PAUSE TestAccS3Object_contentBase64
=== RUN   TestAccS3Object_sourceHashTrigger
=== PAUSE TestAccS3Object_sourceHashTrigger
=== RUN   TestAccS3Object_withContentCharacteristics
=== PAUSE TestAccS3Object_withContentCharacteristics
=== RUN   TestAccS3Object_nonVersioned
=== PAUSE TestAccS3Object_nonVersioned
=== RUN   TestAccS3Object_updates
=== PAUSE TestAccS3Object_updates
=== RUN   TestAccS3Object_updateSameFile
=== PAUSE TestAccS3Object_updateSameFile
=== RUN   TestAccS3Object_updatesWithVersioning
=== PAUSE TestAccS3Object_updatesWithVersioning
=== RUN   TestAccS3Object_updatesWithVersioningViaAccessPoint
=== PAUSE TestAccS3Object_updatesWithVersioningViaAccessPoint
=== RUN   TestAccS3Object_kms
=== PAUSE TestAccS3Object_kms
=== RUN   TestAccS3Object_sse
=== PAUSE TestAccS3Object_sse
=== RUN   TestAccS3Object_acl
=== PAUSE TestAccS3Object_acl
=== RUN   TestAccS3Object_metadata
=== PAUSE TestAccS3Object_metadata
=== RUN   TestAccS3Object_storageClass
=== PAUSE TestAccS3Object_storageClass
=== RUN   TestAccS3Object_tags
=== PAUSE TestAccS3Object_tags
=== RUN   TestAccS3Object_tagsLeadingSingleSlash
=== PAUSE TestAccS3Object_tagsLeadingSingleSlash
=== RUN   TestAccS3Object_tagsLeadingMultipleSlashes
=== PAUSE TestAccS3Object_tagsLeadingMultipleSlashes
=== RUN   TestAccS3Object_tagsMultipleSlashes
=== PAUSE TestAccS3Object_tagsMultipleSlashes
=== RUN   TestAccS3Object_objectLockLegalHoldStartWithNone
=== PAUSE TestAccS3Object_objectLockLegalHoldStartWithNone
=== RUN   TestAccS3Object_objectLockLegalHoldStartWithOn
=== PAUSE TestAccS3Object_objectLockLegalHoldStartWithOn
=== RUN   TestAccS3Object_objectLockRetentionStartWithNone
=== PAUSE TestAccS3Object_objectLockRetentionStartWithNone
=== RUN   TestAccS3Object_objectLockRetentionStartWithSet
=== PAUSE TestAccS3Object_objectLockRetentionStartWithSet
=== RUN   TestAccS3Object_objectBucketKeyEnabled
=== PAUSE TestAccS3Object_objectBucketKeyEnabled
=== RUN   TestAccS3Object_bucketBucketKeyEnabled
=== PAUSE TestAccS3Object_bucketBucketKeyEnabled
=== RUN   TestAccS3Object_defaultBucketSSE
=== PAUSE TestAccS3Object_defaultBucketSSE
=== RUN   TestAccS3Object_ignoreTags
=== PAUSE TestAccS3Object_ignoreTags
=== RUN   TestAccS3ObjectsDataSource_basic
=== PAUSE TestAccS3ObjectsDataSource_basic
=== RUN   TestAccS3ObjectsDataSource_basicViaAccessPoint
=== PAUSE TestAccS3ObjectsDataSource_basicViaAccessPoint
=== RUN   TestAccS3ObjectsDataSource_all
=== PAUSE TestAccS3ObjectsDataSource_all
=== RUN   TestAccS3ObjectsDataSource_prefixes
=== PAUSE TestAccS3ObjectsDataSource_prefixes
=== RUN   TestAccS3ObjectsDataSource_encoded
=== PAUSE TestAccS3ObjectsDataSource_encoded
=== RUN   TestAccS3ObjectsDataSource_maxKeys
=== PAUSE TestAccS3ObjectsDataSource_maxKeys
=== RUN   TestAccS3ObjectsDataSource_startAfter
=== PAUSE TestAccS3ObjectsDataSource_startAfter
=== RUN   TestAccS3ObjectsDataSource_fetchOwner
=== PAUSE TestAccS3ObjectsDataSource_fetchOwner
=== CONT  TestAccS3ObjectCopy_basic
=== CONT  TestAccS3Object_objectLockLegalHoldStartWithNone
=== CONT  TestAccS3ObjectDataSource_leadingSlash
=== CONT  TestAccS3Object_tagsLeadingMultipleSlashes
=== CONT  TestAccS3Object_tagsLeadingSingleSlash
=== CONT  TestAccS3Object_updatesWithVersioningViaAccessPoint
=== CONT  TestAccS3ObjectDataSource_singleSlashAsKey
=== CONT  TestAccS3Object_content
=== CONT  TestAccS3Object_contentBase64
=== CONT  TestAccS3Object_etagEncryption
=== CONT  TestAccS3Object_source
=== CONT  TestAccS3Object_sourceHashTrigger
=== CONT  TestAccS3Object_updatesWithVersioning
=== CONT  TestAccS3Object_updateSameFile
=== CONT  TestAccS3Object_updates
=== CONT  TestAccS3Object_nonVersioned
=== CONT  TestAccS3Object_withContentCharacteristics
=== CONT  TestAccS3Object_tagsMultipleSlashes
=== CONT  TestAccS3ObjectDataSource_bucketKeyEnabled
=== CONT  TestAccS3ObjectDataSource_multipleSlashes
=== CONT  TestAccS3Object_nonVersioned
    acctest.go:1233: skipping test; environment variable TF_ACC_ASSUME_ROLE_ARN must be set. Usage: Amazon Resource Name (ARN) of existing IAM Role to assume for testing restricted permissions
--- SKIP: TestAccS3Object_nonVersioned (3.77s)
=== CONT  TestAccS3Object_objectLockRetentionStartWithSet
--- PASS: TestAccS3Object_contentBase64 (64.98s)
=== CONT  TestAccS3ObjectsDataSource_fetchOwner
--- PASS: TestAccS3Object_source (70.52s)
=== CONT  TestAccS3ObjectsDataSource_startAfter
--- PASS: TestAccS3ObjectDataSource_singleSlashAsKey (73.38s)
=== CONT  TestAccS3ObjectsDataSource_maxKeys
--- PASS: TestAccS3Object_withContentCharacteristics (75.58s)
=== CONT  TestAccS3ObjectsDataSource_encoded
--- PASS: TestAccS3ObjectCopy_basic (76.70s)
=== CONT  TestAccS3ObjectsDataSource_prefixes
--- PASS: TestAccS3ObjectDataSource_bucketKeyEnabled (79.61s)
=== CONT  TestAccS3ObjectsDataSource_all
--- PASS: TestAccS3Object_etagEncryption (80.61s)
=== CONT  TestAccS3ObjectsDataSource_basicViaAccessPoint
--- PASS: TestAccS3Object_content (80.87s)
=== CONT  TestAccS3ObjectsDataSource_basic
--- PASS: TestAccS3ObjectDataSource_leadingSlash (110.02s)
=== CONT  TestAccS3Object_ignoreTags
--- PASS: TestAccS3Object_sourceHashTrigger (115.68s)
=== CONT  TestAccS3Object_defaultBucketSSE
--- PASS: TestAccS3ObjectDataSource_multipleSlashes (128.94s)
=== CONT  TestAccS3Object_bucketBucketKeyEnabled
--- PASS: TestAccS3Object_updateSameFile (177.67s)
=== CONT  TestAccS3Object_objectBucketKeyEnabled
--- PASS: TestAccS3Object_updatesWithVersioning (198.75s)
=== CONT  TestAccS3Object_objectLockRetentionStartWithNone
--- PASS: TestAccS3Object_updatesWithVersioningViaAccessPoint (213.41s)
=== CONT  TestAccS3ObjectDataSource_basicViaAccessPoint
--- PASS: TestAccS3ObjectsDataSource_encoded (164.13s)
=== CONT  TestAccS3Object_objectLockLegalHoldStartWithOn
--- PASS: TestAccS3ObjectsDataSource_all (162.36s)
=== CONT  TestAccS3ObjectDataSource_kmsEncrypted
--- PASS: TestAccS3Object_updates (245.23s)
=== CONT  TestAccS3ObjectDataSource_objectLockLegalHoldOff
--- PASS: TestAccS3ObjectsDataSource_fetchOwner (180.51s)
=== CONT  TestAccS3ObjectDataSource_readableBody
--- PASS: TestAccS3Object_objectBucketKeyEnabled (68.65s)
=== CONT  TestAccS3ObjectDataSource_objectLockLegalHoldOn
--- PASS: TestAccS3ObjectsDataSource_prefixes (170.74s)
=== CONT  TestAccS3Object_empty
--- PASS: TestAccS3Object_bucketBucketKeyEnabled (120.92s)
=== CONT  TestAccS3ObjectCopy_BucketKeyEnabled_object
--- PASS: TestAccS3ObjectsDataSource_basic (177.62s)
=== CONT  TestAccS3ObjectDataSource_allParams
--- PASS: TestAccS3ObjectsDataSource_basicViaAccessPoint (183.32s)
=== CONT  TestAccS3ObjectDataSource_basic
--- PASS: TestAccS3Object_objectLockLegalHoldStartWithNone (265.11s)
=== CONT  TestAccS3Object_noNameNoKey
--- PASS: TestAccS3Object_noNameNoKey (1.29s)
=== CONT  TestAccS3Object_metadata
--- PASS: TestAccS3ObjectsDataSource_startAfter (198.14s)
=== CONT  TestAccS3Object_tags
--- PASS: TestAccS3ObjectsDataSource_maxKeys (200.40s)
=== CONT  TestAccS3ObjectCopy_BucketKeyEnabled_bucket
--- PASS: TestAccS3Object_defaultBucketSSE (159.60s)
=== CONT  TestAccS3Object_storageClass
--- PASS: TestAccS3Object_tagsLeadingMultipleSlashes (281.41s)
=== CONT  TestAccS3Object_sse
--- PASS: TestAccS3Object_objectLockRetentionStartWithSet (284.70s)
=== CONT  TestAccS3Object_acl
--- PASS: TestAccS3Object_tagsLeadingSingleSlash (290.31s)
=== CONT  TestAccS3Object_kms
--- PASS: TestAccS3Object_ignoreTags (180.66s)
--- PASS: TestAccS3ObjectCopy_BucketKeyEnabled_object (60.31s)
--- PASS: TestAccS3Object_empty (69.18s)
--- PASS: TestAccS3ObjectDataSource_kmsEncrypted (89.55s)
--- PASS: TestAccS3Object_tagsMultipleSlashes (343.57s)
--- PASS: TestAccS3ObjectDataSource_basic (80.80s)
--- PASS: TestAccS3ObjectDataSource_readableBody (103.14s)
--- PASS: TestAccS3ObjectDataSource_allParams (93.26s)
--- PASS: TestAccS3ObjectDataSource_basicViaAccessPoint (139.61s)
--- PASS: TestAccS3ObjectDataSource_objectLockLegalHoldOn (106.85s)
--- PASS: TestAccS3ObjectDataSource_objectLockLegalHoldOff (115.98s)
--- PASS: TestAccS3Object_kms (78.72s)
--- PASS: TestAccS3Object_sse (92.79s)
--- PASS: TestAccS3Object_objectLockLegalHoldStartWithOn (134.94s)
--- PASS: TestAccS3Object_objectLockRetentionStartWithNone (196.06s)
--- PASS: TestAccS3ObjectCopy_BucketKeyEnabled_bucket (127.49s)
--- PASS: TestAccS3Object_metadata (153.79s)
--- PASS: TestAccS3Object_acl (153.14s)
--- PASS: TestAccS3Object_tags (197.51s)
--- PASS: TestAccS3Object_storageClass (210.86s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/s3	504.039s

@github-actions github-actions bot added documentation Introduces or discusses updates to documentation. service/s3 Issues and PRs that pertain to the s3 service. needs-triage Waiting for first response or review from a maintainer. size/XS Managed by automation to categorize the size of a PR. labels Oct 11, 2022
@github-actions
Copy link

Community Note

Voting for Prioritization

  • Please vote on this pull request by adding a 👍 reaction to the original post to help the community and maintainers prioritize this pull request.
  • Please see our prioritization guide for information on how we prioritize.
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

For Submitters

  • Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
  • For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
  • Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

@lomluca lomluca changed the title [WIP] No default value for acl attribute in s3 object No default value for acl attribute in s3 object Oct 11, 2022
@lomluca lomluca marked this pull request as ready for review October 11, 2022 15:26
@breathingdust breathingdust added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Oct 11, 2022
@breathingdust breathingdust added this to the v5.0.0 milestone Apr 11, 2023
@jar-b jar-b self-assigned this May 23, 2023
@github-actions github-actions bot added size/S Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/XS Managed by automation to categorize the size of a PR. labels May 24, 2023
jar-b
jar-b approved these changes May 24, 2023
View reviewed changes
Copy link
Member

@jar-b jar-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

$ make testacc PKG=s3 TESTS=TestAccS3Object_
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/s3/... -v -count 1 -parallel 20 -run='TestAccS3Object_'  -timeout 180m

--- SKIP: TestAccS3Object_nonVersioned (0.52s)
=== CONT  TestAccS3Object_storageClass
--- PASS: TestAccS3Object_noNameNoKey (5.40s)
=== CONT  TestAccS3Object_tags
--- PASS: TestAccS3Object_objectBucketKeyEnabled (32.97s)
=== CONT  TestAccS3Object_metadata
--- PASS: TestAccS3Object_bucketBucketKeyEnabled (33.26s)
=== CONT  TestAccS3Object_etagEncryption
--- PASS: TestAccS3Object_defaultBucketSSE (33.88s)
=== CONT  TestAccS3Object_withContentCharacteristics
--- PASS: TestAccS3Object_sse (38.46s)
=== CONT  TestAccS3Object_sourceHashTrigger
--- PASS: TestAccS3Object_kms (38.81s)
=== CONT  TestAccS3Object_contentBase64
--- PASS: TestAccS3Object_withContentCharacteristics (28.12s)
=== CONT  TestAccS3Object_source
--- PASS: TestAccS3Object_objectLockLegalHoldStartWithOn (63.94s)
=== CONT  TestAccS3Object_content
--- PASS: TestAccS3Object_etagEncryption (34.24s)
=== CONT  TestAccS3Object_empty
--- PASS: TestAccS3Object_contentBase64 (31.23s)
--- PASS: TestAccS3Object_updateSameFile (72.38s)
--- PASS: TestAccS3Object_ignoreTags (72.95s)
--- PASS: TestAccS3Object_updatesWithVersioningViaAccessPoint (73.60s)
--- PASS: TestAccS3Object_updatesWithVersioning (74.08s)
--- PASS: TestAccS3Object_updates (76.11s)
--- PASS: TestAccS3Object_objectLockLegalHoldStartWithNone (87.17s)
--- PASS: TestAccS3Object_source (29.24s)
--- PASS: TestAccS3Object_content (28.78s)
--- PASS: TestAccS3Object_objectLockRetentionStartWithNone (94.23s)
--- PASS: TestAccS3Object_sourceHashTrigger (55.99s)
--- PASS: TestAccS3Object_empty (28.31s)
--- PASS: TestAccS3Object_acl (97.91s)
--- PASS: TestAccS3Object_metadata (69.96s)
--- PASS: TestAccS3Object_objectLockRetentionStartWithSet (107.36s)
--- PASS: TestAccS3Object_tagsLeadingSingleSlash (110.76s)
--- PASS: TestAccS3Object_tagsMultipleSlashes (111.38s)
--- PASS: TestAccS3Object_tags (106.37s)
--- PASS: TestAccS3Object_tagsLeadingMultipleSlashes (111.90s)
--- PASS: TestAccS3Object_storageClass (111.67s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/s3 115.383s
$ make testacc PKG=s3 TESTS=TestAccS3ObjectCopy_
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/s3/... -v -count 1 -parallel 20 -run='TestAccS3ObjectCopy_'  -timeout 180m

--- PASS: TestAccS3ObjectCopy_BucketKeyEnabled_object (18.03s)
--- PASS: TestAccS3ObjectCopy_BucketKeyEnabled_bucket (18.07s)
--- PASS: TestAccS3ObjectCopy_basic (18.88s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/s3 22.281s

@jar-b jar-b merged commit 554b904 into hashicorp:main May 24, 2023
45 checks passed
@jar-b
Copy link
Member

jar-b commented May 24, 2023

Thanks for your contribution, @lomluca! 👏

@github-actions
Copy link

This functionality has been released in v5.0.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. Thank you!

@trevorrea trevorrea mentioned this pull request Jun 5, 2023
Open
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jar-b jar-b jar-b approved these changes

Assignees

@jar-b jar-b

Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/s3 Issues and PRs that pertain to the s3 service. size/S Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
v5.0.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lomluca @jar-b @breathingdust