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

Added support for JUPYTER_TOKEN_FILE #5587

Merged
merged 3 commits into from
Jul 23, 2020

Conversation

StromFLIX
Copy link
Contributor

Motivation

Currently, we use JUPYTER_TOKEN as an environment variable to pass a token to the jupyter notebook with which we can authenticate us later. This is a good practice, for a local environment, but in a docker environment the best practice is to use docker secrets. Docker secrets uses instead of a string directly in the env variable, a file location where the string is stored. This file is stored in memory and only accessible by the container that has the correct access right.

Changes

Added an env variable JUPYTER_TOKEN_FILE, if it is available it will read the token from the given location and return it. Priority has JUPYTER_TOKEN.

Further information

Docker-compose with docker secret, best practices: https://docs.docker.com/engine/swarm/secrets/#build-support-for-docker-secrets-into-your-images

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good and address a valid use case - one we're seeing more of these days with containerization. Thank you!

@kevin-bates
Copy link
Member

Hi @nemesisflx - I'm wondering if we should use this opportunity to extend the help test to reference both JUPYTER_TOKEN (for by-value) and JUPYTER_TOKEN_FILE (for by-reference) support options via the env variables? Otherwise, it seems folks would need to peruse the code to make that determination.

@kevin-bates kevin-bates mentioned this pull request Jul 22, 2020
24 tasks
@StromFLIX
Copy link
Contributor Author

StromFLIX commented Jul 22, 2020

@kevin-bates I am unsure what you mean by your request. Both JUPYTER_TOKEN and JUPYTER_TOKEN_FILE can still be used and JUPYTER_TOKEN has the priority. I'll try my best to add your request, but I am unsure of what to do? I don't see when people need to look inside the code to find something out. Should I write documentation for it?

@kevin-bates
Copy link
Member

I'm referring to the help text relative to the token attribute the resides just above your change: https://github.com/jupyter/notebook/pull/5587/files#diff-bbd307804bb6b8836da9f06b8d8a06e9R931-R939

I think it would be helpful to note that token can be effectively set via (either) env variable and what the differences are between the two envs (one is the token value, the other is a filename containing the token value (i.e., "by reference")).

@StromFLIX
Copy link
Contributor Author

@kevin-bates I updated the description. What do you think?

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good - thanks for adding the help text.

@kevin-bates kevin-bates merged commit f5d6995 into jupyter:master Jul 23, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants