-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fix --userns=ns:<path> conflicting with runc 1.1.11+ #27239
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
Conversation
Does this break on earlier runc versions? |
I think so. However, I'm not sure how to check the runc version. |
Also, I'm not sure how old a runc version we support. |
We don't have an official minimum supported version. We might want to figure that out, if we're going to break older runc. @kolyshkin Do you know when runc stopped needing this hack? |
The first release with this change of behavior runc v1.1.11 was released at the beginning of 2024, all I can find documented about a minimum supported version of The safest option could be to make this conditional and drop support for runc <v1.1.11 in our 6.0 release. A potentially brittle method to check the version the user is running could be parsing the output of |
This was merged in runc 1.2.0-rc.1 (Apr 3, 2024, with the final 1.2.0 released in Oct 2024), and was backported to runc 1.1 branch (included in runc v1.1.11, Jan 1, 2024). From what I see RHEL8 uses runc v1.1.12, and that's the oldest version I can find. |
I agree that this is probably the safest way.
Getting the runtime version is not trivial. Podman has a function that gets information about the container runtime, but this function causes cyclic imports. Blindly calling |
test/system/170-run-userns.bats
Outdated
|
||
# bats test_tags=ci:parallel | ||
@test "podman --userns=ns:<path> join existing user namespace" { | ||
skip_if_not_rootless |
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 should work fine rootless I belive?
test/system/170-run-userns.bats
Outdated
skip_if_not_rootless | ||
# Test for issue #27148: --userns=ns:<path> should not add dummy mappings | ||
|
||
local cname="userns_source_$(random_string 8)" |
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.
do not use random_string for resource names, always use safename
test/system/170-run-userns.bats
Outdated
local pid=$output | ||
local userns_path="/proc/$pid/ns/user" | ||
|
||
test -e "$userns_path" |
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 seems unnecessary, podmna run will fail if the path is not valid.
test/system/170-run-userns.bats
Outdated
run_podman run --rm --userns=ns:$userns_path $IMAGE echo "success" | ||
assert "$output" == "success" "Should be able to join existing user namespace" |
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.
The echo doesn't actually test that we successfully joined the same userns though. If userns is a NOP
see the test in test/system/195-run-namespaces.bats as example, you should run something like readlink /proc/self/ns/user
and then compare that against the podman exec $cname readlink /proc/self/ns/user
output to ensure they do in fact use the same userns, we could also compare cat /proc/self/uid_map
for good measures
I am not a fan of version checks, it complicates things and practically good always lie with distros backporting fixes without version number changes. The way I see it --userns ns: is broken with runc of today so podman of today should work with it so I personally would just go ahead with this, as @kolyshkin mentioned the runc fix is out for well over a year so if people update podman without updating the underlying oci runtime they always can encounter problems. |
Remove dummy UID/GID mappings added when joining existing user namespaces, which runc 1.1.11+ rejects as conflicting. RUNC fix: opencontainers/runc#4124 Fixes containers#27148 Signed-off-by: Jan Rodák <[email protected]>
@Luap99 I have updated the test according to your review. |
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.
LGTM
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.
/LGTM
I will create an issue for updating the runc minimum version.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Honny1, Luap99, ninja-quokka The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Remove dummy UID/GID mappings added when joining existing user namespaces, which runc 1.1.11+ rejects as conflicting.
RUNC fix: opencontainers/runc#4124
Fixes #27148
Does this PR introduce a user-facing change?