Remove rate limiting functionality #6513
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PyvesB
added
operations
paulmelnikow
approved these changes
May 19, 2021
chris48s
approved these changes
May 19, 2021
calebcartwright
approved these changes
May 20, 2021
There was a problem hiding this comment.
I suspect part of the motivation for this with OVH was the fact that we publicized the server IPs and malicious traffic could be sent directly to a specific server bypassing the CDN.
That's no longer the case with the runtime in Heroku, so I'll join the party and approve too 😆
PyvesB
added a commit
to platan/metrics-shields-io-config
that referenced
this pull request
May 20, 2021
Related to badges/shields#6513
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our home grown rate limiting feature recently tripped us over in #6511. When taking a closer look at the class, both @chris48s and I challenged its usefulness.
Its main intent is probably to guard against denial of service attacks, but it doesn't feel particularly robust. We still handle the requests on the same machine, so the rate-limiting itself could get easily overwhelmed if you simply sent a little more requests. Looking at our metrics for the past 30 days, outside the past week where I was experimenting with different numbers of servers, it only kicked in very occasionally to limit by ~5 to 15 requests/second. However, I'm not convinced it did any positive impact.
The rate limiting classes have no unit tests, are not easily configurable and still have comments referencing our old OVH servers. The rate limiting numbers had not been updated in many years, despite us servicing much more traffic on a very different underlying platform. I'm not sure what the intent of rate limiting per badge type is, and the hardcoded Camo IPs feels brittle. To me, this really is technical debt in our server code at this point.
I suggest we delete this functionality, slim down our server code and save a few CPU cycles per request. A CDN seems much better suited for these kind of tasks, and Cloudfare does allow one rule with our free plan. Let's discuss the details of that offline.