fix(ecs): adding a circuit breaker causes Service replacement (under feature flag)

[fergusdixon]

Fixes #16126

Copied from #16919 with review comments as it went stale and closed.
Initial implementation #22328 reverted because it was not backward compatible.

Introduces feature flag:

• @aws-cdk/aws-ecs:disableExplicitDeploymentControllerForCircuitBreaker

Enable this feature flag to avoid setting the "ECS" deployment controller when adding a circuit breaker to an
ECS Service, as this will trigger a full replacement which fails to deploy when using set service names.
This does not change any behaviour as the default deployment controller when it is not defined is ECS.

cdk.json

{
  "context": {
    "@aws-cdk/aws-ecs:disableExplicitDeploymentControllerForCircuitBreaker": true
  }
}

---

All Submissions:

• Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

• This PR adds new unconventional dependencies following the process described here

New Features

• Have you added the new feature to an integration test?
  • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

[gitpod-io]

Pull request has been modified.

[corymhall]

@fergusdixon I've been thinking about this and as much as a don't want to introduce a new feature flag I think that's the best thing to do here. We want the new default behavior to be false and a feature flag is the only way to accomplish that.

https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#feature-flags

[fergusdixon]

Sure will give it a bash next week, thanks!

[fergusdixon]

@corymhall I have a working implementation with the flag, however I'm struggling to see this to false in the integration tests specifically. Should the integration tests cover new or old behaviour (I would imagine both), however I cant seem to set the feature flag value the same way as the unit tests do?

[fergusdixon]

It seems the integ-helper sets all feature flags to FUTURE_FLAGS (new behaviour), and this takes precedence over the context prop passed to App in the test. So from this it seems integration tests should test new behaviour and unit tests for both?

[corymhall]

If you want to test the old behavior you can use the postCliContext parameter on App.

[fergusdixon]

Have done, worked a charm, thanks @corymhall!

[rix0rrr]

I really want to ship this, but this property is giving me pause. I really don't see the need for it.

• If you want the ECS controller explicitly, you can just supply it ("The customer is always right", below)
• If you do NOT want the ECS controller, you can set the feature flag
  • If you want the ECS controller for some services and not other, you can either set the feature flag globally and add the explicit parameter to some services, or set the feature flag locally (you can flip a flag at any construct scope, it's all just context which is freely manipulable).

I think we can do without this, no?

Other than that, this is great!

[rgoltz]

Just to cross-check "the need for the 'correct' default after this PR is merged": Now with current CDKv2 versions today, CFN output always has an ECS DeploymentController defined, if you have an CircuitBreaker enabled (even if you not aware as CDK user).
Once a stack has an ECS DeploymentController definied (= all stack build by CDK with CircuitBreaker enabled), you are not able to simply "remove this CFN-lines from JSON output" - If a new/upcoming CDK version would start to not output ECS DeploymentController (by default) to JSON, all existing Services of existing Stacks would run into a replacement - Just because you update your CDK version.

As I understood the current code from Fergus, this default = true would remain the same behaviour as older CDKv2 version, right? - From my point of view that, it's an important brick, isn't it?

Pull request has been modified.

Pull request has been modified.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[fergusdixon]

@rix0rrr any idea why the deploy queue failed?

[aws-cdk-automation]

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

Pull request has been modified.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: f65b647
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[shellscape]

@rix0rrr is there a means to programmatically set feature flags? we've got around 20 fargate services going up, each with their own cdk.json and managing feature flags (specifically setting this one, since all of our services use set names) via cdk.json for each is a bit of a PITA. happy to open a new issue but wanted to ping first.

[corymhall]

@shellscape yes you can set it programmatically using setContext

node.setContext('@aws-cdk/aws-ecs:disableExplicitDeploymentControllerForCircuitBreaker', true);

This will apply the feature flag to all constructs within that scope.