jupyterhub / jupyter-rsession-proxy

Jupyter extensions for running an RStudio rsession proxy
BSD 3-Clause "New" or "Revised" License
118 stars 87 forks source link

Configure rserver options via traitlets #147

Closed rickmcgeer closed 3 months ago

rickmcgeer commented 3 months ago

Proposed change

The documentation for Jupyter Server Proxies says that proxies can be configured by traitlets. However, it appears that the rserver options are hardcoded in this proxy, specifically in https://github.com/jupyterhub/jupyter-rsession-proxy/blob/master/jupyter_rsession_proxy/init.py, lines 88-96:

cmd = [
            get_rstudio_executable('rserver'),
            '--auth-none=1',
            '--www-frame-origin=same',
            '--www-port=' + str(port),
            '--www-verify-user-agent=0',
            '--secure-cookie-key-file=' + ntf.name,
            '--server-user=' + get_system_user(),
        ]

We wanted to change --www-frame-origin=same to ---www-frame-origin=any, appropriate for our application but not in general. In order to do this, we forked the repo, made the change, and installed that. Obviously, configuring this using traitlets would have been preferable.

Alternative options

I don't know of any, but that may be ignorance (that is, there may be a better way to do this that I've simply missed, and any guidance would be appreciated).

Who would use this feature?

Anyone who wants to launch rstudio from a JupyterHub, and tweak the environment.

(Optional): Suggest a solution

Convert the body of this to a class, instantiate and configure the class using traitlets/a config file, and invoke the entry point command as a method on that class (or, alternatively, write a function which instantiates the object, applies the traitlets if any, then invokes the command and returns it). I'll be happy to take this on, branch and do a PR if desired. Any pointers to existing configurable proxies would make this easier on both me and the reviewers :-)

welcome[bot] commented 3 months ago

Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. Also, please try to follow the issue template as it helps other other community members to contribute more effectively. welcome You can meet the other Jovyans by joining our Discourse forum. There is also an intro thread there where you can stop by and say Hi! :wave:
Welcome to the Jupyter community! :tada:

ryanlovett commented 3 months ago

Thanks @rickmcgeer ! I would love if you could submit a PR, perhaps to set the frame origin? For an example of using traitlet in such an extension, you can see an old, deprecated project of mine, https://github.com/ryanlovett/nbnovnc.

Then admins could set the value as c.NBNoVNC.novnc_directory = "/usr/local/src/novnc".

rickmcgeer commented 3 months ago

Thanks, very much for the reply and encouragement @ryanlovett! I will get on it. I may come back with questions. Is this thread the right venue, or would you prefer something else?

ryanlovett commented 3 months ago

No problem if you have questions, feel free to ask here -- I may or may not be able answer :) but I'll do my best.

If you make a PR and have questions about it, you can ask in that thread.

rickmcgeer commented 3 months ago

@ryanlovett I've been going over the code and running some experiments. One thing I notice is that when I use this, and then enter the running container and kill the rserver process, it immediately restarts, which suggests that there's a daemon restarting it. But that daemon doesn't seem to be in the usual spots (supervisor, etc) so I wondered where it was. Also, in the Dockerfiles which use this, I don't see any RUN commands. So it looks like this is all done in setup.py, in the structure: entry_points={ 'jupyter_serverproxy_servers': [ 'rstudio = jupyter_rsession_proxy:setup_rserver' ] }, So my guess is that as I refactor this, I should leave setup.py as it is, and have setup_rserver return a string of the form that it currently does. My approach right now is to move all of the subfunctions of setup_rserver into a subclass of traitlets.config.SingletonConfigurable, and setup_rserver will instantiate the sole class member and dig the command out of it. So, two questions:

Sorry for the noob question.

ryanlovett commented 3 months ago

jupyter-rsession-proxy uses jupyter-server-proxy which uses simpervisor to start supervised processes. That is why the process gets restarted. See https://github.com/jupyterhub/jupyter-server-proxy for nearly everything that this project makes use of.

setup_rserver is the entry point for the proxied server so I don't think you'll be able to convert it to a traitlet object. It returns a dictionary (server_process) and not a string. But you can turn the frame origin value into a traitlet that the command pulls in.

I hope this is helpful. I'm happy to follow up, and I appreciate the work you're putting into this!

rickmcgeer commented 3 months ago

@ryanlovett Thanks! That's exactly the information I was looking for! Just FYI, I was planning to retain setup_rserver as the entry point, I just wanted to check to make sure I knew what it returned. When I make some progress I'll be back to ask for feedback, and I really appreciate all the help you've given so far.

rickmcgeer commented 3 months ago

@ryanlovett I've made the change and issued a PR -- please take a look and tell me what you think. I tried to make the changes minimal and of course tested before I did the push, but more eyes and tests are better than fewer! I am quite new to this codebase, so it's entirely possible I missed something -- please let me know and I will fix it.

ryanlovett commented 3 months ago

Thanks @rickmcgeer ! I should be able to take a look at this later today.

ryanlovett commented 3 months ago

Fixed by #148.