Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-43590

docker.inside() breaks container environment.

    • Icon: Bug Bug
    • Resolution: Unresolved
    • Icon: Major Major
    • docker-workflow-plugin
    • None
    • docker-workflow 1.10

      We have PATH set on our build slaves (via configure -> Node properties -> Environment Variables) that won't work in most docker containers.  Specifically, we use /usr/local/bin:/a/bin.  Note how /usr/bin and /bin aren't in the PATH (which is where cat usually is located.

      Normally, the docker.inside() command's docker run isn't setting the -e PATH=... flag.  But every so often it uses the PATH from the build slave configuration which causes the docker run to not find cat

       

      docker: Error response from daemon: oci runtime error: container_linux.go:247: starting container process caused "exec: \"cat\": executable file not found in $PATH".

       

      I was able to track this down by modifying docker-workflow to not mask the PATH: https://github.com/jenkinsci/docker-workflow-plugin/pull/96

      I don't know what the expected behavior (overriding the PATH on a container seems like a "Bad Idea", since the container may-or-may-not be in LSB/POSIX layout) is supposed to be.

      It is clear that this random flipping between behaviors is bad.

          [JENKINS-43590] docker.inside() breaks container environment.

          Christian Höltje added a comment - - edited

           

          I found the code at https://github.com/jenkinsci/docker-workflow-plugin/blob/master/src/main/java/org/jenkinsci/plugins/docker/workflow/WithContainerStep.java#L126-L128

          EnvVars envReduced = new EnvVars(env);
          EnvVars envHost = computer.getEnvironment();
          envReduced.entrySet().removeAll(envHost.entrySet());

           

          I'm not sure that .removeAll is working or maybe the computer.getEnvironment() isn't working correctly?

          Christian Höltje added a comment - - edited   I found the code at  https://github.com/jenkinsci/docker-workflow-plugin/blob/master/src/main/java/org/jenkinsci/plugins/docker/workflow/WithContainerStep.java#L126-L128 EnvVars envReduced = new EnvVars(env); EnvVars envHost = computer.getEnvironment(); envReduced.entrySet().removeAll(envHost.entrySet());   I'm not sure that .removeAll is working or maybe the computer.getEnvironment() isn't working correctly?

          Given that the PATH is very dependent on the container (e.g. the container may not conform to LSB or POSIX layout) I think it should always be unset from the envReduced environment.

          In addition, probably some other environment variables should be black listed as well, such as LD_LIBRARY_PATH and friends.

          Christian Höltje added a comment - Given that the PATH is very dependent on the container (e.g. the container may not conform to LSB or POSIX layout) I think it should always be unset from the envReduced environment. In addition, probably some other environment variables should be black listed as well, such as LD_LIBRARY_PATH and friends.

          Christian Höltje added a comment - - edited

          I made a custom build of docker-workflow which blacklists PATH (and a bunch of the LD_) environment variables:

          https://github.com/jenkinsci/docker-workflow-plugin/pull/97

          This fixed my issues, but I still don't understand how the code mentioned in my initial comment could fail randomly.

          Christian Höltje added a comment - - edited I made a custom build of docker-workflow which blacklists PATH (and a bunch of the LD_ ) environment variables: https://github.com/jenkinsci/docker-workflow-plugin/pull/97 This fixed my issues, but I still don't understand how the code mentioned in my initial comment could fail randomly.

           

          When trying to use the latest code in master there were similar problems with the docker exec calls made for sh() as well.

          https://github.com/jenkinsci/docker-workflow-plugin/pull/91

          Christian Höltje added a comment -   When trying to use the latest code in master there were similar problems with the docker exec  calls made for sh() as well. https://github.com/jenkinsci/docker-workflow-plugin/pull/91

          Jesse Glick added a comment -

          What similar problems?

          Jesse Glick added a comment - What similar problems?

          The PATH is set to the value from the host, not the container, during a docker exec (e.g. a sh step).

          This breaks, for example, the golang container, which adds /go/bin to the front of the path (which a normal host won't have).

          I'm beginning to think that the way docker.inside works should be changed:

          1. For normal Jenkinsfile}}s the docker run -{{e flag should only be used to set the Jenkins default environment variables and nothing else. e.g. BUILD_NUMBER, WORKSPACE, etc.
          2. The only environment variables passed to docker exec are new variables set on env within a docker.inside block. e.g. sh 'env'}}should return only the container's environment variables + the Jenkins default environment variables.  It should not include {{env.FOO = 'bar' if that was run outside the docker.inside block.
          3. For declarative pipelines, it should follow the above rules.

          Christian Höltje added a comment - The PATH is set to the value from the host, not the container, during a docker exec (e.g. a sh step). This breaks, for example, the golang container, which adds /go/bin to the front of the path (which a normal host won't have). I'm beginning to think that the way docker.inside works should be changed: For normal Jenkinsfile}}s the docker run -{{e flag should only be used to set the Jenkins default environment variables and nothing else. e.g. BUILD_NUMBER , WORKSPACE , etc. The only environment variables passed to docker exec are new variables set on env  within a docker.inside block. e.g. sh 'env'}}should return only the container's environment variables + the Jenkins default environment variables.  It should not include {{env.FOO = 'bar' if that was run outside the docker.inside block. For declarative pipelines, it should follow the above rules.

          Jesse Glick added a comment -

          Everything about inside should be changed. I am inclined to simply deprecate the whole feature, and possibly the whole plugin. You should run docker CLI commands you need directly. What we do with Declarative is an open question. Probably the agent docker system needs to be redesigned.

          Perhaps this was not clear from the documentation, but inside is not designed for use with images such as golang. You must use a different idiom. inside works fine for its designed purpose: a totally passive image which merely serves to preinstall some tools and libraries.

          Jesse Glick added a comment - Everything about inside should be changed. I am inclined to simply deprecate the whole feature, and possibly the whole plugin. You should run docker CLI commands you need directly. What we do with Declarative is an open question. Probably the agent docker system needs to be redesigned. Perhaps this was not clear from the documentation, but inside is not designed for use with images such as golang . You must use a different idiom. inside works fine for its designed purpose: a totally passive image which merely serves to preinstall some tools and libraries.

          I agree that inside should be redesigned.  There is a lot that isn't ideal about it (user management, etc.).

          I'm not sure why inside is inappropriate for running golang, it's just another image used to build/test code.

          Either way, when 1.11 was released we accidentally upgraded to it (without my PR) and the error about 'cat' not being in the $PATH is back.

          Can we accept PR #97 as a work-around until inside is redesigned, jglick?

          Christian Höltje added a comment - I agree that inside should be redesigned.  There is a lot that isn't ideal about it (user management, etc.). I'm not sure why inside is inappropriate for running golang , it's just another image used to build/test code. Either way, when 1.11 was released we accidentally upgraded to it (without my PR) and the error about 'cat' not being in the $PATH is back. Can we accept PR #97  as a work-around until inside is redesigned, jglick ?

          Jesse Glick added a comment -

          golang has a special usage mode IIRC, with a dedicated source directory.

          Jesse Glick added a comment - golang has a special usage mode IIRC, with a dedicated source directory.

          Christian Höltje added a comment - - edited

          It doesn't have to work that way, you can build your source from an arbitrary directory.  Go just prefers it in /go/src/...

          The real problem is that when you do go install <some-tool> it installs it to /go/bin, but /go/bin isn't in the $PATH because it is overridden by the build slave host's $PATH.

          golang works as well as anything else does with dockerImg.inside()

          Christian Höltje added a comment - - edited It doesn't have to work that way, you can build your source from an arbitrary directory.  Go just prefers it in /go/src/... The real problem is that when you do go install <some-tool>  it installs it to /go/bin , but /go/bin isn't in the $PATH because it is overridden by the build slave host's $PATH . golang works as well as anything else does with dockerImg.inside()

          Jesse Glick added a comment -

          Has been a long time since I tried it but I recall problems with retrieving packages, which went to a fixed location. Anyway golang has a documented usage mode which it would be better to use as is rather than relying on WithContainerStep.

          Jesse Glick added a comment - Has been a long time since I tried it but I recall problems with retrieving packages, which went to a fixed location. Anyway golang has a documented usage mode which it would be better to use as is rather than relying on WithContainerStep .

          Fair enough. Still doesn't address the fact that $PATH (and some other variables, like LOCALE and LANG) shouldn't be overridden inside dockerImg.inside().

          Christian Höltje added a comment - Fair enough. Still doesn't address the fact that $PATH  (and some other variables, like LOCALE and LANG ) shouldn't be overridden inside dockerImg.inside() .

          Jesse Glick added a comment -

          Sure, this sounds like a bug offhand. Just not sure what is going to be broken by a fix. Generally every time I do anything relating to environment variables in Jenkins, the regressions start piling up!

          Jesse Glick added a comment - Sure, this sounds like a bug offhand. Just not sure what is going to be broken by a fix. Generally every time I do anything relating to environment variables in Jenkins, the regressions start piling up!

          Well, if you want to just deliberately break every thing then:

          1. Use no environmental variables for run cat.

          2. Have a new object created inside the block that is similar to env. Maybe call it insideEnv.

          3. Use insideEnv for the docker exec.

          This gives the ability to explicitly modify the environment variables from the pipeline.

          Maybe an optional flag to inside to prepopulate it with the Jenkins build vars,if that can be done reliably.

          Christian Höltje added a comment - Well, if you want to just deliberately break every thing then: 1. Use no environmental variables for run cat. 2. Have a new object created inside the block that is similar to env . Maybe call it insideEnv . 3. Use insideEnv for the docker exec. This gives the ability to explicitly modify the environment variables from the pipeline. Maybe an optional flag to inside to prepopulate it with the Jenkins build vars,if that can be done reliably.

          I think I run into the same issue while trying to override HOME env when using .inside() since updated to docker-workflow v1.9.1:

          docker.image('node:baseline-6.9.5').inside('-e HOME=/tmp') { [...]
          
          08:39:41 $ docker run -t -d -u 1001:1001 -e HOME=/tmp [...] -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** --entrypoint cat node:baseline-6.9.5
          [Pipeline] \{
          [Pipeline] sh
          08:39:42 + env
          08:39:42 JENKINS_HOME=/apps/jenkins/latest
          08:39:42 HOME=/apps/jenkins/latest
          

          Jonathan Amiez added a comment - I think I run into the same issue while trying to override HOME env when using .inside() since updated to docker-workflow v1.9.1: docker.image( 'node:baseline-6.9.5' ).inside( '-e HOME=/tmp' ) { [...] 08:39:41 $ docker run -t -d -u 1001:1001 -e HOME=/tmp [...] -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** --entrypoint cat node:baseline-6.9.5 [Pipeline] \{ [Pipeline] sh 08:39:42 + env 08:39:42 JENKINS_HOME=/apps/jenkins/latest 08:39:42 HOME=/apps/jenkins/latest

          Yeah, there are a bunch of environment variables that can potentially break a container.  And you can't "pre-determine" the list of environment variables that may break a container before hand. 

          Christian Höltje added a comment - Yeah, there are a bunch of environment variables that can potentially break a container.  And you can't "pre-determine" the list of environment variables that may break a container before hand. 

          Christian Höltje added a comment - - edited

          jglick: At the very least, do you have any idea why this code isn't working?

          EnvVars envReduced = new EnvVars(env);
          EnvVars envHost = computer.getEnvironment();
          envReduced.entrySet().removeAll(envHost.entrySet());

          ...because in my case, I have PATH set on the agent/node/slave; if this code worked it would unset PATH which would at least let cat work again for me. 

          Christian Höltje added a comment - - edited jglick : At the very least, do you have any idea why this code isn't working? EnvVars envReduced = new EnvVars(env); EnvVars envHost = computer.getEnvironment(); envReduced.entrySet().removeAll(envHost.entrySet()); ...because in my case, I have PATH set on the agent/node/slave; if this code worked it would unset PATH which would at least let cat work again for me. 

          Jesse Glick added a comment -

          Not sure offhand, would need to spend time studying it.

          Generally I would treat Image.inside as “take it or leave it”—if it works as a convenience, great; if not, ignore it and use generic techniques such as calling docker run to run desired steps (and docker cp to extra any files needed for archiving or reporting).

          Jesse Glick added a comment - Not sure offhand, would need to spend time studying it. Generally I would treat Image.inside as “take it or leave it”—if it works as a convenience, great; if not, ignore it and use generic techniques such as calling docker run to run desired steps (and docker cp to extra any files needed for archiving or reporting).

          James Johnson added a comment -

          This is an issue for me as well.  The composer docker image has the php binary in /usr/local/bin.  I've tried setting PATH via inside('-e PATH') without luck.  I was able to get around it at the moment by setting the PATH before calling the composer command.  

          Fails:

          docker.image("composer:latest").inside{
            sh 'composer install -a --no-dev --ignore-platform-reqs --no-scripts --no-interaction --no-ansi --no-progress'
          }
          

          Works:

          docker.image("composer:latest").inside{
            sh 'PATH=$PATH:/usr/local/bin composer install -a --no-dev --ignore-platform-reqs --no-scripts --no-interaction --no-ansi --no-progress'
          }
          

           

          James Johnson added a comment - This is an issue for me as well.  The composer docker image has the php binary in /usr/local/bin.  I've tried setting PATH via inside('-e PATH') without luck.  I was able to get around it at the moment by setting the PATH before calling the composer command.   Fails: docker.image( "composer:latest" ).inside{ sh 'composer install -a --no-dev --ignore-platform-reqs --no-scripts --no-interaction --no-ansi --no-progress' } Works: docker.image( "composer:latest" ).inside{ sh 'PATH=$PATH:/usr/local/bin composer install -a --no-dev --ignore-platform-reqs --no-scripts --no-interaction --no-ansi --no-progress' }  

          Christian Höltje added a comment - - edited

          It looks like multi-stage docker builds are a way around this, and they work on a workstation the same as in Jenkins.

          Note: the documentation doesn't mention this, but you can use --target= to pick which image gets tagged by the build.

          Example:

           

          docker build --target=builder --tag=build-env .
          

          ...would tag the builder from the multi-stage build so you could examine it.

          Christian Höltje added a comment - - edited It looks like multi-stage docker builds are a way around this, and they work on a workstation the same as in Jenkins. Note: the documentation doesn't mention this, but you can use --target= to pick which image gets tagged by the build. Example:   docker build --target=builder --tag=build-env . ...would tag the builder from the multi-stage build so you could examine it.

          Jesse Glick added a comment -

          Yes, though there are some limitations to that.

          Jesse Glick added a comment - Yes, though there are some limitations to that .

          Code changed in jenkins
          User: Christian Höltje
          Path:
          src/main/java/org/jenkinsci/plugins/docker/workflow/WithContainerStep.java
          http://jenkins-ci.org/commit/docker-workflow-plugin/e04959ea87d391ec89965c760b9d054b73bed317
          Log:
          Don't pass PATH to docker containers

          Docker containers usually have special PATHs; the PATH defined
          in on the Agent won't make sense for the container.

          JENKINS-43590 docker.inside()'s -e flags are random

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Christian Höltje Path: src/main/java/org/jenkinsci/plugins/docker/workflow/WithContainerStep.java http://jenkins-ci.org/commit/docker-workflow-plugin/e04959ea87d391ec89965c760b9d054b73bed317 Log: Don't pass PATH to docker containers Docker containers usually have special PATHs; the PATH defined in on the Agent won't make sense for the container. JENKINS-43590 docker.inside()'s -e flags are random

          I think blocking PATH isn't a correct fix.

          Environment is built from various sources, including Computer.getEnvironment(), which returns node's `PATH`.

          Tool installers do amend this using `PATH+` syntax

          docker-pipeline should replace Computer.getEnvironment() result with the actual `env` from container, so this mechanism works as expected.

          Nicolas De Loof added a comment - I think blocking PATH isn't a correct fix. Environment is built from various sources, including Computer.getEnvironment(), which returns node's `PATH`. Tool installers do amend this using `PATH+` syntax docker-pipeline should replace Computer.getEnvironment() result with the actual `env` from container, so this mechanism works as expected.

          ndeloof

          I didn't even know that was possible, but it makes sense.  Do we need a new issue for that?

          Christian Höltje added a comment - ndeloof I didn't even know that was possible, but it makes sense.  Do we need a new issue for that?

          Rong Shen added a comment -

          I agree with ndeloof

          This code change actually break our pipeline that passes PATH environment from hosts.

          Docker container path should be an optional overwrite, instead of blocking!

          Rong Shen added a comment - I agree with ndeloof This code change actually break our pipeline that passes PATH environment from hosts. Docker container path should be an optional overwrite, instead of blocking!

            Unassigned Unassigned
            docwhat Christian Höltje
            Votes:
            12 Vote for this issue
            Watchers:
            21 Start watching this issue

              Created:
              Updated: