Skip to content
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

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

Open
1 of 2 tasks
blimmer opened this issue Apr 7, 2023 · 3 comments
Open
1 of 2 tasks

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

blimmer opened this issue Apr 7, 2023 · 3 comments
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2

Comments

@blimmer
Copy link
Contributor

blimmer commented Apr 7, 2023

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:

/**
* The common task definition attributes used across all types of task definitions.
*/
export interface CommonTaskDefinitionAttributes {
/**
* The arn of the task definition
*/
readonly taskDefinitionArn: string;
/**
* The networking mode to use for the containers in the task.
*
* @default Network mode cannot be provided to the imported task.
*/
readonly networkMode?: NetworkMode;
/**
* The name of the IAM role that grants containers in the task permission to call AWS APIs on your behalf.
*
* @default Permissions cannot be granted to the imported task.
*/
readonly taskRole?: iam.IRole;
}

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:

// If it so happens that a Task Execution Role was created for the TaskDefinition,
// then the EventBridge Role must have permissions to pass it (otherwise it doesn't).
if (this.taskDefinition.executionRole !== undefined) {
policyStatements.push(new iam.PolicyStatement({
actions: ['iam:PassRole'],
resources: [this.taskDefinition.executionRole.roleArn],
}));
}

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:

/**
* Execution role for this task definition
*/
readonly executionRole?: IRole = undefined;

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

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.73.0

Environment details (OS name and version, etc.)

macOS

@blimmer blimmer added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Apr 7, 2023
@github-actions github-actions bot added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Apr 7, 2023
blimmer added a commit to blimmer/aws-cdk that referenced this issue Apr 7, 2023
blimmer added a commit to blimmer/aws-cdk that referenced this issue Apr 7, 2023
blimmer added a commit to blimmer/aws-cdk that referenced this issue Apr 7, 2023
@blimmer
Copy link
Contributor Author

blimmer commented Apr 7, 2023

Fix proposed with #24987

@pahud
Copy link
Contributor

pahud commented Apr 7, 2023

Makes sense to me and thank you for your PR!

@pahud pahud added p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Apr 7, 2023
mergify bot pushed a commit that referenced this issue Apr 18, 2023
…24987)

See #24984 for details.

TLDR; there's not a way currently exposed to define the `executionRole` on an imported `TaskDefinition`. This change allows optionally passing an `executionRole`.

Fixes issue #24984.

----

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

deeTEEcee commented Jul 18, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

No branches or pull requests

3 participants