Skip to content

Commit 6d315b8

Browse files
tmokmsstmokmss
authored
fix(cli): cannot hotswap ECS task definitions containing certain intrinsics (#26404)
## 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](https://github.com/aws/aws-cdk/blob/5ccc56975c323ea19fd0917def51184e13f440d9/packages/aws-cdk/lib/api/evaluate-cloudformation-template.ts#L352)) 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](https://github.com/aws/aws-cdk/blob/5ccc56975c323ea19fd0917def51184e13f440d9/packages/aws-cdk/lib/api/evaluate-cloudformation-template.ts#L134) 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: ```json { "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*
1 parent 0b8f31d commit 6d315b8

File tree

3 files changed

+734
-193
lines changed

3 files changed

+734
-193
lines changed

‎packages/aws-cdk/lib/api/hotswap/common.ts

Lines changed: 212 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import * as cfn_diff from '@aws-cdk/cloudformation-diff';
22
import { ISDK } from '../aws-auth';
3+
import { CfnEvaluationException, EvaluateCloudFormationTemplate } from '../evaluate-cloudformation-template';
34

45
export const ICON = '✨';
56

@@ -135,6 +136,13 @@ export function lowerCaseFirstCharacter(str: string): string {
135136
return str.length > 0 ? `${str[0].toLowerCase()}${str.slice(1)}` : str;
136137
}
137138

139+
/**
140+
* This function upper cases the first character of the string provided.
141+
*/
142+
export function upperCaseFirstCharacter(str: string): string {
143+
return str.length > 0 ? `${str[0].toUpperCase()}${str.slice(1)}` : str;
144+
}
145+
138146
export type PropDiffs = Record<string, cfn_diff.PropertyDifference<any>>;
139147

140148
export class ClassifiedChanges {
@@ -213,3 +221,207 @@ export function reportNonHotswappableResource(
213221
reason,
214222
}];
215223
}
224+
225+
type ChangedProps = {
226+
/**
227+
* Array to specify the property from an object.
228+
* e.g. Given this object `{ 'a': { 'b': 1 } }`, the key array for the element `1` will be `['a', 'b']`
229+
*/
230+
key: string[];
231+
232+
/**
233+
* Whether the property is added (also modified) or removed.
234+
*/
235+
type: 'removed' | 'added';
236+
237+
/**
238+
* evaluated value of the property.
239+
* undefined if type == 'removed'
240+
*/
241+
value?: any
242+
};
243+
244+
function detectChangedProps(next: any, prev: any): ChangedProps[] {
245+
const changedProps: ChangedProps[] = [];
246+
changedProps.push(...detectAdditions(next, prev));
247+
changedProps.push(...detectRemovals(next, prev));
248+
return changedProps;
249+
}
250+
251+
function detectAdditions(next: any, prev: any, keys: string[] = []): ChangedProps[] {
252+
// Compare each value of two objects, detect additions (added or modified properties)
253+
// If we encounter CFn intrinsic (key.startsWith('Fn::') || key == 'Ref'), stop recursion
254+
255+
if (typeof next !== 'object') {
256+
if (next !== prev) {
257+
// there is an addition or change to the property
258+
return [{ key: new Array(...keys), type: 'added' }];
259+
} else {
260+
return [];
261+
}
262+
}
263+
264+
if (typeof prev !== 'object') {
265+
// there is an addition or change to the property
266+
return [{ key: new Array(...keys), type: 'added' }];
267+
}
268+
269+
// If the next is a CFn intrinsic, don't recurse further.
270+
const childKeys = Object.keys(next);
271+
if (childKeys.length === 1 && (childKeys[0].startsWith('Fn::') || childKeys[0] === 'Ref')) {
272+
if (!deepCompareObject(prev, next)) {
273+
// there is an addition or change to the property
274+
return [{ key: new Array(...keys), type: 'added' }];
275+
} else {
276+
return [];
277+
}
278+
}
279+
280+
const changedProps: ChangedProps[] = [];
281+
// compare children
282+
for (const key of childKeys) {
283+
keys.push(key);
284+
changedProps.push(...detectAdditions((next as any)[key], (prev as any)[key], keys));
285+
keys.pop();
286+
}
287+
return changedProps;
288+
}
289+
290+
function detectRemovals(next: any, prev: any, keys: string[] = []): ChangedProps[] {
291+
// Compare each value of two objects, detect removed properties
292+
// To do this, find any keys that exist only in prev object.
293+
// If we encounter CFn intrinsic (key.startsWith('Fn::') || key == 'Ref'), stop recursion
294+
if (next === undefined) {
295+
return [{ key: new Array(...keys), type: 'removed' }];
296+
}
297+
298+
if (typeof prev !== 'object' || typeof next !== 'object') {
299+
// either prev or next is not an object nor undefined, then the property is not removed
300+
return [];
301+
}
302+
303+
// If the prev is a CFn intrinsic, don't recurse further.
304+
const childKeys = Object.keys(prev);
305+
if (childKeys.length === 1 && (childKeys[0].startsWith('Fn::') || childKeys[0] === 'Ref')) {
306+
// next is not undefined here, so it is at least not removed
307+
return [];
308+
}
309+
310+
const changedProps: ChangedProps[] = [];
311+
// compare children
312+
for (const key of childKeys) {
313+
keys.push(key);
314+
changedProps.push(...detectRemovals((next as any)[key], (prev as any)[key], keys));
315+
keys.pop();
316+
}
317+
return changedProps;
318+
}
319+
320+
/**
321+
* return true when two objects are identical
322+
*/
323+
function deepCompareObject(lhs: any, rhs: any): boolean {
324+
if (typeof lhs !== 'object') {
325+
return lhs === rhs;
326+
}
327+
if (typeof rhs !== 'object') {
328+
return false;
329+
}
330+
if (Object.keys(lhs).length != Object.keys(rhs).length) {
331+
return false;
332+
}
333+
for (const key of Object.keys(lhs)) {
334+
if (!deepCompareObject((lhs as any)[key], (rhs as any)[key])) {
335+
return false;
336+
}
337+
}
338+
return true;
339+
}
340+
341+
interface EvaluatedPropertyUpdates {
342+
readonly updates: ChangedProps[];
343+
readonly unevaluatableUpdates: ChangedProps[];
344+
}
345+
346+
/**
347+
* Diff each property of the changes, and check if each diff can be actually hotswapped (i.e. evaluated by EvaluateCloudFormationTemplate.)
348+
* If any diff cannot be evaluated, they are reported by unevaluatableUpdates.
349+
* This method works on more granular level than HotswappableChangeCandidate.propertyUpdates.
350+
*
351+
* If propertiesToInclude is specified, we only compare properties that are under keys in the argument.
352+
*/
353+
export async function evaluatableProperties(
354+
evaluate: EvaluateCloudFormationTemplate,
355+
change: HotswappableChangeCandidate,
356+
propertiesToInclude?: string[],
357+
): Promise<EvaluatedPropertyUpdates> {
358+
const prev = change.oldValue.Properties!;
359+
const next = change.newValue.Properties!;
360+
const changedProps = detectChangedProps(next, prev).filter(
361+
prop => propertiesToInclude?.includes(prop.key[0]) ?? true,
362+
);
363+
const evaluatedUpdates = await Promise.all(
364+
changedProps
365+
.filter((prop) => prop.type === 'added')
366+
.map(async (prop) => {
367+
const val = getPropertyFromKey(prop.key, next);
368+
try {
369+
const evaluated = await evaluate.evaluateCfnExpression(val);
370+
return {
371+
...prop,
372+
value: evaluated,
373+
};
374+
} catch (e) {
375+
if (e instanceof CfnEvaluationException) {
376+
return prop;
377+
}
378+
throw e;
379+
}
380+
}));
381+
const unevaluatableUpdates = evaluatedUpdates.filter(update => update.value === undefined);
382+
evaluatedUpdates.push(...changedProps.filter(prop => prop.type == 'removed'));
383+
384+
return {
385+
updates: evaluatedUpdates,
386+
unevaluatableUpdates,
387+
};
388+
}
389+
390+
function getPropertyFromKey(key: string[], obj: object) {
391+
return key.reduce((prev, cur) => (prev as any)?.[cur], obj);
392+
}
393+
394+
function overwriteProperty(key: string[], newValue: any, target: object) {
395+
for (const next of key.slice(0, -1)) {
396+
if (next in target) {
397+
target = (target as any)[next];
398+
} else if (Array.isArray(target)) {
399+
// When an element is added to an array, we need explicitly allocate the new element.
400+
target = {};
401+
(target as any)[next] = {};
402+
} else {
403+
// This is an unexpected condition. Perhaps the deployed task definition is modified outside of CFn.
404+
return false;
405+
}
406+
}
407+
if (newValue === undefined) {
408+
delete (target as any)[key[key.length - 1]];
409+
} else {
410+
(target as any)[key[key.length - 1]] = newValue;
411+
}
412+
return true;
413+
}
414+
415+
/**
416+
* Take the old template and property updates, and synthesize a new template.
417+
*/
418+
export function applyPropertyUpdates(patches: ChangedProps[], target: any) {
419+
target = JSON.parse(JSON.stringify(target));
420+
for (const patch of patches) {
421+
const res = overwriteProperty(patch.key, patch.value, target);
422+
if (!res) {
423+
throw new Error(`failed to applying patch to ${patch.key.join('.')}. Please try deploying without hotswap first.`);
424+
}
425+
}
426+
return target;
427+
}

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

Lines changed: 81 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import * as AWS from 'aws-sdk';
2-
import { ChangeHotswapResult, classifyChanges, HotswappableChangeCandidate, lowerCaseFirstCharacter, reportNonHotswappableChange, transformObjectKeys } from './common';
2+
import { ChangeHotswapResult, classifyChanges, HotswappableChangeCandidate, lowerCaseFirstCharacter, reportNonHotswappableChange, transformObjectKeys, upperCaseFirstCharacter, applyPropertyUpdates, evaluatableProperties } from './common';
33
import { ISDK } from '../aws-auth';
44
import { EvaluateCloudFormationTemplate } from '../evaluate-cloudformation-template';
55

@@ -16,7 +16,8 @@ export async function isHotswappableEcsServiceChange(
1616
// We only allow a change in the ContainerDefinitions of the TaskDefinition for now -
1717
// it contains the image and environment variables, so seems like a safe bet for now.
1818
// We might revisit this decision in the future though!
19-
const classifiedChanges = classifyChanges(change, ['ContainerDefinitions']);
19+
const propertiesToHotswap = ['ContainerDefinitions'];
20+
const classifiedChanges = classifyChanges(change, propertiesToHotswap);
2021
classifiedChanges.reportNonHotswappablePropertyChanges(ret);
2122

2223
// find all ECS Services that reference the TaskDefinition that changed
@@ -33,7 +34,8 @@ export async function isHotswappableEcsServiceChange(
3334
// if there are no resources referencing the TaskDefinition,
3435
// hotswap is not possible in FALL_BACK mode
3536
reportNonHotswappableChange(ret, change, undefined, 'No ECS services reference the changed task definition', false);
36-
} if (resourcesReferencingTaskDef.length > ecsServicesReferencingTaskDef.length) {
37+
}
38+
if (resourcesReferencingTaskDef.length > ecsServicesReferencingTaskDef.length) {
3739
// if something besides an ECS Service is referencing the TaskDefinition,
3840
// hotswap is not possible in FALL_BACK mode
3941
const nonEcsServiceTaskDefRefs = resourcesReferencingTaskDef.filter(r => r.Type !== 'AWS::ECS::Service');
@@ -44,26 +46,76 @@ export async function isHotswappableEcsServiceChange(
4446

4547
const namesOfHotswappableChanges = Object.keys(classifiedChanges.hotswappableProps);
4648
if (namesOfHotswappableChanges.length > 0) {
47-
const taskDefinitionResource = await prepareTaskDefinitionChange(evaluateCfnTemplate, logicalId, change);
49+
const familyName = await getFamilyName(evaluateCfnTemplate, logicalId, change);
50+
if (familyName === undefined) {
51+
reportNonHotswappableChange(ret, change, undefined, 'Failed to determine family name of the task definition', false);
52+
return ret;
53+
}
54+
const oldTaskDefinitionArn = await evaluateCfnTemplate.findPhysicalNameFor(logicalId);
55+
if (oldTaskDefinitionArn === undefined) {
56+
reportNonHotswappableChange(ret, change, undefined, 'Failed to determine ARN of the task definition', false);
57+
return ret;
58+
}
59+
60+
const changes = await evaluatableProperties(evaluateCfnTemplate, change, propertiesToHotswap);
61+
if (changes.unevaluatableUpdates.length > 0) {
62+
reportNonHotswappableChange(ret, change, undefined, `Found changes that cannot be evaluated locally in the task definition - ${
63+
changes.unevaluatableUpdates.map(p => p.key.join('.')).join(', ')
64+
}`, false);
65+
return ret;
66+
}
67+
4868
ret.push({
4969
hotswappable: true,
5070
resourceType: change.newValue.Type,
5171
propsChanged: namesOfHotswappableChanges,
5272
service: 'ecs-service',
5373
resourceNames: [
54-
`ECS Task Definition '${await taskDefinitionResource.Family}'`,
74+
`ECS Task Definition '${familyName}'`,
5575
...ecsServicesReferencingTaskDef.map(ecsService => `ECS Service '${ecsService.serviceArn.split('/')[2]}'`),
5676
],
5777
apply: async (sdk: ISDK) => {
5878
// Step 1 - update the changed TaskDefinition, creating a new TaskDefinition Revision
5979
// we need to lowercase the evaluated TaskDef from CloudFormation,
6080
// as the AWS SDK uses lowercase property names for these
6181

62-
// The SDK requires more properties here than its worth doing explicit typing for
63-
// instead, just use all the old values in the diff to fill them in implicitly
64-
const lowercasedTaskDef = transformObjectKeys(taskDefinitionResource, lowerCaseFirstCharacter, {
65-
// All the properties that take arbitrary string as keys i.e. { "string" : "string" }
66-
// https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_RegisterTaskDefinition.html#API_RegisterTaskDefinition_RequestSyntax
82+
// get the task definition of the family and revision corresponding to the old CFn template
83+
const target = await sdk
84+
.ecs()
85+
.describeTaskDefinition({
86+
taskDefinition: oldTaskDefinitionArn,
87+
include: ['TAGS'],
88+
})
89+
.promise();
90+
if (target.taskDefinition === undefined) {
91+
throw new Error(`Could not find a task definition: ${oldTaskDefinitionArn}. Try deploying without hotswap first.`);
92+
}
93+
94+
// The describeTaskDefinition response contains several keys that must not exist in a registerTaskDefinition request.
95+
// We remove these keys here, comparing these two structs:
96+
// https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_RegisterTaskDefinition.html#API_RegisterTaskDefinition_RequestSyntax
97+
// https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_DescribeTaskDefinition.html#API_DescribeTaskDefinition_ResponseSyntax
98+
[
99+
'compatibilities',
100+
'taskDefinitionArn',
101+
'revision',
102+
'status',
103+
'requiresAttributes',
104+
'compatibilities',
105+
'registeredAt',
106+
'registeredBy',
107+
].forEach(key=> delete (target.taskDefinition as any)[key]);
108+
109+
// the tags field is in a different location in describeTaskDefinition response,
110+
// moving it as intended for registerTaskDefinition request.
111+
if (target.tags !== undefined && target.tags.length > 0) {
112+
(target.taskDefinition as any).tags = target.tags;
113+
delete target.tags;
114+
}
115+
116+
// Don't transform the properties that take arbitrary string as keys i.e. { "string" : "string" }
117+
// https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_RegisterTaskDefinition.html#API_RegisterTaskDefinition_RequestSyntax
118+
const excludeFromTransform = {
67119
ContainerDefinitions: {
68120
DockerLabels: true,
69121
FirelensConfiguration: {
@@ -79,7 +131,14 @@ export async function isHotswappableEcsServiceChange(
79131
Labels: true,
80132
},
81133
},
82-
});
134+
} as const;
135+
// We first uppercase the task definition to properly merge it with the one from CloudFormation template.
136+
const upperCasedTaskDef = transformObjectKeys(target.taskDefinition, upperCaseFirstCharacter, excludeFromTransform);
137+
// merge evaluatable diff from CloudFormation template.
138+
const updatedTaskDef = applyPropertyUpdates(changes.updates, upperCasedTaskDef);
139+
// lowercase the merged task definition to use it in AWS SDK.
140+
const lowercasedTaskDef = transformObjectKeys(updatedTaskDef, lowerCaseFirstCharacter, excludeFromTransform);
141+
83142
const registerTaskDefResponse = await sdk.ecs().registerTaskDefinition(lowercasedTaskDef).promise();
84143
const taskDefRevArn = registerTaskDefResponse.taskDefinition?.taskDefinitionArn;
85144

@@ -171,14 +230,15 @@ interface EcsService {
171230
readonly serviceArn: string;
172231
}
173232

174-
async function prepareTaskDefinitionChange(
175-
evaluateCfnTemplate: EvaluateCloudFormationTemplate, logicalId: string, change: HotswappableChangeCandidate,
176-
) {
233+
async function getFamilyName(
234+
evaluateCfnTemplate: EvaluateCloudFormationTemplate,
235+
logicalId: string,
236+
change: HotswappableChangeCandidate) {
177237
const taskDefinitionResource: { [name: string]: any } = {
178238
...change.oldValue.Properties,
179239
ContainerDefinitions: change.newValue.Properties?.ContainerDefinitions,
180240
};
181-
// first, let's get the name of the family
241+
// first, let's get the name of the family
182242
const familyNameOrArn = await evaluateCfnTemplate.establishResourcePhysicalName(logicalId, taskDefinitionResource?.Family);
183243
if (!familyNameOrArn) {
184244
// if the Family property has not been provided, and we can't find it in the current Stack,
@@ -189,17 +249,12 @@ async function prepareTaskDefinitionChange(
189249
// remove it if needed
190250
const familyNameOrArnParts = familyNameOrArn.split(':');
191251
const family = familyNameOrArnParts.length > 1
192-
// familyNameOrArn is actually an ARN, of the format 'arn:aws:ecs:region:account:task-definition/<family-name>:<revision-nr>'
193-
// so, take the 6th element, at index 5, and split it on '/'
252+
// familyNameOrArn is actually an ARN, of the format 'arn:aws:ecs:region:account:task-definition/<family-name>:<revision-nr>'
253+
// so, take the 6th element, at index 5, and split it on '/'
194254
? familyNameOrArnParts[5].split('/')[1]
195-
// otherwise, familyNameOrArn is just the simple name evaluated from the CloudFormation template
255+
// otherwise, familyNameOrArn is just the simple name evaluated from the CloudFormation template
196256
: familyNameOrArn;
197-
// then, let's evaluate the body of the remainder of the TaskDef (without the Family property)
198-
return {
199-
...await evaluateCfnTemplate.evaluateCfnExpression({
200-
...(taskDefinitionResource ?? {}),
201-
Family: undefined,
202-
}),
203-
Family: family,
204-
};
257+
// then, let's evaluate the body of the remainder of the TaskDef (without the Family property)
258+
259+
return family;
205260
}

0 commit comments

Comments
 (0)