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

Add option to override host in displayed url #3668

Merged
merged 1 commit into from Jun 20, 2018

Conversation

Projects
None yet
5 participants
@tomjorquera
Copy link

tomjorquera commented Jun 7, 2018

This commit introduces a new alias connect-host to override the host
info displayed at launch with a custom string.

It is intended to be used when the app is run in an environment where
the url to display to the users is not detectable reliably (proxified or
containerized setups for example).

Closes #3605

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jun 10, 2018

We have properties connection_url and display_url - I think it's surprising if connect_host affects display but not connection. But I don't know whether it's better for it to affect both, or to rename the config option.

@minrk Do you remember why we have two separate URL options? I think connection_url gets recorded in the server info file, but I'm not sure why we make two different estimates of the URL. Is connection_url meant to be the address you can connect to on the same host that's running the server? If we can't unify them, we should certainly find a way to make this clearer.

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Jun 11, 2018

Maybe this should change display_url to a trait with the current logic as a default value generator, rather than a property? Since this is just for display, calling it display_url makes more sense to me. connect would be right if there actually is logic that uses this to establish connections. If we intend for this to be the case, then calling it connect_url does makes sense to me. (pedantry: it's not a 'host', since it includes protocol and url path, it's a full URL. We just ran into an issue where I made this mistake and confused people in jupyterhub/oauthenticator).

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jun 11, 2018

I think there probably wants to be a property involved, because something has to add the generated token to the bit you can configure.

We use URLs for:

  • Printing in the terminal to tell the user what to open
  • Launching a browser when the server and the browser are on the same system
  • Writing into the notebook server info file
    • This is used by jupyter notebook stop to connect and send an HTTP request
    • It can also be used by tools like nbmanager to query a running notebook server

Those are all the use cases I can think of at the moment, but there may be others.

@tomjorquera

This comment has been minimized.

Copy link
Author

tomjorquera commented Jun 13, 2018

@takluyver, regarding the different display_url and connection_url property:

I am not too clear on the inner workings of jupyter-notebook, so I was worried that connect_url may be used internally and be required to contains the actual locally accessible URL. Maybe it is the case in the two examples you give under "Writing into the notebook server info file" ?

More generally, my concern is about using jupyter-notebook as a proxied service (in a docker container, behind a proxy etc.), and needing both a publicly accessible URL for the users, and an internal URL for administering. But maybe it's actually a non-issue?

@minrk, regarding the URL vs host terminology:

Agreed! I initially kept the name proposed in #3605 for consistency, but I think it would be better to rename it. Just waiting for the discussion to converge on what to do to rename it accordingly.

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Jun 13, 2018

@takluyver good summary. In that case, I think connect_url is the right choice, since other programs may use this to connect, so if a human wants to override "use this URL to connect", then I think it's a good choice.

In that case, this PR roughly as-is with connect_url as the config option rather than connect_host should work.

if self.ip in ('', '0.0.0.0'):
ip = socket.gethostname()
if self.connect_host:
url = self.connect_host + '/'

This comment has been minimized.

@minrk

minrk Jun 13, 2018

Member

Probably don't want to add + '/' here, instead leave connect_url untouched.

This comment has been minimized.

@willingc

willingc Jun 19, 2018

Member

Should connect_host be renamed to custom_connect_url or custom_display_url. I'm a bit confused by the naming. Or possibly public_url or public_display_url.

More generally, my concern is about using jupyter-notebook as a proxied service (in a docker container, behind a proxy etc.), and needing both a publicly accessible URL for the users, and an internal URL for administering.

This comment has been minimized.

@tomjorquera

tomjorquera Jun 19, 2018

Author

Renamed

@minrk : regarding the trailing '/', i changed things to append it if it is missing. I think it is the "nicest" approach, but tell me if you have reserves on this.

@@ -548,6 +548,7 @@ def start(self):
'notebook-dir': 'NotebookApp.notebook_dir',
'browser': 'NotebookApp.browser',
'pylab': 'NotebookApp.pylab',
'connect-host': 'NotebookApp.connect_host',

This comment has been minimized.

@minrk

minrk Jun 13, 2018

Member

I don't think this is an option that warrants a CLI alias.

This comment has been minimized.

@tomjorquera
@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jun 13, 2018

You mean replace connection_url with a configurable option? I was coming to the opposite conclusion, that there was a need for a separate display URL and connection URL - the later being valid only on the same host.

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Jun 14, 2018

Yeah, I mean that the URL that should be used to connect should be overrideable by the user. That is, the URL written to notebook list commands, etc. This may actually be used for connect, etc. This is useful for the cases where the notebook cannot determine its connect URL.

The display URL for humans should use the connect URL, and can be a property adding things like the token.

@tomjorquera

This comment has been minimized.

Copy link
Author

tomjorquera commented Jun 15, 2018

@minrk Sorry if I have misunderstood your proposal, what I got is that you suggest the option to not just override the return of display_url but the return of connection_url instead.

It seems to me that it would break the functioning of commands such as notebook stop (it does in my quick testing but maybe I missed something).

The use case I have in mind is the following:
With a notebook server running in a container, I want:

  • as a user, to have a URL I can directly paste in my browser transparently
  • as an admin, to be able to go inside the container and use admin commands as usual (list, stop and so on...)

Its not clear for me if there is a way to make this work with your proposal without breaking the second point (or even if it is a non-issue and that I missed something when testing it locally) .

Any thought ?

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Jun 15, 2018

I guess the question is:

  1. should the user be able to specify what URL should be used to connect to the notebook server in general (connection_url), or
  2. only specify what URL should be printed in the logs (display_url), i.e. what a human should use to open the notebook, but not affect anything that discovers the URL programmatically.

1 would allow things like jupyter notebook list to provide useful info for connecting from outside the container, but possibly break things like jupyter notebook stop inside the container. 2 suggests that we never want the notebook to know its own URL that things should use, e.g. reporting its own connection URL for use outside the container after the initial log.

From your use cases, I think jupyter notebook list in the container would work better in the first case, because it would give you the info you need to connect from outside the container, rather than having to dig into the logs for that. That said, I don't think that the process-management tools like notebook list and notebook stop make a great deal of sense in a container, since there's necessarily only one server and it can be terminated normally with docker stop commands, etc.

I genuinely don't know the right answer for this. Overriding display_url is certainly the least disruptive since it can't break anything, but overriding connect_url enables solving the same "How should things connect to me" problem in multiple places, not just the logs. I'm genuinely not sure what the right answer is.

Another option is to add a separate 'remote_url' for connections from 'remote' hosts (e.g. the host system in the docker case), and store and display this alongside connection_url, essentially baking in the fact that there can never be only one answer to the URL question when running in containers.

@minrk minrk added this to the 5.6 milestone Jun 15, 2018

@tomjorquera

This comment has been minimized.

Copy link
Author

tomjorquera commented Jun 19, 2018

Since apparently there is two separate use cases that need different ways to be solved, I wonder if the most straightforward solution wouldn't be to add two separation options.

I am not fond of having lots of small options, but in this case it seems that there is a variety of current and future use cases that could warrant it...

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Jun 19, 2018

@minrk What is needed for this to be completed?

@willingc
Copy link
Member

willingc left a comment

The naming here is a bit confusing to me. Perhaps start with improving the docstring to be more explicit about why and when this would be used as well as what changes when using it.

help=_("""Custom host to show in displayed URL.
Replace actual host, including protocol, address, port and base URL,
with the given value when displaying URL to the users. Do not change

This comment has been minimized.

@willingc

willingc Jun 19, 2018

Member

We probably should clarify this docstring a bit. As it reads now, my understanding is that the connect_host would be displayed to the user, but the notebook would use the actual underlying connection URL.

This comment has been minimized.

@tomjorquera

tomjorquera Jun 19, 2018

Author

Funnily I had a very similar docstring amending in store :) I merged the two. Tell me if it seems clearer.

This comment has been minimized.

@tomjorquera

tomjorquera Jun 19, 2018

Author

Updated. tell me if it's still not clear enough (your understanding was correc btw 😉)

if self.ip in ('', '0.0.0.0'):
ip = socket.gethostname()
if self.connect_host:
url = self.connect_host + '/'

This comment has been minimized.

@willingc

willingc Jun 19, 2018

Member

Should connect_host be renamed to custom_connect_url or custom_display_url. I'm a bit confused by the naming. Or possibly public_url or public_display_url.

More generally, my concern is about using jupyter-notebook as a proxied service (in a docker container, behind a proxy etc.), and needing both a publicly accessible URL for the users, and an internal URL for administering.

@willingc

This comment has been minimized.

Copy link
Member

willingc commented Jun 19, 2018

@tomjorquera, thanks for the PR.

Adding this to the docstring would be helpful:

It is intended to be used when the app is run in an environment where
the url to display to the users is not detectable reliably (proxified or
containerized setups for example)

@willingc

This comment has been minimized.

Copy link
Member

willingc commented Jun 19, 2018

Should this be pushed out to 5.7 release?

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jun 19, 2018

I'd like to get it in 5.6 if practical - we have had a few bug reports since 5.5.

I suggest that for now, we leave the two separate URL properties in existence, and focus on making display_url overridable, since that is what people have been having practical problems with. So let's rename the config option to be clear about that, e.g. something like custom_display_url or display_url_base (base because the token is appended to make the display URL).

That means that for the time being, we're saying that connection_url - which jupyter notebook list and jupyter notebook stop use - is only intended to be usable from the same host that the server is running on. Depending on the network it may be usable from elsewhere (e.g. your system outside a container), but I don't think we can guarantee that in general.

@tomjorquera tomjorquera force-pushed the tomjorquera:connect_host branch from 9e147dc to c9cc5cf Jun 19, 2018

@tomjorquera

This comment has been minimized.

Copy link
Author

tomjorquera commented Jun 19, 2018

Updated the PR to clean things up regarding naming and docstring. So currently it only add the option to override the display URL and not the actual URL.

For what I understand of the code, adding a second option to override the actual URL seems straightforward enough, so I think it can be added to the PR should it be the way to go...

@tomjorquera tomjorquera force-pushed the tomjorquera:connect_host branch from c9cc5cf to 272d886 Jun 19, 2018

Add option to override displayed url
This commit introduces a new alias `custom_display_url` to override the
URL info displayed at launch with a custom string.

It is intended to be used when the app is run in an environment where
the url to display to the users is not detectable reliably (proxified or
containerized setups for example).

@tomjorquera tomjorquera force-pushed the tomjorquera:connect_host branch from 272d886 to 24e39d3 Jun 19, 2018

@tomjorquera

This comment has been minimized.

Copy link
Author

tomjorquera commented Jun 19, 2018

(also rebased on top of current master, and updated commit description)

@willingc
Copy link
Member

willingc left a comment

This looks good to me. Thanks @tomjorquera for the improvements.

@minrk minrk merged commit 2bad812 into jupyter:master Jun 20, 2018

4 checks passed

codecov/patch 55.55% of diff hit (target 0%)
Details
codecov/project 74.07% (-0.32%) compared to e7771ba
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@minrk

This comment has been minimized.

Copy link
Member

minrk commented Jun 20, 2018

@tomjorquera thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.