Logging module for s3 buckets #40
Conversation
| default = "s3/" | ||
| } | ||
|
|
||
| variable "bucket" { |
There was a problem hiding this comment.
since this needs to be unique, i'd suggest you set it inside here instead of relying on the caller do it.
In other situations we used domain name + environment name + some identifying suffix like adhoc.team-shared-logs or something. But the details are up to you
| @@ -0,0 +1,18 @@ | |||
| variable "cdn_logs_path" { | |||
There was a problem hiding this comment.
for these can you a type = string that helps with Terraform 0.12+ to allow type checking of input variables. You get better error messages
| value = "${aws_s3_bucket.logs.bucket_domain_name}" | ||
| } | ||
|
|
||
| output "s3_logs_path" { |
There was a problem hiding this comment.
I'd suggest not passing out the vars as the same. Since the caller passed them in, it already has these values. the more important ones are the stuff you had above where we are passing out "internal' values
| resources = ["arn:aws:s3:::${aws_s3_bucket.logs.bucket}/${var.alb_logs_path}*"] | ||
| principals { | ||
| type = "AWS" | ||
| identifiers = ["arn:aws:iam::156460612806:root"] |
There was a problem hiding this comment.
Are intending to hard code these accounts? I didn't see them in our organization so I'm not sure what they are?
|
|
||
| Usage: | ||
|
|
||
| ```terraform |
There was a problem hiding this comment.
Are you going to include in this PR updates to the different buckets, CloudFronts, and LBs to point to these?
orenfromberg
added
the
ready for review 👀
oldsj
added
specialty: infrastructure
Related to https://github.com/adhocteam/infrastructure/issues/265
Proposed changes:
Acceptance criteria validation
Fulfilled Acceptance Criteria
Tests added to cover the change
Optional Details
Alternate Designs
Possible Drawbacks
Security implications
Requested feedback