jupyter-server-proxy icon indicating copy to clipboard operation
jupyter-server-proxy copied to clipboard

Integrating native-proxy

Open jwindgassen opened this issue 1 year ago • 7 comments

Hey everyone.

I was recently made aware, of the desire to be able to create standalone proxies, similar to how it is done in jhsingle-native-proxy (see #1). This would be immensely advantageous for us, so I started porting the code here recently.

There is still a lot to do and I needed to remove/comment out a few of the original features, but it is already fundamentally working as is. I am opening this PR to let you know of this and get an opinion on a few bits here and there. I will continue to improve on it in the next weeks.

In the meantime, any comments and ideas are welcome :)

jwindgassen avatar Sep 10 '24 20:09 jwindgassen

Hi @jwindgassen , thanks for this contribution!

Is the idea to replace jhsingle-native-proxy (so that code can be shared), or will that project continue on? If the latter then could there still be duplication of code? (will this need to periodically get resynced with jhsingle-native-proxy?) Or should shared code be factored out into something separate for each project to use?

If there is documentation on using this, could that be integrated as well?

ryanlovett avatar Sep 10 '24 20:09 ryanlovett

That is a good question. I do not know about the current state of the original project. The newest commit was already made a few months ago, but I do not think it is abandoned. I will contact the original developer soon and ask him about the current status and his opinion on this.

But the proxies used by it are almost identical to the proxies here, and (I think) they originally were a copy of an old version of the ones in this package. IMHO, the best would either be completely merging it into here, or making JSP a dependency of jhsingle-native-proxy and importing the Proxies there. But that is, of course, not my decision to make but one of the original developer. Furthermore, jhsingle-native-proxy does not support Unix Sockets, which we would love to use. But since both projects have a lot of similarity, keeping them completely separate from one another is a lot of redundancy.

The Documentation for it is currently in the ReadMe, but could be added to the docs here, given that we decide to add this feature.

jwindgassen avatar Sep 10 '24 21:09 jwindgassen

This sounds like a great idea. I would suggest to create some kind of checklist of the features that are ported from jhsingle-native-proxy and what's not during the course of this pull request (or a series of pull requests).

We're using jhsingle-native-proxy heavily in jhub-apps would love to move to just using jupyter-server-proxy for everything we can, and having a feature parity checklist will help us understand, what we'll be having/missing with this port.

aktech avatar Sep 18 '24 09:09 aktech

@ryanlovett I talked to the original developer. He welcomes the merging of his feature into Jupyter Server Proxy. There are currently no further plans for jhsingle-native-proxy, but it will persist (for now), as there are a few custom additions, which I currently do not plan to integrate here.

@aktech Sure. Here is a list with the features I have currently ported or plan to do so:

  • [x] Launching standalone proxies from CLI
  • [x] Automatically enable JupyterHub Authentication when we are spawned by one
  • [x] Send Activity Notifications to JupyterHub
  • [x] Ensure usability with Unix-Sockets
  • [x] Ensure authenticated Access via JupyterHub
  • [x] Allow customization of environment and mappath
  • [ ] Progressive Proxies (#502)
  • [x] Docs & Tests (maybe even with JupyterHub?)
  • [ ] (If desired) Re-add pulling of Git Repository and venv/conda activation?

That's at least everything I can think of now. If you have other ideas or require something more, let me know and I will add it to the list :)

jwindgassen avatar Sep 18 '24 12:09 jwindgassen

@jwindgassen Thanks very much for asking! It sounds like this has the potential to consolidate development in the future.

ryanlovett avatar Sep 18 '24 17:09 ryanlovett

@ryanlovett @aktech I have been working on the feature over the last few weeks, and I am now happy to announce, that the code is working 🎉 I tested it with my own TLJH instance, and it also works with our production JupyterHub Setup. I also verified that authentication with JupyterHub works. There are a few small things I will probably improve in the future, but I am quite happy with the state of the code as it is now.

I'm welcoming anyone to test the feature on their JupyterHub instance for testing. Please let me know about any problems or errors you encounter when doing so 🙂

How to use

For testing, I like to use voila. The command to execute might look like this: jupyter standaloneproxy --debug -- voila --no-browser --port={port} /path/to/notebook.ipynb"

What else do you need from my side, besides the code, to be satisfied with this PR? I am currently writing a page for the docs, which I will commit soon. I was also planning to write a few tests for the feature, but I will have to look into that later.

jwindgassen avatar Oct 18 '24 14:10 jwindgassen

@jwindgassen Thanks for the update! I'll try to test this week locally.

How might a hub administrator typically configure use of this feature? For example would they set every user to launch voila via standalone proxy from c.Spawner.cmd instead of jupyterhub-singleuser?

Is the intent of standalone to essentially re-use the hub's auth, spawner, user storage, etc. but limit what apps users can invoke because it specifies just the one? (since jupyter server + jupyter-server-proxy enables users to launch an arbitrary number of proxied apps)

ryanlovett avatar Oct 23 '24 05:10 ryanlovett

In essence, yes. With the standalone proxy feature, admins/developers of the JupyterHub can give the users of the Hub access to other applications instead of just Notebook/JupyterLab. While we can achieve the same with the current jupyter-server-proxy via the button in the Launcher, this is often an indirection when you only want to access e.g. RStudio. By using the SuperviseAndProxyHandler directly, without attaching it to a jupyter-server, we can (re)use the Authentication, User Model, Proxy, etc. JupyterHub provides. Secondly, it is also probably easier to integrate new web apps into an already existing cloud environment this way, than to manually setup everything yourself.

But since you need to overwrite c.Spawner.cmd or modify the behavior of the Spawner in a subclass, new Apps can only be added by the administrators. This gives users access to different apps on the JupyterHub landing page, similar to Open OnDemand. The Spawner then needs to switch between the launch commands accordingly. In our case, we are planning to provide RStudio, XpraHTML (Remote Desktop), MATLAB, NEST-Desktop, and more to come in the future, using this feature.

jwindgassen avatar Oct 24 '24 14:10 jwindgassen

Fascinating, thanks!

More of an aside, but how are you customizing the spawner to launch the different applications?

Edit: oh, is it jhub-apps as mentioned earlier?

ryanlovett avatar Oct 24 '24 16:10 ryanlovett

No. We have created our own custom Spawner. But is similar to this. We have overwritten the start method, which will submit a new Job to our cluster. And inside the start script, we start jupyterhub singleuser at the end.

But now you mention it, jhub-apps might synergyze quite well with the standalone feature.

jwindgassen avatar Oct 24 '24 18:10 jwindgassen

Thanks for the suggestion, I really like that idea. This should also solve the issue with keeping CLI Arguments up with any new options added to the proxies. I will look into it :)

jwindgassen avatar Oct 24 '24 20:10 jwindgassen

I've made a start in https://github.com/jupyterhub/jupyter-server-proxy/pull/507

manics avatar Oct 24 '24 22:10 manics

@manics

So I used your branch to create the CLI via traitlets. I'm no expert with traitlets, but I managed to get it working: https://github.com/jwindgassen/jupyter-server-proxy/commit/2d9eb5bb593e4a4f4a7b535ffc831ece36ea6548

I had to play around with aliases and the extra_args a bit, but I wanted to keep the CLI reasonably unchanged to before. Getting the command, which was previously a positional argument, was a bit tricky, since I couldn't find a proper way to add a positional argument to the Parser traitlets create. It was possible by overwriting _create_loader(), but not really pretty. Using extra_args instead works, but we now lack a proper explanation of command when using --help.

If you have a better idea or otherwise comments on my changes over there, let me know! :)

jwindgassen avatar Oct 28 '24 21:10 jwindgassen

Sorry for the delay. Since this is a new addition to Jupyter server proxy I think we should prioritise long term maintenance over just replicating the previous CLI- if there's a better way to do things we can use it as an opportunity to refactor.

We also don't need to do everything in one go, for example it's fine to initially focus on creating a functional standalone proxy along that only supports standard traitlets configuration, and add additional flags in a follow-up PR.

manics avatar Nov 06 '24 23:11 manics

I'm fine with how the CLI looks right now, so I'm happy to switch to this once your PR has been accepted.

I am also almost done with Tests and Docs, they should be ready by the end of the week.

jwindgassen avatar Nov 11 '24 10:11 jwindgassen

Hey @jwindgassen Thanks a lot for working on this and for the ping. I'll play with it this week to provide some feedback.

aktech avatar Nov 12 '24 14:11 aktech

Ok, I have now also added Docs and Tests for the new feature.

Writing proper tests for Login and Activity is a bit more difficult, since I would need to spawn a JupyterHub instance to gain full access to its API. For now, I limited it to only testing our code, since the classes I import from JupyterHub are tested over there.

I also added a section to the docs, mostly targeted to developers, where I explain the different sections of the code and what features I needed to implement to make this work smoothly.

If you think we need more tests for specific cases or want something to be explained in the docs in more detail, let me know.

jwindgassen avatar Nov 15 '24 14:11 jwindgassen

@jwindgassen So from your perspective this is ready to be merged. Great and Thank you!

( As soon as https://github.com/jupyterhub/jupyter-server-proxy/pull/507 and https://github.com/jupyterhub/jupyter-server-proxy/pull/508 is merged this native-proxy can be updated afterwards. But for now this PR here is implemented to be independent of them. True? )

jhgoebbert avatar Nov 30 '24 11:11 jhgoebbert

Yes. This is currently independent of #507 and functions without it. But in the future, once that has been merged, I would update the standalone feature to use traitlets.Configurable. It would be a lot tidier and future-proof using that approach.

@aktech Have you been able to get it running and did it work in your setup? I would highly appreciate any feedback or comments on this 🙂

@ryanlovett How do we continue for this PR? Is there anyone specific responsible for reviewing it? Is there still more you need here? Sorry for being a bit impatient, but we would like to get this feature running soon on our JupyterHub instance.

jwindgassen avatar Nov 30 '24 14:11 jwindgassen

In terms of next steps, I'm fine with merging if @manics thinks its okay to go ahead and then address the related PRs later.

ryanlovett avatar Dec 01 '24 05:12 ryanlovett

@ryanlovett If you're generally happy with this would you mind merging https://github.com/jupyterhub/jupyter-server-proxy/pull/507 first, then we can rebase this PR, and it'll make this PR smaller, and it'll be a lot clearer what we're adding.

@aktech I don't think we should completely reproduce jhsingle-native-proxy since we have to maintain this long term- cloning git repos and setting up conda envs doesn't feel like it's in scope. However I'd hope the move to traitlets makes it much easier to extend this in a separate app, or perhaps it's as easy as wrapping it in another script?

manics avatar Dec 04 '24 15:12 manics

@aktech Very nice to hear that it worked for you out of the box. I needed to find and fix some bugs when I installed it on our system to make it working, so I am relieved it worked without much effort for you now. You currently have the --skip-authentication flag enabled, does it work without that?

Regarding the conda/env activation and git puller, I would suggest seeing how desired this feature becomes in the future. I think it's probably quite niche, but maybe I am wrong and many people would like to use it. But for now I would consider it out of scope for this PR.

jwindgassen avatar Dec 04 '24 15:12 jwindgassen

@ryanlovett @manics Now that #507 is merged, should I rebase and tidy up the commits here? Or should I merge main into here and then append the required changes to the CLI?

jwindgassen avatar Dec 04 '24 20:12 jwindgassen

Up to you! I usually rebase my PRs.

manics avatar Dec 04 '24 21:12 manics

Ok. So I rebased on top of the new #507 Merge.

I had to refactor the ServerProcess Configurable a bit more to reuse it for the standalone. However, when done like this, it should reasonably well future-proof for any new attributes and functionality that might be added.

There are still 2 minor changes I am thinking about implementing:

  • Instead of using traitlets.config.Application, we could use jupyter_core.application.JupyterApp. This adds the --config argument, meaning existing JupyterHub setups could reuse their config.py file. A config.py might also be a bit nicer when setting up the standalone proxy for complicated setups
  • As I mentioned at the end of #507, I think we should be able to make ServerProcess and LauncherEntry a HasTraits instead of a Configurable.

Any comments, on the changes or these ideas, are very welcome :) @manics @ryanlovett

jwindgassen avatar Dec 07 '24 22:12 jwindgassen

This looks great to me. Can it be merged?

jhgoebbert avatar Jan 08 '25 14:01 jhgoebbert

@manics Thank you for the review.

Both check_origin and check_xsrf_cookie were not called in the original jhsingle-native-proxy implementation, so I assume they are not strictly necessary. The original implementation did not inherit from JupyterHandler, so there was no need to explicitly circumvent it there. We have also overridden check_xsrf_cookie in the ProxyHandler, mentioning to defer the checking of XSRF to the proxied app. The original implementation inherits from HubOAuthenticated like this: ProxyHandler(HubOAuthenticated, WebSocketHandlerMixin), while I inherit like this: StandaloneHubProxyHandler(HubOAuthenticated, SuperviseAndProxyHandler). So I need to override it again. But I will try to replicate the error I got when I did not skip them and take a closer look, if it is safe to skip them. Otherwise I will expand the comments there.

Regarding the --generate-config and --config flags, I would like to get those working. For more complicated setups, we will probably need a callable mappath in the future. This is only possible in the config.py. But I will try to ensure the server only runs when the config is loaded correctly. I will need to rework the CLI anyways when we merge #521, so I will unify and fix it after that.

We are currently working on getting the standalone feature running on our proper JupyterHub instance. We have already found (and fixed) a few bugs in the last few days while doing that. So if it gives you more confidence, we can wait with merging this PR until the standalone feature is fully running on our hub to make sure it is working there :)

jwindgassen avatar Feb 27 '25 15:02 jwindgassen

Great! We are able to directly start

with this StandaloneProxyServer from our JupyterHub. No need to start JupyterLab first and open the launchpad to start Xpra/VSCode from the button there.

jhgoebbert avatar Apr 06 '25 17:04 jhgoebbert

For Xpra the start command is: jupyter-standaloneproxy --config=$HOME/.jupyter/standalone-xpra-config.py with an standalone-xpra-config.py ==

import logging
import os
from jupyter_xprahtml5_proxy import setup_xprahtml5

# Execute the setup from the Jupyter proxy to retrieve the startup information
server_config = setup_xprahtml5()

# Enable debugging and use the correct network address
c.StandaloneProxyServer.log_level = logging.DEBUG
c.StandaloneProxyServer.address = os.environ.get("HOSTNAME", "127.0.0.1")

# Forward the server configuration from the Jupyter proxy to the standalone app
c.StandaloneProxyServer.merge(server_config)

For OpenVSCode-Server the start command is: jupyter-standaloneproxy --config=$HOME/.jupyter/standalone-vscode-config.py with an standalone-vscode-config.py ==

import logging
import os
from urllib.parse import urlparse, parse_qs
from jupyter_openvscodeserver_proxy import setup_openvscodeserver

# Execute the setup from the Jupyter proxy to retrieve the startup information
server_config = setup_openvscodeserver()

# Enable debugging and use the correct network address
c.StandaloneProxyServer.log_level = logging.DEBUG
c.StandaloneProxyServer.address = os.environ.get("HOSTNAME", "127.0.0.1")

# Forward the server configuration from the Jupyter proxy to the standalone app
c.StandaloneProxyServer.merge(server_config)

# Forward query params from launcher_entry.path_info, except the token
# The token will be directly added as a cookie to any request made to the server
# We can't add the token in mappath, otherwise we will end up in an infinite redirect loop
url = urlparse(server_config["launcher_entry"]["path_info"])
query_args = parse_qs(url.query)
token = query_args.pop("tkn")[0]
proxy_query_args = "&".join([f"{key}={value[0]}" for key, value in query_args.items()])

c.StandaloneProxyServer.mappath = {
    "/": f"/?{proxy_query_args}"
}

c.StandaloneProxyServer.request_headers_override = {
    "Cookie": f"vscode-tkn={token};"
}

jhgoebbert avatar Apr 06 '25 18:04 jhgoebbert

@manics It would be great if this patch could be merged. I can confirm that it works on our side.

jhgoebbert avatar Apr 06 '25 18:04 jhgoebbert