(aws-ecs): Adding circuitBreaker config causes service replacement if deploymentController was not previously set

[madobrien]

When adding the circuitBreaker configuration to an ECS service, it automatically sets the DeploymentController to ECS in the resulting Cloudformation stack. If the deploymentController was not specified within the CDK previously, the default value of ECS is used when the service is created anyway.

Unfortunately Cloudformation sees this as a change requiring replacement of the service.

Reproduction Steps

Pre-existing service:

new FargateService(this, "Service", {
  cluster: cluster,
  taskDefinition: taskDefinition
})

Adding the circuitBreaker config:

new FargateService(this, "Service", {
  cluster: cluster,
  taskDefinition: taskDefinition,
  circuitBreaker: { enabled: true, rollback: true }
})

Deploying the above causes service replacement.

What did you expect to happen?

Expected the DeploymentController to be left undefined, since that still results in the default value of ECS being used.

What actually happened?

Upon deployment the service required replacement, which will have been disruptive due to autoscaled values not carrying across to the new service.

Ended up using escape hatches to set the circuitBreak properties without adding the DeploymentController configuration.

Environment

• CDK CLI Version : 1.118.0
• Framework Version: 1.118.0
• Node.js Version: v14.15.1
• OS : Mac OS 11.5
• Language (Version): Typescript (4.3.3)

Other

---

This is 🐛 Bug Report

[jamestelfer]

It seems like the PR adding circuit breaker support also added:

aws-cdk/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

Lines 412 to 414 in f2d77d3

	deploymentController: props.circuitBreaker ? {
	type: DeploymentControllerType.ECS,
	} : props.deploymentController,

The reasoning was that DeploymentCircuitBreaker requires the ECS deployment type, which is correct, just not necessary to add explicitly.

This addition could be removed entirely or (if it was thought valuable) replaced with a warning if a non-ECS deployment type is specified at the same time as the circuitBreaker property.

[rix0rrr]

This addition could be removed entirely or (if it was thought valuable) replaced with a warning if a non-ECS deployment type is specified at the same time as the circuitBreaker property.

I love it, that's a much better behavior! We would accept a PR that does this.

[freekii]

Hi! If this is still available I would be keen to be assigned to this. Just clarifying, the preference here is to do the latter and produce a warning if a non-ECS deployment type is specified? Thanks!

[rgoltz]

The reasoning was that DeploymentCircuitBreaker requires the ECS deployment type, which is correct, just not necessary to add explicitly.

This addition could be removed entirely or (if it was thought valuable) replaced with a warning if a non-ECS deployment type is specified at the same time as the circuitBreaker property.

Following this idea to remove the DeploymentController from CFN output generated by CDK while we are using the circuitBreaker functionality would help us to resolve a CFN drift detection issue described via aws-cloudformation/cloudformation-coverage-roadmap#1259. As stated before DeploymentController of type ECS is the default, in case it's not explicitly set in CFN.

[github-actions]

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

[rgoltz]

Hi @fergusdixon + @corymhall - Please, let's cross-check & discuss following question/issue:

From my experience & testing removing the DeploymentController defintion from an existing stack/CFN output/template the Service needs a replacement - I assume that's something people should know, once the update the CDK version, which includes the new/better behaviour from PR #22328 . I've tested this with a pure CFN template: Even you define DeploymentController = ECS (which is also the CFN default, if nothing is defined) and you just remove this part from CFN template, a replacement is triggered.

This replacement becomes critical / will fail, once you define a custom/"static" ServiceName (via CDK/CFN). I assume that in this case a new CDK version with this (btw: good & helpful) fix could cause an stack-update to fail/break.

[fergusdixon]

@rgoltz so should we make some sort of notice that old stacks not specifying a deployment controller, should now explicitly set one to avoid a replacement?

[rgoltz]

@fergusdixon From my understand the old stacks just having circuitBreaker enabled (which means in the "old code", that DeploymentController = ECS getting added silently to CFN) would have issues.

In case I understood the PR in the right way, CFN output of CDK will no longer contain DeploymentController =ECS , once I just enable circuitBreaker - right? (We dropped this unnecessary conjunction)

So, updates of existing stacks (with circuitBreaker+DeploymentController defined in CFN) would run into a replacement, because the "new code" of CDK do not have DeploymentController = ECS in CFN output.

Replacement of a Service (because DeploymentController is not part of CFN anymore), would fail, if users define a Service with a own (not auto-generated) ServiceName.

[fergusdixon]

@rgoltz correct, the changes in the integration tests showed this

[rgoltz]

I've tried to create & provide a test-case:

• ECS Service with a ServiceName set
• based on the current CDK with: circuitBreaker silently adding DeploymentController to CFN (step1)
• simulate CFN output without DeploymentController

// step1
// Service with circuitBreaker set/enabled + serviceName is set
    const myEcsService = new ecsPatterns.ApplicationLoadBalancedFargateService(this, 'myEcsService', {
      cluster: cluster,
      memoryLimitMiB: 2048,
      desiredCount: 1,
      cpu: 1024,
      taskSubnets: {
        subnets: selectedSubnets.subnets
      },
      loadBalancerName: `alb-${props.environment}`,
      assignPublicIp: false,
      circuitBreaker: {
        rollback: false,  
      },
      taskDefinition: myTaskDefinition,
      domainName: myDomainName,
      targetProtocol: elbv2.ApplicationProtocol.HTTPS,
      sslPolicy: elbv2.SslPolicy.FORWARD_SECRECY_TLS12_RES_GCM,
      redirectHTTP: false,
      publicLoadBalancer: false,
      recordType: ecsPatterns.ApplicationLoadBalancedServiceRecordType.CNAME,
      protocol: elbv2.ApplicationProtocol.HTTPS,
      domainZone: route53.HostedZone.fromLookup(this, 'HostedZone', {
        domainName: hostedZoneDomain,
        vpcId: this.vpc.vpcId,
        privateZone: true, 
      }),
      propagateTags: ecs.PropagatedTagSource.SERVICE,
      enableECSManagedTags: true,
      certificate: myCertificate,
      minHealthyPercent: 0,
      serviceName: serviceName,
      openListener: false,
    });

// step2
// after "inital" deployment of the service / stack (with fixed ServiceName), remove the DeploymentController from CFN output (which seems like #22328)
    const baseService = myEcsService.node.children.find(child => (child as ecs.BaseService).serviceName) as ecs.BaseService;
    (baseService.node.defaultChild! as ecs.CfnService).addPropertyDeletionOverride('DeploymentController');

I really like your PR/the fix, but following this test above (please add step2 code after the intial deployment) showning this breaking stack-updated (even "no changes" were made, just a CDK update). I think (at minimum) a warning/information for "circuitBreaker-user" needs to be added.

Since we have this setup (circuitBreaker + fixed ServiceName string), I doesn't have an idea how we should upgrade to a CDK version having this changed CFN output (regarding DeploymentController). Any ideas how to handle this for existing Services, @fergusdixon?

[fergusdixon]

@rgoltz my only solution would be explicitly setting the deploymentController directly in your service. In terms of making this backwards compatible, I don't have a solution. This is my first time contributing as well as my first time using Typescript so definitely not an expert, perhaps @corymhall can weigh in?

[fergusdixon]

@corymhall @rgoltz do you have any suggestions on how to implement this in a backwards compatible way?

[fergusdixon]

@rgoltz @corymhall please see backwards-compatible attempt #22467

[github-actions]

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

[shellscape]

@madobrien did the association PR with the disableExplicitDeploymentControllerForCircuitBreaker flag fix your issue? we're testing it here without success