Skip to content

Commit 3d22f70

Browse files
authored
fix(cli): ECS hotswap breaks Firelens configuration (#21748)
closes #21692 Now we can specify keys that will not be transformed by `transformObjectKeys` function. ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent f001f7e commit 3d22f70

File tree

3 files changed

+151
-4
lines changed

3 files changed

+151
-4
lines changed

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

+14-3
Original file line numberDiff line numberDiff line change
@@ -61,23 +61,34 @@ export class HotswappableChangeCandidate {
6161
}
6262
}
6363

64+
type Exclude = { [key: string]: Exclude | true }
65+
6466
/**
6567
* This function transforms all keys (recursively) in the provided `val` object.
6668
*
6769
* @param val The object whose keys need to be transformed.
6870
* @param transform The function that will be applied to each key.
71+
* @param exclude The keys that will not be transformed and copied to output directly
6972
* @returns A new object with the same values as `val`, but with all keys transformed according to `transform`.
7073
*/
71-
export function transformObjectKeys(val: any, transform: (str: string) => string): any {
74+
export function transformObjectKeys(val: any, transform: (str: string) => string, exclude: Exclude = {}): any {
7275
if (val == null || typeof val !== 'object') {
7376
return val;
7477
}
7578
if (Array.isArray(val)) {
76-
return val.map((input: any) => transformObjectKeys(input, transform));
79+
// For arrays we just pass parent's exclude object directly
80+
// since it makes no sense to specify different exclude options for each array element
81+
return val.map((input: any) => transformObjectKeys(input, transform, exclude));
7782
}
7883
const ret: { [k: string]: any; } = {};
7984
for (const [k, v] of Object.entries(val)) {
80-
ret[transform(k)] = transformObjectKeys(v, transform);
85+
const childExclude = exclude[k];
86+
if (childExclude === true) {
87+
// we don't transform this object if the key is specified in exclude
88+
ret[transform(k)] = v;
89+
} else {
90+
ret[transform(k)] = transformObjectKeys(v, transform, childExclude);
91+
}
8192
}
8293
return ret;
8394
}

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

+19-1
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,25 @@ class EcsServiceHotswapOperation implements HotswapOperation {
9191
// Step 1 - update the changed TaskDefinition, creating a new TaskDefinition Revision
9292
// we need to lowercase the evaluated TaskDef from CloudFormation,
9393
// as the AWS SDK uses lowercase property names for these
94-
const lowercasedTaskDef = transformObjectKeys(this.taskDefinitionResource, lowerCaseFirstCharacter);
94+
const lowercasedTaskDef = transformObjectKeys(this.taskDefinitionResource, lowerCaseFirstCharacter, {
95+
// All the properties that take arbitrary string as keys i.e. { "string" : "string" }
96+
// https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_RegisterTaskDefinition.html#API_RegisterTaskDefinition_RequestSyntax
97+
ContainerDefinitions: {
98+
DockerLabels: true,
99+
FirelensConfiguration: {
100+
Options: true,
101+
},
102+
LogConfiguration: {
103+
Options: true,
104+
},
105+
},
106+
Volumes: {
107+
DockerVolumeConfiguration: {
108+
DriverOpts: true,
109+
Labels: true,
110+
},
111+
},
112+
});
95113
const registerTaskDefResponse = await sdk.ecs().registerTaskDefinition(lowercasedTaskDef).promise();
96114
const taskDefRevArn = registerTaskDefResponse.taskDefinition?.taskDefinitionArn;
97115

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

+118
Original file line numberDiff line numberDiff line change
@@ -362,3 +362,121 @@ test('if anything besides an ECS Service references the changed TaskDefinition,
362362
expect(deployStackResult).toBeUndefined();
363363
expect(mockRegisterTaskDef).not.toHaveBeenCalled();
364364
});
365+
366+
test('should call registerTaskDefinition with certain properties not lowercased', async () => {
367+
// GIVEN
368+
setup.setCurrentCfnStackTemplate({
369+
Resources: {
370+
TaskDef: {
371+
Type: 'AWS::ECS::TaskDefinition',
372+
Properties: {
373+
Family: 'my-task-def',
374+
ContainerDefinitions: [
375+
{ Image: 'image1' },
376+
],
377+
Volumes: [
378+
{
379+
DockerVolumeConfiguration: {
380+
DriverOpts: { Option1: 'option1' },
381+
Labels: { Label1: 'label1' },
382+
},
383+
},
384+
],
385+
},
386+
},
387+
Service: {
388+
Type: 'AWS::ECS::Service',
389+
Properties: {
390+
TaskDefinition: { Ref: 'TaskDef' },
391+
},
392+
},
393+
},
394+
});
395+
setup.pushStackResourceSummaries(
396+
setup.stackSummaryOf('Service', 'AWS::ECS::Service',
397+
'arn:aws:ecs:region:account:service/my-cluster/my-service'),
398+
);
399+
mockRegisterTaskDef.mockReturnValue({
400+
taskDefinition: {
401+
taskDefinitionArn: 'arn:aws:ecs:region:account:task-definition/my-task-def:3',
402+
},
403+
});
404+
const cdkStackArtifact = setup.cdkStackArtifactOf({
405+
template: {
406+
Resources: {
407+
TaskDef: {
408+
Type: 'AWS::ECS::TaskDefinition',
409+
Properties: {
410+
Family: 'my-task-def',
411+
ContainerDefinitions: [
412+
{
413+
Image: 'image2',
414+
DockerLabels: { Label1: 'label1' },
415+
FirelensConfiguration: {
416+
Options: { Name: 'cloudwatch' },
417+
},
418+
LogConfiguration: {
419+
Options: { Option1: 'option1' },
420+
},
421+
},
422+
],
423+
Volumes: [
424+
{
425+
DockerVolumeConfiguration: {
426+
DriverOpts: { Option1: 'option1' },
427+
Labels: { Label1: 'label1' },
428+
},
429+
},
430+
],
431+
},
432+
},
433+
Service: {
434+
Type: 'AWS::ECS::Service',
435+
Properties: {
436+
TaskDefinition: { Ref: 'TaskDef' },
437+
},
438+
},
439+
},
440+
},
441+
});
442+
443+
// WHEN
444+
const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(cdkStackArtifact);
445+
446+
// THEN
447+
expect(deployStackResult).not.toBeUndefined();
448+
expect(mockRegisterTaskDef).toBeCalledWith({
449+
family: 'my-task-def',
450+
containerDefinitions: [
451+
{
452+
image: 'image2',
453+
dockerLabels: { Label1: 'label1' },
454+
firelensConfiguration: {
455+
options: {
456+
Name: 'cloudwatch',
457+
},
458+
},
459+
logConfiguration: {
460+
options: { Option1: 'option1' },
461+
},
462+
},
463+
],
464+
volumes: [
465+
{
466+
dockerVolumeConfiguration: {
467+
driverOpts: { Option1: 'option1' },
468+
labels: { Label1: 'label1' },
469+
},
470+
},
471+
],
472+
});
473+
expect(mockUpdateService).toBeCalledWith({
474+
service: 'arn:aws:ecs:region:account:service/my-cluster/my-service',
475+
cluster: 'my-cluster',
476+
taskDefinition: 'arn:aws:ecs:region:account:task-definition/my-task-def:3',
477+
deploymentConfiguration: {
478+
minimumHealthyPercent: 0,
479+
},
480+
forceNewDeployment: true,
481+
});
482+
});

0 commit comments

Comments
 (0)