fix #5036: addressing the handling of non-connection errors #5047
Conversation
shawkins
force-pushed
the
iss5036
branch
2 times, most recently
from
78b8674
to
d4824ca
Compare
|
Updated the max frame and message values for jetty and vertx using 2 MB - which roughly comes from accommodating the max size of a configmap https://kubernetes.io/docs/concepts/configuration/configmap/#motivation The other option is to just set the values to unlimited - which seems to be what okhttp and jdk effectively do. |
|
@shawkins Thank you for providing the fix! What are the implications of having unlimited frame size at the client side? IMHO it'd make sense to align with other HTTP Clients (OkHttp) and set it to unlimited. |
I don't believe there is much risk, it just provides a more friendly error than an OOM should an unexpectedly large resources be retrieved. Given that OkHttp was used for years without explicit limits and we didn't have any issues that I'm aware of, I'm okay with unlimited - but without additional input from @vietj and @manusa I thought I'd start off with at least what could be a "principled" bound. |
| @@ -39,6 +39,8 @@ | |||
| extends StandardHttpClientBuilder<JettyHttpClient, JettyHttpClientFactory, JettyHttpClientBuilder> { | |||
|
|
|||
| private static final int MAX_CONNECTIONS = Integer.MAX_VALUE; | |||
| // the max data in a config map is 1 MiB, so pad a little beyond that | |||
| private static final int MAX_WS_MESSAGE_SIZE = 1 << 21; | |||
There was a problem hiding this comment.
1 MiB ? or 2 ?
Updated the max frame and message values for jetty and vertx using 2 MB - which roughly comes from accommodating the max size of a configmap https://kubernetes.io/docs/concepts/configuration/configmap/#motivation
There was a problem hiding this comment.
With some further searching using etcd as a keyword it looks like kubernetes sets the limit on the etcd server to 3 MB per request - kubernetes/kubernetes#105863
https://stackoverflow.com/questions/73549236/argo-and-kubernetes-request-entity-to-large-limit-is-3145728
Rather than the etcd default of 1.5 MB. So the 1 MB limit for a configmap is some other limit that is being enforced. After discussion I'll update this pr to be unlimited and note in the resolution the concern about protecting the client memory from attack, which in this case would imply the api server has been compromised in some way.
| logger.debug("WebSocket error received", t); | ||
| manager.scheduleReconnect(state); | ||
| } else { | ||
| manager.close(new WatcherException("Could not process websocket message", t)); |
There was a problem hiding this comment.
The downside to the approach is that we have not fully captured what the recoverable error types are, and we'll need to update the handling further. The alternative here is to log at an error level and still try to reconnect - we'll still potentially need to refine the handling further to avoid looping on something that is non-recoverable.
|
SonarCloud Quality Gate failed. |
Description
Addresses the additional concerns raised in #5036
This still needs to increase or allow setting the relevant limits in vertx and jetty.
cc @vietj
Type of change
test, version modification, documentation, etc.)
Checklist