RateLimitPolicy v2 (#8) #12
Conversation
…l condition that binds limit definition and HTTPRouteRule
rfcs/0000-rlp-v2.md
Outdated
| @@ -0,0 +1,1380 @@ | |||
| # RFC RateLimitPolicy v2 | |||
There was a problem hiding this comment.
Regarding the the leading 0000 in the name of the file, since this is our most likely RFC to be merged...should this be incremental and starting with 0001 or was meant for different version of the RFC proposal ?
rfcs/0000-rlp-v2.md
Outdated
| - path: | ||
| type: PathPrefix | ||
| value: "/toys" | ||
| method: POST |
There was a problem hiding this comment.
So even tho we are explicitly declaring the method here, the match will not correspond to the 1st HTTPRouteRule POST but instead to both GET & POST... and the most restrictive will shadow the other (if the HTTPRouteRule is not splitted in 2) Same with the case in line 601 I reckon this shadowing behaviour would need to be reflected in the status
There was a problem hiding this comment.
the match will not correspond to the 1st HTTPRouteRule POST but instead to both GET & POST.
Correct.
and the most restrictive will shadow the other
Also correct.
In big bold letters: such "shadowing" will happen in Limitador; and not because the RLP controller will inject one or the other limit. In fact, as you pointed out yourself, in this case both limits will apply to the one HTTPRouteRule (the one that accepts GET or POST on /toys*).
if the HTTPRouteRule is not splitted in 2
Exactly. To avoid the above, the user must split the HTTPRouteRule in 2.
this shadowing behaviour would need to be reflected in the status
In general, yes. However, to be said that reflecting cases like this in the status can be tricky. In this very example, how can you tell which limit shadows which when one is qualified per user and the other is not? Possibly the best we can do for the status in this case is pretty much stating what's in the policy (i.e. without explicitly mentioning anything about "shadowing"):
GET|POST /toys*→ 50rps/auth.identity.username or 100rps overall/assets/*→ 100rps
There was a problem hiding this comment.
My reflections were already shared in the previous PR, but wanted to thank again for the amazing work done in this RFC and make it easy to understand with the examples. Also, being aware of the limitations, still much better than the current state and will ease adoption (hopefully)
Some comments left but noting that would stop the merging.
Co-authored-by: dd di cesare <[email protected]>
Reverts #11
This pull request to undo the changes made in #11 and bring back the original codebase. You can check out the original pull request at #8.
Since there was a long discussion about the changes in the previous pull request, I think it's best if we continue that discussion in the original pull request. Once everything has been sorted out, we can merge this request.
Closes #13.