feat(iot): add Action to republish MQTT messages to another MQTT topic by yamatatsu · Pull Request #18661 · aws/aws-cdk

yamatatsu · 2022-01-26T11:33:20Z

resolve #17701

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

gitpod-io · 2022-01-26T12:24:35Z

skinny85

Thanks for the contribution @yamatatsu, looks great as usual! A few questions, mainly about naming.

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

skinny85 · 2022-01-26T22:21:47Z

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

+/**
+ * The action to put the record from an MQTT message to republish another MQTT topic.
+ */
+export class IotRepublishAction implements iot.IAction {


I wonder, why not use the term "Mqtt" here for the name of this Action? Does it not fit well for some reason?

There is no reason. Once think again, it would be easier to understand if "MQTT" was included.

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

skinny85 · 2022-01-26T22:25:30Z

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

+import { singletonActionRole } from './private/role';
+
+/**
+ * MQTT Quality of Service


This comment doesn't explain what this is, it just expands the acronym in the name 😛. How about adding something like "controls the message delivery semantics used when publishing messages to the MQTT topic".

You're right, I didn't explain it well enough. I've added explaination that copied from the definition.

skinny85 · 2022-01-26T22:26:44Z

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

+    const role = this.role ?? singletonActionRole(rule);
+    role.addToPrincipalPolicy(new iam.PolicyStatement({
+      actions: ['iot:Publish'],
+      resources: ['*'],


Hmm... does this have to be "*"? Doesn't the fact that we have the topic here allow us to write a more constrained permission policy?

I think too, it is maybe less restrictive. But I have no good idea..

When the passed topic is literal, The topic's ARN arn:aws:iot:aws-region:AWS-account-ID:topic/Topic can be identified.
But when the passed topic includes any expressions (e.g. "${topic()}/republished", this case is rather common), the ARN cannot be identified.

Users can use more restrictive permission with that they provide property role that has DENY policy and notResources. Should we explain it in JSDoc? Or add the property to identify candidats topics like topicCandidates?: string[]?

OK. Let's leave it as-is for now (if you want to add a quick blurb in the ReadMe about this, feel free, but I won't require it).

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

skinny85 · 2022-01-26T22:30:07Z

...aws-cdk/aws-iot-actions/test/kinesis-firehose/integ.firehose-put-record-action.expected.json

-                "s3:PutObjectLegalHold",
-                "s3:PutObjectRetention",
-                "s3:PutObjectTagging",
-                "s3:PutObjectVersionTagging",


Sorry, what happened here?

My local build is old... I've fix it!

Pull request has been modified.

skinny85

Looks great @yamatatsu, just one tiny naming change, and we can merge this in.

skinny85 · 2022-01-27T21:41:38Z

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

+   *
+   * @default MqttQualityOfService.ZERO_OR_MORE_TIMES
+   */
+  readonly qos?: MqttQualityOfService;


Sorry, I missed this in the initial review.

Let's rename this to qualityOfService.

Oops... It's my missing of fix. Sorry.

skinny85 · 2022-01-27T21:42:32Z

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

+    const role = this.role ?? singletonActionRole(rule);
+    role.addToPrincipalPolicy(new iam.PolicyStatement({
+      actions: ['iot:Publish'],
+      resources: ['*'],


OK. Let's leave it as-is for now (if you want to add a quick blurb in the ReadMe about this, feel free, but I won't require it).

Pull request has been modified.

skinny85

Thanks @yamatatsu!

mergify · 2022-01-31T16:12:46Z

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-01-31T17:49:15Z

AWS CodeBuild CI Report

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

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

mergify · 2022-01-31T17:49:38Z

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

* origin/master: (74 commits) chore: remove reliance on `decdk` in build system (aws#18760) chore: add repository directory for all packages.json (aws#17203) docs(ecs): correct comment documentation for NetworkMode (aws#17841) feat(ecs): expose image name in container definition (aws#17793) feat(ecr): add server-side encryption configuration (aws#16966) chore(region-info): ap-southeast-3 (Jakarta) ROUTE_53_BUCKET_WEBSITE_ZONE_ID (aws#18110) chore: reassign njlynch's ownership areas (aws#18751) chore(ecs-service-extensions): migrate tests to assertions (aws#18649) chore(s3): Fixed documentation for `InventoryFormat.ORC` (aws#18717) feat(iot): add Action to republish MQTT messages to another MQTT topic (aws#18661) chore(rds): add support for PostgreSQL 14 (aws#18713) fix(core): correctly reference versionless secure parameters (aws#18730) fix(ec2): `UserData.addSignalOnExitCommand` does not work in combination with `userDataCausesReplacement` (aws#18726) fix(vpc): Vpc.fromLookup should throw if subnet group name tag is explicitly given and does not exist (aws#18714) docs(dynamodb): add note around table encryption (aws#18721) chore: override `markdown-it` version (aws#18723) docs(cfnspec): update CloudFormation documentation (aws#18741) chore(release): 1.142.0 chore(lambda-layer-awscli): contains a CLI version with a CVE (aws#18727) chore(lambda-python): remove Pillow dependency (aws#18722) ...

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

yamatatsu added 2 commits January 26, 2022 20:31

feat(iot): add Action to republish MQTT messages to another MQTT topic

0c2063e

Merge branch 'master' into iot-actions-republish

63dfc71

peterwoodworth added the @aws-cdk/aws-iot Related to AWS IoT label Jan 26, 2022

peterwoodworth assigned skinny85 Jan 26, 2022

skinny85 previously requested changes Jan 26, 2022

View reviewed changes

Merge branch 'master' into iot-actions-republish

8eed4d9

yamatatsu added 2 commits January 27, 2022 18:09

fix snapthot

4bf88c8

address comments

8c5aaa6

yamatatsu marked this pull request as ready for review January 27, 2022 12:12

skinny85 previously requested changes Jan 27, 2022

View reviewed changes

rename qos

4868f69

yamatatsu requested a review from skinny85 January 28, 2022 14:45

skinny85 approved these changes Jan 31, 2022

View reviewed changes

Merge branch 'master' into iot-actions-republish

0a8ad71

mergify bot merged commit 7ac1215 into aws:master Jan 31, 2022

yamatatsu deleted the iot-actions-republish branch April 6, 2022 15:17

Conversation

yamatatsu commented Jan 26, 2022

Uh oh!

gitpod-io bot commented Jan 26, 2022

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yamatatsu Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yamatatsu Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jan 31, 2022

Uh oh!

aws-cdk-automation commented Jan 31, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jan 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yamatatsu Jan 27, 2022 •

edited

Loading

yamatatsu Jan 27, 2022 •

edited

Loading