-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add explicit configuration for websockets, http1, and a proxy hint. #2731
Conversation
Related to #2725 |
@Value.Check | ||
final void checkWebsocketsNeedsHttp1() { | ||
if (websocketsOrDefault() && !http1OrDefault()) { | ||
throw new IllegalArgumentException("Jetty configuration includes websockets but not HTTP1.1"); |
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 is my understanding that there is no reason that websockets can't work on h2 - for example (as we have discussed elsewhere) if the server is behind multiple proxies, and one doesn't support h2, we would want to encourage websockets.
Example:
graph TD
C[deephaven-server-jetty-app]
B[Proxy #1]
A[Proxy #2]
D[plain h2 grpc client]
E["public internet"]
A-->|http/1.1|B
D-->|h2|C
B-->|h2|C
E-->A
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.
Ah yes - I think this fact was established after I created this PR. I'll remove this improper check.
if (Boolean.TRUE.equals(proxyHint())) { | ||
return false; | ||
} | ||
return ssl().isEmpty(); |
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.
if ssl is enabled without a proxy, have you confirmed that a browser can connect to it without needing to first try http/1.1? It has been a while since I tested that, and recall some hiccup there.
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.
At least with my version of Firefox on Linux, yes. I can't speak for Chrome, older versions, and other operating systems at this point in time. It would surprise me a bit if anything relatively modern couldn't handle it - but might be worth some further testing?
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'd vote do the testing before turning it off, yes - bad form to ship it and then find out we broke it.
Alternatively, just be less aggressive about disabling http/1.1, and merge the rest.
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've set it to be more conservative, and filed a follow-up issue.
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 need a comment here to kick it back to you, either test in more cases to ensure we arent breaking browsers, or drop the "ssl implies no http/1.1")
No description provided.