Skip to content
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

Add validations to prevent duplicate custom rate limits on the same duration & accuracy #304

Closed
GUI opened this issue Jan 6, 2016 · 0 comments

Comments

@GUI
Copy link
Member

GUI commented Jan 6, 2016

In light of the bug in #303, it's possible that users with custom rate limits might have ended up with duplicate custom rate limits on the exact same time frame (in other words, multiple per minute rate limits might be defined). These duplicate rate limits are even more problematic than it first seems, since they impact our rate limit counters and make the rate limiting behave really unintuitively.

In this example:

screen shot 2016-01-05 at 8 50 45 pm

I'm only able to make 1 request per minute, not 5, as you might expect. The issue stems from how we track hit counters for each rate limit. What happens is that since all of these rate limits have the same duration (1 minute) the bucket ID used for counting hits ends up being the exact same (something like apiKey:60000:API_KEY_HERE:0). So when I make a single request with these duplicate rate limits defined, we end up incrementing the hit counter on the same bucket 4 times (since all 4 rate limits use the same bucket ID). So after 1 request, our counters incorrectly think the user has made 4 requests within that time-frame. When I try to make a second request, I end up getting blocked, because the system then thinks I've made 8 requests.

Fixing #302 should largely solve this by eliminating these rogue duplicate rate limits records from unexpectedly popping up. However, an admin could still manually create duplicate entries, so given the fact that this breaks our counters, it seems like we should prevent that. There's not really a use-case for duplicate rate limits on the same time duration, so I think our counter logic is okay, but we should prevent these duplicates from accidentally being entered into the admin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant