feat(iot-actions): add SNS publish action #18839

AdamVD · 2022-02-05T20:56:41Z

First-time contributor 👋

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

gitpod-io · 2022-02-05T20:56:44Z

AdamVD · 2022-02-05T21:07:18Z

packages/@aws-cdk/aws-iot-actions/lib/sns-topic-action.ts

+   * @param props Properties to configure the action.
+   */
+  constructor(topic: sns.ITopic, props: SnsTopicActionProps = {}) {
+    if (topic.topicName.endsWith('.fifo')) {


Discussion probably needed here.

On the implementation: I don't like having to check for the ".fifo" suffix directly, but ITopic doesn't expose anything to check if it is FIFO (unlike IQueue.fifo). However, since the suffix is mandated by the service this may be adequate until a better option comes available.

On whether or not the error should even be thrown: The docs for this action clearly state that FIFO topics are not supported due to the distributed nature of the rules engine. That said, there isn't any validation done by the Rules service to reject a configuration with a FIFO topic. I ran a test with a FIFO queue as the target and received an error in the IoT logs Invalid parameter: The MessageGroupId parameter is required for FIFO topics (Service: AmazonSNS; Status Code: 400; Error Code: InvalidParameter;. So I'll argue that throwing this error is likely to save at least a couple people from headache and wasted time debugging a bad configuration.

It's also worth noting that the current SQS action implementation does not perform this check, even though the same FIFO limitation applies. If we decide this check is justified we might want to create a ticket to apply the same in the SQS action.

I'm amazed at your great insight! And I think it's good:+1:.
Reference for reviewer: the API reference about .fifo sufix.

I'll second @yamatatsu's comment, this is some excellent detective work @AdamVD! I agree with everything you said.

One subtlety here. Note that topicName could be something that CDK calls a Token. For example, in your integ tests, notice that tokenName resolves to { Ref: 'MyTopic86869434' }.

Because of that, it would be a good idea to call the Token.isUnresolved() method, and only do the check if that method returns false (otherwise, we're comparing against the encoded Token, which doesn't make too much sense).

Also, if we need ITopic to have a fifo property, like IQueue does - let's do it! If this is a small change in the SNS module, I'm fine adding it to this PR, otherwise, let's create an issue in the backlog for it, and follow-up after this PR gets merged.

Let me know what you think @AdamVD!

Thanks @skinny85 this is a great call out, the Token concept is new new me and I don't yet understand exactly when it is present. However, I wonder if #18740 makes it so that the name of a FIFO topic is always resolved. Regardless, this implementation shouldn't be dependent on a temporary shortcoming of another module, so I'll look into expanding ITopic with a fifo property as a more robust solution.

AdamVD · 2022-02-05T21:18:39Z

packages/@aws-cdk/aws-iot-actions/lib/sns-topic-action.ts

+    this.messageFormat = props.messageFormat;
+  }
+
+  bind(rule: iot.ITopicRule): iot.ActionConfig {


Limitation here is we don't check if the SNS topic is encrypted and adjust the role or KMS resource policy accordingly. ITopic doesn't expose encryption details so I'm not sure how we'd perform this check (unlike IQueue.encryptionMasterKey). The current SQS queue action has the same limitation.

What if we used the grantPublish() method of ITopic, instead of creating the Policy directly?

If that method doesn't handle permissions to the Key correctly, then that's a bug in the SNS library!

You're right, I should use the grantPublish() method. Unfortunately, it looks like this is another limitation of the current SNS topic implementation. The grant method doesn't take encryption into account.

From topic-base.ts:

/** * Grant topic publishing permissions to the given identity */ public grantPublish(grantee: iam.IGrantable) { return iam.Grant.addToPrincipalOrResource({ grantee, actions: ['sns:Publish'], resourceArns: [this.topicArn], resource: this, }); }

Compare this against queue-base.ts:

public grantSendMessages(grantee: iam.IGrantable) { const ret = this.grant(grantee, 'sqs:SendMessage', 'sqs:GetQueueAttributes', 'sqs:GetQueueUrl'); if (this.encryptionMasterKey) { // kms:Decrypt necessary to execute grantsendMessages to an SSE enabled SQS queue this.encryptionMasterKey.grantEncryptDecrypt(grantee); } return ret; }

This is concisely described in #18387, and can be assumed to affect all other SNS topic integrations (e.g. #16271, #11121) unless a separate workaround was implemented. I'd be happy to look into this and and try to bring it in-line with what SQS queues do, but that probably shouldn't be a part of this PR. What do you think @skinny85?

With that said, changing this implementation to use grantPublish() means that no further changes will be required once #18387 is fixed.

I agree with everything you said 🙂. Ideally, this would be a separate PR that only touches the SNS module, but if you'd rather do it all in one, because that will be easier for you - go for it. You're fixing bugs in our project, I won't be too harsh 😉.

yamatatsu

It is so grad for me to see more committers!:tada:
I reviewed with perspective I've taked comments by maintainer usually. It is aimed to reduce suggestions the maintainer will do.

yamatatsu · 2022-02-06T03:12:55Z

packages/@aws-cdk/aws-iot-actions/lib/sns-topic-action.ts

+   * @param props Properties to configure the action.
+   */
+  constructor(topic: sns.ITopic, props: SnsTopicActionProps = {}) {
+    if (topic.topicName.endsWith('.fifo')) {


I'm amazed at your great insight! And I think it's good:+1:.
Reference for reviewer: the API reference about .fifo sufix.

yamatatsu · 2022-02-06T03:31:31Z

packages/@aws-cdk/aws-iot-actions/lib/sns-topic-action.ts

+  /**
+   * RAW message format.
+   */
+  RAW = 'RAW',
+  /**
+   * JSON message format.
+   */
+  JSON = 'JSON'


Empty lines between definitions:

Suggested change

/**

* RAW message format.

*/

RAW = 'RAW',

/**

* JSON message format.

*/

JSON = 'JSON'

/**

* RAW message format.

*/

RAW = 'RAW',

/**

* JSON message format.

*/

JSON = 'JSON'

yamatatsu · 2022-02-06T04:01:39Z

packages/@aws-cdk/aws-iot-actions/test/sns/integ.sns-topic-action.ts

+import * as cdk from '@aws-cdk/core';
+import * as actions from '../../lib';
+
+const app = new cdk.App();


This line is better above the line it uses app:

const app = new cdk.App(); new TestStack(app, 'test-stack');

yamatatsu · 2022-02-06T04:02:59Z

packages/@aws-cdk/aws-iot-actions/test/sns/integ.sns-topic-action.ts

+  }
+}
+
+new TestStack(app, 'test-stack');


Let's use more clear name:

Suggested change

new TestStack(app, 'test-stack');

new TestStack(app, 'sns-topic-action-test-stack');

yamatatsu · 2022-02-06T05:00:53Z

packages/@aws-cdk/aws-iot-actions/test/sns/sns-topic-action.test.ts

+      Actions: [
+        {
+          Sns: {
+            RoleArn: {
+              'Fn::GetAtt': [
+                RULE_ROLE_ID,
+                'Arn',
+              ],
+            },
+            TargetArn: SNS_TOPIC_ARN,
+          },
+        },
+      ],


Less indentions:

Suggested change

Actions: [

{

Sns: {

RoleArn: {

'Fn::GetAtt': [

RULE_ROLE_ID,

'Arn',

],

},

TargetArn: SNS_TOPIC_ARN,

},

},

],

Actions: [{

Sns: {

RoleArn: { 'Fn::GetAtt': [RULE_ROLE_ID, 'Arn'] },

TargetArn: SNS_TOPIC_ARN,

},

}],

yamatatsu · 2022-02-06T05:02:07Z

packages/@aws-cdk/aws-iot-actions/test/sns/sns-topic-action.test.ts

+      Statement: [
+        {
+          Action: 'sts:AssumeRole',
+          Effect: 'Allow',
+          Principal: {
+            Service: 'iot.amazonaws.com',
+          },
+        },
+      ],


Less indentions:

Suggested change

Statement: [

{

Action: 'sts:AssumeRole',

Effect: 'Allow',

Principal: {

Service: 'iot.amazonaws.com',

},

},

],

Statement: [{

Action: 'sts:AssumeRole',

Effect: 'Allow',

Principal: { Service: 'iot.amazonaws.com' },

}],

yamatatsu · 2022-02-06T05:02:55Z

packages/@aws-cdk/aws-iot-actions/test/sns/sns-topic-action.test.ts

+      Statement: [
+        {
+          Action: 'sns:Publish',
+          Effect: 'Allow',
+          Resource: SNS_TOPIC_ARN,
+        },
+      ],


Less indentions:

Suggested change

Statement: [

{

Action: 'sns:Publish',

Effect: 'Allow',

Resource: SNS_TOPIC_ARN,

},

],

Statement: [{

Action: 'sns:Publish',

Effect: 'Allow',

Resource: SNS_TOPIC_ARN,

}],

yamatatsu · 2022-02-06T05:05:11Z

packages/@aws-cdk/aws-iot-actions/test/sns/sns-topic-action.test.ts

+    Roles: [
+      { Ref: RULE_ROLE_ID },
+    ],


Less indentions:

Suggested change

Roles: [

{ Ref: RULE_ROLE_ID },

],

Roles: [{ Ref: RULE_ROLE_ID }],

@yamatatsu

Thanks @yamatatsu

AdamVD · 2022-02-06T12:32:37Z

Thanks for the warm welcome @yamatatsu and the suggestions! Those formatting changes really improve the readability of the tests.

skinny85

Welcome to the CDK contributors @AdamVD! We're very lucky to have you, as your first contribution is of very high quality already 🙂.

I have a few comments, but they're all very minor.

skinny85 · 2022-02-07T21:14:23Z

packages/@aws-cdk/aws-iot-actions/lib/sns-topic-action.ts

+ * Configuration options for the SNS topic action.
+ */
+export interface SnsTopicActionProps extends CommonActionProps {
+


No need for this empty line here:

Suggested change

skinny85 · 2022-02-07T21:27:43Z

packages/@aws-cdk/aws-iot-actions/lib/sns-topic-action.ts

+   * @param props Properties to configure the action.
+   */
+  constructor(topic: sns.ITopic, props: SnsTopicActionProps = {}) {
+    if (topic.topicName.endsWith('.fifo')) {


I'll second @yamatatsu's comment, this is some excellent detective work @AdamVD! I agree with everything you said.

One subtlety here. Note that topicName could be something that CDK calls a Token. For example, in your integ tests, notice that tokenName resolves to { Ref: 'MyTopic86869434' }.

Because of that, it would be a good idea to call the Token.isUnresolved() method, and only do the check if that method returns false (otherwise, we're comparing against the encoded Token, which doesn't make too much sense).

Also, if we need ITopic to have a fifo property, like IQueue does - let's do it! If this is a small change in the SNS module, I'm fine adding it to this PR, otherwise, let's create an issue in the backlog for it, and follow-up after this PR gets merged.

Let me know what you think @AdamVD!

skinny85 · 2022-02-07T21:29:49Z

packages/@aws-cdk/aws-iot-actions/lib/sns-topic-action.ts

+   */
+  constructor(topic: sns.ITopic, props: SnsTopicActionProps = {}) {
+    if (topic.topicName.endsWith('.fifo')) {
+      throw Error('An SNS topic IoT Rule action cannot be configured with a FIFO topic, only standard topics are allowed.');


I think we can word-smith this error message a little bit:

Suggested change

throw Error('An SNS topic IoT Rule action cannot be configured with a FIFO topic, only standard topics are allowed.');

throw Error('IoT Rule actions cannot be used with FIFO SNS Topics, please pass a non-FIFO Topic instead');

skinny85 · 2022-02-07T21:35:11Z

packages/@aws-cdk/aws-iot-actions/lib/sns-topic-action.ts

+    this.messageFormat = props.messageFormat;
+  }
+
+  bind(rule: iot.ITopicRule): iot.ActionConfig {


What if we used the grantPublish() method of ITopic, instead of creating the Policy directly?

If that method doesn't handle permissions to the Key correctly, then that's a bug in the SNS library!

skinny85 · 2022-02-07T21:38:13Z

packages/@aws-cdk/aws-iot-actions/test/sns/sns-topic-action.test.ts

+import * as actions from '../../lib';
+
+const SNS_TOPIC_ARN = 'arn:aws:sns::123456789012:test-topic';
+const RULE_ROLE_ID = 'MyTopicRuleTopicRuleActionRoleCE2D05DA';


I would probably inline this variable (it's exact value depends on the specifics of the test it's used in, for example, it won't be generated in the explicit Role test).

@skinny85

Thanks @skinny85

@skinny85

Thanks @skinny85

Pull request has been modified.

AdamVD · 2022-02-08T01:47:14Z

@skinny85 thanks for the kind words and the review! I've addressed the trivial changes and will begin looking into exposing fifo from ITopic so that this implementation is more robust.

skinny85 · 2022-02-08T19:56:52Z

@skinny85 thanks for the kind words and the review! I've addressed the trivial changes and will begin looking into exposing fifo from ITopic so that this implementation is more robust.

My pleasure 🙂. Thank you for the contribution! Let me know when you're ready for the next round of reviews - just re-request my review (there's a button in the top-right of the PR window, next to my avatar).

skinny85

Looks fantastic, thanks so much for the contribution @AdamVD! I hope it's only the first one of many more to come 🙂.

mergify · 2022-02-09T21:10:27Z

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 · 2022-02-09T21:56:55Z

AWS CodeBuild CI Report

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

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

mergify · 2022-02-09T21:57:14Z

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).

First-time contributor 👋 fixes aws#17700 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

AdamVD added 11 commits February 5, 2022 14:07

Add SnsTopicAction class

3e76dfe

Update README

547a826

Add test with defaults

8ab37f6

Add test for messageFormat config

43a5017

Add test for role config

c7a7bb5

Test FIFO queue throws error

aa3827f

Add integration test

c243276

Add stack verification steps to integ test file

54e6c99

Remove TODOs

458cdf5

Add message format docstrings to pass lint

08e1208

Fix README example

8ab7c7a

github-actions bot added the @aws-cdk/aws-iot-actions label Feb 5, 2022

github-actions bot assigned skinny85 Feb 5, 2022

AdamVD commented Feb 5, 2022

View reviewed changes

yamatatsu approved these changes Feb 6, 2022

View reviewed changes

Improve formatting and naming

86a2fa2

Thanks @yamatatsu

skinny85 previously requested changes Feb 7, 2022

View reviewed changes

AdamVD added 2 commits February 8, 2022 01:38

Improve code format and wording

69f8ea4

Thanks @skinny85

Use topic grantPublish on rule role

d916076

Thanks @skinny85

Merge branch 'master' into feature/iot-rule-sns-action

23efd68

AdamVD added 3 commits February 8, 2022 20:19

Merge branch 'master' into feature/iot-rule-sns-action

b423449

Expose fifo attribute from ITopic

217da01

Use fifo attribute in input validation

2abb973

AdamVD requested a review from skinny85 February 9, 2022 02:09

skinny85 approved these changes Feb 9, 2022

View reviewed changes

Merge branch 'master' into feature/iot-rule-sns-action

061b9bd

mergify bot merged commit 3a39f6b into aws:master Feb 9, 2022

AdamVD deleted the feature/iot-rule-sns-action branch February 9, 2022 23:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(iot-actions): add SNS publish action #18839

feat(iot-actions): add SNS publish action #18839

AdamVD commented Feb 5, 2022 •

edited

Loading

gitpod-io bot commented Feb 5, 2022

AdamVD Feb 5, 2022

AdamVD Feb 5, 2022 •

edited

Loading

yamatatsu Feb 6, 2022

skinny85 Feb 7, 2022

AdamVD Feb 8, 2022

AdamVD Feb 5, 2022 •

edited

Loading

skinny85 Feb 7, 2022

AdamVD Feb 8, 2022

AdamVD Feb 8, 2022

skinny85 Feb 8, 2022

yamatatsu left a comment

yamatatsu Feb 6, 2022

yamatatsu Feb 6, 2022

yamatatsu Feb 6, 2022

yamatatsu Feb 6, 2022

yamatatsu Feb 6, 2022

yamatatsu Feb 6, 2022

yamatatsu Feb 6, 2022

yamatatsu Feb 6, 2022

AdamVD commented Feb 6, 2022

skinny85 left a comment

skinny85 Feb 7, 2022

skinny85 Feb 7, 2022

skinny85 Feb 7, 2022

skinny85 Feb 7, 2022

skinny85 Feb 7, 2022

AdamVD commented Feb 8, 2022

skinny85 commented Feb 8, 2022

skinny85 left a comment

mergify bot commented Feb 9, 2022

aws-cdk-automation commented Feb 9, 2022

mergify bot commented Feb 9, 2022

	new TestStack(app, 'test-stack');
	new TestStack(app, 'sns-topic-action-test-stack');

	throw Error('An SNS topic IoT Rule action cannot be configured with a FIFO topic, only standard topics are allowed.');
	throw Error('IoT Rule actions cannot be used with FIFO SNS Topics, please pass a non-FIFO Topic instead');

feat(iot-actions): add SNS publish action #18839

feat(iot-actions): add SNS publish action #18839

Conversation

AdamVD commented Feb 5, 2022 • edited Loading

gitpod-io bot commented Feb 5, 2022

Choose a reason for hiding this comment

AdamVD Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdamVD Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamatatsu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdamVD commented Feb 6, 2022

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdamVD commented Feb 8, 2022

skinny85 commented Feb 8, 2022

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Feb 9, 2022

aws-cdk-automation commented Feb 9, 2022

AWS CodeBuild CI Report

mergify bot commented Feb 9, 2022

AdamVD commented Feb 5, 2022 •

edited

Loading

AdamVD Feb 5, 2022 •

edited

Loading

AdamVD Feb 5, 2022 •

edited

Loading