local rate limit: add new rate_limits support to the filter #36099

wbpcode · 2024-09-12T15:17:54Z

Commit Message: local rate limit: add new rate_limits api to the filter's api
Additional Description:

In the previous local rate limit, the rate_limits field of route is used to generate the descriptor entries. Then the generated entries will be used to match a token bucket which is configured in the filter configs (route level, vhost level, etc).

However, it make the configuration very complex, and cannot cover some common scenarios easily. For example, give a specific virtual host X and a special route Y that under this virtual host X.

We want to provides a virtual host level rate limit for the specific virtual host X, and a route level rate limit for the specific route Y. We hope the configuration of virtual host could works for all routes except the Y.

For most filters, this requirement could be achieved by getting the most specific filter config and applying it. But for the local rate limit, thing become very complex. Because the rate limit configuration is split into rate_limits field of route and the filter config. The local rate limit need to handle these relationship carefully.

This PR try to simplify it.

Risk Level: low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

Signed-off-by: wangbaiping <[email protected]>

repokitteh-read-only · 2024-09-12T15:18:05Z

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

🐱

Caused by: #36099 was opened by wbpcode.

see: more, trace.

wbpcode · 2024-09-12T15:22:52Z

cc @tyxia I think you did similiar thing to the remote rate limit filter in the #18044 But you put that API under the extensions/filters/http/ratelimit, so I cannot reuse it. And still doesn't have the implementation.

Signed-off-by: wangbaiping <[email protected]>

wbpcode · 2024-09-13T10:28:25Z

/retest

kyessenov

Thanks for doing this. We had an issue somewhere about de-coupling rate limit from core config -- this is the right direction IMO.

…ocal-rate-limit-api

wbpcode · 2024-09-14T14:17:52Z

/retest

tyxia

LGTM, Thanks @wbpcode

Should http/ratelimit also just use the one you added in common/ratelimit in the future?

wbpcode · 2024-09-14T16:25:07Z

LGTM, Thanks @wbpcode

Should http/ratelimit also just use the one you added in common/ratelimit in the future?

I think it's okay. But note the current API of http/ratelimit had been added for a long time.
According to our API policy, we couldn't change it.

But I think we can treat it as special case. It's never be implemented by Envoy and I think no third party xds clients will use it.

adisuissa

Thanks for tackling this!
I agree that making the API easier to use is something we should strive for.
I do wonder whether this should be a different proto. WDYT?

wbpcode · 2024-09-16T13:57:59Z

Thanks for tackling this! I agree that making the API easier to use is something we should strive for. I do wonder whether this should be a different proto. WDYT?

Do you mean should we re-use the RateLimit in the route.v3? I initially think about it. But there some fields that will never be supported in the local rate limit. So, I finally create a new proto message. And will gradually to deprecate the previous Ratelimit's support in the local rate limit extensions.

adisuissa · 2024-09-16T20:26:57Z

Do you mean should we re-use the RateLimit in the route.v3? I initially think about it. But there some fields that will never be supported in the local rate limit. So, I finally create a new proto message. And will gradually to deprecate the previous Ratelimit's support in the local rate limit extensions.

What I'm trying to understand is whether this should be added to the current local-rate-limit proto as you are currently suggesting, or should it be a new local-rate-limit.
This is highly dependent on the how the internal fields in the new RateLimitConfig config work well with the other fields in the LocalRateLimit config. If they don't align well together, I think it should be a configuration in a new local-rate-limiter (new proto), because sharing the config doesn't make sense in this case.

wbpcode · 2024-09-16T23:21:14Z

Do you mean should we re-use the RateLimit in the route.v3? I initially think about it. But there some fields that will never be supported in the local rate limit. So, I finally create a new proto message. And will gradually to deprecate the previous Ratelimit's support in the local rate limit extensions.

What I'm trying to understand is whether this should be added to the current local-rate-limit proto as you are currently suggesting, or should it be a new local-rate-limit.
This is highly dependent on the how the internal fields in the new RateLimitConfig config work well with the other fields in the LocalRateLimit config. If they don't align well together, I think it should be a configuration in a new local-rate-limiter (new proto), because sharing the config doesn't make sense in this case.

Oh, then I will prefer to place it in the extensions/common as a new proto. Because the config self is not local rate limit specific thing. It could be shared by other rate limit module in future, just like the @tyxia 's comment.

Signed-off-by: wangbaiping <[email protected]>

…ocal-rate-limit-new-implementation

Signed-off-by: wangbaiping <[email protected]>

…ocal-rate-limit-new-implementation

wbpcode · 2024-09-19T15:02:32Z

friendly ping @adisuissa

wbpcode · 2024-09-21T09:32:12Z

I found I mis-understood @adisuissa again :( sorry. The new rate_limits is used to generate the descriptor list which will replace the rate_limits (https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-field-config-route-v3-virtualhost-rate-limits) in the route and vhost.

By this way, the local rate limit filter could work without dependency to the core route configuration. And could resolve the problems in the PR description.

I completed the implementation. So, you could take a quick look at that to ensure it could work as expected.

Signed-off-by: wangbaiping <[email protected]>

…ocal-rate-limit-api

Signed-off-by: wangbaiping <[email protected]>

…ocal-rate-limit-api

wbpcode · 2024-09-25T15:49:16Z

Hi, @mattklein123 , @adisuissa and me basically get agreement on the API. Could you take another look to both the API and implementation when you get some free time? Thanks.

…ocal-rate-limit-api

wbpcode · 2024-09-26T06:57:54Z

/retest

wbpcode · 2024-10-01T09:54:04Z

Friendly ping @mattklein123 :)

mattklein123

LGTM at a high level. Few small comments.

/wait

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

mattklein123 · 2024-10-02T23:03:41Z

source/extensions/filters/common/ratelimit_config/ratelimit_config.cc

+  if (config.has_stage() || !config.disable_key().empty()) {
+    ENVOY_LOG(warn, "'stage' field and 'disable_key' field are not supported");
+  }
+
+  if (config.has_limit()) {
+    if (no_limit) {
+      ENVOY_LOG(warn, "'limit' field is only supported in filter that calls remote rate "
+                      "limit service.");
+    }
+  }


Should these return errors that lead to config rejection?

Configuration rejection actually bring some risks in production env.

I agree we should reject the configuration if there are more serious problem like unknown factory for extension rate limit policy.

But specific to these fields, basically I inclined to flush warning log because these fields won't effect the feature

Feel free to let me know if you think this is a block point.

People generally don't notice warnings. For this case I would make it a failure. People should be checking for config failures.

mattklein123 · 2024-10-02T23:10:50Z

source/extensions/filters/http/local_ratelimit/local_ratelimit.cc

+  if (rate_limit_config_->empty()) {
+    if (!config.descriptors().empty()) {
+      ENVOY_LOG_FIRST_N(
+          warn, 20,
+          "'descriptors' are set for local rate limit filter but no 'rate_limits' "
+          "are configured in the filter config. Please use the 'rate_limits' field "
+          "in filter config to instead of the 'rate_limits' field in the route config.");
+    }
+  }


Shouldn't this cause a config load failure?

Nope. This warning will notice the users to switch the rate_limits from route to the embedded rate_limits of filter config.

But considering the backward compatibility, before we removed the support to the route rate_limits completely in the filter, we shouldn't reject the configuration that without embedded rate_limits.

Then sorry I don't understand this warning. Can you make it more clear?

Let me have a try make it more clear.

I have updated the log message. Hope it makes sense for you. The core points are:

descriptors only makes sense when rate_limits (of filter config or route) is configured.

Now, if the rate_limits in filter config is not set, may be it's set in the route. Please take the rate_limits of filter config as priority because we may remove the support of rate_limits of route config from the local rate limit filter in the future.

…e_limit.proto Co-authored-by: Matt Klein <[email protected]> Signed-off-by: code <[email protected]>

Signed-off-by: wangbaiping <[email protected]>

…/envoy into dev-local-rate-limit-api

…ocal-rate-limit-api

wbpcode · 2024-10-03T15:15:37Z

/retest

wbpcode · 2024-10-03T23:30:05Z

/retest

mattklein123 · 2024-10-04T22:52:27Z

source/extensions/filters/http/local_ratelimit/local_ratelimit.cc

+  if (rate_limit_config_->empty()) {
+    if (!config.descriptors().empty()) {
+      ENVOY_LOG_FIRST_N(
+          warn, 20,
+          "'descriptors' is set but no valid 'rate_limits' in LocalRateLimit config. Note that "
+          "'descriptors' only makes sense when 'rate_limits' in LocalRateLimit config or route "
+          "config is specified. And please take the 'rate_limits' in the LocalRateLimit config as "
+          "priority because the 'rate_limits' in the route config may be ignored by the filter in "
+          "the future.");
+    }
+  }


Sorry for being dense but I still don't understand why this has to be a runtime failure? This is a new feature, right? If the user configured it incorrectly can we fail to load the config?

/wait-any

For backward compatibility.

Note, in the previous local ratelimit, the local ratelimit's descriptors field will be used to declare the rate limit config of specific descriptor, like 10 requests per second for descriptor (key1, value1), etc.

And the rate_limits in the route configuration will be used to generate specific descriptor (key, value) of request and the descriptor will be used to match a config in the descriptors field (The naming is a little confusing because the historical reason.)

But previous implementation has some drawbacks in actual practices (see PR description), so I added the new rate_limits field in the filter config directly as an alternative and resolve previous drawbacks.

But we cannot reject the filter config which doesn't contains the new rate_limits, because the users may have configured rate_limits in the route.

source/extensions/filters/http/local_ratelimit/local_ratelimit.cc

Co-authored-by: Matt Klein <[email protected]> Signed-off-by: code <[email protected]>

Signed-off-by: wangbaiping <[email protected]>

wbpcode · 2024-10-05T09:30:35Z

/retest

…xy#36099) Commit Message: local rate limit: add new rate_limits api to the filter's api Additional Description: In the previous local rate limit, the [rate_limits](https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-field-config-route-v3-virtualhost-rate-limits) field of route is used to generate the descriptor entries. Then the generated entries will be used to match a token bucket which is configured in the filter configs (route level, vhost level, etc). However, it make the configuration very complex, and cannot cover some common scenarios easily. For example, give a specific virtual host X and a special route Y that under this virtual host X. We want to provides a virtual host level rate limit for the specific virtual host X, and a route level rate limit for the specific route Y. We hope the configuration of virtual host could works for all routes except the Y. For most filters, this requirement could be achieved by getting the most specific filter config and applying it. But for the local rate limit, thing become very complex. Because the rate limit configuration is split into `rate_limits` field of route and the filter config. The local rate limit need to handle these relationship carefully. This PR try to simplify it. Risk Level: low. Testing: n/a. Docs Changes: n/a. Release Notes: n/a. Platform Specific Features: n/a. --------- Signed-off-by: wangbaiping <[email protected]> Signed-off-by: code <[email protected]> Co-authored-by: Matt Klein <[email protected]> Signed-off-by: Steven Jin Xuan <[email protected]>

local rate limit: add new rate_limits api to the filter's api

0090d86

Signed-off-by: wangbaiping <[email protected]>

repokitteh-read-only bot added the api label Sep 12, 2024

repokitteh-read-only bot assigned adisuissa Sep 12, 2024

address format

111aaf7

Signed-off-by: wangbaiping <[email protected]>

kyessenov reviewed Sep 13, 2024

View reviewed changes

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-l…

90da99b

…ocal-rate-limit-api

tyxia reviewed Sep 14, 2024

View reviewed changes

adisuissa reviewed Sep 16, 2024

View reviewed changes

wbpcode added 4 commits September 18, 2024 21:21

basic rate limit

68df70c

Signed-off-by: wangbaiping <[email protected]>

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-l…

e9f5510

…ocal-rate-limit-new-implementation

complete the implementation

cddefa7

Signed-off-by: wangbaiping <[email protected]>

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-l…

0dd7f9b

…ocal-rate-limit-new-implementation

wbpcode force-pushed the dev-local-rate-limit-api branch from 9669986 to 0dd7f9b Compare September 21, 2024 09:33

wbpcode requested a review from mattklein123 as a code owner September 21, 2024 09:33

fix format

5c01cd5

Signed-off-by: wangbaiping <[email protected]>

wbpcode changed the title ~~local rate limit: add new rate_limits api to the filter's api~~ local rate limit: add new rate_limits support to the filter Sep 21, 2024

wbpcode added 4 commits September 21, 2024 18:03

add change log

26efa09

Signed-off-by: wangbaiping <[email protected]>

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-l…

085b3aa

…ocal-rate-limit-api

fix docs

3638131

Signed-off-by: wangbaiping <[email protected]>

fix docs

a573560

Signed-off-by: wangbaiping <[email protected]>

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-l…

a3e2f62

…ocal-rate-limit-api

wbpcode assigned mattklein123 Sep 25, 2024

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-l…

de1a61d

…ocal-rate-limit-api

mattklein123 requested changes Oct 2, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 2, 2024

Update api/envoy/extensions/filters/http/local_ratelimit/v3/local_rat…

135975e

…e_limit.proto Co-authored-by: Matt Klein <[email protected]> Signed-off-by: code <[email protected]>

repokitteh-read-only bot removed the waiting label Oct 2, 2024

mattklein123 added the waiting label Oct 3, 2024

wbpcode added 2 commits October 3, 2024 21:35

address comments

3d5429a

Signed-off-by: wangbaiping <[email protected]>

Merge branch 'dev-local-rate-limit-api' of https://github.com/wbpcode…

384970f

…/envoy into dev-local-rate-limit-api

repokitteh-read-only bot removed the waiting label Oct 3, 2024

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-l…

7cf2a8b

…ocal-rate-limit-api

mattklein123 reviewed Oct 4, 2024

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting:any labels Oct 4, 2024

mattklein123 reviewed Oct 5, 2024

View reviewed changes

source/extensions/filters/http/local_ratelimit/local_ratelimit.cc Outdated Show resolved Hide resolved

wbpcode and others added 2 commits October 5, 2024 13:16

Update source/extensions/filters/http/local_ratelimit/local_ratelimit.cc

3b8d8b7

Co-authored-by: Matt Klein <[email protected]> Signed-off-by: code <[email protected]>

fix format

e9893ed

Signed-off-by: wangbaiping <[email protected]>

mattklein123 approved these changes Oct 5, 2024

View reviewed changes

repokitteh-read-only bot removed the api label Oct 5, 2024

wbpcode merged commit e486663 into envoyproxy:main Oct 5, 2024
22 checks passed

wbpcode deleted the dev-local-rate-limit-api branch October 5, 2024 23:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

local rate limit: add new rate_limits support to the filter #36099

local rate limit: add new rate_limits support to the filter #36099

wbpcode commented Sep 12, 2024 •

edited

Loading

repokitteh-read-only bot commented Sep 12, 2024

wbpcode commented Sep 12, 2024

wbpcode commented Sep 13, 2024

kyessenov left a comment

wbpcode commented Sep 14, 2024

tyxia left a comment

wbpcode commented Sep 14, 2024 •

edited

Loading

adisuissa left a comment

wbpcode commented Sep 16, 2024

adisuissa commented Sep 16, 2024

wbpcode commented Sep 16, 2024 •

edited

Loading

wbpcode commented Sep 19, 2024

wbpcode commented Sep 21, 2024 •

edited

Loading

wbpcode commented Sep 25, 2024

wbpcode commented Sep 26, 2024

wbpcode commented Oct 1, 2024

mattklein123 left a comment

mattklein123 Oct 2, 2024

wbpcode Oct 2, 2024 •

edited

Loading

mattklein123 Oct 3, 2024

wbpcode Oct 3, 2024

mattklein123 Oct 2, 2024

wbpcode Oct 2, 2024 •

edited

Loading

mattklein123 Oct 3, 2024

wbpcode Oct 3, 2024

wbpcode Oct 3, 2024 •

edited

Loading

wbpcode commented Oct 3, 2024

wbpcode commented Oct 3, 2024

mattklein123 Oct 4, 2024

wbpcode Oct 5, 2024 •

edited

Loading

wbpcode commented Oct 5, 2024

local rate limit: add new rate_limits support to the filter #36099

local rate limit: add new rate_limits support to the filter #36099

Conversation

wbpcode commented Sep 12, 2024 • edited Loading

repokitteh-read-only bot commented Sep 12, 2024

wbpcode commented Sep 12, 2024

wbpcode commented Sep 13, 2024

kyessenov left a comment

Choose a reason for hiding this comment

wbpcode commented Sep 14, 2024

tyxia left a comment

Choose a reason for hiding this comment

wbpcode commented Sep 14, 2024 • edited Loading

adisuissa left a comment

Choose a reason for hiding this comment

wbpcode commented Sep 16, 2024

adisuissa commented Sep 16, 2024

wbpcode commented Sep 16, 2024 • edited Loading

wbpcode commented Sep 19, 2024

wbpcode commented Sep 21, 2024 • edited Loading

wbpcode commented Sep 25, 2024

wbpcode commented Sep 26, 2024

wbpcode commented Oct 1, 2024

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Oct 2, 2024

Choose a reason for hiding this comment

wbpcode Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

mattklein123 Oct 3, 2024

Choose a reason for hiding this comment

wbpcode Oct 3, 2024

Choose a reason for hiding this comment

mattklein123 Oct 2, 2024

Choose a reason for hiding this comment

wbpcode Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

mattklein123 Oct 3, 2024

Choose a reason for hiding this comment

wbpcode Oct 3, 2024

Choose a reason for hiding this comment

wbpcode Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

wbpcode commented Oct 3, 2024

wbpcode commented Oct 3, 2024

mattklein123 Oct 4, 2024

Choose a reason for hiding this comment

wbpcode Oct 5, 2024 • edited Loading

Choose a reason for hiding this comment

wbpcode commented Oct 5, 2024

wbpcode commented Sep 12, 2024 •

edited

Loading

wbpcode commented Sep 14, 2024 •

edited

Loading

wbpcode commented Sep 16, 2024 •

edited

Loading

wbpcode commented Sep 21, 2024 •

edited

Loading

wbpcode Oct 2, 2024 •

edited

Loading

wbpcode Oct 2, 2024 •

edited

Loading

wbpcode Oct 3, 2024 •

edited

Loading

wbpcode Oct 5, 2024 •

edited

Loading