feat(stepfunctions): support cross-account task invocations #23012

humanzz · 2022-11-21T12:12:52Z

support configuring a role to be assumed for task invocations https://docs.aws.amazon.com/step-functions/latest/dg/concepts-access-cross-acct-resources.html

closes #22994

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-11-21T12:12:57Z

humanzz · 2022-11-21T12:14:23Z

This is a first stab at such support.
I'm seeking feedback, and also guidance on how to best integ test this - also where (in this package or in stepfunctions-tasks

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.

humanzz · 2022-11-21T16:02:08Z

packages/@aws-cdk/aws-stepfunctions/lib/states/task-base.ts

+   * The ARN of the IAM role to be assumed.
+   * Either a fixed value or a JSONPath expression can be used.
+   */
+  readonly roleArn: string;


This can be much nicer if there was a concrete type denoting json path e.g.

role: iam.IRole | JsonPath;

We wouldn't want to add this depth to these props just to get a roleArn. Typically, we pass IRole, but I see what you're trying to do here so I suggest you take the example of Schedule and do something like what they did there with an enum like class.

I am not sure I 100% follow

When you say "this depth", do you mean the iam.IRole | JsonPath bit, or do you mean the hierarchy of of the prop Credentials.roleArn? If hierarchy, i.e. whether roleArn should be top level or not, the only reason I didn't do that, is I thought maybe in the future their can be more attributes in the Credentials object e.g. session name, etc.

The role - rather than Credentials - is what really might benefit from different modelling similar to Schedule.

If we go for modelling a Role class here it would be something along the lines of

class Role { public static jsonExpression(expression: string): Role public static role(role: iam.Role): Role }

The other thing is, I don't think there's a strong type for json expressions (to the best of my knowledge)

I introduced a new change to introduce TaskRole (inspired by Schedule as you suggested)

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

TheRealAmazonKendra

There's a bit that I would comment on in here but the comment inline will likely change a lot of that so I'm just going to leave it there for the moment. If you want to make a Credentials class, modeling it similar to the class I linked inline would be the best approach.

TheRealAmazonKendra · 2022-11-22T02:54:56Z

packages/@aws-cdk/aws-stepfunctions/lib/states/task-base.ts

+   * The ARN of the IAM role to be assumed.
+   * Either a fixed value or a JSONPath expression can be used.
+   */
+  readonly roleArn: string;


We wouldn't want to add this depth to these props just to get a roleArn. Typically, we pass IRole, but I see what you're trying to do here so I suggest you take the example of Schedule and do something like what they did there with an enum like class.

Pull request has been modified.

corymhall

Just 1 minor comment, otherwise this looks good!

corymhall · 2022-11-30T14:16:55Z

packages/@aws-cdk/aws-stepfunctions/test/state-machine.test.ts

@@ -278,6 +280,75 @@ describe('State Machine', () => {
    });
  });

+  test('Instantiate a State Machine with a task assuming a literal roleArn', () => {


Can you also add a test using fromRoleName using a stack with a different
account from the state machine? I want to make sure it builds the arn/reference
correctly.

@humanzz I think this is still unresolved.

I think I've misunderstood your intention - twice now :)
The last change breaks this test case into 2; one for cross-account role assumption, and one for same-account role assumption.

For integ tests, I wasn't sure how the cross-account behaviour can be tested i.e. can we create stacks in different test accounts, so I used fromRoleArn there.

…-cdk into sfn-task-credentials

Pull request has been modified.

humanzz · 2022-12-04T13:22:10Z

Just 1 minor comment, otherwise this looks good!

Updated the integ tests.

corymhall · 2022-12-06T13:32:41Z

packages/@aws-cdk/aws-stepfunctions/test/state-machine.test.ts

@@ -278,6 +280,75 @@ describe('State Machine', () => {
    });
  });

+  test('Instantiate a State Machine with a task assuming a literal roleArn', () => {


@humanzz I think this is still unresolved.

Pull request has been modified.

packages/@aws-cdk/aws-stepfunctions/test/state-machine.test.ts

mergify · 2022-12-06T15:24:34Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-12-06T16:03:01Z

AWS CodeBuild CI Report

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

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

mergify · 2022-12-06T16:03:19Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

support configuring a role to be assumed for task invocations https://docs.aws.amazon.com/step-functions/latest/dg/concepts-access-cross-acct-resources.html closes aws#22994 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

feat(stepfunctions): support task credentials

c622b2c

github-actions bot added feature-request A feature should be added or improved. p2 labels Nov 21, 2022

aws-cdk-automation requested a review from a team November 21, 2022 12:13

github-actions bot added the valued-contributor [Pilot] contributed between 6-12 PRs to the CDK label Nov 21, 2022

aws-cdk-automation previously requested changes Nov 21, 2022

View reviewed changes

humanzz added 2 commits November 21, 2022 12:18

feat(stepfunctions): support task credentials

5e2447f

Update README

3598647

humanzz mentioned this pull request Nov 21, 2022

(stepfunctions-tasks): support newly released cross-account capabilities #22994

Closed

2 tasks

humanzz commented Nov 21, 2022

View reviewed changes

add credentials integ test

e0a2a14

TheRealAmazonKendra previously requested changes Nov 22, 2022

View reviewed changes

introduce TaskRole

86b9a5b

humanzz added 2 commits November 22, 2022 15:46

update README

a23b4b6

rename TaskRole methods

44553f9

humanzz changed the title ~~feat(stepfunctions): support task credentials~~ feat(stepfunctions): support cross-account task invocations Nov 22, 2022

Merge branch 'main' into sfn-task-credentials

865ebc7

humanzz requested a review from TheRealAmazonKendra November 25, 2022 14:10

Merge branch 'main' into sfn-task-credentials

ef5011f

corymhall previously requested changes Nov 30, 2022

View reviewed changes

humanzz added 3 commits December 3, 2022 22:20

add cross-account iam role integ test

afa7ac0

Merge branch 'sfn-task-credentials' of https://github.com/humanzz/aws…

ba12e06

…-cdk into sfn-task-credentials

Merge branch 'main' into sfn-task-credentials

836bc65

humanzz removed the request for review from TheRealAmazonKendra December 4, 2022 13:24

humanzz requested review from corymhall and aws-cdk-automation and removed request for corymhall December 4, 2022 13:24

corymhall previously requested changes Dec 6, 2022

View reviewed changes

corymhall self-assigned this Dec 6, 2022

update unit tests with iam.Role.fromName test case

a57b830

update unit tests

93303a3

corymhall suggested changes Dec 6, 2022

View reviewed changes

packages/@aws-cdk/aws-stepfunctions/test/state-machine.test.ts Show resolved Hide resolved

corymhall approved these changes Dec 6, 2022

View reviewed changes

Merge branch 'main' into sfn-task-credentials

96c81e5

mergify bot merged commit df163ec into aws:main Dec 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stepfunctions): support cross-account task invocations #23012

feat(stepfunctions): support cross-account task invocations #23012

humanzz commented Nov 21, 2022 •

edited

Loading

gitpod-io bot commented Nov 21, 2022

humanzz commented Nov 21, 2022

aws-cdk-automation left a comment

humanzz Nov 21, 2022

TheRealAmazonKendra Nov 22, 2022

humanzz Nov 22, 2022 •

edited

Loading

humanzz Nov 22, 2022 •

edited

Loading

TheRealAmazonKendra left a comment

TheRealAmazonKendra Nov 22, 2022

corymhall left a comment

corymhall Nov 30, 2022

corymhall Dec 6, 2022

humanzz Dec 6, 2022 •

edited

Loading

humanzz commented Dec 4, 2022

corymhall Dec 6, 2022

mergify bot commented Dec 6, 2022

aws-cdk-automation commented Dec 6, 2022

mergify bot commented Dec 6, 2022

feat(stepfunctions): support cross-account task invocations #23012

feat(stepfunctions): support cross-account task invocations #23012

Conversation

humanzz commented Nov 21, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Nov 21, 2022

humanzz commented Nov 21, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

humanzz Nov 21, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra Nov 22, 2022

Choose a reason for hiding this comment

humanzz Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

humanzz Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra Nov 22, 2022

Choose a reason for hiding this comment

corymhall left a comment

Choose a reason for hiding this comment

corymhall Nov 30, 2022

Choose a reason for hiding this comment

corymhall Dec 6, 2022

Choose a reason for hiding this comment

humanzz Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

humanzz commented Dec 4, 2022

corymhall Dec 6, 2022

Choose a reason for hiding this comment

mergify bot commented Dec 6, 2022

aws-cdk-automation commented Dec 6, 2022

AWS CodeBuild CI Report

mergify bot commented Dec 6, 2022

humanzz commented Nov 21, 2022 •

edited

Loading

humanzz Nov 22, 2022 •

edited

Loading

humanzz Nov 22, 2022 •

edited

Loading

humanzz Dec 6, 2022 •

edited

Loading