fix(cli): cannot hotswap ECS task definitions containing certain intrinsics

[tmokmss]

Reason for this change

Currently an ECS task definition cannot be hotswapped in many cases, for example when it contains references to certain AWS resources as environment variables. This PR removes the limitation and make hotswaps possible in much more various situations.

Specifically, if there is any token that is not part of the following, we cannot hotswap a task definition:

1. Ref functions, which will be resolved as physical resource ids
2. GetAtt functions for some resources and attributes (manually supported one by one, code)
3. several simple CFn functions (join, split, select, sub)
4. parameters like AWS::AccountId, Region, Partition, or UrlSuffix

Although it is not supported much, using GetAtt function in a task definition is very common (imagine you reference other resource's property as an environment variable). This PR allows to hotswap a task definition even if it contains these tokens.

Solution

To hotswap a task definition, we need to construct a task definition to call registerTaskDefinition API. For this, we have to evaluate CloudFormation template locally to resolve all the intrinsics in the template. However, some intrinsics such as Fn::GetAtt is not fully supported by CDK CLI because we have to manually support them for each AWS resource type.

The reason why some task definitions are unhotswappable is that there are such intrinsics in the template and the CDK fails to evaluate it locally. So the basic idea to overcome this limitation in this PR is that we don't try to evaluate it locally, but we fetch the latest task definition already deployed and get the required values from it.

Here's how we can implement the idea.

How we determine if changes to a task definition can be hotswapped

In the hotswap process, we have to decide whether the change can be hotswapped or not. Now we can hotswap the task definition if

1. there are only changes in ContainerDefinitions field, and all the fields in the task definition can be evaluated locally. (original behavior) OR,
2. there are only changes in ContainerDefinitions field, and all the updated field can be evaluated locally (added in this PR).

The first condition can actually be included in the second condition, but for now I keep it as-is to avoid the possibility of breaking the existing behavior.

If the second condition is true, we fetch the latest task definition from AWS account, override the updated fields, and register a new task definition to update a service. By this way, we don't have to evaluate tokens in unchanged fields locally, allowing to use hotswap in much more situations.

How we compare the old and new task definition

Here is an example task definition:

{
    "ContainerDefinitions": [
        {
            "Environment": [
                {
                    "Name": "VPC_ID",
                    "Value": {
                        "Fn::GetAtt": [
                            "Vpc8378EB38",
                            "CidrBlock"
                        ]
                    }
                }
            ],
            "Essential": true,
            "Image": "nginx:stable",
            "Name": "EcsApp"
        }
    ],
    "Cpu": "256",
    "Family": "myteststackTask289798EC",
    "Memory": "512",
    "NetworkMode": "awsvpc",
    "RequiresCompatibilities": [
        "FARGATE"
    ],
    "TaskRoleArn": {
        "Fn::GetAtt": [
            "TaskTaskRoleE98524A1",
            "Arn"
        ]
    }
}

We compare the old and new task definition in the following steps:

1. Check if there are only changes in ContainerDefinitions field. If not, we cannot hotswap.
2. Try evaluateCfnExpression on the containerDefinitons. If it can be evaluated, proceed to hotswap. If not, proceed to step 3.
3. Check if the length of ContainerDefinitions is the same. If not, we cannot hotswap.
4. For each container definition, deep-compare each key (e.g. Environment, Image, Name, etc)
5. For each key, if there is any diff in the corresponding value, try evaluateCfnExpression on the value. If the evaluation fails, we cannot hotswap.
6. After checking all the keys and there is no field that cannot be hotswapped, proceed to hotswap.

Imagine if there is a change only in Image field (container image tag) but Environment field contains unsupported intrinsics (e.g. "Fn::GetAtt": ["Vpc8378EB38", "CidrBlock"]). In the previous CDK CLI we cannot hotswap it due to an evaluation error. We can now hotswap it because we don't have to evaluate the Environment field when it has no diffs.

Closes #25563

---

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

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

[tmokmss]

Exemption Request: hotswap code does not support integration tests. fwiw I tested it manually by deploying a stack with the below code:

import * as ec2 from 'aws-cdk-lib/aws-ec2';
import * as cdk from 'aws-cdk-lib';
import * as ecs from 'aws-cdk-lib/aws-ecs';
import { StringParameter } from 'aws-cdk-lib/aws-ssm';

const app = new cdk.App();
const stack = new cdk.Stack(app, 'my-test-stack');

const vpc = new ec2.Vpc(stack, 'Vpc', {
  subnetConfiguration: [
    {
      name: 'Public',
      subnetType: ec2.SubnetType.PUBLIC,
      cidrMask: 18,
    },
    {
      name: 'Isolated',
      subnetType: ec2.SubnetType.PRIVATE_ISOLATED,
      cidrMask: 18,
    },
  ],
});

const cluster = new ecs.Cluster(stack, 'EcsCluster', {
  vpc,
});

const taskDefinition = new ecs.FargateTaskDefinition(stack, 'Task', {});

taskDefinition.addContainer('EcsApp', {
  // change latest to stable and run hotswap
  image: ecs.ContainerImage.fromRegistry('nginx:latest'),
  environment: {
    // Previously we couldn't hotswap the task definition referencing properties like vpcCidrBlock.
    VPC_ID: vpc.vpcCidrBlock,
  },
});

cdk.Tags.of(taskDefinition).add('Key', 'Value');

new ecs.FargateService(stack, 'Service', {
  cluster,
  taskDefinition,
  assignPublicIp: true,
});

Place the file to somewhere like packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/hotswap.ts, run tsc, and run npx cdk -a test/aws-ecs/test/hotswap.js deploy.

[kaizencc]

Running the cli integ tests right now, just so I can exempt you from the linter

[tmokmss]

@kaizencc Thank you very much!

By the way, how about removing the cli integ test check from the PR linter? I believe the PR linter is for a contributor to validate if the PR is ready for review, but as for the cli integ check, there is nothing they can do to make the linter pass, which is a bit frustrating. I think the need to run the cli integ test can be expressed with another label.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[tmokmss]

@rix0rrr I added/fixed unit tests and now it's ready for review :) Hope you can check this, thanks.

[rix0rrr]

I like this a lot, thanks! Ready to ship it, but final check: did you do manual testing on the latest revision with all the requested changes in it?

Cheers!

[tmokmss]

@rix0rrr Thanks :) Yes, I confirmed that I can successfully hotswap this CDK code: #26404 (comment)

[aws-cdk-automation]

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

[mergify]

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

[mergify]

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

Pull request has been modified.

[tmokmss]

@rix0rrr mergify was failing for some reason:

Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/close-stale-prs.yml without workflows permission
err-code: 5E909

I guess merging with main manually will fix this issue (because updating workflow files from mergify is prohibited?). Can you approve it again? Thanks 🙏

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[mergify]

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