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

Exposing a WebSocket Handler for embeded NATS servers #5392

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BlizzTom
Copy link
Contributor

@BlizzTom BlizzTom commented May 6, 2024

When embedding a NATS server with a UI, in order to take advantage of the Web Socket connection on the same server, it was required that you made a reverse proxy to the web socket port.

This exposes a small handler without MQTT/LEAF node support for embedding scenarios.

Signed-off-by: Tom Anderson [email protected]

There are more edge cases that this doesn't support, ideally the tradeoffs are valid if you understand them and need them
@BlizzTom BlizzTom requested a review from a team as a code owner May 6, 2024 22:19
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

@BlizzTom How would you use it? It looks to me that there is already a PR that would maybe cover this but on a more complete? Can you have a look at #4122 and see if that one would work? If so, maybe we need to revive it and go from there?

@jordan-rash
Copy link

@BlizzTom How would you use it? It looks to me that there is already a PR that would maybe cover this but on a more complete? Can you have a look at #4122 and see if that one would work? If so, maybe we need to revive it and go from there?

Happy to get #4122 rebased here in the next few hours so @BlizzTom can see if it satisfies his requirements

@jordan-rash
Copy link

#4122 has been rebased. Looked forward to any feedback!

@BlizzTom
Copy link
Contributor Author

BlizzTom commented May 8, 2024

The only issues I see of that other PR is that the http server is still controlled by the NATS server instance, rather than my own, and I am forced to use the http.ServeMux instead of any other http.Handler.

I could be talked into being ok with that, it's more of a limitation of that PR, and I didn't even realize it existed.

A use case that I have is that my application has pretty strict security access through a single port (443), and NATS sits behind that network boundary. The application uses NATS embedded to cluster some logic between nodes. The website uses websockets to communicate with the application API. Currently, I have to reverse proxy the websocket port back into localhost:<port> on the same process to expose that websocket.

While the work around hasn't been terrible, it's an extra step of complexity and a network hop. With the web server mux being quite complicated and not using http.ServeMux as the mux, the other PR would not suit the needs.

@BlizzTom
Copy link
Contributor Author

BlizzTom commented May 8, 2024

I should probably clarify the statements above about the "the web server mux being quite complicated".

There are a few things we set on the http.Server instance, and how we customize our listener:

  • server.BaseContext: this is set across the entire space so that every request context is rooted in the application context
  • server.ReadTimeout: configured outside of default values (can pass into NATS websocket options as HandshakeTimeout)
  • server.ReadHeaderTimeout: configured outside of default values
  • server.WriteTimeout: configured outside of default values
  • server.IdleTimeout: configured outside of default values
  • server.MaxHeaderBytes: configured outside of default values
  • server.Shutdown: use configurable timeout to finish up any pending requests after closing listener
  • net.Listener: tls configuration is integrated with an ACME solution to provide latest up to date certificates on every connect
  • net.Listener: support for PROXY protocols for being hosted behind an L4 load balancer for H2 compatibility
  • net.Listener: configured tcp keep alive and keep alive period with configurable timeouts

The mux is really the least of the issues, as we could just route a specific path in our mux to use the http.ServeMux ServeHTTP function.

@kozlovic
Copy link
Member

kozlovic commented May 8, 2024

@BlizzTom Ok, I think you have a compelling reason why you would not be able to use the other PR and need your own http server and handler. I have few comments/questions:

  • http.Server needs a Handler which must implement the ServerHTTP interface, so I assume that the new function you propose would be invoked from your own handler?
  • I understand that you would not even configure the NATS Server websocket options, which means that the websocket internal structure will not be "initialized", which means that your NATS websocket clients will not benefit from the server features such as checking origin (that you document in the function doc), but also receiving INFO protocols with information regarding the addresses where clients could reconnect should the connection be disconnected. I guess this is fine since in your context, but something to keep in mind.
  • Because of the point above, the INFO protocol received by the server will have possibly erroneous information regarding TLS since the NATS Server itself will not have websocket's TLS configuration specified while your http server may. It was reported in No tls_required in INFO for TLS-only WebSocket connections #4252 and addressed in WebSocket-specific INFO #4255 (which I was planning on reworking by lifting that to the initialization of the NATS websocket server since these info - as far as I know - can't be config reloaded and will be immutable).
  • I would have liked to see a test added to demonstrate how this would be used and have a bit of code coverage for this new function.

@BlizzTom
Copy link
Contributor Author

BlizzTom commented May 8, 2024

Thanks for that feedback @kozlovic, I will work on a test case for it, as well as seeing if I can figure out the INFO point brought up , as that could potentially be an issue, since in my use case, the host and port would always be the same.

I would need to check the nats.js implementation to see if it actually uses the INFO block or not for reconnects. From my own experiences, it hasn't been using it, and just reconnects to the original endpoint.

On the testing front, the intended usage is from the nats.js package, and I am not really sure how to invoke that properly from the go tests, do you have any recommendations on that?

@kozlovic
Copy link
Member

kozlovic commented May 8, 2024

@BlizzTom The nats.js uses the INFO protocol to get the list of possible "connect URLs", which is a list of URLs that a client can reconnect to (augmented to the list of NATS Server URLs in a given cluster). In your approach, this list would not be specified, so the client library would indeed always attempt to reconnect to the initial given URL, which again, in your context makes sense.

As for a test, I wanted to at least see something like starting a NATS Server and create an http.Server and run it, passing the new function you have added, then a simple test that checks that a NATS client can connect:

// port will be the port of the net listener that you have created and passed to http.Server
nc := natsConnect(t, fmt.Sprintf("ws://127.0.0.1:%d", port))
defer nc.Close()

If the connection succeeds, and knowing that the port is not a NATS Server port (neither normal client nor websocket), it will good enough for a basic test.

Otherwise, using the low level test helpers (as in TestWSTLSVerifyAndMap) would be able to inspect an incoming INFO protocol. But there is no need since you won't have a way to influence this with the new function (HandleWebsocket) you want to add. But yes, say that if in the test the NATS Server does not have websocket{} defined and your http.Server uses TLS, and you connect the NATS client with wss:// instead of ws://, it would work, but the INFO would likely not show that TLS is available/required/etc.. as described in the previous comment.

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

3 participants