Skip to content

Commit

Permalink
fix(autoscaling): StepScalingPolicy intervals not checked for going o…
Browse files Browse the repository at this point in the history
…ver 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*
  • Loading branch information
scanlonp authored Jul 25, 2023
1 parent dc287e5 commit 58b004e
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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[];

Expand Down Expand Up @@ -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}`);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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[];

Expand Down Expand Up @@ -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;

Expand Down
54 changes: 54 additions & 0 deletions packages/aws-cdk-lib/aws-autoscaling/test/scaling.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit 58b004e

Please sign in to comment.