New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Detect rootless mode #1484
Detect rootless mode #1484
Conversation
It's my first attempt to write a python patch ever. I apologize if I did it wrong. |
Build failing on coverage, I am afraid it is beyond my knowledge to fix this :/. |
Now I am facing a coverage error: https://asottile.visualstudio.com/asottile/_build/results?buildId=3831&view=logs&j=291e3f77-befc-520d-9779-f5b46c027190&t=037f03e6-93d9-56f0-123e-44a4e724aa8c&l=73 I am clueless on how to fix it:
|
I spent some time trying to understand how to write a test to make the build pass. I just don't understand how to get it. Too much to learn in one go. May I request some help (again)? So far I understand that it should probably take the for of a block like that in Then I assume we have to mock the content of Then I got lost in the pytest documentation.
|
@themr0c you can use a built-in fixture called The simple one case test would be _ROOTLESS_DOCKER_OUTPUT = """
some garbage
rootless: true
even more garbage
"""
def test_rootless_docker(monkeypatch):
"""Verify that only rootless docker/podman doesn't add args."""
monkeypatch.setattr('subprocess.check_output', lambda cmd, text: _ROOTLESS_DOCKER_OUTPUT)
assert docker.get_docker_user() == () |
And going further this would create tests for 4 test cases (rootless docker, rootless podman, non-rootless docker and non-rootless podman): import os
import pytest
_CURRENT_UID = os.getuid()
_CURRENT_GID = os.getgid()
@pytest.mark.parametrize(
('docker_sys_info', 'expected_args'),
(
pytest.param(
"""
some garbage
rootless: true
even more garbage
""",
(),
id='rootless podman',
),
pytest.param(
"""
some garbage
rootless: false
even more garbage
""",
(_CURRENT_UID, _CURRENT_GID),
id='non-rootless podman',
),
pytest.param(
"""
some garbage
rootless
even more garbage
""",
(),
id='rootless docker',
),
pytest.param(
"""
some garbage
nothing good
even more garbage
""",
(_CURRENT_UID, _CURRENT_GID),
id='non-rootless docker',
),
),
)
def test_rootless_docker(docker_sys_info, expected_args, monkeypatch):
"""Verify that only rootless docker/podman doesn't add args."""
monkeypatch.setattr('subprocess.check_output', lambda cmd, text: docker_sys_info)
assert docker.get_docker_user() == expected_args |
before going further please address my comments here in particular:
additionally:
|
I am completely illiterate in writing python code, it seems I chose a task too complex for me as first attempt to contribute :/.
|
@themr0c you literally can just |
in-process cache is fine -- there's currently no precedent for across-process caching in pre-commit, though I plan to do that eventually for virtualenv invalidation |
you're doing great! we'll help you through it :)
in this case it would just be importing
the helper is
taking the test above, you'd use with mock.patch.object(docker, 'cmd_output', return_value=(0, ..., '')):
... instead of the |
Side note: We have a videocall scheduled with @themr0c for early next week (this week it didn't work out) and we'll go trough the requested changes together. |
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.
I think you could also add a test calling docker.docker_is_rootless()
twice and checking that cmd_output
got called only once.
This seems a bit too far fetched to me. Isn't it rather testing lru_cache? Also, technically, other test might already called this. |
It's a regression test: when somebody decides to remove the decorator, the test should explode. It's up to you, of course, to skip adding it. |
# rootless docker has "rootless" | ||
# rootless podman has "rootless: true" | ||
if line.strip().startswith('rootless'): | ||
if 'false' not in line: |
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.
Note: Technically, the case where false
is present is not tested. However in practice, we haven't found a case like this, this check is present as precaution.
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 would be useful as a code 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.
Is that what is causing the coverage error?
Name Stmts Miss Branch BrPart Cover Missing
----------------------------------------------------------------------------
pre_commit/languages/docker.py 72 2 10 2 95% 89->91, 91, 101->102, 102
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 is weird. If tests include example outputs containing rootless
, they should also hit this condition check.
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.
@asottile any ideas about this?
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.
there's no test for rootless: false
so line 91 is never hit -- perhaps simpler would be if line.strip() == 'rootless: true': ...
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.
That would probably not catch docker's behavior. Better to just improve the test matrix
@themr0c I think this PR's title/description should be updated. |
Insecure Registries: | ||
127.0.0.0/8 | ||
Live Restore Enabled: true | ||
''' # noqa |
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.
it's usually better to specify specific violation codes instead of ignoring everything
let me know if you'd like me to finish this one, I've finally gotten around to setting up podman to reproduce this |
Depends on @themr0c. I am available to meet again and address the review comments, but if they prefer you to handle it, I don't mind. |
I may be able to take a look at this as I use CentOS and have both docker and podman installed. Are the review comments still current? |
looks like |
The user option is not necessary, podman does the correct mapping.
On the contrary, specifying user and userns can lead to trouble:
One of the main advantages of podman is to get rid of the -u option that you need when you run docker if you don't want to see your workspaces filled by files owned by root... So why insist on keeping some unnecessary complexity? |
the default mapping gives too much permission and can create undeletable files outside: $ podman run --rm -ti -v $PWD:/z:rw ubuntu:focal bash -c 'mkdir -p /z/1/2/3 && chown -R nobody /z/1'
$ rm -rf 1
rm: cannot remove '1/2/3': Permission denied |
Now I undertand better. |
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.
good for issue
Came here from #2173 and would be interested in taking this over. Would it be sufficient to return |
@PhilipTrauner please take it over. I am happy if this pull request gets closed, even unresolved. I apologize for having started something that I am not able to finish. |
run-in-node-container is used for javascript "rollup", so the tools running in the container produce files which must be owned by the user on the host. To achieve this, the docker run --user option is used to ensure that the tools in the container are run as host user. However, with rootless mode - apparently in both docker and podman, but I'm using podman - a user namespace is used and users in the container are mapped to a range of users on the host. This means that if we run a command as root in the container, this corresponds to the host user. When we specify --user, this results in a different host user being used. There are apparently two ways of achieving what we want - not using --user so that the commands run as root in the container, which is mapped to the desired host user. Or we can use --userns keep-id which means a 1:1 user mapping is used, and the user specified by --user corresponds to the same user on the host. The former seems more like how you'd typically use this mode. And so we detect rootless mode using "docker system info", and avoid the --user flag in this case. Podman reports "rootless: (true|false)", whereas docker just includes a "rootless" keyword. For more on this, see: https://www.redhat.com/sysadmin/user-flag-rootless-containers https://docs.docker.com/engine/security/rootless pre-commit/pre-commit#1243 pre-commit/pre-commit#1484 (Note: all of the above applies even without SELinux and was tested with "setenforce 0")
returncode, stdout, stderr = cmd_output( | ||
'docker', 'system', '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.
It would be even faster and simplify both the logic below and the mock output used in testing to only query Docker/Podman for the field we care about. From the rootless-docker GitHub Action's README, docker info --format "{{ .ClientInfo.Context }}"
outputs rootless
in Docker when in rootless mode, and other strings (most commonly default
, I believe) otherwise. I suspect, based on Podman's documentation, that podman info --format "{{ .host.security.rootless }}"
outputs true
when in rootless mode and false
otherwise. Go templates are quite powerful, so we can say: docker info --format '{{ if .ClientInfo.Context }} {{ eq .ClientInfo.Context "rootless" }} {{ else }} {{ .host.security.rootless }} {{ end }}'
. This command outputs true
when running in rootless mode and false
otherwise in both Docker and Podman, although I have only tested it on Docker. I don't have Podman installed, so hopefully someone who does can straighten me out if I'm mistaken. Disclosure: I am the author of the rootless-docker GitHub Action.
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.
Here on my machine (Linux Mint 19.3, Docker version 20.10.16, build aa7e414) your suggestion returns default
for rootless docker, as rootless
seems to be part of the Security Options
under Server
.
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.
Thanks for letting me know. Bummer that the behavior isn't consistent across our machines even on the exact same build of Docker. I am on Ubuntu 22.04, but I wonder whether the difference has more to do with how we installed rootless Docker? It's possible to detect rootless mode under SecurityOptions
too (e.g., docker info --format '{{ range .SecurityOptions }}{{ if eq . "name=rootless" }}true{{ end }}{{ end }}'
), FWIW.
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.
IIRC I followed the official instructions for Ubuntu. Maybe you chose to set the client context explicitly where I went for the DOCKER_HOST
environment variable? Both options are listed in the instructions: https://docs.docker.com/engine/security/rootless/#client
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.
Yes, you are correct. I suppose one could check the DOCKER_HOST
environment variable before running docker info
.
Closing, as this pull request implements the feature: kubernetes/test-infra#25841 |
fix #1243 - the -u option is not necessary on podman