Skip to content

Commit

Permalink
fix(autoscaling): Allow adding AutoScalingGroup to multiple target gr…
Browse files Browse the repository at this point in the history
…oups (aws#23044)

While this doesn't solve the underlying problem, which will require an API breakage in `scaleOnRequestCount()` (see
aws#5667 (comment)).

As `scaleOnRequestCount()` seems to be the only part that is affected by this, we'll only issue a validation error if we have multiple `targetGroupArns` and `scaleOnRequestCount()` has been called.

closes aws#5667


----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] 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*
  • Loading branch information
azatoth authored and Brennan Ho committed Feb 22, 2023
1 parent 539b065 commit 532b230
Show file tree
Hide file tree
Showing 13 changed files with 2,495 additions and 56 deletions.
18 changes: 14 additions & 4 deletions packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts
Expand Up @@ -985,6 +985,7 @@ abstract class AutoScalingGroupBase extends Resource implements IAutoScalingGrou
public abstract readonly osType: ec2.OperatingSystemType;
protected albTargetGroup?: elbv2.ApplicationTargetGroup;
public readonly grantPrincipal: iam.IPrincipal = new iam.UnknownPrincipal({ resource: this });
protected hasCalledScaleOnRequestCount: boolean = false;

/**
* Send a message to either an SQS queue or SNS topic when instances launch or terminate
Expand Down Expand Up @@ -1088,6 +1089,7 @@ abstract class AutoScalingGroupBase extends Resource implements IAutoScalingGrou
});

policy.node.addDependency(this.albTargetGroup.loadBalancerAttached);
this.hasCalledScaleOnRequestCount = true;
return policy;
}

Expand Down Expand Up @@ -1388,6 +1390,8 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
if (props.requireImdsv2) {
Aspects.of(this).add(new AutoScalingGroupRequireImdsv2Aspect());
}

this.node.addValidation({ validate: () => this.validateTargetGroup() });
}

/**
Expand Down Expand Up @@ -1415,10 +1419,6 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
* Attach to ELBv2 Application Target Group
*/
public attachToApplicationTargetGroup(targetGroup: elbv2.IApplicationTargetGroup): elbv2.LoadBalancerTargetProps {
if (this.albTargetGroup !== undefined) {
throw new Error('Cannot add AutoScalingGroup to 2nd Target Group');
}

this.targetGroupArns.push(targetGroup.targetGroupArn);
if (targetGroup instanceof elbv2.ApplicationTargetGroup) {
// Copy onto self if it's a concrete type. We need this for autoscaling
Expand Down Expand Up @@ -1765,6 +1765,16 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
};
}
}


private validateTargetGroup(): string[] {
const errors = new Array<string>();
if (this.hasCalledScaleOnRequestCount && this.targetGroupArns.length > 1) {
errors.push('Cannon use multiple target groups if `scaleOnRequestCount()` is being used.');
}

return errors;
}
}

/**
Expand Down
3 changes: 2 additions & 1 deletion packages/@aws-cdk/aws-autoscaling/package.json
Expand Up @@ -82,10 +82,11 @@
"devDependencies": {
"@aws-cdk/assertions": "0.0.0",
"@aws-cdk/cdk-build-tools": "0.0.0",
"@aws-cdk/integ-runner": "0.0.0",
"@aws-cdk/cfn2ts": "0.0.0",
"@aws-cdk/cloud-assembly-schema": "0.0.0",
"@aws-cdk/cx-api": "0.0.0",
"@aws-cdk/integ-runner": "0.0.0",
"@aws-cdk/integ-tests": "0.0.0",
"@aws-cdk/pkglint": "0.0.0",
"@types/jest": "^27.5.2",
"jest": "^27.5.1"
Expand Down
56 changes: 56 additions & 0 deletions packages/@aws-cdk/aws-autoscaling/test/auto-scaling-group.test.ts
Expand Up @@ -2,6 +2,7 @@ import { Annotations, Match, Template } from '@aws-cdk/assertions';
import * as cloudwatch from '@aws-cdk/aws-cloudwatch';
import * as ec2 from '@aws-cdk/aws-ec2';
import { AmazonLinuxCpuType, AmazonLinuxGeneration, AmazonLinuxImage, InstanceType, LaunchTemplate } from '@aws-cdk/aws-ec2';
import { ApplicationListener, ApplicationLoadBalancer, ApplicationTargetGroup } from '@aws-cdk/aws-elasticloadbalancingv2';
import * as iam from '@aws-cdk/aws-iam';
import * as sns from '@aws-cdk/aws-sns';
import { testDeprecated } from '@aws-cdk/cdk-build-tools';
Expand Down Expand Up @@ -1864,6 +1865,61 @@ describe('auto scaling group', () => {
})).not.toThrow();

});

describe('multiple target groups', () => {
let asg: autoscaling.AutoScalingGroup;
let stack: cdk.Stack;
let vpc: ec2.IVpc;
let alb: ApplicationLoadBalancer;
let listener: ApplicationListener;

beforeEach(() => {
stack = new cdk.Stack(undefined, 'MyStack', { env: { region: 'us-east-1', account: '1234' } });
vpc = mockVpc(stack);
alb = new ApplicationLoadBalancer(stack, 'alb', {
vpc,
internetFacing: true,
});

listener = alb.addListener('Listener', {
port: 80,
open: true,
});

asg = new autoscaling.AutoScalingGroup(stack, 'MyFleet', {
instanceType: ec2.InstanceType.of(ec2.InstanceClass.M4, ec2.InstanceSize.MICRO),
machineImage: new ec2.AmazonLinuxImage(),
vpc,
});
});

test('Adding two application target groups should succeed validation', () => {
const atg1 = new ApplicationTargetGroup(stack, 'ATG1', { port: 443 });
const atg2 = new ApplicationTargetGroup(stack, 'ATG2', { port: 443 });

listener.addTargetGroups('tgs', { targetGroups: [atg1, atg2] });

asg.attachToApplicationTargetGroup(atg1);
asg.attachToApplicationTargetGroup(atg2);

expect(asg.node.validate()).toEqual([]);
});

test('Adding two application target groups should fail validation validate if `scaleOnRequestCount()` has been called', () => {
const atg1 = new ApplicationTargetGroup(stack, 'ATG1', { port: 443 });
const atg2 = new ApplicationTargetGroup(stack, 'ATG2', { port: 443 });

listener.addTargetGroups('tgs', { targetGroups: [atg1, atg2] });

asg.attachToApplicationTargetGroup(atg1);
asg.attachToApplicationTargetGroup(atg2);

asg.scaleOnRequestCount('requests-per-minute', { targetRequestsPerMinute: 60 });

expect(asg.node.validate()).toContainEqual('Cannon use multiple target groups if `scaleOnRequestCount()` is being used.');
});
});

});

function mockVpc(stack: cdk.Stack) {
Expand Down
@@ -0,0 +1,19 @@
{
"version": "22.0.0",
"files": {
"21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22": {
"source": {
"path": "LambdaTestDefaultTestDeployAssert1AF2B360.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
}
},
"dockerImages": {}
}
@@ -0,0 +1,36 @@
{
"Parameters": {
"BootstrapVersion": {
"Type": "AWS::SSM::Parameter::Value<String>",
"Default": "/cdk-bootstrap/hnb659fds/version",
"Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]"
}
},
"Rules": {
"CheckBootstrapVersion": {
"Assertions": [
{
"Assert": {
"Fn::Not": [
{
"Fn::Contains": [
[
"1",
"2",
"3",
"4",
"5"
],
{
"Ref": "BootstrapVersion"
}
]
}
]
},
"AssertDescription": "CDK bootstrap stack version 6 required. Please run 'cdk bootstrap' with a recent version of the CDK CLI."
}
]
}
}
}
@@ -0,0 +1,19 @@
{
"version": "22.0.0",
"files": {
"6e4ba6d1c10cb8a0be78abd30ef94d231f8913f1cd4b02c0b62b8626f3425c00": {
"source": {
"path": "aws-cdk-asg-atg-integ.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "6e4ba6d1c10cb8a0be78abd30ef94d231f8913f1cd4b02c0b62b8626f3425c00.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
}
},
"dockerImages": {}
}

0 comments on commit 532b230

Please sign in to comment.