-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat: implement UsagePlan and ApiKey support in L2 constructs for aws-apigatewayv2 (WebSocketApi) #35060
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
Conversation
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.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
|
||
| const webSocketApi = new WebSocketApi(stack, 'WebSocketApi'); | ||
|
|
||
| webSocketApi.addRoute('$connect', { |
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.
What's the reasons for adding routes('$connect', 'sendMessage')?
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.
They were written just to test if addRoute method works. But because I want to test only the usage plan and the routes don't have an impact on this, I deleted them from the integ test.
| const webSocketStage = new WebSocketStage(stack, 'WebSocketStage', { | ||
| webSocketApi, | ||
| stageName: 'dev', | ||
| throttle: { |
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.
since we're testing UsagePlan, can we remove all the optional props in other constructor. That will make the code easier to read/understand
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.
Yes, I kept only the mandatory properties for both WebSocketStages.
|
|
||
| const apiKey = new ApiKey(stack, 'ApiKey'); | ||
|
|
||
| const usagePlan = new UsagePlan(stack, 'UsagePlan', { |
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.
can we add all the optional parameter? since we want to cover as much branches as possible
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.
Yes, I added both the mandatory and optional parameters for ApiKey and UsagePlan, because these are the 2 new added features.
| class Import extends ApiKeyBase { | ||
| public keyId = apiKeyId; | ||
| public keyArn = Stack.of(this).formatArn({ | ||
| service: 'apigateway', |
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 other constructors in apigwv2, i see service: 'execute-api',. Why are we going with apigateway here? also same comment on line 181
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 here
I saw that 'execute-api' is used only for the endpoints, while the other resources use 'apigateway' no matter if they are apigateway v1 or v2.
| public keyId = apiKeyId; | ||
| public keyArn = Stack.of(this).formatArn({ | ||
| service: 'apigateway', | ||
| account: '', |
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.
why are we going with empty string account? instead of removing the account prop
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.
For a resource's ARN we need the account field empty (as I have seen here). If we would delete the account property, the account would be written by default.
| * @param options options that control the behaviour of this method | ||
| */ | ||
| public addApiKey(apiKey: IApiKey, options?: AddApiKeyOptions): void { | ||
| let id: string; |
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.
What's the point of having let here? also since it's used one time isn't it better to have
const resource = new CfnUsagePlanKey(this, `${prefix}:${Names.nodeUniqueId(apiKey.node)}`, {`
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.
let is used only if that variable could take more values depending on some conditions. Actually you are right, we don't need let in this case because to the id we assign only one string, no matter what. I modified that part as you suggested.
| addConstructMetadata(this, props); | ||
| let resource: CfnUsagePlan; | ||
|
|
||
| resource = new CfnUsagePlan(this, 'Resource', { |
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.
can we have
const resource = new CfnUsagePlan(this, 'Resource', {
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.
Yes, actually this is the good way of writing. Same explanation as in the previous comment.
| } | ||
| } | ||
|
|
||
| private renderThrottle(props: ThrottleSettings | undefined): (CfnUsagePlan.ThrottleSettingsProperty | Token) { |
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.
This function never returns Token, right? can we remove 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.
Yes, this function never return Token, but it is possible to return undefined. I replaced Token with undefined and I adjusted the code so that if the props are undefined, the function would also return undefined.
| validateInteger(limit, 'Throttle quota limit'); | ||
| const ret = { | ||
| limit: limit ? limit : undefined, | ||
| offset: props.quota ? props.quota.offset : undefined, |
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.
this check not needed, since it's checked in the if L283
the same comment on L291
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.
Yes, they are not necessary. I removed the checks for limit, offset and period.
For example, this is enough: limit: props.quota.limit, because if the limit is undefined it will return undefined which is the desired outcome.
| * @default none | ||
| * @deprecated use `addApiKey()` | ||
| */ | ||
| readonly apiKey?: IApiKey; |
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.
Why are we adding deprecated field?
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.
I have kept the deprecated field apiKey only because it is kept in apigatewayv1 too. Now, I removed it from the WebSocket's UsagePlan because is not needed.
Why the apiKey property was not useful:
- It only allowed adding a single API key to the
UsagePlan - Internally, it just called the
addApiKey()method anyway - The
addApiKey()method provides the same functionality with more flexibility
The better approach:
- Use only the
addApiKey()method directly, which allows adding multiple keys - Keys can be added after
UsagePlancreation, providing more control - This eliminates the deprecated property while maintaining full functionality
| } | ||
|
|
||
| private createStage(apiStage: UsagePlanPerApiStage): CfnUsagePlan.ApiStageProperty { | ||
| const stage = apiStage.stage ? apiStage.stage.stageName.toString() : undefined; |
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.
suggestion: isn't it better to wrap this function with one condition
private createStage(apiStage: UsagePlanPerApiStage): CfnUsagePlan.ApiStageProperty {
if(apiStage.stage){
return {
stage: apiStage.stage.stageName.toString(),
apiId: apiStage.stage.api.apiId
}
}
}
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.
Yes, is much clear in this way. Also, I added the fact that if apiStage.stage is undefined, the method would explicitly return undefined.
| } | ||
|
|
||
| private renderApiStages(apiStages: UsagePlanPerApiStage[] | undefined): CfnUsagePlan.ApiStageProperty[] | undefined { | ||
| if (apiStages && apiStages.length > 0) { |
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.
this condition && apiStages.length > 0 is useless, can we remove 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.
It is somehow useful for maintaining the CDK standards.
Without the apiStages.length > 0 condition:
- When a user creates a
UsagePlanwith an emptyapiStagesarray (apiStages: []) - The
CloudFormation Templateexplicitly shows:"ApiStages": []
With the apiStages.length > 0 condition:
- Empty arrays are treated as
undefined - The
ApiStagesproperty is omitted entirely from theCloudFormation Template - This follows CDK's standard practice of omitting optional properties when they have no meaningful values
| // WHEN | ||
| new RateLimitedApiKey(stack, 'test-api-key', { | ||
| customerId: 'test-customer', | ||
| quota: { |
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.
can we pass also throttle and apiStages? to check that we're propagating them correctly
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.
Yes, this would be a better approach. I added throttle and apiStages too.
| rateLimit: rateLimit, | ||
| }; | ||
| } | ||
| return ret!; |
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.
non null assertion operator should be avoided unless it's needed
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.
Yes, I modified the code such that all the cases are covered (props are undefined or not) and return ret! is not needed anymore.
63229eb to
596eb12
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| /** | ||
| * Uniquely identifies this class. | ||
| */ | ||
| public static readonly PROPERTY_INJECTION_ID: string = 'aws-cdk-lib.aws-apigatewayv2.ApiKey'; |
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.
The PROPERTY_INJECTION_ID should be more specific to avoid conflicts with other similar constructs. Consider changing to:'aws-cdk-lib.aws-apigatewayv2.websocket.ApiKey or aws-cdk-lib.aws-apigatewayv2.WebSocketApiKey. Applicable for other constructs also.
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.
Yes, this implementation follows CDK patterns. I used aws-cdk-lib.aws-apigatewayv2.websocket.ApiKey because it aligns with existing CDK conventions to having clear names for the properties. The alternative approach would have required renaming classes, which would have been an unnecessary refactoring effort.
|
|
||
| /** | ||
| * The API key ARN. | ||
| */ |
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.
Based on the Design guidelines, we should add the @attribute tag for this resource property.
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.
Thank you for the resource you attached. I didn't know this information. I added @attribute tag now.
| /** | ||
| * A name for the API key. If you don't specify a name, AWS CloudFormation generates a unique physical ID and uses that ID for the API key name. | ||
| * @link http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-apigateway-apikey.html#cfn-apigateway-apikey-name | ||
| * @default automically generated name |
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.
Typo: automically should be automatically.
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.
Corrected it
|
|
||
| usagePlan.addApiKey(apiKey); | ||
| usagePlan.addApiStage({ api: webSocketApi, stage: webSocketStage2 }); | ||
|
|
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.
Consider adding a RateLimitedApiKey test case as well to the integration test.
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.
Good idea. I added the creation of a new WebsocketStage and a RateLimitedApiKey. It is not needed to do a UsagePlan too, because it is automatically created when a RateLimitedApiKey is created.
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
This pull request has been removed from the queue for the following reason: The pull request can't be updated. You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #28756.
Reason for this change
Couldn't create
UsagePlansandApiKeysforWebSocketApi(apigatewayv2) using L2 constructs, cause these 2 features were supported only byRestApi(apigatewayv1).Description of changes
HttpApi(apigatewayv2) doesn't permit creatingUsagePlansorApiKeysin any way, so bringing these functionalities fromapigatewayv1toapigatewayv2is possible only forWebSocketApi.UsagePlanandApiKeyfiles inaws-apigatewayv2/lib/websocketfolderRestApi(apigatewayv1)Describe any new or updated permissions being added
N/A
Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license