Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Initial updates for supporting s3 bucket logging targets. #302

Merged
merged 4 commits into from
Apr 15, 2021
Merged

Initial updates for supporting s3 bucket logging targets. #302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
047de2d
Initial updates for supporting s3 bucket logging targets.
jgadling Apr 13, 2021
ada364a
Merge remote-tracking branch 'origin/main' into main
jgadling Apr 13, 2021
275daf0
Formatting.
jgadling Apr 13, 2021
cfc00a7
Merge branch 'main' into jgadling/bucket-logging
jgadling Apr 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions aws-s3-private-bucket/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,13 @@ resource "aws_s3_bucket" "bucket" {
}
}

# TODO
# logging {
# target_bucket = ""
# target_prefix = ""
# }
dynamic "logging" {
for_each = var.logging_bucket == null ? [] : [var.logging_bucket]
content {
target_bucket = var.logging_bucket.name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know if it's better to do each.name/each.prefix, or leave it like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I could go either way as well. I think the each syntax is a little bit more confusing though for an iterator where we'll always have 0 or 1 loops.

target_prefix = var.logging_bucket.prefix
}
}
Comment on lines +113 to +119
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you know what's the difference between this and cloudtrail logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a pretty interesting comparison: https://www.netskope.com/blog/aws-s3-logjam-server-access-logging-vs-object-level-logging

It looks like cloudtrail is more complete and more reliable, but also more complicated to configure.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a table in here that I used to determine that S3 logs was the way forward: https://docs.aws.amazon.com/AmazonS3/latest/userguide/logging-with-S3.html. Ultimately it was a combination of ease to set up (took me just a few minutes to set up and start querying) plus the fact that the S3 logs contain more useful information for us (i.e. Object Size, Total Time, Turn-Around Time, and HTTP Referer).


server_side_encryption_configuration {
rule {
Expand Down
6 changes: 6 additions & 0 deletions aws-s3-private-bucket/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,12 @@ variable "transfer_acceleration" {
default = false
}

variable "logging_bucket" {
type = object({ name = string, prefix = string })
description = "Log bucket name and prefix to enable logs for this bucket"
default = null
}

variable "public_access_block" {
type = bool
default = true
Expand Down