-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(scheduler-targets): EcsRunTask scheduler target #33697
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33697 +/- ##
=======================================
Coverage 82.24% 82.24%
=======================================
Files 119 119
Lines 6875 6875
Branches 1161 1161
=======================================
Hits 5654 5654
Misses 1118 1118
Partials 103 103
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
435019c
to
e97c70c
Compare
e97c70c
to
109b25f
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gracelu0 for this great work. LGTM overall. Left few nits and comments for my understanding.
* | ||
* @default - all private subnets of the VPC are selected. | ||
*/ | ||
readonly subnetSelection?: ec2.SubnetSelection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: May be it would be good to rename the property like vpcSubnets
or subnets
if (this.props.taskDefinition.executionRole !== undefined) { | ||
role.addToPrincipalPolicy(new PolicyStatement({ | ||
actions: ['iam:PassRole'], | ||
resources: [this.props.taskDefinition.executionRole.roleArn], | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qq: For my understanding, do we need to add PassRole
statement to the executionRole?
grantRun already provides the passRole statement required to run this task defintion. Usually ExecutionRole grants the ECS Fargate agent to make AWS API calls on behalf. I'm not sure if we need to add PassRole statement to it. From the unit test, I could see two times the PassRole is added for the same ARN
actions: ['iam:PassRole'], | ||
resources: [this.props.taskDefinition.taskRole.roleArn], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here not sure if we need to add PassRole statement to the task role.
target: targets.EcsRunTask.onFargate(cluster, { | ||
taskDefinition, | ||
subnetSelection: { subnetType: ec2.SubnetType.PRIVATE_WITH_EGRESS }, | ||
assignPublicIp: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have one test case for assignPublicIp: true
with public subnet?
stack = new cdk.Stack(); | ||
vpc = new ec2.Vpc(stack, 'Vpc'); | ||
cluster = new ecs.Cluster(stack, 'Cluster', { vpc }); | ||
taskDefinition = new ecs.TaskDefinition(stack, 'TaskDef', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: since this taskDefinition is for Fargate, would be good to rename to fargateTaskDefinition
?
new scheduler.Schedule(stack, 'Schedule', { | ||
schedule: scheduler.ScheduleExpression.rate(cdk.Duration.minutes(5)), | ||
target: targets.EcsRunTask.onFargate(cluster, { | ||
taskDefinition, | ||
subnetSelection: { subnetType: ec2.SubnetType.PRIVATE_WITH_EGRESS }, | ||
securityGroups: [securityGroup], | ||
}), | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we a unit test for both fargate and ec2 task by calling like below also?
new scheduler.Schedule(stack, 'Schedule', {
schedule: scheduler.ScheduleExpression.rate(cdk.Duration.minutes(5)),
target: new targets.FargateTask(...)
});
new scheduler.Schedule(stack, 'Schedule', {
schedule: scheduler.ScheduleExpression.rate(cdk.Duration.minutes(5)),
target: new targets.EC2Task(...)
});
PlacementConstraints: [{ | ||
Type: 'memberOf', | ||
Expression: 'task:group == databases', | ||
}], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also assert PlacementStrategy?
|
||
// Only one of capacityProviderStrategy or launchType can be set | ||
// See https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_RunTask.html#ECS-RunTask-request-launchType | ||
const launchType = this.capacityProviderStrategies ? undefined : ecs.LaunchType.EC2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Good to have a test case for this capacityProviderStrategies
and launchType behaviour.
Issue # (if applicable)
Closes #27456
Reason for this change
Currently the module supports all templated targets for EventBridge scheduler except for
EcsRunTask
.Description of changes
EcsRunTask
with public static methodsonFargate
andonEc2
depending on where user wants to schedule their ECS task. Decided on this design since some of the parameters inEcsParameters
only apply one of Fargate or EC2.Describe any new or updated permissions being added
ecs:RunTask
to the schedule execution role for the task definitionecs:TagResource
to the schedule execution tole for tasks in the clusteriam:PassRole
permissions to the schedule execution role for each task execution role (docs)iam:PassRole
permissions to the schedule execution role for the task roleDescription of how you validated changes
Added unit tests and integration tests with assertions.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license