Skip to content

Commit

Permalink
revert: "fix(cli): cannot hotswap ECS task definitions containing cer…
Browse files Browse the repository at this point in the history 
…tain 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*
  • Loading branch information
@tmokmss
tmokmss committed Oct 12, 2023
1 parent b2200a8 commit 48d7726
Show file tree
Hide file tree
Showing 3 changed files with 195 additions and 746 deletions.
212 changes: 0 additions & 212 deletions packages/aws-cdk/lib/api/hotswap/common.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import * as cfn_diff from '@aws-cdk/cloudformation-diff';
import { ISDK } from '../aws-auth';
import { CfnEvaluationException, EvaluateCloudFormationTemplate } from '../evaluate-cloudformation-template';

export const ICON = '✨';

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

/**
* This function upper cases the first character of the string provided.
*/
export function upperCaseFirstCharacter(str: string): string {
return str.length > 0 ? `${str[0].toUpperCase()}${str.slice(1)}` : str;
}

export type PropDiffs = Record<string, cfn_diff.PropertyDifference<any>>;

export class ClassifiedChanges {
Expand Down Expand Up @@ -221,207 +213,3 @@ export function reportNonHotswappableResource(
reason,
}];
}

type ChangedProps = {
/**
* Array to specify the property from an object.
* e.g. Given this object `{ 'a': { 'b': 1 } }`, the key array for the element `1` will be `['a', 'b']`
*/
key: string[];

/**
* Whether the property is added (also modified) or removed.
*/
type: 'removed' | 'added';

/**
* evaluated value of the property.
* undefined if type == 'removed'
*/
value?: any
};

function detectChangedProps(next: any, prev: any): ChangedProps[] {
const changedProps: ChangedProps[] = [];
changedProps.push(...detectAdditions(next, prev));
changedProps.push(...detectRemovals(next, prev));
return changedProps;
}

function detectAdditions(next: any, prev: any, keys: string[] = []): ChangedProps[] {
// Compare each value of two objects, detect additions (added or modified properties)
// If we encounter CFn intrinsic (key.startsWith('Fn::') || key == 'Ref'), stop recursion

if (typeof next !== 'object') {
if (next !== prev) {
// there is an addition or change to the property
return [{ key: new Array(...keys), type: 'added' }];
} else {
return [];
}
}

if (typeof prev !== 'object') {
// there is an addition or change to the property
return [{ key: new Array(...keys), type: 'added' }];
}

// If the next is a CFn intrinsic, don't recurse further.
const childKeys = Object.keys(next);
if (childKeys.length === 1 && (childKeys[0].startsWith('Fn::') || childKeys[0] === 'Ref')) {
if (!deepCompareObject(prev, next)) {
// there is an addition or change to the property
return [{ key: new Array(...keys), type: 'added' }];
} else {
return [];
}
}

const changedProps: ChangedProps[] = [];
// compare children
for (const key of childKeys) {
keys.push(key);
changedProps.push(...detectAdditions((next as any)[key], (prev as any)[key], keys));
keys.pop();
}
return changedProps;
}

function detectRemovals(next: any, prev: any, keys: string[] = []): ChangedProps[] {
// Compare each value of two objects, detect removed properties
// To do this, find any keys that exist only in prev object.
// If we encounter CFn intrinsic (key.startsWith('Fn::') || key == 'Ref'), stop recursion
if (next === undefined) {
return [{ key: new Array(...keys), type: 'removed' }];
}

if (typeof prev !== 'object' || typeof next !== 'object') {
// either prev or next is not an object nor undefined, then the property is not removed
return [];
}

// If the prev is a CFn intrinsic, don't recurse further.
const childKeys = Object.keys(prev);
if (childKeys.length === 1 && (childKeys[0].startsWith('Fn::') || childKeys[0] === 'Ref')) {
// next is not undefined here, so it is at least not removed
return [];
}

const changedProps: ChangedProps[] = [];
// compare children
for (const key of childKeys) {
keys.push(key);
changedProps.push(...detectRemovals((next as any)[key], (prev as any)[key], keys));
keys.pop();
}
return changedProps;
}

/**
* return true when two objects are identical
*/
function deepCompareObject(lhs: any, rhs: any): boolean {
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;
}

interface EvaluatedPropertyUpdates {
readonly updates: ChangedProps[];
readonly unevaluatableUpdates: ChangedProps[];
}

/**
* Diff each property of the changes, and check if each diff can be actually hotswapped (i.e. evaluated by EvaluateCloudFormationTemplate.)
* If any diff cannot be evaluated, they are reported by unevaluatableUpdates.
* This method works on more granular level than HotswappableChangeCandidate.propertyUpdates.
*
* If propertiesToInclude is specified, we only compare properties that are under keys in the argument.
*/
export async function evaluatableProperties(
evaluate: EvaluateCloudFormationTemplate,
change: HotswappableChangeCandidate,
propertiesToInclude?: string[],
): Promise<EvaluatedPropertyUpdates> {
const prev = change.oldValue.Properties!;
const next = change.newValue.Properties!;
const changedProps = detectChangedProps(next, prev).filter(
prop => propertiesToInclude?.includes(prop.key[0]) ?? true,
);
const evaluatedUpdates = await Promise.all(
changedProps
.filter((prop) => prop.type === 'added')
.map(async (prop) => {
const val = getPropertyFromKey(prop.key, next);
try {
const evaluated = await evaluate.evaluateCfnExpression(val);
return {
...prop,
value: evaluated,
};
} catch (e) {
if (e instanceof CfnEvaluationException) {
return prop;
}
throw e;
}
}));
const unevaluatableUpdates = evaluatedUpdates.filter(update => update.value === undefined);
evaluatedUpdates.push(...changedProps.filter(prop => prop.type == 'removed'));

return {
updates: evaluatedUpdates,
unevaluatableUpdates,
};
}

function getPropertyFromKey(key: string[], obj: object) {
return key.reduce((prev, cur) => (prev as any)?.[cur], obj);
}

function overwriteProperty(key: string[], newValue: any, target: object) {
for (const next of key.slice(0, -1)) {
if (next in target) {
target = (target as any)[next];
} else if (Array.isArray(target)) {
// When an element is added to an array, we need explicitly allocate the new element.
target = {};
(target as any)[next] = {};
} else {
// This is an unexpected condition. Perhaps the deployed task definition is modified outside of CFn.
return false;
}
}
if (newValue === undefined) {
delete (target as any)[key[key.length - 1]];
} else {
(target as any)[key[key.length - 1]] = newValue;
}
return true;
}

/**
* Take the old template and property updates, and synthesize a new template.
*/
export function applyPropertyUpdates(patches: ChangedProps[], target: any) {
target = JSON.parse(JSON.stringify(target));
for (const patch of patches) {
const res = overwriteProperty(patch.key, patch.value, target);
if (!res) {
throw new Error(`failed to applying patch to ${patch.key.join('.')}. Please try deploying without hotswap first.`);
}
}
return target;
}
Loading

0 comments on commit 48d7726

Please sign in to comment.