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

[apigateway] Throttling per method not set when value=0 #10071

Closed
cyuste opened this issue Aug 31, 2020 · 4 comments · Fixed by #10088
Closed

[apigateway] Throttling per method not set when value=0 #10071

cyuste opened this issue Aug 31, 2020 · 4 comments · Fixed by #10088
Assignees
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway bug This issue is a bug. effort/small Small work item – less than a day of effort in-progress This issue is being actively worked on. p1

Comments

@cyuste
Copy link
Contributor

cyuste commented Aug 31, 2020

When we create a throttling for a method with rate and burst limits set to 0 we expect to have that method configured to allow 0 requests (disabled) as it happens using aws console, but in cdk the throttling is not configured causing the method to use global values

Reproduction Steps

Create an API Gateway and assign to an usage plan a throttling per method set to 0:

throttle = new APIGateway.ThrottleSettings
{
     RateLimit = 0,
     BurstLimit = 0
};

What did you expect to happen?

A new throttling per method is created for the specified method with rate and burst set to 0

What actually happened?

No throttling is configured for that method. In the cloudformation that throttling is described as:

"Throttle": {
  ...
  "/ServiceManager.svc/ANY": {},
}

It should contain BurstLimit and RateLimit set to 0.

Environment

  • CLI Version : 1.60.0 (build 8e3f53a)
  • Framework Version:
  • Node.js Version: v14.7.0
  • OS : Max OS X Catalina 10.15.6
  • Language (Version): dotnet (3.1.301)

Other

I think that the problem is in this condition

As 0 is considered false an thus undefined


This is 🐛 Bug Report

@cyuste cyuste added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 31, 2020
@SomayaB SomayaB changed the title [ApiGateway] Throttling per method not set when value=0 [api-gateway] Throttling per method not set when value=0 Aug 31, 2020
@github-actions github-actions bot added the @aws-cdk/aws-apigateway Related to Amazon API Gateway label Aug 31, 2020
@cyuste
Copy link
Contributor Author

cyuste commented Aug 31, 2020

I downloaded aws-cdk repo and removed the conditional, i.e modify renderThrottle method and leave it as:

private renderThrottle(props: ThrottleSettings | undefined): (CfnUsagePlan.ThrottleSettingsProperty | Token) {
    let ret: CfnUsagePlan.ThrottleSettingsProperty | Token;
    if (props !== undefined) {
      const burstLimit = props.burstLimit;
      validateInteger(burstLimit, 'Throttle burst limit');
      const rateLimit = props.rateLimit;
      validateInteger(rateLimit, 'Throttle rate limit');

      ret = {
        burstLimit: burstLimit,
        rateLimit: rateLimit,
      };
    }
    return ret!;
  }

Unit tests pass and now the resultant cloudformation is correct. I can create a PR if needed. Any idea of why is that condition in the code? The values are already validated using validateInteger function...

@cyuste
Copy link
Contributor Author

cyuste commented Sep 1, 2020

I created a branch with this change and a unit test that fails with the original version, it's my first PR so I left it as Draft

@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Sep 1, 2020
@nija-at
Copy link
Contributor

nija-at commented Sep 2, 2020

This looks like the correct fix. Thanks @cyuste for creating a PR.
Move this out of draft state when you're ready so it can be reviewed and merged.

@nija-at nija-at changed the title [api-gateway] Throttling per method not set when value=0 [apigateway] Throttling per method not set when value=0 Sep 2, 2020
@nija-at nija-at added effort/small Small work item – less than a day of effort p1 and removed needs-triage This issue or PR still needs to be triaged. labels Sep 2, 2020
@cyuste
Copy link
Contributor Author

cyuste commented Sep 2, 2020

PR ready for review :)

@mergify mergify bot closed this as completed in #10088 Sep 2, 2020
mergify bot pushed a commit that referenced this issue Sep 2, 2020
…igured to 0 (#10088)

Closes #10071 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway bug This issue is a bug. effort/small Small work item – less than a day of effort in-progress This issue is being actively worked on. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants