-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(stepfunctions): support cross-account task invocations #23012
Changes from 1 commit
c622b2c
5e2447f
3598647
e0a2a14
86b9a5b
a23b4b6
44553f9
865ebc7
ef5011f
afa7ac0
ba12e06
836bc65
a57b830
93303a3
96c81e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import * as iam from '@aws-cdk/aws-iam'; | ||
import * as constructs from 'constructs'; | ||
import * as sfn from '../../lib'; | ||
|
||
export interface FakeTaskProps extends sfn.TaskStateBaseProps { | ||
readonly metrics?: sfn.TaskMetricsConfig; | ||
} | ||
|
||
export class FakeTask extends sfn.TaskStateBase { | ||
protected readonly taskMetrics?: sfn.TaskMetricsConfig; | ||
protected readonly taskPolicies?: iam.PolicyStatement[]; | ||
|
||
constructor(scope: constructs.Construct, id: string, props: FakeTaskProps = {}) { | ||
super(scope, id, props); | ||
this.taskMetrics = props.metrics; | ||
} | ||
|
||
/** | ||
* @internal | ||
*/ | ||
protected _renderTask(): any { | ||
return { | ||
Resource: 'my-resource', | ||
Parameters: sfn.FieldUtils.renderObject({ | ||
MyParameter: 'myParameter', | ||
}), | ||
}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ import * as logs from '@aws-cdk/aws-logs'; | |
import * as s3 from '@aws-cdk/aws-s3'; | ||
import * as cdk from '@aws-cdk/core'; | ||
import * as stepfunctions from '../lib'; | ||
import { FakeTask } from './private/fake-task'; | ||
|
||
describe('State Machine', () => { | ||
test('Instantiate Default State Machine', () => { | ||
|
@@ -278,6 +279,74 @@ describe('State Machine', () => { | |
}); | ||
}); | ||
|
||
test('Instantiate a State Machine with a task assuming a literal roleArn', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also add a test using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @humanzz I think this is still unresolved. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I've misunderstood your intention - twice now :) 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 |
||
// GIVEN | ||
const stack = new cdk.Stack(); | ||
|
||
// WHEN | ||
new stepfunctions.StateMachine(stack, 'MyStateMachine', { | ||
definition: new FakeTask(stack, 'fakeTask', { credentials: { roleArn: 'arn:aws:iam::123456789012:role/example-role' } }), | ||
}); | ||
|
||
// THEN | ||
Template.fromStack(stack).hasResourceProperties('AWS::StepFunctions::StateMachine', { | ||
DefinitionString: '{"StartAt":"fakeTask","States":{"fakeTask":{"End":true,"Type":"Task","Credentials":{"RoleArn":"arn:aws:iam::123456789012:role/example-role"},"Resource":"my-resource","Parameters":{"MyParameter":"myParameter"}}}}', | ||
}); | ||
|
||
Template.fromStack(stack).hasResourceProperties('AWS::IAM::Policy', { | ||
PolicyDocument: { | ||
Statement: [ | ||
{ | ||
Effect: 'Allow', | ||
Action: 'sts:AssumeRole', | ||
Resource: 'arn:aws:iam::123456789012:role/example-role', | ||
}, | ||
], | ||
Version: '2012-10-17', | ||
}, | ||
PolicyName: 'MyStateMachineRoleDefaultPolicyE468EB18', | ||
Roles: [ | ||
{ | ||
Ref: 'MyStateMachineRoleD59FFEBC', | ||
}, | ||
], | ||
}); | ||
}); | ||
|
||
test('Instantiate a State Machine with a task assuming a JSONPath roleArn', () => { | ||
// GIVEN | ||
const stack = new cdk.Stack(); | ||
|
||
// WHEN | ||
new stepfunctions.StateMachine(stack, 'MyStateMachine', { | ||
definition: new FakeTask(stack, 'fakeTask', { credentials: { roleArn: stepfunctions.JsonPath.stringAt('$.RoleArn') } }), | ||
}); | ||
|
||
// THEN | ||
Template.fromStack(stack).hasResourceProperties('AWS::StepFunctions::StateMachine', { | ||
DefinitionString: '{"StartAt":"fakeTask","States":{"fakeTask":{"End":true,"Type":"Task","Credentials":{"RoleArn.$":"$.RoleArn"},"Resource":"my-resource","Parameters":{"MyParameter":"myParameter"}}}}', | ||
}); | ||
|
||
Template.fromStack(stack).hasResourceProperties('AWS::IAM::Policy', { | ||
PolicyDocument: { | ||
Statement: [ | ||
{ | ||
Effect: 'Allow', | ||
Action: 'sts:AssumeRole', | ||
Resource: '*', | ||
}, | ||
], | ||
Version: '2012-10-17', | ||
}, | ||
PolicyName: 'MyStateMachineRoleDefaultPolicyE468EB18', | ||
Roles: [ | ||
{ | ||
Ref: 'MyStateMachineRoleD59FFEBC', | ||
}, | ||
], | ||
}); | ||
}); | ||
|
||
describe('StateMachine.fromStateMachineArn()', () => { | ||
let stack: cdk.Stack; | ||
|
||
|
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.
This can be much nicer if there was a concrete type denoting json path e.g.
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.
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.
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.
I am not sure I 100% follow
iam.IRole | JsonPath
bit, or do you mean the hierarchy of of the propCredentials.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.Schedule
.If we go for modelling a
Role
class here it would be something along the lines ofThe other thing is, I don't think there's a strong type for json expressions (to the best of my knowledge)
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.
I introduced a new change to introduce
TaskRole
(inspired bySchedule
as you suggested)