From 1d78413548781dc79be5b2e09fa64c55ee5e079e Mon Sep 17 00:00:00 2001 From: Sebastien Corbiere Date: Fri, 24 May 2024 15:25:06 -0700 Subject: [PATCH 1/6] Let AsgCapacityProvider use IAutoScalingGroup even when Managed Termination Protection is enabled. The code will emit a warning message but will let the user specify a self managed ASG using `AutoScalingGroup.fromAutoScalingGroupName`. Also makes sure that when calling `Cluster.addAsgCapacityProvider`, the ASG used is not an imported one. --- packages/aws-cdk-lib/aws-ecs/lib/cluster.ts | 15 +- .../aws-cdk-lib/aws-ecs/test/cluster.test.ts | 136 ++++++++++++++---- 2 files changed, 118 insertions(+), 33 deletions(-) diff --git a/packages/aws-cdk-lib/aws-ecs/lib/cluster.ts b/packages/aws-cdk-lib/aws-ecs/lib/cluster.ts index 95979e740f8b2..b221e9eb84c55 100644 --- a/packages/aws-cdk-lib/aws-ecs/lib/cluster.ts +++ b/packages/aws-cdk-lib/aws-ecs/lib/cluster.ts @@ -11,7 +11,7 @@ import * as kms from '../../aws-kms'; import * as logs from '../../aws-logs'; import * as s3 from '../../aws-s3'; import * as cloudmap from '../../aws-servicediscovery'; -import { Duration, IResource, Resource, Stack, Aspects, ArnFormat, IAspect, Token, Names } from '../../core'; +import { Duration, IResource, Resource, Stack, Aspects, ArnFormat, IAspect, Token, Names, Annotations } from '../../core'; const CLUSTER_SYMBOL = Symbol.for('@aws-cdk/aws-ecs/lib/cluster.Cluster'); @@ -474,6 +474,9 @@ export class Cluster extends Resource implements ICluster { } private configureAutoScalingGroup(autoScalingGroup: autoscaling.AutoScalingGroup, options: AddAutoScalingGroupCapacityOptions = {}) { + if (!(autoScalingGroup instanceof autoscaling.AutoScalingGroup)) { + throw new Error('Cannot configure the AutoScalingGroup because it is an imported resource.'); + } if (autoScalingGroup.osType === ec2.OperatingSystemType.WINDOWS) { this.configureWindowsAutoScalingGroup(autoScalingGroup, options); } else { @@ -1177,6 +1180,10 @@ export interface AsgCapacityProviderProps extends AddAutoScalingGroupCapacityOpt /** * The autoscaling group to add as a Capacity Provider. + * + * Warning: When passing an imported resource using `AutoScalingGroup.fromAutoScalingGroupName`, + * the AsgCapacityProvider construct will not be able to enforce the option `newInstancesProtectedFromScaleIn` of the AutoScalingGroup will be enabled. + * If this property is not enable in the AutoScalingGroup, the CFN stack execution will fail. */ readonly autoScalingGroup: autoscaling.IAutoScalingGroup; @@ -1306,7 +1313,11 @@ export class AsgCapacityProvider extends Construct { throw new Error('Cannot enable Managed Termination Protection on a Capacity Provider when Managed Scaling is disabled. Either enable Managed Scaling or disable Managed Termination Protection.'); } if (this.enableManagedTerminationProtection) { - this.autoScalingGroup.protectNewInstancesFromScaleIn(); + if (this.autoScalingGroup instanceof autoscaling.AutoScalingGroup) { + this.autoScalingGroup.protectNewInstancesFromScaleIn(); + } else { + Annotations.of(this).addWarningV2('@aws-cdk/aws-ecs:cannotEnforceNewInstancesProtectedFromScaleIn', 'Cannot enforce `newInstancesProtectedFromScaleIn: true` on the AutoScalingGroup. This will have no effect if the AutoScalingGroup was created in this CDK app.'); + } } const capacityProviderNameRegex = /^(?!aws|ecs|fargate).+/gm; diff --git a/packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts b/packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts index 4f10a1163f98f..bc6d8247b6108 100644 --- a/packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts +++ b/packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts @@ -8,6 +8,7 @@ import * as logs from '../../aws-logs'; import * as s3 from '../../aws-s3'; import * as cloudmap from '../../aws-servicediscovery'; import * as cdk from '../../core'; +import { getWarnings } from '../../core/test/util'; import * as cxapi from '../../cx-api'; import * as ecs from '../lib'; @@ -2194,36 +2195,81 @@ describe('cluster', () => { }); - test('creates ASG capacity providers with expected defaults', () => { - // GIVEN - const app = new cdk.App(); - const stack = new cdk.Stack(app, 'test'); - const vpc = new ec2.Vpc(stack, 'Vpc'); - const autoScalingGroup = new autoscaling.AutoScalingGroup(stack, 'asg', { - vpc, - instanceType: new ec2.InstanceType('bogus'), - machineImage: ecs.EcsOptimizedImage.amazonLinux2(), + describe('creates ASG capacity providers ', () => { + test('with expected defaults', () => { + // GIVEN + const app = new cdk.App(); + const stack = new cdk.Stack(app, 'test'); + const vpc = new ec2.Vpc(stack, 'Vpc'); + const autoScalingGroup = new autoscaling.AutoScalingGroup(stack, 'asg', { + vpc, + instanceType: new ec2.InstanceType('bogus'), + machineImage: ecs.EcsOptimizedImage.amazonLinux2(), + }); + + // WHEN + new ecs.AsgCapacityProvider(stack, 'provider', { + autoScalingGroup, + }); + + // THEN + Template.fromStack(stack).hasResourceProperties('AWS::ECS::CapacityProvider', { + AutoScalingGroupProvider: { + AutoScalingGroupArn: { + Ref: 'asgASG4D014670', + }, + ManagedScaling: { + Status: 'ENABLED', + TargetCapacity: 100, + }, + ManagedTerminationProtection: 'ENABLED', + }, + }); }); - // WHEN - new ecs.AsgCapacityProvider(stack, 'provider', { - autoScalingGroup, + test('with IAutoScalingGroup should emit a warning if Managed Termination Protection is enabled.', () => { + // GIVEN + const app = new cdk.App(); + const stack = new cdk.Stack(app, 'test'); + const autoScalingGroup = autoscaling.AutoScalingGroup.fromAutoScalingGroupName(stack, 'ASG', 'my-asg'); + + // WHEN + new ecs.AsgCapacityProvider(stack, 'provider', { + autoScalingGroup, + }); + + // THEN + expect(getWarnings(app.synth())).toEqual([{ + message: 'Cannot enforce `newInstancesProtectedFromScaleIn: true` on the AutoScalingGroup. This will have no effect if the AutoScalingGroup was created in this CDK app. [ack: @aws-cdk/aws-ecs:cannotEnforceNewInstancesProtectedFromScaleIn]', + path: '/test/provider', + }]); }); - // THEN - Template.fromStack(stack).hasResourceProperties('AWS::ECS::CapacityProvider', { - AutoScalingGroupProvider: { - AutoScalingGroupArn: { - Ref: 'asgASG4D014670', - }, - ManagedScaling: { - Status: 'ENABLED', - TargetCapacity: 100, + test('with IAutoScalingGroup should not emit warning if Managed Termination Protection is disabled.', () => { + // GIVEN + const app = new cdk.App(); + const stack = new cdk.Stack(app, 'test'); + const autoScalingGroup = autoscaling.AutoScalingGroup.fromAutoScalingGroupName(stack, 'ASG', 'my-asg'); + + // WHEN + new ecs.AsgCapacityProvider(stack, 'provider', { + autoScalingGroup, + enableManagedTerminationProtection: false, + }); + + // THEN + Template.fromStack(stack).hasResourceProperties('AWS::ECS::CapacityProvider', { + AutoScalingGroupProvider: { + AutoScalingGroupArn: 'my-asg', + ManagedScaling: { + Status: 'ENABLED', + TargetCapacity: 100, + }, + ManagedTerminationProtection: 'DISABLED', }, - ManagedTerminationProtection: 'ENABLED', - }, + }); + expect(getWarnings(app.synth())).toEqual([]); }); - }); test('can disable Managed Scaling and Managed Termination Protection for ASG capacity provider', () => { @@ -2483,6 +2529,22 @@ describe('cluster', () => { }); + test('should throw an error if the capacity provider was created using an imported AsgCapacityProvider', () => { + // GIVEN + const app = new cdk.App(); + const stack = new cdk.Stack(app, 'test'); + const autoScalingGroup = autoscaling.AutoScalingGroup.fromAutoScalingGroupName(stack, 'ASG', 'my-asg'); + const cluster = new ecs.Cluster(stack, 'EcsCluster'); + + const capacityProvider = new ecs.AsgCapacityProvider(stack, 'provider', { + autoScalingGroup, + }); + // THEN + expect(() => { + cluster.addAsgCapacityProvider(capacityProvider); + }).toThrow('Cannot configure the AutoScalingGroup because it is an imported resource.'); + }); + test('should throw an error if capacity provider with default strategy is not present in capacity providers', () => { // GIVEN const app = new cdk.App(); @@ -3042,11 +3104,17 @@ test('throws when InstanceWarmupPeriod is greater than 10000', () => { describe('Accessing container instance role', function () { const addUserDataMock = jest.fn(); - const autoScalingGroup: autoscaling.AutoScalingGroup = { - addUserData: addUserDataMock, - addToRolePolicy: jest.fn(), - protectNewInstancesFromScaleIn: jest.fn(), - } as unknown as autoscaling.AutoScalingGroup; + + function getAutoScalingGroup(stack: cdk.Stack) : autoscaling.AutoScalingGroup { + const vpc = new ec2.Vpc(stack, 'Vpc'); + const asg = new autoscaling.AutoScalingGroup(stack, 'asg', { + vpc, + instanceType: new ec2.InstanceType('bogus'), + machineImage: ecs.EcsOptimizedImage.amazonLinux2(), + }); + asg.addUserData = addUserDataMock; + return asg; + } afterEach(() => { addUserDataMock.mockClear(); @@ -3057,11 +3125,12 @@ describe('Accessing container instance role', function () { const app = new cdk.App(); const stack = new cdk.Stack(app, 'test'); const cluster = new ecs.Cluster(stack, 'EcsCluster'); + const autoScalingGroup = getAutoScalingGroup(stack); // WHEN const capacityProvider = new ecs.AsgCapacityProvider(stack, 'Provider', { - autoScalingGroup: autoScalingGroup, + autoScalingGroup, }); cluster.addAsgCapacityProvider(capacityProvider); @@ -3077,10 +3146,11 @@ describe('Accessing container instance role', function () { const app = new cdk.App(); const stack = new cdk.Stack(app, 'test'); const cluster = new ecs.Cluster(stack, 'EcsCluster'); + const autoScalingGroup = getAutoScalingGroup(stack); // WHEN const capacityProvider = new ecs.AsgCapacityProvider(stack, 'Provider', { - autoScalingGroup: autoScalingGroup, + autoScalingGroup, }); cluster.addAsgCapacityProvider(capacityProvider, { @@ -3098,6 +3168,7 @@ describe('Accessing container instance role', function () { const app = new cdk.App(); const stack = new cdk.Stack(app, 'test'); const cluster = new ecs.Cluster(stack, 'EcsCluster'); + const autoScalingGroup = getAutoScalingGroup(stack); // WHEN const capacityProvider = new ecs.AsgCapacityProvider(stack, 'Provider', { @@ -3118,6 +3189,7 @@ describe('Accessing container instance role', function () { const app = new cdk.App(); const stack = new cdk.Stack(app, 'test'); const cluster = new ecs.Cluster(stack, 'EcsCluster'); + const autoScalingGroup = getAutoScalingGroup(stack); // WHEN const capacityProvider = new ecs.AsgCapacityProvider(stack, 'Provider', { @@ -3140,6 +3212,7 @@ describe('Accessing container instance role', function () { const app = new cdk.App(); const stack = new cdk.Stack(app, 'test'); const cluster = new ecs.Cluster(stack, 'EcsCluster'); + const autoScalingGroup = getAutoScalingGroup(stack); // WHEN const capacityProvider = new ecs.AsgCapacityProvider(stack, 'Provider', { @@ -3162,6 +3235,7 @@ describe('Accessing container instance role', function () { const app = new cdk.App(); const stack = new cdk.Stack(app, 'test'); const cluster = new ecs.Cluster(stack, 'EcsCluster'); + const autoScalingGroup = getAutoScalingGroup(stack); // WHEN const capacityProvider = new ecs.AsgCapacityProvider(stack, 'Provider', { From d0d05dd81e9da973862a06d54ca923d39c7fd39d Mon Sep 17 00:00:00 2001 From: Jimmy Gaussen Date: Mon, 17 Jun 2024 23:52:40 +0200 Subject: [PATCH 2/6] fix(core): overrideLogicalId validation (#29708) ### Issue # (if applicable) Closes #29701 ### Reason for this change Calling `overrideLogicalId` on a `Construct` with an invalid logical ID ([docs](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/resources-section-structure.html#resources-section-structure-logicalid)) would not throw an error at synthesis time. CloudFormation would ### Description of changes * Validate `overrideLogicalId` (must not be empty, must not be over 255 characters, must match `/^[A-Za-z0-9]+$/` * Document exceptions with `@error` JSDoc tags ### Description of how you validated changes I've added unit tests, integration tests should not be necessary ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/aws-cdk-lib/core/lib/cfn-element.ts | 23 +++++- packages/aws-cdk-lib/core/test/stack.test.ts | 84 ++++++++++++++++++-- 2 files changed, 99 insertions(+), 8 deletions(-) diff --git a/packages/aws-cdk-lib/core/lib/cfn-element.ts b/packages/aws-cdk-lib/core/lib/cfn-element.ts index 230ed20376662..396a27b6f8d5d 100644 --- a/packages/aws-cdk-lib/core/lib/cfn-element.ts +++ b/packages/aws-cdk-lib/core/lib/cfn-element.ts @@ -81,14 +81,33 @@ export abstract class CfnElement extends Construct { /** * Overrides the auto-generated logical ID with a specific ID. * @param newLogicalId The new logical ID to use for this stack element. + * + * @throws an error if `logicalId` has already been locked + * @throws an error if `newLogicalId` is empty + * @throws an error if `newLogicalId` contains more than 255 characters + * @throws an error if `newLogicalId` contains non-alphanumeric characters + * + * @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/resources-section-structure.html#resources-section-structure-logicalid */ public overrideLogicalId(newLogicalId: string) { if (this._logicalIdLocked) { throw new Error(`The logicalId for resource at path ${Node.of(this).path} has been locked and cannot be overridden\n` + 'Make sure you are calling "overrideLogicalId" before Stack.exportValue'); - } else { - this._logicalIdOverride = newLogicalId; } + + if (!Token.isUnresolved(newLogicalId)) { + if (!newLogicalId) { + throw new Error('Cannot set an empty logical ID'); + } + if (newLogicalId.length > 255) { + throw new Error(`Invalid logical ID override: '${newLogicalId}'. It must be at most 255 characters long, got ${newLogicalId.length} characters.`); + } + if (!newLogicalId.match(/^[A-Za-z0-9]+$/)) { + throw new Error(`Invalid logical ID override: '${newLogicalId}'. It must only contain alphanumeric characters.`); + } + } + + this._logicalIdOverride = newLogicalId; } /** diff --git a/packages/aws-cdk-lib/core/test/stack.test.ts b/packages/aws-cdk-lib/core/test/stack.test.ts index 82be67b19499b..5aa4b5d44fa5f 100644 --- a/packages/aws-cdk-lib/core/test/stack.test.ts +++ b/packages/aws-cdk-lib/core/test/stack.test.ts @@ -1370,10 +1370,49 @@ describe('stack', () => { // THEN - producers are the same expect(() => { - resourceM.overrideLogicalId('OVERRIDE_LOGICAL_ID'); + resourceM.overrideLogicalId('OVERRIDELOGICALID'); }).toThrow(/The logicalId for resource at path Producer\/ResourceXXX has been locked and cannot be overridden/); }); + test('throw error if overrideLogicalId contains non-alphanumeric characters', () => { + // GIVEN: manual + const appM = new App(); + const producerM = new Stack(appM, 'Producer'); + const resourceM = new CfnResource(producerM, 'ResourceXXX', { type: 'AWS::Resource' }); + + // THEN - producers are the same + expect(() => { + resourceM.overrideLogicalId('INVALID_LOGICAL_ID'); + }).toThrow(/must only contain alphanumeric characters/); + }); + + test('throw error if overrideLogicalId is over 255 characters', () => { + // GIVEN: manual + const appM = new App(); + const producerM = new Stack(appM, 'Producer'); + const resourceM = new CfnResource(producerM, 'ResourceXXX', { type: 'AWS::Resource' }); + + // THEN - producers are the same + expect(() => { + resourceM.overrideLogicalId( + // 256 character long string of "aaaa..." + Array(256).fill('a').join(''), + ); + }).toThrow(/must be at most 255 characters long, got 256 characters/); + }); + + test('throw error if overrideLogicalId is an empty string', () => { + // GIVEN: manual + const appM = new App(); + const producerM = new Stack(appM, 'Producer'); + const resourceM = new CfnResource(producerM, 'ResourceXXX', { type: 'AWS::Resource' }); + + // THEN - producers are the same + expect(() => { + resourceM.overrideLogicalId(''); + }).toThrow('Cannot set an empty logical ID'); + }); + test('do not throw error if overrideLogicalId is used and logicalId is not locked', () => { // GIVEN: manual const appM = new App(); @@ -1381,26 +1420,59 @@ describe('stack', () => { const resourceM = new CfnResource(producerM, 'ResourceXXX', { type: 'AWS::Resource' }); // THEN - producers are the same - resourceM.overrideLogicalId('OVERRIDE_LOGICAL_ID'); + resourceM.overrideLogicalId('OVERRIDELOGICALID'); + producerM.exportValue(resourceM.getAtt('Att')); + + const template = appM.synth().getStackByName(producerM.stackName).template; + expect(template).toMatchObject({ + Outputs: { + ExportsOutputFnGetAttOVERRIDELOGICALIDAtt76AC816F: { + Export: { + Name: 'Producer:ExportsOutputFnGetAttOVERRIDELOGICALIDAtt76AC816F', + }, + Value: { + 'Fn::GetAtt': [ + 'OVERRIDELOGICALID', + 'Att', + ], + }, + }, + }, + Resources: { + OVERRIDELOGICALID: { + Type: 'AWS::Resource', + }, + }, + }); + }); + + test('do not throw if overrideLogicalId is unresolved', () => { + // GIVEN: manual + const appM = new App(); + const producerM = new Stack(appM, 'Producer'); + const resourceM = new CfnResource(producerM, 'ResourceXXX', { type: 'AWS::Resource' }); + + // THEN - producers are the same + resourceM.overrideLogicalId(Lazy.string({ produce: () => 'INVALID_LOGICAL_ID' })); producerM.exportValue(resourceM.getAtt('Att')); const template = appM.synth().getStackByName(producerM.stackName).template; expect(template).toMatchObject({ Outputs: { - ExportsOutputFnGetAttOVERRIDELOGICALIDAtt2DD28019: { + ExportsOutputFnGetAttINVALIDLOGICALIDAtt6CB9E5B9: { Export: { - Name: 'Producer:ExportsOutputFnGetAttOVERRIDELOGICALIDAtt2DD28019', + Name: 'Producer:ExportsOutputFnGetAttINVALIDLOGICALIDAtt6CB9E5B9', }, Value: { 'Fn::GetAtt': [ - 'OVERRIDE_LOGICAL_ID', + 'INVALID_LOGICAL_ID', 'Att', ], }, }, }, Resources: { - OVERRIDE_LOGICAL_ID: { + INVALID_LOGICAL_ID: { Type: 'AWS::Resource', }, }, From d85ce7b922557573b837c86d3507141943b6a30e Mon Sep 17 00:00:00 2001 From: Sebastien Corbiere Date: Tue, 18 Jun 2024 14:36:35 -0700 Subject: [PATCH 3/6] Prevent the usage of an imported ASG with Cluster and ASGProvider --- packages/aws-cdk-lib/aws-ecs/lib/cluster.ts | 4 ++-- .../aws-cdk-lib/aws-ecs/test/cluster.test.ts | 21 ++++++++----------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/packages/aws-cdk-lib/aws-ecs/lib/cluster.ts b/packages/aws-cdk-lib/aws-ecs/lib/cluster.ts index b221e9eb84c55..df11bc1832e14 100644 --- a/packages/aws-cdk-lib/aws-ecs/lib/cluster.ts +++ b/packages/aws-cdk-lib/aws-ecs/lib/cluster.ts @@ -11,7 +11,7 @@ import * as kms from '../../aws-kms'; import * as logs from '../../aws-logs'; import * as s3 from '../../aws-s3'; import * as cloudmap from '../../aws-servicediscovery'; -import { Duration, IResource, Resource, Stack, Aspects, ArnFormat, IAspect, Token, Names, Annotations } from '../../core'; +import { Duration, IResource, Resource, Stack, Aspects, ArnFormat, IAspect, Token, Names } from '../../core'; const CLUSTER_SYMBOL = Symbol.for('@aws-cdk/aws-ecs/lib/cluster.Cluster'); @@ -1316,7 +1316,7 @@ export class AsgCapacityProvider extends Construct { if (this.autoScalingGroup instanceof autoscaling.AutoScalingGroup) { this.autoScalingGroup.protectNewInstancesFromScaleIn(); } else { - Annotations.of(this).addWarningV2('@aws-cdk/aws-ecs:cannotEnforceNewInstancesProtectedFromScaleIn', 'Cannot enforce `newInstancesProtectedFromScaleIn: true` on the AutoScalingGroup. This will have no effect if the AutoScalingGroup was created in this CDK app.'); + throw new Error('Cannot enable Managed Termination Protection on a Capacity Provider when providing an imported AutoScalingGroup.'); } } diff --git a/packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts b/packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts index bc6d8247b6108..347db96d03893 100644 --- a/packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts +++ b/packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts @@ -2227,25 +2227,21 @@ describe('cluster', () => { }); }); - test('with IAutoScalingGroup should emit a warning if Managed Termination Protection is enabled.', () => { + test('with IAutoScalingGroup should throw an error if Managed Termination Protection is enabled.', () => { // GIVEN const app = new cdk.App(); const stack = new cdk.Stack(app, 'test'); const autoScalingGroup = autoscaling.AutoScalingGroup.fromAutoScalingGroupName(stack, 'ASG', 'my-asg'); - // WHEN - new ecs.AsgCapacityProvider(stack, 'provider', { - autoScalingGroup, - }); - // THEN - expect(getWarnings(app.synth())).toEqual([{ - message: 'Cannot enforce `newInstancesProtectedFromScaleIn: true` on the AutoScalingGroup. This will have no effect if the AutoScalingGroup was created in this CDK app. [ack: @aws-cdk/aws-ecs:cannotEnforceNewInstancesProtectedFromScaleIn]', - path: '/test/provider', - }]); + expect(() => { + new ecs.AsgCapacityProvider(stack, 'provider', { + autoScalingGroup, + }); + }).toThrow('Cannot enable Managed Termination Protection on a Capacity Provider when providing an imported AutoScalingGroup.'); }); - test('with IAutoScalingGroup should not emit warning if Managed Termination Protection is disabled.', () => { + test('with IAutoScalingGroup should not throw an error if Managed Termination Protection is disabled.', () => { // GIVEN const app = new cdk.App(); const stack = new cdk.Stack(app, 'test'); @@ -2538,6 +2534,7 @@ describe('cluster', () => { const capacityProvider = new ecs.AsgCapacityProvider(stack, 'provider', { autoScalingGroup, + enableManagedTerminationProtection: false, }); // THEN expect(() => { @@ -3105,7 +3102,7 @@ describe('Accessing container instance role', function () { const addUserDataMock = jest.fn(); - function getAutoScalingGroup(stack: cdk.Stack) : autoscaling.AutoScalingGroup { + function getAutoScalingGroup(stack: cdk.Stack): autoscaling.AutoScalingGroup { const vpc = new ec2.Vpc(stack, 'Vpc'); const asg = new autoscaling.AutoScalingGroup(stack, 'asg', { vpc, From 197e8d8ed3285cb904bdd003956ae29d73973f21 Mon Sep 17 00:00:00 2001 From: Sebastien Corbiere Date: Thu, 20 Jun 2024 13:45:26 -0700 Subject: [PATCH 4/6] Fix the doc. --- packages/aws-cdk-lib/aws-ecs/lib/cluster.ts | 6 +++--- packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/aws-cdk-lib/aws-ecs/lib/cluster.ts b/packages/aws-cdk-lib/aws-ecs/lib/cluster.ts index df11bc1832e14..cfead1e8d7f5b 100644 --- a/packages/aws-cdk-lib/aws-ecs/lib/cluster.ts +++ b/packages/aws-cdk-lib/aws-ecs/lib/cluster.ts @@ -1181,9 +1181,9 @@ export interface AsgCapacityProviderProps extends AddAutoScalingGroupCapacityOpt /** * The autoscaling group to add as a Capacity Provider. * - * Warning: When passing an imported resource using `AutoScalingGroup.fromAutoScalingGroupName`, - * the AsgCapacityProvider construct will not be able to enforce the option `newInstancesProtectedFromScaleIn` of the AutoScalingGroup will be enabled. - * If this property is not enable in the AutoScalingGroup, the CFN stack execution will fail. + * Warning: When passing an imported resource using `AutoScalingGroup.fromAutoScalingGroupName` along with `enableManagedTerminationProtection: true`, + * the AsgCapacityProvider construct will not be able to enforce the option `newInstancesProtectedFromScaleIn` of the AutoScalingGroup. + * In this case the constructor of `AsgCapacityProvider` will throw an exception. */ readonly autoScalingGroup: autoscaling.IAutoScalingGroup; diff --git a/packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts b/packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts index 347db96d03893..9f44aecdaceea 100644 --- a/packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts +++ b/packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts @@ -2264,7 +2264,6 @@ describe('cluster', () => { ManagedTerminationProtection: 'DISABLED', }, }); - expect(getWarnings(app.synth())).toEqual([]); }); }); From e82136232404c3fa1da1ac625cdac73e7f0aeb4f Mon Sep 17 00:00:00 2001 From: Sebastien Corbiere Date: Fri, 21 Jun 2024 14:55:38 -0700 Subject: [PATCH 5/6] Some rewording to clarify the test. --- packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts b/packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts index 9f44aecdaceea..ee4850409a510 100644 --- a/packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts +++ b/packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts @@ -2524,15 +2524,15 @@ describe('cluster', () => { }); - test('should throw an error if the capacity provider was created using an imported AsgCapacityProvider', () => { + test('throws when calling Cluster.addAsgCapacityProvider with an AsgCapacityProvider created with an imported ASG', () => { // GIVEN const app = new cdk.App(); const stack = new cdk.Stack(app, 'test'); - const autoScalingGroup = autoscaling.AutoScalingGroup.fromAutoScalingGroupName(stack, 'ASG', 'my-asg'); + const importedAsg = autoscaling.AutoScalingGroup.fromAutoScalingGroupName(stack, 'ASG', 'my-asg'); const cluster = new ecs.Cluster(stack, 'EcsCluster'); const capacityProvider = new ecs.AsgCapacityProvider(stack, 'provider', { - autoScalingGroup, + autoScalingGroup: importedAsg, enableManagedTerminationProtection: false, }); // THEN From 7fd9c3c7e954a01f748c2454c050e04e731a9c48 Mon Sep 17 00:00:00 2001 From: CORBIERE Sebastien Date: Fri, 21 Jun 2024 14:58:05 -0700 Subject: [PATCH 6/6] Update cluster.ts doc string Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com> --- packages/aws-cdk-lib/aws-ecs/lib/cluster.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/aws-ecs/lib/cluster.ts b/packages/aws-cdk-lib/aws-ecs/lib/cluster.ts index cfead1e8d7f5b..26f476983ee07 100644 --- a/packages/aws-cdk-lib/aws-ecs/lib/cluster.ts +++ b/packages/aws-cdk-lib/aws-ecs/lib/cluster.ts @@ -1182,7 +1182,7 @@ export interface AsgCapacityProviderProps extends AddAutoScalingGroupCapacityOpt * The autoscaling group to add as a Capacity Provider. * * Warning: When passing an imported resource using `AutoScalingGroup.fromAutoScalingGroupName` along with `enableManagedTerminationProtection: true`, - * the AsgCapacityProvider construct will not be able to enforce the option `newInstancesProtectedFromScaleIn` of the AutoScalingGroup. + * the `AsgCapacityProvider` construct will not be able to enforce the option `newInstancesProtectedFromScaleIn` of the `AutoScalingGroup`. * In this case the constructor of `AsgCapacityProvider` will throw an exception. */ readonly autoScalingGroup: autoscaling.IAutoScalingGroup;