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

feat(aws-s3-sns): created new construct #849

Merged
merged 21 commits into from
Dec 4, 2022
Merged

feat(aws-s3-sns): created new construct #849

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
51db29c
Add README for new aws-s3-sns construct.
georgebearden Nov 21, 2022
ee52701
add required directory for build to pass
georgebearden Nov 22, 2022
adf993d
implement the aws-s3-sns construct
georgebearden Nov 23, 2022
70e8994
Add more tests to s3-sns construct and helper classes.
georgebearden Nov 30, 2022
fa4eb4b
Merge branch 'main' into aws-s3-sns
biffgaut Nov 30, 2022
6ee16f9
feat (cloudfront-to-s3): add optional parameter cloudfront.ResponseHe…
tbelmega Nov 23, 2022
69c6ba3
fix (cloudfront-to-s3): add missing line to fix failing test
tbelmega Nov 25, 2022
6a14de7
fix (cloudfront-to-s3): add link in READMEs to https://docs.aws.amazo…
tbelmega Nov 28, 2022
47a9399
refactoring (cloudfront-to-s3): rename _api variable to api …
tbelmega Nov 28, 2022
b7aecd0
refactoring (cloudfront-to-s3): incorporate validateSecurityHeadersBe…
tbelmega Nov 28, 2022
073b35b
chore(release): 2.28.0
aws-solutions-constructs-team Nov 30, 2022
b4b7c4a
chore(changelog): Updated CHANGELOG.md
biffgaut Nov 30, 2022
a16048c
CDK Upgrade Sync
biffgaut Nov 30, 2022
2a08f10
CDK Upgrade Sync
biffgaut Nov 30, 2022
f0a0745
fix (cloudfront-to-s3): add closing bracket
tbelmega Dec 1, 2022
0288b6b
fix (cloudfront-to-s3): flipped logic when changing || to ? :
tbelmega Dec 1, 2022
bace851
fix (cloudfront-to-s3): change httpSecurityHeaders to insertHttpSecur…
tbelmega Dec 1, 2022
3ffe4b1
update test-helper function to assert inline.
georgebearden Dec 2, 2022
d330ada
Merge branch 'main' into aws-s3-sns
georgebearden Dec 2, 2022
7048e3f
fix aws-s3-sns package.json version number
georgebearden Dec 2, 2022
1f6d9bc
Add cfn nag suppression for test that is intended to use unencrypted …
georgebearden Dec 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions DESIGN_GUIDELINES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,8 +307,9 @@ Existing Inconsistencies would not be published, that’s for our internal use

| Name | Type | Description | Notes |
| --- | --- | --- |--- |
| existingTopicObj? | [`sns.Topic`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_sns.Topic.html)|An optional, existing SNS topic to be used instead of the default topic. Providing both this and `topicProps` will cause an error|
| topicProps? | [`sns.TopicProps`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_sns.TopicProps.html)|Optional user provided properties to override the default properties for the SNS topic.
| existingTopicObj? | [`sns.Topic`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_sns.Topic.html)|An optional, existing SNS topic to be used instead of the default topic. Providing both this and `topicProps` will cause an error. If the SNS Topic is encrypted with a Customer-Managed KMS Key, the key must be specified in the `existingTopicEncryptionKey` property. |
| existingTopicEncryptionKey? | [`kms.Key`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_kms.Key.html) | If an existing topic is provided in the `existingTopicObj` property, and that topic is encrypted with a Customer-Managed KMS key, this property also needs to be set with same key. |
| topicProps? | [`sns.TopicProps`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_sns.TopicProps.html)|Optional user provided properties to override the default properties for the SNS topic. |
| enableEncryptionWithCustomerManagedKey? | `boolean`|If no key is provided, this flag determines whether the SNS Topic is encrypted with a new CMK or an AWS managed key.|This flag is ignored if any of the following are defined: topicProps.masterKey, encryptionKey or encryptionKeyProps.| Sending messages from an AWS service to an encrypted Topic [requires a Customer Master key](https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGuide/sqs-key-management.html#compatibility-with-aws-services). Those constructs require these properties. |
| encryptionKey? | [`kms.Key`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_kms.Key.html)|An optional, imported encryption key to encrypt the SNS Topic with.|
| encryptionKeyProps? | [`kms.KeyProps`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_kms.KeyProps.html)|Optional user provided properties to override the default properties for the KMS encryption key used to encrypt the SNS Topic with. |
Expand Down
4 changes: 2 additions & 2 deletions source/patterns/@aws-solutions-constructs/aws-s3-sns/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ new S3ToSns(this, "S3ToSNSPattern", new S3ToSnsProps.Builder()
|s3EventFilters?|[`s3.NotificationKeyFilter[]`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_s3.NotificationKeyFilter.html)|S3 object key filter rules to determine which objects trigger this event. If not specified no filter rules will be applied.|
|loggingBucketProps?|[`s3.BucketProps`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.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|
|existingTopicObj?|[`sns.Topic`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_sns.Topic.html)|Existing SNS topic to be used instead of the default topic. Providing both this and `topicProps` will cause an error. If the SNS Topic is encrypted, the KMS key utilized for encryption must be a customer managed KMS key and it must be specified in the `existingTopicEncryptionKey` property|
|existingTopicEncryptionKey?|[`kms.Key`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_kms.Key.html)|If an existing topic is provided in the `existingTopicObj` property, and that topic is encrypted with a customer managed KMS key, this property also needs to be set with same CMK.|
|existingTopicObj?|[`sns.Topic`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_sns.Topic.html)|An optional, existing SNS topic to be used instead of the default topic. Providing both this and `topicProps` will cause an error. If the SNS Topic is encrypted with a Customer-Managed KMS Key, the key must be specified in the `existingTopicEncryptionKey` property.|
|existingTopicEncryptionKey?|[`kms.Key`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_kms.Key.html)|If an existing topic is provided in the `existingTopicObj` property, and that topic is encrypted with a Customer-Managed KMS key, this property also needs to be set with same key.|
|topicProps?|[`sns.TopicProps`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_sns.TopicProps.html)|Optional user provided props to override the default props for the SNS topic.|
|enableEncryptionWithCustomerManagedKey?|`boolean`|If no key is provided, this flag determines whether the topic is encrypted with a new CMK or an AWS managed key. This flag is ignored if any of the following are defined: topicProps.encryptionMasterKey, encryptionKey or encryptionKeyProps.|
|encryptionKey?|[`kms.Key`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_kms.Key.html)|An optional, imported encryption key to encrypt the SNS Topic with.|
Expand Down
48 changes: 19 additions & 29 deletions source/patterns/@aws-solutions-constructs/aws-s3-sns/lib/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,16 +64,15 @@ export interface S3ToSnsProps {
*/
readonly logS3AccessLogs?: boolean;
/**
* Existing SNS topic to be used instead of the default topic. Providing both this and `topicProps` will cause an error.
* If the SNS Topic is encrypted, the KMS key utilized for encryption must be a customer managed KMS key and it must be
* specified in the `existingTopicEncryptionKey` property.
* An optional, existing SNS topic to be used instead of the default topic. Providing both this and `topicProps` will cause an error.
* If the SNS Topic is encrypted with a Customer-Managed KMS Key, the key must be specified in the `existingTopicEncryptionKey` property.
*
* @default - Default props are used
*/
readonly existingTopicObj?: sns.Topic;
/**
* If an existing topic is provided in the `existingTopicObj` property, and that topic is encrypted with a customer managed KMS key,
* this property also needs to be set with same CMK.
* If an existing topic is provided in the `existingTopicObj` property, and that topic is encrypted with a Customer-Managed KMS key,
* this property also needs to be set with same key.
*
* @default - None
*/
Expand Down Expand Up @@ -127,13 +126,8 @@ export class S3ToSns extends Construct {
super(scope, id);
defaults.CheckProps(props);

let bucket: s3.Bucket;
let enableEncryptionParam = props.enableEncryptionWithCustomerManagedKey;

if (props.enableEncryptionWithCustomerManagedKey === undefined ||
props.enableEncryptionWithCustomerManagedKey === true) {
enableEncryptionParam = true;
}
// If the enableEncryptionWithCustomerManagedKey is undefined, default it to true
const enableEncryptionParam = props.enableEncryptionWithCustomerManagedKey === false ? false : true;

// Setup the S3 bucket
if (!props.existingBucketObj) {
Expand All @@ -142,42 +136,38 @@ export class S3ToSns extends Construct {
loggingBucketProps: props.loggingBucketProps,
logS3AccessLogs: props.logS3AccessLogs
});
bucket = this.s3Bucket;
this.s3BucketInterface = this.s3Bucket;
} else {
bucket = props.existingBucketObj;
this.s3BucketInterface = props.existingBucketObj;
}

this.s3BucketInterface = bucket;

// Setup the topic
[this.snsTopic, this.encryptionKey] = defaults.buildTopic(this, {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

existingTopicObj: props.existingTopicObj,
existingTopicEncryptionKey: props.existingTopicEncryptionKey,
topicProps: props.topicProps,
enableEncryptionWithCustomerManagedKey: enableEncryptionParam,
encryptionKey: props.encryptionKey,
encryptionKeyProps: props.encryptionKeyProps
});

if (props.existingTopicEncryptionKey) {
this.encryptionKey = props.existingTopicEncryptionKey;
}

// Setup the S3 bucket event types
const s3EventTypes = props.s3EventTypes ? props.s3EventTypes : defaults.defaultS3NotificationEventTypes;
const s3EventTypes = props.s3EventTypes ?? defaults.defaultS3NotificationEventTypes;

// Setup the S3 bucket event filters
const s3Eventfilters = props.s3EventFilters ? props.s3EventFilters : [];
const s3Eventfilters = props.s3EventFilters ?? [];

// Setup the S3 bucket event notifications
s3EventTypes.forEach(type => bucket.addEventNotification(type, new s3n.SnsDestination(this.snsTopic), ...s3Eventfilters));
s3EventTypes.forEach((type) => {
const destination = new s3n.SnsDestination(this.snsTopic);
this.s3BucketInterface.addEventNotification(type, destination, ...s3Eventfilters);
});

// Grant S3 permission to use the topic's encryption key so it can publish messages to it
if (this.encryptionKey) {
this.encryptionKey.grant(new iam.ServicePrincipal("s3.amazonaws.com"),
'kms:Decrypt',
'kms:GenerateDataKey*',
);
}
this.encryptionKey?.grant(new iam.ServicePrincipal("s3.amazonaws.com"),
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the topic is not encrypted and this.encryptionKey is undefined, won't this break?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is using optional chaining in typescript, so if encryptionKey evaluates to null, it will immediately stop running the expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created a new set of unit/integ tests to prove it out.

'kms:Decrypt',
'kms:GenerateDataKey*',
);

addCfnNagS3BucketNotificationRulesToSuppress(Stack.of(this), 'BucketNotificationsHandler050a0587b7544547bf325f094a3db834');
}
Expand Down
28 changes: 20 additions & 8 deletions source/patterns/@aws-solutions-constructs/aws-s3-sns/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,20 @@
}
},
"dependencies": {
"@aws-solutions-constructs/core": "2.27.0"
"@aws-cdk/aws-s3": "0.0.0",
"@aws-cdk/aws-s3-notifications": "0.0.0",
"@aws-cdk/aws-sns": "0.0.0",
"@aws-cdk/aws-lambda": "0.0.0",
"@aws-cdk/aws-iam": "0.0.0",
"@aws-cdk/aws-kms": "0.0.0",
"@aws-cdk/core": "0.0.0",
"@aws-solutions-constructs/core": "0.0.0",
"constructs": "^3.2.0"
},
"devDependencies": {
"@aws-cdk/assert": "2.50.0",
"@aws-cdk/assert": "0.0.0",
"@types/jest": "^27.4.0",
"@types/node": "^10.3.0",
"aws-cdk-lib": "2.50.0",
"constructs": "^10.0.0"
"@types/node": "^10.3.0"
},
"jest": {
"moduleFileExtensions": [
Expand All @@ -77,9 +83,15 @@
]
},
"peerDependencies": {
"@aws-solutions-constructs/core": "2.27.0",
"aws-cdk-lib": "^2.50.0",
"constructs": "^10.0.0"
"@aws-cdk/aws-s3": "0.0.0",
"@aws-cdk/aws-s3-notifications": "0.0.0",
"@aws-cdk/aws-sns": "0.0.0",
"@aws-cdk/aws-lambda": "0.0.0",
"@aws-cdk/aws-iam": "0.0.0",
"@aws-cdk/aws-kms": "0.0.0",
"@aws-cdk/core": "0.0.0",
"@aws-solutions-constructs/core": "0.0.0",
"constructs": "^3.2.0"
},
"keywords": [
"aws",
Expand Down
Loading