Support helpers for label-based userns remapping#4259
Support helpers for label-based userns remapping#4259dmcgowan merged 3 commits intocontainerd:masterfrom
Conversation
|
ping @AkihiroSuda @dmcgowan : this is my attempt to take the comments from #3885 and modify the code to move away from special remapper flags. Given there are no callers in containerd, it seems up to custom clients to determine how to utilize the remapping helpers anyway. |
estesp
force-pushed
the
fuse-overlayfs
branch
2 times, most recently
from
8b73ac5 to
9ffbc69
Compare
AkihiroSuda
left a comment
AkihiroSuda
left a comment
There was a problem hiding this comment.
Thanks, but I think function names should not contain "fuse".
Also, can we have ctr flag for this?
estesp
changed the title
There was a problem hiding this comment.
How come this is mapping everything after uid?
Can we scope this down?
Also just to note, this is specifically a root remap.
There was a problem hiding this comment.
First, this isn't anything to do with the actual user namespace mapping; that's handled via the OCI spec and related options. Our filesystem "shifting" (to complement the userns mappings provided to the spec) was patterned after a more simplistic monotonic UID/GID shift (as if your /etc/sub{u,g}id files had the most default setup found in most distros--a single entry, starting at 100,000--as an example--and mapping the next 64k). You can see incrementFS in this same file showing that same simple model implementation.
This new function is to support letting a snapshotter handle the filesystem ownership mapping via non-brute force/chown methods (e.g. FUSE overlay support, experimental shiftfs support in Ubuntu, etc.), and so we are starting with the same simple idea of providing the base, offset, and a static 64k block (again, in line with defaults on most distros). I think @AkihiroSuda mentioned that future implementations will need to support more interesting use cases (e.g. just like uid maps can be non-contiguous and different sizes than the default 64k). How the actual snapshotter impl. uses the data in the label is really up to the snapshotter.
|
I stick with my original recommendation in |
|
@dmcgowan clearly I should have asked since this implementation was my exact following of your comment "provide a helper which can set the labels and just use |
estesp
force-pushed
the
fuse-overlayfs
branch
2 times, most recently
from
1e1955c to
c919171
Compare
|
I think I have this correctly now @dmcgowan. Per @AkihiroSuda's comment I'm going to add another commit with a |
|
Can we have either ctr flag or an integration test? (having ctr would be probably easier) |
|
@AkihiroSuda yes, working on it today (a |
Signed-off-by: Jie Hao Liao <[email protected]>
estesp
force-pushed
the
fuse-overlayfs
branch
2 times, most recently
from
4f8ad6a to
f1b1f96
Compare
Provide a snapshotter opt to add labels used by any supporting snapshotter to handle user namespace filesystem remapping. Currently supported by the fuse-overlayfs snapshotter, and others can use this information as well. Signed-off-by: Phil Estes <[email protected]>
|
@AkihiroSuda interestingly I started this branch from the PR I carried, which left me with the impression that there wasn't already support for running a container with UID/GID maps, but that PR (from the same contributor) was merged soon after this original PR was created. Rebasing on master, I now have the uid/gid maps on |
Yes, I prefer the extra flag, thanks |
A simple starting point for testing the remapper labels with fuse-overlayfs snapshotter Signed-off-by: Phil Estes <[email protected]>
|
@AkihiroSuda done! |
|
Build succeeded.
|
| } | ||
| opts = append(opts, | ||
| oci.WithUserNamespace([]specs.LinuxIDMapping{uidMap}, []specs.LinuxIDMapping{gidMap})) | ||
| if context.Bool("read-only") { |
There was a problem hiding this comment.
Should this remain as an else if?
There was a problem hiding this comment.
I was aligning the user ns path with the "main" path which has this comment a few lines below: "Even when "read-only" is set, we don't use KindView snapshot here. (#1495)"; this was never applied for the userns path when it was added, but I believe the same problem will exist (runc won't be able to make mount points because of a read only root fs; but is able to remount it ro once that work is completed at the runc level)
5 participants
Carry #3885
Modified the original implementation to simply have unique helpers for the label-based approach to supporting remapping snapshotters. We don't have any callers of either of these helpers in containerd today.
Added commit: "Provide remapping helpers to add labels used by any supporting
snapshotter to handle user namespace filesystem remapping. Currently
supported by the fuse-overlayfs snapshotter, and others can use this
information as well."