Skip to content

Commit 48d7726

Browse files
authored
revert: "fix(cli): cannot hotswap ECS task definitions containing certain intrinsics" (#27358)
Closes #27343 From v2.93.0 (#26404), we hotswap ECS task definition in the following steps: 1. Calculate patch from old/new CFn template 2. Apply the patch to the task definition fetched from describeTaskDefinition API 3. register the new task definition and update services The root cause of the issue #27343 is the order of containerDefinitions array is somehow shuffled when deployed to ECS, so the patch calculated from CFn template becomes invalid. For example, when the containerDefinitions in a CFn template is like below: ```json "ContainerDefinitions": [ { "Name": "main", "Image": "imageA" }, { "Name": "sidecar", "Image": "imageB" } ], ``` the deployed task definition can sometimes become like this: ```json "ContainerDefinitions": [ { "Name": "sidecar", "Image": "imageB" }, { "Name": "main", "Image": "imageA" } ], ``` This makes a patch calculated from CFn template diff completely invalid. We can sort both CFn template and the response of describeTaskDefinition API in a deterministic order, but it is still unreliable because there can be more arrays whose order will be shuffled. [The response of describeTaskDefinition](https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_DescribeTaskDefinition.html#API_DescribeTaskDefinition_ResponseSyntax) has many array fields, and it is not documented if they may be shuffled or not. I guess we should completely abandon this approach, because it cannot be reliable enough. I have an idea for more reliable approach, but at least it should be reverted asap as it's breaking the ECS hotswap feature. I'm really sorry for me not being aware with this behavior 🙏 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent b2200a8 commit 48d7726

File tree

3 files changed

+195
-746
lines changed

3 files changed

+195
-746
lines changed
-212
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import * as cfn_diff from '@aws-cdk/cloudformation-diff';
22
import { ISDK } from '../aws-auth';
3-
import { CfnEvaluationException, EvaluateCloudFormationTemplate } from '../evaluate-cloudformation-template';
43

54
export const ICON = '✨';
65

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

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-
146138
export type PropDiffs = Record<string, cfn_diff.PropertyDifference<any>>;
147139

148140
export class ClassifiedChanges {
@@ -221,207 +213,3 @@ export function reportNonHotswappableResource(
221213
reason,
222214
}];
223215
}
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-
}

0 commit comments

Comments
 (0)