feat(s3): server access logs #5072

arnulfojr · 2019-11-17T19:39:43Z

PR title
PR description
Unit testing
Integ testing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

mergify · 2019-11-17T19:40:20Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-11-17T20:06:13Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-11-17T20:08:10Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

packages/@aws-cdk/aws-s3/lib/bucket.ts

eladb · 2019-11-18T08:18:04Z

packages/@aws-cdk/aws-s3/lib/bucket.ts

+   * @param logFilePrefix Optional log file prefix.
+   */
+  public enableServerAccessLogs(destinationBucket: IBucket, logFilePrefix?: string): void {
+    (this.node.defaultChild as CfnBucket).loggingConfiguration = {


move this to Bucket and don't use node.defaultChild but rather directly reference the resource.

Pull request has been modified.

aws-cdk-automation · 2019-11-18T09:30:03Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

eladb

Missing unit tests to cover both usages (props and method)

arnulfojr · 2019-11-18T10:10:28Z

@eladb yeap, I'll add the unit tests later today

Pull request has been modified.

aws-cdk-automation · 2019-12-02T16:04:55Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-02T16:05:33Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-02T16:26:54Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-02T16:30:32Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-02T17:10:57Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

packages/@aws-cdk/aws-s3/lib/bucket.ts

mergify · 2019-12-16T11:18:20Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T12:33:58Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:01:11Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

Pull request has been modified.

aws-cdk-automation · 2019-12-16T13:43:07Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-09T15:51:26Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-09T16:21:08Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb

sorry, but I think we need another round... Eventually we'll nail it :-)
happy to send you a PR with my proposal against your branch.

eladb · 2020-01-12T08:55:21Z

packages/@aws-cdk/aws-s3/lib/bucket.ts

+   */
+  public allowLogDelivery() {
+    if (this.accessControl && this.accessControl !== BucketAccessControl.LOG_DELIVERY_WRITE) {
+      throw new Error("The bucket's ACL has been set and can't be changed");


Suggested change

throw new Error("The bucket's ACL has been set and can't be changed");

throw new Error("Cannot enable log delivery to this bucket because the bucket's ACL has been set and can't be changed");

eladb · 2020-01-12T09:00:30Z

packages/@aws-cdk/aws-s3/lib/bucket.ts

+  /**
+   * Grants write permissions for the LogDelivery group to the bucket.
+   */
+  allowLogDelivery(): void;


I am still don't feel too good about this. The name is confusing, and users might think that this is how they can enable log delivery.

I am starting to think that maybe we should make this an internal API on Bucket (instead of IBucket). If people want to set the log delivery access control they can use the accessControl property when they create the bucket.

export class Bucket ... { /** @internal */ public _allowLogDelivery() { ... } };

Then, in the consuming side, you will need to check if your IBucket is a Bucket. Otherwise, you won't be able to use it for log delivery because you can't set it's access control:

if (props.serverAccessLogsBucket instanceof Bucket) { props.serverAccessLogsBucket._allowLogDelivery(); }

And that's it. I think it will simplify dramatically.

LMK if you want me to send you a PR with this proposal.

Yea, sounds good. I moved the allowLogDelivery to the Bucket class and set it as a private method. I'm not sure what would be the difference by using the @internal annotation, tho.

Pull request has been modified.

aws-cdk-automation · 2020-01-12T10:26:03Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-12T11:06:51Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb

Yes, looks better... I really like it that we can do this without extending the surface area.
A few additional minor comments

eladb · 2020-01-13T09:24:32Z

packages/@aws-cdk/aws-s3/lib/bucket.ts

+  /**
+   * Optional bucket Access Control.
+   *
+   * @default BucketAccessControl.PRIVATE
+   */
+  accessControl?: BucketAccessControl;
+


Remove this please

eladb · 2020-01-13T09:24:38Z

packages/@aws-cdk/aws-s3/lib/bucket.ts

+  /**
+   * Optional Bucket access control.
+   */
+  public abstract accessControl?: BucketAccessControl;
+


Remove this please

eladb · 2020-01-13T09:24:49Z

packages/@aws-cdk/aws-s3/lib/bucket.ts

@@ -980,6 +1005,7 @@ export class Bucket extends BucketBase {

  public readonly encryptionKey?: kms.IKey;
  public policy?: BucketPolicy;
+  public accessControl?: BucketAccessControl;


Make this private

eladb · 2020-01-13T09:25:53Z

packages/@aws-cdk/aws-s3/lib/bucket.ts

@@ -1358,6 +1405,14 @@ export class Bucket extends BucketBase {
      routingRules
    };
  }
+
+  private allowLogDelivery() {


add docstring that explains what this is doing

Pull request has been modified.

aws-cdk-automation · 2020-01-13T10:27:06Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb

Please add a README entry

eladb · 2020-01-13T12:45:42Z

packages/@aws-cdk/aws-s3/lib/bucket.ts

@@ -956,6 +968,7 @@ export class Bucket extends BucketBase {
      public readonly bucketWebsiteNewUrlFormat = newUrlFormat;
      public readonly encryptionKey = attrs.encryptionKey;
      public policy?: BucketPolicy = undefined;
+      public accessControl?: BucketAccessControl = undefined;


Suggested change

public accessControl?: BucketAccessControl = undefined;

aws-cdk-automation · 2020-01-13T13:16:46Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

Pull request has been modified.

eladb

Yaaas! Thanks for all your patience

aws-cdk-automation · 2020-01-13T13:56:56Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-01-13T13:57:50Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2020-01-13T14:34:15Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-01-13T14:36:55Z

Thank you for contributing! Your pull request is now being automatically merged.

arnulfojr · 2020-01-13T15:28:04Z

@eladb thanks for all the learnings, really insightful :) really appreciate it!

arnulfojr requested a review from eladb as a code owner November 17, 2019 19:39

arnulfojr force-pushed the s3-server-access-logs branch from e2f87dc to 7cc5e96 Compare November 17, 2019 19:41

eladb previously requested changes Nov 18, 2019

View reviewed changes

SomayaB assigned eladb Nov 18, 2019

arnulfojr force-pushed the s3-server-access-logs branch 3 times, most recently from 2bd9431 to b07655a Compare December 2, 2019 16:02

arnulfojr requested a review from eladb December 6, 2019 11:14

eladb previously requested changes Dec 12, 2019

View reviewed changes

packages/@aws-cdk/aws-s3/lib/bucket.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-s3/lib/bucket.ts Outdated Show resolved Hide resolved

arnulfojr force-pushed the s3-server-access-logs branch from 9c07f82 to 6a44bfa Compare December 16, 2019 13:20

arnulfojr force-pushed the s3-server-access-logs branch from 6a44bfa to f4ea18c Compare December 16, 2019 13:22

arnulfojr requested a review from eladb December 16, 2019 13:23

arnulfojr requested a review from eladb January 9, 2020 15:44

eladb previously requested changes Jan 12, 2020

View reviewed changes

Merge branch 'master' into s3-server-access-logs

ef22ff9

allowLogDelivery method moved to Bucket and made private

a0134c2

eladb previously requested changes Jan 13, 2020

View reviewed changes

Merge branch 'master' into s3-server-access-logs

610f0a0

Bucket's accessControl is not anymore public

551fef2

eladb previously requested changes Jan 13, 2020

View reviewed changes

README entry and removed accessControl from the import

797a6df

eladb approved these changes Jan 13, 2020

View reviewed changes

Merge branch 'master' into s3-server-access-logs

d86e1cc

mergify bot merged commit c9b074b into aws:master Jan 13, 2020

eladb mentioned this pull request Jan 23, 2020

[s3] Support LoggingConfiguration for Bucket #4967

Closed

2 tasks

This was referenced Mar 2, 2020

s3: Setup s3 server access logging target bucket policy #6545

Closed

S3 Logging not exposed #2177

Closed

laurelmay mentioned this pull request Jan 4, 2023

cdk: Breaking change to S3 bucket server access logging (v2.59.0) #23547

Closed

	throw new Error("The bucket's ACL has been set and can't be changed");
	throw new Error("Cannot enable log delivery to this bucket because the bucket's ACL has been set and can't be changed");

feat(s3): server access logs #5072

feat(s3): server access logs #5072

Conversation

arnulfojr commented Nov 17, 2019 • edited Loading

mergify bot commented Nov 17, 2019

aws-cdk-automation commented Nov 17, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 17, 2019

AWS CodeBuild CI Report

Choose a reason for hiding this comment

aws-cdk-automation commented Nov 18, 2019

AWS CodeBuild CI Report

eladb left a comment

Choose a reason for hiding this comment

arnulfojr commented Nov 18, 2019

aws-cdk-automation commented Dec 2, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 2, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 2, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 2, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 2, 2019

AWS CodeBuild CI Report

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

aws-cdk-automation commented Dec 16, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 9, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 9, 2020

AWS CodeBuild CI Report

eladb 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

aws-cdk-automation commented Jan 12, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 12, 2020

AWS CodeBuild CI Report

eladb 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

aws-cdk-automation commented Jan 13, 2020

AWS CodeBuild CI Report

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 13, 2020

AWS CodeBuild CI Report

eladb left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 13, 2020

AWS CodeBuild CI Report

mergify bot commented Jan 13, 2020

aws-cdk-automation commented Jan 13, 2020

AWS CodeBuild CI Report

mergify bot commented Jan 13, 2020

arnulfojr commented Jan 13, 2020

arnulfojr commented Nov 17, 2019 •

edited

Loading