Hi @rata. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

cc @halaney in case you are interested in this PR too :)

@halaney Thanks for the review, added the before/after to the PR description.

IIUC let's say the host has an empty host volume /run/ owned by 0, container has /run/foobar file which is owned by uid/gid 0/0. Let's say the {u,g}idmap is 0 on host to 65556 in container. Without this change we'd:

I think you had a typo here, it's 0 inside the container mapped to 65536 on the host (or whatever hostID). This is what the mapping is doing.

3. The result is the host's /run/foobar is now owned by 65556, not 0 as expected. 0 is expected since as you mention the container's user namespace mapping will effectively undo the idmap mapping from its pov, resulting in 65556 being 0 in the actual container. When we actually do the final mount in the container proper, its also a idmap'ed bind mount onto the overlayfs right? So it will also be all messed up in the container's POV (it'll be shifted an extra 65556 resulting in user 65556 owning it within the container as well)?

Not 100% sure what you mean its also idmapped bind mount onto the rootfs, but IIUC nope. There are no "nested" idmapping here, if that is what you are saying. The rootfs is overlay and is idmapped, this is a bind-mount and is idmapped, but those idmappings don't nest and no AT_RECURSIVE is used either.

Also, overlayfs doesn't support to idmap the merged dir, what it supports is to idmap the lowerdir/upperdir. So, we can't really use AT_RECURSIVE either here.

So either we need to idmap the volume too, or not do the idmap on the overlayfs. So your approach seems like the path of least work!

No, the overlayfs always needs to be idmap. As I said, they are not nested, they don't interfere, nor idmap is recursive either.

It could work to have "the right permissions" for the directory when you look only at this layer, but it doesn't when you look at the bigger picture. The hostUID is dynamic, you don't want to chown all the files every time you get a new mapping. You want pods to be able to enable/disable userns, but you can't unless you chown all the volumes with that solution, etc.

The current design is simpler, we just idmap when running, all works as expected, no chown ever required.
This is not done only in this part (for image volume directives), but all the way in all volume files (even configmaps, etc.).

@rata me explaining myself below, but tldr we're saying the same thing I just wrote it confusingly earlier (and maybe still) below):

Not 100% sure what you mean its also idmapped bind mount onto the rootfs, but IIUC nope. There are no "nested" idmapping here, if that is what you are saying. The rootfs is overlay and is idmapped, this is a bind-mount and is idmapped, but those idmappings don't nest and no AT_RECURSIVE is used either.

Nesting is not what I meant, your description matches what I was attempting (:P) to say. My bits about:

So either we need to idmap the volume too, or not do the idmap on the overlayfs. So your approach seems like the path of least work!

was trying to express that to fix the current problem either we don't want to idmap the overlayfs for this WithVolume operation (what you opted for), or we need to idmap mount both the overlayfs (really its lower dirs but just using that term for the whole thing) and the volume, copying between those two idmap'ed views to keep the users consistent. i.e. both the source and destination need to be idmapped or neither should be idmapped, not mixed.

Here in WithVolume we can just skip it as it provides no benefit, later when the container runs we need to idmap both because the from the host's pov the container is running as user 100000 in example below and needs to be given a view of the mounts that's accessible to user 100000 via the idmaps.

i.e. before it was something like

idmap mount 0 -> 100000 of overlayfs at /tmp/idmapped-overlayfs
cp -R /tmp/idmapped-overlayfs/<volume>/ /<volume>/
# this results in /<volume>/ containing files owned by 100000 instead 
# of 0 (assuming everything is originally owned by 0 before idmaps)
...
# make real container rootfs mounts:
idmap mount 0 -> 100000 of overlayfs at /var/lib/container/.../rootfs
idmap mount 0 -> 100000 of <volume> at /var/lib/container/.../rootfs/<volume>
# this results in  /var/lib/container/.../rootfs/<volume> contents being owned 
# by 200000 instead of intended 100000 and within the container this would look
# like user 100000 owns it instead of 0 as expected (or nobody user if 100000 is
# out of uidmap range) due to user namespace mapping

now things work since we don't shift them at the beginning. Alternatively, you could do the copy content with both locations idmapped and that would also work out consistently. Its the half doing of it that's the problem, and since there's no reason to do it in WithVolumes this PR the best solution to the problem.

Sorry for confusion! this looks good to me, thanks for the extra details on the MR description, if I'm still not making sense just ignore me we're trying to say the same thing :D

Does this need backporting?

/cherry-pick release/2.1 release/2.0

@rata: only containerd org members may request cherry picks. If you are already part of the org, make sure to change your membership to public. Otherwise you can still do the cherry-pick manually.

Opened backport PRs manually: #12241 and #12242