-
Notifications
You must be signed in to change notification settings - Fork 654
feat: add support for userns #3941
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
feat: add support for userns #3941
Conversation
@@ -279,6 +279,7 @@ func setCreateFlags(cmd *cobra.Command) { | |||
cmd.Flags().String("ipfs-address", "", "multiaddr of IPFS API (default uses $IPFS_PATH env variable if defined or local directory ~/.ipfs)") | |||
|
|||
cmd.Flags().String("isolation", "default", "Specify isolation technology for container. On Linux the only valid value is default. Windows options are host, process and hyperv with process isolation as the default") | |||
cmd.Flags().String("userns", "", "Support idmapping of containers. This options is only supported on linux. If `host` is passed, no idmapping is done. if a user name is passed, it does idmapping based on the uidmap and gidmap ranges specified in /etc/subuid and /etc/subgid respectively") |
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.
Please add docs/command-reference.md
.
Also, the command line seems incompatible with Docker?
Docker doesn't accept a username here, and the name is hardcoded to "dockremap".
Maybe we should have its equivalent as "nerdremap"?
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.
Podman accepts --subuidname string --subgidname string
to specify a custom user name
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.
Also, the command line seems incompatible with Docker?
if they add host
it will behave as docker as we check for that string and create the default snapshot.
For other names it behaves as docker daemon but at a container level rather than at daemon level. Will you suggest we configure it in nerdctl config instead?
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.
For other names it behaves as docker daemon but at a container level rather than at daemon level. Will you suggest we configure it in nerdctl config instead?
Eventually, the both level should be supported, as in Podman: https://github.com/containers/podman/blob/v5.4.1/docs/source/markdown/options/userns.container.md?plain=1
podman run --userns=auto
allocates subuids from the "containers" entry in/etc/subuid
.- When
userns=...
is specified incontainers.conf
, Podman enables UserNS globally, unless--userns=host
is specified.
nerdctl should probably follow the same convention, but s/containers.conf/nerdctl.toml/
Not all the features need to be implemented at once. Can just begin with the easiest one.
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.
ping @Shubhranshu153
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 does sound good to me in terms of supporting all the things that podman supports but would like to create a separate PR as this one has already been quite long. Can probably get in by 2.1.
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.
No need to support all the things, but --userns=<USERNAME>
is a new syntax that is not adopted by either Docker nor Podman, and is hard to adopt here unless there is a strong reason
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.
dockerd --userns-remap="testuser:testuser"
For docker it is a docker daemon entry although i have not added all the different option and patterns to support. Should i add userns-remap instead of userns?
Docker has a daemon so it applies it to all containers launched for the daemon.
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.
userns-remap
SGTM.
It should be also available in /etc/nerdctl/nerdctl.toml
https://github.com/containerd/nerdctl/blob/main/docs/config.md#properties
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.
sure i can add to the nerdctl.toml
e094328
to
068faa5
Compare
d2b2c70
to
3279901
Compare
@Shubhranshu153 I need to do some reading on userns to get a better grasp first, but short term, happy to help you review testing / overall. Hit me up when ready. |
4f8ae7b
to
bcc209d
Compare
bcc209d
to
b962609
Compare
Almalinux test seems to be flaky, |
Mmmm... failure is #4132 - but 4 fails out of 4 tries is no longer "flaky", it is a cold hard clusterf... Do you mind closing and reopening? |
ok this is repeatable. |
this test is racy in sense we cant guarantee the echo to complete before checking the logs. Needs to wait or retry |
653a2d3
to
52f8831
Compare
For test create: |
52f8831
to
2552378
Compare
Not to de-sanitize this PR will create a separate PR to improve the TestCreate, for other bugs i still dont know what is the flakiness. |
Thanks a lot @Shubhranshu153 - the log you just identified is #3717 Our other main PITA (especially on EL) is #4132 Your latest Docker issue is some networking issue connecting to ghcr.io The motherload (most known flaky issues) is here: #4120 |
@apostasie ok got it, i think this is great to have a list to be able to focus on. I will try to pick some of these. |
Rerunning the test for flaky test. |
docs/command-reference.md
Outdated
@@ -235,6 +235,9 @@ User flags: | |||
- :nerd_face: `--umask`: Set the umask inside the container. Defaults to 0022. | |||
Corresponds to Podman CLI. | |||
- :whale: `--group-add`: Add additional groups to join | |||
- :nerd_face: `--userns-remap=<username>:<groupname>`: Support idmapping of containers. This options is only supported on rootful linux for container create and run if a user name and optionally group name is passed, it does idmapping based on the uidmap and gidmap ranges specified in /etc/subuid and /etc/subgid respectively. Note: `--userns-remap` is not supported for building containers. Nerdctl Build doesn't support userns-remap feature. (format: <name|uid>[:<group|gid>]) |
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.
Isn't this a top-level flag?
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.
you are right after the refactor. let me update it.
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.
Updated.
Signed-off-by: Shubharanshu Mahapatra <[email protected]>
2552378
to
38d67f0
Compare
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
Adds support for running containers with custom user namespace mappings through
the --userns flag in 'run' and 'create' commands.
Key Features:
Technical Details:
Dependencies:
Testing:
This enhancement improves container isolation by providing
flexible user namespace mapping capabilities.
Size: XL