Open creack opened 2 years ago
Hi @creack,
I'm just looking into this PR and it looks good. Thank you!
~However, I think we should combine the changes with ideas from https://github.com/go-chi/httprate/issues/2, if we're to expand the LimitCounter interface.~
As I needed a networked-based limiter with transactions, I had to add the context to httprate.
Can you please describe your use-case a bit more? Do you use redis as the LimitCounter
or something else?
(~And would you be able to GetAndIncr
in a single atomic operation?~)
@creack I'm thinking about the pros/cons of passing the context timeout down to the networked-based limit counter(s).
Few things to consider:
.Get()
and .IncrementBy()
implementation without considering request.Context()
timeout?
HTTP 429
limit? Or should we "cancel" the request to network-based limiter and let the attacker continue?I came across this with a related need for context propagation that I can describe in further detail if needed: tracing.
If you're using httprate-redis and wrap your Redis client with tracing, the Redis spans aren't attached to the parent. Tracers in Go rely on threading the active span through with context.WithValue
. I happen to be using Datadog's dd-trace-go library and hook for go-redis but this is functionally equivalent:
https://github.com/redis/go-redis/tree/master/extra/redisotel
A context version of IncrementBy
should be included too—httprate-redis uses that.
Some thoughts on your DDOS question @VojtechVitek:
Or should we "cancel" the request to network-based limiter and let the attacker continue?
As a last resort, it's up to the limiter implementation to respect the context, and it can opt to ignore cancelation. In theory you could a context to pass tracing context but ignore the cancelation signal.
Or, there's the option to add an option to httprate
to wrap contexts in WithoutCancel
before passing them:
https://pkg.go.dev/context#WithoutCancel
If that were the default behavior it would be truly non-breaking. Tracing would work by default but cancelation could require opt-in. context.WithoutCancel
requires Go 1.21 but the type withoutCancelCtx
it returns can be easily inlined for earlier versions.
As I needed a networked-based limiter with transactions, I had to add the context to httprate.
Initially made the change locally, copy/pasting most of the repo, but might as well push upstream.
It is fully backward compatible.