feat(aws-cloudfront-s3): added logS3AccessLogs prop (#506)

* added logS3AccessLogs * updated integ test with no logging bucket * added cfn nag suppress rule for no logging bucket Co-authored-by: biffgaut <[email protected]>
awslabs · Nov 17, 2021 · 6d3c7c9 · 6d3c7c9
1 parent b769f85
commit 6d3c7c9
Show file tree

Hide file tree

Showing 6 changed files with 127 additions and 166 deletions.
diff --git a/source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/README.md b/source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/README.md
@@ -45,12 +45,13 @@ _Parameters_
 
 | **Name**     | **Type**        | **Description** |
 |:-------------|:----------------|-----------------|
-|existingBucketInterface?|[`s3.IBucket`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-s3.IBucket.html)|Existing instance of S3 Bucket object or interface. If this is provided, then also providing bucketProps will cause an error. |
+|existingBucketObj?|[`s3.IBucket`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-s3.IBucket.html)|Existing instance of S3 Bucket object or interface. If this is provided, then also providing bucketProps will cause an error. |
 |bucketProps?|[`s3.BucketProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-s3.BucketProps.html)|Optional user provided props to override the default props for the S3 Bucket.|
 |cloudFrontDistributionProps?|[`cloudfront.DistributionProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-cloudfront.DistributionProps.html)|Optional user provided props to override the default props for CloudFront Distribution|
 |insertHttpSecurityHeaders?|`boolean`|Optional user provided props to turn on/off the automatic injection of best practice HTTP security headers in all responses from CloudFront|
 |loggingBucketProps?|[`s3.BucketProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-s3.BucketProps.html)|Optional user provided props to override the default props for the S3 Logging Bucket.|
 |cloudFrontLoggingBucketProps?|[`s3.BucketProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-s3.BucketProps.html)|Optional user provided props to override the default props for the CloudFront Logging Bucket.|
+|logS3AccessLogs?| boolean|Whether to turn on Access Logging for the S3 bucket. Creates an S3 bucket with associated storage costs for the logs. Enabling Access Logging is a best practice. default - true|
 
 ## Pattern Properties
 

diff --git a/source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/lib/index.ts b/source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/lib/index.ts
@@ -21,77 +21,89 @@ import * as defaults from '@aws-solutions-constructs/core';
  * @summary The properties for the CloudFrontToS3 Construct
  */
 export interface CloudFrontToS3Props {
-   /**
-    * Existing instance of S3 Bucket object, providing both this and `bucketProps` will cause an error.
-    *
-    * @default - None
-    */
-   readonly existingBucketInterface?: s3.IBucket,
-   /**
-    * Optional user provided props to override the default props for the S3 Bucket.
-    *
-    * @default - Default props are used
-    */
-   readonly bucketProps?: s3.BucketProps,
-   /**
-    * Optional user provided props to override the default props
-    *
-    * @default - Default props are used
-    */
-   readonly cloudFrontDistributionProps?: cloudfront.DistributionProps | any,
-   /**
-    * Optional user provided props to turn on/off the automatic injection of best practice HTTP
-    * security headers in all responses from cloudfront
-    *
-    * @default - true
-    */
-   readonly insertHttpSecurityHeaders?: boolean;
-   /**
-    * Optional user provided props to override the default props for the S3 Logging Bucket.
-    *
-    * @default - Default props are used
-    */
-    readonly loggingBucketProps?: s3.BucketProps
-    /**
-     * Optional user provided props to override the default props for the CloudFront Logging Bucket.
-     *
-     * @default - Default props are used
-     */
-    readonly cloudFrontLoggingBucketProps?: s3.BucketProps
- }
+  /**
+   * Existing instance of S3 Bucket object, providing both this and `bucketProps` will cause an error.
+   *
+   * @default - None
+   */
+  readonly existingBucketObj?: s3.IBucket,
+  /**
+   * Optional user provided props to override the default props for the S3 Bucket.
+   *
+   * @default - Default props are used
+   */
+  readonly bucketProps?: s3.BucketProps,
+  /**
+   * Optional user provided props to override the default props
+   *
+   * @default - Default props are used
+   */
+  readonly cloudFrontDistributionProps?: cloudfront.DistributionProps | any,
+  /**
+   * Optional user provided props to turn on/off the automatic injection of best practice HTTP
+   * security headers in all responses from cloudfront
+   *
+   * @default - true
+   */
+  readonly insertHttpSecurityHeaders?: boolean;
+  /**
+   * Optional user provided props to override the default props for the S3 Logging Bucket.
+   *
+   * @default - Default props are used
+   */
+  readonly loggingBucketProps?: s3.BucketProps
+  /**
+   * Optional user provided props to override the default props for the CloudFront Logging Bucket.
+   *
+   * @default - Default props are used
+   */
+  readonly cloudFrontLoggingBucketProps?: s3.BucketProps
+  /**
+   * Whether to turn on Access Logs for the S3 bucket with the associated storage costs.
+   * Enabling Access Logging is a best practice.
+   *
+   * @default - true
+   */
+  readonly logS3AccessLogs?: boolean;
+}
 
 export class CloudFrontToS3 extends Construct {
-     public readonly cloudFrontWebDistribution: cloudfront.Distribution;
-     public readonly cloudFrontFunction?: cloudfront.Function;
-     public readonly cloudFrontLoggingBucket?: s3.Bucket;
-     public readonly s3BucketInterface: s3.IBucket;
-     public readonly s3Bucket?: s3.Bucket;
-     public readonly s3LoggingBucket?: s3.Bucket;
+  public readonly cloudFrontWebDistribution: cloudfront.Distribution;
+  public readonly cloudFrontFunction?: cloudfront.Function;
+  public readonly cloudFrontLoggingBucket?: s3.Bucket;
+  public readonly s3BucketInterface: s3.IBucket;
+  public readonly s3Bucket?: s3.Bucket;
+  public readonly s3LoggingBucket?: s3.Bucket;
+
+  /**
+   * @summary Constructs a new instance of the CloudFrontToS3 class.
+   * @param {cdk.App} scope - represents the scope for all the resources.
+   * @param {string} id - this is a a scope-unique id.
+   * @param {CloudFrontToS3Props} props - user provided props for the construct
+   * @since 0.8.0
+   * @access public
+   */
+  constructor(scope: Construct, id: string, props: CloudFrontToS3Props) {
+    super(scope, id);
+    defaults.CheckProps(props);
+
+    let bucket: s3.IBucket;
 
-     /**
-      * @summary Constructs a new instance of the CloudFrontToS3 class.
-      * @param {cdk.App} scope - represents the scope for all the resources.
-      * @param {string} id - this is a a scope-unique id.
-      * @param {CloudFrontToS3Props} props - user provided props for the construct
-      * @since 0.8.0
-      * @access public
-      */
-     constructor(scope: Construct, id: string, props: CloudFrontToS3Props) {
-       super(scope, id);
-       defaults.CheckProps(props);
+    if (!props.existingBucketObj) {
+      [this.s3Bucket, this.s3LoggingBucket] = defaults.buildS3Bucket(this, {
+        bucketProps: props.bucketProps,
+        loggingBucketProps: props.loggingBucketProps,
+        logS3AccessLogs: props.logS3AccessLogs
+      });
+      bucket = this.s3Bucket;
+    } else {
+      bucket = props.existingBucketObj;
+    }
 
-       if (!props.existingBucketInterface) {
-         [this.s3Bucket, this.s3LoggingBucket] = defaults.buildS3Bucket(this, {
-           bucketProps: props.bucketProps,
-           loggingBucketProps: props.loggingBucketProps
-         });
-         this.s3BucketInterface = this.s3Bucket;
-       } else {
-         this.s3BucketInterface = props.existingBucketInterface;
-       }
+    this.s3BucketInterface = bucket;
 
-       [this.cloudFrontWebDistribution, this.cloudFrontFunction, this.cloudFrontLoggingBucket] =
-           defaults.CloudFrontDistributionForS3(this, this.s3BucketInterface,
-             props.cloudFrontDistributionProps, props.insertHttpSecurityHeaders, props.cloudFrontLoggingBucketProps);
-     }
+    [this.cloudFrontWebDistribution, this.cloudFrontFunction, this.cloudFrontLoggingBucket] =
+      defaults.CloudFrontDistributionForS3(this, this.s3BucketInterface,
+        props.cloudFrontDistributionProps, props.insertHttpSecurityHeaders, props.cloudFrontLoggingBucketProps);
+  }
 }
diff --git a/source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/test/integ.existing-bucket.ts b/source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/test/integ.existing-bucket.ts
@@ -29,7 +29,7 @@ let mybucket: s3.Bucket;
 mybucket = defaults.CreateScrapBucket(stack, { removalPolicy: RemovalPolicy.DESTROY });
 
 const _construct = new CloudFrontToS3(stack, 'test-cloudfront-s3', {
-  existingBucketInterface: mybucket,
+  existingBucketObj: mybucket,
 });
 
 // Add Cache Policy

diff --git a/...atterns/@aws-solutions-constructs/aws-cloudfront-s3/test/integ.no-arguments.expected.json b/...atterns/@aws-solutions-constructs/aws-cloudfront-s3/test/integ.no-arguments.expected.json
@@ -1,90 +1,6 @@
 {
   "Description": "Integration Test for aws-cloudfront-s3",
   "Resources": {
-    "testcloudfronts3S3LoggingBucket90D239DD": {
-      "Type": "AWS::S3::Bucket",
-      "Properties": {
-        "AccessControl": "LogDeliveryWrite",
-        "BucketEncryption": {
-          "ServerSideEncryptionConfiguration": [
-            {
-              "ServerSideEncryptionByDefault": {
-                "SSEAlgorithm": "AES256"
-              }
-            }
-          ]
-        },
-        "PublicAccessBlockConfiguration": {
-          "BlockPublicAcls": true,
-          "BlockPublicPolicy": true,
-          "IgnorePublicAcls": true,
-          "RestrictPublicBuckets": true
-        },
-        "VersioningConfiguration": {
-          "Status": "Enabled"
-        }
-      },
-      "UpdateReplacePolicy": "Delete",
-      "DeletionPolicy": "Delete",
-      "Metadata": {
-        "cfn_nag": {
-          "rules_to_suppress": [
-            {
-              "id": "W35",
-              "reason": "This S3 bucket is used as the access logging bucket for another bucket"
-            }
-          ]
-        }
-      }
-    },
-    "testcloudfronts3S3LoggingBucketPolicy529D4CFF": {
-      "Type": "AWS::S3::BucketPolicy",
-      "Properties": {
-        "Bucket": {
-          "Ref": "testcloudfronts3S3LoggingBucket90D239DD"
-        },
-        "PolicyDocument": {
-          "Statement": [
-            {
-              "Action": "*",
-              "Condition": {
-                "Bool": {
-                  "aws:SecureTransport": "false"
-                }
-              },
-              "Effect": "Deny",
-              "Principal": {
-                "AWS": "*"
-              },
-              "Resource": [
-                {
-                  "Fn::Join": [
-                    "",
-                    [
-                      {
-                        "Fn::GetAtt": [
-                          "testcloudfronts3S3LoggingBucket90D239DD",
-                          "Arn"
-                        ]
-                      },
-                      "/*"
-                    ]
-                  ]
-                },
-                {
-                  "Fn::GetAtt": [
-                    "testcloudfronts3S3LoggingBucket90D239DD",
-                    "Arn"
-                  ]
-                }
-              ],
-              "Sid": "HttpsOnly"
-            }
-          ],
-          "Version": "2012-10-17"
-        }
-      }
-    },
     "testcloudfronts3S3BucketE0C5F76E": {
       "Type": "AWS::S3::Bucket",
       "Properties": {
@@ -110,11 +26,6 @@
             }
           ]
         },
-        "LoggingConfiguration": {
-          "DestinationBucketName": {
-            "Ref": "testcloudfronts3S3LoggingBucket90D239DD"
-          }
-        },
         "PublicAccessBlockConfiguration": {
           "BlockPublicAcls": true,
           "BlockPublicPolicy": true,
@@ -126,7 +37,17 @@
         }
       },
       "UpdateReplacePolicy": "Delete",
-      "DeletionPolicy": "Delete"
+      "DeletionPolicy": "Delete",
+      "Metadata": {
+        "cfn_nag": {
+          "rules_to_suppress": [
+            {
+              "id": "W35",
+              "reason": "This S3 bucket is created for unit/ integration testing purposes only."
+            }
+          ]
+        }
+      }
     },
     "testcloudfronts3S3BucketPolicy250F1F61": {
       "Type": "AWS::S3::BucketPolicy",

diff --git a/source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/test/integ.no-arguments.ts b/source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/test/integ.no-arguments.ts
@@ -15,17 +15,27 @@
 import { App, Stack, RemovalPolicy } from "@aws-cdk/core";
 import { CloudFrontToS3 } from "../lib";
 import { generateIntegStackName } from '@aws-solutions-constructs/core';
+import * as s3 from "@aws-cdk/aws-s3";
+import * as defaults from '@aws-solutions-constructs/core';
 
 // Setup
 const app = new App();
 const stack = new Stack(app, generateIntegStackName(__filename));
 stack.templateOptions.description = 'Integration Test for aws-cloudfront-s3';
 
-new CloudFrontToS3(stack, 'test-cloudfront-s3', {
+const construct = new CloudFrontToS3(stack, 'test-cloudfront-s3', {
   bucketProps: {
     removalPolicy: RemovalPolicy.DESTROY,
-  }
+  },
+  logS3AccessLogs: false
 });
 
+const s3Bucket = construct.s3Bucket as s3.Bucket;
+
+defaults.addCfnSuppressRules(s3Bucket, [
+  { id: 'W35',
+    reason: 'This S3 bucket is created for unit/ integration testing purposes only.' },
+]);
+
 // Synth
 app.synth();
diff --git a/source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/test/test.cloudfront-s3.test.ts b/source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/test/test.cloudfront-s3.test.ts
@@ -89,7 +89,7 @@ test('check existing bucket', () => {
   });
 
   const props: CloudFrontToS3Props = {
-    existingBucketInterface: existingBucket
+    existingBucketObj: existingBucket
   };
 
   new CloudFrontToS3(stack, 'test-cloudfront-s3', props);
@@ -143,7 +143,7 @@ test("Test bad call with existingBucket and bucketProps", () => {
   const app = () => {
     // Helper declaration
     new CloudFrontToS3(stack, "bad-s3-args", {
-      existingBucketInterface: testBucket,
+      existingBucketObj: testBucket,
       bucketProps: {
         removalPolicy: RemovalPolicy.DESTROY
       },
@@ -153,11 +153,11 @@ test("Test bad call with existingBucket and bucketProps", () => {
   expect(app).toThrowError();
 });
 
-test("Test existingBucketInterface", () => {
+test("Test existingBucketObj", () => {
   // Stack
   const stack = new cdk.Stack();
   const construct: CloudFrontToS3 = new CloudFrontToS3(stack, "existingIBucket", {
-    existingBucketInterface: s3.Bucket.fromBucketName(stack, 'mybucket', 'mybucket')
+    existingBucketObj: s3.Bucket.fromBucketName(stack, 'mybucket', 'mybucket')
   });
   // Assertion
   expect(construct.cloudFrontWebDistribution !== null);
@@ -313,4 +313,21 @@ test('Cloudfront logging bucket error when providing existing log bucket and log
   };
 
   expect(app).toThrowError();
+});
+
+// --------------------------------------------------------------
+// s3 bucket with one content bucket and no logging bucket
+// --------------------------------------------------------------
+test('s3 bucket with one content bucket and no logging bucket', () => {
+  const stack = new cdk.Stack();
+
+  const construct = new CloudFrontToS3(stack, 'cloudfront-s3', {
+    bucketProps: {
+      removalPolicy: cdk.RemovalPolicy.DESTROY,
+    },
+    logS3AccessLogs: false
+  });
+
+  expect(stack).toCountResources("AWS::S3::Bucket", 2);
+  expect(construct.s3LoggingBucket).toEqual(undefined);
 });