Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend RateLimitFilter to support multiple ordered RateLimit rules #5194

Open
nezdolik opened this issue Feb 3, 2025 · 10 comments
Open

Extend RateLimitFilter to support multiple ordered RateLimit rules #5194

nezdolik opened this issue Feb 3, 2025 · 10 comments
Labels
Milestone

Comments

@nezdolik
Copy link
Member

nezdolik commented Feb 3, 2025

Description:
In certain cases users may need to specify multiple ratelimit rules that should be applied in certain order. E.g. a global RL rule per ip and subsequent rule per ip per upstream/backend service. The Api could look like:

#first RL rule
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: RateLimitFilter
metadata:
  name: global-ip-rule
spec:
  type: Global
  priority: 0
  global:
    rules:
    - clientSelectors:
      - headers:
        - name: x-real-ip
      limit:
        requests: 3
        unit: Hour

#second RL rule
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: RateLimitFilter
metadata:
  name: per-service-per-ip-rule
spec:
  type: Global
  priority: 1
  global:
    rules:
    - clientSelectors:
      - headers:
        - name: x-real-ip
        - name: some-header-that-denotes-backend-name # destination cluster is calculated at router?
      limit:
        requests: 2
        unit: Hour

The config would translate into multiple Envoy filters ordered according to priority. Note, in my personal opinion is easier to operate with multiple envoy filters/multiple RL domains than try to order descriptors within single RL domain. This is due how envoy ratelimit service descriptor matcing algo works, e.g. match the longest descriptor first (which will not work for the example above). So Envoy filter chain config would look like:

- name: envoy.filters.http.ratelimit
  typed_config:
    "@type": type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit
    domain: global-ip-rule
    ...
- name: envoy.filters.http.ratelimit
  typed_config:
    "@type": type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit
    domain: per-service-per-ip-rule
    ...

And RL service descriptors will be populated accordingly.
Looking forward to feedback, if this proposal makes sense.

[optional Relevant Links:]

Any extra documentation required to understand the issue.

@nezdolik nezdolik added the triage label Feb 3, 2025
@nezdolik
Copy link
Member Author

nezdolik commented Feb 3, 2025

cc @envoyproxy/gateway-maintainers

@arkodg
Copy link
Contributor

arkodg commented Feb 3, 2025

@nezdolik this API RateLimitFilter has been removed (although chatgpt still remembers it :) ), and RateLimit exists in BackendTrafficPolicy https://gateway.envoyproxy.io/docs/tasks/traffic/global-rate-limit/

@rsaelim
Copy link

rsaelim commented Feb 5, 2025

@arkodg Does the premise of this issue still stand with BackendTrafficPolicy? What if I want all my routes exposed via the Gateway to have a high general rate limit rule, but for a specific HTTPRoute, I apply a more specific BackendTrafficPolicy? Does one take precedent over the other or is it non-deterministic?

I'm using v1.2.5 of EG, and I remember in older version of the doc (v0.6), there's a specific BackendTrafficPolicy design page that spelled out this collision: "xRoute resource will win in a conflict", just want to make sure this is still the behavior.

@arkodg
Copy link
Contributor

arkodg commented Feb 5, 2025

What if I want all my routes exposed via the Gateway to have a high general rate limit rule, but for a specific HTTPRoute, I apply a more specific BackendTrafficPolicy? Does one take precedent over the other or is it non-deterministic?

For this case, you'd need to define 2 BackendTrafficPolicies - one that targets the top level Gateway or all the other routes and a second that has a more specific rule for a specific route

@ryanhristovski
Copy link
Contributor

@arkodg would it make sense to define the top level gateway ratelimiting within the ClientTrafficPolicy rather than a BackendTrafficPolicy (and keep the route level limiting in the btp)?

@arkodg
Copy link
Contributor

arkodg commented Feb 5, 2025

@ryanhristovski I think this intent

In certain cases users may need to specify multiple ratelimit rules that should be applied in certain order. E.g. a global RL rule per ip and subsequent rule per ip per upstream/backend service

can be achieved by

  1. Defining the RL Rule in a BTP and applying it at the Gateway level.
  2. Above RL Rule is special, and needs a new field like shared: true or scope: gateway so its treated as a common bucket when applying across all routes
  3. Using Merge Semantics to merge 2 BTPs - applied to the Gateway and xRoute Add the ability to override only a portion of the spec using Policies #1934

Im not sure if priority is needed here, when merging we can ensure gateway rules are set before route rules

@ryanhristovski
Copy link
Contributor

ryanhristovski commented Feb 6, 2025

@arkodg wdyt the functionality should be if an HTTPRoute level btp had shared: true or scope: gateway (I am leaning shared as the field).

For example:

apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy 
metadata:
  name: per-service-per-ip-rule
spec:
  targetRefs:
  - group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: http-ratelimit
  rateLimit:
    type: Global
    shared: true
    global:
      rules:
      - clientSelectors:
        - headers:
          - name: x-real-ip
        limit:
          requests: 3
          unit: Hour

Should I add some logic that only allows shared on Gateway & shared=false to be default?

@arkodg
Copy link
Contributor

arkodg commented Feb 8, 2025

hey @ryanhristovski @nezdolik can we close this issue and create a new one or update title and description to better define shared ratelimit bucket feature

@arkodg arkodg added this to the v1.4.0-rc.1 milestone Feb 8, 2025
@ryanhristovski
Copy link
Contributor

@arkodg yup, @nezdolik is ooo now so I'll create a new one if you can close this one once i'm done

@ryanhristovski
Copy link
Contributor

@arkodg #5265

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants