s3_logging add bucket policy as default access method to target bucket #2108
Conversation
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 3m 52s (non-voting) |
abraverm
force-pushed
the
s3_logging_policy
branch
2 times, most recently
from
caaa494 to
bfb458f
Compare
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 3m 28s (non-voting) |
abraverm
force-pushed
the
s3_logging_policy
branch
from
bfb458f to
414da1f
Compare
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 3m 31s (non-voting) |
There was a problem hiding this comment.
Thank you for taking the time to submit this PR.
Unfortunately, as it stands today I would be a -1 on this PR:
-
I don't see the need for this to be implemented as a breaking change
-
This kind of automatic manipulation of policies is inherently unstable.
For example if someone already used that Sid in the bucket policy then the update will fail. -
It looks like it may still attempt to manipulate the bucket policy even if someone tries to continue using ACL based access
-
The change as written requires additional bucket permissions even if using the old ACL method rather than your new policy based mechanism
What I would be more open to is a mechanism for disabling the setting of the ACL, so the someone can manage the policy directly. We already support managing access policies with amazon.aws.s3_bucket.
I suspect that you're trying to mimic some of the AWS Web-UI features, however, policies are very finicky and trying to do this in Ansible just leads to people opening bugs because it didn't quite manipulate things in the way they expected. This can be especially problematic when people have more complex use-cases.
If you've tried doing more complex things with policies you may have encountered examples where Amazon just disable the "simple" UI because your policy can't be represented that way
| module.warn(json.dumps(updated_acl)) | ||
|
|
There was a problem hiding this comment.
This looks like you've left some debugging behind.
| if target_access == "acl": | ||
| for p in current_policy.get("Statement", []): | ||
| if p != policy_statement: | ||
| new_policy_statements.append(p) | ||
| new_policy["Statement"] = new_policy_statements | ||
| if new_policy == current_policy: | ||
| return False |
There was a problem hiding this comment.
Why is the policy being modified if the access type is acl? It's not like you go back and clean up the policy when logging is disabled.
There was a problem hiding this comment.
Good point that I have missed. I think that these leftovers are harmless and I don't know what is the common practice, but I believe it should cleanup. Should I add a cleanup for when the logging is disabled, or remove it this part
Yeah, I think long term it is better handled in s3_bucket. If we're removing the grant management from s3_logging we'll need to go through a deprecation period anyway since it'll be a breaking change. |
|
Closing in favor of #2136 |
SUMMARY
S3 ACL is disabled by default and AWS documentation encourages using policies.
This change adds policy as the default option and allows switching between ACL and policy.
https://docs.aws.amazon.com/AmazonS3/latest/userguide/enable-server-access-logging.html
ISSUE TYPE
COMPONENT NAME
s3_logging
ADDITIONAL INFORMATION
The module modifies target bucket policy.
Reference:
https://docs.aws.amazon.com/AmazonS3/latest/userguide/enable-server-access-logging.html
https://docs.aws.amazon.com/AmazonS3/latest/userguide/about-object-ownership.html#object-ownership-setting