-
-
Notifications
You must be signed in to change notification settings - Fork 178
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
feat: add user_policy_document parameter #142
Conversation
This parameter allows the user to specify policies that are applied to the S3 bucket along the policies defined by this module. Closes: cloudposse#115
3f01190
to
93a90d6
Compare
Hi |
Hi Any news about this PR ? Is there a problem preventing a review ? All the best |
Best way to get attention is in #pr-reviews channel in slack |
@dod38fr thank you for the PR, it looks good please run the following commands and commit the changes: make init
make github/init
make readme |
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.
please see comments
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.
Sorry for the delay reviewing this. We were hoping to make these changes and more by now as part of a overhaul, but obviously we have not gotten to it yet.
Please make the requested changes so that this feature works just like source_policy_documents
in terraform-aws-s3-bucket.
main.tf
Outdated
source_policy_documents = var.user_policy_documents | ||
|
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.
Move to separate policy document which overrides the default bucket policy. See s3-bucket implementation for example:
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.
Done.
Unfortunately, I won't be able to test this change on my client's system before October.
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.
I can confirm that the change works as expected on said client's system where we are running the changed version in this PR, thanks @dod38fr!
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.
All set then, @Nuru , can you resume the review ?
All the best
Done. All the best |
/terratest |
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 failing the Terratest. I'm not sure why, something to do with order of operations or waiting. You can compare with our s3-bucket module. It might be a while before I get to look into this further.
@Nuru The terratest job (https://github.com/cloudposse/terraform-aws-tfstate-backend/actions/runs/6303986183) is now marked as successful. I don't really know what went on. I guess that he failure you've seen was a temporary failure and that the job was restarted. In any case, I cannot investigate further as I cannot see any error message. Is there something more I can do ? |
Hi I've successfully run the test that failed in your CI:
I really don't know what can be done to unblock this PR. Any idea ? |
/terratest |
what
This parameter allows the user to specify policies that are applied to the S3 bucket with the policies defined by this module.
why
We want to add policies that forbid non admin users to access the bucket containing tfstates.
This commit allow us to specify a policy that implement these restriction without clobbering the policies put in place by this module.
Note that I have no problem to change the name of this new parameter if you want another.
references
Closes: #115