ratelimit: new per descriptor hits-addend support and dynamic hits addend #37567

wbpcode · 2024-12-09T04:20:38Z

Commit Message: api: new per descriptor hits-addend support and dynamic hits addend
Additional Description:

Now, we could get custom hits_addend from the envoy.ratelimit.hits_addend. But if there are multiple rate limit filters that requrie custom hits_addend, the envoy.ratelimit.hits_addend couldn't meet the requirement.
And we cann't also to support different hits_addend for diffferent descriptots in same request.

This API changes try to meet above two requirements.

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

repokitteh-read-only · 2024-12-09T04:20:48Z

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

🐱

Caused by: #37567 was opened by wbpcode.

see: more, trace.

abeyad

@wbpcode should we add the implementation using this API to the PR so we can see how it'll be used?

abeyad · 2024-12-09T20:42:15Z

api/envoy/config/route/v3/route_components.proto

+    // For example, the ``%BYTES_RECEIVED%`` format string will be replaced with the number of bytes
+    // received in the request.
+    //
+    // Only one of the ``number`` or ``format`` fields can be set.


what will happen if both are set?

This is oneof semantics. (although oneof is not recommend according to our style).

If both are set, the configuration will be rejected.

abeyad · 2024-12-09T20:52:17Z

api/envoy/extensions/common/ratelimit/v3/ratelimit.proto

+  // Optional hits_addend for the rate limit descriptor. If set the value will override the
+  // request level hits_addend.
+  // [#not-implemented-hide:]
+  google.protobuf.UInt32Value hits_addend = 3;


In the request level, it can be an integer or a format string. How come here it is only an integer?

The format string is used to extract a number based on the request and stream info. See the comments of format string comment

// Substitution format string to extract the number of hits to add to the rate limit descriptor. // The same :ref:`format specifier <config_access_log_format>` as used for // :ref:`HTTP access logging <config_access_log>` applies here. // // .. note:: // The format string must contain only single valid substitution field that will be replaced // with a non-negative number. // // For example, the ``%BYTES_RECEIVED%`` format string will be replaced with the number of bytes // received in the request.

wbpcode · 2024-12-10T02:35:19Z

@wbpcode should we add the implementation using this API to the PR so we can see how it'll be used?

SGTM.

wbpcode · 2024-12-10T14:38:07Z

cc @abeyad I complete the local ratelimit version of the per descriptor custom hits addend support.

When the request is coming, we will generated a list of descriptors based on the configuration. If the hits_addend is configured, the filter will also generate a dynamic hits_addend value based on the request info and configuration.

When a descriptor matchs a rule, in the previous implementation, the fixed value 1 will be used as hits addend. Now, in the new implementation, the custom hits_addend that generated in the previous step will be used.

wbpcode · 2024-12-10T14:55:15Z

envoy/ratelimit/ratelimit.h

+/**
+ * A single rate limit request descriptor. See ratelimit.proto.
+ * This is generated from the request based on the configured rate limit actions.
+ */
+struct Descriptor : public DescriptorBase {
+  absl::optional<RateLimitOverride> limit_ = absl::nullopt;
+  absl::optional<uint32_t> hits_addend_ = absl::nullopt;
+};
+
+using LocalDescriptor = DescriptorBase;


In the previous implementation, the class LocalDescriptor has two usages:

as the key of rate limit rules that every descriptor is related to a token bucket.

as the output of the descriptor populating of local rate limt filter. It will be used to find a matched rate limit rule.

But now, to support per descriptor hits_addend, we need a new class to represent the output of the descriptor populating. Finally, we choose to enhance and re-use the Descriptor class. (The Descriptor class is used as the output of global rate limit filter. Re-using it could also simplify future development when we want to add similar support for global rate limit filter in the future.)

wbpcode · 2024-12-10T18:15:22Z

/retest

abeyad

/lgtm api

arkodg · 2024-12-10T23:31:02Z

api/envoy/config/route/v3/route_components.proto

  Override limit = 4;
+
+  // An optional hits addend to be appended to the descriptor produced by this rate limit


instead of appending to the descriptor, it should be used to populate the hits_addend field in the RLS request
https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/ratelimit/v3/rls.proto#service-ratelimit-v3-ratelimitrequest

Single RateLimit message is used to generate a single descriptor. The RateLimit.hits_addend field here is also a descriptor level configuration. So, the generated value of RateLimit.hits_addend field here should also be populated to the per-descriptor Descriptor.hits_addend rather than request level RateLimitRequest.hits_addend.

shouldnt this be one level up, under https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#config-route-v3-ratelimit ? so it can set the hits_addend field of https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/ratelimit/v3/rls.proto#service-ratelimit-v3-ratelimitrequest ?
The ratelimit request holds a list of descriptors

arkodg · 2024-12-10T23:33:01Z

api/envoy/config/route/v3/route_components.proto

@@ -2168,16 +2169,47 @@ message RateLimit {
    }
  }

+  message HitsAddend {


should this look more like https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/core/v3/base.proto#config-core-v3-datasource ?

sorry, but I didn't get what you mean. 🤣 This HitsAddend is only used to configure single fixed number of a dynamic formatter provider.

yeah what I was getting at, is there any anything existing thats reusable ?

for e.g. the style used here https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/http/custom_response/local_response_policy/v3/local_response_policy.proto#extensions-http-custom-response-local-response-policy-v3-localresponsepolicy is body and body_format

wbpcode · 2024-12-11T03:05:51Z

Hi, @mattklein123 , could you take a look when you get some free time? Thansk. This PR only contain local-ratelimit related change. And I will create a new PR to support global rate limit after this is done.

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

wbpcode · 2024-12-11T16:37:40Z

Finally, reduced 50% code changes. I have tried my best to reduce the complexity of review. orz.

repokitteh-read-only bot added the api label Dec 9, 2024

repokitteh-read-only bot assigned abeyad Dec 9, 2024

abeyad reviewed Dec 9, 2024

View reviewed changes

wbpcode requested a review from mattklein123 as a code owner December 10, 2024 13:57

wbpcode changed the title ~~api: new per descriptor hits-addend support and dynamic hits addend~~ ratelimit: new per descriptor hits-addend support and dynamic hits addend Dec 10, 2024

wbpcode commented Dec 10, 2024

View reviewed changes

wbpcode force-pushed the dev-per-descriptor-hits-adden-support branch from 2e2b2c3 to dc93699 Compare December 10, 2024 16:36

abeyad previously approved these changes Dec 10, 2024

View reviewed changes

repokitteh-read-only bot removed the api label Dec 10, 2024

arkodg reviewed Dec 10, 2024

View reviewed changes

mathetake mentioned this pull request Dec 11, 2024

http ratelimit: option to reduce budget on stream done #37548

Open

wbpcode assigned mattklein123 and unassigned abeyad Dec 11, 2024

wbpcode dismissed abeyad’s stale review via d3f28fd December 11, 2024 07:36

wbpcode requested a review from zuercher as a code owner December 11, 2024 10:36

wbpcode requested a review from kyessenov as a code owner December 11, 2024 13:55

rate limit: add per-descriptor custom hits addend support

774066d

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

wbpcode force-pushed the dev-per-descriptor-hits-adden-support branch from 18489a3 to 774066d Compare December 11, 2024 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ratelimit: new per descriptor hits-addend support and dynamic hits addend #37567

ratelimit: new per descriptor hits-addend support and dynamic hits addend #37567

wbpcode commented Dec 9, 2024

repokitteh-read-only bot commented Dec 9, 2024

abeyad left a comment

abeyad Dec 9, 2024

wbpcode Dec 10, 2024

abeyad Dec 9, 2024

wbpcode Dec 10, 2024

wbpcode commented Dec 10, 2024

wbpcode commented Dec 10, 2024

wbpcode Dec 10, 2024 •

edited

Loading

wbpcode commented Dec 10, 2024

abeyad left a comment

arkodg Dec 10, 2024

wbpcode Dec 11, 2024 •

edited

Loading

arkodg Dec 11, 2024

arkodg Dec 10, 2024

wbpcode Dec 11, 2024

arkodg Dec 11, 2024

arkodg Dec 11, 2024

wbpcode commented Dec 11, 2024 •

edited

Loading

wbpcode commented Dec 11, 2024

		Override limit = 4;

		// An optional hits addend to be appended to the descriptor produced by this rate limit

ratelimit: new per descriptor hits-addend support and dynamic hits addend #37567

Are you sure you want to change the base?

ratelimit: new per descriptor hits-addend support and dynamic hits addend #37567

Conversation

wbpcode commented Dec 9, 2024

repokitteh-read-only bot commented Dec 9, 2024

abeyad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode commented Dec 10, 2024

wbpcode commented Dec 10, 2024

wbpcode Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

wbpcode commented Dec 10, 2024

abeyad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode commented Dec 11, 2024 • edited Loading

wbpcode commented Dec 11, 2024

wbpcode Dec 10, 2024 •

edited

Loading

wbpcode Dec 11, 2024 •

edited

Loading

wbpcode commented Dec 11, 2024 •

edited

Loading