feat: add query parameter match support in ratelimit #7330
Conversation
slayer321
force-pushed
the
add-query-parameter-local-ratelimit
branch
from
1acdccd to
e635afa
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7330 +/- ##
==========================================
+ Coverage 71.99% 72.02% +0.03%
==========================================
Files 230 230
Lines 33379 33440 +61
==========================================
+ Hits 24032 24086 +54
- Misses 7596 7604 +8
+ Partials 1751 1750 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
slayer321
force-pushed
the
add-query-parameter-local-ratelimit
branch
3 times, most recently
from
522705d to
fc46b82
Compare
slayer321
force-pushed
the
add-query-parameter-local-ratelimit
branch
6 times, most recently
from
97b2144 to
0d91c6c
Compare
slayer321
force-pushed
the
add-query-parameter-local-ratelimit
branch
from
0d91c6c to
ae95d00
Compare
Signed-off-by: sachin maurya <[email protected]>
slayer321
force-pushed
the
add-query-parameter-local-ratelimit
branch
from
ae95d00 to
47ab711
Compare
api/v1alpha1/ratelimit_types.go
Outdated
|
|
||
| // Rate limit on query parameters. | ||
| // +optional | ||
| QueryParameters *QueryParameters `json:"queryParameters,omitempty"` |
There was a problem hiding this comment.
this should look more like []QueryParamMatch like []HeaderMatch fields
also lets use queryParams since that is what upstream uses https://gateway-api.sigs.k8s.io/reference/spec/#httproutematch
Signed-off-by: sachin maurya <[email protected]>
api/v1alpha1/ratelimit_types.go
Outdated
| DescriptorKey string `json:"descriptorKey,omitempty"` | ||
| // | ||
| // +kubebuilder:validation:Required | ||
| DescriptorKey string `json:"descriptorKey"` |
There was a problem hiding this comment.
this is not exposed, but generated internally
Signed-off-by: sachin maurya <[email protected]>
|
Hey @arkodg, I noticed there are quite a few merge conflicts in my PR. I tried resolving them, but it’s getting a bit messy. Would it be okay if I open a new PR with my changes rebased on top of main? |
| // Headers is a list of request headers to match. Multiple header values are ANDed together, | ||
| // meaning, a request MUST match all the specified headers. | ||
| // At least one of headers or sourceCIDR condition must be specified. | ||
| // At least one of headers, sourceCIDR, or queryParams condition must be specified. |
There was a problem hiding this comment.
can we move this to the description for RateLimitSelectCondition and add CEL validation for it?
I recalled I raised simliar question once.
| // Name of the query parameter. | ||
| Name string `json:"name" yaml:"name"` | ||
| // DescriptorKey is the key to use when creating the rate limit descriptor entry. | ||
| DescriptorKey string `json:"descriptorKey" yaml:"descriptorKey"` |
| rlActions = append(rlActions, action) | ||
| } | ||
| } | ||
| for _, queryParam := range rule.QueryParamMatches { |
There was a problem hiding this comment.
shouldnt this look more like // HeaderMatches logic ?
| } | ||
| } | ||
|
|
||
| for _, queryParam := range rule.QueryParamMatches { |
There was a problem hiding this comment.
shoudnt this look like header matches logic ?
| } | ||
| // Case when both header and cidr match are not set and the ratelimit | ||
|
|
||
| // 3) Query Parameters |
There was a problem hiding this comment.
shouldnt this look like header matches logic ?
3 participants
What type of PR is this?
feat: add query parameter match support in ratelimit
What this PR does / why we need it:
It adds the query parameter support for ratelimit
Which issue(s) this PR fixes:
Fixes #6790
Release Notes: Yes/No