rate limit: add local rate limit network filter

[mattklein123]

Allows local rate limit via a token bucket.

Risk Level: None, new feature
Testing: Integration/UT
Docs Changes: Added
Release Notes: Added

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9354 was opened by mattklein123.

see: more, trace.

[tonya11en]

You wouldn't underflow because the only place you'd decrement is in the refill handler. Otherwise, the counter only increases, so you can safely subtract whatever you observed in the refill handler. This is all moot if you're going with the CAS loop. I like that approach better and it's impossible to continuously spin since the token count stops changing once it hits 0.

…

On Sun, Dec 15, 2019, 8:55 AM Matt Klein ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In source/extensions/filters/network/local_ratelimit/local_ratelimit.cc <#9354 (comment)>: > + fill_interval_(PROTOBUF_GET_MS_REQUIRED(proto_config.token_bucket(), fill_interval)), + enabled_(proto_config.enabled(), runtime), + stats_(generateStats(proto_config.stat_prefix(), scope)), tokens_(max_tokens_) { + fill_timer_->enableTimer(fill_interval_); +} + +LocalRateLimitStats Config::generateStats(const std::string& prefix, Stats::Scope& scope) { + const std::string final_prefix = "local_rate_limit." + prefix; + return {ALL_LOCAL_RATE_LIMIT_STATS(POOL_COUNTER_PREFIX(scope, final_prefix))}; +} + +void Config::onFillTimer() { + uint32_t new_tokens_value; + { + // TODO(mattklein123): Consider replacing with an atomic CAS loop. + absl::MutexLock lock(&mutex_); Interesting idea. Still, you would need a CAS loop in the fill timer callback to avoid underflow. I also think that we should be exact on the rate limit. TBH the CAS implementation is simple, I can just do it now. I didn't because I thought people would complain about premature optimization. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#9354?email_source=notifications&email_token=AAIOZ7UPBM7OY5YFYVJ6WX3QYZOQXA5CNFSM4J25PZQ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCPHB5AQ#discussion_r357991563>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIOZ7Q76C6NK7FYF6MUMXTQYZOQXANCNFSM4J25PZQQ> .

[mattklein123]

You wouldn't underflow because the only place you'd decrement is in the
refill handler. Otherwise, the counter only increases, so you can safely
subtract whatever you observed in the refill handler.

Yes I suppose you could refill up to what you observe having been used, though it would be very easy to miss a fill window this way. In any case, I updated to a full CAS implementation which does not have this issue.

[mattklein123]

Assigning over to @junr03 and @tonya11en for review. @alyssawilk if you feel like taking a look also that would be great but not required if you don't have time. :)

[alyssawilk]

Looks great overall!

[junr03]

I am glad @tonya11en and you went for the CAS loop solution, it is pretty nice. Just a few comments and an ask in the testing code.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9354 was synchronize by mattklein123.

see: more, trace.

[mattklein123]

All comments addressed other than additional test comments which I am working on.

[mattklein123]

@tonya11en @junr03 @alyssawilk updated to cover the remaining comments. PTAL.

[mattklein123]

Note also that I think I have a cleaner way of doing the thread synchronizer stuff so going to update that also.

[mattklein123]

@htuch updated per comments.

@jmarantz PTAL at the new thread synchronizer. I think this version is much better.

[htuch]

/lgtm api

[jmarantz]

Looks great; left a few minor comments; feel free to apply in a follow-up if you agree.

[mattklein123]

/retest

[repokitteh-read-only]

🔨 rebuilding ci/circleci: api (failed build)

🐱

Caused by: a #9354 (comment) was created by @mattklein123.

see: more, trace.

[mattklein123]

@htuch @jmarantz this contains format fix for master as well as updated per @jmarantz comments. Can I get a stamp on this?