feat(aws-s3-sns): created new construct #849
Conversation
georgebearden
requested review from
biffgaut,
hayesry,
mobri2a,
emcfins and
mickychetta
as code owners
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 |
|
The aws-s3-sns construct has 100% line coverage for unit tests, and respective integration tests. I've deployed each integration test case and validated the events are actually published to the destination SNS Topic. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
|
||
| if (props.enableEncryptionWithCustomerManagedKey === undefined || | ||
| props.enableEncryptionWithCustomerManagedKey === true) { | ||
| enableEncryptionParam = true; |
There was a problem hiding this comment.
Just using
if (props.enableEncryptionWithCustomerManagedKey !== false)
directly in the code achieves the same result without hiding the meaning within another variable. Could even:
if (props.enableEncryptionWithCustomerManagedKey !== false) // default to true
for clarity
(quick code snippet from Typescript Playground for proof:
interface test {
attrib?: boolean
}
let testValue: test;
testValue = { attrib: false };
if (testValue.attrib !== false) {
console.log('attrib: false, incorrect');
} else {;
console.log('attrib: false, correct')
}
testValue = {};
if (testValue.attrib !== false) {
console.log('attrib: undefined, correct');
} else {;
console.log('attrib: undefined, incorrect')
}
testValue = { attrib: true };
if (testValue.attrib !== false) {
console.log('attrib: true, correct');
} else {;
console.log('attrib: true, incorrect')
}
There was a problem hiding this comment.
Wait - all we do is pass this to an optional prop attribute on BuildTopicProps? Why not just handle this default inside of BuildTopicProps? That ensures consistent behavior across constructs better than code reviews that check if each construct is using the correct default value.
There was a problem hiding this comment.
Will do on the simplified if logic.
Regarding the default value- I'd love to do that, but, unfortunately the sns-helper defaults the value to False, which is not overridden in other constructs, so we would be changing the behavior there as well. Specifically, the following constructs keep the default as False - aws-fargate-sns, aws-lambda-sns, aws-sns-lambda.
Let me know how you would prefer to proceed, leaving it alone, or updating it as described. If updating the default to True, which will use a CMK results in a better security posture anyway, then maybe we should do it for both consistency and security's sake.
| this.s3BucketInterface = bucket; | ||
|
|
||
| // Setup the topic | ||
| [this.snsTopic, this.encryptionKey] = defaults.buildTopic(this, { |
There was a problem hiding this comment.
I really hate how dependent we are becoming on this paradigm of returning an array of anonymous values (there's a lot more of this in firehose-s3).
I think we need to start considering defining an interface for return values from our build* functions.
There was a problem hiding this comment.
yeah I love that idea too. I'd prefer to keep this PR for the new construct and then do that refactor as a quick follow-up. I've created #853 to track it.
source/patterns/@aws-solutions-constructs/aws-s3-sns/lib/index.ts
Outdated
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-s3-sns/lib/index.ts
Outdated
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-s3-sns/lib/index.ts
Outdated
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-s3-sns/test/integ.s3EventTypesAndFilters.ts
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-s3-sns/test/test.s3-sns.test.ts
Outdated
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-s3-sns/test/test.s3-sns.test.ts
Outdated
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-s3-sns/test/test.s3-sns.test.ts
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-s3-sns/test/test.s3-sns.test.ts
Outdated
Show resolved
Hide resolved
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 |
| 'kms:GenerateDataKey*', | ||
| ); | ||
| } | ||
| this.encryptionKey?.grant(new iam.ServicePrincipal("s3.amazonaws.com"), |
There was a problem hiding this comment.
What if the topic is not encrypted and this.encryptionKey is undefined, won't this break?
There was a problem hiding this comment.
This is using optional chaining in typescript, so if encryptionKey evaluates to null, it will immediately stop running the expression.
There was a problem hiding this comment.
I've created a new set of unit/integ tests to prove it out.
| const cmk = defaults.buildEncryptionKey(stack); | ||
| const cmk = defaults.buildEncryptionKey(stack, { | ||
| description: 'existing-key-description' | ||
| }); | ||
| const [ existingTopicObj ] = defaults.buildTopic(stack, { |
There was a problem hiding this comment.
Based on my comment above, we need a test with an unencrypted topic and no key to prove to me that the code I flagged will not break.
There was a problem hiding this comment.
I've created a new set of unit/integ tests to prove it out.
| * @param description The value of the Description property on the CloudFormation Resource | ||
| * @returns The Logical ID of the found resource | ||
| */ | ||
| export function getResourceLogicalIdFromDescription(stack: Stack, resourceType: string, description: string): string { |
There was a problem hiding this comment.
Splitting the execution of these tests between this function and then checking the resulting logical ID in the test itself hides the extra level of confirmation. Let's expand this function to perform the entire "Is the resource we expect actually attached to the correct parent resource" check in one place:
function expectKmsKeyAttachedToCorrectResource(
stack: Stack,
parentResourceType: string,
keyDescription: string) {
// Find the unique KMS key we expect to be in
// in the stack
const template = Template.fromStack(stack);
const resource = template.findResources('AWS::KMS::Key', {
Properties: {
Description: Match.exact(keyDescription)
}
});
const [ const [ keyResourceId ] = Object.keys(resource); ] = Object.keys(resource);
// Confirm that the key is attached to the
// parent resource we expect
expect(stack).toHaveResource(parentResourceType, {
KmsMasterKeyId: {
"Fn::GetAtt": [
keyResourceId,
"Arn"
]
}
});
}
There was a problem hiding this comment.
Good suggestion - Updated in latest revision.
…adersPolicyProps to CloudFrontToS3Props, CloudFrontToMediaStoreProps, CloudFronToApiGatewayProps
…havior into input-validation.ts
…ityHeaders in input-validation.ts
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 |
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 |
Issue #, if available:
N/A
Description of changes:
README for the new
aws-s3-snsconstructBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.