LocalRateLimit(HTTP): Add dynamic token bucket support by vikaschoudhary16 · Pull Request #36623 · envoyproxy/envoy

vikaschoudhary16 · 2024-10-16T05:50:02Z

Commit Message: LocalRateLimit(HTTP): Add dynamic token bucket support
Additional Description:
fixes: #23351 and #19895

User configures descriptors in the http local rate limit filter. These descriptors are the "target" to match using the source descriptors built using the traffic(http requests). Only matched traffic will be rate limited. When request comes, at runtime, based on rate_limit configuration, descriptors are generated where values are picked from the request as directed by the rate_limit configuration. These generated descriptors are matched with "target"(user configured) descriptors. Generated descriptors are very flexible already in the sense that "values" from the request can be extracted using number of ways such as dynamic metadata, matcher api, computed reg expressions etc etc, but in "target"(user configured) descriptors are very rigid and it is expected that user statically configures the "values" in the descriptor.
This PR is adding flexibility by allowing blank "values" in the user configured descriptors. Blank values will be treated as wildcard. Suppose descriptor entry key is client-id and value is left blank by the user, local rate limit filter will create a descriptor dynamically for each unique value of header client-id. That means client1, client2 and so on will have dedicated descriptors and token buckets.

To keep a resource consumption under limit, LRU cache is maintained for dynamic descriptors with a default size of 20, which is configurable.

Docs Changes: TODO
Release Notes: TODO

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

repokitteh-read-only · 2024-10-16T05:50:13Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #36623 was opened by vikaschoudhary16.

see: more, trace.

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

wbpcode · 2024-10-16T09:45:26Z

I think we still need a way to limit the overhead and memory of the token buckets. It's unacceptable to let it increases unlimited.

vikaschoudhary16 · 2024-10-16T10:39:59Z

I think we still need a way to limit the overhead and memory of the token buckets. It's unacceptable to let it increases unlimited.

Thanks a lot for taking a look.
Will add logic to keep it under limits

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

wbpcode · 2024-10-20T10:31:17Z

Thanks for this contribution. Dynamic descriptor support is a very complex problem in the local rate limit, considering various limitations.

I have take a pass to current implementation, but before flush more comments to the implementation details, I will throw some quesions first:

Should the LRU list be updated when one of the entry is hit in the worker? I think it should to keep the active descriptor alive. But this will also introduces more compleixity.
Should different descriptor has independent list? Or the descriptor with much more different dynamic values will occupies the whole list?
Lifetime management and search logic is much more complex now.

So, I will suggest you to take a step back and think is there other way to resolve your requirement, like a new limit filter in your fork or using the rate_limit/rate_limit_quota directly.

If you insist on to enhance the local_rate_limit, then, I think we should provide an abstraction (like DynamicDescriptor) to wrap all these complexity first (list/memory management, lifetime problem, cross workers updating, etc. (PS: I think lock may be an option if we can limit the lock only be used in the new feature)), and then, we could integrate the high level abstraction into the local_rate_limit. Or it's hard for reviewer to ensure the quality of the exist feature.

Thanks again for all your help and contribution. This is not simple feature. 🌷

vikaschoudhary16 · 2024-10-21T03:23:01Z

Thanks for this contribution. Dynamic descriptor support is a very complex problem in the local rate limit, considering various limitations.

I have take a pass to current implementation, but before flush more comments to the implementation details, I will throw some quesions first:

Should the LRU list be updated when one of the entry is hit in the worker? I think it should to keep the active descriptor alive. But this will also introduces more compleixity.

Should different descriptor has independent list? Or the descriptor with much more different dynamic values will occupies the whole list?

Lifetime management and search logic is much more complex now.

So, I will suggest you to take a step back and think is there other way to resolve your requirement, like a new limit filter in your fork or using the rate_limit/rate_limit_quota directly.

If you insist on to enhance the local_rate_limit, then, I think we should provide an abstraction (like DynamicDescriptor) to wrap all these complexity first (list/memory management, lifetime problem, cross workers updating, etc. (PS: I think lock may be an option if we can limit the lock only be used in the new feature)), and then, we could integrate the high level abstraction into the local_rate_limit. Or it's hard for reviewer to ensure the quality of the exist feature.

Thanks again for all your help and contribution. This is not simple feature. 🌷

Thanks a lot @wbpcode for taking a look. Really appreciate!!

So, I will suggest you to take a step back and think is there other way to resolve your requirement, like a new limit filter in your fork or using the rate_limit/rate_limit_quota directly.

From the conversations on the linked issues, seems like there has been repetitive request for this functionality since long time, so figuring out a solution in community might benefit number of users.
yeah, rate_limit_quota can address the usecase in case user is fine with external dependency. In some cases, users are keen in avoiding external dependency and looking for solution through local rate limiter path.

If you insist on to enhance the local_rate_limit, then, I think we should provide an abstraction (like DynamicDescriptor) to wrap all these complexity first (list/memory management, lifetime problem, cross workers updating, etc. (PS: I think lock may be an option if we can limit the lock only be used in the new feature)), and then, we could integrate the high level abstraction into the local_rate_limit.

Sounds good. I will work on adding the abstraction as per your suggestion.

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

wbpcode · 2024-10-25T07:15:20Z

Thanks so much for this update. I think this make sense. Here are some high level suggestions to this (I think we are in the correct way, thanks):

Please add an explict bool flag to enable this feature to avoid some users mis-use this when they configure an empty descriptor in case.
This is a new feature. So, you can ignore the timer based token bucket completely. It would make the implementation simper.
You may need to refactor the return type of requestAllowed. Now, because the buckets in the dynamic descriptor may be destroyed in another thread.

wbpcode · 2024-10-25T07:20:13Z

api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto

+  // Actual number of dynamic descriptors will depend on the cardinality of unique values received from the http request for the omitted
+  // values.
+  // Default is 20.
+  uint32 dynamic_descripters_lru_cache_limit = 18;


May name this simplely max_dynamic_descripters. (We may change the elimination algorithm in future, who know?) and please use wrapper number type google.protobuf.UInt32Value.

And Please add explict bool to enable this feature, like google.protobuf.BoolValue use_dynamic_descripters.

github-actions · 2024-11-24T08:01:17Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

adisuissa · 2025-01-23T13:51:22Z

@wbpcode PTAL.

wbpcode · 2025-01-26T09:26:22Z

Thanks for this great contribution. I am so happy to see this happens. I think we are at correct way after a quick check. But I prefer to review and land this after #38197 because it will also simplify this PR.

vikaschoudhary16 · 2025-01-27T03:59:06Z

Thanks for this great contribution. I am so happy to see this happens. I think we are at correct way after a quick check. But I prefer to review and land this after #38197 because it will also simplify this PR.

sure, I am anyways not assuming timer token bucket in this PR as per your previous suggestion so will not break anything in this PR rather will help simplify by allowing to remove few checks/validations. So totally agree on landing #38197 first. Thanks a lot!!

alyssawilk · 2025-01-30T14:00:06Z

/wait on the other PR. please main merge when it's ready for another pass, and it'll show back up in the oncall dashboard

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

wbpcode

Thanks for the update. Some comments are added. And please check all the comments to ensure it match our style. Thanks again!

wbpcode · 2025-02-02T03:34:01Z

source/common/runtime/runtime_features.cc


+// A flag to enable the usage of dynamic buckets for local rate limiting. For example, dynamically
+// created token buckets for each unique value of a request header.
+FALSE_RUNTIME_GUARD(envoy_reloadable_features_local_rate_limiting_with_dynamic_buckets);


I think runtime flag is unnecessary because dynamic descriptors is only enabled when the empty descriptor value is set which is not allowed in the past.

I we can treat this is a new feature that controlled by explict API configuration.

wbpcode · 2025-02-02T03:37:00Z

source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h

 using RateLimitTokenBucketSharedPtr = std::shared_ptr<RateLimitTokenBucket>;

-class LocalRateLimiterImpl {
+class LocalRateLimiterImpl : public Logger::Loggable<Logger::Id::rate_limit_quota> {


I think you are using incorrect logger id.

added a new id local_rate_limit

I think you forget to change this line code?

wbpcode · 2025-02-02T03:38:59Z

api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto

+  // Actual number of dynamic descriptors will depend on the cardinality of unique values received from the http request for the omitted
+  // values.
+  // Minimum is 1. Default is 20.
+  google.protobuf.UInt32Value max_dynamic_descriptors = 18;


Please add PGV for this field.

wbpcode · 2025-02-02T03:40:50Z

source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc

+        if (per_connection) {
+          throw EnvoyException(
+              "local rate descriptor value cannot be empty in per connection rate limit mode");
+        }


Why this restriction is necessary?

not necessary. Just wanted to reduce scope and cover it in follow up. I think it will require just passing max_dynamic_descriptors from filter config to PerConnectionRateLimiter.
~~Do you think it is must to cover PerConnectionRateLimiter also in this PR?~~
Pushed changes to cover this as well.

wbpcode · 2025-02-02T03:42:49Z

source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc

+        if (lru_size == 0) {
+          throw EnvoyException("minimum allowed value for max_dynamic_descriptors is 1");
+        }


This should be validated when loading the local rate limit configuration rather than doing it here.

added PGV to cover this

wbpcode · 2025-02-02T03:50:16Z

source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc

+    if (wildcard_found) {
+      DynamicDescriptorSharedPtr dynamic_descriptor = std::make_shared<DynamicDescriptor>(
+          per_descriptor_token_bucket, lru_size, dispatcher.timeSource());


For the dynamic descriptor, the per_descriptor_token_bucket is unnecessary. I think you should created a DynamicDescriptor based on the per_descriptor_max_tokens, per_descriptor_tokens_per_fill, per_descriptor_fill_interval rather then keeping a meaningless per_descriptor_token_bucket.

The per_descriptor_token_bucket should only be created for the non-dynamic descriptors here.

wbpcode · 2025-02-02T05:11:10Z

source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc

+bool DynamicDescriptorMap::matchDescriptorEntries(
+    const std::vector<RateLimit::DescriptorEntry>& request_entries,
+    const std::vector<RateLimit::DescriptorEntry>& user_entries) {


The user_entries is a little unclear. It would be better to call it config_entries or something.

wbpcode · 2025-02-02T05:15:52Z

source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc

+  bool has_empty_value = false;
+  for (size_t i = 0; i < request_entries.size(); ++i) {
+    // Check if the keys are equal
+    if (request_entries[i].key_ != user_entries[i].key_) {
+      return false;
+    }
+
+    // all non-blank user values must match the request values
+    if (!user_entries[i].value_.empty() && user_entries[i].value_ != request_entries[i].value_) {
+      return false;
+    }
+
+    // Check for empty value in user entries
+    if (user_entries[i].value_.empty()) {
+      has_empty_value = true;
+    }
+  }
+  return has_empty_value;


Suggested change

bool has_empty_value = false;

for (size_t i = 0; i < request_entries.size(); ++i) {

// Check if the keys are equal

if (request_entries[i].key_ != user_entries[i].key_) {

return false;

}

// all non-blank user values must match the request values

if (!user_entries[i].value_.empty() && user_entries[i].value_ != request_entries[i].value_) {

return false;

}

// Check for empty value in user entries

if (user_entries[i].value_.empty()) {

has_empty_value = true;

}

}

return has_empty_value;

for (size_t i = 0; i < request_entries.size(); ++i) {

// Check if the keys are equal.

if (request_entries[i].key_ != user_entries[i].key_) {

return false;

}

// Check values are equal or wildcard value is used.

if (user_entries[i].value_.empty()) {

continue;

}

if (request_entries[i].value_ != user_entries[i].value_) {

return false;

}

}

return true;

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

wbpcode

LGTM with very very minor comments. Thanks for update 🙏

wbpcode · 2025-02-09T14:44:45Z

source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h

 using RateLimitTokenBucketSharedPtr = std::shared_ptr<RateLimitTokenBucket>;

-class LocalRateLimiterImpl {
+class LocalRateLimiterImpl : public Logger::Loggable<Logger::Id::rate_limit_quota> {


I think you forget to change this line code?

wbpcode · 2025-02-09T14:46:20Z

source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc

+RateLimitTokenBucketSharedPtr
+DynamicDescriptorMap::getBucket(const RateLimit::Descriptor request_descriptor) {
+  for (const auto& pair : user_descriptors_) {
+    auto user_descriptor = pair.first;


nit: config_descriptor

wbpcode · 2025-02-09T14:47:29Z

source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc

+      continue;
+    }
+
+    // we found a user configured wildcard descriptor that matches the request descriptor.


Please all check all these comments match our style.

I have updated this specific comment. Is there a doc/readme that have guidelines about the comment styles for my reference?

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

wbpcode

LGTM. Thanks.

Http LocalRateLimit: Add dynamic token bucket support

6ff6a4b

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

vikaschoudhary16 requested review from mattklein123 and wbpcode as code owners October 16, 2024 05:50

vikaschoudhary16 marked this pull request as draft October 16, 2024 05:50

repokitteh-read-only bot added the api label Oct 16, 2024

repokitteh-read-only bot assigned markdroth Oct 16, 2024

comments

71ecf21

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

vikaschoudhary16 force-pushed the lrl-dynamic-tokenbuckets branch from 457f0a9 to 71ecf21 Compare October 16, 2024 08:34

updates

365e7c4

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

wbpcode self-assigned this Oct 16, 2024

vikaschoudhary16 added 8 commits October 18, 2024 09:42

lru cache with max size, remove locks and basic test

cde6760

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

some cleanup

e1dc1ac

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

updates

f1e4765

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

updates

15f655e

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

updates

9d251f8

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

updates

ed02fa6

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

cleanup

d305bd5

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

fix

53e84bb

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

DynamicDescriptor interface

bb34179

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

wbpcode reviewed Oct 25, 2024

View reviewed changes

wbpcode unassigned markdroth Oct 25, 2024

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 24, 2024

wbpcode removed the stale stalebot believes this issue/PR has not been touched recently label Nov 25, 2024

vikaschoudhary16 requested a review from wbpcode January 20, 2025 11:01

repokitteh-read-only bot added the waiting label Jan 30, 2025

vikaschoudhary16 added 2 commits January 31, 2025 04:51

Merge branch 'main' into lrl-dynamic-tokenbuckets

eaa9c95

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

fix merging issues

db7a666

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

repokitteh-read-only bot removed the waiting label Jan 31, 2025

vikaschoudhary16 added 2 commits January 31, 2025 08:22

format

538d597

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

ci

e46f681

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

wbpcode reviewed Feb 2, 2025

View reviewed changes

wbpcode added the waiting label Feb 2, 2025

Address review comments

605ef6e

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

repokitteh-read-only bot removed the waiting label Feb 3, 2025

PerConnection

5527817

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

vikaschoudhary16 force-pushed the lrl-dynamic-tokenbuckets branch from a7d420f to 5527817 Compare February 3, 2025 23:47

cleanup

2bff228

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

vikaschoudhary16 requested a review from wbpcode February 5, 2025 00:46

vikaschoudhary16 added 2 commits February 5, 2025 18:01

Merge branch 'main' into lrl-dynamic-tokenbuckets

894a264

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

ci kick

45496e7

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

wbpcode reviewed Feb 9, 2025

View reviewed changes

Address review comments

eaaf2d4

Signed-off-by: Vikas Choudhary (vikasc) <choudharyvikas16@gmail.com>

vikaschoudhary16 requested a review from wbpcode February 10, 2025 01:19

wbpcode approved these changes Feb 10, 2025

View reviewed changes

repokitteh-read-only bot removed the api label Feb 10, 2025

wbpcode merged commit c32e210 into envoyproxy:main Feb 10, 2025
26 checks passed

zhaohuabing mentioned this pull request Feb 19, 2025

Support distinct match for local ratelimit envoyproxy/gateway#5315

Closed

This was referenced Feb 27, 2026

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#6

Closed

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#10

Merged

Conversation

vikaschoudhary16 commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Oct 16, 2024

Uh oh!

wbpcode commented Oct 16, 2024

Uh oh!

vikaschoudhary16 commented Oct 16, 2024

Uh oh!

wbpcode commented Oct 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vikaschoudhary16 commented Oct 21, 2024

Uh oh!

wbpcode commented Oct 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 24, 2024

Uh oh!

adisuissa commented Jan 23, 2025

Uh oh!

wbpcode commented Jan 26, 2025

Uh oh!

vikaschoudhary16 commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssawilk commented Jan 30, 2025

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vikaschoudhary16 Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

vikaschoudhary16 commented Oct 16, 2024 •

edited

Loading

wbpcode commented Oct 20, 2024 •

edited

Loading

vikaschoudhary16 commented Jan 27, 2025 •

edited

Loading

vikaschoudhary16 Feb 3, 2025 •

edited

Loading