Skip to content

Commit

Permalink
fix(cli): ecs hotswap fails on log configuration enabled (aws#26876)
Browse files Browse the repository at this point in the history
## Problem
ECS hotswap fails on a task definition containing log configuration like the following.

```ts
const taskDefinition = new ecs.FargateTaskDefinition(stack, 'Task', {});

taskDefinition.addContainer('EcsApp', {
  image: ecs.ContainerImage.fromRegistry('nginx:stable'),
  logging: ecs.LogDriver.awsLogs({ streamPrefix: 'log' }),
});
```

## Root cause
When we transform object keys in a task definition, we pass `excludeFromTransform` to avoid from transforming keys with arbitrary string, such as [`logDriver.options`](https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_LogConfiguration.html#ECS-Type-LogConfiguration-options).

However, it was not working when we transform keys with upperCaseFirstCharacter, because the keys in `excludeFromTransform` was uppercased, where the source object was lowercased.

```
// source task definition
{
  "logConfiguration": {
      "logDriver": "awslogs",
      "options": {
          "awslogs-group": "my-test-stack-TaskEcsAppLogGroupD5C9C1DD-BPB6zgX4S0wU",
          "awslogs-region": "ap-northeast-1",
      },
  },
}

// excludeFromTransform 
{ 
  LogConfiguration: {
      Options: true,
    },
  },
}

// where it should be
{ 
  logConfiguration: {
      options: true,
    },
  },
}
```

This misconfiguration resulted in the output task definition uppercased in an unexpected way:

```json
{
  "logConfiguration": {
      "logDriver": "awslogs",
      "options": {
          "Awslogs-group": "my-test-stack-TaskEcsAppLogGroupD5C9C1DD-BPB6zgX4S0wU",
          "Awslogs-region": "ap-northeast-1",
      },
  },
}

```

The problem was not detected by unit tests because they only contained cases with uppercase keys in a source task definition.

## Fix
Use lowercased `excludeFromTransform` when we use it with `upperCaseFirstCharacter`, also adding a test case with lowercased keys in a source task definition.


Closes aws#26871.

----

*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 committed Aug 25, 2023
1 parent 702d9d5 commit 6cffca0
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 5 deletions.
3 changes: 2 additions & 1 deletion packages/aws-cdk/lib/api/hotswap/ecs-services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,9 @@ export async function isHotswappableEcsServiceChange(
},
},
} as const;
const excludeFromTransformLowercased = transformObjectKeys(excludeFromTransform, lowerCaseFirstCharacter);
// We first uppercase the task definition to properly merge it with the one from CloudFormation template.
const upperCasedTaskDef = transformObjectKeys(target.taskDefinition, upperCaseFirstCharacter, excludeFromTransform);
const upperCasedTaskDef = transformObjectKeys(target.taskDefinition, upperCaseFirstCharacter, excludeFromTransformLowercased);
// merge evaluatable diff from CloudFormation template.
const updatedTaskDef = applyPropertyUpdates(changes.updates, upperCasedTaskDef);
// lowercase the merged task definition to use it in AWS SDK.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -812,13 +812,22 @@ describe.each([HotswapMode.FALL_BACK, HotswapMode.HOTSWAP_ONLY])('%p mode', (hot
});
});

test('should call registerTaskDefinition with certain properties not lowercased', async () => {
test('should call registerTaskDefinition with certain properties not lowercased nor uppercased', async () => {
// GIVEN
setupCurrentTaskDefinition({
taskDefinitionProperties: {
Family: 'my-task-def',
ContainerDefinitions: [
{ Image: 'image1' },
{
Image: 'image1',
DockerLabels: { Label1: 'label1' },
FirelensConfiguration: {
Options: { Name: 'cloudwatch' },
},
LogConfiguration: {
Options: { Option1: 'option1', option2: 'option2' },
},
},
],
Volumes: [
{
Expand Down Expand Up @@ -846,7 +855,7 @@ describe.each([HotswapMode.FALL_BACK, HotswapMode.HOTSWAP_ONLY])('%p mode', (hot
Options: { Name: 'cloudwatch' },
},
LogConfiguration: {
Options: { Option1: 'option1' },
Options: { Option1: 'option1', option2: 'option2' },
},
},
],
Expand Down Expand Up @@ -887,7 +896,7 @@ describe.each([HotswapMode.FALL_BACK, HotswapMode.HOTSWAP_ONLY])('%p mode', (hot
},
},
logConfiguration: {
options: { Option1: 'option1' },
options: { Option1: 'option1', option2: 'option2' },
},
},
],
Expand Down

0 comments on commit 6cffca0

Please sign in to comment.