Closed acrellin closed 3 years ago
Hello @acrellin! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:
app/access.py
:Line 15:80: E501 line too long (142 > 79 characters) Line 55:80: E501 line too long (85 > 79 characters)
Hey, Ari, thanks for giving this a try.
We typically run many instances of the app, and requests get routed between all of them. As such, it is a bit tricky to do rate limiting in the app itself, since one instance of the app may see all or none of a specific user's requests.
So, I think the right place to do this is at the nginx layer. nginx can look at HTTP headers, and can limit on repeated requests with the same Authorization headers, e.g.
@stefanv see updated comment. We simply need to factor the number of server processes into the limits we set. This can just be a stopgap until we get nginx-based rate limiting in place.
@stefanv but yes, I agree that's still the long-term goal, and this is just meant to be an intermediate stopgap (we've been talking about that for months, with no solution still).
The main concern is that the rate limit will not be fairly applied. It can very well happen that one user gets their requests distributed across 8 workers, and another across 1. That means that one user will have 8x the number of requests available to them.
If we are happy with a ~10x discrepancy between rate limits, then this is probably fine.
An easy middle-ground workaround could be to use a table to store the request.
Did a quick bit of research. Here's the form of the line we need in nginx:
limit_req_zone $http_authorization zone=req_zone:10m rate=5r/s;
It uses the HTTP authorization
header's value (i.e. token) to do the rate limiting.
Will it be straightforward to give the Kowalski bot token unlimited access via nginx, while rate-limiting everyone else?
Yes, you should be able to achieve that with a map. This is untested, but roughly:
map $http_authorization $limit_key {
default $http_authorization;
"auth {{ kowalski_token }}": "";
}
limit_req_zone $limit_key zone=req_zone:10m rate=5r/s;
server {
location / {
limit_req zone=req_zone burst=10 nodelay;
# ...
}
}
Approving as I think this is fine as a stopgap, but also willing to skip this entirely if the nginx implementation is pretty easy to get done. Up to you guys
I think the key pieces to the nginx solution are given above; let's give it a shot?
@stefanv We don't necessarily have the kowalski token upon app start, do we? That might make sense in Fritz, but not here in SP.
@acrellin Right. I think most projects will need to customize their nginx configurations anyway.
If you want default load balancing, you can simply do it as in the first snippet I suggested (implements exactly what this PR does).
For Fritz (the only place where Kowalski is relevant), we will have to change the nginx config to be aware of the Kowalski token (as shown in the second code example).
This PR implements a simple per-token API rate limiting check integrated with the
access
module. Note that each app instance tracks its own number of requests handled only by that instance, so the number of server processes must be taken into account when configuring rate limits.See tests in https://github.com/skyportal/skyportal/pull/2122 Will also add tests to baselayer template app