Add user namespace ID mapping support for overlaybd - #398
Conversation
There was a problem hiding this comment.
Pull request overview
Adds opt-in user-namespace / ID-mapped mount support to the overlaybd snapshotter when remapIDs is enabled, aiming to avoid containerd’s expensive chown-walk fallback for userns workloads while keeping default behavior unchanged.
Changes:
- Introduces
remapIDssnapshotter config gating, enabled only after a host support probe (kernel/overlay idmap support, d_type, userns FD creation). - Adds logic to pre-idmap overlaybd block-device lowers (via
mount.GetUsernsFD+mount.IDMapMount) and propagatesuidmap=/gidmap=overlay options for normal overlay mounts when labels are present. - Adjusts upperdir ownership based on mapping labels and unmounts the
idmapped-lowermount during snapshot removal; adds unit tests around mapping parsing / option propagation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/snapshot/overlay.go | Core remapIDs wiring, idmapped-lower handling for block lowers, uid/gid mapping option propagation, ownership adjustments, and removal cleanup. |
| pkg/snapshot/overlay_test.go | Adds unit tests for mapping parsing and option propagation (plus a lightweight option-prefix helper). |
| pkg/snapshot/idmap_linux.go | Linux-only host support probe for enabling remapIDs. |
| pkg/snapshot/idmap_linux_test.go | Tests that remapIDs only enables when the support probe succeeds. |
| pkg/snapshot/docker.go | Updates one docker fallback path to the new normalOverlayMount signature (but another call site still needs updating). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| remapIDs := false | ||
| if bootConfig.RemapIDs { | ||
| supported, err := remapSupportProbe(root) | ||
| if err != nil { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
pkg/snapshot/overlay.go:1231
- If
mount.IDMapMountfails due to the destination already being mounted (e.g., concurrent calls for the same snapshot), this currently falls back to the unmapped lower even though the idmapped mount may exist. Re-check whetherdstis mounted on error and treat that case as success to avoid inconsistent behavior and potential double-shifting.
if err := mount.IDMapMount(original, dst, int(usernsFd.Fd())); err != nil {
log.G(ctx).WithError(err).Warn("failed to idmap block device mountpoint, using unmapped block lower")
return original, nil
}
pkg/snapshot/overlay.go:1510
createSnapshotonly appliesLchownwhen bothmappedUIDandmappedGIDare set. If one mapping parses successfully and the other doesn’t (or is absent), ownership is left at the default even thoughos.Lchownsupports passing-1to leave one side unchanged. This can lead to incorrect upperdir ownership in partially-specified/partially-parsed mapping cases.
if mappedUID != -1 && mappedGID != -1 {
if err := os.Lchown(filepath.Join(td, "fs"), mappedUID, mappedGID); err != nil {
return "", snapshots.Info{}, fmt.Errorf("failed to chown: %w", err)
}
}
pkg/snapshot/overlay.go:268
remapSupportProbeis only defined inidmap_linux.go(linux build tag), butoverlay.gois built on all platforms. This makes non-linux builds fail with an undefined identifier. Provide a!linuxstub forremapSupportProbe(returningfalse, nil), or move the variable definition to a non-tagged file and override it from linux-only code.
remapIDs := false
if bootConfig.RemapIDs {
supported, err := remapSupportProbe(root)
if err != nil {
pkg/snapshot/overlay_test.go:167
- This test name says it covers
basedOnBlockDeviceMount, but it never calls that function (it only checks a locally-constructed options slice). Rename the test to reflect what it actually verifies, or update it to exercisebasedOnBlockDeviceMountdirectly.
func TestBasedOnBlockDeviceMount_omitsOverlayIdmapWhenLowerPreMapped(t *testing.T) {
Signed-off-by: Archana Choudhary <archana.choudhary.9693@gmail.com>
Signed-off-by: Archana Choudhary <archana.choudhary.9693@gmail.com>
Signed-off-by: Archana Choudhary <archana.choudhary.9693@gmail.com>
|
|
||
| // Normal image: fall back to standard overlay mount | ||
| // s.ParentIDs already contains [initLayerID, imageLayer1ID, ...] | ||
| return o.normalOverlayMount(s), nil | ||
| return o.normalOverlayMount(s, info), nil | ||
| } |
| if v, ok := info.Labels[labelSnapshotUIDMapping]; ok { | ||
| options = append(options, "uidmap="+v) | ||
| } | ||
| if v, ok := info.Labels[labelSnapshotGIDMapping]; ok { | ||
| options = append(options, "gidmap="+v) | ||
| } |
| // remapSupportProbe is overridden in tests on Linux. | ||
| var remapSupportProbe = detectRemapIDsSupport |
|
Hey @BigVan , Can you please review? |
What this PR does / why we need it:
This PR adds opt-in user namespace / ID-mapped mount support to overlaybd-snapshotter when remapIDs is enabled in the snapshotter config.
This change:
remapIDssnapshotter config option (default:false) and enables it only after a host support probe (kernel ID-mapped overlay support, d_type on the snapshotter root, and user namespace FD creation).snapshots/<active-id>/block/idmapped-lowervia containerd’smount.GetUsernsFDandmount.IDMapMount, then uses that path as overlaylowerdirwithout overlayuidmap/gidmap(avoids double-shifting).uidmap/gidmapmount options from snapshot labels for normal overlay mounts whenremapIDsis enabled.idmapped-lowerduring snapshot removal.Without
--remap-labels, existing containerd chown/remap behavior is unchanged. WithremapIDs: false(default), behavior is unchanged for all workloads.Fixes #354
Please check the following list:
Test plan
Unit tests
go test ./pkg/snapshot/... -count=1Manual (overlaybd image +
remapIDs: truein/etc/overlaybd-snapshotter/config.json)Fast path with
--remap-labels:idmapped block device mount: ... -> .../idmapped-lowerfindmntshowslowerdir=.../idmapped-lowerBackward-compatible slow path without
--remap-labels:Two concurrent containers with different maps show different host UIDs on files under
.../block/idmapped-lower/bin/sh(e.g. 100000 vs 200000).After
--rm,idmapped-lowerfor that snapshot is removed.