Cleanup the docker permission issues. #1706
Conversation
The permissions on the mounted docker.sock where incorrect for the current user, which lead to workarounds setting the docker binary SUID. However this was a bit hacky and if programatic access to docker was needed (e.g. TestContainers, or anything else that used the socket and not the binary) then access would fail. Rather than set the binary SUID which only works for some of the docker use cases, we add the ath-user to the docker group that has access to the socket on the host at run time.
| # Set SUID and SGID for docker binary so it can communicate with mapped socket its uid:gid we can not control. Alternative | ||
| # approach used for this is adding ath-user to the group of /var/run/docker.sock but that require root permission we do not | ||
| # have in ENTRYPOINT as the container is started as ath-user. | ||
| RUN chmod ug+s /usr/bin/docker* | ||
|
|
There was a problem hiding this comment.
removed as we now have the correct groups when we run (due to --group-add).
| @@ -125,7 +125,8 @@ for (int i = 0; i < splits.size(); i++) { | |||
| def image = skipImageBuild ? docker.image('jenkins/ath') : docker.build('jenkins/ath', '--build-arg uid="$(id -u)" --build-arg gid="$(id -g)" ./src/main/resources/ath-container/') | |||
| sh 'mkdir -p target/ath-reports && chmod a+rwx target/ath-reports' | |||
| def cwd = pwd() | |||
| image.inside("-v /var/run/docker.sock:/var/run/docker.sock -v '${cwd}/target/ath-reports:/reports:rw' --shm-size 2g") { | |||
| def dockergid = sh label: 'get docker group', returnStdout: true, script: 'getent group docker | cut -d: -f3' | |||
| image.inside("--group-add ${dockergid} -v /var/run/docker.sock:/var/run/docker.sock -v '${cwd}/target/ath-reports:/reports:rw' --shm-size 2g") { | |||
There was a problem hiding this comment.
use the supported https://docs.docker.com/engine/containers/run/#additional-groups
There was a problem hiding this comment.
| image.inside("--group-add ${dockergid} -v /var/run/docker.sock:/var/run/docker.sock -v '${cwd}/target/ath-reports:/reports:rw' --shm-size 2g") { | |
| image.inside("--group-add $(getent group docker | cut -d: -f3) -v /var/run/docker.sock:/var/run/docker.sock -v '${cwd}/target/ath-reports:/reports:rw' --shm-size 2g") { |
Would it avoid to execute the shell step before (to avoid a back and forth) ? I remember that $(id -u) was used on other cases
There was a problem hiding this comment.
this would appear to me to be very brittle and relies on the image.inside implementation that would currently appear go via a shell to interpret this. (which is why I moved it outside).
jtnord
requested review from
basil and
dduportal
and removed request for
basil
|
build failure due to upstream issue As we have a successfull build of the image and we have |
Suggested by @dduportal that for docker-dekstop on mac the permission needeed needs to be obtained from the server (so spawn a container and check it!)
|
@jtnord Can you also please update https://github.com/jenkinsci/jenkins/blob/db5121061c90fee5c0387e7bd2cf49fe74b31bc7/ath.sh as well? Even though we don't currently run Docker-based tests as part of core PRs, we might in the future, and it seems like good hygiene in general to keep all consumers of this image up-to-date with recent calling conventions. |
The ATH image no longer SUIDs the docker executable but relies on permissions of the docker.sock. This was needed to allow test-containers based tests to run as this uses the socket directly rather than relying on the docker cli. cf: jenkinsci/acceptance-test-harness#1706
Thanks, I had forgotten about that, filed jenkinsci/jenkins#9701 |
) The ATH image no longer SUIDs the docker executable but relies on permissions of the docker.sock. This was needed to allow test-containers based tests to run as this uses the socket directly rather than relying on the docker cli. cf: jenkinsci/acceptance-test-harness#1706
The permissions on the mounted
docker.sockwhere incorrect for the current user, which lead to workarounds setting the docker binary SUID.However this was a bit hacky and if programmatic access to docker was needed (e.g. TestContainers, or anything else that used the socket and not the binary) then access would fail.
Rather than set the binary SUID which only works for some of the docker use cases, we add the
ath-userto the docker group that has access to the socket on the host at run time.extracted from #1705
Testing done
Testing in this PR and #705 for the test containers case.
https://ci.jenkins.io/job/Core/job/acceptance-test-harness/job/PR-1705/4/testReport/plugins/OicAuthPluginTest/ (shows the test-container tests run and do not fail because of docker)
LdapPluginTest which requires docker is passing
Shell script has not been tested, nor has any testing been performed on a macbook with docker-desktop
Submitter checklist