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

[FAST_BUILD] Add support for Docker/Podman in rootless mode #2039

Merged
merged 5 commits into from
Jan 14, 2024

Conversation

benz0li
Copy link
Contributor

@benz0li benz0li commented Nov 22, 2023

Describe your changes

Add support for Docker/Podman in rootless mode.

Enables the container to run as root user, e.g.

podman run -p 8888:8888 -u root -e NB_USER=root -e NB_UID=0 -e NB_GID=0 quay.io/jupyter/scipy-notebook start-notebook.sh --allow-root

FYI @anil-resero

Issue ticket if applicable

Fixes #2036

Checklist (especially for first-time contributors)

  • I have performed a self-review of my code
  • If it is a core feature, I have added thorough tests
  • I will try not to use force-push to make the review process easier for reviewers
  • I have updated the documentation for significant changes

@benz0li
Copy link
Contributor Author

benz0li commented Nov 22, 2023

NB_UID=0 and NB_GID=0 are mandatory so that the UID/GID is correctly mapped to the host's user.

Due to the additional cp option --no-preserve=ownership, a CHOWN_HOME is not required.

@benz0li
Copy link
Contributor Author

benz0li commented Nov 22, 2023

And yes, the root user's home directory is set to /home/root.

IMHO we do not want to use (and touch) the original /root directory.

@mathbunnyru mathbunnyru changed the title Add support for Docker/Podman in rootless mode [FAST_BUILD] Add support for Docker/Podman in rootless mode Nov 22, 2023
@mathbunnyru
Copy link
Member

Added [FAST_BUILD] to the PR name, to make the build significantly faster.
More info about it: https://github.com/jupyter/docker-stacks/blob/main/.github/workflows/docker.yml#L3-L6

@manics
Copy link
Contributor

manics commented Nov 22, 2023

IMHO we do not want to use (and touch) the original /root directory.

What's your thinking behind not touching /root, but modifying the root user? What happens if you create multiple users with the same UID?

root@8db194d8497c:/# useradd -m test -u0 --non-unique
root@8db194d8497c:/# su - test
# pwd
/home/test
# id
uid=0(root) gid=1000(test) groups=1000(test)

@benz0li
Copy link
Contributor Author

benz0li commented Nov 22, 2023

What's your thinking behind not touching /root,

  1. The root user's home directory (/root) is pre-populated.
  2. A NB_USER's1 home directory only gets populated if inexistent.

See

if [[ "${NB_USER}" != "jovyan" ]]; then
if [[ ! -e "/home/${NB_USER}" ]]; then

But also with regard to #1478, which may be implemented in the future.

but modifying the root user?

The root user's [new] home directory2 should be treated like any other user's home directory.

What happens if you create multiple users with the same UID?

root@8db194d8497c:/# useradd -m test -u0 --non-unique
root@8db194d8497c:/# su - test
# pwd
/home/test
# id
uid=0(root) gid=1000(test) groups=1000(test)

This PR is meant for one use case only: Starting the container with options -u root -e NB_USER=root -e NB_UID=0 -e NB_GID=0 together with start-notebook.sh --allow-root using Docker/Podman in rootless mode.

@manics I do not understand exactly what you are trying to demonstrate. But regarding the container startup there is

    # Update the home directory if the desired user (NB_USER) is root and the
    # desired user id (NB_UID) is 0 and the desired group id (NB_GID) is 0.
    if [ "${NB_USER}" = "root" ] && [ "${NB_UID}" = "$(id -u "${NB_USER}")" ] && [ "${NB_GID}" = "$(id -g "${NB_USER}")" ]; then
        sed -i "s|/root|/home/root|g" /etc/passwd
        # Do not preserve ownership in rootless mode
        CP_OPTS="-a --no-preserve=ownership"
    fi

in place.

Footnotes

  1. if not equal to jovyan

  2. for the Jupyter Docker Stacks

@benz0li
Copy link
Contributor Author

benz0li commented Nov 22, 2023

The triplet -e NB_USER=root -e NB_UID=0 -e NB_GID=0 is strictly required for this use case.

Otherwise you run into

if [ "${NB_UID}" != "$(id -u "${NB_USER}")" ] || [ "${NB_GID}" != "$(id -g "${NB_USER}")" ]; then

followed by

userdel "${NB_USER}"
useradd --no-log-init --home "/home/${NB_USER}" --shell /bin/bash --uid "${NB_UID}" --gid "${NB_GID}" --groups 100 "${NB_USER}"
which is supposed to fail – and does at L78 because there is no --non-unique.

👉 You cannot remove the user the container is running with (-u root).

@mathbunnyru
Copy link
Member

@manics may I ask you to review this, please?

@manics
Copy link
Contributor

manics commented Dec 8, 2023

It looks like this solves one instance of the problem of running a rootless container without dealing with different subuids.

I'm still not convinced changing root's home directory to /home/root is the right approach though. It's true that /root may be pre-populated, but so is but since this is the container it's unlikely that there's anything important in there. There's already a well known distinction between root's homedir and standard homedirs, so given that there's going to be confusion regardless of which option is chosen I think sticking with the Linux default is clearer.

Having said that there's actually two different issues being discussed here:

  • How should NB_USER=root NB_UID=0 behave?
  • How should NB_USER=jovyan NB_UID=0 (or any other NB_USER) behave? For example, one way to solve the rootless problem is to allow jovyan to have NB_UID=0 which requires useradd --non-unique. This isn't perfect since a terminal still shows the username as root but USER=jovyan and HOME=/home/jovyan are correct.

I think it'd be nice to solve the more general problem for rootless running and allow NB_UID=0 for any NB_USER. The specific decision on whether root's home directory should be changed for NB_USER=root is something I'm happy for @mathbunnyru to decide since it's a matter of opinion.

Edit: If root's home directory is automatically changed then I think this needs to be documented somewhere.

@mathbunnyru
Copy link
Member

mathbunnyru commented Jan 10, 2024

The specific decision on whether root's home directory should be changed for NB_USER=root is something I'm happy for @mathbunnyru to decide since it's a matter of opinion.

I checked, and current images already run the command above and rename /root->/home/root/, so it's been like this for a long time.
So, we might not want to break existing users.
The current PR fixes /etc/passwd, this is the important part, if I get it right.

I also think having /home/root is a bit easier for use cases where JupyterHub instances have both NB_USER as root and as something else (they won't need to have an if-case).

So, let's keep this option for now.

Edit: If root's home directory is automatically changed then I think this needs to be documented somewhere.

I do agree here, this is not obvious.
@benz0li could you please describe it here?
https://github.com/jupyter/docker-stacks/blob/main/docs/using/common.md#user-related-configurations

If you document it, please also add Close: https://github.com/jupyter/docker-stacks/issues/2042 to the issue description, so we fix that issue as well (by documenting current behavior).

If you can also add a test case, that the triplet in the issue runs without an error, that would be nice (I do not insist though).

@mathbunnyru
Copy link
Member

Merging this one. I will document the current behaviour in a separate PR.

@mathbunnyru mathbunnyru merged commit 37018f9 into jupyter:main Jan 14, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running jupyterlab as root instead of jovyan when running in rootless docker mode
3 participants