AND NOT style matches for RateLimiting

[arkodg]

Description:

Describe the desired behavior, what scenario it enables and how it
would be used.

Consider this config

cat <<EOF | kubectl apply -f -
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: SecurityPolicy
metadata:
  name: jwt-backend
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: backend
  jwt:
    providers:
    - name: example
      remoteJWKS:
        uri: https://test.eu.auth0.com/.well-known/jwks.json
      claimToHeaders:
      - claim: email
        header: x-user-email
---
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy 
metadata:
  name: policy-httproute
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: backend
    namespace: envoy
  rateLimit:
    type: Global
    global:
      rules:
      - clientSelectors:
        - headers:
          - name: x-user-email
            value: [email protected]
        limit:
          requests: 10
          unit: Minute
    - clientSelectors:
      - headers:
        - type: Distinct
          name: x-claim-email
      limit:
        requests: 5
        unit: Minute
---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: backend
spec:
  parentRefs:
    - name: eg
      namespace: envoy
  hostnames:
    - "www.example.com"
  rules:
    - backendRefs:
        - group: ""
          kind: Service
          name: backend
          port: 3000
          weight: 1
      matches:
        - path:
            type: PathPrefix
            value: /
EOF

I want to allow [email protected] user to 10 req/min but all other user 5 req/min.
But with this config [email protected] user is getting 429 after 5 requests.

Relates to https://envoyproxy.slack.com/archives/C03E6NHLESV/p1699784524023299

[optional Relevant Links:]

Any extra documentation required to understand the issue.

[arkodg]

thinking out loud, this use case can be achieved in two ways

1. by adding a dontMatch field

    - clientSelectors:
      - headers:
        - type: Distinct
          name: x-claim-email
        - type: Exact
           name: x-claim-email
           dontMatch: true
           value: [email protected]

2. by adding a except field

    - clientSelectors:
      - headers:
        - type: Distinct
          name: x-claim-email
         except:
           headers:    
           - type: Exact
              name: x-claim-email
              value: [email protected]

this api field would decide the value of expect_match in https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#config-route-v3-ratelimit-action-headervaluematch

[slayer321]

I'm new to this part of the code , will like to learn an implement this feature.
/assign

[arkodg]

ptal @envoyproxy/gateway-maintainers

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

[arkodg]

hey @slayer321 still planning on working on this issue ?

[slayer321]

Hey @arkodg , currently stuck with some other stuff .. will not be able to work on this issue as of now.

[rudrakhp]

@arkodg interested in picking up this issue, thanks!

[arkodg]

great thanks @rudrakhp, suggest breaking up the work into multiple PRs (API, Implementation, Docs/E2E)

[rudrakhp]

@arkodg raised API PR to support this in both header and source matches. Thought it might be easier to do so when dealing with scenarios where subset IP CIDRs might be exempted or have different rate limits. Preferred the dontMatch approach, went with another name though. Please do review, thanks!

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

[arkodg]

keeping this issue open to track completion of docs and e2e

[rudrakhp]

@arkodg E2E tests in PR #4452 is failing with following reason:

2024-10-16T11:30:45.0052006Z          Error:        Received unexpected error:
2024-10-16T11:30:45.0055244Z                        BackendTrafficPolicy.gateway.envoyproxy.io "ratelimit-anded-headers-with-invert" is invalid: spec.rateLimit.global.rules[0].clientSelectors[0].headers[1]: Duplicate value: map[string]interface {}{"name":"x-user-name"}

I noticed that the HeaderMatch list is defined as a +listType=map in CEL validations, with +listMapKey=name. This restricts rate limit filters with multiple header match filters with same name (example below).

  rateLimit:
    type: Global
    global:
      rules:
        - clientSelectors:
            - headers:
                - name: x-user-name
                  type: Distinct
                - name: x-user-name
                  type: Exact
                  value: admin
                  invert: true
          limit:
            requests: 3
            unit: Hour

I think a API fix is needed to change the +listType=set, unless there was a specific reason map was chosen?

PS: I would also remove invert in SourceCIDRMatch since the xDS APIs don't provide a means to do that today. Playing around with CIDR ranges would probably render invert redundant in this case.

[arkodg]

thanks for catching this @rudrakhp

• I think its fine to rm https://github.com/envoyproxy/gateway/blob/958df4899d826c03ecd3aafaea379289bba85f3a/api/v1alpha1/ratelimit_types.go#L104C6-L104C14 . We probably missed this earlier
• For SourceCIDRMatch, lets highlight this limitation in the status

  gateway/internal/gatewayapi/backendtrafficpolicy.go

  Line 745 in 958df48

  if match.SourceCIDR != nil {

[rudrakhp]

Thanks for confirming, also not sure why we need a status check as Invert is an attribute of HeaderMatch and cannot be found in SourceMatch type after I remove it from the API spec (check the API fix PR).

[arkodg]

Thanks for confirming, also not sure why we need a status check as Invert is an attribute of HeaderMatch and cannot be found in SourceMatch type after I remove it from the API spec (check the API fix PR).

Ah in that case, let's skip