fix(event-targets): ecsTask uses invalid task definition arn in policy

[samson-keung]

Issue # (if applicable)

Closes #30390 .

Reason for this change

This is extending a closed PR #30484 by @jwoehrle . I couldn't update that PR so I am creating this new one.

Reason for this change is due to a AWS ECS campaign where they are asking customers to add task definition revision number (or wildcard as the revision number) to IAM policies.

Description of changes

When adding permission to the Events Role to allow it to use the task definition, check if the task definition arn has a revision number, if yes, do nothing, if not, add the wildcard *. This is only done when the task definition arn is not using any token.

Description of how you validated changes

Unit tests and Integ tests are added.

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

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

[msambol]

Per #30751, without the * isn't valid. I believe you can copy this PR and simplify the code here.

[msambol]

This code isn't needed. As mentioned in #30751, the task definition without the trailing :* isn't a valid ARN.

[samson-keung]

Hi @msambol , thanks for taking time to review. It is possible for taskDefinitionArn to have a revision number that isn't *, hence, this if block. When there is already a revision number, we shouldn't append :*. I left a comment in the PR you linked about this case.

[msambol]

The * means it will match all revision numbers (it's a wildcard).

[samson-keung]

Yes. That is correct. It is also possible that customer may want to restrict the IAM policy resource to specific revision number.

Sample code of such case where customer is specifying the revision number MyTask:19:

import { Duration, Stack, type StackProps } from 'aws-cdk-lib';
import { Cluster, FargateTaskDefinition, NetworkMode } from 'aws-cdk-lib/aws-ecs';
import { Rule, Schedule } from 'aws-cdk-lib/aws-events';
import { EcsTask } from 'aws-cdk-lib/aws-events-targets';
import { Role } from 'aws-cdk-lib/aws-iam';
import { Construct } from 'constructs';

export class TestCdkStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);

    const cluster = new Cluster(this, 'Cluster');
    const taskDefinition = FargateTaskDefinition.fromFargateTaskDefinitionAttributes(this, "TaskDefImport", {
      taskDefinitionArn: "arn:aws:ecs:us-west-2:123456789101:task-definition/MyTask:19",
      taskRole: Role.fromRoleArn(this, "RoleImport", "arn:aws:iam::123456789101:role/MyTaskRole"),
      networkMode: NetworkMode.AWS_VPC,
    });
    new Rule(this, "Rule", {
      targets: [new EcsTask({
        cluster,
        taskDefinition
      })],
      schedule: Schedule.rate(Duration.days(1)),
    });
  }
}

[msambol]

apologies! I'm with you now. In https://github.com/msambol/aws-cdk/blob/main/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/ecs/run-task.ts, the revision number is stripped out.

[samson-keung]

No worries. Appreciate the time for checking. I will open an issue about the StepFunction ECS task.

[msambol]

That said, could you move this code to a function and write unit tests for it? I think it'd be easier to follow. Similar to https://github.com/msambol/aws-cdk/blob/main/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/ecs/run-task.ts#L379-L396.

    let needsRevisionWildcard = false;
    if (!cdk.Token.isUnresolved(this.taskDefinition.taskDefinitionArn)) {
      const revisionAtEndPattern = /:[0-9]+$/;
      const hasRevision = revisionAtEndPattern.test(this.taskDefinition.taskDefinitionArn);
      needsRevisionWildcard = !hasRevision;
    }

[msambol]

Obviously different purpose of the function but similar pattern.

[samson-keung]

Moving it out to a public method to allow unit testing means exposing it for potential public use, which will become a API contract that cannot have any breaking change. I would prefer keeping it private and local in this method for now. The unit tests added in this PR are primarily focusing on this block of code.

[msambol]

See inline comments.

[msambol]

Does this policy have the trailing :* ?

[samson-keung]

This uses a CloudFormation (CFN for short) Ref token to reference the TaskDefinition resource. According to the CFN doc, the token will resolve to the TaskDefinition ARN >with< the revision number (but not *).

[msambol]

👍🏼 – I wanted to make sure there was either a revision number or a * at the end.

[msambol]

@samson-keung I think this integration test should in aws-event-targets , no ?

[samson-keung]

You are right! Let me move it.

[msambol]

This is what I wanted to see 👍🏼

[msambol]

nit: could you change the verbiage here? Based on the name, I'm not certain what it's testing.

[samson-keung]

Moving it out to a public method to allow unit testing means exposing it for potential public use, which will become a API contract that cannot have any breaking change. I would prefer keeping it private and local in this method for now. The unit tests added in this PR are primarily focusing on this block of code.

[msambol]

LGTM. Sorry for the back and forth!

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 239fdaa
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository