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

Fix USER switching in Docker #1199

Closed
wants to merge 1 commit into from
Closed

Conversation

kunalkapoor
Copy link

Refers #1198.

In attempting to switch to the daemonUser we are instead switching to the hardcoded uid 1001. If the daemon user exists with a different uid then the docker container default user is incorrect. Correcting the makeUser function to switch to the username specified by daemonUser instead of the static uid.

Refers sbt#1198.

In attempting to switch to the `daemonUser` we are instead switching to the hardcoded uid 1001. If the daemon user exists with a different uid then the docker container default user is incorrect. Correcting the `makeUser` function to switch to the username specified by `daemonUser` instead of the static uid.
@muuki88 muuki88 requested a review from eed3si9n February 8, 2019 13:51
@muuki88
Copy link
Contributor

muuki88 commented Feb 8, 2019

cc @jw3

@muuki88
Copy link
Contributor

muuki88 commented Feb 8, 2019

The docker-filepermissions test failed:
https://travis-ci.org/sbt/sbt-native-packager/jobs/490338861#L1258

can you have a look? If you need help running the tests take a look at the CONTRIBUTING.MD

@jw3
Copy link

jw3 commented Feb 8, 2019

Sounds like an issue in the category of working on OpenShift vs working in other places. OpenShift doesn't need the user to exist at all, but that doesn't usually play well elsewhere.

@kunalkapoor a reference to a public Docker image that demonstrates the issue out of the box would help.

@eed3si9n
Copy link
Member

eed3si9n commented Feb 8, 2019

I used numeric UID in #1190 based on the OKD Creating Images Guidelines. (OKD, The Origin Community Distribution, is an open source fork of Kubernetes by Red Hat, which powers OpenShift):

Lastly, the final USER declaration in the Dockerfile should specify the user ID (numeric value) and not the user name.
...
If your S2I image does not include a USER declaration with a numeric user, your builds will fail by default.

Having said that, I think the issue brought up by @kunalkapoor in #1198 is totally fair, and I don't think non-OpenShift users would have to suffer from any drawbacks. I continue to seek for the middle ground where the Docker image is friendly to OpenShift by default.

Also, it seems the numeric USER requirement is limited to S2I (Source-To-Image), which is used by subset of OpenShift users, so perhaps this could be an opt-in feature. Also to avoid possible conflict with base image maybe we should consider changing from "daemon" to something more unique like "demiourgos".

@kunalkapoor
Copy link
Author

From what I gather, this issue is unrelated to the docker image used. You can try to reproduce this using any docker image as long as you have a "daemon" user already created in that image with a uid that is not 1001. Or else, we could use any random image and add docker commands to create the "daemon" user before sbt-native-packager docker commands are added.

An example excerpt of a build.sbt:

val userCreationCommands = Seq(Cmd("useradd --system --create-home --uid 6 --gid 0 daemon"))
dockerCommands := userCreationCommands ++ dockerCommands

Will result in a Dockerfile like below:

# The custom command to create the daemon user
RUN useradd --system --create-home --uid 1001 --gid 0 daemon

# The commands added by sbt-native-packager
RUN id -u daemon || useradd --system --create-home --uid 1001 --gid 0 daemon
USER 1001

The above results in a daemon user with uid 6 being created but it switches to uid 1001 which doesn't exist in the system.

If we must switch to uid for OpenSwitch users, then can we dynamically determine the uid of the daemon user and use that instead if it already exists? I'm not sure if that's possible. But an alternative might be to expose the uid as a configurable parameter (just like daemonUser).

@eed3si9n
Copy link
Member

eed3si9n commented Feb 8, 2019

There's a setting called daemonUserUid. I will send a PR to use that.

@eed3si9n
Copy link
Member

eed3si9n commented Feb 8, 2019

Here's my proposal - #1200

@kunalkapoor
Copy link
Author

Closing this as it is not the correct solution. #1200 seems better suited to fix the problem.

@kunalkapoor kunalkapoor closed this Feb 8, 2019
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.

4 participants