do not create cloudfront log bucket if disabled #303

naseemkullah · 2021-08-04T13:58:45Z

*Issue #, if available: #302

Description of changes:

Avoid creating a cloudfront log bucket if cloudfront logging is disabled.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

aws-solutions-constructs-team · 2021-08-04T14:01:10Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 55e9b0e
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2021-08-04T14:23:18Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 8cc2f8c
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

naseemkullah · 2021-08-04T14:33:47Z

are the AWS CodeBuild logs publicly viewable? If not @biffgaut could you please let me know what they say?

aws-solutions-constructs-team · 2021-08-07T02:13:32Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 22db0f3
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2021-08-07T02:31:25Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: a74e36c
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2021-08-07T02:50:38Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: d373bdc
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2021-08-07T03:44:19Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 43453cb
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

naseemkullah · 2021-08-07T12:23:35Z

source/patterns/@aws-solutions-constructs/core/test/cloudfront-distribution-s3-helper.test.ts

+    enableLogging: false,
+  });
+
+  expect(stack).toHaveResourceLike("AWS::CloudFront::Distribution", {


This does not explicitly test for the absence of Logging prop which would be better, is there a test checking for absence elsewhere in the codebase?

I believe this command looks for an identical resource, so if a logging prop exists it will fail. What we are missing (and what would have prevented this issue) is a check that the logging bucket does not exist. You could add that with an expect(stack).not.toHaveResourceLike("AWS::S3::Bucket", { _define logging bucket here_ }); call if you're willing to add that...

aws-solutions-constructs-team · 2021-08-07T12:26:20Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 56fbab8
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2021-08-07T13:09:10Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 2e194ce
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2021-08-07T14:13:45Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 0c958ba
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2021-08-07T14:30:01Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 6fae6e9
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2021-08-07T18:41:27Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: c878d60
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2021-08-07T19:04:04Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 5638074
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2021-08-07T19:08:49Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: e8c5ca0
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2021-08-07T19:11:57Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 03545cc
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2021-08-07T19:23:33Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: a86d17a
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2021-08-12T12:23:35Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 4133f4f
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

biffgaut · 2021-08-12T13:38:53Z

Is there an AWS account I could have access to to run the integration test's deployments against?

You make an excellent point, I don't think we had considered that before. We will definitely update the docs, although I don't think making an account available is going to possible. Perhaps some way of generating the .expected files on our end during the pipeline. Hand editing can make them pass, we've had trouble in the past when hand edits masked the fact the the construct didn't launch - but we have new testing that launches all the constructs every night so that's no longer a concern.

We need to do a better job overall of describing the steps involved in contributing - we're still pretty new and improving those steps, but it's definitely something on our (too long) todo list.

aws-solutions-constructs-team · 2021-08-12T15:00:09Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 4e878c7
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

biffgaut

A couple questions within...

biffgaut · 2021-08-12T15:33:04Z

source/patterns/@aws-solutions-constructs/core/lib/cloudfront-distribution-helper.ts

@@ -62,33 +62,18 @@ function defaultCloudfrontFunction(scope: cdk.Construct): cloudfront.Function {
 export function CloudFrontDistributionForApiGateway(scope: cdk.Construct,
  apiEndPoint: api.RestApi,
  cloudFrontDistributionProps?: cloudfront.DistributionProps | any,
-  httpSecurityHeaders?: boolean): [cloudfront.Distribution,
+  httpSecurityHeaders: boolean = true): [cloudfront.Distribution,


Big fan of this !

biffgaut · 2021-08-12T15:39:35Z

source/patterns/@aws-solutions-constructs/core/lib/cloudfront-distribution-helper.ts

+
+function getLoggingBucket(cloudFrontDistributionProps: cloudfront.DistributionProps | any, scope: cdk.Construct): s3.Bucket | undefined {
+  return cloudFrontDistributionProps?.enableLogging !== false


Parsing this logic and comparing it to the original logic had me doing my homework on truthy, falsey and missing values...

hehe, sorry I should have probably explained it in more depth, though I had gotten engulfed with the snapshot updates

It's much easier to follow than the original code - my struggles were determining the behavior of the original code and comparing it to this.

I guess this is where the extensive test suite pays off! :)

biffgaut · 2021-08-12T15:50:30Z

source/patterns/@aws-solutions-constructs/core/test/cloudfront-distribution-s3-helper.test.ts

+    enableLogging: false,
+  });
+
+  expect(stack).toHaveResourceLike("AWS::CloudFront::Distribution", {


I believe this command looks for an identical resource, so if a logging prop exists it will fail. What we are missing (and what would have prevented this issue) is a check that the logging bucket does not exist. You could add that with an expect(stack).not.toHaveResourceLike("AWS::S3::Bucket", { _define logging bucket here_ }); call if you're willing to add that...

biffgaut · 2021-08-12T15:59:33Z

source/patterns/@aws-solutions-constructs/core/test/cloudfront-distribution-s3-helper.test.ts

@@ -58,7 +58,7 @@ test('check bucket metadata', () => {
        rules_to_suppress: [
          {
            id: "W35",
-            reason: "This S3 bucket is used as the access logging bucket for CloudFront Distribution"
+            reason: "This S3 bucket is used as the access logging bucket for another bucket"


I don't think we want to do this. This is finding the logging bucket set up in buildS3Bucket(), which a resource by the test infrastructure and doesn't tell us anything about construct behavior. As I see it, logging should still be the default behavior, so there should still be a logging bucket with this suppression rule.

good catch, I believe I got the getLoggingBucket() logic wrong, fixing...

OK, the updated logic did not change the test result apparently. Could you elaborate as to whats going on here in terms of rules_to_suppress ?

We run a tool called cfn_nag that looks for security concerns in cloudformation templates (in this case, it looks at test snapshots). It's an external tool that doesn't allow us to ignore specific rules in configuration so we need to add metadata to the CFN to remove false positives. There are 2 buckets in the construct, both with metadata suppressing W35 - one is test infrastructure ("...another bucket...") and another that is part of the construct ("...CloudFront distribution..."). So either statement should pass expect statement, but the latter is looking at the construct and evaluating what we are looking for.

Thanks for the explanation! I changed it back, not sure why I had changed it in the first place, there has been many iterations as you know 😅

aws-solutions-constructs-team · 2021-08-14T21:38:14Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 61ae0c8
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2021-08-14T21:57:29Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: e2115e6
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2021-08-14T22:11:02Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: bc25f6b
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

naseemkullah · 2021-08-15T00:26:56Z

Getting the following error which is unrelated AFAICT:

module     level     offender                                                                                     description               mitigation            
---------  --------  -------------------------------------------------------------------------------------------  ------------------------  ----------------------
files-aws  critical  source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/test/test.cloudfront-s3.test.ts  Potential AWS Account Id  Check line number: 125

offending line was checked in 10 months ago: offending line is

aws-solutions-constructs/source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/test/test.cloudfront-s3.test.ts

Line 123 in d96c11b

    
           const certificate = acm.Certificate.fromCertificateArn(stack, 'Cert', 'arn:aws:acm:us-east-1:123456789012:certificate/12345678-1234-1234-1234-123456789012');

:

biffgaut · 2021-08-16T17:36:59Z

That's a tool we run internally on PRs that looks for patterns that indicate sensitive information being checked in. In this case it's a false positive around the fake account number. The .viperlightignore file is a list of all the false positives to ignore by filename and line number. Unfortunately that's a very brittle system for identifying the false positives, because even trivial code changes can change line numbers for an entire file. In this case the account number moved rom 123 to 124, so line 41 of .viperlightignore should be changed to :124

Not sure why the error message says look at line 125, everything I can see says the issue is on line 124.

aws-solutions-constructs-team · 2021-08-16T17:50:15Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 640eda3
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2021-08-17T12:08:56Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: b7484d4
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

biffgaut · 2021-08-17T12:14:03Z

@aws-solutions-constructs/aws-cloudfront-s3: ERROR: /codebuild/output/src815151308/src/github.com/awslabs/aws-solutions-constructs/source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/test/test.cloudfront-s3.test.ts:290:1 - Consecutive blank lines are forbidden

This line comes after the viperlight tag, so at least it wont affect that.

aws-solutions-constructs-team · 2021-08-17T12:24:08Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 575bac0
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

also refactor cloudfront-distribution-defaults Signed-off-by: Naseem <naseem@toric.com>

aws-solutions-constructs-team · 2021-08-19T01:00:57Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 25262ab
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

biffgaut · 2021-08-19T13:28:59Z

Naseem - quite a long journey, thanks for sticking with it and adding this improvement that will benefit lot of users! This will probably get pushed to the NPM repos within the next week. Because of the requirement that all versions must match, we're locked into releasing identical version numbers with the CDK. Since we can't get ahead of them we usually lag at least 1 version behind so if we have an emergency we can drop it right away. This requirement will ease with the coming release of CDK 2.0.

naseemkullah · 2021-08-19T20:12:45Z

Thanks for your patience and various explanations along the way @biffgaut!

naseemkullah · 2021-09-02T17:50:08Z

@biffgaut was this included in 1.118.0?

biffgaut · 2021-09-02T18:08:27Z

Yes, it did. Somehow we didn't catch it for CHANGELOG.md - I will update CHANGELOG.md to reflect that. Thanks.

hnishar · 2021-09-02T18:18:51Z

naseemkullah@ If the commit message follows the conventionalcommits it will be automatically picked up by CHANGELOG.md

naseemkullah requested review from biffgaut, hayesry and hnishar as code owners August 4, 2021 13:58

naseemkullah mentioned this pull request Aug 4, 2021

[cloudfront-s3] - option to disable creation of cloudfront logging bucket or pass in removal policy to it #302

Closed

naseemkullah force-pushed the fix-cloudfront-s3-cf-log-bucket-config branch from 22db0f3 to a74e36c Compare August 7, 2021 01:56

naseemkullah force-pushed the fix-cloudfront-s3-cf-log-bucket-config branch from a74e36c to d373bdc Compare August 7, 2021 02:49

naseemkullah force-pushed the fix-cloudfront-s3-cf-log-bucket-config branch from d373bdc to 43453cb Compare August 7, 2021 03:42

naseemkullah commented Aug 7, 2021

View reviewed changes

naseemkullah force-pushed the fix-cloudfront-s3-cf-log-bucket-config branch from 56fbab8 to 2e194ce Compare August 7, 2021 13:04

naseemkullah force-pushed the fix-cloudfront-s3-cf-log-bucket-config branch from 0c958ba to 6fae6e9 Compare August 7, 2021 14:26

naseemkullah force-pushed the fix-cloudfront-s3-cf-log-bucket-config branch from c878d60 to 5638074 Compare August 7, 2021 19:01

naseemkullah force-pushed the fix-cloudfront-s3-cf-log-bucket-config branch from 5638074 to e8c5ca0 Compare August 7, 2021 19:06

naseemkullah force-pushed the fix-cloudfront-s3-cf-log-bucket-config branch from e8c5ca0 to 03545cc Compare August 7, 2021 19:09

naseemkullah force-pushed the fix-cloudfront-s3-cf-log-bucket-config branch from 03545cc to a86d17a Compare August 7, 2021 19:19

naseemkullah changed the title ~~[cloudfront-s3] do not create cf log bucket if cf logs disabled~~ do not create cloudfront log bucket if disabled Aug 12, 2021

biffgaut reviewed Aug 12, 2021

View reviewed changes

naseemkullah force-pushed the fix-cloudfront-s3-cf-log-bucket-config branch from 4e878c7 to 61ae0c8 Compare August 14, 2021 21:28

naseemkullah force-pushed the fix-cloudfront-s3-cf-log-bucket-config branch from e2115e6 to bc25f6b Compare August 14, 2021 22:09

naseemkullah force-pushed the fix-cloudfront-s3-cf-log-bucket-config branch from bc25f6b to 640eda3 Compare August 16, 2021 17:48

naseemkullah force-pushed the fix-cloudfront-s3-cf-log-bucket-config branch from 640eda3 to b7484d4 Compare August 17, 2021 12:05

naseemkullah force-pushed the fix-cloudfront-s3-cf-log-bucket-config branch from b7484d4 to 575bac0 Compare August 17, 2021 12:15

do not create cloudfront log bucket if disabled

25262ab

also refactor cloudfront-distribution-defaults Signed-off-by: Naseem <naseem@toric.com>

naseemkullah force-pushed the fix-cloudfront-s3-cf-log-bucket-config branch from 575bac0 to 25262ab Compare August 19, 2021 00:52

biffgaut approved these changes Aug 19, 2021

View reviewed changes

biffgaut merged commit 84dbd11 into awslabs:main Aug 19, 2021

naseemkullah deleted the fix-cloudfront-s3-cf-log-bucket-config branch August 19, 2021 20:11


		function getLoggingBucket(cloudFrontDistributionProps: cloudfront.DistributionProps \| any, scope: cdk.Construct): s3.Bucket \| undefined {
		return cloudFrontDistributionProps?.enableLogging !== false

do not create cloudfront log bucket if disabled #303

do not create cloudfront log bucket if disabled #303

Conversation

naseemkullah commented Aug 4, 2021 • edited Loading

aws-solutions-constructs-team commented Aug 4, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Aug 4, 2021

AWS CodeBuild CI Report

naseemkullah commented Aug 4, 2021

aws-solutions-constructs-team commented Aug 7, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Aug 7, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Aug 7, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Aug 7, 2021

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-solutions-constructs-team commented Aug 7, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Aug 7, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Aug 7, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Aug 7, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Aug 7, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Aug 7, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Aug 7, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Aug 7, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Aug 7, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Aug 12, 2021

AWS CodeBuild CI Report

biffgaut commented Aug 12, 2021

aws-solutions-constructs-team commented Aug 12, 2021

AWS CodeBuild CI Report

biffgaut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naseemkullah Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

aws-solutions-constructs-team commented Aug 14, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Aug 14, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Aug 14, 2021

AWS CodeBuild CI Report

naseemkullah commented Aug 15, 2021

biffgaut commented Aug 16, 2021

aws-solutions-constructs-team commented Aug 16, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Aug 17, 2021

AWS CodeBuild CI Report

biffgaut commented Aug 17, 2021

aws-solutions-constructs-team commented Aug 17, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Aug 19, 2021

AWS CodeBuild CI Report

biffgaut commented Aug 19, 2021

naseemkullah commented Aug 19, 2021

naseemkullah commented Sep 2, 2021

biffgaut commented Sep 2, 2021

hnishar commented Sep 2, 2021

naseemkullah commented Aug 4, 2021 •

edited

Loading

naseemkullah Aug 19, 2021 •

edited

Loading