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

improve ratelimit doc #11654

Merged
merged 3 commits into from
Aug 18, 2022
Merged

improve ratelimit doc #11654

merged 3 commits into from
Aug 18, 2022

Conversation

zirain
Copy link
Member

@zirain zirain commented Jul 28, 2022

Please provide a description for what this PR is for.

  • add test
  • improve global ratelimit EnvoyFilter

fix: #10372

And to help us figure out who should review this PR, please
put an X in all the areas that this PR affects.

  • Configuration Infrastructure
  • Docs
  • Installation
  • Networking
  • Performance and Scalability
  • Policies and Telemetry
  • Security
  • Test and Release
  • User Experience
  • Developer Infrastructure

@zirain zirain requested a review from a team as a code owner July 28, 2022 11:13
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 28, 2022
@zirain zirain changed the title improve ratelimit task improve ratelimit doc Jul 28, 2022
@zirain
Copy link
Member Author

zirain commented Aug 5, 2022

cc @douglas-reid

@douglas-reid
Copy link
Contributor

@kyessenov can you review this? it seems good at first pass, but i'd like a more rate-limit savvy set of eyes to confirm.

@ericvn
Copy link
Contributor

ericvn commented Aug 17, 2022

@kyessenov Any thoughts? Thanks.

Copy link
Contributor

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

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

Thanks, this is great, definitely improving the doc.

Comment on lines 54 to 60
# wait a while for envoyfilter
sleep 1s

for i in {1..10}; do
echo "$i"
snip_verify_local_rate_limit_1
done
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just extra debugging? The verify_same should repeat the snip_verify_local_rate_limit_1 and compare output until the condition is met or the retries have expired.

Copy link
Member Author

Choose a reason for hiding this comment

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

you're right, I misunderstand the usage of _verify_same.
will fix this, thanks!

snip_global_rate_limit_4

# verify global ratelimit
snip_verify_global_rate_limit_1
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this line is needed, as it's done as part of the next line.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

@istio-testing istio-testing merged commit 02cd7a8 into istio:master Aug 18, 2022
@istio-testing
Copy link
Contributor

In response to a cherrypick label: new pull request created: #11717

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Automate Testing for "Enabling Rate Limits using Envoy"
5 participants