-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[ADDED] Websocket for Leaf Node connections #1858
Conversation
This reduces code duplication Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Added two options in the remote leaf node configuration - compress, for websocket only at the moment - ws_masking, to force remote leafnode connections to mask websocket frames (default is no masking since it is communication between server to server) Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
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.
LGTM - Lots of changes, leaning on tests for confidence.
I think we should default to compressed and masked.
With the optic of performance and CPU load on the server, I opted to make the opposite defaults (meaning opt-in for compression and masking). Compression is just a negotiation with the remote (this is how websocket works). So if the accepting side does not have compression enabled, compression won't happen (sure, you could say that we should also make |
ok trying to optimize for best compatibility getting through network perimeters. |
Here is the problem I have with boolean options: Unless I named them The problem is with users that embed the server and create the options like we do in tests. In that case the "default" options when configuring, such as So either I make the options "negative", so that the default in all cases match and we get the default behavior that we want (compression and masking), or we take the risk of having users embedding the server have to be aware that they need to set the options? What do you think? |
I think make them negative, no_compression and no_masking, but let's also see what @ripienaar thinks. |
Negative names is unfortunately the best option I think too |
how about |
This will allow a better experience if there is a load balancer in between and expects websocket frames to be masked. Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@derekcollison I have made the masking the default and so change options to make it an opt-in to disable masking. Having masking by default will help if there is say an F5 in between. |
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.
LGTM
The issue was introduced by PR #1858. Key points: - Sec-WebSocket-Extensions must contain approved headers, so moving the "no-masking" private extension to its own header "Nats-No-Masking". - The format of the permessage-deflate negotiation response became invalid, I have fixed that. - For leaf nodes, if `permessage-deflate` extension is not at all present in the response, then simply disable compression, however if it is present but there is no server/client no context take over, then we have to fail the connection. - A leafnode test was not setting the "NoMasking" option so the test TestLeafNodeWSNoMaskingRejected was not capturing possible error if negotiation failed. Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Users will now be able to create a Leafnode connection to the websocket
port of the remote server.
The URL in the remote configuration has to start with "ws" (or "wss").
By default, since this is communication between two servers, the websocket
frames will not be masked. However, there is an option to force the masking.
Here is what a remote leafnode configuration would look like:
/cc @nats-io/core