feat(stepfunctions): distributed map state#24331
Conversation
|
I decided to create this as draft because it's my first PR here and my first time working with Typescript (I'm a Python guy). I appreciate any feedback you can provide and please let me know if you think I can mark this ready for official review. |
aws-cdk-automation
left a comment
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
|
Also, thanks to @ayush987goyal and their PR #23306 for inspiration and guidance. I borrowed some of this work from their PR. |
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
|
I just decided to promote this to "ready for review". I think I've covered everything in the contributing guide and design guide. I look forward to your feedback. |
|
@kaizencc tagging you here because I see you were involved with the PR flow chart in the contributing guide. According to the flow chart it seems that this PR should be labelled |
comcalvi
left a comment
There was a problem hiding this comment.
Thanks for the contribution, looks good overall
|
|
||
| In this mode, the Map state supports up to 40 concurrent iterations. | ||
|
|
||
| A Map state set to Inline is known as an Inline Map state. Use the Map state in Inline mode if your workflow's execution history won't exceed 25,000 entries, or if you don't require more than 40 concurrent iterations. |
There was a problem hiding this comment.
| A Map state set to Inline is known as an Inline Map state. Use the Map state in Inline mode if your workflow's execution history won't exceed 25,000 entries, or if you don't require more than 40 concurrent iterations. | |
| Use the Map state in Inline mode if your workflow's execution history won't exceed 25,000 entries, or if you don't require more than 40 concurrent iterations. |
There was a problem hiding this comment.
Happy to clean this up if you feel strongly about it, but this was also copy/pasted directly from the docs. https://docs.aws.amazon.com/step-functions/latest/dg/concepts-asl-use-map-state-inline.html
| // Leaving this commented for review, as I want to make sure this truly isn't necessary. | ||
| // My understanding is that the `Ref` of the role should be enough to create an implicit dependency. | ||
| // However, this was adding both a dependency on the role and the policy, but the policy | ||
| // depends on the state machine thus creating a circular dependency. By removing | ||
| // this statement, the circular dependency is no longer an issue. | ||
| // If removing this is approved in the PR process, I will delete it before merge. | ||
| resource.node.addDependency(this.role); |
There was a problem hiding this comment.
This currently creates a circular dependency? Does this mean that every state machine currently fails to deploy with an error?
There was a problem hiding this comment.
I was able to clean this up a little bit and I removed this comment. The issue here is that Distributed Map states require that the state machine role have execute permissions on the state machine itself. So this resulted in the policy depending on the state machine, the state machine depending on the state machine role and the state machine role depending on the policy. Thus, a circular dependency. I was able to get around this by using some wildcards instead of the specific state machine arn in the policy document. This is only an issue for the Distributed Map state because I had to add the execution permissions to the policy for the state machine in that case.
packages/@aws-cdk/aws-stepfunctions/test/distributed-map.test.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions/lib/states/distributed-map.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions/lib/states/distributed-map.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions/lib/states/distributed-map.ts
Outdated
Show resolved
Hide resolved
Pull request has been modified.
|
Quick question from PR review comments. There were many suggestions regarding fixing whitespace in the code. I setup my dev environment in VSCode as described in the contributing guide. I guess I expected the tooling to format the code according to the desired style guide. Does the CDK repo not have a code formatter tool? Did I miss that? I did notice that code would change on save in my IDE to add missing punctuation and adjust whitespace, but apparently it didn't get all of it for some reason or it wasn't in line with the desired coding style. |
aws-cdk-automation
left a comment
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
Running |
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
I started working my way through this thinking that it was just adding functionality to an existing L2, but upon getting part of the way though, I'm seeing that distributed-map is a new L2. Given that and the size of this PR, I think this needs to go through the RFC process to ensure we're getting the design correct.
Pull request has been modified.
|
I am looking forward to using this. Thanks for your effort. |
| /** | ||
| * Binds this StateGraph to the StateMachine it defines and updates state machine permissions | ||
| */ | ||
| public bind(stateMachine: StateMachine) { |
There was a problem hiding this comment.
I was able to figure out how to use a separate inline policy to lock this down to state machine arn. The existing addRoleToPolicy or grantStartExecution methods use the default policy for the role which results in a circular reference, but a separate inline policy does not have the same circular reference issue.
| resources: [stateMachine.stack.formatArn({ | ||
| service: 'states', | ||
| resource: 'stateMachine', | ||
| resourceName: '*', |
There was a problem hiding this comment.
Not loving the wildcard here.
According to the docs we should be able to lock this down.
https://docs.aws.amazon.com/step-functions/latest/dg/concepts-asl-use-map-state-distributed.html#distributed-map-iam-policies
However I can see us running into a circular dependency here. Did you explore this?
We potentially could use a ManagedPolicy to get around the circular dependency. 🤔
There was a problem hiding this comment.
Yes. I originally tried using addRoleToPolicy with the state machine name and then I tried using the grantStartExecution method, but I did indeed run into a circular dependency. I wasn't completely sure how to proceed.
I noticed that the state machine adds an explicit dependency on the role and doing so also adds an explicit dependency on the role's policy to the state machine. We shouldn't need an explicit dependency on the policy since in the resulting CloudFormation template both the state machine and the policy depend on the role.
I tried removing the explicit stateMachine.addDependency call where the role gets added as a dependency to the state machine. This approach worked, but had huge changes across integration tests throughout the CDK library. I decided it was best not to update all those integration tests for this one use case.
So, in short, IMO the CDK is adding explicit dependencies where they are not needed and fixing that would fix the circular dependency issue and allow us to lock this down. But, changing that behavior would be a significant change across the repository, not just affecting the stepfunctions library.
…les values and json paths
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Any news about this pr? |
|
This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
|
Hey! Will it ever be finished? Or is there another implementation of DistributedMap in CDK somewhere else? |
|
Any progress on pushing this PR / feature through your process? I'd really like to see a feature to address DistributedMap in the CDK as embedding CF JSON in the code is less than ideal. |
|
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
Adds support for Step Functions Map state in Distributed mode. Currently, in order to create a Distributed Map in CDK, CDK users have to define a Custom State containing their Amazon States Language definition. This solution consists of the creation of a new L2 construct, `DistributedMap`. This design decision was made due to the fact that some fields are exclusive to Distributed Maps, such as `ItemReader`. Adding support for it through the existing `Map` L2 construct would lead to some fields being conditionally available. Some design decisions that were made: - I created an abstract class `MapBase` that encapsulates all fields currently supported by both `inline` and `distributed` maps. This includes all currently supported fields in the CDK except for `iterator` and `parameters` (deprecated fields). Those are now part of the Map subclass which extends `MapBase`. All new Distributed Maps fields are part of the new `DistributedMap` construct (also a subclass of `MapBase`) - Permissions specific to Distributed Maps are added as part of this new construct Thanks to @beck3905 and their PR #24331 for inspiration. A lot of the ideas here are re-used from the PR cited. Closes #23216 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Adds support for Step Functions Map state in Distributed mode. Currently, in order to create a Distributed Map in CDK, CDK users have to define a Custom State containing their Amazon States Language definition. This solution consists of the creation of a new L2 construct, `DistributedMap`. This design decision was made due to the fact that some fields are exclusive to Distributed Maps, such as `ItemReader`. Adding support for it through the existing `Map` L2 construct would lead to some fields being conditionally available. Some design decisions that were made: - I created an abstract class `MapBase` that encapsulates all fields currently supported by both `inline` and `distributed` maps. This includes all currently supported fields in the CDK except for `iterator` and `parameters` (deprecated fields). Those are now part of the Map subclass which extends `MapBase`. All new Distributed Maps fields are part of the new `DistributedMap` construct (also a subclass of `MapBase`) - Permissions specific to Distributed Maps are added as part of this new construct Thanks to @beck3905 and their PR #24331 for inspiration. A lot of the ideas here are re-used from the PR cited. Closes #23216 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Adds support for Step Functions Map state in Distributed mode. I decided to implement this as a new state type rather than as properties on the existing Map state. I thought that would make for a simpler user experience rather than having a bunch of complicated combinations of configuration properties.
Closes #23265.
Closes #23216.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license