search

solo-io / envoy-gloo

Apache License 2.0
25 stars 14 forks source link

local rate limiter: backport the non-timer based bucket #357

Closed nfuden closed 3 months ago

nfuden commented 3 months ago

backport the first 3 rate limit pieces of upstream's 34774 issue

This allows us to run local rate limiting not on the main thread if an environment variable is set to true.

In the future can be disabled via envoy_reloadable_features_no_timer_based_rate_limit_token_bucket for now can be ENABLED via envoy_reloadable_features_no_timer_based_rate_limit_token_bucket

solo-changelog-bot[bot] commented 3 months ago

Issues linked to changelog: https://github.com/solo-io/gloo/issues/9564

ashishb-solo commented 3 months ago

I'm not sure exactly how to review/test this so any guidance you have would be appreciated. So far, envoy-static seems to compile successfully. I'm also looking at the tests that were modified in the patch. It looks like here's a list of test files that were updated:

test/common/common/BUILD
test/common/common/token_bucket_impl_test.cc
test/extensions/filters/common/local_ratelimit/BUILD
test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc
test/extensions/filters/http/local_ratelimit/BUILD
test/extensions/filters/http/local_ratelimit/config_test.cc
test/extensions/filters/http/local_ratelimit/filter_test.cc
test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc
test/extensions/filters/listener/local_ratelimit/local_ratelimit_test.cc
test/extensions/filters/network/local_ratelimit/local_ratelimit_integration_test.cc
test/extensions/filters/network/local_ratelimit/local_ratelimit_test.cc

So we can test this by running

bazel test \
@envoy//test/common/common:token_bucket_impl_test \
@envoy//test/extensions/filters/common/local_ratelimit/... \
@envoy//test/extensions/filters/http/local_ratelimit/... \
@envoy//test/extensions/filters/listener/local_ratelimit/... \
@envoy//test/extensions/filters/network/local_ratelimit/...

I'm in the process of running this locally. If it's possible to add this to CI, that would be amazing, but if it's too much of a lift, then don't worry about it.

I'm also wondering if we want to do some sort of performance test before merging this into Gloo and/or releasing it. Do you have any thoughts here?

nfuden commented 3 months ago

https://github.com/solo-io/gloo/pull/9822 shows that basic local rate limit works with this change

nfuden commented 3 months ago

/kick aml-cpp

nfuden commented 3 months ago

/kick 92f51882