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

HTTP local rate limit #1842

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

HTTP local rate limit #1842

wants to merge 3 commits into from

Conversation

kyessenov
Copy link
Contributor

Expose per-route local rate limiting in HTTP.
Simplified version of Envoy APIs.

Signed-off-by: Kuat Yessenov [email protected]

Signed-off-by: Kuat Yessenov <[email protected]>
@istio-policy-bot
Copy link

😊 Welcome @kyessenov! This is either your first contribution to the Istio api repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@google-cla google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 22, 2021
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 22, 2021
@kyessenov kyessenov added the release-notes-none Indicates a PR that does not require release notes. label Jan 22, 2021
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
Copy link
Contributor Author

/assign @bianpengyuan
/assign @douglas-reid
/assign @mandarjog

We can use stage 0 for local rate limit (this PR) and stage 1 for global rate limit (follow up).

Signed-off-by: Kuat Yessenov <[email protected]>
// max_tokens: 100
// tokens_per_fill: 100
// fill_interval: 1s
// ```
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks !

  1. either description or example when max tokens and tokens per fill are different? We can point to the envoy documentation or another place as well, but would be good to have basics here.
  2. Add an example for sidecar to sidecar communication where we want to limit easy source canonical service to a certain amount.

Copy link
Contributor Author

@kyessenov kyessenov Jan 22, 2021

Choose a reason for hiding this comment

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

  1. It's hard to put in words a custom token bucket (it's a tradeoff between the timer overhead and "smoothness" of request bursts).
  2. I don't think that's possible with the networking APIs since we need this on the server-side. We might need to copy-paste parts of networking into server-side API like authorization did. The whole networking API assumes a known source already, but it doesn't use "canonical" terminology.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a bummer, also a deal breaker. Ability to ratelimit inter service traffic is one of the main customer asks.

@kyessenov kyessenov mentioned this pull request Jan 26, 2021
@istio-testing
Copy link
Collaborator

@kyessenov: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. needs-rebase Indicates a PR needs to be rebased before being merged release-notes-none Indicates a PR that does not require release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants