Skip to content

Commit

Permalink
fix(cli): ECS hotswap breaks Firelens configuration (#21748)
Browse files Browse the repository at this point in the history
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*
  • Loading branch information
tmokmss authored Aug 25, 2022
1 parent f001f7e commit 3d22f70
Show file tree
Hide file tree
Showing 3 changed files with 151 additions and 4 deletions.
17 changes: 14 additions & 3 deletions packages/aws-cdk/lib/api/hotswap/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
} else {
ret[transform(k)] = transformObjectKeys(v, transform, childExclude);
}
}
return ret;
}
Expand Down
20 changes: 19 additions & 1 deletion packages/aws-cdk/lib/api/hotswap/ecs-services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
});

0 comments on commit 3d22f70

Please sign in to comment.