fix(stepfunctions): distributed maps under branches do not have necessary permissions #29913

curquhart · 2024-04-20T17:33:03Z

Issue

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

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

aws-cdk-automation

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.

curquhart · 2024-04-20T17:37:42Z

ahh re pull request validator failing for no integration test update, i will look at that later this weekend of sometime next week.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

curquhart · 2024-04-21T15:14:46Z

added a couple integration tests and fixed the linting problems

curquhart · 2024-04-21T16:04:59Z

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:

could i expose branches in State? then this stuff could be entirely (other than that visibility change in State) be in StateGraph
add containsDistributedMap to State (as in, StateGraph would call state.containsDistributedState())... in the base class, this would return false, in DistributedMap this would return true
processorMode is already in State... it's protected but if we made it public we might be also use that to determine if distributed map (by swapping with a getter, throwing the real value in _processorMode, and then having the getter traverse any children)... i do worry about side effects on this one though since we'd be considering processorMode as distributed if any of the children are distributed

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!

curquhart · 2024-04-27T03:39:28Z

👋 just wanted to touch base on this... I'm happy to make changes if yall don't like the approach!

thanks :)

curquhart · 2024-05-05T02:26:53Z

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

curquhart · 2024-06-05T18:52:16Z

just following up... again i'm happy to make changes, would appreciate feedback! thanks :)

comcalvi

branches is protected because many of the stepfunctions features live on the base class but are defined and exposed in subclasses, as explained here: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-stepfunctions/lib/states/state.ts#L163, and since branches is only usable with Parallel states, I don't think it makes sense to expose it.

I don't think the processor mode approach is the right one either, for the the reason you outlined.

The design suggests we should include IsDistributedMap on the base class, as you describe in option 2. This is an odd placement, because the base class should not know anything about its Distributed subclass variant - but we've already broken this assumption by declaring all of these subclass-unique properties on the base class, so we may as well include isDistributedMap on the base class.

aws-cdk-automation · 2024-07-09T00:09:19Z

This PR has been in the CHANGES REQUESTED 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.

distributed maps under branch states (i.e., Parallel) do not apply the necessary permissions to run the state. instead of only iterating states in the parent graph, iterate all children as well. if any of them are distributed maps, add the necessary policy.

curquhart · 2024-07-09T18:00:32Z

sorry finally got back to this... I realized adding isDistributedMap was impractical because of DistributedMap. isDistributedMap being public which would make making it an instance method a breaking change to the public interface and dug a bit more through the innards

i realized that child graphs are contained within allContainedStates which means we can just recursively call bind on each graph in that collection, which in turn avoids making changes outside of the original StateGraph logic (so less code touched, woohoo).

anyway! updated to do that, let me know if this makes sense

Pull request has been modified.

aws-cdk-automation · 2024-07-09T18:31:51Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: a1f8263
Result: SUCCEEDED
Build Logs (available for 30 days)

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

TheRealAmazonKendra

Thank you for your contribution! I can see that a lot of hard work went into this and it looks right. I just have a couple requests inline for improving the testing a bit.

TheRealAmazonKendra · 2024-07-24T23:16:34Z

...es/@aws-cdk-testing/framework-integ/test/aws-stepfunctions/test/integ.map-distributed-iam.ts

+/**
+ * Stack verification steps:
+ *
+ * -- aws stepfunctions describe-state-machine --state-machine-arn <stack-output> has a status of `ACTIVE`


Could you please add an assertion that does this instead of making it a manual verification step?

TheRealAmazonKendra · 2024-07-24T23:17:38Z

...es/@aws-cdk-testing/framework-integ/test/aws-stepfunctions/test/integ.map-distributed-iam.ts

+  timeout: cdk.Duration.seconds(30),
+});
+
+new cdk.CfnOutput(stack, 'StateMachine1ARN', {


These outputs are great but we should do something with them. Perhaps add assertions that get the machine state of each of these?

TheRealAmazonKendra · 2024-07-24T23:20:00Z

packages/aws-cdk-lib/aws-stepfunctions/test/state-graph.test.ts

+    // is that our permissions adding doesn't create infinite recursion as we iterate through all nested graphs.
+    // a failure in this test case (for the thing it is meant to test) would present as a stack overflow ("maximum call
+    // stack size exceeded" error.)
+    template.hasResource('AWS::IAM::Policy', createPolicyProps(stateMachineLogicalId));


Can we check for the expected amount of policies based off the amount of child states?

curquhart · 2024-08-02T17:08:55Z

oh i just saw your comments! thanks for looking :D I'll try to address them this weekend - i looked at a couple integ tests and the (manual) way i did it was based on an existing test, i didn't realize there's a framework in other tests for validating via api, that'll be much nicer :)

aws-cdk-automation · 2024-08-15T00:09:49Z

This PR has been in the CHANGES REQUESTED 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.

aws-cdk-automation · 2024-08-22T00:10:03Z

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.

github-actions · 2024-08-22T00:10:22Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Apr 20, 2024

aws-cdk-automation requested a review from a team April 20, 2024 17:33

aws-cdk-automation previously requested changes Apr 20, 2024

View reviewed changes

curquhart force-pushed the fix_distributed_map_perms branch from 526527e to 62f08f4 Compare April 21, 2024 15:10

curquhart force-pushed the fix_distributed_map_perms branch 2 times, most recently from 1a2ed6f to 38c98b2 Compare April 21, 2024 15:35

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 21, 2024

This was referenced Apr 22, 2024

Weekly PR metrics report #29922

Closed

Weekly PR metrics report #29980

Closed

Weekly PR metrics report #29986

Closed

github-actions bot mentioned this pull request May 1, 2024

Monthly PR metrics report #30026

Closed

github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 and removed p2 labels May 5, 2024

aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels May 5, 2024

curquhart force-pushed the fix_distributed_map_perms branch from 38c98b2 to a820573 Compare May 8, 2024 19:04

comcalvi previously requested changes Jun 17, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 17, 2024

curquhart force-pushed the fix_distributed_map_perms branch from a820573 to a1f8263 Compare July 9, 2024 18:00

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jul 9, 2024

TheRealAmazonKendra requested changes Jul 24, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jul 24, 2024

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Aug 22, 2024

aws-cdk-automation closed this Aug 22, 2024

github-actions bot locked as resolved and limited conversation to collaborators Aug 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(stepfunctions): distributed maps under branches do not have necessary permissions #29913

fix(stepfunctions): distributed maps under branches do not have necessary permissions #29913

curquhart commented Apr 20, 2024 •

edited

Loading

aws-cdk-automation left a comment

curquhart commented Apr 20, 2024

curquhart commented Apr 21, 2024

curquhart commented Apr 21, 2024 •

edited

Loading

curquhart commented Apr 27, 2024

curquhart commented May 5, 2024

curquhart commented Jun 5, 2024

comcalvi left a comment •

edited

Loading

aws-cdk-automation commented Jul 9, 2024

curquhart commented Jul 9, 2024

aws-cdk-automation commented Jul 9, 2024

TheRealAmazonKendra left a comment

TheRealAmazonKendra Jul 24, 2024

TheRealAmazonKendra Jul 24, 2024

TheRealAmazonKendra Jul 24, 2024

curquhart commented Aug 2, 2024

aws-cdk-automation commented Aug 15, 2024

aws-cdk-automation commented Aug 22, 2024

github-actions bot commented Aug 22, 2024

fix(stepfunctions): distributed maps under branches do not have necessary permissions #29913

fix(stepfunctions): distributed maps under branches do not have necessary permissions #29913

Conversation

curquhart commented Apr 20, 2024 • edited Loading

Issue

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

curquhart commented Apr 20, 2024

curquhart commented Apr 21, 2024

curquhart commented Apr 21, 2024 • edited Loading

curquhart commented Apr 27, 2024

curquhart commented May 5, 2024

curquhart commented Jun 5, 2024

comcalvi left a comment • edited Loading

Choose a reason for hiding this comment

aws-cdk-automation commented Jul 9, 2024

curquhart commented Jul 9, 2024

aws-cdk-automation commented Jul 9, 2024

AWS CodeBuild CI Report

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra Jul 24, 2024

Choose a reason for hiding this comment

TheRealAmazonKendra Jul 24, 2024

Choose a reason for hiding this comment

TheRealAmazonKendra Jul 24, 2024

Choose a reason for hiding this comment

curquhart commented Aug 2, 2024

aws-cdk-automation commented Aug 15, 2024

aws-cdk-automation commented Aug 22, 2024

github-actions bot commented Aug 22, 2024

curquhart commented Apr 20, 2024 •

edited

Loading

curquhart commented Apr 21, 2024 •

edited

Loading

comcalvi left a comment •

edited

Loading