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
fix clusterRatelimit with more than one ratelimit instance #913
Conversation
👍 |
as we discussed the PR in its current form doesn't fully fix the problem |
👎 |
After an internal discussion we agreed on having the implementation changed to #912 (comment) which enables full control of grouping ratelimiters across routes. |
3e41f4f
to
b72a1c1
Compare
@aryszka the code was changed as discussed with internal customers and based on your comment #912 (comment). If the code is good in the review and tested successfully in a dev cluster, I would fix all docs according to the implementation. |
3f61594
to
00df64f
Compare
proxy/proxy.go
Outdated
|
||
if rl.Allow(s) { | ||
return settings, 0 | ||
allow = allow && rl.Allow(s) |
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 think the allow
var is not required. It could be just:
if !rl.Allow(s) {
return setting, rl.RetryAfter(s)
}
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.
fixed
lgtm please update the branch once #932 gets merged |
feature: cluster ratelimit instances can now grouped fix: cluster ratelimit instances do not override shared data anymore, based on the group the ratelimit data is stored in the swarm fix: X-Forwarded-For Header Lookuper in cluster ratelimits is now treated similar to the default refactor: rename file to make it more expressive refactor: rename lb group eskip file test: change test setup to have 2 different ratelimit groups Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
dd5cdd9
to
c2023f5
Compare
👍 |
1 similar comment
👍 |
fix #903 clusterRatelimit with more than one ratelimit instance, which overwrote the shared data from other ratelimit instances.
Signed-off-by: Sandor Szücs sandor.szuecs@zalando.de