stepchowfun / toast

Containerize your development and continuous integration environments. šŸ„‚
Other
1.57k stars 39 forks source link

PATH is overridden when using su #451

Open ivarref opened 1 year ago

ivarref commented 1 year ago

Description It seems that PATH is overridden when using su. This is not the case when using docker directly.

Instructions to reproduce the bug

$ cat toast.yml 
image: eclipse-temurin:11-jdk-focal
tasks:
  java_version:
    command: java -version

$ toast
[INFO] Ready to run 1 task: java_version.
[INFO] Running task java_versionā€¦
bash: java: command not found

Example correct output using docker directly:

$ docker run -t eclipse-temurin:11-jdk-focal java -version
openjdk version "11.0.17" 2022-10-18
OpenJDK Runtime Environment Temurin-11.0.17+8 (build 11.0.17+8)
OpenJDK 64-Bit Server VM Temurin-11.0.17+8 (build 11.0.17+8, mixed mode, sharing)

I suspect this is because su is invoked, and that the PATH environment variable is then overridden.

Environment information:

Is there any way to fix this?

Thanks for an interesting and good project!

stepchowfun commented 1 year ago

Hi @ivarref, thanks for starting this discussion. This is by design, but it's a questionable design. Toast uses su because that seems to be the only portable way to run a command as a specific user (which can be specified in the toastfile) with that user's preferred shell (Toast doesn't make any assumptions about which shells are available in the container). But, as you noticed, it has the unfortunate side effect of overwriting the PATH from the environment (confusingly, even when --preserve-environment is provided).

A simple workaround for your use case might be as follows:

image: eclipse-temurin:11-jdk-focal
command_prefix: export PATH="/opt/java/openjdk/bin:$PATH"
tasks:
  java_version:
    command: java -version

Update: What I previously wrote here was not quite right. I previously thought this was a general issue with environment variables provided by the Docker image, but now I see it only affects certain environment variables like PATH.

ivarref commented 1 year ago

Thanks for the quick reply @stepchowfun and suggested fix! I suppose logging in using su overrides the PATH variable (but inhertis others like you say).

Would it be possible to avoid using su if the desired user is already the default user? Does that make sense?

Thanks again.

stepchowfun commented 1 year ago

That's an interesting idea, but I have a couple of concerns:

1) Toast doesn't know the default USER from the base image (or any other information from the base image), and querying it could be expensive. In the Dockerfile, the USER can be specified as a username or a UID, so Toast would need to handle both possibilities (possibly by performing a lookup in the /etc/passwd file). To make this performant, we'd probably want to come up with some mechanism for caching the user <> UID mapping. 2) This would be an awkward special case that is difficult to explain: if the user matches the one specified in the Dockerfile, we run the command with /bin/sh (presumably?) and otherwise we use the user's preferred shell. It makes the reader immediately wonder why user's shell preference is ignored in the first case.

My current thinking is that it's not worth it. I understand how this feature would be useful, though.