feat(ecs): Support specifying revision of task definition by luxaritas · Pull Request #27036 · aws/aws-cdk

luxaritas · 2023-09-06T23:25:31Z

If using CodePipeline EcsDeployAction without using the CODE_DEPLOY deployment controller, future deployments of an ECS service will revert the task definition to the task definition deployed by CloudFormation, even though the latest active revision created by the deploy action is the one that is intended to be used. This provides a way to specify the specific revision of a task definition that should be used, including the special value latest which uses the latest ACTIVE revision.

Closes #26983.

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

lpizzinidev

Thanks for the implementation (and sorry for the late review) 👍
Some minor adjustments are needed in my opinion.
Not sure about the behavior for DeploymentControllerType.CODE_DEPLOY so feel free to clarify that.
Also, are you sure that it is necessary to explicitly set latest?
Looks like the ECS service defaults to the latest active value already.

lpizzinidev · 2023-10-09T10:14:00Z

packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts

+   *
+   * @default - Uses the revision of the passed task definition deployed by CloudFormation
+   */
+  readonly taskDefinitionRevision?: string;


Suggested change

readonly taskDefinitionRevision?: string;

readonly taskDefinitionRevision?: number | 'latest';

Stricter type definition helps with validation and enables users to specify the correct value.

Isn't this not allowed by CDK guidelines?

aws-cdk/docs/DESIGN_GUIDELINES.md

Lines 660 to 664 in 94ccec6

#### Unions

Do not use TypeScript union types in construct APIs (`string | number`) since

many of the target languages supported by the CDK cannot strongly-model such

types _[awslint:props-no-unions]_.

Ive attempted to do what the guidelines suggest and use a class with static methods, though would appreciate a look over how I approached that

lpizzinidev · 2023-10-25T14:52:24Z

packages/aws-cdk-lib/aws-ecs/README.md

+By default, the service will use the revision of the passed task definition generated when the `TaskDefinition`
+is deployed by CloudFormation. In order to specify a specific revision, pass a `taskDefinitionRevision`:
+
+```ts
+declare const cluster: ecs.Cluster;
+declare const taskDefinition: ecs.TaskDefinition;
+
+const service = new ecs.ExternalService(this, 'Service', {
+  cluster,
+  taskDefinition,
+  desiredCount: 5,
+  taskDefinitionRevision: '1'
+});
+```
+
+Or, to always use the latest active revision (for example, when using the CodePipeline EcsDeployAction
+without using the CODE_DEPLOY deployment controller to ensure future service deployments don't revert
+the task revision used by the service):
+
+```ts
+declare const cluster: ecs.Cluster;
+declare const taskDefinition: ecs.TaskDefinition;
+
+const service = new ecs.ExternalService(this, 'Service', {
+  cluster,
+  taskDefinition,
+  desiredCount: 5,
+  taskDefinitionRevision: 'latest'
+});


Suggested change

By default, the service will use the revision of the passed task definition generated when the `TaskDefinition`

is deployed by CloudFormation. In order to specify a specific revision, pass a `taskDefinitionRevision`:

```ts

declare const cluster: ecs.Cluster;

declare const taskDefinition: ecs.TaskDefinition;

const service = new ecs.ExternalService(this, 'Service', {

cluster,

taskDefinition,

desiredCount: 5,

taskDefinitionRevision: '1'

});

```

Or, to always use the latest active revision (for example, when using the CodePipeline EcsDeployAction

without using the CODE_DEPLOY deployment controller to ensure future service deployments don't revert

the task revision used by the service):

```ts

declare const cluster: ecs.Cluster;

declare const taskDefinition: ecs.TaskDefinition;

const service = new ecs.ExternalService(this, 'Service', {

cluster,

taskDefinition,

desiredCount: 5,

taskDefinitionRevision: 'latest'

});

```suggestion

By default, the service will use the revision of the passed task definition generated when the `TaskDefinition`

is deployed by CloudFormation.

To set a specific revision number or use the latest revision use the `taskDefinitionRevision` parameter:

```ts

declare const cluster: ecs.Cluster;

declare const taskDefinition: ecs.TaskDefinition;

new ecs.ExternalService(this, 'Service', {

cluster,

taskDefinition,

desiredCount: 5,

taskDefinitionRevision: 1

});

new ecs.ExternalService(this, 'Service', {

cluster,

taskDefinition,

desiredCount: 5,

taskDefinitionRevision: 'latest'

});

More concise.

To make sure I understand correctly, you think the description of the latest special case and why it would be used is unnecessary/too much information?

No, but I think that documentation needs to be clear and direct.
Feel free to add examples or use cases to my suggestion.

lpizzinidev · 2023-10-25T15:22:30Z

packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts

    }

+    if (props.taskDefinitionRevision) {
+      this.resource.taskDefinition = taskDefinition.family;
+      if (props.taskDefinitionRevision !== 'latest') {
+        this.resource.taskDefinition += `:${props.taskDefinitionRevision}`;
+      }
+      this.node.addDependency(taskDefinition);
+    }
+


Suggested change

}

if (props.taskDefinitionRevision) {

this.resource.taskDefinition = taskDefinition.family;

if (props.taskDefinitionRevision !== 'latest') {

this.resource.taskDefinition += `:${props.taskDefinitionRevision}`;

}

this.node.addDependency(taskDefinition);

}

} else if (props.taskDefinitionRevision) {

if (props.taskDefinitionRevision !== 'latest' && props.taskDefinitionRevision < 1) {

throw new Error(`taskDefinitionRevision must be 'latest' or a positive number, got ${props.taskDefinitionRevision}`);

}

this.resource.taskDefinition = taskDefinition.family + (props.taskDefinitionRevision !== 'latest'

`:${props.taskDefinitionRevision}` : '');

this.node.addDependency(taskDefinition);

}

Should the revision number be enforced only for non-CODE_DEPLOY types?
Also, I would add validation for the revision when numeric (and a relative unit test).

Hmmm... initially I didn't see any reason why the revision can't be made more specific than latest for CODE_DEPLOY - it's only as it is so that it's not whatever the latest version is at the time of deployment. However, the entire point of CODE_DEPLOY is that it's being deployed from code deploy, so if a new deployment is triggered it would override this anyways. I could maybe see a use case for if you want to temporarily pin and just not run the pipeline, but that seems rare and likely to cause more confusion than benefit. So you're probably right.

Good point on additionally validating that it's a number if not latest

the entire point of CODE_DEPLOY is that it's being deployed from code deploy, so if a new deployment is triggered it would override this anyways

If this is the case it's better to keep the else if then, otherwise this.resource.taskDefinition would get overridden if taskDefinitionRevision is numeric.

In this case, I'll also throw an error if a non-latest revision is specified and the deployment type is CODE_DEPLOY, to prevent confusion around explicitly asking for something different and it silently not doing that

lpizzinidev · 2023-10-25T15:24:46Z

packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts

+   * The revision of the service's task definition to use for tasks in this service
+   * or 'latest' to use the latest ACTIVE task revision


Suggested change

* The revision of the service's task definition to use for tasks in this service

* or 'latest' to use the latest ACTIVE task revision

* Revision number for the task definition or `latest` to use the latest active task revision.

luxaritas · 2023-10-25T23:40:14Z

Thanks for the response - some good points which I should be able to address soon. With respect to the comments not mentioned inline:

Not sure about the behavior for DeploymentControllerType.CODE_DEPLOY so feel free to clarify that.

See

aws-cdk/packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts

Lines 638 to 644 in c551a64

    
           if (props.deploymentController?.type === DeploymentControllerType.CODE_DEPLOY) { 
        
             // Strip the revision ID from the service's task definition property to 
        
             // prevent new task def revisions in the stack from triggering updates 
        
             // to the stack's ECS service resource 
        
             this.resource.taskDefinition = taskDefinition.family; 
        
             this.node.addDependency(taskDefinition); 
        
           }

Also, are you sure that it is necessary to explicitly set latest?
Looks like the ECS service defaults to the latest active value already.

Yes - and indeed it is already done in the code I linked above. I believe the reason why it is necessary is because in the concrete implementations of BaseService (FargateService/Ec2Service), the task arn is passed as part of additionalProps, and the arn includes the revision

lpizzinidev · 2023-10-26T14:09:44Z

@luxaritas

Yes - and indeed it is already done in the code I linked above. I believe the reason why it is necessary is because in the concrete implementations of BaseService (FargateService/Ec2Service), the task arn is passed as part of additionalProps, and the arn includes the revision

Ok, got it.
Thanks for the follow-up and clarifications 👍

… deployment controller

luxaritas · 2023-11-11T05:43:48Z

Sorry for the delay in revisiting this. Made changes based on your comments. I've adjusted the way taskDefinitionRevision is typed per the comment thread above, let me know if it's still not right.

lpizzinidev

Thanks 👍
Some adjustments for the added data structure are needed.

lpizzinidev · 2023-11-11T16:27:08Z

packages/aws-cdk-lib/aws-ecs/lib/base/task-definition.ts

+
+/**
+ * Represents revision of a task definition, either a specific numbered revision or
+ * the specia "latest" revision
+ */
+export class TaskDefinitionRevision {
+  /**
+   * The most recent revision of a task
+   */
+  static latest() {
+    return new TaskDefinitionRevision('latest');
+  }
+
+  /**
+   * A specfic numbered revision of a task
+   */
+  static revision(revision: number) {
+    return new TaskDefinitionRevision(revision.toString());
+  }
+
+  public readonly revisionId: string;
+
+  constructor(revision: string) {
+    this.revisionId = revision;
+  }
+}


Suggested change

/**

* Represents revision of a task definition, either a specific numbered revision or

* the specia "latest" revision

*/

export class TaskDefinitionRevision {

/**

* The most recent revision of a task

*/

static latest() {

return new TaskDefinitionRevision('latest');

}

/**

* A specfic numbered revision of a task

*/

static revision(revision: number) {

return new TaskDefinitionRevision(revision.toString());

}

public readonly revisionId: string;

constructor(revision: string) {

this.revisionId = revision;

}

}

/**

* Represents a task definition revision.

*/

export class TaskDefinitionRevision {

/**

* The most recent revision of a task

*/

public static readonly LATEST = new TaskDefinitionRevision('latest');

/**

* Specific revision of a task

*/

public static of(revision: number) {

if (revision < 1) {

throw new Error(`A task definition revision must be 'latest' or a positive number, got ${revision}`);

}

return new TaskDefinitionRevision(revision.toString());

}

private constructor(public readonly revision: string) {}

}

Let's follow the enum-class pattern for this.
Thanks for the clarification on union types 👍
A unit test is needed on revision number validation.

lpizzinidev · 2023-11-11T16:29:32Z

packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts

+      if (props.taskDefinitionRevision.revisionId !== 'latest') {
+        this.resource.taskDefinition += `:${props.taskDefinitionRevision.revisionId}`;


Suggested change

if (props.taskDefinitionRevision.revisionId !== 'latest') {

this.resource.taskDefinition += `:${props.taskDefinitionRevision.revisionId}`;

if (props.taskDefinitionRevision !== TaskDefinitionRevision.LATEST) {

this.resource.taskDefinition += `:${props.taskDefinitionRevision.revision}`;

lpizzinidev · 2023-11-11T16:32:54Z

packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts

+    if (
+      props.deploymentController?.type === DeploymentControllerType.CODE_DEPLOY
+      && props.taskDefinitionRevision
+      && props.taskDefinitionRevision.revisionId !== 'latest'
+    ) {
+      throw new Error('CODE_DEPLOY deploymentController cannot be used with a non-latest task definition revision');
+    }
+


Suggested change

if (

props.deploymentController?.type === DeploymentControllerType.CODE_DEPLOY

&& props.taskDefinitionRevision

&& props.taskDefinitionRevision.revisionId !== 'latest'

) {

throw new Error('CODE_DEPLOY deploymentController cannot be used with a non-latest task definition revision');

}

I don't think it's necessary.
If props.deploymentController?.type === DeploymentControllerType.CODE_DEPLOY we will run the first if statement and taskDefinitionRevision will just get ignored.

The reason I did this is because if the user specifically asked for revision 23, but they got latest instead, that seems like an unintended surprise/footgun

Thanks for clarifying, however, I think it's overkill to fail a deployment that would work.
Let's use Annotations.addWarningV2 to provide some feedback and be more user-friendly.

Warning sounds completely fair - will do!

I disagree here, I feel like an error is reasonable even though the deployment would work because we are changing a setting that the user had set and not actually running the revision the user had specified.

That was my gut feeling. I'll revert that change unless I'm told otherwise

lpizzinidev

Thanks for the changes!
Some minor cleanup and it will be good to go for me.
Note #27036 (comment) from the previous review.

packages/aws-cdk-lib/aws-ecs/test/task-definition.test.ts

packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts

packages/aws-cdk-lib/aws-ecs/lib/base/task-definition.ts

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

paulhcsun · 2023-12-11T17:48:59Z

Overall looks good to me, just left one comment regarding changing the error to a warning. Thanks for the contribution and thanks @lpizzinidev for reviewing!

luxaritas · 2023-12-13T22:30:43Z

[Container] 2023/12/13 22:28:16.388276 Running command npm config set unsafe-perm true
npm ERR! unsafe-perm is not a valid npm option

Nothing I did, is this a situation where I just need to pull in the latest changes from main?

paulhcsun · 2023-12-14T17:54:44Z

I'm seeing the following linting error in the build logs, but it's strange because a docstring is included.

aws-cdk-lib: error: [awslint:docs-public-apis:aws-cdk-lib.aws_ecs.TaskDefinitionRevision.revision] Public API element must have a docstring 
aws-cdk-lib: Error: /codebuild/output/src2582654880/src/github.com/aws/aws-cdk/tools/@aws-cdk/cdk-build-tools/bin/cdk-awslint exited with error code 1
aws-cdk-lib: Build failed.!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

luxaritas · 2023-12-14T17:56:27Z

I think it's the public member declared in the constructor? I'm assuming that needs to be pulled out in order to add a docstring

packages/aws-cdk-lib/aws-ecs/lib/base/task-definition.ts

packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts

paulhcsun

A couple minor wording changes.

paulhcsun · 2023-12-14T18:12:35Z

I think it's the public member declared in the constructor? I'm assuming that needs to be pulled out in order to add a docstring

That could be it, could you give that a try please?

Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com>

aws-cdk-automation · 2023-12-14T18:58:06Z

AWS CodeBuild CI Report

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

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

paulhcsun

Great work @luxaritas, thanks for the contribution!

mergify · 2023-12-14T18:59:02Z

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).

feat(ecs): Support specifying revision of task definition

7c9b760

aws-cdk-automation requested a review from a team September 6, 2023 23:25

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Sep 6, 2023

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 6, 2023

github-actions bot mentioned this pull request Oct 1, 2023

Monthly PRs metrics report - Sep 2023 #27368

Closed

lpizzinidev suggested changes Oct 25, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 26, 2023

luxaritas added 7 commits November 11, 2023 03:23

Add literal value for better autocomplete

53a95f8

Dont allow task definition revision to be overridden with CODE_DEPLOY…

22f3460

… deployment controller

Docs tweaks

20bff80

Fix type in docs

c1799ee

Change from union type to class with static methods

865c716

Fix intermediary cruft

59452e4

Fix property overlap

c3821cb

lpizzinidev suggested changes Nov 11, 2023

View reviewed changes

luxaritas added 2 commits November 14, 2023 02:04

Enum-class pattern

3d6073e

Error -> warning

0491d13

lpizzinidev suggested changes Nov 16, 2023

View reviewed changes

luxaritas and others added 5 commits November 16, 2023 14:31

Fix enum reference

164f57b

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

Copy tweaks

e35c1ac

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

Revision vs revision ID

4506776

Enum usage

2f4918b

Fix constructor access

363ba6f

lpizzinidev approved these changes Nov 16, 2023

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 16, 2023

Revert warning back to error

cfe0111

Merge branch 'main' into ecs-task-specific-revision

861f4fc

paulhcsun reviewed Dec 14, 2023

View reviewed changes

packages/aws-cdk-lib/aws-ecs/lib/base/task-definition.ts Outdated Show resolved Hide resolved

paulhcsun reviewed Dec 14, 2023

View reviewed changes

packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts Outdated Show resolved Hide resolved

paulhcsun reviewed Dec 14, 2023

View reviewed changes

luxaritas and others added 3 commits December 14, 2023 18:17

Wording tweaks

58aefdb

Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com>

Docstring for revision string member

e5265b3

Reorder fields

ef6f775

paulhcsun approved these changes Dec 14, 2023

View reviewed changes

mergify bot merged commit de0d77b into aws:main Dec 14, 2023

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 14, 2023

roeysha mentioned this pull request Mar 31, 2024

(aws-ecs): (ECS Patterns: Support Latest Task Definition Revision for ApplicationLoadBalancedFargateService) #29662

Open

2 tasks

	readonly taskDefinitionRevision?: string;
	readonly taskDefinitionRevision?: number \| 'latest';

	#### Unions

	Do not use TypeScript union types in construct APIs (`string \| number`) since
	many of the target languages supported by the CDK cannot strongly-model such
	types _[awslint:props-no-unions]_.

		* The revision of the service's task definition to use for tasks in this service
		* or 'latest' to use the latest ACTIVE task revision

	* The revision of the service's task definition to use for tasks in this service
	* or 'latest' to use the latest ACTIVE task revision
	* Revision number for the task definition or `latest` to use the latest active task revision.

		if (props.taskDefinitionRevision.revisionId !== 'latest') {
		this.resource.taskDefinition += `:${props.taskDefinitionRevision.revisionId}`;

Conversation

luxaritas commented Sep 6, 2023

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luxaritas Nov 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luxaritas Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luxaritas commented Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpizzinidev commented Oct 26, 2023

Uh oh!

luxaritas commented Nov 11, 2023

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luxaritas Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulhcsun commented Dec 11, 2023

Uh oh!

luxaritas commented Dec 13, 2023

Uh oh!

paulhcsun commented Dec 14, 2023

Uh oh!

luxaritas commented Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulhcsun left a comment

luxaritas Nov 11, 2023 •

edited

Loading

luxaritas Oct 25, 2023 •

edited

Loading

luxaritas commented Oct 25, 2023 •

edited

Loading

luxaritas Dec 13, 2023 •

edited

Loading

luxaritas commented Dec 14, 2023 •

edited

Loading