Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(aws-fargate-s3): Created new construct #591

Merged
merged 8 commits into from
Feb 19, 2022
Merged

feat(aws-fargate-s3): Created new construct #591

merged 8 commits into from
Feb 19, 2022

Conversation

mickychetta
Copy link
Contributor

Issue #590 , if available:

Description of changes:
-Created new construct of aws-fargate-s3
-Added environment variables in fargate environment to allow read/write to bucket
-Created unit/integ tests

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

fixes #590

@mickychetta mickychetta requested a review from biffgaut as a code owner February 8, 2022 00:27
@mickychetta mickychetta self-assigned this Feb 8, 2022
@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: githubautobuild-for-cdk-v2
  • Commit ID: 6265f15
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: githubautobuild-for-cdk-v1
  • Commit ID: 6265f15
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: githubautobuild-for-cdk-v1
  • Commit ID: aec48ca
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: githubautobuild-for-cdk-v2
  • Commit ID: aec48ca
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: githubautobuild-for-cdk-v2
  • Commit ID: 845bd7c
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: githubautobuild-for-cdk-v1
  • Commit ID: 845bd7c
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: githubautobuild-for-cdk-v2
  • Commit ID: dd8c5ea
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: githubautobuild-for-cdk-v1
  • Commit ID: dd8c5ea
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: githubautobuild-for-cdk-v2
  • Commit ID: 19a7f59
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: githubautobuild-for-cdk-v1
  • Commit ID: 19a7f59
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: githubautobuild-for-cdk-v2
  • Commit ID: 07e4322
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

biffgaut
biffgaut requested changes Feb 17, 2022
View reviewed changes
Copy link
Contributor

@biffgaut biffgaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar issue around permissions. Change should very simple.

Testing for conditions needs improvement.

source/patterns/@aws-solutions-constructs/aws-fargate-s3/README.md Outdated Show resolved Hide resolved
source/patterns/@aws-solutions-constructs/aws-fargate-s3/README.md Outdated
|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.|
|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.|
|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|
|bucketPermissions?|`string[]`|Optional bucket permissions to grant to the Lambda function. One or more of the following may be specified: `Delete`, `Put`, `Read`, `ReadWrite`, `Write`.|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo - Lambda function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These terms seem specific to DDB, there's no difference between Put and Write in S3 that I can think of easily. I think the correct list would be:

List
Put
Get
Delete

If we want to stay 'generic' so read/write are common terms across all constructs (which has appeal), then it would be:

Read (we would want to ensure this includes ListBucket)
Write
Delete

If the docs are correct and more than 1 can be specified, then there is no need to include ReadWrite - I haven't looked at lambda-s3 yet, but this seems like a mistake we made back then.

Let's go with the latter and make all constructs use the terms Read and Write for consistency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is optional, the docs need to clearly state the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default bucket permissions in aws-lambda-s3 is ReadWrite which composes of Read, Write, and Delete operations so I will stick to to that for consistency. I allowed Read, Write, and Delete strings for bucketPermissions in order to create a more fine-grained policy. I removed Put as it is repetitive and made ReadWrite as default which separates confusion from inputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need to make following changes to aws-lambda-s3 if we agree with this.

source/patterns/@aws-solutions-constructs/aws-fargate-s3/lib/index.ts Show resolved Hide resolved
source/patterns/@aws-solutions-constructs/aws-fargate-s3/test/fargate-s3.test.ts Outdated Show resolved Hide resolved
@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: githubautobuild-for-cdk-v2
  • Commit ID: 27b9006
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: githubautobuild-for-cdk-v2
  • Commit ID: f14cc54
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

biffgaut
biffgaut requested changes Feb 19, 2022
View reviewed changes
Copy link
Contributor

@biffgaut biffgaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of changes to ensure backwards compatibility with existing stacks.

source/patterns/@aws-solutions-constructs/aws-fargate-s3/README.md
@@ -68,7 +56,7 @@ _Parameters_
|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.|
|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.|
|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|
|bucketPermissions?|`string[]`|Optional bucket permissions to grant to the Lambda function. One or more of the following may be specified: `Delete`, `Put`, `Read`, `ReadWrite`, `Write`.|
|bucketPermissions?|`string[]`|Optional bucket permissions to grant to the Fargate service. One or more of the following may be specified: `Delete`, `Read`, and `Write`. Default is `ReadWrite` which includes `[s3:GetObject*, s3:GetBucket*, s3:List*, s3:DeleteObject*, s3:PutObject*, s3:Abort*]`.|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to change this to Default is 'Read' and 'Write' as we don't want to give the impression that ReadWrite is not deprecated. I like that we don't list it here - I assume the code still allows it for backwards compatibility.

If the code looks good I can approve this now and we can make a quick inline change of this comment later.

source/patterns/@aws-solutions-constructs/aws-fargate-s3/lib/index.ts
if (props.bucketPermissions.includes('Read')) {
bucket.grantRead(this.service.taskDefinition.taskRole);
}
if (props.bucketPermissions.includes('ReadWrite')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't delete this functionality as that will break existing clients - we should drop them from the doc but leave the code that supports them in place along with comments that 'Put' and 'ReadWrite' are deprecated but this code remains for backwards compatibility.

   // ReadWrite and Put are deprecated, but included here for backwards compatibility
   if (props.bucketPermissions.includes('Write') || 
        props.bucketPermissions.includes{'ReadWrite') ||
        props.bucketPermissions.includes('Put') )
   {
        ...
   }
   if (props.bucketPermissions.includes('Read') || props.bucketPermissions.include('ReadWrite') {
      ...
   }

etc.

  if (props.bucketPermissions.includes('ReadWrite') || props.bucketPermissions.includes('Put')) {
    printWarning('Put and ReadWrite are deprecated, please switch to some combination of Read, Write and Delete');
  }

biffgaut
biffgaut approved these changes Feb 19, 2022
View reviewed changes
Copy link
Contributor

@biffgaut biffgaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore my comments about backwards compatibility - I was thinking aws-lambda-s3 and this is a brand new construct.

@biffgaut biffgaut merged commit 29a3b2a into awslabs:main Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@biffgaut biffgaut biffgaut approved these changes

@hayesry hayesry Awaiting requested review from hayesry hayesry is a code owner

@hnishar hnishar Awaiting requested review from hnishar

Assignees

@mickychetta mickychetta

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat(aws-fargate-s3): New Construct
3 participants
@mickychetta @aws-solutions-constructs-team @biffgaut