feat(sns): adding support for firehose subscription protocol #15764

david-doyle-as24 · 2021-07-26T14:34:47Z

This PR adds support for the firehose subscription protocol by extending the protocol enum and by requiring the field "subscriptionRoleArn" if the protocol is set to firehose.

This is so that users can take advantage of the new SNS-to-Firehose integration introduced in February 2021 (see here for the announcement).

Here also is a link to the sample cloudformation, documenting the SNS-to-Firehose integration.

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

gitpod-io · 2021-07-26T14:34:50Z

mergify · 2021-07-26T14:35:23Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

david-doyle-as24 · 2021-08-11T17:03:04Z

@njlynch , is there anything that I can do on my side to move this forward?

inception-cto · 2021-08-16T02:48:57Z

I am curious why the role is not managed automatically by the construct and it has to be setup manually? @david-doyle-as24 , shall it be instead managed?

Also, we might want to think about adding a custom construct FirehoseSubscription to the @aws-cdk/aws-sns-subscriptions module.

njlynch

Thanks for the contribution, and apologies for the delay in first response!

I tend to agree with @melek-inception 's comments here -- this change is fine as-is as the bare minimum needed to support Firehose, but it seems like the right API here is to add a FirehoseSubscription construct to the aws-sns-subscriptions package, which would at least have the option of automatically creating the correct role for you.

Would you be willing to give that a shot? If not, someone else might be able to pick it up. My one comment with this as-is is on the renaming of the test.

njlynch · 2021-08-16T17:18:46Z

packages/@aws-cdk/aws-sns/test/subscription.test.ts

@@ -176,7 +176,7 @@ describe('Subscription', () => {

  });

-  test('throws with raw delivery for protocol other than http, https or sqs', () => {
+  test('throws with raw delivery for lambda protocol', () => {


❓ Why did you make this change? While the text more accurately describes the specifics of the test, they lose the intent behind the test.

@njlynch , the choice was between changing the name to "protocol other than http, https, sqs, or firehose" (since firehose supports raw delivery) or changing it to "lambda protocol", which has the advantage of being more accurate. As it is, the test name is misleading - it doesn't cover email or sms which also don't support raw delivery. Better just to name it more transparently.

Ah. If Firehose supports raw delivery, then it looks like this check needs to be updated:

aws-cdk/packages/@aws-cdk/aws-sns/lib/subscription.ts

Lines 84 to 86 in b838f95

if (props.rawMessageDelivery && ['http', 'https', 'sqs'].indexOf(props.protocol) < 0) {

throw new Error('Raw message delivery can only be enabled for HTTP/S and SQS subscriptions.');

}

You're accurate the test name is now more accurate, but it points out the inadequacy of it. Could you swap it to use a test.each and enumerate the protocols that should throw?

njlynch · 2021-08-16T17:21:43Z

packages/@aws-cdk/aws-sns/README.md

@@ -95,6 +95,20 @@ topic.addSubscription(new subs.LambdaSubscription(fn, {
 }));
 ```

+### Example of Firehose Subscription


~ (Nit) This is okay, if we're going to leave this implementation as the basics only. I'd greatly prefer we document it as part of a FirehoseSubscription in the aws-sns-subscriptions README instead though.

david-doyle-as24 · 2021-08-17T16:18:32Z

@njlynch and @melek-inception , would the FirehoseSubscription supplement this addition to the Subscription construct or replace it? I would prefer to build it on top of the Subscription construct in a separate PR.

njlynch · 2021-08-27T14:49:21Z

Sorry for the delayed response; this fell off my queue.

would the FirehoseSubscription supplement this addition to the Subscription construct or replace it?

Supplement. See the existing constructs in https://github.com/aws/aws-cdk/tree/master/packages/%40aws-cdk/aws-sns-subscriptions/lib for examples.

…ationRecord (aws#15782) add support for RemovalPolicy in CrossAccountZoneDelegationRecord closes aws#15211 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Fixes an issue where unresolved Listener `priority` would resolve in Error: `Priority must have value greater than or equal to 1`. This adds support for cases where the Listener `priority` can be supplied through CloudFormation Template Parameters. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…packages (aws#15826) Remove all hard coded references to the @aws-cdk/aws-lambda and @aws-cdk/aws-lambda-go packages from the tests which should fix the issue that prevent building the aws-cdk-lib package. Resolves aws#15585 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

fixes: aws#15831 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

… (under feature flag) (aws#15477) This pull request adds the new TLSv1.2_2021 security policy to the respective enum and adds the feature flag `@aws-cdk/aws-cloudfront:defaultSecurityPolicyTLSv1.2_2021`, which, when enabled, causes distributions to use the new security policy by default. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Feature flags -- by definition -- don't introduce breaking changes to existing customers, simply alter behavior for new apps, or allow opting in to new behavior. Additionally, our linter does not allow breaking changes in stable modules, which means as stands it's impossible to satisfy this requirement and introduce a feature flag for stable modules. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws#15725) Closes aws#15724 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

) Add ability to configure parameter options for when launching a product. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* Co-authored-by: Dillon Ponzo <dponzo18@gmail.com>

…s#15819) Fixes aws#15711. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

The class `TemplateAssertions` is already part of the `@aws-cdk/assertions` module. The word 'assertions' is redundant. Further, this class now has the method `findResources()` which is not an assertion. All assertion methods are named in a way that makes it clear that it is an assertion, viz., `hasResource()`, `resourceCountIs()`, `templateMatches()`. BREAKING CHANGE: `TemplateAssertions` is now renamed to `Template`. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Add support for BucketDeployment accessControl property to aws-s3-deployment package. This is needed to run, for example, `aws s3 sync` with `--acl bucket-owner-full-control`. Without this feature there is no easy way (without hacking cdk nodes) to sync assets to bucket located in another account. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…eCluster (aws#16296) fixes aws#16252 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

This will be used by a change in `tools/individual-pkg-gen/copy-files-removing-deps.ts` on the `v2-main` branch, so that the `transform` step can correctly set the `version` key and the version for each dependency in each alpha module's `package.json` file. See aws#16322 for usage. Part of aws#15591 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

) The tests for `yarn-cling` currently reference what were imaginary package names as placeholders; those package names have now been registered. While this has no practical impact, switching the package names to be ones we own just so there's no ambiguity. An `npm install` is never done on these directories, so the packages chosen really doesn't matter. Just picked two of our favorites at random. :) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

---- This PR adds a new service extension, `SubscribeExtension`. This extension can be added to a service to create SQS Queues which can subscribe to the SNS Topics provided by the user. It creates a default SQS Queue called `eventsQueue` . It also supports creation of topic-specific queues and sets up the SNS subscriptions accordingly. The created topic-queue subscriptions can be accessed using `subscriptions` field of the extension and the default queue for this service can be accessed using the `eventsQueue` getter method. (This PR does not include autoscaling, will be adding it in a separate PR) *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…#16172) Closes aws#16171. The expression is missing a right parentheses, and it is just as clear anyway as a new sentence. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…CodeBuildStep (aws#16294) fix(pipelines): Fix documentation regarding rolePolicyStatements of CodeBuildStep ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

---- Revisiting [aws#9445](aws#9445) but looking to see if running on linux with selinux enabled. This PR aims to allow for asset bundling on linux os with selinux enabled *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

This is the first PR implementing the ["Accelerated personal deployments" RFC](https://github.com/aws/aws-cdk-rfcs/blob/master/text/0001-cdk-update.md). It adds a (boolean) `--hotswap` flag to the `deploy` command that attempts to perform a short-circuit deployment, updating the resource directly, and skipping CloudFormation. If we detect that the current change cannot be short-circuited (because it contains an infrastructure change to the CDK code, most likely), we fall back on performing a full CloudFormation deployment, same as if `cdk deploy` was called without the `--hotswap` flag. In this PR, the new switch supports only Lambda functions. Later PRs will add support for new resource types. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…to tasks.EvaluateExpression (aws#16290) Currently the way `EvaluateExpression` is set up, if you pass an async expression to it, it never waits for the execution to complete. By adding an `await` before the `eval()` call, async ops can be passed in. This also works for sync ops same as before (await works with sync code as well). ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…aws#16348) Early on in the CDK history, a decision was made to delineate between subnets with Internet access (i.e., those with a NAT) and those without. The convention chosen at that time was to label the subnets as `PRIVATE` and `ISOLATED`, respectively. The intent was to make it clear that subnets without a NAT were completely isolated from the broader Internet (unless connected through another subnet). However, this introduction of a new subnet type that does not match EC2 documentation and naming conventions can cause confusion. Most critically, a user may select a `PRIVATE` subnet without realizing that it automatically requires one (or more) NAT gateways. As NAT gateways are not free, this can lead to unintended charges. To realign to the EC2 terminology -- while retaining the existing logic surrounding SubnetTypes -- the existing types of `PRIVATE` and `ISOLATED` are being renamed to `PRIVATE_WITH_NAT` and `PRIVATE_ISOLATED`, respectively. fixes aws#15929 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…s#15652) Add method `fromLookup` in KMS key which provides the option to get a KMS key including its key id by an alias name. In some cases, aliases can't be used because access to the underlying key id is necessary. In this case, the `fromLookup` method can be used. The following packages were changed: - @aws-cdk/aws-kms: introduce new `fromLookup` method - @aws-cdk/cx-api: new KeyContextResponse - @aws-cdk/cloud-assembly-schema: new ContextProvider KEY_PROVIDER and KeyContextQuery - aws-cdk: implementation of key ContextProvider Closes aws#8822 ----- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Add Python 3.9 to the list of supported runtimes. closes: aws#16144 , aws#16355 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

… params (aws#16121) Resolves aws#16004 When using CloudformationInit, can opt to include the `--role` and `--url` argument to `cfn-init` and `cfn-signal` ([ref](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/cfn-init.html#cfn-init-Syntax)). User can do this by including [InitOptions](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-ec2.ApplyCloudFormationInitOptions.html) that include `includeUrl` and `includeRole` ```typescript ec2.Instance(this, 'MyInstance', { init: // init config, initOptions: { includeUrl: true, includeRole: true, } } ``` It's possible to _always_ include these added arguments, but not sure whether that would cause regressions. Open to any suggestions on a better way to implement ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Adds a static instance of InterfaceVpcEndpointAwsService for the AWS Keyspaces service so that users do not need to define it manually. Keyspaces uses a custom TCP port (9142), so this avoids confusion. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…ws#15956) Allow specifying the ALPN Policy ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…le without casting it to `IHttpStage` (aws#15607) ---- Closes aws#14787. Changes the defaultStage to an IHttpStage. *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…ecutionContext even when object is empty (aws#16141) fixes: aws#16133 As described in aws#16133, the empty `QueryExecutionContext`, which was included before this fix, leads to an error. To mitigate this, I implemented a check to exclude the `QueryExecutionContext`when both params are empty. This way, it behaves like a truly optional parameter. Additionally I integrated an easier way to merge an OutputLocation if provided. This means less redundancy than the original solution, which returned an object that was only different in one key in each branch. The unittest is based on the default unittest, but a bit "stripped down". It proofs that no empty `QueryExecutionContext` object is present. In my tests, this fixes the ``` { "resourceType": "athena", "resource": "startQueryExecution.sync", "error": "Athena.InvalidRequestException", "cause": "Both queryExecutionContext.catalog and queryExecutionContext.database are null or empty (Service: AmazonAthena; Status Code: 400; Error Code: InvalidRequestException; Request ID: [Censored]; Proxy: null)" } ``` error. This solution turned out to be a bit different than I envisioned in the issue. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Pull request has been modified.

njlynch

Approving as-is for now (apologies for the delay).

Ideally, we would implement the FirehoseSubscription construct to make this whole thing much simpler for users, but that work will have to come in a separate PR. Volunteers welcome!

mergify · 2021-09-10T09:28:35Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2021-09-10T10:19:48Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 7d928a6
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2021-09-10T10:20:01Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

david-doyle-as24 added 2 commits July 26, 2021 16:27

adding firehose subscription support

8627c08

removing firehose from package.json

c7cc346

david-doyle-as24 changed the title ~~feat(sns) - adding support for firehose subscription protocol~~ feat(sns): adding support for firehose subscription protocol Jul 26, 2021

Merge branch 'master' into adding-firehose-support

a8ba85d

peterwoodworth assigned njlynch Jul 28, 2021

peterwoodworth added the @aws-cdk/aws-sns Related to Amazon Simple Notification Service label Jul 28, 2021

david-doyle-as24 added 2 commits July 29, 2021 20:59

adding documentation

8348cfe

Merge branch 'master' into adding-firehose-support

08f1647

njlynch added effort/small Small work item – less than a day of effort p1 labels Jul 30, 2021

david-doyle-as24 added 2 commits July 30, 2021 15:20

fixing typo

5eb7c1b

Merge branch 'master' into adding-firehose-support

25568c3

njlynch previously requested changes Aug 16, 2021

View reviewed changes

ayush987goyal and others added 11 commits September 7, 2021 12:15

fix(ec2): volumename doesn't set name of volume (aws#15832)

08843e3

fixes: aws#15831 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

feat(kinesisfirehose): add support for backing up source records to S3 (

1961e11

aws#15725) Closes aws#15724 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

fix(pipelines): Prepare stage doesn't have AUTO_EXPAND capability (aw…

3dd9347

…s#15819) Fixes aws#15711. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

johsbk and others added 21 commits September 7, 2021 12:15

feat(stepfunctions-tasks): support allocation strategies in EMR Creat…

d7ba92d

…eCluster (aws#16296) fixes aws#16252 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

chore(release): 1.121.0

79c5adf

Update CHANGELOG.md

6d00c85

Update CHANGELOG.md

a69ff4c

Apply suggestions from code review

05aca34

feat(lambda): python 3.9 runtime (aws#16366)

665d038

Add Python 3.9 to the list of supported runtimes. closes: aws#16144 , aws#16355 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

feat(elasticloadbalancingv2): ALPN policy support for NLB listener (a…

098c014

…ws#15956) Allow specifying the ALPN Policy ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

david-doyle-as24 added 2 commits September 7, 2021 13:07

making requested PR changes

e33d8c6

Merge branch 'master' into adding-firehose-support

b5a1258

njlynch approved these changes Sep 10, 2021

View reviewed changes

Merge branch 'master' into adding-firehose-support

7d928a6

mergify bot merged commit 18aff6b into aws:master Sep 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sns): adding support for firehose subscription protocol #15764

feat(sns): adding support for firehose subscription protocol #15764

david-doyle-as24 commented Jul 26, 2021 •

edited

Loading

gitpod-io bot commented Jul 26, 2021

mergify bot commented Jul 26, 2021

david-doyle-as24 commented Aug 11, 2021

inception-cto commented Aug 16, 2021 •

edited

Loading

njlynch left a comment

njlynch Aug 16, 2021

david-doyle-as24 Aug 17, 2021

njlynch Aug 27, 2021

njlynch Aug 16, 2021

david-doyle-as24 commented Aug 17, 2021 •

edited

Loading

njlynch commented Aug 27, 2021

njlynch left a comment

mergify bot commented Sep 10, 2021

aws-cdk-automation commented Sep 10, 2021

mergify bot commented Sep 10, 2021

	if (props.rawMessageDelivery && ['http', 'https', 'sqs'].indexOf(props.protocol) < 0) {
	throw new Error('Raw message delivery can only be enabled for HTTP/S and SQS subscriptions.');
	}

feat(sns): adding support for firehose subscription protocol #15764

feat(sns): adding support for firehose subscription protocol #15764

Conversation

david-doyle-as24 commented Jul 26, 2021 • edited Loading

gitpod-io bot commented Jul 26, 2021

mergify bot commented Jul 26, 2021

david-doyle-as24 commented Aug 11, 2021

inception-cto commented Aug 16, 2021 • edited Loading

njlynch left a comment

Choose a reason for hiding this comment

njlynch Aug 16, 2021

Choose a reason for hiding this comment

david-doyle-as24 Aug 17, 2021

Choose a reason for hiding this comment

njlynch Aug 27, 2021

Choose a reason for hiding this comment

njlynch Aug 16, 2021

Choose a reason for hiding this comment

david-doyle-as24 commented Aug 17, 2021 • edited Loading

njlynch commented Aug 27, 2021

njlynch left a comment

Choose a reason for hiding this comment

mergify bot commented Sep 10, 2021

aws-cdk-automation commented Sep 10, 2021

AWS CodeBuild CI Report

mergify bot commented Sep 10, 2021

david-doyle-as24 commented Jul 26, 2021 •

edited

Loading

inception-cto commented Aug 16, 2021 •

edited

Loading

david-doyle-as24 commented Aug 17, 2021 •

edited

Loading