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
fix(stepfunctions): distributed maps under branches do not have necessary permissions #29913
base: main
Are you sure you want to change the base?
Conversation
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.
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.
ahh re pull request validator failing for no integration test update, i will look at that later this weekend of sometime next week. |
526527e
to
62f08f4
Compare
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
added a couple integration tests and fixed the linting problems |
1a2ed6f
to
38c98b2
Compare
i was wondering about my approach... because branches aren't exposed, i moved the policy stuff into State. however, State cannot depend on DistributedMap because of a cyclic dependency. To deal with this, I just passed isDistributedMap into the method call. This is based on the assumption that branches is hidden for a reason. This works but could be cleaner. I was wondering about thoughts on a few alternative solutions:
from my point of view, anything is fine, i just want to remove the manual perms from my cdk since it's weird that cdk sometimes but not always adds them (depending on how the graph is shaped) heh anyway will update if needed! |
👋 just wanted to touch base on this... I'm happy to make changes if yall don't like the approach! thanks :) |
just saw there is also an issue reported for this - linked it in desc... just following up again though, is someone able to look at this? Thanks :) |
distributed maps under branch states (i.e., Parallel) do not apply the necessary permissions to run the state. this moves the bind functionality into state and calls it on both state and all child states. rather than relying on the single purpose that it is now (add distributed map perms) and fast returning all the way out, this instead just checks if the policy it is trying to add is in place before proceeding and uses that condition to return immediately.
38c98b2
to
a820573
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue
Closes #29266
Reason for this change
distributed maps under branch states (i.e., Parallel) do not apply the necessary permissions to run the state.
Description of changes
this moves the bind functionality into state and calls it on both state and all child (branch) states. Previously it did not run on branch states and as such did not work for example a distributed map under a parallel state without adding the permissions out of band.
An alternative would be to have a bind method that the user calls out of band on the distributed map (or in my case, in a wrapper class, but it's clunky)
Description of how you validated changes
Unit tests
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license