-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(apprunner): add AutoScalingConfiguration for AppRunner Service #30358
Changes from 19 commits
0a5114b
bdb85cb
4ada3f6
7e2c80f
7ed5abc
61a1b00
84a3cce
55501a7
e999f83
59a6f32
215e5a9
b161ed7
50fc0d3
0cac270
f46a7c7
9bb0dcb
3ca5979
2c74142
0aef49e
361ab63
b8c7d61
2633a0d
2c5e154
5a88d1d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,196 @@ | ||||
import * as cdk from 'aws-cdk-lib/core'; | ||||
import { Construct } from 'constructs'; | ||||
import { CfnAutoScalingConfiguration } from 'aws-cdk-lib/aws-apprunner'; | ||||
|
||||
/** | ||||
* Properties of the App Runner Auto Scaling Configuration. | ||||
*/ | ||||
export interface AutoScalingConfigurationProps { | ||||
/** | ||||
* The name for the Auto Scaling Configuration. | ||||
* | ||||
* @default - a name generated by CloudFormation | ||||
*/ | ||||
readonly autoScalingConfigurationName?: string; | ||||
|
||||
/** | ||||
* The maximum number of concurrent requests that an instance processes. | ||||
* If the number of concurrent requests exceeds this limit, App Runner scales the service up. | ||||
* | ||||
* Must be between 1 and 200. | ||||
* | ||||
* @default 100 | ||||
*/ | ||||
readonly maxConcurrency?: number; | ||||
|
||||
/** | ||||
* The maximum number of instances that a service scales up to. | ||||
* At most maxSize instances actively serve traffic for your service. | ||||
* | ||||
* Must be between 1 and 25. | ||||
* | ||||
* @default 25 | ||||
*/ | ||||
readonly maxSize?: number; | ||||
|
||||
/** | ||||
* The minimum number of instances that App Runner provisions for a service. | ||||
* The service always has at least minSize provisioned instances. | ||||
* | ||||
* | ||||
* Must be between 1 and 25. | ||||
* | ||||
* @default 1 | ||||
*/ | ||||
readonly minSize?: number; | ||||
} | ||||
|
||||
/** | ||||
* Attributes for the App Runner Auto Scaling Configuration. | ||||
*/ | ||||
export interface AutoScalingConfigurationAttributes { | ||||
/** | ||||
* The name of the Auto Scaling Configuration. | ||||
*/ | ||||
readonly autoScalingConfigurationName: string; | ||||
|
||||
/** | ||||
* The revision of the Auto Scaling Configuration. | ||||
*/ | ||||
readonly autoScalingConfigurationRevision: number; | ||||
} | ||||
|
||||
/** | ||||
* Represents the App Runner Auto Scaling Configuration. | ||||
*/ | ||||
export interface IAutoScalingConfiguration extends cdk.IResource { | ||||
/** | ||||
* The ARN of the Auto Scaling Configuration. | ||||
* @attribute | ||||
*/ | ||||
readonly autoScalingConfigurationArn: string; | ||||
|
||||
/** | ||||
* The Name of the Auto Scaling Configuration. | ||||
* @attribute | ||||
*/ | ||||
readonly autoScalingConfigurationName: string; | ||||
|
||||
/** | ||||
* The revision of the Auto Scaling Configuration. | ||||
* @attribute | ||||
*/ | ||||
readonly autoScalingConfigurationRevision: number; | ||||
} | ||||
|
||||
/** | ||||
* The App Runner Auto Scaling Configuration. | ||||
* | ||||
* @resource AWS::AppRunner::AutoScalingConfiguration | ||||
*/ | ||||
export class AutoScalingConfiguration extends cdk.Resource implements IAutoScalingConfiguration { | ||||
/** | ||||
* Imports an App Runner Auto Scaling Configuration from attributes | ||||
*/ | ||||
public static fromAutoScalingConfigurationAttributes(scope: Construct, id: string, | ||||
attrs: AutoScalingConfigurationAttributes): IAutoScalingConfiguration { | ||||
const autoScalingConfigurationName = attrs.autoScalingConfigurationName; | ||||
const autoScalingConfigurationRevision = attrs.autoScalingConfigurationRevision; | ||||
|
||||
class Import extends cdk.Resource implements IAutoScalingConfiguration { | ||||
public readonly autoScalingConfigurationName = autoScalingConfigurationName; | ||||
public readonly autoScalingConfigurationRevision = autoScalingConfigurationRevision; | ||||
public readonly autoScalingConfigurationArn = cdk.Stack.of(this).formatArn({ | ||||
resource: 'autoscalingconfiguration', | ||||
service: 'apprunner', | ||||
resourceName: `${attrs.autoScalingConfigurationName}/${attrs.autoScalingConfigurationRevision}`, | ||||
}); | ||||
} | ||||
|
||||
return new Import(scope, id); | ||||
} | ||||
|
||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we just know the Name and Revision, we can formate the Arn string. How about just have Name and Revision in the interface: export interface AutoScalingConfigurationAttributes {
/**
* The name of the Auto Scaling Configuration.
*/
readonly autoScalingConfigurationName: string;
/**
* The revision of the Auto Scaling Configuration.
*/
readonly autoScalingConfigurationRevision: number;
} And in the fromXxx method: public static fromAutoScalingConfigurationAttributes(scope: Construct, id: string,
attrs: AutoScalingConfigurationAttributes): IAutoScalingConfiguration {
class Import extends cdk.Resource implements IAutoScalingConfiguration {
public readonly autoScalingConfigurationName = attrs.autoScalingConfigurationName;
public readonly autoScalingConfigurationRevision = attrs.autoScalingConfigurationRevision;
public readonly autoScalingConfigurationArn = Stack.of(this).formatArn({
resource: 'autoscalingconfiguration',
service: 'apprunner',
resourceName: `${attrs.autoScalingConfigurationName}/${attrs.autoScalingConfigurationRevision}`,
})
}
return new Import(scope, id);
}; If we can generate the Arn from given Name and Revision then Arn would not be required in the Attributes? Think about it, if user already knows the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you. You are certainly right. I have removed arn from the attributes and made the correction. |
||||
/** | ||||
* Imports an App Runner Auto Scaling Configuration from its ARN | ||||
*/ | ||||
public static fromArn(scope: Construct, id: string, autoScalingConfigurationArn: string): IAutoScalingConfiguration { | ||||
const resourceParts = cdk.Fn.split('/', autoScalingConfigurationArn); | ||||
|
||||
if (!resourceParts || resourceParts.length < 3) { | ||||
throw new Error(`Unexpected ARN format: ${autoScalingConfigurationArn}`); | ||||
} | ||||
|
||||
const autoScalingConfigurationName = cdk.Fn.select(0, resourceParts); | ||||
const autoScalingConfigurationRevision = Number(cdk.Fn.select(1, resourceParts)); | ||||
|
||||
class Import extends cdk.Resource implements IAutoScalingConfiguration { | ||||
public readonly autoScalingConfigurationName = autoScalingConfigurationName; | ||||
public readonly autoScalingConfigurationRevision = autoScalingConfigurationRevision; | ||||
public readonly autoScalingConfigurationArn = autoScalingConfigurationArn; | ||||
} | ||||
|
||||
return new Import(scope, id); | ||||
} | ||||
|
||||
/** | ||||
* The ARN of the Auto Scaling Configuration. | ||||
* @attribute | ||||
*/ | ||||
readonly autoScalingConfigurationArn: string; | ||||
|
||||
/** | ||||
* The name of the Auto Scaling Configuration. | ||||
* @attribute | ||||
*/ | ||||
readonly autoScalingConfigurationName: string; | ||||
|
||||
/** | ||||
* The revision of the Auto Scaling Configuration. | ||||
* @attribute | ||||
*/ | ||||
readonly autoScalingConfigurationRevision: number; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we probably should make it
Also in the doc
It's unclear to me if it would be number. I am afraid some day it might support revision like With that being said, I don't see the benefits using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you. Personally, I'm in favor of making the revision a string. The revision in the Cfn (L1 construct) is a number, but would it be appropriate to change it to a string in the L2 construct? I'm not sure if there's a possibility of it changing, so if you know anything about it, please let me know. Additionally, since the already implemented VpcConnector treats the revision as a number, I'm also concerned about having a different approach from that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the callout. I checked the cfnspec and yes the type is Integer, which is number in TS.
I am fine having it as @GavinZZ @paulhcsun what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed with team internally. I think we don't need to over-optimize the type here. CFN schema type changes are not supposed to happen especially after it's already in CloudFormation resource schema registry. |
||||
|
||||
public constructor(scope: Construct, id: string, props: AutoScalingConfigurationProps = {}) { | ||||
super(scope, id, { | ||||
physicalName: props.autoScalingConfigurationName, | ||||
}); | ||||
|
||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also add validation for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I added validation and unit tests. |
||||
if ( | ||||
props.autoScalingConfigurationName !== undefined && | ||||
!cdk.Token.isUnresolved(props.autoScalingConfigurationName) && | ||||
!/^[A-Za-z0-9][A-Za-z0-9\-_]{3,31}$/.test(props.autoScalingConfigurationName) | ||||
) { | ||||
throw new Error(`autoScalingConfigurationName must match the ^[A-Za-z0-9][A-Za-z0-9\-_]{3,31}$ pattern, got ${props.autoScalingConfigurationName}`); | ||||
} | ||||
|
||||
const isMinSizeDefined = props.minSize !== undefined && !cdk.Token.isUnresolved(props.minSize); | ||||
const isMaxSizeDefined = props.maxSize !== undefined && !cdk.Token.isUnresolved(props.maxSize); | ||||
if (isMinSizeDefined && (props.minSize < 1 || props.minSize > 25)) { | ||||
throw new Error(`minSize must be between 1 and 25, got ${props.minSize}`); | ||||
} | ||||
if (isMaxSizeDefined && (props.maxSize < 1 || props.maxSize > 25)) { | ||||
throw new Error(`maxSize must be between 1 and 25, got ${props.maxSize}`); | ||||
} | ||||
if (isMinSizeDefined && isMaxSizeDefined && !(props.minSize < props.maxSize)) { | ||||
throw new Error('maxSize must be greater than minSize'); | ||||
} | ||||
if ( | ||||
props.maxConcurrency !== undefined && | ||||
!cdk.Token.isUnresolved(props.maxConcurrency) && | ||||
(props.maxConcurrency < 1 || props.maxConcurrency > 200) | ||||
) { | ||||
throw new Error(`maxConcurrency must be between 1 and 200, got ${props.maxConcurrency}`); | ||||
} | ||||
|
||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider the code like this function validateAutoScalingConfiguration(props: {
minSize?: number;
maxSize?: number;
maxConcurrency?: number;
}) {
const isMinSizeDefined = typeof props.minSize === 'number';
const isMaxSizeDefined = typeof props.maxSize === 'number';
const isMaxConcurrencyDefined = typeof props.maxConcurrency === 'number';
if (isMinSizeDefined && (props.minSize < 1 || props.minSize > 25)) {
throw new Error(`minSize must be between 1 and 25, got ${props.minSize}`);
}
if (isMaxSizeDefined && (props.maxSize < 1 || props.maxSize > 25)) {
throw new Error(`maxSize must be between 1 and 25, got ${props.maxSize}`);
}
if (isMinSizeDefined && isMaxSizeDefined && !(props.minSize < props.maxSize)) {
throw new Error('maxSize must be greater than minSize');
}
if (isMaxConcurrencyDefined && (props.maxConcurrency < 1 || props.maxConcurrency > 200)) {
throw new Error(`maxConcurrency must be between 1 and 200, got ${props.maxConcurrency}`);
}
} The main changes are:
typeof props.minSize === 'number' && !cdk.Token.isUnresolved(props.minSize);
There are a few benefits to using
In summary, the This optimized version of the code is more concise, easier to read, and easier to maintain. It also follows best practices for input validation, such as using clear error messages and keeping the validation logic separate from the main application logic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would consider to make it a construct private method and use something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I like your suggested your approach. |
||||
const resource = new CfnAutoScalingConfiguration(this, 'Resource', { | ||||
autoScalingConfigurationName: props.autoScalingConfigurationName, | ||||
maxConcurrency: props.maxConcurrency, | ||||
maxSize: props.maxSize, | ||||
minSize: props.minSize, | ||||
}); | ||||
|
||||
this.autoScalingConfigurationArn = resource.attrAutoScalingConfigurationArn; | ||||
this.autoScalingConfigurationRevision = resource.attrAutoScalingConfigurationRevision; | ||||
this.autoScalingConfigurationName = resource.ref; | ||||
} | ||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
// AWS::AppRunner CloudFormation Resources: | ||
export * from './auto-scaling-configuration'; | ||
export * from './service'; | ||
export * from './vpc-connector'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ import { Lazy } from 'aws-cdk-lib/core'; | |
import { Construct } from 'constructs'; | ||
import { CfnService } from 'aws-cdk-lib/aws-apprunner'; | ||
import { IVpcConnector } from './vpc-connector'; | ||
import { IAutoScalingConfiguration } from './auto-scaling-configuration'; | ||
|
||
/** | ||
* The image repository types | ||
|
@@ -656,6 +657,18 @@ export interface ServiceProps { | |
*/ | ||
readonly autoDeploymentsEnabled?: boolean; | ||
|
||
/** | ||
* Specifies an App Runner Auto Scaling Configuration. | ||
* | ||
* A default configuration is either the AWS recommended configuration, | ||
* or the configuration you set as the default. | ||
* | ||
* @see https://docs.aws.amazon.com/apprunner/latest/dg/manage-autoscaling.html | ||
* | ||
* @default - the latest revision of a default auto scaling configuration is used. | ||
*/ | ||
readonly autoScalingConfiguration?: IAutoScalingConfiguration; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you. I will keep it as is, in the form of aligning with the class names or property names of the L1 construct. |
||
|
||
/** | ||
* The number of CPU units reserved for each instance of your App Runner service. | ||
* | ||
|
@@ -1272,6 +1285,7 @@ export class Service extends cdk.Resource implements iam.IGrantable { | |
encryptionConfiguration: this.props.kmsKey ? { | ||
kmsKey: this.props.kmsKey.keyArn, | ||
} : undefined, | ||
autoScalingConfigurationArn: this.props.autoScalingConfiguration?.autoScalingConfigurationArn, | ||
networkConfiguration: { | ||
egressConfiguration: { | ||
egressType: this.props.vpcConnector ? 'VPC' : 'DEFAULT', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
import { Match, Template } from 'aws-cdk-lib/assertions'; | ||
import * as cdk from 'aws-cdk-lib'; | ||
import { AutoScalingConfiguration } from '../lib'; | ||
|
||
let stack: cdk.Stack; | ||
beforeEach(() => { | ||
stack = new cdk.Stack(); | ||
}); | ||
|
||
test.each([ | ||
['MyAutoScalingConfiguration'], | ||
['my-autoscaling-configuration_1'], | ||
])('create an Auto scaling Configuration with all properties (name: %s)', (autoScalingConfigurationName: string) => { | ||
// WHEN | ||
new AutoScalingConfiguration(stack, 'AutoScalingConfiguration', { | ||
autoScalingConfigurationName, | ||
maxConcurrency: 150, | ||
maxSize: 20, | ||
minSize: 5, | ||
}); | ||
|
||
// THEN | ||
Template.fromStack(stack).hasResourceProperties('AWS::AppRunner::AutoScalingConfiguration', { | ||
AutoScalingConfigurationName: autoScalingConfigurationName, | ||
MaxConcurrency: 150, | ||
MaxSize: 20, | ||
MinSize: 5, | ||
}); | ||
}); | ||
|
||
test('create an Auto scaling Configuration without all properties', () => { | ||
// WHEN | ||
new AutoScalingConfiguration(stack, 'AutoScalingConfiguration'); | ||
|
||
// THEN | ||
Template.fromStack(stack).hasResourceProperties('AWS::AppRunner::AutoScalingConfiguration', { | ||
AutoScalingConfigurationName: Match.absent(), | ||
MaxConcurrency: Match.absent(), | ||
MaxSize: Match.absent(), | ||
MinSize: Match.absent(), | ||
}); | ||
}); | ||
|
||
test.each([0, 26])('invalid minSize', (minSize: number) => { | ||
mazyu36 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
expect(() => { | ||
new AutoScalingConfiguration(stack, 'AutoScalingConfiguration', { | ||
minSize, | ||
}); | ||
}).toThrow(`minSize must be between 1 and 25, got ${minSize}`); | ||
}); | ||
|
||
test.each([0, 26])('invalid maxSize', (maxSize: number) => { | ||
expect(() => { | ||
new AutoScalingConfiguration(stack, 'AutoScalingConfiguration', { | ||
maxSize, | ||
}); | ||
}).toThrow(`maxSize must be between 1 and 25, got ${maxSize}`); | ||
}); | ||
|
||
test('minSize greater than maxSize', () => { | ||
expect(() => { | ||
new AutoScalingConfiguration(stack, 'AutoScalingConfiguration', { | ||
minSize: 5, | ||
maxSize: 3, | ||
}); | ||
}).toThrow('maxSize must be greater than minSize'); | ||
}); | ||
|
||
test.each([0, 201])('invalid maxConcurrency', (maxConcurrency: number) => { | ||
expect(() => { | ||
new AutoScalingConfiguration(stack, 'AutoScalingConfiguration', { | ||
maxConcurrency, | ||
}); | ||
}).toThrow(`maxConcurrency must be between 1 and 200, got ${maxConcurrency}`); | ||
}); | ||
|
||
test.each([ | ||
['tes'], | ||
['test-autoscaling-configuration-name-over-limitation'], | ||
['-test'], | ||
['test-?'], | ||
])('invalid autoScalingConfigurationName (name: %s)', (autoScalingConfigurationName: string) => { | ||
expect(() => { | ||
new AutoScalingConfiguration(stack, 'AutoScalingConfiguration', { | ||
autoScalingConfigurationName, | ||
}); | ||
}).toThrow(`autoScalingConfigurationName must match the ^[A-Za-z0-9][A-Za-z0-9\-_]{3,31}$ pattern, got ${autoScalingConfigurationName}`); | ||
}); | ||
mazyu36 marked this conversation as resolved.
Show resolved
Hide resolved
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we allow to specify
tags
as well?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the other L2 Constructs, it seemed like there were few resources adding tags as properties, so I didn't add them.
Since tags can also be added through aspects, I thought they might be unnecessary like other resources. What do you think?
I would appreciate your opinion on the criteria for adding tags, as I'm not sure about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense 👍 Thanks for clarifying!