-
-
Notifications
You must be signed in to change notification settings - Fork 839
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
Adds concept of additional bucket policies #17
Adds concept of additional bucket policies #17
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @asiegman
LGTM, just a few comments.
Also please rebuild README by executing these commands:
make init
make readme/deps
make readme
It will add the new variables and outputs to README.md
automatically.
In general, any changes to README should be made in README.yaml
(not in this case), and after that executing the commands above will rebuild README.yaml
into README.md
and add all new variables and outputs to README.md
thanks
Co-Authored-By: Andriy Knysh <[email protected]>
Co-Authored-By: Andriy Knysh <[email protected]>
@aknysh Thanks for the comments, agreed on both. Updated and READMEs generated. |
@@ -87,9 +87,14 @@ data "aws_iam_policy_document" "bucket_policy" { | |||
} | |||
} | |||
|
|||
module "aggregated_policy" { | |||
source = "git::https://github.com/cloudposse/terraform-aws-iam-policy-document-aggregator.git?ref=tags/0.1.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
count = "${var.enabled == "true" ? 1 : 0}"
let's add count
to this resource as well.
Because if var.enabled
is set to false
, all resources w/o a count=0 will still be created by terraform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit tidier; done. No need for the state to have that intermediate resource if not needed.
bucket = "${join("", aws_s3_bucket.default.*.id)}" | ||
|
||
policy = "${join("", data.aws_iam_policy_document.bucket_policy.*.json)}" | ||
policy = "${module.aggregated_policy.result_document}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
policy = "${module.aggregated_policy.result_document}" | |
policy = "${join("", module.aggregated_policy.*.result_document)}" |
because of the count
, it's now a list, not a single item
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asiegman
just one minor addition
@aknysh it seems count is not valid on module statements? https://travis-ci.org/cloudposse/terraform-aws-s3-bucket/builds/574024219#L245 Did I miss something? |
@asiegman |
What
This allows a user of this module to specify additional arbitrary policies they would like to apply to the bucket.
Why
Currently, the
allow_encrypted_uploads_only
option uses the only attachment available for bucket policies on the s3 bucket. If a user needs to add their own policies, say to enable cross account access to the bucket, they cannot. This allows them to do that.Example usage
Example Output: