awslabs / aws-solutions-constructs

The AWS Solutions Constructs Library is an open-source extension of the AWS Cloud Development Kit (AWS CDK) that provides multi-service, well-architected patterns for quickly defining solutions
https://docs.aws.amazon.com/solutions/latest/constructs/
Apache License 2.0
1.24k stars 249 forks source link

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

Closed lukepritchard-homstar closed 1 month ago

lukepritchard-homstar commented 3 months ago

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

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


This is :bug: Bug Report

biffgaut commented 2 months ago

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 commented 1 month ago

This bug was fixed in v2.65.0

biffgaut commented 1 month ago

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.

lukepritchard-homstar commented 1 month ago

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!