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.

Sign up for GitHub

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

Jump to bottom

Support multiple uid/gid mappings [1/2] #10307

Merged
merged 1 commit into from
Sep 23, 2024
Merged

Support multiple uid/gid mappings [1/2] #10307

merged 1 commit into from
Sep 23, 2024

Conversation

henry118
Copy link
Member

@henry118 henry118 commented Jun 6, 2024 •
edited
Loading

Enhance user namespace implementation to support multi-entry uid/gid mappings.

This is 1st patch (out of 2) that supports multiple entries of uid/gid configurations. This patch focuses on the "slow" path that requires chown on every file in the root FS. uidmapped mounts implementation will be covered in a separate PR.

This implementation has changes in the following area:

  1. Ported the IdentityMapping implementation from idtools package in Moby.
  2. Added a few container opt funcs to take multiple uid/gid mappings.
  3. Allowing multiple --uidmap, --gidmap options in ctr run command.
  4. Fixed a bug where special file permissions like setuid, setgid or sticky bits are lost during chown

An sample command to run a container with remapped foo user:

ctr run --uidmap 0:100000:1000 --uidmap 1000:201795:1 --uidmap 1001:10000000:64535 --gidmap 0:100000:100 --gidmap 100:1001:1 --gidmap 101:10000000:65435 --user foo <image> test bash

I hope the implementation makes sense and I'm looking forward to your feedback :)

@henry118 henry118 changed the title update ctr run to support multiple uid/gid mappings Support multiple uid/gid mappings Jun 6, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 6, 2024
View reviewed changes
@henry118 henry118 changed the title Support multiple uid/gid mappings Support multiple uid/gid mappings [1/2] Jun 6, 2024
@henry118 henry118 marked this pull request as draft June 13, 2024 17:23
@henry118 henry118 force-pushed the uidmap branch 2 times, most recently from 2e0a5a2 to ac777c9 Compare June 19, 2024 22:20
@henry118 henry118 marked this pull request as ready for review June 19, 2024 22:31
@henry118 henry118 force-pushed the uidmap branch 3 times, most recently from 26107e7 to 4e7f0ea Compare June 24, 2024 20:03
@estesp
Copy link
Member

estesp commented Jun 28, 2024

/test pull-containerd-node-e2e

@estesp estesp added this to the 2.0 milestone Jun 28, 2024
@estesp
Copy link
Member

estesp commented Jun 28, 2024

I'm adding to 2.0 milestone; @containerd/maintainers let me know if anyone thinks it should not be in 2.0

@henry118 henry118 force-pushed the uidmap branch 2 times, most recently from 0af7ad4 to 56972bc Compare July 3, 2024 17:39
@henry118
Copy link
Member Author

henry118 commented Jul 3, 2024

/test pull-containerd-node-e2e

@AkihiroSuda
Copy link
Member

@henry118 Do you plan to add the second part in this PR? Or will it be a separate PR?

@AkihiroSuda
Copy link
Member

Needs tests

@estesp
Copy link
Member

estesp commented Jul 31, 2024

@henry118 Do you plan to add the second part in this PR? Or will it be a separate PR?

He was going to do it as a separate PR; he is out right now so it will be after he returns

@henry118
Copy link
Member Author

@rata

CRI should only use idmap mounts if the kernel and the snapshotter support it. If you are using an old kernel, it will use chown. This is indeed what is happening in CI when the kernel is old.

Yes I am aware of the fallback. The fallback is currently implemented in resolveSnapshotOptions. The entry point of this func is based on the idmapping labels configured on snapshotters, which is still limited to single mapping entry without my 2nd PR.

containerd/client/snapshotter_opts_unix.go

Line 113 in 7b948fa

if err := remapRootFS(ctx, mounts, hostUID, hostGID); err != nil {

If this is targeted for 2.0, I'm unsure we want to do this change now for CRI, though. What do other maintainers think?

We can discuss about how we want to support CRI in my next PR that wires up the CRI logic. Basically if we don't want to change the CRI logic, we can simply keep using the old API in CRI. But I don't think the discussion is relevant to this PR though.

containerd/internal/cri/server/helpers_linux.go

Line 206 in 7b948fa

snapshotOpt = append(snapshotOpt, containerd.WithRemapperLabels(0, uids[0].HostID, 0, gids[0].HostID, uids[0].Size))

@rata
Copy link
Contributor

rata commented Aug 27, 2024

@henry118 okay, you were saying the opposite before (CRI userns required idmap mounts) :)

If you plan to do a follow-up PR with the idmap mounts part and prefer to wire CRI there, it seems fine to me. I think the final release should be close and changing that later seems safer, IMHO, besides other PRs that are inflight that would conflict with this. But I'm not a maintainer here :)

@henry118
Copy link
Member Author

@rata

I think the final release should be close and changing that later seems safer, IMHO, besides other PRs that are inflight that would conflict with this. But I'm not a maintainer here :)

My 2nd PR is pretty much ready. I didn't open it because GH does not support stacked PRs. And I wanted to use the 1st PR to align on all the APIs. I wish that I can still catch the 2.0 release window to get this feature in. But I agree it's up to the maintainers. Again let's defer this discussion to the follow-up patch.

I believe I should have addressed all your comments at this stage. PTAL and let me know what else I can do to move this further.

@rata
Copy link
Contributor

rata commented Aug 28, 2024

@henry118 cool, thanks! Just to understand, would you like this PR for 2.0? Or also the follow-up PR?

I think maintainers need to review now, that is the next step :)

@henry118
Copy link
Member Author

henry118 commented Aug 28, 2024 •
edited
Loading

Ping @estesp @AkihiroSuda @dmcgowan @fuweid PTAL. Really hope this enhancement can be part of 2.0.

estesp
estesp approved these changes Sep 3, 2024
View reviewed changes
Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp
Copy link
Member

estesp commented Sep 3, 2024

@dmcgowan would love your review and yours or any @containerd/committers opinion on 2.0 inclusion. It improves our general user namespaces support to handle ranges, which is a core feature of user namespaces in Linux and has a follow-on change for the CRI as well that @henry118 has ready to go.

cpuguy83
cpuguy83 requested changes Sep 10, 2024
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

I think I'd prefer the idmap type to be an internal type.
Other than that this seems OK

@henry118
Copy link
Member Author

I think I'd prefer the idmap type to be an internal type. Other than that this seems OK

@cpuguy83 thanks for the input. I moved the userns package under internals.

@fuweid fuweid added this pull request to the merge queue Sep 23, 2024
Merged via the queue into containerd:main with commit 906c232 Sep 23, 2024
52 checks passed
@henry118 henry118 deleted the uidmap branch September 23, 2024 15:30
@henry118 henry118 mentioned this pull request Sep 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rata rata rata left review comments

@Kern-- Kern-- Kern-- left review comments

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@estesp estesp estesp approved these changes

@fuweid fuweid fuweid approved these changes

Assignees
No one assigned
Labels
size/XL
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.
8 participants
@henry118 @estesp @AkihiroSuda @rata @cpuguy83 @Kern-- @fuweid @k8s-ci-robot