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.

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(iot-actions): add SNS publish action #18839

Merged
merged 19 commits into from
Feb 9, 2022
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
22 changes: 22 additions & 0 deletions packages/@aws-cdk/aws-iot-actions/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ Currently supported are:
- Put records to Kinesis Data stream
- Put records to Kinesis Data Firehose stream
- Send messages to SQS queues
- Publish messages on SNS topics

## Republish a message to another MQTT topic

Expand Down Expand Up @@ -256,3 +257,24 @@ const topicRule = new iot.TopicRule(this, 'TopicRule', {
],
});
```

## Publish messages on an SNS topic

The code snippet below creates and AWS IoT Rule that publishes messages to an SNS topic when it is triggered:

```ts
import * as sns from '@aws-cdk/aws-sns';

const topic = new sns.Topic(this, 'MyTopic');

const topicRule = new iot.TopicRule(this, 'TopicRule', {
sql: iot.IotSql.fromStringAsVer20160323(
"SELECT topic(2) as device_id, year, month, day FROM 'device/+/data'",
),
actions: [
new actions.SnsTopicAction(topic, {
messageFormat: actions.SnsActionMessageFormat.JSON, // optional property, default is SnsActionMessageFormat.RAW
}),
],
});
```
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-iot-actions/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ export * from './kinesis-put-record-action';
export * from './lambda-function-action';
export * from './s3-put-object-action';
export * from './sqs-queue-action';
export * from './sns-topic-action';
79 changes: 79 additions & 0 deletions packages/@aws-cdk/aws-iot-actions/lib/sns-topic-action.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import * as iam from '@aws-cdk/aws-iam';
import * as iot from '@aws-cdk/aws-iot';
import * as sns from '@aws-cdk/aws-sns';
import { CommonActionProps } from '.';
import { singletonActionRole } from './private/role';

/**
* SNS topic action message format options.
*/
export enum SnsActionMessageFormat {
/**
* RAW message format.
*/
RAW = 'RAW',

/**
* JSON message format.
*/
JSON = 'JSON'
}

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

Copy link
Contributor

Choose a reason for hiding this comment

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

No need for this empty line here:

Suggested change

/**
* The message format of the message to publish.
*
* SNS uses this setting to determine if the payload should be parsed and relevant platform-specific bits of the payload should be extracted.
* @see https://docs.aws.amazon.com/sns/latest/dg/sns-message-and-json-formats.html
*
* @default SnsActionMessageFormat.RAW
*/
readonly messageFormat?: SnsActionMessageFormat;
}

/**
* The action to write the data from an MQTT message to an Amazon SNS topic.
*
* @see https://docs.aws.amazon.com/iot/latest/developerguide/sns-rule-action.html
*/
export class SnsTopicAction implements iot.IAction {
private readonly role?: iam.IRole;
private readonly topic: sns.ITopic;
private readonly messageFormat?: SnsActionMessageFormat;

/**
* @param topic The Amazon SNS topic to publish data on. Must not be a FIFO topic.
* @param props Properties to configure the action.
*/
constructor(topic: sns.ITopic, props: SnsTopicActionProps = {}) {
if (topic.topicName.endsWith('.fifo')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@AdamVD AdamVD Feb 5, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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');

}

this.topic = topic;
this.role = props.role;
this.messageFormat = props.messageFormat;
}

bind(rule: iot.ITopicRule): iot.ActionConfig {
Copy link
Contributor Author

@AdamVD AdamVD Feb 5, 2022

Choose a reason for hiding this comment

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

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.

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 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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

const role = this.role ?? singletonActionRole(rule);
role.addToPrincipalPolicy(new iam.PolicyStatement({
actions: ['sns:Publish'],
resources: [this.topic.topicArn],
}));

return {
configuration: {
sns: {
targetArn: this.topic.topicArn,
roleArn: role.roleArn,
messageFormat: this.messageFormat,
},
},
};
}
}
2 changes: 2 additions & 0 deletions packages/@aws-cdk/aws-iot-actions/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@
"@aws-cdk/aws-lambda": "0.0.0",
"@aws-cdk/aws-logs": "0.0.0",
"@aws-cdk/aws-s3": "0.0.0",
"@aws-cdk/aws-sns": "0.0.0",
"@aws-cdk/aws-sqs": "0.0.0",
"@aws-cdk/core": "0.0.0",
"case": "1.6.3",
Expand All @@ -110,6 +111,7 @@
"@aws-cdk/aws-lambda": "0.0.0",
"@aws-cdk/aws-logs": "0.0.0",
"@aws-cdk/aws-s3": "0.0.0",
"@aws-cdk/aws-sns": "0.0.0",
"@aws-cdk/aws-sqs": "0.0.0",
"@aws-cdk/core": "0.0.0",
"constructs": "^3.3.69"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
{
"Resources": {
"TopicRule40A4EA44": {
"Type": "AWS::IoT::TopicRule",
"Properties": {
"TopicRulePayload": {
"Actions": [
{
"Sns": {
"RoleArn": {
"Fn::GetAtt": [
"TopicRuleTopicRuleActionRole246C4F77",
"Arn"
]
},
"TargetArn": {
"Ref": "MyTopic86869434"
}
}
}
],
"AwsIotSqlVersion": "2016-03-23",
"Sql": "SELECT topic(2) as device_id, year, month, day FROM 'device/+/data'"
}
}
},
"TopicRuleTopicRuleActionRole246C4F77": {
"Type": "AWS::IAM::Role",
"Properties": {
"AssumeRolePolicyDocument": {
"Statement": [
{
"Action": "sts:AssumeRole",
"Effect": "Allow",
"Principal": {
"Service": "iot.amazonaws.com"
}
}
],
"Version": "2012-10-17"
}
}
},
"TopicRuleTopicRuleActionRoleDefaultPolicy99ADD687": {
"Type": "AWS::IAM::Policy",
"Properties": {
"PolicyDocument": {
"Statement": [
{
"Action": "sns:Publish",
"Effect": "Allow",
"Resource": {
"Ref": "MyTopic86869434"
}
}
],
"Version": "2012-10-17"
},
"PolicyName": "TopicRuleTopicRuleActionRoleDefaultPolicy99ADD687",
"Roles": [
{
"Ref": "TopicRuleTopicRuleActionRole246C4F77"
}
]
}
},
"MyTopic86869434": {
"Type": "AWS::SNS::Topic"
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* Stack verification steps:
* * aws sns subscribe --topic-arn "arn:aws:sns:<region>:<account>:test-stack-MyTopic86869434-10F6E3DMK3E5P" --protocol email --notification-endpoint <email-addr>
* * confirm subscription from email
* * echo '{"message": "hello world"}' > testfile.txt
* * aws iot-data publish --topic device/mydevice/data --qos 1 --payload fileb://testfile.txt
* * verify that an email was sent from the SNS
* * rm testfile.txt
*/
/// !cdk-integ pragma:ignore-assets
import * as iot from '@aws-cdk/aws-iot';
import * as sns from '@aws-cdk/aws-sns';
import * as cdk from '@aws-cdk/core';
import * as actions from '../../lib';

class TestStack extends cdk.Stack {
constructor(scope: cdk.App, id: string, props?: cdk.StackProps) {
super(scope, id, props);

const topicRule = new iot.TopicRule(this, 'TopicRule', {
sql: iot.IotSql.fromStringAsVer20160323(
"SELECT topic(2) as device_id, year, month, day FROM 'device/+/data'",
),
});

const snsTopic = new sns.Topic(this, 'MyTopic');
topicRule.addAction(new actions.SnsTopicAction(snsTopic));
}
}

const app = new cdk.App();
new TestStack(app, 'sns-topic-action-test-stack');
app.synth();
104 changes: 104 additions & 0 deletions packages/@aws-cdk/aws-iot-actions/test/sns/sns-topic-action.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
import { Match, Template } from '@aws-cdk/assertions';
import * as iam from '@aws-cdk/aws-iam';
import * as iot from '@aws-cdk/aws-iot';
import * as sns from '@aws-cdk/aws-sns';
import * as cdk from '@aws-cdk/core';
import * as actions from '../../lib';

const SNS_TOPIC_ARN = 'arn:aws:sns::123456789012:test-topic';
const RULE_ROLE_ID = 'MyTopicRuleTopicRuleActionRoleCE2D05DA';
Copy link
Contributor

Choose a reason for hiding this comment

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

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


let stack: cdk.Stack;
let topicRule: iot.TopicRule;
let snsTopic: sns.ITopic;

beforeEach(() => {
stack = new cdk.Stack();
topicRule = new iot.TopicRule(stack, 'MyTopicRule', {
sql: iot.IotSql.fromStringAsVer20160323("SELECT topic(2) as device_id FROM 'device/+/data'"),
});
snsTopic = sns.Topic.fromTopicArn(stack, 'MySnsTopic', SNS_TOPIC_ARN);
});

test('Default SNS topic action', () => {
// WHEN
topicRule.addAction(new actions.SnsTopicAction(snsTopic));

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::IoT::TopicRule', {
TopicRulePayload: {
Actions: [{
Sns: {
RoleArn: { 'Fn::GetAtt': [RULE_ROLE_ID, 'Arn'] },
TargetArn: SNS_TOPIC_ARN,
},
}],
},
});

Template.fromStack(stack).hasResourceProperties('AWS::IAM::Role', {
AssumeRolePolicyDocument: {
Statement: [{
Action: 'sts:AssumeRole',
Effect: 'Allow',
Principal: { Service: 'iot.amazonaws.com' },
}],
},
});

Template.fromStack(stack).hasResourceProperties('AWS::IAM::Policy', {
PolicyDocument: {
Statement: [{
Action: 'sns:Publish',
Effect: 'Allow',
Resource: SNS_TOPIC_ARN,
}],
},
Roles: [{ Ref: RULE_ROLE_ID }],
});
});

test('Can set messageFormat', () => {
// WHEN
topicRule.addAction(new actions.SnsTopicAction(snsTopic, {
messageFormat: actions.SnsActionMessageFormat.JSON,
}));

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::IoT::TopicRule', {
TopicRulePayload: {
Actions: [
Match.objectLike({ Sns: { MessageFormat: 'JSON' } }),
],
},
});
});

test('Can set role', () => {
// GIVEN
const roleArn = 'arn:aws:iam::123456789012:role/testrole';
const role = iam.Role.fromRoleArn(stack, 'MyRole', roleArn);

// WHEN
topicRule.addAction(new actions.SnsTopicAction(snsTopic, {
role,
}));

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::IoT::TopicRule', {
TopicRulePayload: {
Actions: [
Match.objectLike({ Sns: { RoleArn: roleArn } }),
],
},
});
});

test('Action with FIFO topic throws error', () => {
// GIVEN
const snsFifoTopic = sns.Topic.fromTopicArn(stack, 'MyFifoTopic', `${SNS_TOPIC_ARN}.fifo`);

expect(() => {
topicRule.addAction(new actions.SnsTopicAction(snsFifoTopic));
}).toThrowError('An SNS topic IoT Rule action cannot be configured with a FIFO topic, only standard topics are allowed.');
});