-
Notifications
You must be signed in to change notification settings - Fork 887
fix(evmrpc): don't charge an innocent client's per-IP bucket on mid-read budget exhaustion #3935
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
9f0ee73
1c57165
03407ac
37e1494
6f71feb
2ad70de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,13 @@ func (m *rateLimitMiddleware) ServeHTTP(w http.ResponseWriter, r *http.Request) | |
| m.rejectAdmission(r.Context(), w, ip, rejectReasonOversize, http.StatusRequestEntityTooLarge, "request body too large") | ||
| return | ||
| } | ||
| if errors.Is(err, errBudgetExhausted) || isReadIdleTimeout(err) { | ||
| // The outer requestSizeLimiter already recorded the rejection reason | ||
| // (budget_midread/slow_body) and owns the response for this failure; | ||
| // charging admission here would debit an innocent client's per-IP | ||
| // bucket for a server-side capacity event and double-count the metric. | ||
| return | ||
| } | ||
|
cursor[bot] marked this conversation as resolved.
|
||
| m.rejectAdmission(r.Context(), w, ip, rejectReasonReadError, http.StatusBadRequest, "bad request") | ||
| return | ||
| } | ||
|
Comment on lines
36
to
51
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Outside the scope of your PR, but a bit strange that we seem to be swallowing errors and not logging them.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for feedback. Regarding logging, I think that opens the gate for more DOS problems due to writing logs to disk. It seems adding metrics around them could be a more suitable approach. Perhaps we can revisit this once it is rolled out and we see how much rate limiting traffic we get |
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.