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.

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

Greater control over Access Logging #161

Merged
merged 2 commits into from
May 13, 2021
Merged

Greater control over Access Logging #161

merged 2 commits into from
May 13, 2021

Conversation

Nuru
Copy link
Contributor

@Nuru Nuru commented May 13, 2021

what

  • More clearly distinguish between S3 Access Logging and Cloudfront Access Logging
    • Deprecate:
      • access_log_bucket_name
      • logging_enabled
      • log_include_cookies
      • log_prefix
    • Replace with variables prefixed with s3_access_log and cloudfront_access_log to indicate which access logs they apply to
  • Provide independent feature flags for S3 Access Logging and Cloudfront Access Logging
  • Provide ability to configure independent logging object prefixes for S3 Access Logging and Cloudfront Access Logging
  • Provide ability to direct Cloudfront Access Logging to an existing S3 bucket
  • Fix issues causing module to fail when enabled was set to false
  • Add tests

why

  • This module (potentially) creates an S3 Bucket to use as the Cloudfront Origin, and a Cloudfront Distribution. Both S3 Buckets and Cloudfront Distributions provide the ability to generate Access Logs and save them to an S3 bucket. However, the variables in the module referred only to "log" or "logging", which made it non-obvious whether they applied to S3 Access Logs or Cloudfront Access Logs. The new naming largely removes the confusion, while the old variables are still supported for backward compatibility.
  • Explicit flags enabling and disabling logging (rather than implicit based on providing a destination bucket name) avoids Terraform problems with plan depending on results of apply when S3 Bucket is created in the same root module that calls this module.
  • The user was stuck with the prefix this module generated for S3 Access Logs, which may not have been what they wanted.
  • This module always created a new S3 bucket for Cloudfront Access Logging when Cloudfront Access Logging was enabled, which did not allow users to store logs for multiple distributions in the same bucket.
  • All Cloud Posse modules should plan and apply without error and without creating resources when enabled is set to false
  • Ensure module behaves properly when enabled is set to false. Verify that the same S3 Bucket can be supplied for both sets of access logs

@Nuru Nuru requested review from jamengual, nitrocode and korenyoni May 13, 2021 02:49
@Nuru Nuru requested review from a team as code owners May 13, 2021 02:49
@Nuru
Copy link
Contributor Author

Nuru commented May 13, 2021

/test all

@Nuru Nuru merged commit 7e5a104 into master May 13, 2021
@Nuru Nuru deleted the enabled branch May 13, 2021 03:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants