Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ecs] Cannot disable containerInsights of a cluster whose setting is enabled #9149

Closed
tmokmss opened this issue Jul 18, 2020 · 3 comments · Fixed by #9151
Closed

[ecs] Cannot disable containerInsights of a cluster whose setting is enabled #9149

tmokmss opened this issue Jul 18, 2020 · 3 comments · Fixed by #9151
Assignees
@uttarasridhar @MrArnoldPalmer @SoManyHs
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. in-progress This issue is being actively worked on.

Comments

@tmokmss
Copy link
Contributor

tmokmss commented Jul 18, 2020 •
edited
Loading

I tried disabling containerInsights of an ECS cluster but it failed.
It seems CFn doesn't support removing the setting but replacing.

Reproduction Steps

  1. deploy a cluster with containerInsights enabled:
new ecs.Cluster(this, "cluster", {
    clusterName: "cluster",
    vpc: vpc,
    containerInsights: true,
});
  1. re-deploy the cluster after switching containerInsights to disabled:
new ecs.Cluster(this, "cluster", {
    clusterName: "cluster",
    vpc: vpc,
    containerInsights: false,
});

c.f. cdk diff when deploying in 2nd step

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

Error Log

When deploying in 2nd step, an error occurs:

11:34:57 | UPDATE_FAILED        | AWS::ECS::Cluster       | cluster611F8AFF
Error occurred during operation 'Settings can only be modified, not removed. Required Settings: [containerInsights]'.

Environment

  • CLI Version :1.50.0
  • Framework Version:1.50.0
  • Node.js Version:v13.11.0
  • OS :macOS 10.15.4
  • Language (Version): TypeScript

Other

The cdk diff is supposed to be below:

[~] AWS::ECS::Cluster cluster cluster611F8AFF 
 └─ [~] ClusterSettings
     └─ @@ -1,6 +1,6 @@
        [ ] [
        [ ]   {
        [ ]     "Name": "containerInsights",
        [-]     "Value": "enabled"
        [+]     "Value": "disabled"
        [ ]   }
        [ ] ]

This is 🐛 Bug Report
@tmokmss tmokmss added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 18, 2020
@tmokmss tmokmss mentioned this issue Jul 18, 2020
Merged
@SomayaB SomayaB added @aws-cdk/aws-ecs Related to Amazon Elastic Container in-progress This issue is being actively worked on. labels Jul 20, 2020
@SoManyHs SoManyHs removed the needs-triage This issue or PR still needs to be triaged. label Jul 27, 2020
@chrisb87
Copy link

chrisb87 commented Sep 15, 2020 •
edited
Loading

I see there's an unreleased fix for this (#9151).

Until that's released, what's the process for manually fixing this? I have a service deployed with CDK that has container insights enabled and I want to disable that setting to reduce my bill. If I do this outside the CDK it will never know the value was changed and continue to fail deployments.

@tmokmss
Copy link
Contributor Author

tmokmss commented Sep 16, 2020 •
edited
Loading

@chrisb87
I think there's no workaround as long as you use CDK.
To disable the setting there's only two ways:

  1. Re-create the cluster
  2. disable it using AWS-CLI and tolerate CFn drift

anyway hope it'll be merged soon:(

@mergify mergify bot closed this as completed in #9151 Dec 9, 2020
mergify bot pushed a commit that referenced this issue Dec 9, 2020
@tmokmss
fix(ecs): cannot disable container insights of an ECS cluster (#9151)
e328f22 
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*
@github-actions
Copy link

github-actions bot commented Dec 9, 2020

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

flochaz pushed a commit to flochaz/aws-cdk that referenced this issue Jan 5, 2021
@tmokmss @flochaz
fix(ecs): cannot disable container insights of an ECS cluster (aws#9151)
82b9483 
fixes aws#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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@uttarasridhar uttarasridhar

@MrArnoldPalmer MrArnoldPalmer

@SoManyHs SoManyHs

Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. in-progress This issue is being actively worked on.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(ecs): cannot disable container insights of an ECS cluster tmokmss/aws-cdk
6 participants
@chrisb87 @uttarasridhar @MrArnoldPalmer @tmokmss @SomayaB @SoManyHs