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

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
  • Loading branch information
azatoth committed Nov 29, 2022
1 parent 52ef046 commit 89f051a
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 4 deletions.
18 changes: 14 additions & 4 deletions packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts
Expand Up @@ -961,6 +961,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 hasSetScaleOnRequest: boolean = false;

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

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

Expand Down Expand Up @@ -1363,6 +1365,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 @@ -1390,10 +1394,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 @@ -1740,6 +1740,16 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
};
}
}


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

return errors;
}
}

/**
Expand Down
38 changes: 38 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 { 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 @@ -1845,6 +1846,43 @@ describe('auto scaling group', () => {
})).not.toThrow();

});

test('Should validate multiple target groups i conjunction with `setScaleOnRequest()`', () => {
const stack = new cdk.Stack(undefined, 'MyStack', { env: { region: 'us-east-1', account: '1234' } });


const vpc = mockVpc(stack);

const alb = new ApplicationLoadBalancer(stack, 'alb', {
vpc,
internetFacing: true,
});

const listener = alb.addListener('Listener', {
port: 80,
open: true,
});
const asg = new autoscaling.AutoScalingGroup(stack, 'MyFleet', {
instanceType: ec2.InstanceType.of(ec2.InstanceClass.M4, ec2.InstanceSize.MICRO),
machineImage: new ec2.AmazonLinuxImage(),
vpc,
});

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([]);

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

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

});
});

function mockVpc(stack: cdk.Stack) {
Expand Down

0 comments on commit 89f051a

Please sign in to comment.