jupyterhub / jupyter-server-proxy

Jupyter notebook server extension to proxy web services.
https://jupyter-server-proxy.readthedocs.io
BSD 3-Clause "New" or "Revised" License
351 stars 148 forks source link

Add `raw_socket_proxy` to directly proxy websockets to TCP/unix sockets #447

Closed dylex closed 4 months ago

dylex commented 8 months ago

This incorporates the functionality of websockify by allowing websockets to be proxied to simple TCP or unix socket streams.

While I understand that websockify already provides a working solution to this problem (see discussion on #75), this seems to have a number of benefits, mainly that it eliminates the extra proxy layer and process that websockify imposes, improving latency and performance overall. It does so with a fairly small amount of code, much smaller and ismpler than websockify itself.

To simplify the instantiation of proxy handlers, this also generalizes and combines the _make_namedproxy_handler and _make_supervisedproxy_handler functions into a single _make_proxy_handler that dynamically determines the class to use, improving flexibility and reducing code duplication, at the cost of a little complexity.

welcome[bot] commented 8 months ago

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. welcome You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! :wave:
Welcome to the Jupyter community! :tada:

manics commented 6 months ago

Thanks for the PR, and sorry for the delay in reviewing. I like the idea of having a self contained pure Python implementation! I can't guarantee we'll merge it since websockify works, seems reliable, and it's always nice if someone else maintains things, but https://github.com/jupyterhub/jupyter-server-proxy/issues/75 is several years old and I think we should consider this.

To help with review would you mind rebasing this onto main? We recently fixed a security vulnerability, so it would be helpful when testing locally if we could just checkout an up-to-date branch with your changes. Thanks!

dylex commented 6 months ago

Thanks, yeah I understand the concerns, but having worked on the websockify code as well, I can at least say that this PR is overall much simpler and easier to maintain, and I'm happy to help do so.

I've rebased, and tested again on the updated version (with jupyter-remote-desktop-proxy) and things look okay. Probably there should be some automated tests for this, too -- I can look into that if there's interest in moving forward.

manics commented 6 months ago

Thanks! Tests would be great if we go ahead but hold off for now. I'll try out your changes locally first, feel free to bump this issue if I forget!

yuvipanda commented 4 months ago

Bump reminder, @manics :)

yuvipanda commented 4 months ago

I very much like the idea of not depending on websockify for TCP streams. This would allow us to package remote-desktop-proxy in conda-forge more easily too. I've re-opened #75 and will look through the code now.

dylex commented 4 months ago

Okay, I think everything has been addressed now. Option is now named raw_socket_proxy, and there are a couple tests with a simple tcp and unix socket server backend. Let me know if there's anything I should cleanup/rebase.

yuvipanda commented 4 months ago

Alright, thank you very much for your work, @dylex! And sorry about the slow review. I hope to see more contributions from you in the future too <3