Accept an unset `command` to proxy to an already started process (unmanaged process) #339

ryshoooo · 2022-06-21T15:28:47Z

Updated PR description by Erik

jupyter-server-proxy enables users to proxy to already started processes by {base_url}/proxy/{port}/{proxied_path}, but for a better user experience it could be nice to just be able to use {base_url}/{proxied_path}. This would rely on someone having configured how that should be proxied ahead of time. This PR adds support for this.

By providing a server process configuration like this, omitting the previousy required command, jupyter-server-proxy will assume the server process isn't to be managed/started by jupyter-server-proxy, and that its already started and accepting traffic. jupyter-server-proxy won't try to await it either before proxying traffic to it.

# Below we configure jupyter-server-proxy to
# provide the route {base_url}/already-started-process
# as a convenience for the user un-friendly route
# {base_url}/proxy/54321/already-started-process,
# note how command is omitted!
c.ServerProxy.servers = {
{
    "already-started-process": {
        "port": 54321,
    },
}

Not providing a command to a server process definition means that its an "jupyter-server-proxy unmanaged server process" that we proxy to, supposed to be started ahead of time.

Outdated PR description

This PR introduces additional parameter for the server proxy configuration, which specifies whether the port availability checking is necessary to perform or not. This is useful to disable if the application is already running (started by something else) and the user just wants to create a simple proxy for it (not start it through proxy directly).

ryanlovett · 2022-06-23T01:15:59Z

Hi @ryshoooo, if/when I need to start the proxied service manually I use the anonymous proxy URL, e.g. {base_url}/proxy/{port}/{proxied_path} rather than {base_url}/{name}/{proxied_path}. Is this method not desired or sufficient? Is this what you mean by, "(not start it through proxy directly)" ?

ryshoooo · 2022-06-23T07:16:26Z

Hi @ryshoooo, if/when I need to start the proxied service manually I use the anonymous proxy URL, e.g. {base_url}/proxy/{port}/{proxied_path} rather than {base_url}/{name}/{proxied_path}. Is this method not desired or sufficient? Is this what you mean by, "(not start it through proxy directly)" ?

Hi @ryanlovett, yes it is exactly not desired :) To give you a specific example of what we're facing, we're using Z2JH with Kubernetes and we wish to provide code-server for each single-user environment. The old way we used to do this, was that we pre-installed code-server into the single-user base image and then used j-s-p the way it's supposed to be used, i.e. the command starts code-server and we open it up at some random open port. We're moving away from this solution in order to have even more micro-service architecture in place, so now we start the code-server as an additional extra container in the single-user pod during spawning. This approach has quite a few advantages, mainly that now we can do upgrades of code-server without touching the single-user base image. Anyway, the users got very much used to the fact that URL user/{username}/code points to the vscode, so we'd like to preserve that with the new setup :)

manics · 2022-06-23T08:21:44Z

I think the general idea sounds good. Did you consider any other options, e.g. something like managed_service=True|False to control whether JSP starts the service or not, which is easier to debug (since you're not in an indeterminate state)

ryshoooo · 2022-06-23T10:02:26Z

I think the general idea sounds good. Did you consider any other options, e.g. something like managed_service=True|False to control whether JSP starts the service or not, which is easier to debug (since you're not in an indeterminate state)

A little bit yes, I just decided not to go this way fully just yet :) I merely just wanted to have something quickly working without too many changes to the codebase. Which this does for me :) But I do like your suggestion and I do think it is the better solution with the managed_service parameter. But then it makes command parameter conditionally optional, which I'm not sure how I feel about it, feels like spaghetti. So that's mainly why I didn't go that way :)

If you think it's the right way, I can try and turn it into managed_service parameter. It'll take a bit of time :)

manics · 2022-07-01T11:05:56Z

But then it makes command parameter conditionally optional, which I'm not sure how I feel about it, feels like spaghetti.

That's a good point! Maybe we could make the command argument optional instead of adding the new flag. If command is empty just setup the port-fowarding?
@ryanlovett what do you think?

ryanlovett · 2022-07-06T17:44:06Z

@manics That seems reasonable to me.

Co-authored-by: Yuvi Panda <yuvipanda@gmail.com>

Bumps [moment](https://github.com/moment/moment) from 2.29.1 to 2.29.2. - [Release notes](https://github.com/moment/moment/releases) - [Changelog](https://github.com/moment/moment/blob/develop/CHANGELOG.md) - [Commits](moment/moment@2.29.1...2.29.2) --- updated-dependencies: - dependency-name: moment dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>

ryshoooo · 2022-07-07T13:20:41Z

I've added the changes, removed the check_port parameter, and made the command argument optional. However, the tests seem to be failing due to missing jupyter-notebook binary? Seems a bit unrelated to this PR right?

ryanlovett · 2022-07-07T17:08:11Z

Thanks @ryshoooo ! Yes, I'm guessing we need to add jupyter-notebook as a new dependency before running tests. (in a separate PR)

manics · 2022-07-08T17:39:40Z

The tests should be fixed by #340

Would you mind adding a test for this new configuration, e.g. based on

jupyter-server-proxy/tests/resources/jupyter_server_config.py

Lines 39 to 41 in 099fce2

    
           'python-http-port54321': { 
        
               'command': ['python3', './tests/resources/httpinfo.py', '{port}'], 
        
               'port': 54321,

jupyter-server-proxy/tests/test_proxies.py

Lines 141 to 150 in 099fce2

    
           def test_server_proxy_requested_port(): 
        
               r = request_get(PORT, '/python-http-port54321/ghi', TOKEN) 
        
               assert r.code == 200 
        
               s = r.read().decode('ascii') 
        
               assert s.startswith('GET /ghi?token=') 
        
               assert 'X-Forwarded-Context: /python-http-port54321\n' in s 
        
               assert 'X-Proxycontextpath: /python-http-port54321\n' in s 
        
               direct = request_get(54321, '/ghi', TOKEN) 
        
               assert direct.code == 200

For the backend service you can just connect to one of the existing ones, so effectively the new configuration would be an alias.

ryshoooo · 2022-07-08T18:58:23Z

@manics

Thanks for the guidance :) I've added an additional test for the new functionality.

ryshoooo · 2022-12-20T22:41:12Z

Looks like the pipeline is failing again, but the logs are a bit vague. Does anybody have any idea what's going on?

docs/source/server-process.rst

jupyter_server_proxy/config.py

consideRatio · 2022-12-28T12:40:36Z

jupyter_server_proxy/handlers.py

+                if not cmd:
+                    return
+


Is this called to late? Should self.state['proc'] be set to a new SupervisedProcess object? The idea was that if the command is unset, it shouldn't be supervised - right?

If it is correct, a comment is needed to clarify why it is correct in my mind as this is quite unexpected for me.

Besides this, the changes looks good to me!

Like it is now, it won't be awaited readiness either. I'll push a commit to move this logic upwards: d6fbbd8

consideRatio · 2022-12-28T12:41:46Z

Looks like the pipeline is failing again, but the logs are a bit vague. Does anybody have any idea what's going on?

Please ignore them. I'm not sure, but its related to jupyter_server 2 being released and that the jupyterlab extension isn't doing well with it.

consideRatio

This LGTM as it is now. I added two commits, one updating docs and another one relocating a return statement to happen sooner rather than later when an unmanaged process is involved.

The test failures should be unrelated, see Tests broken for jupyter_server 2 - help wanted! #362.

@ryshoooo does this look good to you as it is now?

ryshoooo · 2023-01-02T09:49:27Z

This LGTM as it is now. I added two commits, one updating docs and another one relocating a return statement to happen sooner rather than later when an unmanaged process is involved.

The test failures should be unrelated, see Tests broken for jupyter_server 2 - help wanted! #362.

@ryshoooo does this look good to you as it is now?

Hi @consideRatio

Yes this looks totally fine to me, thanks a lot for your help! :)

welcome · 2023-01-02T09:54:48Z

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

consideRatio · 2023-01-02T09:55:13Z

Thank you @ryshoooo! ❤️ 🎉

This comment was marked as resolved.

Sign in to view

feat: make command optional, only proxy on good faith

50a4f28

ryshoooo force-pushed the main branch from ef19fa0 to 50a4f28 Compare July 7, 2022 11:46

ryshoooo and others added 8 commits July 7, 2022 14:26

Merge remote-tracking branch 'upstream/main'

f82e008

add allow-downloads and allow-modals to sandbox

4afd012

Add documentation on how-to disable the extension

e5ec689

Blacklist Transfer-Encoding header

9f94ca0

Hop-By-Hop header handling

f7b5089

Update jupyter_server_proxy/handlers.py

295afb1

Co-authored-by: Yuvi Panda <yuvipanda@gmail.com>

Merge branch 'main' of https://github.com/ryshoooo/jupyter-server-proxy

e9ee439

fix: if no command is specified, process is ensured

2e740aa

ryshoooo added 2 commits July 8, 2022 20:47

Merge branch 'jupyterhub:main' into main

f5257d6

test: added test for port-forwarding

a2b1df7

ryshoooo added 4 commits July 8, 2022 21:43

docs: add the note into documentation

62ac65a

fix: specify docs language

05107fe

Merge branch 'jupyterhub:main' into main

ee31d7b

chore: merge with main

d261d3b

consideRatio added the new label Dec 28, 2022

consideRatio changed the title ~~Make port checking optional~~ Accept an unset command to proxy to an already started process Dec 28, 2022

consideRatio changed the title ~~Accept an unset command to proxy to an already started process~~ Accept an unset command to proxy to an already started process (unmanaged process) Dec 28, 2022

consideRatio reviewed Dec 28, 2022

View reviewed changes

docs/source/server-process.rst Outdated Show resolved Hide resolved

consideRatio reviewed Dec 28, 2022

View reviewed changes

jupyter_server_proxy/config.py Outdated Show resolved Hide resolved

consideRatio reviewed Dec 28, 2022

View reviewed changes

consideRatio added 2 commits December 29, 2022 18:33

Adjust docs about use of unset command option

ebb8e35

Let ensuring_process return early for unmanaged processes

d6fbbd8

consideRatio approved these changes Dec 29, 2022

View reviewed changes

consideRatio merged commit 15447ba into jupyterhub:main Jan 2, 2023

takluyver mentioned this pull request Jan 18, 2023

Support proxying to a server process via a Unix socket #337

Merged

mahendrapaipuri mentioned this pull request Apr 12, 2023

Add manager for tracking and terminating running server proxies #395

Open

manics mentioned this pull request Apr 24, 2023

Increase timeout for the default proxying mechanism #403

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept an unset `command` to proxy to an already started process (unmanaged process) #339

Accept an unset `command` to proxy to an already started process (unmanaged process) #339

ryshoooo commented Jun 21, 2022 •

edited by consideRatio

Loading

This comment was marked as resolved.

ryanlovett commented Jun 23, 2022

ryshoooo commented Jun 23, 2022

manics commented Jun 23, 2022

ryshoooo commented Jun 23, 2022

manics commented Jul 1, 2022 •

edited

Loading

ryanlovett commented Jul 6, 2022

ryshoooo commented Jul 7, 2022

ryanlovett commented Jul 7, 2022 •

edited

Loading

manics commented Jul 8, 2022

ryshoooo commented Jul 8, 2022

ryshoooo commented Dec 20, 2022

consideRatio Dec 28, 2022

consideRatio Dec 28, 2022

consideRatio Dec 29, 2022 •

edited

Loading

consideRatio commented Dec 28, 2022

consideRatio left a comment •

edited

Loading

ryshoooo commented Jan 2, 2023

welcome bot commented Jan 2, 2023

consideRatio commented Jan 2, 2023

Accept an unset command to proxy to an already started process (unmanaged process) #339

Accept an unset command to proxy to an already started process (unmanaged process) #339

Conversation

ryshoooo commented Jun 21, 2022 • edited by consideRatio Loading

Updated PR description by Erik

Outdated PR description

This comment was marked as resolved.

ryanlovett commented Jun 23, 2022

ryshoooo commented Jun 23, 2022

manics commented Jun 23, 2022

ryshoooo commented Jun 23, 2022

manics commented Jul 1, 2022 • edited Loading

ryanlovett commented Jul 6, 2022

ryshoooo commented Jul 7, 2022

ryanlovett commented Jul 7, 2022 • edited Loading

manics commented Jul 8, 2022

ryshoooo commented Jul 8, 2022

ryshoooo commented Dec 20, 2022

consideRatio Dec 28, 2022

Choose a reason for hiding this comment

consideRatio Dec 28, 2022

Choose a reason for hiding this comment

consideRatio Dec 29, 2022 • edited Loading

Choose a reason for hiding this comment

consideRatio commented Dec 28, 2022

consideRatio left a comment • edited Loading

Choose a reason for hiding this comment

ryshoooo commented Jan 2, 2023

welcome bot commented Jan 2, 2023

consideRatio commented Jan 2, 2023

Accept an unset `command` to proxy to an already started process (unmanaged process) #339

Accept an unset `command` to proxy to an already started process (unmanaged process) #339

ryshoooo commented Jun 21, 2022 •

edited by consideRatio

Loading

manics commented Jul 1, 2022 •

edited

Loading

ryanlovett commented Jul 7, 2022 •

edited

Loading

consideRatio Dec 29, 2022 •

edited

Loading

consideRatio left a comment •

edited

Loading