From e328f22e7daa5fb5ea3de9fb26828314131e8a57 Mon Sep 17 00:00:00 2001
From: tmokmss <tmokmss@users.noreply.github.com>
Date: Thu, 10 Dec 2020 08:49:05 +0900
Subject: [PATCH] fix(ecs): cannot disable container insights of an ECS cluster
 (#9151)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

fixes #9149

The clusterSettings should be explicitly defined even when container insights is disabled, because CFn doesn't allow to remove the existing settings (as described in the issue.)

By this change, users who sets `containerInsights` prop `false` will see a diff like below, but it won't be a problem because by default container insights is disabled hence no actual change to existing resources:

```
Resources
[~] AWS::ECS::Cluster cluster cluster611F8AFF
 └─ [+] ClusterSettings
     └─ [{"Name":"containerInsights","Value":"disabled"}]
```

To prevent the existing tests from failing, the clusterSettings is only defined when `containerInsight` prop is explicitly defined(i.e. `true` or `false`, not `undefined`.)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
---
 packages/@aws-cdk/aws-ecs/lib/cluster.ts      |  6 +++--
 .../@aws-cdk/aws-ecs/test/test.ecs-cluster.ts | 22 ++++++++++++++++++-
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/packages/@aws-cdk/aws-ecs/lib/cluster.ts b/packages/@aws-cdk/aws-ecs/lib/cluster.ts
index 1ae92f0abd4fc..4108002c2a1fb 100644
--- a/packages/@aws-cdk/aws-ecs/lib/cluster.ts
+++ b/packages/@aws-cdk/aws-ecs/lib/cluster.ts
@@ -124,8 +124,10 @@ export class Cluster extends Resource implements ICluster {
       physicalName: props.clusterName,
     });
 
-    const containerInsights = props.containerInsights !== undefined ? props.containerInsights : false;
-    const clusterSettings = containerInsights ? [{ name: 'containerInsights', value: 'enabled' }] : undefined;
+    let clusterSettings = undefined;
+    if (props.containerInsights !== undefined) {
+      clusterSettings = [{ name: 'containerInsights', value: props.containerInsights ? 'enabled' : 'disabled' }];
+    }
 
     const cluster = new CfnCluster(this, 'Resource', {
       clusterName: this.physicalName,
diff --git a/packages/@aws-cdk/aws-ecs/test/test.ecs-cluster.ts b/packages/@aws-cdk/aws-ecs/test/test.ecs-cluster.ts
index 146c5a908c074..4cf4667663949 100644
--- a/packages/@aws-cdk/aws-ecs/test/test.ecs-cluster.ts
+++ b/packages/@aws-cdk/aws-ecs/test/test.ecs-cluster.ts
@@ -1457,7 +1457,27 @@ export = {
     test.done();
   },
 
-  'default container insights undefined'(test: Test) {
+  'disable container insights'(test: Test) {
+    // GIVEN
+    const app = new cdk.App();
+    const stack = new cdk.Stack(app, 'test');
+
+    new ecs.Cluster(stack, 'EcsCluster', { containerInsights: false });
+
+    // THEN
+    expect(stack).to(haveResource('AWS::ECS::Cluster', {
+      ClusterSettings: [
+        {
+          Name: 'containerInsights',
+          Value: 'disabled',
+        },
+      ],
+    }, ResourcePart.Properties));
+
+    test.done();
+  },
+
+  'default container insights is disabled'(test: Test) {
     // GIVEN
     const app = new cdk.App();
     const stack = new cdk.Stack(app, 'test');