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

add object ownership controls options for s3 bucket #311

Merged
merged 22 commits into from
May 4, 2021
Merged

add object ownership controls options for s3 bucket #311

merged 22 commits into from
May 4, 2021

Conversation

abikouo
Copy link
Contributor

@abikouo abikouo commented Mar 29, 2021 •
edited by goneri
Loading

Depends-On: #345

SUMMARY

This feature would allow the bucket settting "Object Ownership" to be changed from the default of "Object writer" to "Bucket owner preferred". In conjunction with a bucket policy condition, this allows a bucket owner to ensure that all objects uploaded to a bucket are always owned by them.

#245

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

@ansibullbot
Copy link

cc @jillr @s-hertel @tremble @wimnat
click here for bot help

@ansibullbot ansibullbot added community_review feature This issue/PR relates to a feature request integration tests/integration module module needs_triage plugins plugin (any type) tests tests needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels Mar 29, 2021
tremble
tremble requested changes Mar 29, 2021
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

You've fallen foul of the sanity linter. We generally follow PEP8 as a formatting guide when it comes to spacing.

plugins/modules/s3_bucket.py Outdated Show resolved Hide resolved
plugins/modules/s3_bucket.py Outdated Show resolved Hide resolved
plugins/modules/s3_bucket.py Outdated Show resolved Hide resolved
plugins/modules/s3_bucket.py Show resolved Hide resolved
plugins/modules/s3_bucket.py Outdated Show resolved Hide resolved
plugins/modules/s3_bucket.py Outdated Show resolved Hide resolved
plugins/modules/s3_bucket.py Outdated Show resolved Hide resolved
plugins/modules/s3_bucket.py Outdated Show resolved Hide resolved
plugins/modules/s3_bucket.py Outdated Show resolved Hide resolved
plugins/modules/s3_bucket.py Outdated Show resolved Hide resolved
abikouo and others added 9 commits March 29, 2021 14:40
@abikouo @tremble
Update plugins/modules/s3_bucket.py
4e1e591 
Co-authored-by: Mark Chappell <[email protected]>
@abikouo @tremble
Update plugins/modules/s3_bucket.py
d646ec3 
Co-authored-by: Mark Chappell <[email protected]>
@abikouo @tremble
Update plugins/modules/s3_bucket.py
b701235 
Co-authored-by: Mark Chappell <[email protected]>
@abikouo @tremble
Update plugins/modules/s3_bucket.py
72fe77d 
Co-authored-by: Mark Chappell <[email protected]>
@abikouo @tremble
Update plugins/modules/s3_bucket.py
f19b4a8 
Co-authored-by: Mark Chappell <[email protected]>
@abikouo @tremble
Update plugins/modules/s3_bucket.py
b0370ec 
Co-authored-by: Mark Chappell <[email protected]>
@abikouo @tremble
Update plugins/modules/s3_bucket.py
61e206d 
Co-authored-by: Mark Chappell <[email protected]>
@abikouo @tremble
Update plugins/modules/s3_bucket.py
7dec11c 
Co-authored-by: Mark Chappell <[email protected]>
@abikouo @tremble
Update plugins/modules/s3_bucket.py
c33bbda 
Co-authored-by: Mark Chappell <[email protected]>
tremble
tremble requested changes Mar 29, 2021
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

More whitespace cleanup

plugins/modules/s3_bucket.py Outdated Show resolved Hide resolved
plugins/modules/s3_bucket.py Outdated Show resolved Hide resolved
plugins/modules/s3_bucket.py Outdated Show resolved Hide resolved
plugins/modules/s3_bucket.py Outdated Show resolved Hide resolved
plugins/modules/s3_bucket.py Outdated Show resolved Hide resolved
tremble
tremble requested changes Mar 29, 2021
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

even more...

plugins/modules/s3_bucket.py Show resolved Hide resolved
plugins/modules/s3_bucket.py Show resolved Hide resolved
@tremble
Copy link
Contributor

tremble commented Mar 29, 2021

We'll need a change to the storage permissions for our CI - https://github.com/mattclay/aws-terminator

Specifically https://github.com/mattclay/aws-terminator/blob/master/aws/policy/storage-services.yaml

@abikouo
Copy link
Contributor Author

abikouo commented Mar 30, 2021

S3 policy updated PR https://github.com/mattclay/aws-terminator/pull/142/files

@goneri
Copy link
Member

goneri commented Apr 23, 2021

I've yet to take a look at the PR.

@goneri goneri mentioned this pull request Apr 26, 2021
Merged
@goneri
Copy link
Member

goneri commented Apr 26, 2021

recheck

4 similar comments
@goneri
Copy link
Member

goneri commented Apr 26, 2021

recheck

@goneri
Copy link
Member

goneri commented Apr 26, 2021

recheck

@jillr
Copy link
Collaborator

jillr commented Apr 26, 2021

recheck

@goneri
Copy link
Member

goneri commented Apr 27, 2021

recheck

@abikouo abikouo requested review from goneri, tremble and jillr May 3, 2021 07:36
jillr
jillr reviewed May 3, 2021
View reviewed changes
Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

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

LGTM when the version_added is updated

plugins/modules/s3_bucket.py Outdated Show resolved Hide resolved
@abikouo abikouo requested a review from jillr May 4, 2021 07:56
tremble
tremble requested changes May 4, 2021
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

Just a minor nit. Once the extra deletion test is in place LGTM

tests/integration/targets/s3_bucket/roles/s3_bucket/tasks/ownership_controls.yml Show resolved Hide resolved
@abikouo abikouo requested a review from tremble May 4, 2021 08:16
jillr
jillr approved these changes May 4, 2021
View reviewed changes
Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

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

Thanks for all your work on this @abikouo! tremble's last comment is addressed in this latest changeset so I'm going to gate this one.

@jillr jillr added the gate label May 4, 2021
@ansible-zuul ansible-zuul bot merged commit 0fc4761 into ansible-collections:main May 4, 2021
@tremble tremble mentioned this pull request May 5, 2021
Closed
@PayBas
Copy link

PayBas commented Oct 31, 2021

For those using amazon.aws.s3_bucket on Minio servers. Stick with amazon.aws:1.5.1 for now.
Using version 2.0.0 of this collection will result in:

botocore.exceptions.ClientError: An error occurred (NotImplemented) when calling the GetBucketOwnershipControls operation: A header you provided implies functionality that is not implemented

It's due to this PR ;)

@tremble
Copy link
Contributor

tremble commented Oct 31, 2021

@PayBas The issue you're referring to has been fixed by #390 we hope to have release 2.1.0 out in a couple of weeks.

abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
@PandaWill
Fix for aws_kms_info with external/custom key store keys (ansible-col…
9bad65c 
…lections#311)

* Fix for aws_kms_info with external/custom key store keys
* Added changelog fragment
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
@PandaWill @abikouo
Fix for aws_kms_info with external/custom key store keys (ansible-col…
0a55eba 
…lections#311)

* Fix for aws_kms_info with external/custom key store keys
* Added changelog fragment
@abikouo abikouo deleted the 245 branch October 24, 2023 14:38
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Oct 24, 2023
@PandaWill
Fix for aws_kms_info with external/custom key store keys (ansible-col…
44ad137 
…lections#311)

* Fix for aws_kms_info with external/custom key store keys
* Added changelog fragment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jillr jillr jillr approved these changes

@goneri goneri Awaiting requested review from goneri

@tremble tremble Awaiting requested review from tremble

Assignees

@goneri goneri

Labels
community_review feature This issue/PR relates to a feature request integration tests/integration module module plugins plugin (any type) tests tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@abikouo @ansibullbot @tremble @jillr @goneri @PayBas