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 format_volume_name extension point #124

Merged
merged 8 commits into from Nov 1, 2016

Conversation

Projects
None yet
2 participants
@namdets
Copy link

namdets commented Oct 18, 2016

While working with third party oauth providers for authentication(using oauthenticator's Google example), I ran into an issue where the Google oauth provider returned an email as the username. When naming the docker container the username was being escaped, but when naming the volume it was not. This was causing Docker to explode resulting in the following stack trace:

[I 2016-10-18 15:26:42.667 JupyterHub dockerspawner:332] Container 'jupyter-user_40email_2Ecom' is gone
[W 2016-10-18 15:26:42.668 JupyterHub dockerspawner:303] container not found
[E 2016-10-18 15:26:42.699 JupyterHub web:1524] Uncaught exception GET /hub/user/user@email.com (x.x.x.x)
HTTPServerRequest(protocol='https', host='localhost', method='GET', uri='/hub/user/user@email.com', version='HTTP/1.1', remote_ip='x.x.x.x', headers={'Host': 'localhost', 'Referer': 'https://localhost/hub/home', 'User-Agent': 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/52.0.2743.116 Chrome/52.0.2743.116 Safari/537.36', 'Accept': 'text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,/;q=0.8', 'Upgrade-Insecure-Requests': '1', 'Cookie': 'jupyter-hub-token="2|1:0|10:1476801982|17:jupyter-hub-token|44:OWIwODc1YzhhN2Y4NDJjN2E2NzQ1NDFjMWE4YzRiMmQ=|d343f8c1ab24a119ab45a29753bf2b2fe36d390dbd5970ef5ba836dd9fe19c79"', 'X-Forwarded-Port': '443', 'X-Forwarded-Proto': 'https', 'X-Forwarded-For': 'x.x.x.x', 'Connection': 'close', 'Accept-Language': 'en-US,en;q=0.8', 'Accept-Encoding': 'gzip, deflate, sdch, br'})
Traceback (most recent call last):
File "/opt/conda/lib/python3.5/site-packages/tornado/web.py", line 1445, in _execute
result = yield result
File "/opt/conda/lib/python3.5/site-packages/jupyterhub/handlers/base.py", line 494, in get
yield self.spawn_single_user(current_user)
File "/opt/conda/lib/python3.5/site-packages/jupyterhub/handlers/base.py", line 312, in spawn_single_user
yield gen.with_timeout(timedelta(seconds=self.slow_spawn_timeout), f)
File "/opt/conda/lib/python3.5/site-packages/jupyterhub/user.py", line 247, in spawn
raise e
File "/opt/conda/lib/python3.5/site-packages/jupyterhub/user.py", line 228, in spawn
yield gen.with_timeout(timedelta(seconds=spawner.start_timeout), f)
File "/srv/jupyterhub/dockerspawner/dockerspawner/dockerspawner.py", line 391, in start
resp = yield self.docker('create_container', *_create_kwargs)
File "/opt/conda/lib/python3.5/concurrent/futures/_base.py", line 398, in result
return self.__get_result()
File "/opt/conda/lib/python3.5/concurrent/futures/_base.py", line 357, in __get_result
raise self._exception
File "/opt/conda/lib/python3.5/concurrent/futures/thread.py", line 55, in run
result = self.fn(_self.args, *_self.kwargs)
File "/srv/jupyterhub/dockerspawner/dockerspawner/dockerspawner.py", line 289, in _docker
return m(_args, **kwargs)
File "/opt/conda/lib/python3.5/site-packages/docker/api/container.py", line 135, in create_container
return self.create_container_from_config(config, name)
File "/opt/conda/lib/python3.5/site-packages/docker/api/container.py", line 146, in create_container_from_config
return self._result(res, True)
File "/opt/conda/lib/python3.5/site-packages/docker/client.py", line 178, in _result
self._raise_for_status(response)
File "/opt/conda/lib/python3.5/site-packages/docker/client.py", line 174, in raise_for_status
raise errors.APIError(e, response, explanation=explanation)
docker.errors.APIError: 500 Server Error: Internal Server Error ("b'{"message":"create jupyterhub-user-user@email.com: "jupyterhub-user-user@email.com" includes invalid characters for a local volume name, only "[a-zA-Z0-9][a-zA-Z0-9
.-]" are allowed"}'")

[E 2016-10-18 15:26:42.792 JupyterHub log:99] {
"Host": "localhost",
"Referer": "https://localhost/hub/home",
"User-Agent": "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/52.0.2743.116 Chrome/52.0.2743.116 Safari/537.36",
"Accept": "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,/;q=0.8",
"Upgrade-Insecure-Requests": "1",
"Cookie": "jupyter-hub-token="2|1:0|10:1476801982|17:jupyter-hub-token|44:OWIwODc1YzhhN2Y4NDJjN2E2NzQ1NDFjMWE4YzRiMmQ=|d343f8c1ab24a119ab45a29753bf2b2fe36d390dbd5970ef5ba836dd9fe19c79"",
"X-Forwarded-Port": "443",
"X-Forwarded-Proto": "https",
"X-Forwarded-For": "x.x.x.x",
"Connection": "close",
"Accept-Language": "en-US,en;q=0.8",
"Accept-Encoding": "gzip, deflate, sdch, br"
}
[E 2016-10-18 15:26:42.793 JupyterHub log:100] 500 GET /hub/user/user@email.com (user@email.com@x.x.x.x) 411.14ms

The fix implemented was to call self.escaped_name instead of self.user.name in _volumes_to_binds, I'm new to the project, so please let me know if this is wrong. I did add a test case as well as test it in jupyterhub.

I don't think this will break any existing installations on upgrade as the volumes could not have been created before.

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Oct 24, 2016

Welcome! I think we need to be a little careful about what names we coerce and when. The change right now is coercing the path, as well as coercing local filesystem mounts, which we probably don't want to do. The escaping should only be applied to volume names, and not paths on the host or container.

I also think that error message might be a bit misleading, and telling you that @ is not allowed in a local mount point, which is not true. When you are mounting a local path, make sure it is always an absolute path (start with /). For instance, when I try:

mkdir a@b
docker run -v a@b:/tmp/a@b -it debian bash

I get the error you see:

docker: Error response from daemon: create a@b: "a@b" includes invalid characters for a local volume name, only "[a-zA-Z0-9][a-zA-Z0-9_.-]" are allowed.
See 'docker run --help'.

That looks like it's saying that I cannot mount directories with @ in them, but it's not. I didn't tell docker to mount the directory a@b, I told it to mount the docker-volume a@b, not the directory a@b. If I give it the full path, everything is okay:

mkdir a@b
docker run -v "$PWD/a@b:/tmp/a@b" -it debian bash
root@b115bd48d3bf:/# ls /tmp/a@b ls -la /tmp/a@b
# aok

So the error message really only means to limit volume names, not paths. We probably shouldn't be adding extra escapes to paths, be they in containers, volumes, or on hosts.

@namdets

This comment has been minimized.

Copy link

namdets commented Oct 25, 2016

This solution forces users of dockerspawner to use absolute paths for user container volumes instead of named volumes if their authentication provider uses emails for usernames.

I'm OK with this idea on the surface, but it might be desirable to some users to use volume plugins which require named volumes, not absolute paths.

Perhaps a better approach may be to build an extension point in to define volume naming strategy.

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Oct 27, 2016

An extension point is a great idea.

@namdets

This comment has been minimized.

Copy link

namdets commented Oct 27, 2016

I took a first stab at adding an extension point. To see it in action add this to jupyterhub_config.py :

class EscapedVolumeNamingStrategy(VolumeNamingStrategy):
    def name_volume(self, label_template, spawner):
        return label_template.format(username=spawner.escaped_name)
c.DockerSpawner.volume_naming_strategy_class = EscapedVolumeNamingStrategy

My Python is not strong, so I used the pattern provided by Jupyterhub for the authenticator_class
extension point.

I'd still like to add some tests as well as improve and add the EscapedVolumeNamingStrategy class
to the pull request, but figured it is more than far enough for some feedback.

Thanks!

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Oct 28, 2016

Nice! I'd have a slight preference for making the configurable any callable, rather than requiring that it be a class:

def default_format_volume(template, spawner):
    return template.format(username=spawner.user.name)

...
format_volume_name = Any(
    help='...'
).tag(config=True)

@default('format_volume_name')
def _get_default_format_volume(self):
    return default_format_volume

How does that sound?

@namdets

This comment has been minimized.

Copy link

namdets commented Oct 28, 2016

Maybe this is more what you had in mind?

I like keeping the naming strategies in a separate file to encourage future implementations to be contributed back.

Also, I would still like to build in some tests for the extension config itself as well as each strategy.

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Oct 31, 2016

@namdets great! Would you like to hold this PR until you add the tests, or do it in another PR?

label).
If format_volume_name is not set,
default_format_volume_name is used for naming volumes.

This comment has been minimized.

@minrk

minrk Oct 31, 2016

Member

looks like docstring indentation is off here and below.

@namdets

This comment has been minimized.

Copy link

namdets commented Oct 31, 2016

I should be able to get the tests and cleanup done today, same pull request is perfect. I'll try to fix up the docstring too.

Jason Stedman

@minrk minrk changed the title Allow username to contain illegal volume characters like @ Add format_volume_name extension point Nov 1, 2016

@minrk minrk merged commit 7455d93 into jupyterhub:master Nov 1, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@minrk

This comment has been minimized.

Copy link
Member

minrk commented Nov 1, 2016

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment