Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: work with rootless docker #362

Merged
merged 1 commit into from
Mar 27, 2022

Conversation

paulpach
Copy link
Contributor

@paulpach paulpach commented Mar 26, 2022

Running docker currently mounts the docker.sock file into the container.
This was introduced in 2ab738c but
there is no explanation provided.

The docker.sock file is only needed if we want to run docker inside the container
to create other images or start other containers.
I searched through the code and I did not find any such use.

In particular, on fedora this gives permission denied because docker.sock
is owned by root and the container runs under an unprivileged user.
One has to change the permissions of docker.sock
(which is actually a link to /run/podman/podman.sock) to be writeable by the user.

If we don't need to use docker inside the containers, then we can remove this file,
thus we can run this GitHub action as an unprivileged user out of the box.

Changes

  • Remove mounting docker.sock in the container

Checklist

  • Read the contribution guide and accept the code of conduct
  • Readme (updated or not needed)
  • Tests (added, updated or not needed)

@github-actions
Copy link

Cat Gif

@codecov-commenter
Copy link

codecov-commenter commented Mar 26, 2022

Codecov Report

Merging #362 (491c691) into main (1ae498b) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #362   +/-   ##
=======================================
  Coverage   44.65%   44.65%           
=======================================
  Files          51       51           
  Lines        1496     1496           
  Branches      233      233           
=======================================
  Hits          668      668           
  Misses        824      824           
  Partials        4        4           
Impacted Files Coverage Δ
src/model/docker.ts 18.75% <ø> (ø)

@paulpach
Copy link
Contributor Author

paulpach commented Mar 26, 2022

I just checked, docker is not even installed inside those images, so mounting docker.sock is not useful at all.

@paulpach paulpach changed the title feat: work with rootless accounts by default feat: work with rootless docker by default Mar 26, 2022
@paulpach paulpach changed the title feat: work with rootless docker by default feat: work with rootless docker Mar 26, 2022
Running docker currently mounts the docker.sock file into the container.
This was introduced in 2ab738c but
there is no explanation provided.

The docker.sock file is only needed if we want to run docker inside the container
to create other images or start other containers.
I searched through the code and I did not find any such use.

In particular, on fedora this gives permission denied because docker.sock
is owned by root and the container runs under an unprivileged user.
One has to change the permissions of docker.sock
(which is actually a link to /run/podman/podman.sock) to be writeable by the user.

If we don't need to use docker inside the containers,  then we can remove this file,
thus we can run this GitHub action as an unprivileged user out of the box.
Copy link
Member

@webbertakken webbertakken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clear description. Looks good to me!

@webbertakken webbertakken merged commit 9440c54 into game-ci:main Mar 27, 2022
@paulpach paulpach deleted the nodockerindocker branch March 27, 2022 08:18
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants