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.

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

Add S3 bucket ownership controls #61

Merged
merged 16 commits into from
Oct 29, 2021
Merged

Conversation

max-lobur
Copy link
Contributor

@max-lobur max-lobur commented Oct 13, 2021

what

  • Set aws_s3_bucket_ownership_controls to BucketOwnerPreferred

why

  • Per docs this setting will let object uploader decide ownership. If bucket-owner-full-control ACL is specified, the bucket account take ownership, otherwise the writer account keeps ownership. Bucket on its side may enforce presence of the bucket-owner-full-control ACL which we already do when needed. So this setting was the only missing piece to make ownership work like we expected
  • I found no use cases for the other value of this resource: ObjectWriter. It corresponds to legacy S3 behavior which was broken for us. Decided to always set to BucketOwnerPreferred and not allow to customize. If the owner must be preserved, we should not pass bucket-owner-full-control - this is a more clear way to control the behavior.

references

@max-lobur max-lobur requested review from a team as code owners October 13, 2021 14:09
@max-lobur max-lobur requested a review from a team as a code owner October 13, 2021 14:10
max-lobur added a commit to cloudposse/terraform-aws-cloudtrail-s3-bucket that referenced this pull request Oct 13, 2021
max-lobur added a commit to cloudposse/terraform-aws-cloudtrail-s3-bucket that referenced this pull request Oct 13, 2021
max-lobur added a commit to cloudposse/terraform-aws-config-storage that referenced this pull request Oct 13, 2021
max-lobur added a commit to cloudposse/terraform-aws-lb-s3-bucket that referenced this pull request Oct 13, 2021
@max-lobur
Copy link
Contributor Author

/test all

versions.tf Outdated Show resolved Hide resolved
versions.tf Outdated Show resolved Hide resolved
@nitrocode
Copy link
Member

this module should probably be using the cloudposse s3 bucket module so we don't have to duplicate these pr changes across these two repos

Co-authored-by: nitrocode <nitrocode@users.noreply.github.com>
@max-lobur
Copy link
Contributor Author

Agreed, it was discussed already. I will merge separately. There are some drifts between two already, so have to be careful

@max-lobur
Copy link
Contributor Author

Also going to be breaking change as tf resource address will change

max-lobur and others added 2 commits October 29, 2021 14:40
Co-authored-by: nitrocode <nitrocode@users.noreply.github.com>
@max-lobur
Copy link
Contributor Author

/test all

@max-lobur max-lobur merged commit f1b6ec3 into cloudposse:master Oct 29, 2021
max-lobur added a commit to cloudposse/terraform-aws-lb-s3-bucket that referenced this pull request Oct 29, 2021
* Make use of S3 bucket ownership

Inherit cloudposse/terraform-aws-s3-log-storage#61

* Auto Format

* Update versions.tf

* Auto Format

Co-authored-by: cloudpossebot <11232728+cloudpossebot@users.noreply.github.com>
Co-authored-by: nitrocode <nitrocode@users.noreply.github.com>
max-lobur added a commit to cloudposse/terraform-aws-config-storage that referenced this pull request Oct 29, 2021
* Make use of S3 bucket ownership

Inherit cloudposse/terraform-aws-s3-log-storage#61

* Auto Format

* Update versions.tf

* Auto Format

Co-authored-by: cloudpossebot <11232728+cloudpossebot@users.noreply.github.com>
Co-authored-by: nitrocode <nitrocode@users.noreply.github.com>
max-lobur added a commit to cloudposse/terraform-aws-cloudtrail-s3-bucket that referenced this pull request Oct 29, 2021
* Update main.tf

Inherit cloudposse/terraform-aws-s3-log-storage#61

* Auto Format

* Update versions.tf

* Auto Format

Co-authored-by: cloudpossebot <11232728+cloudpossebot@users.noreply.github.com>
Co-authored-by: nitrocode <nitrocode@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants