-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Changes from 18 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
3e76dfe
Add SnsTopicAction class
AdamVD 547a826
Update README
AdamVD 8ab37f6
Add test with defaults
AdamVD 43a5017
Add test for messageFormat config
AdamVD c7a7bb5
Add test for role config
AdamVD aa3827f
Test FIFO queue throws error
AdamVD c243276
Add integration test
AdamVD 54e6c99
Add stack verification steps to integ test file
AdamVD 458cdf5
Remove TODOs
AdamVD 08e1208
Add message format docstrings to pass lint
AdamVD 8ab7c7a
Fix README example
AdamVD 86a2fa2
Improve formatting and naming
AdamVD 69f8ea4
Improve code format and wording
AdamVD d916076
Use topic grantPublish on rule role
AdamVD 23efd68
Merge branch 'master' into feature/iot-rule-sns-action
AdamVD b423449
Merge branch 'master' into feature/iot-rule-sns-action
AdamVD 217da01
Expose fifo attribute from ITopic
AdamVD 2abb973
Use fifo attribute in input validation
AdamVD 061b9bd
Merge branch 'master' into feature/iot-rule-sns-action
mergify[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
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 { | ||
/** | ||
* 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.fifo) { | ||
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 { | ||
const role = this.role ?? singletonActionRole(rule); | ||
this.topic.grantPublish(role); | ||
|
||
return { | ||
configuration: { | ||
sns: { | ||
targetArn: this.topic.topicArn, | ||
roleArn: role.roleArn, | ||
messageFormat: this.messageFormat, | ||
}, | ||
}, | ||
}; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
71 changes: 71 additions & 0 deletions
71
packages/@aws-cdk/aws-iot-actions/test/sns/integ.sns-topic-action.expected.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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" | ||
} | ||
} | ||
} |
33 changes: 33 additions & 0 deletions
33
packages/@aws-cdk/aws-iot-actions/test/sns/integ.sns-topic-action.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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(); |
103 changes: 103 additions & 0 deletions
103
packages/@aws-cdk/aws-iot-actions/test/sns/sns-topic-action.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,103 @@ | ||
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'; | ||
|
||
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': ['MyTopicRuleTopicRuleActionRoleCE2D05DA', '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: 'MyTopicRuleTopicRuleActionRoleCE2D05DA' }], | ||
}); | ||
}); | ||
|
||
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('IoT Rule actions cannot be used with FIFO SNS Topics, please pass a non-FIFO Topic instead'); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 (unlikeIQueue.encryptionMasterKey
). The current SQS queue action has the same limitation.There was a problem hiding this comment.
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 ofITopic
, 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!
There was a problem hiding this comment.
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
:Compare this against
queue-base.ts
: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?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 😉.