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

packages/aws-cdk/lib/api/hotswap/ecs-services.ts

@@ -33,7 +33,8 @@ export async function isHotswappableEcsServiceChange(
     // if there are no resources referencing the TaskDefinition,
     // hotswap is not possible in FALL_BACK mode
     reportNonHotswappableChange(ret, change, undefined, 'No ECS services reference the changed task definition', false);
-  } if (resourcesReferencingTaskDef.length > ecsServicesReferencingTaskDef.length) {
+  }
+  if (resourcesReferencingTaskDef.length > ecsServicesReferencingTaskDef.length) {
     // if something besides an ECS Service is referencing the TaskDefinition,
     // hotswap is not possible in FALL_BACK mode
     const nonEcsServiceTaskDefRefs = resourcesReferencingTaskDef.filter(r => r.Type !== 'AWS::ECS::Service');
@@ -45,14 +46,18 @@ export async function isHotswappableEcsServiceChange(
   const namesOfHotswappableChanges = Object.keys(classifiedChanges.hotswappableProps);
   if (namesOfHotswappableChanges.length > 0) {
     const taskDefinitionResource = await prepareTaskDefinitionChange(evaluateCfnTemplate, logicalId, change);
+    if (taskDefinitionResource === undefined) {
+      reportNonHotswappableChange(ret, change, undefined, 'Found unsupported changes to the task definition', false);
+      return ret;
+    }
     ret.push({
       hotswappable: true,
       resourceType: change.newValue.Type,
       propsChanged: namesOfHotswappableChanges,
       service: 'ecs-service',
       resourceNames: [
-        `ECS Task Definition '${await taskDefinitionResource.Family}'`,
         ...ecsServicesReferencingTaskDef.map(ecsService => `ECS Service '${ecsService.serviceArn.split('/')[2]}'`),
+        `ECS Task Definition '${taskDefinitionResource.family}'`,
       ],
       apply: async (sdk: ISDK) => {
         // Step 1 - update the changed TaskDefinition, creating a new TaskDefinition Revision
@@ -61,8 +66,8 @@ export async function isHotswappableEcsServiceChange(
 
         // The SDK requires more properties here than its worth doing explicit typing for
         // instead, just use all the old values in the diff to fill them in implicitly
-        const lowercasedTaskDef = transformObjectKeys(taskDefinitionResource, lowerCaseFirstCharacter, {
-          // All the properties that take arbitrary string as keys i.e. { "string" : "string" }
+        let lowercasedTaskDef = transformObjectKeys(taskDefinitionResource.taskDefinition, lowerCaseFirstCharacter, {
+          // Don't transform the properties that take arbitrary string as keys i.e. { "string" : "string" }
           // https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_RegisterTaskDefinition.html#API_RegisterTaskDefinition_RequestSyntax
           ContainerDefinitions: {
             DockerLabels: true,
@@ -80,6 +85,23 @@ export async function isHotswappableEcsServiceChange(
             },
           },
         });
+
+        if (taskDefinitionResource.hotswappableWithMerge) {
+          // get the latest task definition of the family
+          const describeTaskDefinitionResponse = await sdk
+            .ecs()
+            .describeTaskDefinition({
+              taskDefinition: taskDefinitionResource.family,
+              include: ['TAGS'],
+            })
+            .promise();
+          if (describeTaskDefinitionResponse.taskDefinition === undefined) {
+            throw new Error(`Could not find TaskDefinition ${taskDefinitionResource.family}`);
+          }
+          mergeTaskDefinitions(lowercasedTaskDef, describeTaskDefinitionResponse);
+          lowercasedTaskDef = describeTaskDefinitionResponse.taskDefinition;
+        }
+
         const registerTaskDefResponse = await sdk.ecs().registerTaskDefinition(lowercasedTaskDef).promise();
         const taskDefRevArn = registerTaskDefResponse.taskDefinition?.taskDefinitionArn;
 
@@ -167,13 +189,47 @@ export async function isHotswappableEcsServiceChange(
   return ret;
 }
 
+function mergeTaskDefinitions(patch: {containerDefinitions: {[key:string]: any}[]}, target: AWS.ECS.DescribeTaskDefinitionResponse) {
+  const src = patch.containerDefinitions;
+  const dst = target.taskDefinition?.containerDefinitions;
+  if (dst === undefined) return;
+  // schema: https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_ContainerDefinition.html
+  for (const i in src) {
+    for (const key of Object.keys(src[i])) {
+      (dst[i] as any)[key] = src[i][key];
+    }
+  }
+
+  // The describeTaskDefinition response contains several keys that must not exist in a registerTaskDefinition request.
+  // We remove these keys here, comparing these two structs:
+  // https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_RegisterTaskDefinition.html#API_RegisterTaskDefinition_RequestSyntax
+  // https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_DescribeTaskDefinition.html#API_DescribeTaskDefinition_ResponseSyntax
+  [
+    'compatibilities',
+    'taskDefinitionArn',
+    'revision',
+    'status',
+    'requiresAttributes',
+    'compatibilities',
+    'registeredAt',
+    'registeredBy',
+  ].forEach(key=> delete (target.taskDefinition as any)[key]);
+
+  if (target.tags !== undefined && target.tags.length > 0) {
+    // the tags field is in a different location in describeTaskDefinition response, moving it as intended for registerTaskDefinition request.
+    (target.taskDefinition as any).tags = target.tags;
+  }
+}
+
 interface EcsService {
   readonly serviceArn: string;
 }
 
 async function prepareTaskDefinitionChange(
-  evaluateCfnTemplate: EvaluateCloudFormationTemplate, logicalId: string, change: HotswappableChangeCandidate,
-) {
+  evaluateCfnTemplate: EvaluateCloudFormationTemplate,
+  logicalId: string,
+  change: HotswappableChangeCandidate,
+): Promise<undefined | { taskDefinition: any; family: string; hotswappableWithMerge: boolean }> {
   const taskDefinitionResource: { [name: string]: any } = {
     ...change.oldValue.Properties,
     ContainerDefinitions: change.newValue.Properties?.ContainerDefinitions,
@@ -195,11 +251,93 @@ async function prepareTaskDefinitionChange(
     // otherwise, familyNameOrArn is just the simple name evaluated from the CloudFormation template
     : familyNameOrArn;
   // then, let's evaluate the body of the remainder of the TaskDef (without the Family property)
-  return {
-    ...await evaluateCfnTemplate.evaluateCfnExpression({
+
+  let evaluated;
+  let hotswappableWithMerge = false;
+  try {
+    evaluated = await evaluateCfnTemplate.evaluateCfnExpression({
       ...(taskDefinitionResource ?? {}),
       Family: undefined,
-    }),
-    Family: family,
+    });
+  } catch (e) {
+    const result = await deepCompareContainerDefinitions(
+      evaluateCfnTemplate,
+      change.oldValue.Properties?.ContainerDefinitions ?? [],
+      change.newValue.Properties?.ContainerDefinitions ?? []);
+    if (result === false) {
+      throw e;
+    } else {
+      evaluated = {
+        // return a partial definition that should be merged with the current definition
+        ContainerDefinitions: result,
+      };
+      hotswappableWithMerge = true;
+    }
+  }
+
+  return {
+    taskDefinition: {
+      ...evaluated,
+      Family: family,
+    },
+    family,
+    hotswappableWithMerge,
   };
 }
+
+// return false if the new container definitions cannot be hotswapped
+// i.e. there are changes containing unsupported tokens or additions/removals of properties
+// If not, we return the properties that should be updated (they will be merged with the deployed task definition later)
+async function deepCompareContainerDefinitions(evaluateCfnTemplate: EvaluateCloudFormationTemplate, oldDefinition: any[], newDefinition: any[]) {
+  const result: any[] = [];
+  // schema: https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_ContainerDefinition.html
+  if (oldDefinition.length !== newDefinition.length) {
+    // one or more containers are added or removed
+    return false;
+  }
+  for (const i in oldDefinition) {
+    result.push({});
+    const prev = oldDefinition[i];
+    const next = newDefinition[i];
+
+    if (!deepCompareObject(Object.keys(prev), Object.keys(next))) {
+      // one or more fields are added or removed
+      return false;
+    }
+    for (const key of Object.keys(prev)) {
+      // compare two properties first
+      if (deepCompareObject(prev[key], next[key])) {
+        // if there is no difference, skip the field
+        continue;
+      }
+      // if there is any diff found, check if it can be evaluated without raising an error
+      try {
+        result[i][key] = await evaluateCfnTemplate.evaluateCfnExpression(next[key]);
+      } catch (e) {
+        // Give up hotswap if the diff contains unsupported expression
+        return false;
+      }
+    }
+  }
+
+  return result;
+}
+
+// return true when two objects are identical
+function deepCompareObject(lhs: object, rhs: object) {
+  if (typeof lhs !== 'object') {
+    return lhs === rhs;
+  }
+  if (typeof rhs !== 'object') {
+    return false;
+  }
+  if (Object.keys(lhs).length != Object.keys(rhs).length) {
+    return false;
+  }
+  for (const key of Object.keys(lhs)) {
+    if (!deepCompareObject((lhs as any)[key], (rhs as any)[key])) {
+      return false;
+    }
+  }
+  return true;
+}

packages/aws-cdk/test/api/hotswap/ecs-services-hotswap-deployments.test.ts

@@ -5,15 +5,18 @@ import { HotswapMode } from '../../../lib/api/hotswap/common';
 
 let hotswapMockSdkProvider: setup.HotswapMockSdkProvider;
 let mockRegisterTaskDef: jest.Mock<AWS.ECS.RegisterTaskDefinitionResponse, AWS.ECS.RegisterTaskDefinitionRequest[]>;
+let mockDescribeTaskDef: jest.Mock<AWS.ECS.DescribeTaskDefinitionResponse, AWS.ECS.DescribeTaskDefinitionRequest[]>;
 let mockUpdateService: (params: AWS.ECS.UpdateServiceRequest) => AWS.ECS.UpdateServiceResponse;
 
 beforeEach(() => {
   hotswapMockSdkProvider = setup.setupHotswapTests();
 
   mockRegisterTaskDef = jest.fn();
+  mockDescribeTaskDef = jest.fn();
   mockUpdateService = jest.fn();
   hotswapMockSdkProvider.stubEcs({
     registerTaskDefinition: mockRegisterTaskDef,
+    describeTaskDefinition: mockDescribeTaskDef,
     updateService: mockUpdateService,
   }, {
     // these are needed for the waiter API that the ECS service hotswap uses
@@ -519,6 +522,122 @@ describe.each([HotswapMode.FALL_BACK, HotswapMode.HOTSWAP_ONLY])('%p mode', (hot
     }
   });
 
+  test('should call registerTaskDefinition, describeTaskDefinition, and updateService for a difference only in the container image but with environment variables of unsupported intrinsics', async () => {
+    // GIVEN
+    setup.setCurrentCfnStackTemplate({
+      Resources: {
+        TaskDef: {
+          Type: 'AWS::ECS::TaskDefinition',
+          Properties: {
+            Family: 'my-task-def',
+            ContainerDefinitions: [
+              {
+                Image: 'image1',
+                Environment: [
+                  {
+                    Name: 'FOO',
+                    Value: { 'Fn::GetAtt': ['Bar', 'Baz'] },
+                  },
+                ],
+              },
+            ],
+          },
+        },
+        Service: {
+          Type: 'AWS::ECS::Service',
+          Properties: {
+            TaskDefinition: { Ref: 'TaskDef' },
+          },
+        },
+      },
+    });
+    setup.pushStackResourceSummaries(
+      setup.stackSummaryOf('Service', 'AWS::ECS::Service',
+        'arn:aws:ecs:region:account:service/my-cluster/my-service'),
+    );
+    mockRegisterTaskDef.mockReturnValue({
+      taskDefinition: {
+        taskDefinitionArn: 'arn:aws:ecs:region:account:task-definition/my-task-def:3',
+      },
+    });
+    mockDescribeTaskDef.mockReturnValue({
+      taskDefinition: {
+        taskDefinitionArn: 'arn:aws:ecs:region:account:task-definition/my-task-def:2',
+        family: 'my-task-def',
+        containerDefinitions: [
+          {
+            image: 'image1',
+            environment: [
+              {
+                name: 'FOO',
+                value: 'value',
+              },
+            ],
+          },
+        ],
+      },
+    });
+    const cdkStackArtifact = setup.cdkStackArtifactOf({
+      template: {
+        Resources: {
+          TaskDef: {
+            Type: 'AWS::ECS::TaskDefinition',
+            Properties: {
+              Family: 'my-task-def',
+              ContainerDefinitions: [
+                {
+                  Image: 'image2',
+                  Environment: [
+                    {
+                      Name: 'FOO',
+                      Value: { 'Fn::GetAtt': ['Bar', 'Baz'] },
+                    },
+                  ],
+                },
+              ],
+            },
+          },
+          Service: {
+            Type: 'AWS::ECS::Service',
+            Properties: {
+              TaskDefinition: { Ref: 'TaskDef' },
+            },
+          },
+        },
+      },
+    });
+
+    // WHEN
+    const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(hotswapMode, cdkStackArtifact);
+
+    // THEN
+    expect(deployStackResult).not.toBeUndefined();
+    expect(mockRegisterTaskDef).toBeCalledWith({
+      family: 'my-task-def',
+      containerDefinitions: [
+        {
+          image: 'image2',
+          environment: [
+            {
+
+              name: 'FOO',
+              value: 'value',
+            },
+          ],
+        },
+      ],
+    });
+    expect(mockUpdateService).toBeCalledWith({
+      service: 'arn:aws:ecs:region:account:service/my-cluster/my-service',
+      cluster: 'my-cluster',
+      taskDefinition: 'arn:aws:ecs:region:account:task-definition/my-task-def:3',
+      deploymentConfiguration: {
+        minimumHealthyPercent: 0,
+      },
+      forceNewDeployment: true,
+    });
+  });
+
   test('should call registerTaskDefinition with certain properties not lowercased', async () => {
     // GIVEN
     setup.setCurrentCfnStackTemplate({