fix(s3-bucket-helper): not populating response.loggingBucket when buc…

…ket supplied (#934) * Set loggingBucket in response * Remove underscore prefixes * Adjust construct to continue to pass test
awslabs · Mar 29, 2023 · b65986d · b65986d
1 parent 7339ba4
commit b65986d
Show file tree

Hide file tree

Showing 3 changed files with 12 additions and 6 deletions.
diff --git a/source/patterns/@aws-solutions-constructs/aws-kinesisfirehose-s3/lib/index.ts b/source/patterns/@aws-solutions-constructs/aws-kinesisfirehose-s3/lib/index.ts
@@ -109,7 +109,10 @@ export class KinesisFirehoseToS3 extends Construct {
         logS3AccessLogs: props.logS3AccessLogs,
       });
       this.s3Bucket = buildS3BucketResponse.bucket;
-      this.s3LoggingBucket = buildS3BucketResponse.loggingBucket;
+      // Commit fd5a4f1fe5bd4fb85265b895eec4c36349a8bf64 fixed the core routine,
+      // but changed this behavior. Forcing undefined to pass existing test, but we
+      // should clarify behavior for construct properties when existing values are passed in.
+      this.s3LoggingBucket = props.existingLoggingBucketObj ? undefined : buildS3BucketResponse.loggingBucket;
 
       bucket = this.s3Bucket;
     } else {

diff --git a/source/patterns/@aws-solutions-constructs/core/lib/s3-bucket-helper.ts b/source/patterns/@aws-solutions-constructs/core/lib/s3-bucket-helper.ts
@@ -130,8 +130,8 @@ export function buildS3Bucket(scope: Construct,
   // Create the Application Bucket
   let customBucketProps: s3.BucketProps;
   let loggingBucket;
-  const _bucketId = bucketId ? bucketId + 'S3Bucket' : 'S3Bucket';
-  const _loggingBucketId = bucketId ? bucketId + 'S3LoggingBucket' : 'S3LoggingBucket';
+  const resolvedBucketId = bucketId ? bucketId + 'S3Bucket' : 'S3Bucket';
+  const loggingBucketId = bucketId ? bucketId + 'S3LoggingBucket' : 'S3LoggingBucket';
 
   // If logging S3 access logs is enabled/undefined and an existing bucket object is not provided
   if (props.logS3AccessLogs !== false && !(props.bucketProps?.serverAccessLogsBucket)) {
@@ -147,8 +147,11 @@ export function buildS3Bucket(scope: Construct,
       loggingBucketProps = overrideProps(loggingBucketProps, { removalPolicy: props.bucketProps.removalPolicy });
     }
 
-    loggingBucket = createLoggingBucket(scope, _loggingBucketId, loggingBucketProps);
+    loggingBucket = createLoggingBucket(scope, loggingBucketId, loggingBucketProps);
+  } else if (props.bucketProps?.serverAccessLogsBucket) {
+    loggingBucket = props.bucketProps?.serverAccessLogsBucket as s3.Bucket;
   }
+
   // Attach the Default Life Cycle policy ONLY IF the versioning is ENABLED
   if (props.bucketProps?.versioned === undefined || props.bucketProps.versioned) {
     customBucketProps = DefaultS3Props(loggingBucket, lifecycleRules);
@@ -158,7 +161,7 @@ export function buildS3Bucket(scope: Construct,
 
   customBucketProps = props.bucketProps ? overrideProps(customBucketProps, props.bucketProps) : customBucketProps;
 
-  const s3Bucket: s3.Bucket = new s3.Bucket(scope, _bucketId, customBucketProps );
+  const s3Bucket: s3.Bucket = new s3.Bucket(scope, resolvedBucketId, customBucketProps );
 
   return { bucket: s3Bucket, loggingBucket };
 }

diff --git a/source/patterns/@aws-solutions-constructs/core/test/s3-bucket-helper.test.ts b/source/patterns/@aws-solutions-constructs/core/test/s3-bucket-helper.test.ts
@@ -230,7 +230,7 @@ test('s3 bucket with LoggingBucket and versioning turned off', () => {
 
   expect(buildS3BucketResponse.bucket).toBeDefined();
   // The line below fails, this appears to be a bug. Entered Issue 906
-  //  expect(response.loggingBucket).toBeDefined();
+  expect(buildS3BucketResponse.loggingBucket).toBeDefined();
 
   const template = Template.fromStack(stack);
   template.hasResourceProperties("AWS::S3::Bucket", {