fix(ecs): allow both Fargate and ASG capacity providers in a cluster

Previously, adding/enabling Fargate capacity providers would add the FARGATE and FARGATE_SPOT capacity providers a part of the [capacityProviders field](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ecs-cluster.html#cfn-ecs-cluster-capacityproviders) on the ECS::Cluster resource. However, ASG Capacity Providers can only be specified as part of the [capacityProviders field on the ECS::ClusterCapacityProviderAssociation](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ecs-clustercapacityproviderassociations.html#cfn-ecs-clustercapacityproviderassociations-capacityproviders) resource, due to circular dependency issues. However, when a cluster is created, any capacity providers regardless of type must all be specified *either* on the cluster *or* as a ClusterCapacityProviderAssociation. Therefore, in order to support both types on the same cluster, Fargate capacity providers must also be specified as a ClusterCapacityProviderAssociation. This change removes the Fargate capacity providers from the Cluster resource and adds them as a ClusterCapacityProviderAssociation. Fixes aws#14730.
SoManyHs · Jun 9, 2021 · ea2b3b9 · ea2b3b9
1 parent 8625510
commit ea2b3b9
Show file tree

Hide file tree

Showing 6 changed files with 349 additions and 28 deletions.
diff --git a/packages/@aws-cdk/aws-ecs/lib/cluster.ts b/packages/@aws-cdk/aws-ecs/lib/cluster.ts
@@ -126,14 +126,9 @@ export class Cluster extends Resource implements ICluster {
   public readonly clusterName: string;
 
   /**
-   * The cluster-level (FARGATE, FARGATE_SPOT) capacity providers.
+   * The names of both ASG and Fargate capacity providers associated with the cluster.
    */
-  private _fargateCapacityProviders: string[] = [];
-
-  /**
-   * The EC2 Auto Scaling Group capacity providers associated with the cluster.
-   */
-  private _asgCapacityProviders: AsgCapacityProvider[] = [];
+  private _capacityProviderNames: string[] = [];
 
   /**
    * The AWS Cloud Map namespace to associate with the cluster.
@@ -173,7 +168,7 @@ export class Cluster extends Resource implements ICluster {
       clusterSettings = [{ name: 'containerInsights', value: props.containerInsights ? ContainerInsights.ENABLED : ContainerInsights.DISABLED }];
     }
 
-    this._fargateCapacityProviders = props.capacityProviders ?? [];
+    this._capacityProviderNames = props.capacityProviders ?? [];
     if (props.enableFargateCapacityProviders) {
       this.enableFargateCapacityProviders();
     }
@@ -189,7 +184,6 @@ export class Cluster extends Resource implements ICluster {
     const cluster = new CfnCluster(this, 'Resource', {
       clusterName: this.physicalName,
       clusterSettings,
-      capacityProviders: Lazy.list({ produce: () => this._fargateCapacityProviders }, { omitEmpty: true }),
       configuration: this._executeCommandConfiguration && this.renderExecuteCommandConfiguration(),
     });
 
@@ -216,16 +210,16 @@ export class Cluster extends Resource implements ICluster {
     // since it's harmless, but we'd prefer not to add unexpected new
     // resources to the stack which could surprise users working with
     // brown-field CDK apps and stacks.
-    Aspects.of(this).add(new MaybeCreateCapacityProviderAssociations(this, id, this._asgCapacityProviders));
+    Aspects.of(this).add(new MaybeCreateCapacityProviderAssociations(this, id, this._capacityProviderNames));
   }
 
   /**
    * Enable the Fargate capacity providers for this cluster.
    */
   public enableFargateCapacityProviders() {
     for (const provider of ['FARGATE', 'FARGATE_SPOT']) {
-      if (!this._fargateCapacityProviders.includes(provider)) {
-        this._fargateCapacityProviders.push(provider);
+      if (!this._capacityProviderNames.includes(provider)) {
+        this._capacityProviderNames.push(provider);
       }
     }
   }
@@ -329,7 +323,7 @@ export class Cluster extends Resource implements ICluster {
    */
   public addAsgCapacityProvider(provider: AsgCapacityProvider, options: AddAutoScalingGroupCapacityOptions = {}) {
     // Don't add the same capacity provider more than once.
-    if (this._asgCapacityProviders.includes(provider)) {
+    if (this._capacityProviderNames.includes(provider.capacityProviderName)) {
       return;
     }
 
@@ -340,7 +334,7 @@ export class Cluster extends Resource implements ICluster {
       taskDrainTime: provider.enableManagedTerminationProtection ? Duration.seconds(0) : options.taskDrainTime,
     });
 
-    this._asgCapacityProviders.push(provider);
+    this._capacityProviderNames.push(provider.capacityProviderName);
   }
 
   /**
@@ -462,8 +456,8 @@ export class Cluster extends Resource implements ICluster {
       throw new Error('CapacityProvider not supported');
     }
 
-    if (!this._fargateCapacityProviders.includes(provider)) {
-      this._fargateCapacityProviders.push(provider);
+    if (!this._capacityProviderNames.includes(provider)) {
+      this._capacityProviderNames.push(provider);
     }
   }
 
@@ -1340,22 +1334,24 @@ export class AsgCapacityProvider extends CoreConstruct {
 class MaybeCreateCapacityProviderAssociations implements IAspect {
   private scope: CoreConstruct;
   private id: string;
-  private capacityProviders: AsgCapacityProvider[]
+  private capacityProviders: string[]
+  private resource?: CfnClusterCapacityProviderAssociations
 
-  constructor(scope: CoreConstruct, id: string, capacityProviders: AsgCapacityProvider[]) {
+  constructor(scope: CoreConstruct, id: string, capacityProviders: string[] ) {
     this.scope = scope;
     this.id = id;
     this.capacityProviders = capacityProviders;
   }
+
   public visit(node: IConstruct): void {
     if (node instanceof Cluster) {
-      const providers = this.capacityProviders.map(p => p.capacityProviderName).filter(p => p !== 'FARGATE' && p !== 'FARGATE_SPOT');
-      if (providers.length > 0) {
-        new CfnClusterCapacityProviderAssociations(this.scope, this.id, {
+      if (this.capacityProviders.length > 0 && !this.resource) {
+        const resource = new CfnClusterCapacityProviderAssociations(this.scope, this.id, {
           cluster: node.clusterName,
           defaultCapacityProviderStrategy: [],
-          capacityProviders: Lazy.list({ produce: () => providers }),
+          capacityProviders: Lazy.list({ produce: () => this.capacityProviders }),
         });
+        this.resource = resource;
       }
     }
   }

diff --git a/packages/@aws-cdk/aws-ecs/test/cluster.test.ts b/packages/@aws-cdk/aws-ecs/test/cluster.test.ts
@@ -1709,6 +1709,10 @@ nodeunitShim({
 
     // THEN
     expect(stack).to(haveResource('AWS::ECS::Cluster', {
+      CapacityProviders: ABSENT,
+    }));
+
+    expect(stack).to(haveResource('AWS::ECS::ClusterCapacityProviderAssociations', {
       CapacityProviders: ['FARGATE_SPOT'],
     }));
 
@@ -1727,6 +1731,10 @@ nodeunitShim({
 
     // THEN
     expect(stack).to(haveResource('AWS::ECS::Cluster', {
+      CapacityProviders: ABSENT,
+    }));
+
+    expect(stack).to(haveResource('AWS::ECS::ClusterCapacityProviderAssociations', {
       CapacityProviders: ['FARGATE', 'FARGATE_SPOT'],
     }));
 
@@ -1744,6 +1752,10 @@ nodeunitShim({
 
     // THEN
     expect(stack).to(haveResource('AWS::ECS::Cluster', {
+      CapacityProviders: ABSENT,
+    }));
+
+    expect(stack).to(haveResource('AWS::ECS::ClusterCapacityProviderAssociations', {
       CapacityProviders: ['FARGATE', 'FARGATE_SPOT'],
     }));
 
@@ -1762,6 +1774,10 @@ nodeunitShim({
 
     // THEN
     expect(stack).to(haveResource('AWS::ECS::Cluster', {
+      CapacityProviders: ABSENT,
+    }));
+
+    expect(stack).to(haveResource('AWS::ECS::ClusterCapacityProviderAssociations', {
       CapacityProviders: ['FARGATE'],
     }));
 
@@ -1780,6 +1796,10 @@ nodeunitShim({
 
     // THEN
     expect(stack).to(haveResource('AWS::ECS::Cluster', {
+      CapacityProviders: ABSENT,
+    }));
+
+    expect(stack).to(haveResource('AWS::ECS::ClusterCapacityProviderAssociations', {
       CapacityProviders: ['FARGATE'],
     }));
 
@@ -1928,7 +1948,6 @@ nodeunitShim({
       enableManagedTerminationProtection: false,
     });
 
-    // These should not be added at the association level
     cluster.enableFargateCapacityProviders();
 
     // Ensure not added twice
@@ -1941,6 +1960,8 @@ nodeunitShim({
         Ref: 'EcsCluster97242B84',
       },
       CapacityProviders: [
+        'FARGATE',
+        'FARGATE_SPOT',
         {
           Ref: 'providerD3FF4D3A',
         },