-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Exclude admin / diagnostic requests from HTTP request limiting #18833
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
Exclude admin / diagnostic requests from HTTP request limiting #18833
Conversation
With this commit we exclude certain HTTP requests that are needed to inspect the cluster from HTTP request limiting to ensure these commands are processed even in critical memory conditions. Relates elastic#17951, relates elastic#18145
|
I wonder if we should go from a whitelist to a blacklist, it seems like we are adding more and more exceptions - maybe we should just put the memory hungry ones in there explicitly? |
|
Previous exceptions were defined on transport level (OTOH, we have currently 3 exceptions defined on transport level). This PR adds support for HTTP level. I think the better option is to be conservative with exceptions and just use a whitelist but it would be pretty easy to use a blacklist instead (just swap the default and blacklist the relevant actions). |
| } | ||
| } | ||
|
|
||
| private static final class RestHandlerHolder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make more sense to add a method to the RestHandler interface default boolean canTripCircuitBreaker() { return true; } that would safe us all the extra indirection and is simple to override?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would also be consistent across all the methods the handler is registered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also considered this but did not want to mix various aspects in the RestHandler interface. But I like the approach and can change that.
ok sure.... I left some comments |
|
@s1monw I've pushed a commit that addresses your comments. Could you please have another look? |
|
LGTM |
|
Thanks; I'll merge soon. |
With this commit we exclude certain HTTP requests that are needed to inspect the cluster
from HTTP request limiting to ensure these commands are processed even in critical
memory conditions.
Relates #17951, relates #18145