Add inbound rate limiting #722
Conversation
|
cc @n3wscott |
|
Thx for the feedback @benmoss. Once Scott gets a chance to review I'll incorporate any changes and fix the outstanding TODO items, e.g. docs. |
| type RateLimiter interface { | ||
| // Take attempts to take one token from the rate limiter for the specified | ||
| // request. It returns ok when this operation was successful. In case ok is | ||
| // false, reset will indicate the time in seconds when it is safe to perform |
There was a problem hiding this comment.
reset seems like it should not be unit seconds, but nanoseconds
There was a problem hiding this comment.
oh I see, it is because of https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After is unit seconds.
I still feel like unit seconds is too large of a unit. hmmm
There was a problem hiding this comment.
Yup, rational for secs. Even if a rate limiter internally would use sub-sec precision, IMHO the result should just be (rounded) sec so we don't have to do additional conversion in the handler.
v2/protocol/http/protocol_rate.go
Outdated
| // false, reset will indicate the time in seconds when it is safe to perform | ||
| // another attempt. An error is returned when this operation failed, e.g. due to | ||
| // a backend error. | ||
| Take(ctx context.Context, r *http.Request) (ok bool, reset uint64, err error) |
There was a problem hiding this comment.
| Take(ctx context.Context, r *http.Request) (ok bool, reset uint64, err error) | |
| Allow(ctx context.Context, r *http.Request) (ok bool, after uint64, err error) |
There was a problem hiding this comment.
If we changed the type of r to be a string, we could extract the target host in the http side and then also use this interface at the overall level, perhaps even let us limit at the client send level if we wanted to. The r becomes bucket string and you could configure it to be any thing you want.
There was a problem hiding this comment.
So just to understand (since I had string in a first iteration): which value would we pass then from req into the string of the rate limiter interface in ServeHTTP? If we hardcode to target (source?) host, we lose bucketing options on other req properties, no?
Closes: cloudevents#59 Signed-off-by: Michael Gasch <mgasch@vmware.com>
|
/hold until question on which value to pass to limiter interface is clarified (will squash merge afterwards too) |
Signed-off-by: Michael Gasch <mgasch@vmware.com>
WIP, first proposal up for discussion to implement inbound rate limiting.
Features/Behavior:
ServeHTTP, i.e. cannot be bypassedInterface,i.e. bring your own via HTTPOptionnoOpLimiterto keep current behavior, even though we have been advertising amagicnumber1000when asked for in HTTP optionsSee #59 for more context.
Closes: #59
Signed-off-by: Michael Gasch mgasch@vmware.com