From b8c0ca450c03dd04be3cbbeb23dce91d40fec36e Mon Sep 17 00:00:00 2001 From: Min RK Date: Fri, 17 Jul 2020 14:17:25 +0200 Subject: [PATCH] use . to separate username and servername in object names separator must not be a safe character or the escape character to avoid collisions '.' is not in the safe char list, but is a valid docker identifier - servername is template variable is escaped, matching username - add raw_servername for the raw value - fix safe_username to be the safe version --- .travis.yml | 1 + dockerspawner/dockerspawner.py | 20 ++++++++++++++------ tests/conftest.py | 18 ++++++++++++++---- tests/test_dockerspawner.py | 30 ++++++++++++++++++++++++------ tests/test_swarmspawner.py | 17 +++++++++-------- tests/test_systemuserspawner.py | 3 +-- 6 files changed, 63 insertions(+), 26 deletions(-) diff --git a/.travis.yml b/.travis.yml index 0b23e602..589cbbd5 100644 --- a/.travis.yml +++ b/.travis.yml @@ -11,6 +11,7 @@ before_install: - docker swarm init - nvm install 10; nvm use 10 - npm install -g configurable-http-proxy + - pip install --upgrade setuptools pip - pip install --upgrade -r dev-requirements.txt jupyterhub==${JUPYTERHUB:-0}.* install: diff --git a/dockerspawner/dockerspawner.py b/dockerspawner/dockerspawner.py index 73d6dbbc..b4989401 100644 --- a/dockerspawner/dockerspawner.py +++ b/dockerspawner/dockerspawner.py @@ -326,8 +326,13 @@ def options_from_form(self, formdata): It is important to include {servername} if JupyterHub's "named servers" are enabled (JupyterHub.allow_named_servers = True). If the server is named, the default name_template is - "{prefix}-{username}-{servername}". If it is unnamed, the default + "{prefix}-{username}.{servername}". If it is unnamed, the default name_template is "{prefix}-{username}". + + Note: when using named servers, + it is important that the separator between {username} and {servername} + is not a character that can occur in an escaped {username}, + and also not the single escape character '_'. """ ), ) @@ -335,7 +340,7 @@ def options_from_form(self, formdata): @default('name_template') def _default_name_template(self): if self.name: - return "{prefix}-{username}-{servername}" + return "{prefix}-{username}.{servername}" else: return "{prefix}-{username}" @@ -728,17 +733,20 @@ def _escape(self, s): def template_namespace(self): escaped_image = self.image.replace("/", "_") server_name = getattr(self, "name", "") + safe_server_name = self._escape(server_name.lower()) return { "username": self.escaped_name, - "safe_username": self.user.name, + "safe_username": self.escaped_name, "raw_username": self.user.name, "imagename": escaped_image, - "servername": server_name, + "servername": safe_server_name, + "raw_servername": server_name, "prefix": self.prefix, } - @property - def object_name(self): + object_name = Unicode() + @default("object_name") + def _object_name_default(self): """Render the name of our container/service using name_template""" return self.name_template.format(**self.template_namespace()) diff --git a/tests/conftest.py b/tests/conftest.py index e96ec10e..ce87ac83 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -17,15 +17,25 @@ @pytest.fixture -def dockerspawner_configured_app(app): +def named_servers(app): + with mock.patch.dict( + app.tornado_settings, + {"allow_named_servers": True, "named_server_limit_per_user": 2}, + ): + yield + + +@pytest.fixture +def dockerspawner_configured_app(app, named_servers): """Configure JupyterHub to use DockerSpawner""" app.config.DockerSpawner.prefix = "dockerspawner-test" # app.config.DockerSpawner.remove = True with mock.patch.dict(app.tornado_settings, {"spawner_class": DockerSpawner}): yield app + @pytest.fixture -def swarmspawner_configured_app(app): +def swarmspawner_configured_app(app, named_servers): """Configure JupyterHub to use DockerSpawner""" app.config.SwarmSpawner.prefix = "dockerspawner-test" with mock.patch.dict( @@ -37,9 +47,9 @@ def swarmspawner_configured_app(app): @pytest.fixture -def systemuserspawner_configured_app(app): +def systemuserspawner_configured_app(app, named_servers): """Configure JupyterHub to use DockerSpawner""" - app.config.SwarmSpawner.prefix = "dockerspawner-test" + app.config.DockerSpawner.prefix = "dockerspawner-test" with mock.patch.dict( app.tornado_settings, {"spawner_class": SystemUserSpawner} ): diff --git a/tests/test_dockerspawner.py b/tests/test_dockerspawner.py index dbc90708..7f44ce58 100644 --- a/tests/test_dockerspawner.py +++ b/tests/test_dockerspawner.py @@ -1,35 +1,53 @@ """Tests for DockerSpawner class""" +import asyncio import json -import asyncio import docker import pytest from jupyterhub.tests.test_api import add_user, api_request from jupyterhub.tests.mocking import public_url from jupyterhub.utils import url_path_join -from tornado.httpclient import AsyncHTTPClient, HTTPRequest +from tornado.httpclient import AsyncHTTPClient from dockerspawner import DockerSpawner # Mark all tests in this file as asyncio pytestmark = pytest.mark.asyncio +def test_name_collision(dockerspawner_configured_app): + app = dockerspawner_configured_app + has_hyphen = "user--foo" + add_user(app.db, app, name=has_hyphen) + user = app.users[has_hyphen] + spawner1 = user.spawners[""] + assert isinstance(spawner1, DockerSpawner) + assert spawner1.object_name == "{}-{}".format(spawner1.prefix, has_hyphen) + + part1, part2 = ["user", "foo"] + add_user(app.db, app, name=part1) + user2 = app.users[part1] + spawner2 = user2.spawners[part2] + assert spawner1.object_name != spawner2.object_name + + async def test_start_stop(dockerspawner_configured_app): app = dockerspawner_configured_app name = "has@" add_user(app.db, app, name=name) user = app.users[name] - assert isinstance(user.spawner, DockerSpawner) + server_name = 'also-has@' + spawner = user.spawners[server_name] + assert isinstance(spawner, DockerSpawner) token = user.new_api_token() # start the server - r = await api_request(app, "users", name, "server", method="post") + r = await api_request(app, "users", name, "servers", server_name, method="post") while r.status_code == 202: # request again - r = await api_request(app, "users", name, "server", method="post") + r = await api_request(app, "users", name, "servers", server_name, method="post") assert r.status_code == 201, r.text - url = url_path_join(public_url(app, user), "api/status") + url = url_path_join(public_url(app, user), server_name, "api/status") resp = await AsyncHTTPClient().fetch(url, headers={"Authorization": "token %s" % token}) assert resp.effective_url == url resp.rethrow() diff --git a/tests/test_swarmspawner.py b/tests/test_swarmspawner.py index da49572d..4a375a3e 100644 --- a/tests/test_swarmspawner.py +++ b/tests/test_swarmspawner.py @@ -1,34 +1,35 @@ """Tests for SwarmSpawner""" -from unittest import mock - import pytest from jupyterhub.tests.test_api import add_user, api_request from jupyterhub.tests.mocking import public_url from jupyterhub.utils import url_path_join -from tornado.httpclient import AsyncHTTPClient, HTTPRequest +from tornado.httpclient import AsyncHTTPClient from dockerspawner import SwarmSpawner # Mark all tests in this file as asyncio pytestmark = pytest.mark.asyncio + async def test_start_stop(swarmspawner_configured_app): app = swarmspawner_configured_app name = "somebody" add_user(app.db, app, name=name) user = app.users[name] - assert isinstance(user.spawner, SwarmSpawner) + server_name = 'also-has@' + spawner = user.spawners[server_name] + assert isinstance(spawner, SwarmSpawner) token = user.new_api_token() # start the server - r = await api_request(app, "users", name, "server", method="post") + r = await api_request(app, "users", name, "servers", server_name, method="post") while r.status_code == 202: # request again - r = await api_request(app, "users", name, "server", method="post") + r = await api_request(app, "users", name, "servers", server_name, method="post") assert r.status_code == 201, r.text - url = url_path_join(public_url(app, user), "api/status") + url = url_path_join(public_url(app, user), server_name, "api/status") resp = await AsyncHTTPClient().fetch(url, headers={"Authorization": "token %s" % token}) assert resp.effective_url == url resp.rethrow() - assert "kernels" in resp.body.decode("utf-8") \ No newline at end of file + assert "kernels" in resp.body.decode("utf-8") diff --git a/tests/test_systemuserspawner.py b/tests/test_systemuserspawner.py index bfc49662..336f5a8b 100644 --- a/tests/test_systemuserspawner.py +++ b/tests/test_systemuserspawner.py @@ -1,13 +1,12 @@ """Tests for SwarmSpawner""" from getpass import getuser -from unittest import mock import pytest from jupyterhub.tests.test_api import add_user, api_request from jupyterhub.tests.mocking import public_url from jupyterhub.utils import url_path_join -from tornado.httpclient import AsyncHTTPClient, HTTPRequest +from tornado.httpclient import AsyncHTTPClient from dockerspawner import SystemUserSpawner