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.

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

(ecs): Deployment Circuit Breaker cannot be turned off via CDK #27131

Closed
mlartz opened this issue Sep 13, 2023 · 2 comments · Fixed by #28611
Closed

(ecs): Deployment Circuit Breaker cannot be turned off via CDK #27131

mlartz opened this issue Sep 13, 2023 · 2 comments · Fixed by #28611
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@mlartz
Copy link

mlartz commented Sep 13, 2023

Describe the bug

I believe this is similar #25840, but for the DeploymentCircuitBreaker.

We are unable to turn off the DeploymentCircuitBreaker from an ECS service via CDK.

Expected Behavior

Deploying an ECS service with an unset .circuitBreaker will turn off the ECS Deployment Circuit Breaker if it was previously turned on.

Current Behavior

When setting the .circuitBreaker property on an ECS service in CDK, we see the following added to the resulting ECS::Service CFN:

   "Type": "AWS::ECS::Service",
   "Properties": {
    "DeploymentConfiguration": {
     "DeploymentCircuitBreaker": {
      "Enable" : true,
      "Rollback" : false,
     },
    ...
    },
   ...

When attempting to remove the circuit breaker by not setting the .circuitBreaker property on the CDK service, the DeploymentCircuitBreaker is no longer set in the resulting CFN. When this new stack (with a removed DeploymentCircuitBreaker), ECS does not turn off the circuit breaker. I believe that this is due to ECS not treating missing values as "different" when processing the CFN.

Manually editing the CFN to explicitly set the "Enable": false in the CFN does cause ECS to turn off the circuit breaker, e.g.:

   "Type": "AWS::ECS::Service",
   "Properties": {
    "DeploymentConfiguration": {
     "DeploymentCircuitBreaker": {
      "Enable" : false,
      "Rollback" : false,
     },
    ...
    },
   ...

Unfortunately, this ability to explicitly disable the circuit breaker is not provided by the L2 CDK construct, and instead it is implied that not settting .circuitBreaker on the service will achieve this.

Reproduction Steps

See "Current Behavior"

Possible Solution

I believe the right way to fix this is explicitly disable the DeploymentCircuitBreaker in the CFN when the .circuitBreaker property is unset.

Additional Information/Context

https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_ecs.FargateService.html#circuitbreaker

CDK CLI Version

2.93.0

Framework Version

No response

Node.js Version

14.21.3

OS

Linux

Language

Typescript

Language Version

No response

Other information

No response

@mlartz mlartz added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 13, 2023
@github-actions github-actions bot added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Sep 13, 2023
@peterwoodworth
Copy link
Contributor

I believe the right way to fix this is explicitly disable the DeploymentCircuitBreaker in the CFN when the .circuitBreaker property is unset.

Yes, this issue has come up before. Sometimes it's beneficial to expliticly define this, other times it becomes a hindrance for regions where a property is not supported yet. Regardless, thanks for the thorough bug report. You will be able to work around this by using escape hatches for now

@peterwoodworth peterwoodworth added p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Sep 14, 2023
@mergify mergify bot closed this as completed in #28611 Jan 23, 2024
mergify bot pushed a commit that referenced this issue Jan 23, 2024
…8611)

This PR has enabled explicit disabling of the circuit breaker. 

```ts
declare const cluster: ecs.Cluster;
declare const taskDefinition: ecs.TaskDefinition;
const service = new ecs.FargateService(this, 'Service', {
  cluster,
  taskDefinition,
  circuitBreaker: {
    enable: true, // added
    rollback: true
  },
});
```

This is useful for removing a circuit breaker that has been set previously.

Closes #27131.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Copy link

⚠️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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants