jenkinsci / acceptance-test-harness

Acceptance tests cases for Jenkins and its plugins based on selenium and docker.
128 stars 234 forks source link

Cleanup the docker permission issues. #1706

Closed jtnord closed 1 month ago

jtnord commented 1 month ago

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 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-user to 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

jtnord commented 1 month ago

build failure due to upstream issue

15:28:27   > [ 3/16] RUN install -m 0755 -d /etc/apt/keyrings   && curl -fsSL https://packages.mozilla.org/apt/repo-signing-key.gpg -o /etc/apt/keyrings/packages.mozilla.org.asc   && printf 'deb [signed-by=/etc/apt/keyrings/packages.mozilla.org.asc] https://packages.mozilla.org/apt mozilla main\n' > /etc/apt/sources.list.d/mozilla.list   && printf 'Package: *\nPin: origin packages.mozilla.org\nPin-Priority: 1000\n' > /etc/apt/preferences.d/mozilla   && apt-get update   && DEBIAN_FRONTEND=noninteractive TZ=Etc/UTC apt-get install --no-install-recommends -y     firefox="129.0.2*"   && apt-get clean   && rm -rf /var/lib/apt/lists/*:
15:28:27  0.430 curl: (22) The requested URL returned error: 502

As we have a successfull build of the image and we have LdapPluginTest passing I am intending to merge with these failings, once approved (to save getting another round of builds run on the infra)

basil commented 1 month ago

@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.

jtnord commented 1 month ago

@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.

Thanks, I had forgotten about that, filed https://github.com/jenkinsci/jenkins/pull/9701