Add the ability to set the hits_addend for a given rate_limit request via a hardcoded dynamic metadata field #34184

EItanya · 2024-05-16T02:02:43Z

Commit Message: Adds the ability to set the hits_addend for a given rate_limit request via a hardcoded dynamic metadata field: envoy.ratelimit:hits_addend.
Additional Description:
Risk Level: Low
Testing: Added unit test. I have also manually tested this using gloo-edge as the control-plane.
Docs Changes:
Release Notes:
Platform Specific Features: N/A
[Optional Runtime guard:] N/A
[Optional Fixes #Issue] N/A
[Optional Fixes commit #PR or SHA] N/A
[Optional Deprecated:] N/A
[Optional API Considerations:] N/A

repokitteh-read-only · 2024-05-16T02:02:47Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #34184 was opened by EItanya.

see: more, trace.

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

EItanya · 2024-05-16T02:12:33Z

The changes to ratelimit.cc are meant mostly as a conversation starter. I originally debated adding a MetadataKey to the Route spec to be able to generically specify a metadata key, but that felt overkill for such a small feature so I decided to start here. I can also of course make envoy.ratelimit a constant a la envoy.lb, but I wasn't sure of the appetite for that.

I'm also not sure exactly where the docs should live for this feature. I definitely think that updating the release notes make sense and I can do that, but it could definitely also make sense to add it to the proper docs.

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

mattklein123 · 2024-05-20T14:16:40Z

At a high level this seems fine to me. For documentation I would add to the filter specific documentation as well as the release notes.

/wait

EItanya · 2024-05-20T19:17:13Z

Ok, I'll add the documentation, are there any code changes you'd like me to make while I'm at it?

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

repokitteh-read-only · 2024-05-24T18:30:21Z

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

🐱

Caused by: #34184 was synchronize by EItanya.

see: more, trace.

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

…addend_dynamic_metadata Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

jmarantz · 2024-05-30T16:19:16Z

/retest

jmarantz · 2024-05-30T21:31:24Z

format failure

/wait

EItanya · 2024-06-03T15:24:19Z

@jmarantz is there a good way to ensure all of my tools are up to date? When I run formatting it comes up with no diff. Or potentially does the CI job have the diff I need to apply?

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

source/extensions/filters/http/ratelimit/ratelimit.cc

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

wbpcode · 2024-06-05T02:31:35Z

api/envoy/service/ratelimit/v3/rls.proto

+  // This value can be modified by setting "envoy.ratelimit:hits_addend" in the dynamic metadata
+  // for a given request.


In most of cases, we prefer do similar things in the FilterState rather than dynamic metadata (like various sock options in the filter state and other dynamic configuration from filter state)? Is there any reason prefer the dynamic metadata?

cc @mattklein123

I based this off of envoy_lb style decisions which can be made via DynamicMetadata. I don't have super strong feelings either way, but there are more easier ways to set DynamicMetadata via other filters so I slightly prefer this method.

envoy.lb use the metadata because the subset lb construct the match structure from the endpoint metadata.

Then, in this scenario, I think filter state should be preferred for these typed options.

in this case we use dynamic metadata as it allows to feed data in without the source filter being aware that it is for rate limit.
For example, we can use ext-auth server to add the metadata for the rate limit filter.

in this case we use dynamic metadata as it allows to feed data in without the source filter being aware that it is for rate limit. For example, we can use ext-auth server to add the metadata for the rate limit filter.

As far as I know, the ext_authz could only inject the dynamic metadata with namespace envoy.filters.http.ext_authz?

The use-case is that we want to dynamically increment the hits_addend value based on the contents of the body. We have a filter to process the body and then write the value to the dynamic_metadata. Maybe I'm not understanding the distinction between dynamic_metadata and filter_state. Typically I've used dynamic_metadata to internally pass data between multiple filters, but it seems like there's nuance here I'm missing.

Here are some discussion about the filter state and dynamic metadata #25130

Basically, we prefer to use filter state for non-proto/JSON-like data.

Alright, I plan to use the UInt32Accessor class. Does that make sense?

@wbpcode does that sound correct?

We have a filter to process the body and then write the value to the dynamic_metadata

Would this also allow forwarding the call to the downstream service and e.g. taking something off the response header for hits_addend before getting to the rate limiter filter, as described here? envoyproxy/ratelimit#636 (comment)

…oy into hits_addend_dynamic_metadata

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

…addend_dynamic_metadata

wbpcode · 2024-07-29T08:15:34Z

api/envoy/service/ratelimit/v3/rls.proto

@@ -49,6 +49,9 @@ message RateLimitRequest {

  // Rate limit requests can optionally specify the number of hits a request adds to the matched
  // limit. If the value is not set in the message, a request increases the matched limit by 1.
+  // This value can be overridden by setting filter state value `envoy.ratelimit.hits_addend`
+  // to the desired number. Invalid number (< 0) or number will be ignored.
+


This is a unnecessary empty line.

wbpcode

Thanks so much for the update and all patience. And so sorry for the delay. This PR is lost in my timeline. (hah, this is not assigned to me. So, the pr reminder doesn't work, but anyway, it's assigned now.)

Almost everything is fine now except two nit comments. And feel free to ping me at slack if you are ready. Then we can merge this PR in time.

source/extensions/filters/http/ratelimit/ratelimit.cc

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

EItanya · 2024-07-29T12:51:23Z

Responded to all comments, thanks for reviewing and re-assigning.

test/extensions/filters/http/ratelimit/ratelimit_test.cc

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

wbpcode

LGTM. Thanks so much for the long time work. 🌹

wbpcode · 2024-07-29T15:51:22Z

api/envoy/service/ratelimit/v3/rls.proto

@@ -49,6 +49,8 @@ message RateLimitRequest {

  // Rate limit requests can optionally specify the number of hits a request adds to the matched
  // limit. If the value is not set in the message, a request increases the matched limit by 1.
+  // This value can be overridden by setting filter state value `envoy.ratelimit.hits_addend`


``envoy.ratelimit.hits_addend``

Please fix the CI, these is a issue that I know. Double backticks should be used.

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

Signed-off-by: code <wbphub@gmail.com>

wbpcode

LGTM. Thanks. Pray for CI 🙏

… via a hardcoded dynamic metadata field (envoyproxy#34184)  Commit Message: Adds the ability to set the hits_addend for a given rate_limit request via a hardcoded dynamic metadata field: envoy.ratelimit:hits_addend. Additional Description: Risk Level: Low Testing: Added unit test. I have also manually tested this using gloo-edge as the control-plane. Docs Changes: Release Notes: Platform Specific Features: N/A [Optional Runtime guard:] N/A [Optional Fixes #Issue] N/A [Optional Fixes commit #PR or SHA] N/A [Optional Deprecated:] N/A [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] N/A --------- Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io> Signed-off-by: code <wbphub@gmail.com> Co-authored-by: code <wbphub@gmail.com> Signed-off-by: Martin Duke <martin.h.duke@gmail.com>

zirain · 2024-08-10T12:30:39Z

source/extensions/filters/http/ratelimit/ratelimit.cc

 #include "source/extensions/filters/http/ratelimit/ratelimit_headers.h"

 namespace Envoy {
 namespace Extensions {
 namespace HttpFilters {
 namespace RateLimitFilter {

+namespace {
+constexpr absl::string_view HitsAddendFilterStateKey = "envoy.ratelimit.hits_addend";


@mattklein123 can we make this key configurable per filter?

… via a hardcoded dynamic metadata field (envoyproxy#34184)  Commit Message: Adds the ability to set the hits_addend for a given rate_limit request via a hardcoded dynamic metadata field: envoy.ratelimit:hits_addend. Additional Description: Risk Level: Low Testing: Added unit test. I have also manually tested this using gloo-edge as the control-plane. Docs Changes: Release Notes: Platform Specific Features: N/A [Optional Runtime guard:] N/A [Optional Fixes #Issue] N/A [Optional Fixes commit #PR or SHA] N/A [Optional Deprecated:] N/A [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] N/A --------- Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io> Signed-off-by: code <wbphub@gmail.com> Co-authored-by: code <wbphub@gmail.com> Signed-off-by: asingh-g <abhisinghx@google.com>

hits_addend_dynamic_metadata

673a527

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

EItanya force-pushed the hits_addend_dynamic_metadata branch from a2d61d8 to 673a527 Compare May 16, 2024 02:03

EItanya mentioned this pull request May 16, 2024

Envoy to pass hits_addend to RateLimitService #12969

Open

clang-format

79b6103

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

EItanya marked this pull request as ready for review May 16, 2024 02:29

EItanya requested a review from mattklein123 as a code owner May 16, 2024 02:29

ravenblackx assigned mattklein123 May 16, 2024

repokitteh-read-only bot added the waiting label May 20, 2024

docs

560bbd1

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

repokitteh-read-only bot added api and removed waiting labels May 24, 2024

EItanya added 2 commits May 24, 2024 18:30

merge commit

5c123a2

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

Merge branch 'main' of https://github.com/envoyproxy/envoy into hits_…

b93fed1

…addend_dynamic_metadata Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

repokitteh-read-only bot added the waiting label May 30, 2024

formatting in changelog

5c357aa

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

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

docs

77c211f

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

ashishb-solo reviewed Jun 3, 2024

View reviewed changes

source/extensions/filters/http/ratelimit/ratelimit.cc Show resolved Hide resolved

changelog merge

41ea807

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

wbpcode reviewed Jun 5, 2024

View reviewed changes

wbpcode self-assigned this Jun 5, 2024

EItanya requested a review from wbpcode June 5, 2024 13:25

EItanya added 2 commits July 26, 2024 17:26

Merge branch 'hits_addend_dynamic_metadata' of github.com:eitanya/env…

224db24

…oy into hits_addend_dynamic_metadata

add well known filter state doc

4131e40

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

EItanya mentioned this pull request Jul 26, 2024

Add filter-state rate limit hits_addend modifier patch solo-io/envoy-gloo#358

Merged

EItanya added 3 commits July 26, 2024 19:58

test was incorrect

c56320a

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

rename from earlier copy/paste

9288df9

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

Merge branch 'main' of https://github.com/envoyproxy/envoy into hits_…

e4d8c8e

…addend_dynamic_metadata

wbpcode reviewed Jul 29, 2024

View reviewed changes

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

wbpcode reviewed Jul 29, 2024

View reviewed changes

source/extensions/filters/http/ratelimit/ratelimit.cc Show resolved Hide resolved

wbpcode self-assigned this Jul 29, 2024

PR comments for coverage, testing, and formatting

73b73e3

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

wbpcode reviewed Jul 29, 2024

View reviewed changes

test/extensions/filters/http/ratelimit/ratelimit_test.cc Outdated Show resolved Hide resolved

move test into anonymous NS

9e7bc55

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

wbpcode previously approved these changes Jul 29, 2024

View reviewed changes

repokitteh-read-only bot removed the api label Jul 29, 2024

wbpcode reviewed Jul 29, 2024

View reviewed changes

double backticks

20a2109

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

EItanya dismissed wbpcode’s stale review via 20a2109 July 29, 2024 16:04

repokitteh-read-only bot added the api label Jul 29, 2024

Merge branch 'main' into hits_addend_dynamic_metadata

19f5e43

Signed-off-by: code <wbphub@gmail.com>

wbpcode approved these changes Aug 1, 2024

View reviewed changes

repokitteh-read-only bot removed the api label Aug 1, 2024

wbpcode enabled auto-merge (squash) August 1, 2024 04:25

wbpcode merged commit 9a474a3 into envoyproxy:main Aug 1, 2024
50 of 51 checks passed

EItanya deleted the hits_addend_dynamic_metadata branch August 1, 2024 12:07

zirain mentioned this pull request Aug 8, 2024

Send ratelimit request to ratelimit service on response flow with hits_addend #29161

Closed

zirain reviewed Aug 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to set the hits_addend for a given rate_limit request via a hardcoded dynamic metadata field #34184

Add the ability to set the hits_addend for a given rate_limit request via a hardcoded dynamic metadata field #34184

EItanya commented May 16, 2024

repokitteh-read-only bot commented May 16, 2024

EItanya commented May 16, 2024

mattklein123 commented May 20, 2024

EItanya commented May 20, 2024

repokitteh-read-only bot commented May 24, 2024

jmarantz commented May 30, 2024

jmarantz commented May 30, 2024

EItanya commented Jun 3, 2024

wbpcode Jun 5, 2024 •

edited

Loading

EItanya Jun 5, 2024

wbpcode Jun 6, 2024

yuval-k Jun 6, 2024

wbpcode Jun 11, 2024 •

edited

Loading

EItanya Jun 18, 2024

wbpcode Jun 19, 2024

EItanya Jun 20, 2024

EItanya Jun 24, 2024

austince Jul 2, 2024 •

edited

Loading

wbpcode Jul 29, 2024

wbpcode left a comment •

edited

Loading

EItanya commented Jul 29, 2024

wbpcode left a comment

wbpcode Jul 29, 2024

wbpcode left a comment •

edited

Loading

zirain Aug 10, 2024

		// This value can be modified by setting "envoy.ratelimit:hits_addend" in the dynamic metadata
		// for a given request.

Add the ability to set the hits_addend for a given rate_limit request via a hardcoded dynamic metadata field #34184

Add the ability to set the hits_addend for a given rate_limit request via a hardcoded dynamic metadata field #34184

Conversation

EItanya commented May 16, 2024

repokitteh-read-only bot commented May 16, 2024

EItanya commented May 16, 2024

mattklein123 commented May 20, 2024

EItanya commented May 20, 2024

repokitteh-read-only bot commented May 24, 2024

jmarantz commented May 30, 2024

jmarantz commented May 30, 2024

EItanya commented Jun 3, 2024

wbpcode Jun 5, 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

wbpcode Jun 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

austince Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode left a comment • edited Loading

Choose a reason for hiding this comment

EItanya commented Jul 29, 2024

wbpcode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode Jun 5, 2024 •

edited

Loading

wbpcode Jun 11, 2024 •

edited

Loading

austince Jul 2, 2024 •

edited

Loading

wbpcode left a comment •

edited

Loading

wbpcode left a comment •

edited

Loading