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

CloudFrontToS3: Logging buckets shouldn't be created when set to false #1157

Closed
lukepritchard-homstar opened this issue Jul 25, 2024 · 4 comments
Labels
addressed Issue is addressed either through a release or further explanation bug Something isn't working

Comments

@lukepritchard-homstar
Copy link

CloudFrontToS3 allows you to provide two boolean parameters to disable S3 logging, and CloudFront logging:

  • logS3AccessLogs: boolean (default true)
  • logCloudFrontAccessLog: boolean (default true)

When these are set to false, I expect there to be no created S3 buckets for logging purposes. However, this is not the case.

Reproduction Steps

Create the construct with logS3AccessLogs to false, and logCloudFrontAccessLog to false:

import { CloudFrontToS3 } from '@aws-solutions-constructs/aws-cloudfront-s3';

const CloudFronToS3 = new CloudFrontToS3(this, 'CloudFrontToS3', {
      existingBucketObj: siteBucket,
      logS3AccessLogs: false,
      logCloudFrontAccessLog: false,
      cloudFrontDistributionProps: {
         ...
      }
}

Deploying this will result in two logging buckets being created:

image

I expect these two buckets to not have been created in the first place.

Error Log

There is no error log as this is a bug

Environment

  • CDK CLI Version : 2.150.0
  • AWS Solutions Constructs Version : 2.63.0
  • OS : MacOS Sonoma 14.4.1
  • Language : Typescript

This is 🐛 Bug Report

@lukepritchard-homstar lukepritchard-homstar added bug Something isn't working needs-triage The issue or PR still needs to be triaged labels Jul 25, 2024
@biffgaut
Copy link
Contributor

biffgaut commented Aug 7, 2024

The construct can create up to 4 buckets:

  1. The content bucket
  2. A bucket containing access logs for the content bucket
  3. A bucket with logs for CloudFront traffic
  4. A bucket containing access logs for the bucket with CloudFront traffic logs

Setting the two flags you provide to false:

      logS3AccessLogs: false,
      logCloudFrontAccessLog: false,

Should prevent the creation of buckets #2 and 4. Since you provide an existing bucket, setting logS3AccessLogs to false is kind of a NOP. To turn off logging CloudFront access, use the enableLogging flag in cloudFrontDistributionProps:

const CloudFronToS3 = new CloudFrontToS3(this, 'CloudFrontToS3', {
      existingBucketObj: siteBucket,
      logS3AccessLogs: false,
      logCloudFrontAccessLog: false,
      cloudFrontDistributionProps: {
         enableLogging: false
      }
}

This will prevent creation of bucket #3. The construct will also then cease creating bucket #4.

That should satisfy your use case of no logging. This does appear to identify a bug though - if CloudFront logging is enabled (the default) and logCloudFrontAccessLog is false then only bucket #3 should be created and we also see bucket #4 created when running your code. We will look into that- thanks!

@biffgaut
Copy link
Contributor

This bug was fixed in v2.65.0

@biffgaut biffgaut reopened this Oct 1, 2024
@biffgaut
Copy link
Contributor

biffgaut commented Oct 1, 2024

I received an notification about an update to this issue, but I don't see it here - I'm guessing it was then deleted?

It concerned the buckets still being created in the code above. I believe the bucket referenced is the CloudFront traffic log bucket ( bucket #3 above). That's the log of all CloudFront traffic and separate from the two possible S3 Access Log buckets (buckets #2 and #4). CloudFront traffic logging can be turned off in cloudFrontDistributionProps, if you include this in your CloudFrontToS3Props you will not get bucket #324

      cloudFrontDistributionProps: {
         enableLogging: false
      }

I should note that leaving this logging enabled is a best practice and we would strongly recommend against turning it off.

If you deleted the comment because you'd already realized this you can close this Issue again.

@biffgaut biffgaut added addressed Issue is addressed either through a release or further explanation and removed needs-triage The issue or PR still needs to be triaged labels Oct 1, 2024
@lukepritchard-homstar
Copy link
Author

Hey @biffgaut. Thanks for that. I did forget to add in your

      cloudFrontDistributionProps: {
         enableLogging: false
      }

After doing this, it works as intended. Thanks for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addressed Issue is addressed either through a release or further explanation bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants