Skip to content
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

Merged
merged 3 commits into from Jan 29, 2021
Merged

[ADDED] Websocket for Leaf Node connections #1858

merged 3 commits into from Jan 29, 2021

Conversation

kozlovic
Copy link
Member

@kozlovic kozlovic commented Jan 28, 2021

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:

leaf {
  remotes [
     # Ask for compression (the websocket{} block in the remote server has
     # to have compression enabled for them to negotiate), and ask for
     # the server to not mask outbound websocket frames.
     {url: "ws://host1:6222", ws_compression: true, ws_no_masking: true}

     # Same than above but with masking of websocket frames (the default)
     {url: "ws://host2:6222", ws_compression: true}

     # Without specific options, this will mask frames and not ask for compression.
     {url: "ws://host3:6222"}
  ]
}

/cc @nats-io/core

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>
Copy link
Member

@derekcollison derekcollison left a 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.

@kozlovic
Copy link
Member Author

@derekcollison

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 websocket{compression: true} the default :-))

@derekcollison
Copy link
Member

ok trying to optimize for best compatibility getting through network perimeters.

@kozlovic
Copy link
Member Author

kozlovic commented Jan 29, 2021

Here is the problem I have with boolean options:

Unless I named them no_compression or ws_no_masking (as I did in my first approach), then it is really messy. When parsing from the configuration file, we could always create RemoteLeafOpts{Compression: true, WSMasking: true} and if the options are explicitly in the config file, they will overwrite.

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 o.LeafNode.Remotes = []*RemoteLeafOpts{{URL: u}} would mean that compression is off and masking is off, which would be opposite of parsing a config file.

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?

@derekcollison
Copy link
Member

I think make them negative, no_compression and no_masking, but let's also see what @ripienaar thinks.

@ripienaar
Copy link
Contributor

Negative names is unfortunately the best option I think too

@aricart
Copy link
Member

aricart commented Jan 29, 2021

how about disable_xxx.

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>
@kozlovic
Copy link
Member Author

@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.

@derekcollison derekcollison self-requested a review January 29, 2021 18:29
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kozlovic kozlovic merged commit 06c63b5 into master Jan 29, 2021
@kozlovic kozlovic deleted the ln_ws branch January 29, 2021 18:35
kozlovic added a commit that referenced this pull request Feb 1, 2021
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants