-
Notifications
You must be signed in to change notification settings - Fork 11
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
RateLimitPolicy v2 #8
Conversation
d04d63a
to
0cc37bd
Compare
rfcs/0000-rlp-cleanup.md
Outdated
- name: writers | ||
selectors: | ||
# this is a condition | ||
- selector: request.http.method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is the difference here that one has qualifiers and one doesn't. It is is an interesting idea, it did take me a minute to understand that by reading it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maleck13, I imagine you refer to distinction between what is a "condition" and what is a "counter qualifier". While the former includes an operator and value to be matched, the latter is just the selector.
This was one of the suggestions that came up during one of our tech discussion sections. I made sure to include other possible (including more explicit) variations here in the doc: https://github.com/Kuadrant/architecture/blob/rfc/rlp-cleanup/rfcs/0000-rlp-cleanup.md#possible-variations-for-the-selectors-conditions-and-counter-qualifiers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it for the conditions, just not 100% sold on the counters. When I read selector here it makes sense that they are defining when a limit should be applied but using it for defining the counter feels a bit odd, not that I couldn't be convinced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guicassolato I might have missed but are these selectors an and
operation when there are multiple?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are, @maleck13
rfcs/0000-rlp-cleanup.md
Outdated
- limit: 100 | ||
duration: 12 | ||
unit: hour | ||
weight: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unfamiliar with the weight concept when talking about rate limting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maleck13, it could also be called "increment by".
This is a concept in parts inherited from 3scale and, at the same time, a placeholder to kick off the discussion around a more sophisticated rate limit system where the increments are not necessarily linear to the number of hits. I'm thinking ways to increment the counter based upon variable factors such as the size of the payload, the estimated computation effort to process the request, etc – to name a few.
At the very least, in its simplest form, the option to modify the weight (or increment) of a limit can be a way for users to play with the rates without resetting the counter. It opens up implementation options as well for what's sometimes called "dynamic rate limit".
In time: I now wonder if "increment" isn't indeed a better name for the thing, so to avoid the confusion with something else, like weighting between limits (e.g. to infer priority).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah increment seems more explicit. I def think this would be an optional field with a default of 1 though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question for me is also do we have a solid usecase at this point? It is easy to add properties but difficult to remove (breaking compatibility). So perhaps it is something we internalise within kuadrant and then potentially express externally when we see the use cases arise for adjusting the counter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about delta optional field which defaults to 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure whether we should add spec.limits.increment
at this stage, but won't fight it neither… otoh, I wonder if a disabled
or something flag could be useful? i.e. not using a magic value as a limit (e.g. 0
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed from the RFC.
As agreed, the increment feature is out of scope. It deserves its own proposal to be discussed separately.
rfcs/0000-rlp-cleanup.md
Outdated
- selector: request.http.method | ||
operator: eq | ||
value: GET | ||
- selector: request.http.path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the simplicity here. When this is set, it would be good to validate that this path selector matches a rule in the HTTPRoute WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's possible for everything, but in general yes. For things like headers or checking if a path prefix is contained within another, etc, such validation can be challenging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think validating the path and method would be a good place to start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I really like these improvement to the API. I don't want to comment to much on what the implementation might look like as I am not close enough to that component.
Some things worth a little more debate:
- do we want to stick with selectors for the counter qualifier. I feel this could potentially cause some confustion
- do we want to start with the weight property or drop that from this version for now until we have more concrete usecases
- how does this design relate to concept of overrides and defaults
Generally a +1 from me though
@alexsnaps @guicassolato one concern I have is that (assuming we want to stick with policy attachment) adding that to the API, even if we went only with |
0cc37bd
to
b691fa4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I like the abstraction of conditions and variables into selectors with/without values to be matched. 👍
- From the design it is assumed (maybe I am wrong) that the rate limiting service (Limitador) will always be called. I miss some way to specify when I do not want to call Limitador. It is usually based on HTTP method, HTTP path and domain name. 👎
.spec.limits[].name
may not have direct impact on the rate limiting config, but I think it can be useful for reporting state or stats. 👍- Envoy RLS proto allows
hits_addend
field to specify the "weight" (I would call ithits_addend
) https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/ratelimit/v3/rls.proto.html. That can be implemented in the wasm-shim 👍
How would be the spec for the following use case? I want rate limiting of X request per minute (or whatever duration) if the request has the header "X-TO-BE-RL", no matter what is the value of the header?. Or the opposite, no rate limit if the request has the header "X-NOT-TO-BE-RL", no matter what is the value of the header.
rfcs/0000-rlp-cleanup.md
Outdated
## Motivation | ||
[motivation]: #motivation | ||
|
||
The [`RateLimitPolicy`](https://pkg.go.dev/github.com/kuadrant/[email protected]/api/v1beta1#RateLimitPolicy) API (v1beta1), particularly its [`RateLimit`](https://pkg.go.dev/github.com/kuadrant/[email protected]/api/v1beta1#RateLimit) type used in `ratelimitpolicy.spec.rateLimits`, designed in part to fit the underlying implementation based on the Envoy [Rate limit](https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/rate_limit_filter) filter, has been proven to be (i) _unnecessarily complex_, as well as (ii) possibly _limiting for the extension of the API_ either beyond this specific implementation and/or for supporting use cases of rate limiting not contemplated in the original design, such as comprising the concept of _weights_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possibly limiting for the extension of the API either beyond this specific implementation and/or for supporting use cases of rate limiting not contemplated in the original design, such as comprising the concept of weights.
Not sure I understand this, specifically the example of weights. The motivation of the initial design is to not abstract the Envoy rate limiting API which is as far as Kuadrant can go on rate limiting. So, any abstraction on top of the Envoy's rate limit api will add limitations. Those limitations should be the toll for a better UX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That example was before I learned about Envoy's hits_addend
.
Nevertheless, my point here was that there may be things we want to support that are between the Kuadrant control plane (i.e. what's stated in the RLP) and Limitador (i.e. the RL service's capabilities and how it is configured), and that do not necessarily have a 1:1 matching to any specific feature or field provided by Envoy. Obviously, within the constraints imposed by the protocol, we need to find a way to support such things, but the way they are presented to the user do not have to be shaped by the protocol. IWO, abstracting the implementation details and create our own language for how to state RL rules and intended behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving this ref here about the Envoy RateLimitRequest field hits_addend
: envoyproxy/envoy#12969
rfcs/0000-rlp-cleanup.md
Outdated
- limit: 100 | ||
duration: 12 | ||
unit: hour | ||
weight: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about delta optional field which defaults to 1?
That would be the union of all the selectors. For every selector in the policy, we add a "configuration rule" (how it was previously called). IOW, if you have a selector based on a value (e.g. the HTTP method) and that value is present in the proxy before it triggers the RL filter (always the case for the HTTP method), then the proxy will set a descriptor with the value. This is a way to avoid setting rules to trigger the RL filter based on values that are never actually used in the policy, at the same time it validates that what's used in the policy makes sense in the context. |
I did not know about that. Thanks for pointing out! |
e475e23
to
659bb62
Compare
…oposal to be discussed separately
This has been discussed in the Kuadrant tech call yesterday, but I'll leave here a comment for the record. It's about targeting individual I think only in a world of 1:1 relation between matching rules ( Maybe it's a matter of educating people, or establishing a new "best practice"? By declaring highly granular Coupling is also a very important aspect at play here. People should not have to write HTTPRoutes based (only or ever) on how they will then define their RLPs. Otherwise one thing is coupled to the other. Plus, add KAP (and other types of policies) here and an organisation of an HTTPRoute once thought for RL is now broken. Besides, the RLP API must allow one to write limits that target multiple Add there two requirements together, i.e. how people write HTTPRoutes (translated to the necessity of occasionally targeting partial rules) and possibility to target more than one rule/backend, and targeting sets of |
IMO a RLP should target a If the RLP targeted a match, the scenario would be much more complicated, kind of "multicolour" routes using the color analogy. |
@eguzki, unless users write 1 |
I do not see this. I see rate limiting assigned (colouring) a given route. Just like Envoy's API for rate limiting. The rate limit config is at the route level, not at the matcher level. |
Coupling HTTPRoutes to RLPs or 1 HTTPRouteRule == 1 HTTPRouteMatch pattern is a logical conclusion. I guess what you're saying is that Envoy's RL API does not allow us to implement the proposal due to not offering RL at the level of the match, but one level higher. This is a good point. In Example 2, those two independent gateway actions are only possible because of the wasm shim, but using Envoy's RL API, it wouldn't exist one rule for I can agree with that. Then the only way out (without going "all-in" with our wasm shim to solve this) is by forcing users to move toward 1 HTTPRouteRule == 1 HTTPRouteMatch. We change this proposal to atomic targeting of HTTPRouteRules, users will be forced to occasionally redefine their HTTPRouteRules within their HTTPRoutes to be able to attach limits to them, then again for other types of policies, until they realise it's better breaking HTTPRouteRules to no more than one HTTPRouteMatch each so policies can be more fine-grained. |
I am struggling to understand this. Just to clarify: By no means I want to force GwAPI users to 1 HTTPRouteRule == 1 HTTPRouteMatch. What I am proposing is that Maybe |
Let's run a didactic example. By induction, we start with the simplest HTTPRoute with no kind: HTTPRoute
spec:
rules:
- backendRefs: [toystore] # / Now we want a RLP of 100rps on method kind: HTTPRoute
spec:
rules:
- backendRefs: [toystore] # /
- backendRefs: [toystore] # POST /
matches:
- method: POST Then we can write the RLP: kind: RateLimitPolicy
spec:
limits:
- rules:
- matches: # or "first HTTPRouteRule whose entire set of matches is identical to this"
- method: POST
rates:
- limit: 100
duration: second Now we want 500rps for method kind: HTTPRoute
spec:
rules:
- backendRefs: [toystore] # /
- backendRefs: [toystore] # POST /
matches:
- method: POST
- backendRefs: [toystore] # GET /
matches:
- method: GET And edited RLP: kind: RateLimitPolicy
spec:
limits:
- rules:
- matches:
- method: POST
rates:
- limit: 100
duration: second
- rules:
- matches:
- method: GET
rates:
- limit: 500
duration: second Let's keep going... 50rps for path HTTPRoute: kind: HTTPRoute
spec:
rules:
- backendRefs: [toystore] # /
- backendRefs: [toystore] # POST /
matches:
- method: POST
- backendRefs: [toystore] # GET /
matches:
- method: GET
- backendRefs: [toystore] # GET|POST|PUT /expensive
matches:
- path:
type: PathPrefix
value: /expensive
method: GET
- path:
type: PathPrefix
value: /expensive
method: POST
- path:
type: PathPrefix
value: /expensive
method: PUT RLP: kind: RateLimitPolicy
spec:
limits:
- rules:
- matches:
- method: POST
rates:
- limit: 100
duration: second
- rules:
- matches:
- method: GET
rates:
- limit: 500
duration: second
- rules:
- matches:
- path:
type: PathPrefix
value: /expensive
method: GET
- path:
type: PathPrefix
value: /expensive
method: POST
- path:
type: PathPrefix
value: /expensive
method: PUT
rates:
- limit: 50
duration: second Hopefully it's getting clear at this point how the HTTPRoute is being shaped according to the needs of the RLP. Now we want auth (KAP) for We cannot add We cannot target the Solution: we break the HTTPRoute: kind: HTTPRoute
spec:
rules:
- backendRefs: [toystore] # /
- backendRefs: [toystore] # POST /
matches:
- method: POST
- backendRefs: [toystore] # GET /
matches:
- method: GET
- backendRefs: [toystore] # GET /expensive
matches:
- path:
type: PathPrefix
value: /expensive
method: GET
- backendRefs: [toystore] # POST|PUT /expensive
matches:
- path:
type: PathPrefix
value: /expensive
method: POST
- path:
type: PathPrefix
value: /expensive
method: PUT
- backendRefs: [toystore] # DELETE /expensive
matches:
- path:
type: PathPrefix
value: /expensive
method: DELETE RLP: kind: RateLimitPolicy
spec:
limits:
- rules:
- matches:
- method: POST
rates:
- limit: 100
duration: second
- rules:
- matches:
- method: GET
rates:
- limit: 500
duration: second
- rules:
- matches:
- path:
type: PathPrefix
value: /expensive
method: GET
- matches:
- path:
type: PathPrefix
value: /expensive
method: POST
- path:
type: PathPrefix
value: /expensive
method: PUT
rates:
- limit: 50
duration: second KAP: kind: AuthPolicy
spec:
auth:
- rules:
- matches:
- path:
type: PathPrefix
value: /expensive
method: POST
- path:
type: PathPrefix
value: /expensive
method: PUT
- matches:
- path:
type: PathPrefix
value: /expensive
method: DELETE
rates:
- limit: 50
duration: second What if there's another policy for |
I think I see what you mean. Thanks for the more elaborated example @guicassolato
I would like to avoid that by design. Routing is one thing and another is rate limiting. HAving a single route and multiple limit objects can be accomplished with specific In the example apiVersion: kuadrant.io/v2beta1
kind: RateLimitPolicy
metadata:
name: toystore-per-endpoint-per-user
spec:
targetRef:
group: gateway.networking.k8s.io
kind: HTTPRoute
name: toystore
limits:
- name: readers
matches:
- path:
type: PathPrefix
value: "/toys"
method: GET
- path:
type: PathPrefix
value: "/toys"
method: POST
rates:
- limit: 50
duration: 1
unit: minute
counters:
- auth.identity.username
when:
- selector: context.request.http.method
operator: eq
value: GET
- selector: context.request.http.path
operator: eq
value: /toys
- selector: auth.identity.group
operator: neq
value: admin
- name: writers
matches:
- path:
type: PathPrefix
value: "/toys"
method: GET
- path:
type: PathPrefix
value: "/toys"
method: POST
rates:
- limit: 5
duration: 1
unit: minute
- limit: 100
duration: 12
unit: hour
counters:
- auth.identity.username
when:
- selector: context.request.http.method
operator: eq
value: POST
- selector: context.request.http.path
operator: eq
value: /toys
- selector: auth.identity.group
operator: neq
value: admin Is that good enough? not too much DRY? |
We are yet again reopening the same can of worms, but... tl;dr:
So yes, this is verbose. Requires tweaking
So with the 3rd bullet point, the routing becomes:
kind: HTTPRoute
spec:
rules:
- backendRefs: [toystore]
matches:
- method: POST
- method: GET
- backendRefs: [toystore]
matches:
- path:
type: PathPrefix
value: /expensive
method: GET
- path:
type: PathPrefix
value: /expensive
method: POST
- path:
type: PathPrefix
value: /expensive
method: PUT
- path:
type: PathPrefix
value: /expensive
method: DELETE You can now model the different policies using these individual matchers and have different RLP for each of the 4 methods on |
I think the reasons why we attach RL (or auth) to routes and route rules are:
Now you're driving us in the opposite direction than 1 HTTPRouteRule == 1 HTTPRouteMatch. Taken to the limit, we could have just the simplest HTTPRoute with no matches at all and solve everything with In fact, that's exactly what would happen – again exemplifying by induction – if you add to your example above a KAP for To sum up, I think neither you're proposing only using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Gui for the extensive work dedicated in this RFC, specially on examples and the digested comparison with the current RLP... and also the effort to match the KAP as well.
As I had the opportunity to reflect on the proposal both in our meetings and this RFC, some important points to highlight IMHO are the following:
Pros:
- Super user intuitive, a great enhance on API UX
- Having a 1-1 match between
spec.limits.matches
andhttproute.spec.rules.matches
ease the overhead when computing the status, will help to define a better Kuadrant Policies Status conditions/states #9 - Abstracting the most out of Envoy specifics is a great step for any future different gateway implementation.
- The granularity of the limits in time enables a finer rates definition.
Cons:
- Verbosity defining limits per HTTPRouteMatch
- Raised the need of keeping in sync the RLP with the Route matches...
- ... and reflecting this in a clear way in their status.
Even tho we have some negatives implementing the proposal, the benefits are clear and will provide an uniform and comprehensive way of crafting Kuadrant policies. It's also worth mentioning that it will enable us to move forward and have a better understanding once it's started to be in use... real feedback from users will be valuable and we always can redefine it.
Great work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎸
4e50bbc
to
923a701
Compare
923a701
to
874bd7d
Compare
…the HTTPRouteRules to be bound to a limit + Limit's name becomes a first-class citizen
counters: | ||
- auth.identity.username | ||
- auth.identity.username | ||
triggers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not quite sure why we moved away from rules
here... is it because of the hostname
now being part of this?
rfcs/0000-rlp-v2.md
Outdated
- matches: # matches the 1st HTTPRouteRule (i.e. GET or POST to /toys*) | ||
- path: | ||
type: PathPrefix | ||
value: "/toys" | ||
method: GET | ||
- path: | ||
type: PathPrefix | ||
value: "/toys" | ||
method: POST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would:
- matches:
- path:
type: PathPrefix
value: "/toys"
also matches the 1st HTTPRouteRule (i.e. GET or POST to /toys*)
? It would right, as this match is a subset of the two matchers in the route... am I understanding this right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking on atomic HTTPRouteMatches at first, so no. But maybe yes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, if we say "yes", then
- matches:
- path:
type: PathPrefix
value: "/toys"
would match any method, header and query string currently (or futurely) specified as well in the HTTPRouteMatches – not only GET
or POST
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok, I misread that then... You mean any complete match will target the rule, so that if I have match A & match B to Rule 1, I don't need to target A & B, but A or B suffices... but either needs to be complete, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean any subset of the HTTPRouteMatches, not any subset of a HTTPRouteMatch.
In the example,
- matches:
- path:
type: PathPrefix
value: "/toys"
method: GET
- path:
type: PathPrefix
value: "/toys"
method: POST
are 2 HTTPRouteMatches.
The set of all subsets contains 22 = 2 × 2 = 4 elements:
[{}]
[{ path: { type: PathPrefix, value: /toys }, method: GET }]
[{ path: { type: PathPrefix, value: /toys }, method: POST }]
[{ path: { type: PathPrefix, value: /toys }, method: GET }, { path: { type: PathPrefix, value: /toys }, method: POST }]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I have match A & match B to Rule 1, I don't need to target A & B, but A or B suffices... but either needs to be complete, right?
Correct.
If you have:
- matches:
- path:
type: PathPrefix
value: "/toys"
method: GET
- path:
type: PathPrefix
value: "/toys"
method: POST
Then just
- matches:
- path:
type: PathPrefix
value: "/toys"
method: GET
or just
- matches:
- path:
type: PathPrefix
value: "/toys"
method: POST
would suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I re-read this and now need to step back and change what I said before.
To be possible for policy owners to define triggers in the policy that are strictly about what's intended to be protected, we have to allow for a subset of an HTTPRouteMatch. The contrary (i.e. forcing only atomic HTTPRouteMatches in the triggers) would cause the user sometimes having to add to the triggers more than intended, which is exactly what we wanted to avoid when we decided to move away from defining triggers in terms of full HTTPRouteRules.
With that, based on the example from before, one who writes the following trigger matches:
triggers:
- matches:
- path:
type: PathPrefix
value: "/toys"
would have written a valid trigger and binding the limit definition to the HTTPRouteRule:
rules:
- matches:
- path:
type: PathPrefix
value: "/toys"
method: GET
- path:
type: PathPrefix
value: "/toys"
method: POST
What's put in the triggers nonetheless still have to be explicitly stated in the HTTPRouteRules. E.g. the following trigger:
triggers:
- matches:
- path:
type: Exact
value: "/toys/special"
would be an invalid trigger even if it's technically a subset of the HTTPRouteRule above.
This is the opposite of what I had said. I will make sure the text is clear about that.
Thanks @alexsnaps!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Example 2 modified to make it somewhat clearer that a subset of an HTTPRouteMatch (as long as explicitly stated in the HTTPRoute) is also acceptable.
…ns for one that does not involve HTTP attributes used in 'soft' conditions
…ely stated within the trigger matches, but only a portion of it will do (if that portion is enough to scope the limit)
a775286
to
33dec29
Compare
This reverts commit bafafae.
Revert "RateLimitPolicy v2 (#8)"
No description provided.