From f2422449c9a8e7c07ecec98b7861080b25a43c04 Mon Sep 17 00:00:00 2001 From: tmokmss Date: Thu, 25 Aug 2022 05:05:53 +0000 Subject: [PATCH 1/2] allow to exclude specified keys from lowercase transformation --- packages/aws-cdk/lib/api/hotswap/common.ts | 17 ++- .../aws-cdk/lib/api/hotswap/ecs-services.ts | 20 ++- .../ecs-services-hotswap-deployments.test.ts | 118 ++++++++++++++++++ 3 files changed, 151 insertions(+), 4 deletions(-) diff --git a/packages/aws-cdk/lib/api/hotswap/common.ts b/packages/aws-cdk/lib/api/hotswap/common.ts index 03f78df4ddac6..d5e0914494565 100644 --- a/packages/aws-cdk/lib/api/hotswap/common.ts +++ b/packages/aws-cdk/lib/api/hotswap/common.ts @@ -61,23 +61,34 @@ export class HotswappableChangeCandidate { } } +type Exclude = { [key: string]: Exclude | true } + /** * This function transforms all keys (recursively) in the provided `val` object. * * @param val The object whose keys need to be transformed. * @param transform The function that will be applied to each key. + * @param exclude The keys that will not be transformed and copied to output directly * @returns A new object with the same values as `val`, but with all keys transformed according to `transform`. */ -export function transformObjectKeys(val: any, transform: (str: string) => string): any { +export function transformObjectKeys(val: any, transform: (str: string) => string, exclude: Exclude = {}): any { if (val == null || typeof val !== 'object') { return val; } if (Array.isArray(val)) { - return val.map((input: any) => transformObjectKeys(input, transform)); + // For arrays we just pass parent's exclude object directly + // since it makes no sense to specify different exclude options for each array element + return val.map((input: any) => transformObjectKeys(input, transform, exclude)); } const ret: { [k: string]: any; } = {}; for (const [k, v] of Object.entries(val)) { - ret[transform(k)] = transformObjectKeys(v, transform); + const childExclude = exclude[k]; + if (childExclude === true) { + // we don't transform this object if the key is specified in exclude + ret[transform(k)] = v; + continue; + } + ret[transform(k)] = transformObjectKeys(v, transform, childExclude); } return ret; } diff --git a/packages/aws-cdk/lib/api/hotswap/ecs-services.ts b/packages/aws-cdk/lib/api/hotswap/ecs-services.ts index 1794457d86c9f..baa7383fd3d62 100644 --- a/packages/aws-cdk/lib/api/hotswap/ecs-services.ts +++ b/packages/aws-cdk/lib/api/hotswap/ecs-services.ts @@ -91,7 +91,25 @@ class EcsServiceHotswapOperation implements HotswapOperation { // Step 1 - update the changed TaskDefinition, creating a new TaskDefinition Revision // we need to lowercase the evaluated TaskDef from CloudFormation, // as the AWS SDK uses lowercase property names for these - const lowercasedTaskDef = transformObjectKeys(this.taskDefinitionResource, lowerCaseFirstCharacter); + const lowercasedTaskDef = transformObjectKeys(this.taskDefinitionResource, lowerCaseFirstCharacter, { + // All 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, + FirelensConfiguration: { + Options: true, + }, + LogConfiguration: { + Options: true, + }, + }, + Volumes: { + DockerVolumeConfiguration: { + DriverOpts: true, + Labels: true, + }, + }, + }); const registerTaskDefResponse = await sdk.ecs().registerTaskDefinition(lowercasedTaskDef).promise(); const taskDefRevArn = registerTaskDefResponse.taskDefinition?.taskDefinitionArn; diff --git a/packages/aws-cdk/test/api/hotswap/ecs-services-hotswap-deployments.test.ts b/packages/aws-cdk/test/api/hotswap/ecs-services-hotswap-deployments.test.ts index 717f68ecd5b29..dda972f5a90b9 100644 --- a/packages/aws-cdk/test/api/hotswap/ecs-services-hotswap-deployments.test.ts +++ b/packages/aws-cdk/test/api/hotswap/ecs-services-hotswap-deployments.test.ts @@ -362,3 +362,121 @@ test('if anything besides an ECS Service references the changed TaskDefinition, expect(deployStackResult).toBeUndefined(); expect(mockRegisterTaskDef).not.toHaveBeenCalled(); }); + +test('should call registerTaskDefinition with certain properties not lowercased', async () => { + // GIVEN + setup.setCurrentCfnStackTemplate({ + Resources: { + TaskDef: { + Type: 'AWS::ECS::TaskDefinition', + Properties: { + Family: 'my-task-def', + ContainerDefinitions: [ + { Image: 'image1' }, + ], + Volumes: [ + { + DockerVolumeConfiguration: { + DriverOpts: { Option1: 'option1' }, + Labels: { Label1: 'label1' }, + }, + }, + ], + }, + }, + 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', + }, + }); + const cdkStackArtifact = setup.cdkStackArtifactOf({ + template: { + Resources: { + TaskDef: { + Type: 'AWS::ECS::TaskDefinition', + Properties: { + Family: 'my-task-def', + ContainerDefinitions: [ + { + Image: 'image2', + DockerLabels: { Label1: 'label1' }, + FirelensConfiguration: { + Options: { Name: 'cloudwatch' }, + }, + LogConfiguration: { + Options: { Option1: 'option1' }, + }, + }, + ], + Volumes: [ + { + DockerVolumeConfiguration: { + DriverOpts: { Option1: 'option1' }, + Labels: { Label1: 'label1' }, + }, + }, + ], + }, + }, + Service: { + Type: 'AWS::ECS::Service', + Properties: { + TaskDefinition: { Ref: 'TaskDef' }, + }, + }, + }, + }, + }); + + // WHEN + const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(cdkStackArtifact); + + // THEN + expect(deployStackResult).not.toBeUndefined(); + expect(mockRegisterTaskDef).toBeCalledWith({ + family: 'my-task-def', + containerDefinitions: [ + { + image: 'image2', + dockerLabels: { Label1: 'label1' }, + firelensConfiguration: { + options: { + Name: 'cloudwatch', + }, + }, + logConfiguration: { + options: { Option1: 'option1' }, + }, + }, + ], + volumes: [ + { + dockerVolumeConfiguration: { + driverOpts: { Option1: 'option1' }, + labels: { Label1: 'label1' }, + }, + }, + ], + }); + 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, + }); +}); From 96b524c012aa46805fdce4168d472a2230a9ba6d Mon Sep 17 00:00:00 2001 From: tmokmss Date: Thu, 25 Aug 2022 05:24:58 +0000 Subject: [PATCH 2/2] fix --- packages/aws-cdk/lib/api/hotswap/common.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk/lib/api/hotswap/common.ts b/packages/aws-cdk/lib/api/hotswap/common.ts index d5e0914494565..8f4c17ccc21b2 100644 --- a/packages/aws-cdk/lib/api/hotswap/common.ts +++ b/packages/aws-cdk/lib/api/hotswap/common.ts @@ -86,9 +86,9 @@ export function transformObjectKeys(val: any, transform: (str: string) => string if (childExclude === true) { // we don't transform this object if the key is specified in exclude ret[transform(k)] = v; - continue; + } else { + ret[transform(k)] = transformObjectKeys(v, transform, childExclude); } - ret[transform(k)] = transformObjectKeys(v, transform, childExclude); } return ret; }