Closed tgeoghegan closed 11 months ago
It's also broken. There's a bug or two with determining the oldest event in a rate limiter. I'll send out a follow up PR once I have this figured out.
Aw man :sweat_smile:
Note that this algorithm is (should be?) eventually consistent so you may notice inconsistencies from time to time.
It took a minute to figure out how best to write tests for local and distributed rate limiters, but I have just posted #33 and #34 that fix this bug and will hopefully prevent regressions.
Nice work on that. Looks good!
This is an attempt to provide a useful
retry-after
header value when distributed rate limiting is enabled. In the non-distributed case, this is done by adding the RL window to the time of the oldest event in the ringbuffer. So the approach here is for the distributed RL state to store not just the count of events in each replica's ring buffer, but the time of the oldest event. When a rate limit is exceeded, we find the oldest timestamp across all the replicas, and use that to compute the wait time.The oldest event times are synced around along with the event counts and updated and read in the same
syncDistributed{Read|Write}
functions, so this shouldn't be much more expensive than it already was.This worked in a simple integration test of my application (https://github.com/divviup/janus) which uses this plugin as a rate limiting reverse proxy. If you think this is workable, then I can invest some time into setting up unit tests for it here in the
caddy-ratelimit
repository.