Repository: jupyterhub/jupyterhub
Pull request: add hub_connect_ip
allows specifying the connect ip/hostname for the Hub when it differs from hub_ip (the bind address).

Used when the Hub is not on the same host as the spawners and/or proxy (e.g. docker, kubernetes, etc.)

One default change: when `hub_ip` is 0.0.0.0 (all-interfaces), the default connect ip is `socket.gethostname()` instead of 127.0.0.1, which increases the likelihood that it will work by default, though container cases will likely still need to set this value.

This should mean that Spawner-specific config (e.g. `DockerSpawner.hub_connect_ip`) should no longer be necessary.

closes #1163

cc @yuvipanda

Problem statement:
Any way to give a different hub IP to singleuser servers?
My current setup uses Jupyterhub 0.7.2 in a docker container, using a modified version of the batchspawner to start singleuser servers on a cluster. `JupyterHub.hub_ip` is set to `0.0.0.0`, but it binds to `127.0.0.1`.  This address is given to the singleuser server under the `--hub-api-url` argument. This isn't OK since the singleuser servers are running on different machines than the hub. I can't set the `hub_ip` to the public address because it is running in a docker container and can't bind to that address.

Is there any way from the config to tell the singleuser servers that it should connect to a different IP than the `hub_ip`? I'm currently doing a janky hack in the spawner to inject the correct address that obviously isn't the correct way to do it.

Hints from issue discussion:
Setting `hub_ip` does set the bind ip. If you set this to 0.0.0.0, 127.0.0.1 is used for connecting unless overridden somewhere. Some Spawners (e.g. DockerSpawner) set a separate `hub_connect_ip`, but this should be handled at the Hub level so that different spawners don't need to reimplement this. I'll work on it.

Commit message: update tests for connect_ip changes

Patch to review:
diff --git a/jupyterhub/app.py b/jupyterhub/app.py
index 4ba8018421..930b1977bd 100644
--- a/jupyterhub/app.py
+++ b/jupyterhub/app.py
@@ -392,11 +392,26 @@ def _deprecated_proxy_api(self, change):
         )
 
     hub_port = Integer(8081,
-        help="The port for this process"
+        help="The port for the Hub process"
     ).tag(config=True)
     hub_ip = Unicode('127.0.0.1',
-        help="The ip for this process"
+        help="""The ip address for the Hub process to *bind* to.
+
+        See `hub_connect_ip` for cases where the bind and connect address should differ.
+        """
     ).tag(config=True)
+    hub_connect_ip = Unicode('',
+        help="""The ip or hostname for proxies and spawners to use
+        for connecting to the Hub.
+
+        Use when the bind address (`hub_ip`) is 0.0.0.0 or otherwise different
+        from the connect address.
+
+        Default: when `hub_ip` is 0.0.0.0, use `socket.gethostname()`, otherwise use `hub_ip`.
+
+        .. versionadded:: 0.8
+        """
+    )
     hub_prefix = URLPrefix('/hub/',
         help="The prefix for the hub server.  Always /base_url/hub/"
     )
@@ -836,7 +851,8 @@ def init_hub(self):
             cookie_name='jupyter-hub-token',
             public_host=self.subdomain_host,
         )
-        print(self.hub)
+        if self.hub_connect_ip:
+            self.hub.connect_ip = self.hub_connect_ip
 
     @gen.coroutine
     def init_users(self):
diff --git a/jupyterhub/objects.py b/jupyterhub/objects.py
index b4019aaf25..2d164d1b52 100644
--- a/jupyterhub/objects.py
+++ b/jupyterhub/objects.py
@@ -3,6 +3,7 @@
 # Copyright (c) Jupyter Development Team.
 # Distributed under the terms of the Modified BSD License.
 
+import socket
 from urllib.parse import urlparse
 
 from tornado import gen
@@ -26,10 +27,22 @@ class Server(HasTraits):
     orm_server = Instance(orm.Server, allow_none=True)
 
     ip = Unicode()
+    connect_ip = Unicode()
     proto = Unicode('http')
     port = Integer()
     base_url = Unicode('/')
     cookie_name = Unicode('')
+    
+    @property
+    def _connect_ip(self):
+        """Property for connect_ip"""
+        if self.connect_ip:
+            return self.connect_ip
+        elif self.ip in {'', '0.0.0.0'}:
+            # if listening on all interfaces, default to hostname
+            return socket.gethostname()
+        else:
+            return self.ip
 
     @classmethod
     def from_url(cls, url):
@@ -67,13 +80,9 @@ def _change(self, change):
 
     @property
     def host(self):
-        ip = self.ip
-        if ip in {'', '0.0.0.0'}:
-            # when listening on all interfaces, connect to localhost
-            ip = '127.0.0.1'
         return "{proto}://{ip}:{port}".format(
             proto=self.proto,
-            ip=ip,
+            ip=self._connect_ip,
             port=self.port,
         )
 
@@ -92,7 +101,7 @@ def bind_url(self):
         since it can be non-connectable value, such as '', meaning all interfaces.
         """
         if self.ip in {'', '0.0.0.0'}:
-            return self.url.replace('127.0.0.1', self.ip or '*', 1)
+            return self.url.replace(self._connect_ip, self.ip or '*', 1)
         return self.url
 
     @gen.coroutine
@@ -101,7 +110,7 @@ def wait_up(self, timeout=10, http=False):
         if http:
             yield wait_for_http_server(self.url, timeout=timeout)
         else:
-            yield wait_for_server(self.ip or '127.0.0.1', self.port, timeout=timeout)
+            yield wait_for_server(self._connect_ip, self.port, timeout=timeout)
 
     def is_up(self):
         """Is the server accepting connections?"""
diff --git a/jupyterhub/tests/mocking.py b/jupyterhub/tests/mocking.py
index f8e4628a2b..38474a0229 100644
--- a/jupyterhub/tests/mocking.py
+++ b/jupyterhub/tests/mocking.py
@@ -246,8 +246,8 @@ class StubSingleUserSpawner(MockSpawner):
     _thread = None
     @gen.coroutine
     def start(self):
-        ip = self.user.server.ip
-        port = self.user.server.port = random_port()
+        ip = self.ip = '127.0.0.1'
+        port = self.port = random_port()
         env = self.get_env()
         args = self.get_args()
         evt = threading.Event()
diff --git a/jupyterhub/tests/test_orm.py b/jupyterhub/tests/test_orm.py
index d873cef299..a981effa4d 100644
--- a/jupyterhub/tests/test_orm.py
+++ b/jupyterhub/tests/test_orm.py
@@ -3,6 +3,8 @@
 # Copyright (c) Jupyter Development Team.
 # Distributed under the terms of the Modified BSD License.
 
+import socket
+
 import pytest
 from tornado import gen
 
@@ -24,13 +26,17 @@ def test_server(db):
 
     # test wrapper
     server = objects.Server(orm_server=server)
-    assert server.host == 'http://127.0.0.1:%i' % server.port
+    assert server.host == 'http://%s:%i' % (socket.gethostname(), server.port)
     assert server.url == server.host + '/'
     assert server.bind_url == 'http://*:%i/' % server.port
     server.ip = '127.0.0.1'
     assert server.host == 'http://127.0.0.1:%i' % server.port
     assert server.url == server.host + '/'
 
+    server.connect_ip = 'hub'
+    assert server.host == 'http://hub:%i' % server.port
+    assert server.url == server.host + '/'
+
 
 def test_user(db):
     user = User(orm.User(name='kaylee',


Code review comments:
diff_hunk: @@ -26,10 +27,22 @@ class Server(HasTraits):
     orm_server = Instance(orm.Server, allow_none=True)
 
     ip = Unicode()
+    connect_ip = Unicode()
     proto = Unicode('http')
     port = Integer()
     base_url = Unicode('/')
     cookie_name = Unicode('')
+    
+    @property
+    def _connect_ip(self):
+        """Property for connect_ip"""
+        if self.connect_ip:
+            return self.connect_ip
+        elif self.ip in {'', '0.0.0.0'}:
+            # if listening on all interfaces, default to hostname
path: jupyterhub/objects.py
text: @user1:
Why not just actually listen on all interfaces? 

@user1:
Binding to socketname has very different (and IMO unexpected) behavior from binding to 0.0.0.0.

```bash
python3 -m http.server --bind 0.0.0.0 8000
```

curl localhost:8000 works, curl courage:8000 works (courage is my hostname).

```
python3 -m http.server --bind courage 8000
```

curl localhost:8000 does *not* work, and curl courage:8000 does work. If we do this substitution by default you can never listen on localhost and public interfaces, even though you specified 0.0.0.0 which has that behavior - unintuitive and confusing.

If people want it to only listen on a public interface, they should be asked to explicitly specify that IMO.

@author:
Nothing here will bind to a hostname. I'll add some comments that clarify that `connect_ip` is only ever used for connecting, never binding. Nothing about binding is changed in this PR and users can still specify whatever they want for the bind address. This is only for "If bound on all interfaces, tell clients to connect via hostname unless another ip is specified for connections."

The reasoning behind switching from 127.0.0.1 to hostname when the bind address is all-interfaces:

The default for this case so far has been 127.0.0.1, which works if everything is local and doesn't work when clients are remote. If the bind address is explicitly changed to 0.0.0.0, it is probable that there are remote clients (otherwise, why choose it?). The result up to now is that that setting `ip = 0.0.0.0` *always requires* setting a different config value for the connect address explicitly as well (e.g. `DockerSpawner.hub_ip_connect`), depending on the Spawner. This default case is changed to `hostname`, which will work for remote and local clients in sensible networking conditions, and can always be overridden with `JupyterHub.hub_connect_ip`, just like DockerSpawner and KubeSpawner have been doing themselves.

@user1:
Aah, whoops. +1