Change ratelimit API to support distinct match

[zhaohuabing]

This PR is the API change for #1150. Once we agreed on API, the implementation will be submitted in another PR.

[codecov]

Codecov Report

Merging #1308 (397dd0b) into main (690a045) will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1308      +/-   ##
==========================================
+ Coverage   61.84%   61.91%   +0.06%     
==========================================
  Files          76       76              
  Lines       10718    10718              
==========================================
+ Hits         6629     6636       +7     
+ Misses       3644     3637       -7     
  Partials      445      445              

see 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[qicz]

PR title spell error

[Alice-Lilith]

I like this change and I think it's definitely much needed for common ratelimit use-cases. Looks like a few conformance tests are failing but it could just be because of the pre-install flake we were seeing. I'l restart them real quick.

[arkodg]

hey @zhaohuabing wanted to share some more suggestions, hoping @envoyproxy/gateway-maintainers can chime in

1. Name of the new field SourceCIDR vs SourceIPCIDR vs SourceIPSubnet vs SourceIPRange
   I vote for SourceCIDR
2. Name of the match type

sourceCIDR:
  type: Exact
  value: "192.168.1.0/24"

vs

sourceCIDR:
  type: CIDR
  value: "192.168.1.0/24"

I prefer type: Exact because we have the Exact match type for header match, so its reusing the noun (one less thing to remember for the user) and CIDR as a field name + match type feel repetitiive

[zhaohuabing]

sourceCIDR:
type: Exact
value: "192.168.1.0/24"

@arkodg Exact is a bit vague. How about Masked or Subnet？

[arkodg]

@zhaohuabing Masked and Subnet sound the same as CIDR, my argument is the value is already a CIDR type so the match of type Exact would mean matching on the CIDR value. Hoping this discussion gets more eyes so others can also share their preferences.

[zhaohuabing]

I got it. I'm ok with Exact unless we find a better name.

[chauhanshubham]

no strong opinions around this but for the current struct
I feel calling out match explicitly in the type field makes sense.

current
sourceCIDR: {type: "" value: ""}	sourceCIDR: {matchType: "" value: ""}

Just so it reads source CIDR match type and source CIDR value.

source CIDR type felt like there's a IP type aka ipv4/ipv6 distinction in value.
Thanks

[arkodg]

hey @chauhanshubham this is what it would look like

global:
    rules:
    - clientSelectors:
      - headers:
        - name: x-user-id
           type: Exact
           value: one
        sourceCIDR:
          type: Exact
          value: "192.168.1.0/24"

the word clientSelectors is used to describe matches but specifically match attributes related to the client (request headers and source IP in this case).
This has the same look and feel as the matches field within HTTPRoute

- matches:
    - headers:
      - type: Exact
        name: magic
        value: foo

[arkodg]

@zhaohuabing thanks for incorporating my suggestions, so LGTM from my side

[Alice-Lilith]

These changes work for me

[arkodg]

merging, thanks @zhaohuabing !