You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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:
Adds a remapIDs snapshotter 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).
For overlaybd block lowers, pre-idmaps the parent block mount to snapshots/<active-id>/block/idmapped-lower via containerd’s mount.GetUsernsFD and mount.IDMapMount, then uses that path as overlay lowerdir without overlay uidmap/gidmap (avoids double-shifting).
Propagates uidmap/gidmap mount options from snapshot labels for normal overlay mounts when remapIDs is enabled.
Sets writable upperdir ownership to the mapped container root when mapping labels are present.
Unmounts idmapped-lower during snapshot removal.
Without --remap-labels, existing containerd chown/remap behavior is unchanged. With remapIDs: false (default), behavior is unchanged for all workloads.
The reason will be displayed to describe this comment to others. Learn more.
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 remapIDs snapshotter 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 propagates uidmap=/gidmap= overlay options for normal overlay mounts when labels are present.
Adjusts upperdir ownership based on mapping labels and unmounts the idmapped-lower mount 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).
The reason will be displayed to describe this comment to others. Learn more.
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.IDMapMount fails 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 whether dst is 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
createSnapshot only applies Lchown when both mappedUID and mappedGID are set. If one mapping parses successfully and the other doesn’t (or is absent), ownership is left at the default even though os.Lchown supports passing -1 to 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
remapSupportProbe is only defined in idmap_linux.go (linux build tag), but overlay.go is built on all platforms. This makes non-linux builds fail with an undefined identifier. Provide a !linux stub for remapSupportProbe (returning false, 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 exercise basedOnBlockDeviceMount directly.
Reject invalid negative container IDs before applying ownership
pkg/snapshot/overlay.go:1427
This condition accepts invalid negative container IDs (for example -1:1000:2) and computes a host root ID even though GetUsernsFD will reject the same mapping. That can leave the writable upper directory chowned to an ID mapping that is never used. Only accept a valid range starting at container ID 0 (and reject non-positive sizes/negative IDs) before applying the mapped ownership.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.