aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.41k stars 3.8k forks source link

(aws-ecs): adding cluster config disables container insights #17739

Open rajyan opened 2 years ago

rajyan commented 2 years ago

What is the problem?

ECS Cluster container insights gets disabled, even if the cdk stack settings is set to true.

Reproduction Steps

Created a minimal reproducible example at https://github.com/rajyan/cdk-ecs-bug.

First, deploy a cluster with containerInsights: true only.

import * as cdk from '@aws-cdk/core';
import * as ecs from '@aws-cdk/aws-ecs';

export class CdkEcsBugStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    new ecs.Cluster(this, 'Cluster', {
      containerInsights: true,
    });
  }
}

Then deploy the cluster adding executeCommandConfiguration, leaving containerInsights: true.

import * as cdk from '@aws-cdk/core';
import * as ecs from '@aws-cdk/aws-ecs';

export class CdkEcsBugStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    new ecs.Cluster(this, 'Cluster', {
      containerInsights: true,
      executeCommandConfiguration: {
        logging: ecs.ExecuteCommandLogging.OVERRIDE,
        logConfiguration: {
          cloudWatchLogGroup: new LogGroup(this, 'LogGroup'),
        }
      },
    });
  }
}

What did you expect to happen?

Only executeCommandConfiguration should be added to the cluster, and no changes in containerInsights.

What actually happened?

Container insights got disabled.

CDK CLI Version

1.134.0

Framework Version

No response

Node.js Version

v16.13.0

OS

Both on Linux and maxOS

Language

Typescript

Language Version

TypeScript (3.9.7)

Other information

Also confirmed that deploying these two settings at once won't disable container insights.

rajyan commented 2 years ago

@ryparker Thanks for the labels. Could I ask what I should add to resolve "needs-reproduction"? I believe I had produced a minimal reproducer, copy-paste ready stack example.

ryparker commented 2 years ago

@rajyan The "needs-reproduction" label was just a temporary marker that means we haven't verified the reproduction.

Thank you for providing reproduction code. I was able to reproduce this however i'm not sure if this is a CDK issue. Even when you remove the executeCommandConfiguration, it does not re-enable containerInsights on the cluster. The only way to re-enable containerInsights is to delete the cluster and start fresh. This seems to me like a bug in the ECS service.

I confirmed that even with executeCommandConfiguration provided, the CloudFormation output included the containerInsights config:

"ClusterEB0386A7": {
  "Type": "AWS::ECS::Cluster",
  "Properties": {
    "ClusterSettings": [
      {
        "Name": "containerInsights",
        "Value": "enabled"
      }
    ]
  },
  "Metadata": {
    "aws:cdk:path": "cdk-ecs-bug/Cluster/Resource"
  }
},
ryparker commented 2 years ago

Handling this internally. I'll follow up in this thread when we have updates to share.

rajyan commented 2 years ago

@ryparker

The "needs-reproduction" label was just a temporary marker that means we haven't verified the reproduction.

I’m sorry to hurry you.

Thank you very much for your investigation!

ryparker commented 2 years ago

SIM #ECS-17445

github-actions[bot] commented 1 year ago

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

rajyan commented 1 year ago

I think this issue is not fixed yet?