Enable logging with private s3 bucket #210
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jgadling
requested changes
Jul 3, 2020
aws-s3-private-bucket/main.tf
Outdated
| @@ -1,6 +1,8 @@ | |||
| locals { | |||
| # If grants are defined, we use `grant` to grant permissions, otherwise it will use the `acl` to grant permissions | |||
| acl = length(var.grants) == 0 ? "private" : null | |||
| acl = length(var.grants) == 0 ? "private" : ( | |||
| var.log_delivery_write_acl_enable ? "log-delivery-write" : null | |||
There was a problem hiding this comment.
Think this needs a fix -- the log delivery flag will only work here if we have grants AND log_delivery_write_acl_enable set.
There was a problem hiding this comment.
redid, but had to put in a try because hashicorp/terraform#25014
There was a problem hiding this comment.
I did find this: hashicorp/terraform#22131 (comment), which is what I had previously.
jgadling
requested changes
Jul 7, 2020
|
Ok, this looks like a good start to me -- let's test it in the cellxgene dev environment before merging. |
ryanking
suggested changes
Jul 7, 2020
aws-s3-private-bucket/variables.tf
Outdated
| variable log_delivery_write_acl_enable { | ||
| type = bool | ||
| default = false | ||
| description = "Enables logging" |
There was a problem hiding this comment.
Can we have a more descriptive description?
jgadling
requested changes
Jul 8, 2020
aws-cloudfront-logs-bucket/main.tf
Outdated
| # Define the grant ACL for the Cloudfront logging S3 bucket, | ||
| # In order for the awslogsdelivery account to write log files to the bucket, | ||
| # we need to grant the AWS log delivery group the FULL_CONTROL access to the logging bucket | ||
| # LP's AWS account also has the FULL_CONTROL access to the bucket, this is specified by the canonical user id |
There was a problem hiding this comment.
This comment is confusing -- please remove the reference to LP
jgadling
approved these changes
Jul 8, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This doc describes that if you remove permissions for the
awslogsdeliveryaccount, CloudFront won't be able to save logs to the S3 bucket. Terraform is wiping this out every time we update s3 buckets. We have to explicitly create grants toawslogsdelivery. Inspired by: https://github.com/FB-PLP/terraform-infra-management/pull/386, creating a module for this so that it will be easier to configure.Test Plan
Updating a module that is referencing this module and seeing if logs show up (since they have disappeared when buckets were updated last). Tests in go.