Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork931
Fix permission errors for mounts in rootless docker#3446
Conversation
26b203c to399f2f0Comparetests/languages/docker_test.py Outdated
| ), | ||
| ) | ||
| def test_docker_user_rootless_docker(info_ret, expect_root): | ||
| docker._is_rootless_docker.cache_clear() |
matthewhughes934Apr 13, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🤔 maybe we should clear this beforeand after the test run: otherwise we risk polluting following tests with whatever value we last injected (and not the real value it reads fromdocker)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
usually a better approach is something like this:
@pytest.fixture(autouse=True)def_avoid_cache():withmock.patch.object(docker,'_is_rootless_docker',docker._is_rootless_docker.__wrapped__):yield
this bypasses the cache for the duration of the test
pre_commit/languages/docker.py Outdated
| retcode, out, _ = cmd_output_b( | ||
| 'docker', 'system', 'info', '--format', '{{ json .SecurityOptions }}', | ||
| ) | ||
| # some failures are to be expected, e.g. for 'podman' aliased as 'docker' | ||
| if retcode != 0: | ||
| return False | ||
| info = json.loads(out) | ||
| return any(opt == 'name=rootless' for opt in info) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this doesn't seem to work for podman :(
$readlink -f$(which docker)/usr/bin/podman$docker system info --format'{{ json .SecurityOptions }}'Error: template: info:1:8: executing "info" at <.SecurityOptions>: can't evaluate field SecurityOptions in type *define.Info
matthewhughes934Apr 20, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this doesn't seem to work for podman :(
$ readlink -f $(which docker)/usr/bin/podman$ docker system info --format '{{ json .SecurityOptions }}'Error: template: info:1:8: executing "info" at <.SecurityOptions>: can't evaluate field SecurityOptions in type *define.Info
It looks like the invocation we need forpodman is:podman system info --format '{{ json .Host.Security.Rootless }}', I guess we could either:
- Run the docker command and on failure try the podman one, or
- Run a separate command to figure out if
dockerinvokes docker or podman and then run the corresponding command (ignoring any other container engines for now)
Trying to find a suitable command:
- Naively I just want to run
docker --versionand search the output for"podman" - Or use the entire JSON output from
docker system info- If
.hostexists: check["host"]["Security"]["Rootless"], else - Check in
["SecurityOptions"]
- If
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Parsing the entire info response seemed the most robust:9bc412d
tests/languages/docker_test.py Outdated
| if expect_root: | ||
| assert docker.get_docker_user() == () | ||
| else: | ||
| assert docker.get_docker_user() != () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
don't write logic in tests -- these are two separate disparate behaviours and should be tested separately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
don't write logic in tests -- these are two separate disparate behaviours and should be tested separately
909c165 also included your cache suggestion from above
3b3d042 to026db3aCompare
asottile left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
By running containers in a rootless docker context as root. This isbecause user and group IDs are remapped in the user namespaces uses byrootless docker, and it's unlikely that the current user ID will map tothe same ID under this remap (see docs[1] for some more details).Specifically, it means ownership of mounted volumes will not be for thecurrent user and trying to write can result in permission errors.This change borrows heavily from an existing PR[2].The output format of `docker system info` I don't think isdocumented/guaranteed anywhere, but it should corresponding to theformat of a `/info` API request to Docker[3]The added test _hopes_ to avoid regressions in this behaviour, but sincetests aren't run in a rootless docker context on the PR checks (and Icouldn't find an easy way to make it the case) there's still a risk ofregressions sneaking in.Link:https://docs.docker.com/engine/security/rootless/ [1]Link:pre-commit#1484 [2]Link:https://docs.docker.com/reference/api/engine/version/v1.48/#tag/System/operation/SystemAuth [3]Co-authored-by: Kurt von Laven <Kurt-von-Laven@users.noreply.github.com>Co-authored-by: Fabrice Flore-Thébault <ffloreth@redhat.com>
026db3a to466f6c4Compare8a4af02 intopre-commit:mainUh oh!
There was an error while loading.Please reload this page.
This MR contains the following updates:| Package | Update | Change ||---|---|---|| [pre-commit](https://github.com/pre-commit/pre-commit) | minor | `4.2.0` -> `4.3.0` |MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).**Proposed changes to behavior should be submitted there as MRs.**---### Release Notes<details><summary>pre-commit/pre-commit (pre-commit)</summary>### [`v4.3.0`](https://github.com/pre-commit/pre-commit/blob/HEAD/CHANGELOG.md#430---2025-08-09)[Compare Source](pre-commit/pre-commit@v4.2.0...v4.3.0)\==================##### Features- `language: docker` / `language: docker_image`: detect rootless docker. - [#​3446](pre-commit/pre-commit#3446) MR by [@​matthewhughes934](https://github.com/matthewhughes934). - [#​1243](pre-commit/pre-commit#1243) issue by [@​dkolepp](https://github.com/dkolepp).- `language: julia`: avoid `startup.jl` when executing hooks. - [#​3496](pre-commit/pre-commit#3496) MR by [@​ericphanson](https://github.com/ericphanson).- `language: dart`: support latest dart versions which require a higher sdk lower bound. - [#​3507](pre-commit/pre-commit#3507) MR by [@​bc-lee](https://github.com/bc-lee).</details>---### Configuration📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.🔕 **Ignore**: Close this MR and you won't be reminded about this update again.--- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box---This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MS41OC4yIiwidXBkYXRlZEluVmVyIjoiNDEuNTguMiIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90Il19-->
Uh oh!
There was an error while loading.Please reload this page.
By running containers in a rootless docker context as root. This is because user and group IDs are remapped in the user namespaces uses by rootless docker, and it's unlikely that the current user ID will map to the same ID under this remap (see docs[1] for some more details). Specifically, it means ownership of mounted volumes will not be for the current user and trying to write can result in permission errors.
This change borrows heavily from an existing PR[2].
The output format of
docker system infoI don't think is documented/guaranteed anywhere, but it should corresponding to the format of a/infoAPI request to Docker[3]The added testhopes to avoid regressions in this behaviour, but since tests aren't run in a rootless docker context on the PR checks (and I couldn't find an easy way to make it the case) there's still a risk of regressions sneaking in.
Link:https://docs.docker.com/engine/security/rootless/ [1]
Link:#1484 [2]
Link:https://docs.docker.com/reference/api/engine/version/v1.48/#tag/System/operation/SystemAuth [3]
resolves#1243