feat(aws-s3-sqs): New aws-s3-sqs pattern implementation (#27) #105
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
…et notifications.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
|
||
| | **Name** | **Type** | **Description** | | ||
| |:-------------|:----------------|-----------------| | ||
| |existingBucketObj?|[`s3.IBucket`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-s3.IBucket.html)|Existing instance of S3 Bucket object, if this is set then the bucketProps is ignored.| |
There was a problem hiding this comment.
Is it s3.IBucket or s3.Bucket ?
| }); | ||
|
|
||
| // Setup customer managed KMS CMK for Queue encryption | ||
| let enableEncryptionWithCustomerManagedKey: boolean = false; |
There was a problem hiding this comment.
Can this be exposed via construct props similar to aws-sns-sqs pattern ?
|
|
||
| // Setup customer managed KMS CMK for Queue encryption | ||
| let enableEncryptionWithCustomerManagedKey: boolean = false; | ||
| if (!this.hasQueueEncryptionProperties(props.queueProps)) { |
There was a problem hiding this comment.
Similar to aws-sns-sqs pattern, instead why not expose readonly encryptionKey?: kms.Key and readonly encryptionKeyProps?: kms.KeyProps for user to provide either Key or KeyProps ?
| public readonly sqsQueue: sqs.Queue; | ||
| public readonly deadLetterQueue?: sqs.DeadLetterQueue; | ||
| public readonly s3Bucket?: s3.Bucket; | ||
| public readonly s3LoggingBucket?: s3.Bucket; |
There was a problem hiding this comment.
If the Key is generated by the pattern, add it to the properties for user to access the Key
There was a problem hiding this comment.
Clarifying - the encryptionKey property should be set to the key for the queue regardless of whether it was passed in or generated by the construct.
| this.addCfnNagSuppress(); | ||
| } | ||
|
|
||
| private addCfnNagSuppress() { |
There was a problem hiding this comment.
This should probably move to the /core since its now used by two patterns
| } | ||
|
|
||
| // Setup the S3 bucket event notifications | ||
| s3EventTypes.forEach(type => bucket.addEventNotification(type, new s3n.SqsDestination(this.sqsQueue), ...s3Eventfilters)); |
There was a problem hiding this comment.
Does this take care of granting the KMS key permissions to S3 ?
There was a problem hiding this comment.
Yes it will take care of the KMS key resource policy.
| @@ -0,0 +1,51 @@ | |||
| /** | |||
There was a problem hiding this comment.
What is the purpose of this Integration test ? Don't think the name integ.deployQueue.ts captures it.
| existingQueueObj: queue | ||
| }); | ||
| // Assertion 1 | ||
| expect(SynthUtils.toCloudFormation(stack)).toMatchSnapshot(); |
There was a problem hiding this comment.
Instead of snapshot, please add the targeted unit test to assert the expected resource(s) exist
| existingBucketObj: myBucket | ||
| }); | ||
| // Assertion 1 | ||
| expect(SynthUtils.toCloudFormation(stack)).toMatchSnapshot(); |
There was a problem hiding this comment.
Instead of snapshot, please add the targeted unit test to assert the expected resource(s) exist
| }); | ||
| // Assertion 1 | ||
| expect(SynthUtils.toCloudFormation(stack)).toMatchSnapshot(); | ||
| }); |
There was a problem hiding this comment.
Please add a unit test to assert the S3 is granted required permissions to use the KMS Key
Moved addCfnNagSuppress to core. Exposed encryption key properties in s3-sqs pattern. Improved integration and unit tests.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
…source policy that allows the root account to access it.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
@hnishar could you please review the changes? |
|
@hnishar is there anything I might be able to do to help move this along? |
…ion to use 2 spaces.
…managed CMK to encrypt the SQS Queue.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thanks for the contribution, the updates will go out in v1.77.0 release |
closes #27
Issue #, if available: #27
Description of changes:
Implements the AWS Solutions Construct that creates an Amazon S3 Bucket connected to an Amazon SQS queue.
In addition, it includes the changes in the s3-bucket-defaults.ts to define the defaultS3NotificationEventTypes.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.