Add ratelimiter for node api #577
Conversation
evan2645
requested review from
amartinezfayo,
azdagron,
MarcosDY,
martincapello and
walmav
as code owners
This commit adds ratelimiting logic for calls to the node api. It uses client IP as the ratelimiting key, and will drop log lines if/when a client is affected by the limiting. Signed-off-by: Evan Gilman <evan@scytale.io>
pkg/server/endpoints/node/limiter.go
Outdated
| l.notify(callerID, msgType) | ||
| } | ||
| if !res.OK() { | ||
| res.Cancel() |
There was a problem hiding this comment.
nit: do you need to cancel if the burst limit has been exceeded?
There was a problem hiding this comment.
Not sure :) I put it there "just in case"... I've removed it.
pkg/server/endpoints/node/limiter.go
Outdated
| } | ||
|
|
||
| func (l *limiter) limiterFor(msgType int, callerID string) (*rate.Limiter, error) { | ||
| limiters := l.limitersFor(msgType) |
There was a problem hiding this comment.
since limitersFor is only ever called by limiterFor, and holds the lock for the entire function, you may consider calling it while under the lock (and removing the Lock()/Unlock() inside limitersFor.
There was a problem hiding this comment.
I did this out of concern for code safety... instead, opted to leave a code comment on limitersFor, and moved to only taking a lock in limiterFor
| // be processed. It introspects the context in order to identify the caller. An error will be | ||
| // returned if the context is cancelled, an invalid msgType is specified, or if the number | ||
| // of messages exceeds the burst limit. | ||
| func (l *limiter) Limit(ctx context.Context, msgType, count int) error { |
There was a problem hiding this comment.
so if i understand correctly, the limits are imposed per caller, per msg type? 500 ops a second per caller seems a tad high....
There was a problem hiding this comment.
That's correct, though the "limit" is the max burst size - no request with more than 500 CSRs will be allowed to pass. The actual rate is defined when the struct is created, currently capped at 100 per second... In this example, if a request was submitted with 500 CSRs, it would (under the worst conditions) have to wait 5 seconds for it to be permitted.
Since exceeding this limit would be a permanent error condition, I have taught the agent to truncate requests that would exceed it, allowing the remainder to be submitted on the next manager cycle
| @@ -0,0 +1,141 @@ | |||
| package node | |||
There was a problem hiding this comment.
nit: use github.com/stretchr/testify helpers to cut down on assertion boilterplate
* Clean up tests through use of testify * Teach agent to not exceed burst limit * Use coarse outer lock w/ code comment instead of granular independent locks * Dont cancel reservations for limits that are not "OK" Signed-off-by: Evan Gilman <evan@scytale.io>
This commit adds ratelimiting logic for calls to the node api. It uses
client IP as the ratelimiting key, and will drop log lines if/when a
client is affected by the limiting.
Signed-off-by: Evan Gilman evan@scytale.io