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.51k stars 3.86k forks source link

(ecs): Allow passing executionRole to imported TaskDefinitions #24984

Open blimmer opened 1 year ago

blimmer commented 1 year ago

Describe the feature

Currently, when importing a TaskDefinition, either via TaskDefinition.fromTaskDefinitionAttributes or Fargate.fromFargateTaskDefinitionAttributes, you can pass a taskRole, but not an executionRole.

In both cases, the CommonTaskDefinitionAttributes interface defines the attributes that can be passed when importing: https://github.com/aws/aws-cdk/blob/a98a98147534f89a219521a2e51a6a1e25a2ac06/packages/aws-cdk-lib/aws-ecs/lib/base/task-definition.ts#L231-L253

As you can see, you can pass taskRole but not executionRole.

Use Case

I have a shared TaskDefinition that I use in other CDK apps. This TaskDefinition is triggered by an EventBridge rule. I share the TaskDefinition ARN, Task Role ARN (and Execution Role ARN) via CfnOutputs.

Since I use a custom image stored in ECR, my TaskDefinition has both a Task Role and an Execution Role. When creating the EventBridge rule, the EcsTask target needs to allow the events ServicePrincipal to iam:PassRole both the Task Role and Execution Role for EventBridge to successfully RunTask.

The EcsTask target already has this logic: https://github.com/aws/aws-cdk/blob/a98a98147534f89a219521a2e51a6a1e25a2ac06/packages/aws-cdk-lib/aws-events-targets/lib/ecs-task.ts#L206-L213

However, because I can't pass the executionRole when I import the Task Definition, the logic to allow PassRole isn't added, and the EventBridge invocation fails.

I can work around this issue by forcing taskDefinition.executionRole assignment after the import:

(importedEcsTask.executionRole as any) = importedExecutionRole;

Proposed Solution

Currently, these .from imports rely on the ImportedTaskDefinition class: https://github.com/aws/aws-cdk/blob/a98a98147534f89a219521a2e51a6a1e25a2ac06/packages/aws-cdk-lib/aws-ecs/lib/base/_imported-task-definition.ts

This class already exposes an executionRole property: https://github.com/aws/aws-cdk/blob/a98a98147534f89a219521a2e51a6a1e25a2ac06/packages/aws-cdk-lib/aws-ecs/lib/base/_imported-task-definition.ts#L53-L57

However, it's not exposed for setting in the static methods. I think we can just add executionRole as an optional parameter to the CommonTaskDefinitionAttributes interface and allow passing it through.

Other Information

No response

Acknowledgements

CDK version used

2.73.0

Environment details (OS name and version, etc.)

macOS

blimmer commented 1 year ago

Fix proposed with https://github.com/aws/aws-cdk/pull/24987

pahud commented 1 year ago

Makes sense to me and thank you for your PR!

deeTEEcee commented 2 months ago

any idea why i get this error? it seems like it's saying that we don't have a taskRole in this task definition object either...

"Error: This operation requires the taskRole in ImportedTaskDefinition to be defined. Add the 'taskRole' in ImportedTaskDefinitionProps to instantiate ImportedTaskDefinition at ImportedTaskDefinition.get taskRole [as taskRole]"

const taskDef: ecs.IFargateTaskDefinition = ecs.FargateTaskDefinition.fromFargateTaskDefinitionArn(this, "pasture-task-definition", "<insert arn>");
const passRoleResources = [taskDef.taskRole.roleArn] # Leads to error