fix(event-targets): ecsTask uses invalid task definition arn in policy #31615
Conversation
github-actions
bot
added
bug
| const revisionAtEndPattern = /:[0-9]+$/; | ||
| const hasRevision = revisionAtEndPattern.test(this.taskDefinition.taskDefinitionArn); | ||
| needsRevisionWildcard = !hasRevision; | ||
| } |
There was a problem hiding this comment.
This code isn't needed. As mentioned in #30751, the task definition without the trailing :* isn't a valid ARN.
There was a problem hiding this comment.
There was a problem hiding this comment.
The * means it will match all revision numbers (it's a wildcard).
There was a problem hiding this comment.
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)),
});
}
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
No worries. Appreciate the time for checking. I will open an issue about the StepFunction ECS task.
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
Obviously different purpose of the function but similar pattern.
There was a problem hiding this comment.
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.
aws-cdk-automation
added
the
pr/needs-maintainer-review
Co-authored-by: Michael Sambol <sambol.michael@gmail.com>
| Match.objectLike({ | ||
| Action: 'ecs:RunTask', | ||
| Resource: { | ||
| Ref: 'TaskDef54694570', |
There was a problem hiding this comment.
Does this policy have the trailing :* ?
There was a problem hiding this comment.
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 *).
There was a problem hiding this comment.
👍🏼 – I wanted to make sure there was either a revision number or a * at the end.
packages/aws-cdk-lib/aws-events-targets/test/ecs/event-rule-target.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Michael Sambol <sambol.michael@gmail.com>
|
|
||
| new IntegTest(app, 'IntegTest-EcsImportedTaskDefinition', { | ||
| testCases: [stack], | ||
| }); |
There was a problem hiding this comment.
@samson-keung I think this integration test should in aws-event-targets , no ?
There was a problem hiding this comment.
You are right! Let me move it.
| } | ||
| }, | ||
| "Effect": "Allow", | ||
| "Resource": "arn:aws:ecs:test-region:12345678:task-definition/TaskDef:*" |
There was a problem hiding this comment.
This is what I wanted to see 👍🏼
| @@ -1095,3 +1096,100 @@ test.each([ | |||
| ], | |||
| }); | |||
| }); | |||
|
|
|||
| test('Non-imported TaskDefinition role is targeting by Ref', () => { | |||
There was a problem hiding this comment.
nit: could you change the verbiage here? Based on the name, I'm not certain what it's testing.
| const revisionAtEndPattern = /:[0-9]+$/; | ||
| const hasRevision = revisionAtEndPattern.test(this.taskDefinition.taskDefinitionArn); | ||
| needsRevisionWildcard = !hasRevision; | ||
| } |
There was a problem hiding this comment.
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.
packages/aws-cdk-lib/aws-events-targets/test/ecs/event-rule-target.test.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-events-targets/test/ecs/event-rule-target.test.ts
Outdated
Show resolved
Hide resolved
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
aws-cdk-automation
removed
the
pr/needs-maintainer-review
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
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
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license