-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(stepfunction-tasks): allow ecsruntask to receive dynamically generated task definitions #35347
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
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.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
0551852
to
26d64cb
Compare
...ramework-integ/test/aws-stepfunctions-tasks/test/ecs/integ.fargate-run-task-taskdef-input.ts
Show resolved
Hide resolved
}); | ||
const stack = new cdk.Stack(app, 'aws-sfn-tasks-ecs-fargate-run-tasktask-def'); | ||
stack.node.setContext(EC2_RESTRICT_DEFAULT_SECURITY_GROUP, false); | ||
stack.node.setContext(STEPFUNCTIONS_TASKS_FIX_RUN_ECS_TASK_POLICY, false); |
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 flags below have a default/recommended value true
, we should test the setup with the default/recommended value :
Also I think this not needed as the default value would be false for this :
@aws-cdk/aws-lambda:createNewPoliciesWithAddToRolePolicy': false,
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.
Is idea to allow the new EcsTask changes to work with the default/recommended values ? Or maybe the this feature overall only works in the case that these flags are turned off?
Tell me if I am missing something here(could be that is is a very specific use-case this is trying to solve)
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 initially just copied these from the other tests within the stack, but yes, my specific tests don't really require me to change these values at all. I've removed these lines to revert these flags back to their default values.
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.
Hmm, when I remove the stack.node.setContext(EC2_RESTRICT_DEFAULT_SECURITY_GROUP, false)
the build step will fail since the Node runtime for a custom resource lambda resource it creates differs depending on the machine runs the test, see: https://github.com/aws/aws-cdk/actions/runs/17996160888/job/51196012271 .
Performing a text search reveals that a number of other integrations tests do the same thing to, supposedly, circumvent this issue.
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 think the mapping depends on the region : https://github.com/aws/aws-cdk/actions/runs/17996160888/job/51196012271#step:9:7691
We should use the default/recommended value and run the integration tests to check if the assertions work.
The other tests in the same folder seem to older than the feature flag. Though it's not in the scope of this PR if you want we can revert those tests to use the default values of those flags. (if not we can take that up as another PR as well)
If you need help with running the integration tests you can check this guide : https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md#running-integration-tests
Thanks for raising the PR @Michae1CC , added a few inline comments |
Thanks for the review, I've added in your suggestions. |
78a57c3
to
f1900d9
Compare
I have questions on the PR description, around the 2 statements in descriptions below :
The "not" in both statements is confusing me a bit. The PR does add these specific conditions as Also can you include the proper document that captures this ECS and step-function integration. That would be helpful as reference for the changes being done. Is this the correct link ? |
I mean to say that the current implementation does not include IAM condition checks to the The condition check I've added to the The condition check I've added to the
Yes that is the correct link. |
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.
Thanks for the explanation
Can you also add the link in the PR description so that it's easy to understand the context ? (the documentation link that documents details about ECS and step-function integration)
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/ecs/run-task.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/ecs/run-task.ts
Outdated
Show resolved
Hide resolved
70352b5
to
4093326
Compare
I've added the link |
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/ecs/run-task.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/ecs/run-task.ts
Outdated
Show resolved
Hide resolved
82d4029
to
85ad85f
Compare
85ad85f
to
eb2fb99
Compare
Issue # (if applicable)
Closes #35307.
Reason for this change
AWS Step Functions provide users with the ability to synchronously run ECS tasks via the
.sync
integration pattern, see: https://docs.aws.amazon.com/step-functions/latest/dg/connect-ecs.html. These changes provide CDK users with the ability to provide the task definition ARN as a JSONata/JSONPath inaws_stepfunctions_tasks.EcsRunTask
construct with the.sync
integration pattern. This current isn't possible since theaws_stepfunctions_tasks.EcsRunTask
construct currently requires the task definition to be provided at the time of deploy viaaws_ecs.TaskDefinition
. This enhancement will benefit users building dynamic ECS workflows in Step Functions.Description of changes
Note: These changes allow for complete backwards compatibility and do not effect current usages of
aws_stepfunction_tasks.EcsRunTaskOptions
.taskDefinitionInput
with typestep_function.TaskInput
has been added to theaws_stepfunction_tasks.EcsRunTaskOptions
interface. This will provide users with the ability to provide a ECS task definition using a JSONPath/JSONata expression. A validation error is raise if both or neither thetaskDefinition
andtaskDefinitionInput
are provided._renderTask
task has been modified to use thevalue
property oftaskDefinitionInput
(if thetaskDefinitionInput
is used) when generating theTaskDefinition
. Otherwise the current implementation of generating theTaskDefinition
is used when thetaskDefinition
parameter is provided (to maintain backwards compatibility).taskDefinitionInput
has been typed withstep_function.TaskInput
to remain consistent with how otheraws_stepfunction_tasks
constructs handle parameters that may be JSONPath/JSONata expression (example: https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_stepfunctions_tasks.SnsPublish.html#message).taskDefinitionInput
has been chosen overtaskDefinitionArn
since there is a linter rule which does not allow parameters to be prefixed with "Arn".taskDefinitionInput
andrevisionNumber
since it is impossible to format in arevisionNumber
if a JSONPath/JSONata expression is provided for thetaskDefinitionInput
.networkMode
,taskRole
andtaskExecutionRole
are used to configure AWS_VPC networking configuration and IAM permissions respectively whentaskDefinitionInput
is used. A validation error is thrown if any of these parameters are set when providing ataskDefinition
since these parameters should instead be set on thetaskDefinition
, helping reduce implementation logic/conflating.networkMode
is added since the network mode is currently gleamed from thetaskDefinition
, this cannot be done withtaskDefinitionInput
.taskRole
andtaskExecutionRole
values set on thetaskDefinition
are used to create IAM PassRole permissions. The optionaltaskRole
andtaskExecutionRole
provided through theaws_stepfunction_tasks.EcsRunTaskOptions
interface are used to reach parity with this particular feature of theaws_stepfunctions_tasks.EcsRunTask
construct.taskRole
and/ortaskExecutionRole
is not provided when using thetaskDefinitionInput
since, strictly speaking, an ECS task requires neither a task role nor a task execution role, see: https://docs.aws.amazon.com/AmazonECS/latest/developerguide/security-ecs-iam-role-overview.htmlDescribe any new or updated permissions being added
taskRole
and/ortaskExecutionRole
parameters are provided when using thetaskDefinitionInput
then the following statement is added to the step function role policy'iam:PassedToService'
condition is not present on the'iam:PassRole'
statement generated when using ataskDefinition
, this is a huge gain in terms of security since it prevents the step function from potentially passing highly privileged permission to arbitrary principals. The only reason why I have not included the condition changes for thetaskDefinition
case is because these changes would be breaking and is probably best completed as its own change (if preferable to add the changes here let me know and I'll create a new ticket for it).taskDefinitionInput
the following statement is added to the step function role policy'ecs:cluster'
condition is not present on the'ecs:RunTask'
statement generated when using ataskDefinition
, I think this is also an improvement to security, preventing the step function from running tasks from any ECS cluster. Similarly, the only reason I have not included the condition changes for thetaskDefinition
case is because these changes would be breaking and is probably best completed as its own change.Description of how you validated changes
integ.fargate-run-task-taskdef-input
the integration test. I have confirmed the step function deployed in this integration test executes and completes successfully.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license