feat(aws-kinesisfirehose-s3): added custom logging bucket props to kinesisfirehose-s3 #478

mickychetta · 2021-10-25T20:46:50Z

Issue #474 , if available:

Description of changes:
-Added custom logging bucket props to kinesisfirehose-s3
-Created unit and integ test for custom logging bucket

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

fixes #474
issue #485

aws-solutions-constructs-team · 2021-10-25T20:53:42Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 1af800f
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-10-25T20:55:56Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v1
Commit ID: 1af800f
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-10-25T21:48:35Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 010aec1
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-10-25T21:50:41Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v1
Commit ID: 010aec1
Result: SUCCEEDED
Build Logs (available for 30 days)

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

…nstructs into 474

aws-solutions-constructs-team · 2021-10-26T08:54:24Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 97b653e
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-10-26T08:58:04Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v1
Commit ID: 97b653e
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-10-26T21:13:40Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: ad5f4dc
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-10-26T21:15:43Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v1
Commit ID: ad5f4dc
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-10-26T21:25:44Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v1
Commit ID: a6e2363
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-10-26T21:26:22Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: a6e2363
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-10-26T21:32:28Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v1
Commit ID: 3e62a07
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-10-26T21:33:12Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 3e62a07
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-10-26T22:01:22Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 642a11d
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-10-26T22:03:34Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v1
Commit ID: 642a11d
Result: SUCCEEDED
Build Logs (available for 30 days)

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

biffgaut

Stopped after index.ts because I requested moving a bunch of code to s3-helper.ts

source/patterns/@aws-solutions-constructs/aws-kinesisfirehose-s3/README.md

source/patterns/@aws-solutions-constructs/aws-kinesisfirehose-s3/lib/index.ts

aws-solutions-constructs-team · 2021-11-01T22:12:13Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 50d5bf5
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-11-01T22:14:17Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v1
Commit ID: 50d5bf5
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-11-01T22:22:09Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 1145168
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-11-01T22:24:22Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v1
Commit ID: 1145168
Result: SUCCEEDED
Build Logs (available for 30 days)

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

biffgaut

I spent a lot of time looking at s3-bucket-helper.ts. I made many comments/requests because the changes were pretty major and the module is fundamental - but in general I like that the changes seemed to make it simpler to follow.

biffgaut · 2021-11-02T13:45:58Z

source/patterns/@aws-solutions-constructs/aws-kinesisfirehose-s3/test/integ.no-arguments.ts

@@ -24,6 +24,9 @@ stack.templateOptions.description = 'Integration Test for aws-cdk-apl-kinesisfir
 new KinesisFirehoseToS3(stack, 'test-firehose-s3', {
  bucketProps: {
    removalPolicy: RemovalPolicy.DESTROY,
+  },
+  loggingBucketProps: {
+    removalPolicy: RemovalPolicy.DESTROY


Does this fully remove the bucket? Or do we also need autoDeleteObjects: true?

Fully removes the bucket

source/patterns/@aws-solutions-constructs/core/lib/input-validation.ts

biffgaut · 2021-11-02T14:21:46Z

source/patterns/@aws-solutions-constructs/core/lib/s3-bucket-helper.ts

-      bucketprops = DefaultS3Props();
-    }
-  } else if (logS3AccessLogs !== false) {
+  if (logS3AccessLogs !== false && !userBucketProps?.serverAccessLogsBucket) {


!userBucketProps?.serverAccessLogsBucket

This is really hard to decipher. I've had to write a couple of these as well - I usually go to the TypesScript scratch pad and try to test all the different possibilities. But the combination of not the whole expression, the optional operator on userBucketProps and serverAccessLogsBucket defaulting to true makes it a mess of double negatives. And the first part, logS3AccessLogs !== false has the knowledge that the value defaults to true hidden in it. I never liked my results.

What do you think of

// Put these in core\utils.ts export function EvaluateOptionalBoolean(value: boolean | undefined, defaultValue: boolean) : boolean { return value ?? defaultValue; } export function ValueExists(value | any) : boolean { return (value === undefined) ? false : true; } // Then this line becomes if (EvaluateOptionalBoolean(logS3AccessLogs, true) && !ValueExists(userBucketProps?.serverAccessLogsBucket)) {

The fact that the default for the attribute is true is now explicit in the code. The second condition now has a function name that explicitly describes what we're checking. At this point I'm not insisting on a change - like to hear what you think first.

I think that creating the additional functions is unnecessary as it performs the same functionality. I believe we should take advantage of the built in javascript operators for its purpose. I have changed !userBucketProps?.serverAccessLogsBucket to !(props.bucketProps?.serverAccessLogsBucket) for clarity. I left it as props.logS3AccessLogs !== false because the real default is undefined allowing it to pass for use cases when it is undefined or true. I actually find the additional functions harder to decipher than the ones I provided. Will also add a comment above the line. Let me know what you think

OK - but the default is no undefined. If it is undefined we use the default value, which is true.

biffgaut · 2021-11-02T14:27:30Z

source/patterns/@aws-solutions-constructs/core/lib/s3-bucket-helper.ts

-    } else if (s3BucketProps?.removalPolicy) { // Deletes logging bucket only if it is empty
-      loggingBucketProps = overrideProps(DefaultS3Props(), { removalPolicy: s3BucketProps.removalPolicy });
-    } else { // Default S3 bucket props
+    } else if (userBucketProps?.removalPolicy) {


This would be an argument against ArgumentExists(), because:

If we implemented ArgumentExists(), we would want to use it here.

This is such a common Typescript expression that it's clear without any abstraction behind a function.

Makes me think that ArgumentExists() is overkill and I should just suck it up WRT to the ! operator in line 152. I still think a EvaluatateOptionalBoolean where we explicitly declare a default merits consideration.

I agree, I believe it is overkill as it is a common Typescript expression

source/patterns/@aws-solutions-constructs/core/lib/s3-bucket-helper.ts

biffgaut · 2021-11-02T14:51:38Z

source/patterns/@aws-solutions-constructs/core/lib/s3-bucket-helper.ts

-      bucketprops = DefaultS3Props();
-    }
-  } else if (logS3AccessLogs !== false) {
+  if (logS3AccessLogs !== false && !userBucketProps?.serverAccessLogsBucket) {


Remind me - do we throw an error earlier if they set logS3AccessLogs to false but still send a serverAccessLogsBucket or logBucketProps?

Yes, we throw an error through input-validation.ts

biffgaut · 2021-11-02T14:53:24Z

source/patterns/@aws-solutions-constructs/core/test/input-validation.test.ts

@@ -445,3 +445,53 @@ test('Test fail Log Bucket check', () => {
  // Assertion
  expect(app).toThrowError('Error - Either provide existingLoggingBucketObj or loggingBucketProps, but not both.\n');
 });
+
+test('Test fail false logS3Accesslogs and loggingBucketProps check', () => {


I think this and line 431 answer my earlier "Remind me..." comment?

source/patterns/@aws-solutions-constructs/core/lib/s3-bucket-helper.ts

biffgaut · 2021-11-02T14:59:01Z

source/patterns/@aws-solutions-constructs/core/lib/s3-bucket-helper.ts

@@ -42,7 +42,7 @@ export interface BuildS3BucketProps {
   *
   * @default - true
   */
-   readonly logS3AccessLogs?: boolean;
+  readonly logS3AccessLogs?: boolean;
 }

 export function buildS3Bucket(scope: Construct, props: BuildS3BucketProps, bucketId?: string): [s3.Bucket, s3.Bucket?] {


We made some fundamental changes to this module and it is fundamental to a lot of constructs. If possible we should get to 100% statement and branch coverage:

@aws-solutions-constructs/core: s3-bucket-helper.ts | 98.07 | 96.66 | 100 | 98.07 | 148

…nstructs into 474

aws-solutions-constructs-team · 2021-11-02T21:41:35Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 06c6f87
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-11-02T21:43:44Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v1
Commit ID: 06c6f87
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-11-02T21:50:50Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: f67def9
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-11-02T21:52:57Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v1
Commit ID: f67def9
Result: SUCCEEDED
Build Logs (available for 30 days)

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

…nstructs into 474

aws-solutions-constructs-team · 2021-11-02T22:31:23Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: a37a678
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-11-02T22:33:40Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v1
Commit ID: a37a678
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-11-02T22:54:33Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: b895d90
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-11-02T22:56:45Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v1
Commit ID: b895d90
Result: SUCCEEDED
Build Logs (available for 30 days)

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

added custom logging bucket props to kinesisfirehose-s3

1af800f

mickychetta requested review from biffgaut, hayesry and hnishar as code owners October 25, 2021 20:46

Merge branch 'main' into 474

010aec1

mickychetta added 2 commits October 26, 2021 08:46

added log bucket condition in input validation

dd775fe

Merge branch '474' of https://github.com/mickychetta/aws-solutions-co…

97b653e

…nstructs into 474

Added logS3AccessLogs for enabling/disabling s3 logs

ad5f4dc

added cfn suppress rule for no logging

a6e2363

fix lint issue

3e62a07

redeploy stack for cfn nag suppress changes

642a11d

mickychetta self-assigned this Oct 26, 2021

biffgaut requested changes Oct 27, 2021

View reviewed changes

added logS3AccessLogs property

50d5bf5

Merge branch 'main' into 474

1145168

biffgaut requested changes Nov 2, 2021

View reviewed changes

mickychetta added 2 commits November 2, 2021 21:34

refactored s3 bucket helper and improved tests

b43c4e8

Merge branch '474' of https://github.com/mickychetta/aws-solutions-co…

06c6f87

…nstructs into 474

Merge branch 'main' into 474

f67def9

mickychetta added 2 commits November 2, 2021 22:21

readded test for s3-bucket

f12499c

Merge branch '474' of https://github.com/mickychetta/aws-solutions-co…

a37a678

…nstructs into 474

moved test to s3 bucket helper test file

b895d90

biffgaut approved these changes Nov 3, 2021

View reviewed changes

biffgaut merged commit 6fab3e5 into awslabs:main Nov 3, 2021

feat(aws-kinesisfirehose-s3): added custom logging bucket props to kinesisfirehose-s3 #478

feat(aws-kinesisfirehose-s3): added custom logging bucket props to kinesisfirehose-s3 #478

Conversation

mickychetta commented Oct 25, 2021 • edited Loading

aws-solutions-constructs-team commented Oct 25, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Oct 25, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Oct 25, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Oct 25, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Oct 26, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Oct 26, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Oct 26, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Oct 26, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Oct 26, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Oct 26, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Oct 26, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Oct 26, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Oct 26, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Oct 26, 2021

AWS CodeBuild CI Report

biffgaut left a comment

Choose a reason for hiding this comment

aws-solutions-constructs-team commented Nov 1, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Nov 1, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Nov 1, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Nov 1, 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

mickychetta Nov 2, 2021 • edited Loading

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

aws-solutions-constructs-team commented Nov 2, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Nov 2, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Nov 2, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Nov 2, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Nov 2, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Nov 2, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Nov 2, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Nov 2, 2021

AWS CodeBuild CI Report

mickychetta commented Oct 25, 2021 •

edited

Loading

mickychetta Nov 2, 2021 •

edited

Loading