specgen: honor userns=auto from containers.conf #12621

giuseppe · 2021-12-16T11:34:11Z

when using the default userns value, make sure its value is parsed so
that userns=auto is parsed and the options for the storage are filled.

Closes: #12615

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

openshift-ci · 2021-12-16T11:34:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

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:

~~OWNERS~~ [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

test/system/170-run-userns.bats

vrothberg · 2021-12-16T12:22:58Z

test/system/170-run-userns.bats

+[containers]
+userns="auto"
+EOF
+    export CONTAINERS_CONF=$PODMAN_TMPDIR/userns_auto.conf


Apologies for not spotting earlier. Could we instead of exporting set it as follows:
CONTAINERS_CONF=... run_podman

Otherwise, I am afraid of all subsequent calls using this containers.conf.

fixed.

I wrongly assumed each test would run in its own process

rhatdan · 2021-12-16T12:47:38Z

LGTM Once you make @vrothberg suggestion.

vrothberg

/lgtm
/hold

mheon · 2021-12-16T14:21:41Z

LGTM

edsantiago

Finally getting to this. Thanks for fixing the env issue.

When I run this on my f35 laptop, as root, it fails:

   # .../podman run quay.io/libpod/testimage:20210610 grep -v 4294967295 /proc/self/uid_map
   time="2021-12-16T07:41:07-07:00" level=error msg="Cannot find mappings for user \"containers\": No subuid ranges found for user \"containers\" in /etc/subuid"
   Error: error creating container storage: could not find enough available IDs
   [ rc=125 (** EXPECTED 1 **) ]

If CI fails, then we're consistent, and the test needs fixing.

If CI passes, then the test in inconsistent, and we need to understand why.

edsantiago · 2021-12-16T14:42:35Z

test/system/170-run-userns.bats

+[containers]
+userns="auto"
+EOF
+    CONTAINERS_CONF=$PODMAN_TMPDIR/userns_auto.conf run_podman 1 run $IMAGE grep -v 4294967295 /proc/self/uid_map


Could you add a comment explaining the significance of the magic constant?

And, could you add --rm just to be polite?

sure, added a comment.

Also added a commit to make the existing tests polite as well

edsantiago · 2021-12-16T14:43:09Z

test/system/170-run-userns.bats

@@ -52,3 +52,14 @@ function _require_crun() {
    run_podman 125 run --group-add keep-groups --group-add 457 $IMAGE id
    is "$output" ".*the '--group-add keep-groups' option is not allowed with any other --group-add options" "Check group leaked into container"
 }
+
+@test "podman userns=auto in config file" {
+    skip_if_rootless "the user might not have enough IDs"


Is there a way you could compute this from the current context's /proc/self/uid_map and /etc/subuid?

we could parse /etc/subuid, dropped this line for an explicit check.

edsantiago

I still have serious concerns. None of this seems to work.

edsantiago · 2021-12-16T16:44:38Z

test/system/170-run-userns.bats

+
+    if is_rootless; then
+        if ! grep -q ^$(id -un) /etc/subuid; then
+           echo "there are no IDs allocated for the current user"


Shouldn't this be skip?

edsantiago · 2021-12-16T16:51:15Z

test/system/170-run-userns.bats

+    skip_if_remote "userns=auto is set on the server"
+
+    if is_rootless; then
+        if ! grep -q ^$(id -un) /etc/subuid; then


Suggestion: add a colon, just to catch the possibility of foo vs foo123:

index 8b0591116..513f4ae41 100644 --- a/test/system/170-run-userns.bats +++ b/test/system/170-run-userns.bats @@ -57,13 +57,9 @@ function _require_crun() { skip_if_remote "userns=auto is set on the server" if is_rootless; then - if ! grep -q ^$(id -un) /etc/subuid; then - echo "there are no IDs allocated for the current user" - fi + egrep -q "^$(id -un):" /etc/subuid || skip "no IDs allocated for current user" else - if ! grep -q ^containers /etc/subuid; then - echo "there are no IDs allocated for the user 'containers'" - fi + egrep -q "^containers:" /etc/subuid || skip "no IDs allocated for user 'containers'" fi cat > $PODMAN_TMPDIR/userns_auto.conf <<EOF

edsantiago · 2021-12-16T16:52:04Z

test/system/170-run-userns.bats

+           echo "there are no IDs allocated for the current user"
+        fi
+    else
+        if ! grep -q ^containers /etc/subuid; then


Under what circumstances will containers exist in /etc/subuid? Because it doesn't on my laptop, and apparently (based on previous CI run) not in CI hosts either.

it must be added manually, but this is the user hardcoded in containers/storage to assign a range used by auto user namespaces

edsantiago · 2021-12-16T16:52:46Z

test/system/170-run-userns.bats

+EOF
+    # check that it is running in a user namespace by verifying 4294967295 (the maximum number of IDs) is not present
+    # in the mappings file.
+    CONTAINERS_CONF=$PODMAN_TMPDIR/userns_auto.conf run_podman 1 run --rm $IMAGE grep -v 4294967295 /proc/self/uid_map


This fails on my laptop:

$ .../podman run --rm quay.io/libpod/testimage:20210610 grep -v 4294967295 /proc/self/uid_map 0 1 1024 #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv #| FAIL: exit code is 0; expected 1 #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

when using the default userns value, make sure its value is parsed so that userns=auto is parsed and the options for the storage are filled. Closes: containers#12615 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe · 2021-12-20T19:25:00Z

I think comments were addressed and tests are finally green

vrothberg

/lgtm
/hold

@containers/podman-maintainers PTAL

rhatdan · 2021-12-21T13:41:01Z

/hold cancel

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 16, 2021

giuseppe mentioned this pull request Dec 16, 2021

userns = "auto" not respected in containers.conf #12615

Closed

vrothberg reviewed Dec 16, 2021

View reviewed changes

test/system/170-run-userns.bats Outdated Show resolved Hide resolved

giuseppe force-pushed the honor-userns-auto-conf-file branch from 440efff to 74076b9 Compare December 16, 2021 12:20

vrothberg reviewed Dec 16, 2021

View reviewed changes

giuseppe force-pushed the honor-userns-auto-conf-file branch from 74076b9 to ebe698f Compare December 16, 2021 14:15

vrothberg reviewed Dec 16, 2021

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2021

openshift-ci bot assigned vrothberg Dec 16, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2021

edsantiago requested changes Dec 16, 2021

View reviewed changes

giuseppe force-pushed the honor-userns-auto-conf-file branch from ebe698f to b28834e Compare December 16, 2021 16:12

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2021

giuseppe force-pushed the honor-userns-auto-conf-file branch from b28834e to dfab5bc Compare December 16, 2021 16:14

edsantiago requested changes Dec 16, 2021

View reviewed changes

giuseppe force-pushed the honor-userns-auto-conf-file branch 5 times, most recently from b3a0a01 to 415a9b3 Compare December 17, 2021 14:01

test: add --rm to podman run commands

46a094a

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the honor-userns-auto-conf-file branch 3 times, most recently from ae43e38 to 83187a3 Compare December 20, 2021 13:28

specgen: honor userns=auto from containers.conf

89ee302

when using the default userns value, make sure its value is parsed so that userns=auto is parsed and the options for the storage are filled. Closes: containers#12615 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the honor-userns-auto-conf-file branch from 83187a3 to 89ee302 Compare December 20, 2021 16:04

vrothberg reviewed Dec 21, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 21, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 21, 2021

openshift-merge-robot merged commit da7de33 into containers:main Dec 21, 2021

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

specgen: honor userns=auto from containers.conf #12621

specgen: honor userns=auto from containers.conf #12621

giuseppe commented Dec 16, 2021

openshift-ci bot commented Dec 16, 2021

vrothberg Dec 16, 2021

giuseppe Dec 16, 2021

rhatdan commented Dec 16, 2021

vrothberg left a comment

mheon commented Dec 16, 2021

edsantiago left a comment

edsantiago Dec 16, 2021

giuseppe Dec 16, 2021

edsantiago Dec 16, 2021

giuseppe Dec 16, 2021

edsantiago left a comment

edsantiago Dec 16, 2021

edsantiago Dec 16, 2021

edsantiago Dec 16, 2021

giuseppe Dec 16, 2021

edsantiago Dec 16, 2021

giuseppe commented Dec 20, 2021

vrothberg left a comment

rhatdan commented Dec 21, 2021

specgen: honor userns=auto from containers.conf #12621

specgen: honor userns=auto from containers.conf #12621

Conversation

giuseppe commented Dec 16, 2021

openshift-ci bot commented Dec 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Dec 16, 2021

vrothberg left a comment

Choose a reason for hiding this comment

mheon commented Dec 16, 2021

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppe commented Dec 20, 2021

vrothberg left a comment

Choose a reason for hiding this comment

rhatdan commented Dec 21, 2021