-
Notifications
You must be signed in to change notification settings - Fork 16
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
Issue with having multiple zones for a single key #10
Comments
What do you mean by "failed requests"? Can you be more specific? |
Sorry, I should have said rate-limited requests. e.g., with the above configuration, I test it by sending 10 req/sec. Now, I get a success (200 OK) for 2 reqs. satisfying the 1 req/sec but I expect to see success for 10 requests which never happens. I assume this is because by that time more than 10 reqs have been hit. The retry-after headers also say 59s after about 10-11 requests. Is it possible for it to keep a count of only 200 OK responses in the events ? Caddy access log: (python) script that sends the requests: |
Well letsee, with the same key for those zones, both rate limits will apply to the same request. I'm not sure what you're trying to accomplish here exactly. And I'm still unclear what you mean by "failed requests" -- can you please be specific? Do you mean any request that doesn't have a certain response code? By that time the request has already finished, so it can't be rate limited, right? If you could break down exactly what you're trying to accomplish, that would be productive. I'll pull out a bug report template if that is helpful. |
I'm sorry I thought I explained it pretty well with examples. I want to restrict requests per user to 1 req/s and 10 req/min. Caddyfile:
I then test it by sending 10 requests / sec. It takes less than a minute to run so I expect to see 10 The plugin starts limiting right after ~1s even though the condition of 10 req/min was never satisfied. (screenshots in my previous post) With the other rate_limit plugin, the Caddyfile:
Now, I see that it satisfies both conditions i.e., in total 10 requests get the I can give you the whole log if you like but it's more of the same. |
Ahhh, I see. The requests blocked by the more restrictive rate limiter also count towards the more permissive one. So it sounds like you want the more permissive zone to yield to the more restrictive one, rather than enforcing both simultaneously. I did some testing on my end. What happens for you if you rename |
YES ! Okay, changing the name is a lot better ( can't believe I didn't think of that myself :) ) but it still doesn't fulfil the conditions. I tested both plugins with 5 runs of 100 requests each with 0.01 s of sleep time in between each request. In between each run, I fully stopped, removed and recreated the docker containers. Of the 100 requests, this plugin always allows 6-7 requests on each run and rate-limits the rest. the other plugin consistently allows 10 requests on each run and rate-limits the rest. |
To be fair, this is unintuitive and likely a bug because we are, once again, relying on Go's ordering of object keys in JSON to ensure that the more restrictive zone comes first. Can you show me a working config for the other rate limit plugin? I'll study and try to see what they do. But it sounds like we need to order the zones by ascending permissiveness. Maybe that is computed simply by doing Does that sound about right?
Just to be sure, you're not doing any distributed RL? Distributed RL will be eventually consistent. But internal-only, hmm. I haven't experienced this behavior yet... |
For the other plugin, the Caddyfile is just this.
I don't think the order of the rules matter.
Yeah I would think so as well. And no, I am not using any distributed limiting. |
@accountForIssues So did you confirm, if you reorder those zones in your other-plugin config, does it still behave properly? Anyway, I think I know how to fix the ordering issue. I'm still not sure why only 6-7 requests get allowed, though. (It works for me?) Let me know if you have any ideas on that one. |
@accountForIssues Ok, I've pushed a fix for the sorting problem in 163fde7. I haven't been able to reproduce the other problem you reported:
Can you try with the latest commit anyway and maybe provide some more details to reproduce the bug? Also does it still happen with lower frequency / slower firing off requests? (i.e. maybe like 2 per second over a longer window?) |
I stand corrected. The order there does matter. It just so happened that it did the expected behaviour so I didn't test it the other way.
Thank you ! I built a new image for this update using the latest caddy builders and it seems to work exactly how I want. The issue with 6-7 requests is no more. It always allows 10 requests (or the more permissible limit). I will be testing it more and will update if I encounter something unexpected. |
Yay! Thanks for verifying. I will close but feel free to comment if needed! |
So, I'm trying to use this plugin to limit requests to a single user in the following way: 1 req/sec and 10 req/min.
The issue I'm having is that the plugin seems to be counting failed requests as well so I hit the rate-limit after 10 requests whether they were successful or not.
The other plugin works without this issue but it doesn't support the
retry-after
header.Is this a bug or is there a better way of achieving what I want ?
The text was updated successfully, but these errors were encountered: