Skip to content

Commit

Permalink
fix(ecs): let AsgCapacityProvider use IAutoScalingGroup only when Man…
Browse files Browse the repository at this point in the history 
…aged Termination Protection is disable (#30335)

Let AsgCapacityProvider use IAutoScalingGroup only when Managed Termination Protection is disable.

The code will throw an exception with a clear message when the user specify a self managed ASG using `AutoScalingGroup.fromAutoScalingGroupName` and let the Managed Termination Protection enabled.

It will also throw a clear exception when calling `Cluster.addAsgCapacityProvider` with an `AsgCapacityProvider` created with an imported ASG.

### Issue # (if applicable)

Closes #29174.

### Reason for this change

As there is no clear fix to the original issue, this change's purpose it to bring clarity to the users about what is not allowed when using the L2 Constructs `AsgCapacityProvider` and `Cluster` with an imported ASG.

### Description of changes

This change will replace non explicit exception, caused by missing methods, by clear error messages.

### Description of how you validated changes

Added unit tests.

### 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*
  • Loading branch information
@scorbiere
scorbiere committed Jun 25, 2024
1 parent 8ebfade commit efee07d
Show file tree
Hide file tree
Showing 2 changed files with 113 additions and 32 deletions.
13 changes: 12 additions & 1 deletion packages/aws-cdk-lib/aws-ecs/lib/cluster.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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` 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;

Expand Down Expand Up @@ -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 {
throw new Error('Cannot enable Managed Termination Protection on a Capacity Provider when providing an imported AutoScalingGroup.');
}
}

const capacityProviderNameRegex = /^(?!aws|ecs|fargate).+/gm;
Expand Down
132 changes: 101 additions & 31 deletions packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -2194,36 +2195,76 @@ 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 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');

// THEN
expect(() => {
new ecs.AsgCapacityProvider(stack, 'provider', {
autoScalingGroup,
});
}).toThrow('Cannot enable Managed Termination Protection on a Capacity Provider when providing an imported AutoScalingGroup.');
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::ECS::CapacityProvider', {
AutoScalingGroupProvider: {
AutoScalingGroupArn: {
Ref: 'asgASG4D014670',
},
ManagedScaling: {
Status: 'ENABLED',
TargetCapacity: 100,
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');
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',
},
});
});

});

test('can disable Managed Scaling and Managed Termination Protection for ASG capacity provider', () => {
Expand Down Expand Up @@ -2483,6 +2524,23 @@ describe('cluster', () => {

});

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 importedAsg = autoscaling.AutoScalingGroup.fromAutoScalingGroupName(stack, 'ASG', 'my-asg');
const cluster = new ecs.Cluster(stack, 'EcsCluster');

const capacityProvider = new ecs.AsgCapacityProvider(stack, 'provider', {
autoScalingGroup: importedAsg,
enableManagedTerminationProtection: false,
});
// 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();
Expand Down Expand Up @@ -3042,11 +3100,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();
Expand All @@ -3057,11 +3121,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);
Expand All @@ -3077,10 +3142,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, {
Expand All @@ -3098,6 +3164,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', {
Expand All @@ -3118,6 +3185,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', {
Expand All @@ -3140,6 +3208,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', {
Expand All @@ -3162,6 +3231,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', {
Expand Down

0 comments on commit efee07d

Please sign in to comment.