Skip to content
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

podman: add uid and gid options to keep-id #15389

Merged
merged 3 commits into from
Aug 31, 2022

Conversation

giuseppe
Copy link
Member

@giuseppe giuseppe commented Aug 19, 2022

add two new options to the keep-id user namespace option:

  • uid: allow to override the UID used inside the container.
  • gid: allow to override the GID used inside the container.

For example, the following command will map the rootless user (that
has UID=0 inside the rootless user namespace) to the UID=11 inside the
container user namespace:

$ podman run --userns=keep-id:uid=11 --rm -ti fedora cat /proc/self/uid_map
0 1 11
11 0 1
12 12 65525

Closes: #15294

Does this PR introduce a user-facing change?

The --userns=keep-id now supports the options `uid` and `gid` to override the IDs used inside the user namespace

@openshift-ci openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Aug 19, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 19, 2022

[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:

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

@openshift-ci openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Aug 19, 2022
@giuseppe giuseppe force-pushed the userns-map-user branch 3 times, most recently from f466619 to d342bf7 Compare August 19, 2022 15:26
@vrothberg
Copy link
Member

Rebasing will fix CI

@rhatdan
Copy link
Member

rhatdan commented Aug 22, 2022

I like the idea, but this makes a lot of assumptions around the user knowing the UID inside of the container to run with.

podman run --userns=keep-id:user ...

Would be nicer, but as you say, you would need to do a lot more diagnosing to figure this out if the container run as the mysql user.

@TomSweeneyRedHat
Copy link
Member

Changes LGTM, but I"ll let @rhatdan give the final approval.

@umohnani8
Copy link
Member

changes LGTM

@giuseppe
Copy link
Member Author

rebased, tests are green now

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM
@edsantiago PTAL

Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

You went with the version from podman-create, which LGTM.

One change request, and lots of pointing-outs for those not running markdown-preprocess-review.

Thank you for doing this! This is one option that I could not do on my own.


Podman allocates unique ranges of UIDs and GIDs from the `containers` subordinate user ids. The size of the ranges is based on the number of UIDs required in the image. The number of UIDs and GIDs can be overridden with the `size` option.

The rootless option `--userns=keep-id` uses all the subuids and subgids of the user. Using `--userns=auto` when starting new containers will not work as long as any containers exist that were started with `--userns=keep-id`.
Copy link
Member

Choose a reason for hiding this comment

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

This line does not appear in any other file (podman-create, podman-kube-play) and was not selected for the final winner.


Example: `containers:2147483647:2147483648`.

Podman allocates unique ranges of UIDs and GIDs from the `containers` subordinate user ids. The size of the ranges is based on the number of UIDs required in the image. The number of UIDs and GIDs can be overridden with the `size` option. The `auto` options currently does not work in rootless mode
Copy link
Member

Choose a reason for hiding this comment

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

The final sentence ("auto options ... rootless mode") does not appear in the podman-run file. It does appear in podman-create and podman-kube-play and here (in the winner). Someone please confirm that it is applicable to podman-run also.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh that is a mistake, it works in rootless mode. I'll drop it


**container:**_id_: join the user namespace of the specified container.

**host**: create a new namespace for the container.
Copy link
Member

Choose a reason for hiding this comment

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

This text loses out. Someone please double-check that the new text is applicable to kube-play.


**nomap**: creates a user namespace where the current rootless user's UID:GID are not mapped into the container. This option is not allowed for containers created by the root user.

**ns:**_namespace_: run the container in the given existing user namespace.
Copy link
Member

Choose a reason for hiding this comment

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

Please change this to <<container|pod>>

Copy link
Member Author

Choose a reason for hiding this comment

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

this is the path to an existing namespace. It can have the form /proc/$PID/ns/user as well as a path on the file system which pins an existing user namespace.

Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry, I don't understand how this addresses my request. I'll try to restate it in diff form:

- ...run the container in
+ ...run the <<container|pod>> in

Reason: you are including this new file in podman-kube-play, which talks in terms of pods, not containers.

I strongly encourage you to run hack/markdown-preprocess-review to verify your man-page changes. (Requires dnf install diffuse).

Copy link
Member

Choose a reason for hiding this comment

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

This is still not addressed in your latest push. Is that an oversight, or is it deliberate?

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry, my fault. I misunderstood your comment earlier. I assumed it was about the _namespace_ bit.

I've fixed it now and pushed a new version.

docs/source/markdown/options/userns.container.md Outdated Show resolved Hide resolved
docs/source/markdown/options/userns.container.md Outdated Show resolved Hide resolved
@edsantiago
Copy link
Member

bud test failure is the nasty 0x3c flake (which I haven't filed as an issue, because it's pretty obviously a bug with access.redhat.com). Restarted, even though PR might need one more resubmit.

@edsantiago
Copy link
Member

ping, this is awaiting input from a userns expert. Please see the questions I've raised in the man page.

Signed-off-by: Giuseppe Scrivano <[email protected]>
add two new options to the keep-id user namespace option:

- uid: allow to override the UID used inside the container.
- gid: allow to override the GID used inside the container.

For example, the following command will map the rootless user (that
has UID=0 inside the rootless user namespace) to the UID=11 inside the
container user namespace:

$ podman run --userns=keep-id:uid=11 --rm -ti  fedora cat /proc/self/uid_map
         0          1         11
        11          0          1
        12         12      65525

Closes: containers#15294

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe
Copy link
Member Author

addressed the comments and pushed a new version

@edsantiago
Copy link
Member

@vrothberg could you PTAL at this flake when you get in Wednesday? Ununtu rootless, and it looks like the recent RHEL7 conmon bug:

[+1081s] not ok 274 podman generate - systemd - basic
...
         # #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
         # #|     FAIL: state of service after systemctl stop
         # #| expected: = 'ActiveState=inactive'
         # #|   actual:   'ActiveState=failed'

@rhatdan
Copy link
Member

rhatdan commented Aug 31, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2022
@openshift-merge-robot openshift-merge-robot merged commit 9b4dac4 into containers:main Aug 31, 2022
@github-actions 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 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new --userns flag --userns=sync-id
7 participants