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

feat(apigateway): rate limited API key #6509

Merged
merged 9 commits into from
Mar 11, 2020
Merged

Conversation

vaibhav-walia
Copy link
Contributor

@vaibhav-walia vaibhav-walia commented Feb 28, 2020

Commit Message

feat(apigateway): rate limited API key (#6509)

In cases when we need to configure throttling and quotas etc., at the api key level, we need to create a usage plan for the api key and link the two.
This commit introduces a construct which allows the user to create an api key and spicify rate limiting settings, while creating the api key and not worry about creating usage plan and linking the two.

fixes #6405

End Commit Message


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

… at api key level

In cases when we need to configure throttling and quotas etc., at the api key level, we need to create a usage plan for the api key and link the two.
This commit introduces a construct which allows the user to create an api key and spicify rate limiting settings, while creating the api key and not worry about creating usage plan and linking the two.

fixes aws#6405
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 81f9454
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

… at api key level

In cases when we need to configure throttling and quotas etc., at the api key level, we need to create a usage plan for the api key and link the two.
This commit introduces a construct which allows the user to create an api key and spicify rate limiting settings, while creating the api key and not worry about creating usage plan and linking the two.

fixes aws#6405
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: b677b31
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

… at api key level

In cases when we need to configure throttling and quotas etc., at the api key level, we need to create a usage plan for the api key and link the two.
This commit introduces a construct which allows the user to create an api key and spicify rate limiting settings, while creating the api key and not worry about creating usage plan and linking the two.

fixes aws#6405
@mergify mergify bot dismissed nija-at’s stale review March 4, 2020 22:12

Pull request has been modified.

… at api key level

In cases when we need to configure throttling and quotas etc., at the api key level, we need to create a usage plan for the api key and link the two.
This commit introduces a construct which allows the user to create an api key and spicify rate limiting settings, while creating the api key and not worry about creating usage plan and linking the two.

fixes aws#6405
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 4eafcb5
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: c56bdec
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@vaibhav-walia vaibhav-walia requested a review from nija-at March 6, 2020 06:41
@nija-at nija-at changed the title feat(apigateway): Adds construct which allow configuring ratelimiting… feat(apigateway): rate limited API key Mar 9, 2020
nija-at
nija-at previously requested changes Mar 9, 2020
packages/@aws-cdk/aws-apigateway/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigateway/README.md Outdated Show resolved Hide resolved
@@ -20,6 +20,7 @@ export interface IApiKey extends IResourceBase {
*/
export interface ApiKeyProps extends ResourceOptions {
/**
* [disable-awslint:ref-via-interface]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed? We generally don't encourage any new linter exclusions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The build was failing after I made RateLimitedApiKeyProps extend ApiKeyProps :

error: [awslint:ref-via-interface:@aws-cdk/aws-apigateway.RateLimitedApiKeyProps.resources] API should use interface and not the concrete class (@aws-cdk/aws-apigateway.RestApi). If this is intentional, add "[disable-awslint:ref-via-interface]" to element's jsdoc

So I followed the suggestion and added [disable-awslint:ref-via-interface] to the jsdoc of resources property in ApiKeyProps

Copy link
Contributor

Choose a reason for hiding this comment

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

I see it now.

I believe this is occurring because this object is using RestApi instead of IRestApi as the type for resources.
Can we try switching it to IRestApi and see if all tests still pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried doing that, but it resulted in compilation errors as renderStageKeys method expects resources to be of type RestApi and changing it to IRestApi is not possible as it doesn't have deploymentStage property needed by this method
https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-apigateway/lib/api-key.ts#L90-L101

Co-Authored-By: Niranjan Jayakar <[email protected]>
@mergify mergify bot dismissed nija-at’s stale review March 9, 2020 18:54

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 4bccb02
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 4bccb02
  • Result: FAILED
  • Build Logs (available for 30 days)

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

… at api key level

Incorporates changes from code review.
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 9493c35
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@vaibhav-walia vaibhav-walia requested a review from nija-at March 9, 2020 22:13
@@ -20,6 +20,7 @@ export interface IApiKey extends IResourceBase {
*/
export interface ApiKeyProps extends ResourceOptions {
/**
* [disable-awslint:ref-via-interface]
Copy link
Contributor

Choose a reason for hiding this comment

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

I see it now.

I believe this is occurring because this object is using RestApi instead of IRestApi as the type for resources.
Can we try switching it to IRestApi and see if all tests still pass?

/**
* RateLimitedApiKey properties.
*/
export interface RateLimitedApiKeyProps extends ApiKeyProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missed something from last time. We usually don't extend one Props class from another Props class.

The following changes are needed here

  • Rename ApiKeyProps to ApiKeyOptions
  • Create a new empty ApiKeyProps class -
    export interface ApiKeyProps extends ApiKeyOptions {
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I understand.

Is the suggestion to :

  1. Rename ApiKeyProps defined in ApiKey, to ApiKeyOptions
  2. Create an empty interface (ApiKeyProps) in RateLimitedApiKey extending ApiKeyOptions
  3. Use it like
export interface RateLimitedApiKeyProps extends ApiKeyProps {

If that's the case, I'm confused how it will help/is better

Copy link
Contributor

Choose a reason for hiding this comment

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

You're correct. I think I was looking for an optimization/future-proofing whose value is unclear yet. Ignore this.

nija-at
nija-at previously approved these changes Mar 11, 2020
@mergify
Copy link
Contributor

mergify bot commented Mar 11, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: dd6269a
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@mergify mergify bot dismissed nija-at’s stale review March 11, 2020 16:00

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 6dcce28
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Mar 11, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 05c8742
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Mar 11, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Api-Gateway] Construct to allow quotas and throttling per api key
3 participants