FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix permission errors for mounts in rootless docker by matthewhughes934 · Pull Request #3446 · pre-commit/pre-commit · GitHub

/ pre-commit Public

Fix permission errors for mounts in rootless docker - #3446

Merged
asottile merged 1 commit into
pre-commit:mainfrom
matthewhughes934:fix-docker-rootless-permission-mounts
May 23, 2025
ConversationCommits (1)ChecksFiles changed
Merged

Fix permission errors for mounts in rootless docker#3446
asottile merged 1 commit into
pre-commit:mainfrom
matthewhughes934:fix-docker-rootless-permission-mounts

Conversation

matthewhughes934 commented Apr 13, 2025 •
edited by asottile
Loading

Copy link
Copy Markdown
Contributor

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 info I don't think is documented/guaranteed anywhere, but it should corresponding to the format of a /info API request to Docker[3]

The added test hopes 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

matthewhughes934 force-pushed the fix-docker-rootless-permission-mounts branch 2 times, most recently from 26b203c to 399f2f0 Compare April 13, 2025 19:03
matthewhughes934 commented Apr 13, 2025
View reviewed changes
Comment thread tests/languages/docker_test.py Outdated
),
)
def test_docker_user_rootless_docker(info_ret, expect_root):
docker._is_rootless_docker.cache_clear()

matthewhughes934 Apr 13, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

🤔 maybe we should clear this before and after the test run: otherwise we risk polluting following tests with whatever value we last injected (and not the real value it reads from docker)

asottile Apr 19, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

usually a better approach is something like this:

@pytest.fixture(autouse=True)
def _avoid_cache():
    with mock.patch.object(docker, '_is_rootless_docker', docker._is_rootless_docker.__wrapped__):
        yield

this bypasses the cache for the duration of the test

matthewhughes934 marked this pull request as ready for review April 13, 2025 19:10
asottile reviewed Apr 19, 2025
View reviewed changes
Comment thread pre_commit/languages/docker.py Outdated
Comment on lines +107 to +115
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)

asottile Apr 19, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

matthewhughes934 Apr 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 for podman 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 docker invokes 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 --version and search the output for "podman"
  • Or use the entire JSON output from docker system info
    • If .host exists: check ["host"]["Security"]["Rootless"], else
    • Check in ["SecurityOptions"]

matthewhughes934 Apr 20, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Parsing the entire info response seemed the most robust: 9bc412d

Comment thread tests/languages/docker_test.py Outdated
Comment on lines +78 to +81
if expect_root:
assert docker.get_docker_user() == ()
else:
assert docker.get_docker_user() != ()

asottile Apr 19, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

don't write logic in tests -- these are two separate disparate behaviours and should be tested separately

matthewhughes934 Apr 20, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

asottile force-pushed the fix-docker-rootless-permission-mounts branch from 3b3d042 to 026db3a Compare May 23, 2025 20:54
asottile approved these changes May 23, 2025
View reviewed changes

asottile left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fix permission errors for mounts in rootless docker
466f6c4 
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 info` I don't think is
documented/guaranteed anywhere, but it should corresponding to the
format of a `/info` API request to Docker[3]

The added test _hopes_ 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: 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>
asottile force-pushed the fix-docker-rootless-permission-mounts branch from 026db3a to 466f6c4 Compare May 23, 2025 21:01
asottile enabled auto-merge May 23, 2025 21:01
asottile merged commit 8a4af02 into pre-commit:main May 23, 2025
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Aug 12, 2025
build(deps): update pre-commit to v4.3.0
2caedcd 
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.
  - [#&#8203;3446](pre-commit/pre-commit#3446) MR by [@&#8203;matthewhughes934](https://github.com/matthewhughes934).
  - [#&#8203;1243](pre-commit/pre-commit#1243) issue by [@&#8203;dkolepp](https://github.com/dkolepp).
- `language: julia`: avoid `startup.jl` when executing hooks.
  - [#&#8203;3496](pre-commit/pre-commit#3496) MR by [@&#8203;ericphanson](https://github.com/ericphanson).
- `language: dart`: support latest dart versions which require a higher sdk
  lower bound.
  - [#&#8203;3507](pre-commit/pre-commit#3507) MR by [@&#8203;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-->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

asottile asottile approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Permission issue with rootless containers

2 participants

Back | FazBrowse Home | New Git URL