Make default hitsAddend minimum value configurable

[gcalmettes]

fix: #730

In the current implementation of the ratelimit service, the hitsAddend value that is associated with each request is set to a minimum of 1.
This is done to ensure that if the RateLimit request does not specify any request.hitsAddend (like what the envoy ratelimit filter does by default if no envoy.ratelimit.hits_addend has been defined), the hit is still counted.

This minimum hitsAddend of 1 is enforced via a Max(1, <hitsAddend from request>) function.

This PR adds the possibility to configure the minimum value of the hitsAddend via a settings, so 0 can also be used as minimum hitsAddend value if needed. The current behavior of 1 being the default value is preserved, and if the environment variable is not set, then a value of 1 is applied, like currently.

The ability to configure a minimum hitsAddend to zero can help for example for the following use cases:

• having the possibility to send a RateLimitRequest to check the current remaining rates without consuming a quota (e.g.: Add a way to check rate limits without using quota #244)
• supporting the cases when the "weight" of the request is not known during the request but on the response (e.g.: rate limiting llm tokens as per Send ratelimit request to ratelimit service on response flow with hits_addend envoy#29161 (comment)). In this case, we still want to ratelimit on the request, but we do not want the request itself to add an additional arbitrary 1 value to the real weight defined as hitsAddend.

[gcalmettes]

@envoyproxy/ratelimit-maintainers

@zirain I tried to ping @envoyproxy/ratelimit-maintainers like described in the CONTRIBUTING.md document but it seems that the handle is not available. Any chance you would know who I could ping for this PR ? Thanks a lot.

[zirain]

I think @mattklein123 is the person, hope he have some free slot to review.

[gcalmettes]

Any chance you could take a look @mattklein123 ? Thanks a lot.

[github-actions]

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!

[gcalmettes]

commenting to prevent automatic closing by stale-bot

[arkodg]

can we do w/o this extra field ?
we'll need to make HitsAddend optional/ptr in the API https://github.com/envoyproxy/go-control-plane/blob/e0e383246f4cd59a40967987057c76f03526cffb/envoy/service/ratelimit/v3/rls.pb.go#L179
which may be a breaking change
cc @zirain @mathetake @wbpcode

[mathetake]

if this still defaults to 1 then i don't think this would be a breaking change in practice i guess? but ack we should need to allow zero in the API regardless

[gcalmettes]

@arkodg I confirm that this is not a breaking change and that the HitsAddendMinValue field introduced here is not needed in the RatelimitRequest request send to the ratelimit service.

This is only a config variable of the reatelimit service, with default value of 1 that matches the current hardcoded value of 1.

@mathetake there is no change to be done to the API, as zero is already a valid value for the HitsAddend value. In fact it is the default value when no custom HitsAddend value is set via the Set-Filter-State HTTP filter or directly in the RatelimitRequest and the actual +1 increment is enforced at the ratelimit service level (if the hitAddend value is set to zero in the request -- either voluntarily or because it is absent in the request-- the value is override to 1).

Currently, if someone uses the FilterState to enforce the hitsAddend value voluntarily to zero (we have a use case for that) or set the HitsAddend value to zero in the RatelimitRequest, then in practice the limit counter will still be incremented because of the max(request.HitsAddend, 1) check that is done, which is not expected.

What this PR does basically is just to allow to change this hardcoded 1 to any value >=0 (same constraints than the api, but still keeping 1 as default value to not change the actual behavior) to allow RatelimitRequests with zero increment.

[arkodg]

hey @gcalmettes, we understand the issue and our use case is the same as yours :)
I was questioning the current RLS API and the semantics defined in RateLimitRequest , since its not a pointer, there's no way to figure out unset (default:1 ) vs set to 0.
It maybe too late to edit that API, the easiest thing to do maybe is

• set the default value to 1 in envoy if HitsAddend is not set
• update the code in this project to max(request.HitsAddend, 0) simplifying the logic and maintaining backwards compatibility
  wdyt @mathetake @wbpcode

[arkodg]

sounds like you had the same idea @mathetake #729 (comment)

[wbpcode]

What's is the behavior of zero hits_addend? I think we use 1 as check and CUSTOM_HITS_ADDEND -1 as report, but if 0 could aslo be used as check, maybe it's simpler. cc @mathetake cc @arkodg

[wbpcode]

/wait

[mathetake]

So currently, RateLimitRequest API assumes that null (== zero in Go level) addend should be interpreted as 1:

(uint32) 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.

so if we change the default value to whatever number to non one, then RSL doesn't comply with the API comment in other words. In fact, Envoy's current implementation relies on the fact that "zero must be interpreted as 1":

https://github.com/envoyproxy/envoy/blob/ab9ff8e5aa5735d3bb796bd27c44638c292c5593/source/extensions/filters/http/ratelimit/ratelimit.cc#L92-L95

But I think we could make changes to Envoy as well that

• remove the comment of defaulting to 1
• explicitly use 1 as a default value in the implementation

wdyt

[wbpcode]

Change the exist behavior is not recommended. I would suggest to use per-descriptor hits_addend to replace the per-request hits_addend. Note that every descriptor is evaluated independent anyway. The new per-descriptor hits_addend is more flexible and has no history burden.

The null of per-descriptor hits_addend could be treat as default to reuse request level descriptor.
The explicit 0 could be treat a check only.
The non-zero could be treat a check and report.

[mathetake]

I would suggest to use per-descriptor hits_addend to replace the per-request hits_addend. Note that every descriptor is evaluated independent anyway. The new per-descriptor hits_addend is more flexible and has no history burden.

sounds good, is that the feature-to-be-added in envoyproxy/envoy#37567?

I think then the per-descriptor addend combined with this configuration makes our life easier

[wbpcode]

I would suggest to use per-descriptor hits_addend to replace the per-request hits_addend. Note that every descriptor is evaluated independent anyway. The new per-descriptor hits_addend is more flexible and has no history burden.

sounds good, is that the feature-to-be-added in envoyproxy/envoy#37567?

I think then the per-descriptor addend combined with this configuration makes our life easier

Yeah. The #37567 is the first PR to add API and local rate limit implementation. Then I will add a PR to support it for global ratelimit. At that PR is #37567 is merged, we can do the per-descriptor support the envoyproxy/ratelimit as same time.