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): add Action to put objects in S3 Buckets #17307

Merged
merged 10 commits into from
Nov 10, 2021

Conversation

yamatatsu
Copy link
Contributor

I'm trying to implement aws-iot L2 Constructs.

This PR is one of steps after following PR:


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

1. add the action
2. add tests
3. describe to README
@gitpod-io
Copy link

gitpod-io bot commented Nov 3, 2021

In CodeBuild CI, there was following error:
```
@aws-cdk/aws-iot-actions-alpha: - [yarn/nohoist-bundled-dependencies] Repository-level 'workspaces.nohoist' directive is missing: @aws-cdk/aws-iot-actions-alpha/case, @aws-cdk/aws-iot-actions-alpha/case/** (fixable)
@aws-cdk/aws-iot-actions-alpha: Error: Some package.json files had errors
@aws-cdk/aws-iot-actions-alpha:     at main (/codebuild/output/src514100616/src/github.com/aws/aws-cdk/tools/@aws-cdk/pkglint/bin/pkglint.js:30:15)
@aws-cdk/aws-iot-actions-alpha:     at Object.<anonymous> (/codebuild/output/src514100616/src/github.com/aws/aws-cdk/tools/@aws-cdk/pkglint/bin/pkglint.js:33:1)
@aws-cdk/aws-iot-actions-alpha:     at Module._compile (internal/modules/cjs/loader.js:999:30)
@aws-cdk/aws-iot-actions-alpha:     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
@aws-cdk/aws-iot-actions-alpha:     at Module.load (internal/modules/cjs/loader.js:863:32)
@aws-cdk/aws-iot-actions-alpha:     at Function.Module._load (internal/modules/cjs/loader.js:708:14)
@aws-cdk/aws-iot-actions-alpha:     at Module.require (internal/modules/cjs/loader.js:887:19)
@aws-cdk/aws-iot-actions-alpha:     at require (internal/modules/cjs/helpers.js:74:18)
@aws-cdk/aws-iot-actions-alpha:     at Object.<anonymous> (/codebuild/output/src514100616/src/github.com/aws/aws-cdk/tools/@aws-cdk/pkglint/bin/pkglint:2:1)
@aws-cdk/aws-iot-actions-alpha:     at Module._compile (internal/modules/cjs/loader.js:999:30)
```
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks great @yamatatsu! A few minor comments.


new iot.TopicRule(this, 'TopicRule', {
sql: iot.IotSql.fromStringAsVer20160323("SELECT topic(2) as device_id FROM 'device/+/data'"),
actions: [new actions.S3Action(bucket)],
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you illustrate the optional properties of S3Action in this example? Especially how to use the substitution templates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh. It is important for S3Action! I will illustrate it!

*
* @default a new role will be created
*/
readonly role?: iam.IRole;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will every Action have a role property? Because if the answer is "yes", we should introduce a common interface that all Action prop interfaces extend. Like the CommonActionProps in @aws-cdk/aws-codepipeline.

Copy link
Contributor Author

@yamatatsu yamatatsu Nov 4, 2021

Choose a reason for hiding this comment

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

@skinny85
Not every Action is need a role, but almost. 2 actions in all actions will not have a role property.

  • HTTP Action
  • Lambda Action

For example, LambdaFunctionAction constructor (most simple one) is as following:

constructor(private readonly func: lambda.IFunction) {}

So I thought there are no common property for all actions. But the way to align the arguments of the action's constructor is what I was also struggling with.

What do you think? I'm open to any and all refactoring!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so if all of actions besides two will need role, I think introducing a separate common interface makes sense. And make S3ActionProps (soon to be changed to S3PutObjectActionProps I hope 😉) extend this new interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK! I will! 🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skinny85
If we create CommonActionProps and CommonAwsActionProps, should these constructors be unified in the following form?

interface CommonActionProps {
}

intercace CommonAwsActionProps extends CommonActionProps {
  role?: iam.IRole;
}

class Action {
  constructor(props: CommonActionProps) {}
}

Now, three Actions LambdaFunctionAction, CloudWatchLogsAction and S3PutObjectActionProps have following constructor, and first argument of them constroctor is not Props:

LambdaFunctionAction

constructor(func: lambda.IFunction) {}

CloudWatchLogsAction

constructor(logGroup: logs.ILogGroup, props: CloudWatchLogsActionProps) {}

S3PutObjectActionProps

constructor(bucket: s3.IBucket, props: S3PutObjectActionProps) {}

So if first question is "yes", I will do this refactoring in another PR!

Copy link
Contributor

Choose a reason for hiding this comment

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

No. Let's leave the constructors as they are now.

BTW, there's no point in having both CommonActionProps and CommonAwsActionProps for IoT Actions - you only need one common superinterface (it's a different story with CodePipeline Actions, but I don't want to get into the details of why that is here).

packages/@aws-cdk/aws-iot-actions/lib/s3-action.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iot-actions/lib/s3-action.ts Outdated Show resolved Hide resolved
* Supports substitution templates.
* @see https://docs.aws.amazon.com/iot/latest/developerguide/iot-substitution-templates.html
*
* @default '${topic()}/${timestamp()}'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have static, string-typed constants for these substitution templates? So that it's clear which ones can be used by which Actions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we cannot define all static, string-typed constants. Because users can use MQTT payload property defined by themselves in template.

For example, there are following MQTT payload:

{
  "year": "2021",
  "month": "Nov",
  "day": "04",
  "data": 123.4
}

The user can define the bucket key as ${year}/${month}/${day}/${timestamp()}.

Are we on the same page?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to have all of them - but you have a few that are always available, right? Like ${timestamp()} and ${topic()}. Maybe it's worth it to include those that are always available, and just document the other ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I also want to make it as user-friendly as possible. So I'd like to be on same page!

  1. there are about 70 functions, and they take arguments
  • So we need to use functions, instead of static constants.
  1. there are also Operators.
  • So it is better that our functions return without ${} as topic().
  1. in many cases, they are used embedded in strings without any regularity. For example, cloudwatch metric name could be used substitution templates.

And I thought 3 plans.

PLAN1, implement a set of functions

In light of the above, it might be better for us to implement a set of functions and for users to use them in template literals, as following:

key: `\${${iot.Template.topic(2)}}/\${${iot.Template.timestamp()}}`

PLAN2, just validation

Or, is it better to implement validation as JsonPath of @aws-cdk/aws-stepfunctions
https://github.com/aws/aws-cdk/tree/master/packages/@aws-cdk/aws-stepfunctions/lib/fields.ts#L216-L226

For substitution templates, I think we will be using a lot of regular expressions.

PLAN3, implement Expression and Template

Or, we implement Expression and Template?

I mean Expression as following:

iot.Expression.substring(iot.Expression.topic(2), 1)
// return type of Expression

I mean Template as following:

iot.Template.format(
  '{}/{}',
  iot.Expression.substring(iot.Expression.topic(2), 5, 10),
  iot.Expression.timestamp(),
)
// return type of Template

And, properties that can be used substitution templates will have the type Template.

{
  key: iot.Template,
}

The above may help to naturally communicate to users that substitution templates are available.

Copy link
Contributor

Choose a reason for hiding this comment

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

Before I answer, I have one more clarifying question.

Are any of the substitutions Action-dependent? So, are there any substitutions that can only be used with certain Action(s)?

In any case, I see now this is a much more complicated topic than I initially thought, and should be tackled in a separate PR. Let's continue the discussion on how to best model them - that's fine, but I don't want to block this PR on the discussion. Let's proceed with this PR without anything around substitutions, like it is now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are any of the substitutions Action-dependent? So, are there any substitutions that can only be used with certain Action(s)?

As far as I can tell from the documentation, no. And from the documentation, they all seem to be common functions and not Action-dependent. 🙂

Let's proceed with this PR without anything around substitutions, like it is now.

Sounds good!

Copy link
Contributor

Choose a reason for hiding this comment

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

Are any of the substitutions Action-dependent? So, are there any substitutions that can only be used with certain Action(s)?

As far as I can tell from the documentation, no. And from the documentation, they all seem to be common functions and not Action-dependent. 🙂

Great! Given that, I would go with Plan 1 from #17307 (comment).

as following:
error JSII9002: Unable to resolve type "@aws-cdk/aws-iot-actions.CommonActionProps". It may be @internal or not exported from the module's entry point (as configured in "package.json" as "main").
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks great @yamatatsu! One minor naming change.

Comment on lines 3 to 7
/**
* Common properties shared by Actions it access to AWS service.
*
* @internal
*/
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 it's fine to make this public.

),
actions: [
new actions.S3PutObjectAction(bucket, {
key: '${year}/${month}/${day}/${topic(2)}',
Copy link
Contributor

Choose a reason for hiding this comment

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

Show cannedAcl too?

Copy link
Contributor

Choose a reason for hiding this comment

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

So, where is it? 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I see below. In the future, I would merge these examples, because otherwise the ReadMe becomes way too long.

*
* @default None
*/
readonly cannedAcl?: s3.BucketAccessControl;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the name cannedAcl. Let's call this accessControl, like we do in CodePipeline's S3DeployAction.

Comment on lines 65 to 70
private putEventStatement(bucket: s3.IBucket) {
return new iam.PolicyStatement({
actions: ['s3:PutObject'],
resources: [bucket.arnForObjects('*')],
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's inline this method, it doesn't add much value.

});
topicRule.addAction(
new actions.S3PutObjectAction(bucket, {
key: '${year}/${month}/${day}/${topic(2)}',
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use cannedAclaccessControl here too.

{
S3: {
BucketName: 'test-bucket',
Key: 'test-key',
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's only assert for the presence of this Key (you might have to use the objectLike and arrayLike helpers to achieve this).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Beautiful!🤩

mergify bot pushed a commit that referenced this pull request Nov 9, 2021
This PR refactor the test that I committed earlier based on the above comment.

- #17307 (comment)

----

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

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks great @yamatatsu!

),
actions: [
new actions.S3PutObjectAction(bucket, {
key: '${year}/${month}/${day}/${topic(2)}',
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I see below. In the future, I would merge these examples, because otherwise the ReadMe becomes way too long.

@skinny85 skinny85 changed the title feat(iot-actions): Add the action to put s3 bucket objects feat(iot-actions): add Action to put objects in S3 Buckets Nov 10, 2021
@skinny85 skinny85 changed the title feat(iot-actions): add Action to put objects in S3 Buckets feat(iot): add Action to put objects in S3 Buckets Nov 10, 2021
@github-actions github-actions bot added the @aws-cdk/aws-iot Related to AWS IoT label Nov 10, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 10, 2021

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

mergify bot pushed a commit that referenced this pull request Nov 10, 2021
This PR refactor the test that I committed earlier, and is based on the following comment.

- #17307 (comment)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit 49b87db into aws:master Nov 10, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 10, 2021

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

@yamatatsu yamatatsu deleted the aws-iot-actions-s3 branch December 16, 2021 09:31
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
This PR refactor the test that I committed earlier based on the above comment.

- aws#17307 (comment)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
This PR refactor the test that I committed earlier, and is based on the following comment.

- aws#17307 (comment)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
I'm trying to implement aws-iot L2 Constructs.

This PR is one of steps after following PR: 
- aws#16681 (comment)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants