Skip to content
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

docs(apigateway): update RateLimitedApiKey example code #31490

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

akagire
Copy link

@akagire akagire commented Sep 19, 2024

Issue # (if applicable)

N/A

Reason for this change

RateLimitedApiKey provides associate with provisioned api key and usage plan.
When specify stages, it expects associate with provisioned usage plan and specified stages.

But current behavior did not associate with provisioned usage plan and specified stages but also gives stage like current documents example code.

Considering the context in which RateLimitedApiKey is implemented, it makes sense to associate with provisioned usage plan and specified stages.

Description of changes

Changed sample code to use apiStages instead of stages as get expected behavior.

Checklist


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@aws-cdk-automation aws-cdk-automation requested a review from a team September 19, 2024 03:02
@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Sep 19, 2024
@akagire akagire changed the title docs(api-gateway): update example code to specify stage docs(apigateway): update example code to specify stage Sep 19, 2024
@akagire akagire changed the title docs(apigateway): update example code to specify stage docs(apigateway): update RateLimitedApiKey example code Sep 19, 2024
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 89a0dbc
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 20, 2024
@@ -465,7 +465,7 @@ declare const api: apigateway.RestApi;

const key = new apigateway.RateLimitedApiKey(this, 'rate-limited-api-key', {
customerId: 'hello-customer',
stages: [api.deploymentStage],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the RateLimitApiKey has a stages property.
Can you tell me more about the meaning of 'not available'?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mazyu36 When I used the stages, I observed that while the api key was associated with the provisioned Usage Plan. But the Stage was not. This is what I meant by "not available". ( I realizing this expression was not good...)

If we were to follow the sample code using stages, it would be necessary to call plan.addApiStage. That means would require declaring the UsagePlan explicitly.
In such a case, since we would be setting the rate limit in declare UsagePlan, the necessity of RateLimitedApiKey becomes unclear.

Therefore, I read #6509 to understand the context behind the implementation of RateLimitedApiKey.
I gathered that it was implemented to declare rate limit related configure and associated with usage plan and stage, not only declaring api key.
if current behavior and my understanding is correct, I think sample code is suitable for using apiStages instead of stages

Could you please advice me when my understand is wrong?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akagire
Thank you for the explanation.
I see that apiStages are indeed linked to the UsagePlan as you described.
https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-apigateway/lib/api-key.ts#L261

Since this modification aligns the code example with a more appropriate use case, I think it's fine.
Let's see what the maintainers think about it.

Could you please update the issue description?
This is because the stages property itself is not actually unavailable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akagire thanks for taking on this doc update! it seems confusing to me that apiStages and stages are both available on the construct but are applied in different areas. if that's not a bug, can we also add in the README a more clear distinction for how those properties differ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mazyu36 I updated.

@kaizencc Honestly, I cannot describe those distinction yet but I'll try to and investigate. The possibility of a bug has not been ruled out. If so, I'll post new issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this still seems like an open thread that should be closed before this PR is merged

Copy link
Contributor

@mazyu36 mazyu36 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution. I've included one question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants