From 58b004ef7385cfb42910b6978b4b5b836cbb69f7 Mon Sep 17 00:00:00 2001 From: Parker Scanlon <69879391+scanlonp@users.noreply.github.com> Date: Tue, 25 Jul 2023 14:45:46 -0700 Subject: [PATCH] fix(autoscaling): StepScalingPolicy intervals not checked for going over allowable maximum (#26490) `StepScalingPolicy` did not have a validation that the number of intervals in `scalingSteps` was in the allowable range. The [Autoscaling documentation](https://docs.aws.amazon.com/autoscaling/ec2/userguide/ec2-auto-scaling-quotas.html) states 20 is the maximum step changes in a policy, and since autoscaling creates 2 policies (an [UpperPolicy](https://github.com/aws/aws-cdk/blob/bc029fe5ac69a8b7fd2dfdbcd8834e9a2cf8e000/packages/aws-cdk-lib/aws-autoscaling/lib/step-scaling-policy.ts#L136-L166) and a [LowerPolicy](https://github.com/aws/aws-cdk/blob/bc029fe5ac69a8b7fd2dfdbcd8834e9a2cf8e000/packages/aws-cdk-lib/aws-autoscaling/lib/step-scaling-policy.ts#L105-L134)), our cap is 40. There is an identical change in Application Autoscaling. Closes #26215. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../lib/step-scaling-policy.ts | 6 +++ .../test/step-scaling-policy.test.ts | 41 ++++++++++++++ .../lib/step-scaling-policy.ts | 6 +++ .../aws-autoscaling/test/scaling.test.ts | 54 +++++++++++++++++++ 4 files changed, 107 insertions(+) diff --git a/packages/aws-cdk-lib/aws-applicationautoscaling/lib/step-scaling-policy.ts b/packages/aws-cdk-lib/aws-applicationautoscaling/lib/step-scaling-policy.ts index 1ce505c57c6ba..a2afba5bc480b 100644 --- a/packages/aws-cdk-lib/aws-applicationautoscaling/lib/step-scaling-policy.ts +++ b/packages/aws-cdk-lib/aws-applicationautoscaling/lib/step-scaling-policy.ts @@ -15,6 +15,8 @@ export interface BasicStepScalingPolicyProps { * The intervals for scaling. * * Maps a range of metric values to a particular scaling behavior. + * + * Must be between 2 and 40 steps. */ readonly scalingSteps: ScalingInterval[]; @@ -111,6 +113,10 @@ export class StepScalingPolicy extends Construct { throw new Error('You must supply at least 2 intervals for autoscaling'); } + if (props.scalingSteps.length > 40) { + throw new Error(`'scalingSteps' can have at most 40 steps, got ${props.scalingSteps.length}`); + } + if (props.datapointsToAlarm !== undefined && props.datapointsToAlarm < 1) { throw new RangeError(`datapointsToAlarm cannot be less than 1, got: ${props.datapointsToAlarm}`); } diff --git a/packages/aws-cdk-lib/aws-applicationautoscaling/test/step-scaling-policy.test.ts b/packages/aws-cdk-lib/aws-applicationautoscaling/test/step-scaling-policy.test.ts index 2e53e9062c6ef..f22cbeec130d2 100644 --- a/packages/aws-cdk-lib/aws-applicationautoscaling/test/step-scaling-policy.test.ts +++ b/packages/aws-cdk-lib/aws-applicationautoscaling/test/step-scaling-policy.test.ts @@ -273,6 +273,47 @@ describe('step scaling policy', () => { }); }).toThrow('datapointsToAlarm cannot be less than 1, got: 0'); }); + + test('scalingSteps must have at least 2 steps', () => { + // GIVEN + const stack = new cdk.Stack(); + const target = createScalableTarget(stack); + + expect(() => { + target.scaleOnMetric('Tracking', { + metric: new cloudwatch.Metric({ namespace: 'Test', metricName: 'Metric' }), + scalingSteps: [ + { lower: 0, upper: 2, change: +1 }, + ], + }); + }).toThrow(/must supply at least 2/); + }); + + test('scalingSteps has a maximum of 40 steps', () => { + // GIVEN + const stack = new cdk.Stack(); + const target = createScalableTarget(stack); + + const numSteps = 41; + const messagesPerTask = 20; + let steps: appscaling.ScalingInterval[] = []; + + for (let i = 0; i < numSteps; ++i) { + const step: appscaling.ScalingInterval = { + lower: i * messagesPerTask, + upper: i * (messagesPerTask + 1) - 1, + change: i + 1, + }; + steps.push(step); + } + + expect(() => { + target.scaleOnMetric('Tracking', { + metric: new cloudwatch.Metric({ namespace: 'Test', metricName: 'Metric' }), + scalingSteps: steps, + }); + }).toThrow('\'scalingSteps\' can have at most 40 steps, got 41'); + }); }); /** diff --git a/packages/aws-cdk-lib/aws-autoscaling/lib/step-scaling-policy.ts b/packages/aws-cdk-lib/aws-autoscaling/lib/step-scaling-policy.ts index 1da657f8aae5e..1e467ada652c5 100644 --- a/packages/aws-cdk-lib/aws-autoscaling/lib/step-scaling-policy.ts +++ b/packages/aws-cdk-lib/aws-autoscaling/lib/step-scaling-policy.ts @@ -15,6 +15,8 @@ export interface BasicStepScalingPolicyProps { * The intervals for scaling. * * Maps a range of metric values to a particular scaling behavior. + * + * Must be between 2 and 40 steps. */ readonly scalingSteps: ScalingInterval[]; @@ -96,6 +98,10 @@ export class StepScalingPolicy extends Construct { throw new Error('You must supply at least 2 intervals for autoscaling'); } + if (props.scalingSteps.length > 40) { + throw new Error(`'scalingSteps' can have at most 40 steps, got ${props.scalingSteps.length}`); + } + const adjustmentType = props.adjustmentType || AdjustmentType.CHANGE_IN_CAPACITY; const changesAreAbsolute = adjustmentType === AdjustmentType.EXACT_CAPACITY; diff --git a/packages/aws-cdk-lib/aws-autoscaling/test/scaling.test.ts b/packages/aws-cdk-lib/aws-autoscaling/test/scaling.test.ts index cc748c580abf7..f99b132087382 100644 --- a/packages/aws-cdk-lib/aws-autoscaling/test/scaling.test.ts +++ b/packages/aws-cdk-lib/aws-autoscaling/test/scaling.test.ts @@ -327,6 +327,60 @@ test('step scaling with evaluation period configured', () => { }); }); +describe('step-scaling-policy scalingSteps length validation checks', () => { + test('scalingSteps must have at least 2 steps', () => { + // GIVEN + const stack = new cdk.Stack(); + const fixture = new ASGFixture(stack, 'Fixture'); + + expect(() => { + fixture.asg.scaleOnMetric('Metric', { + metric: new cloudwatch.Metric({ + metricName: 'Legs', + namespace: 'Henk', + dimensionsMap: { Mustache: 'Bushy' }, + }), + estimatedInstanceWarmup: cdk.Duration.seconds(150), + // only one scaling step throws an error. + scalingSteps: [ + { lower: 0, upper: 2, change: +1 }, + ], + }); + }).toThrow(/must supply at least 2/); + }); + + test('scalingSteps has a maximum of 40 steps', () => { + // GIVEN + const stack = new cdk.Stack(); + const fixture = new ASGFixture(stack, 'Fixture'); + + const numSteps = 41; + const messagesPerTask = 20; + let steps: autoscaling.ScalingInterval[] = []; + + for (let i = 0; i < numSteps; ++i) { + const step: autoscaling.ScalingInterval = { + lower: i * messagesPerTask, + upper: i * (messagesPerTask + 1) - 1, + change: i + 1, + }; + steps.push(step); + } + + expect(() => { + fixture.asg.scaleOnMetric('Metric', { + metric: new cloudwatch.Metric({ + metricName: 'Legs', + namespace: 'Henk', + dimensionsMap: { Mustache: 'Bushy' }, + }), + estimatedInstanceWarmup: cdk.Duration.seconds(150), + scalingSteps: steps, + }); + }).toThrow('\'scalingSteps\' can have at most 40 steps, got 41'); + }); +}); + class ASGFixture extends Construct { public readonly vpc: ec2.Vpc; public readonly asg: autoscaling.AutoScalingGroup;