aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.65k stars 3.91k forks source link

(ecs-patterns): allow specifying IAM-role for `EcsTask` through `ScheduledFargateTask` #22673

Open hannseman opened 2 years ago

hannseman commented 2 years ago

Describe the feature

Allow passing a IAM Role to ScheduledFargateTask which in turn is passed as the role argument to EcsTask.

Use Case

The default IAM role created in EcsTask gets a policy for ecs:RunTask with the full task definition arn as the resource, that is with its revision. I want to be able to set a ecs:RunTask policy with a wild card as the task definition revision component, i.e instead of:

PolicyStatement(
    actions=["ecs:RunTask"],
    resource=["arn:aws:ecs:XX:XX:task-definition/some-task-definition:42"],
    ...
)

I want:

PolicyStatement(
    actions=["ecs:RunTask"],
    resource=["arn:aws:ecs:XX:XX:task-definition/some-task-definition:*"],
    ...
)

See: https://github.com/aws/aws-cdk/blob/66d1ed36b1628c116d5f1b3397688308d888c9de/packages/%40aws-cdk/aws-events-targets/lib/ecs-task.ts#L198-L204

Proposed Solution

My proposal is to leverage the already existing role argument on EcsTask by simply adding the same argument to ScheduledFargateTask and passing it on through.

Other Information

No response

Acknowledgements

CDK version used

2.44

Environment details (OS name and version, etc.)

MacOS 12.6.1

peterwoodworth commented 2 years ago

Thanks for the feature request @hannseman,

We should be able to implement this pretty easily here https://github.com/aws/aws-cdk/blob/3528e3de5b5dd0a520624d4af73413370dcdc434/packages/%40aws-cdk/aws-ecs-patterns/lib/fargate/scheduled-fargate-task.ts#L92

Contributions are welcome! Check out our contributing guide if you're interested - there's a low chance the team will be able to address this soon but we'd be happy to review a PR 🙂

sakurai-ryo commented 1 year ago

I would like to use this feature with my team and will submit a PR again. If there are any problems I will close the PR.

comcalvi commented 1 year ago

@hannseman Could you elaborate on why you want to do this? Why is including the wildcard necessary?

hannseman commented 1 year ago

@hannseman Could you elaborate on why you want to do this? Why is including the wildcard necessary?

After deploying new revisions of the task definition, like after updating the image outside of CDK (for example with CodePipeline), the policy will no longer be valid since it's locked to a specific revision, using a wildcard solves this.

comcalvi commented 1 year ago

After deploying new revisions of the task definition, like after updating the image outside of CDK (for example with CodePipeline)

This is deliberately introducing drift into your application. The recommended way to do this is to use CDK Pipelines, or, if you want to manage your images outside of your stacks, use CodeDeploy to push the instance to your ECR repo and have CDK point to the image there.

If you can't / don't want to do those, can you use the L2 constructs directly instead? They'll give you full control over the role