Merge pull request #12816 from AutuSnow/fix-image-volume-userns

cri: Fix image volumes with user namespaces

integration/image_volume_linux_test.go

@@ -28,6 +28,7 @@ import (
 	"github.com/containerd/containerd/v2/integration/images"
 	kernel "github.com/containerd/containerd/v2/pkg/kernelversion"
 	"github.com/containerd/containerd/v2/pkg/namespaces"
+	"github.com/containerd/containerd/v2/pkg/sys"
 	"github.com/containerd/errdefs"
 	"github.com/opencontainers/image-spec/identity"
 	"github.com/opencontainers/selinux/go-selinux"
@@ -324,3 +325,69 @@ func TestImageVolumeSetupIfContainerdRestarts(t *testing.T) {
 		})
 	}
 }
+
+func TestImageVolumeWithUserNamespace(t *testing.T) {
+	// Check if user namespace and idmap are supported
+	if !supportsUserNS() {
+		t.Skip("user namespace not supported")
+	}
+
+	// Check if pidfd is supported
+	if !sys.SupportsPidFD() {
+		t.Skip("pidfd not supported")
+	}
+
+	if !supportsIDMap(defaultRoot) {
+		t.Skipf("idmap mounts not supported on: %s", defaultRoot)
+	}
+
+	containerID := uint32(0)
+	hostID := uint32(65536)
+	size := uint32(65536)
+
+	containerImage := images.Get(images.Alpine)
+	imageVolumeImage := images.Get(images.Pause)
+
+	podLogDir := t.TempDir()
+	podOpts := []PodSandboxOpts{
+		WithPodLogDirectory(podLogDir),
+		WithPodUserNs(containerID, hostID, size),
+	}
+	podCtx := newPodTCtx(t, runtimeService, t.Name(), "image-volume-userns", podOpts...)
+	defer podCtx.stop(true)
+
+	pullImagesByCRI(t, imageService, containerImage, imageVolumeImage)
+
+	// Create a container with image volume mount
+	// Pass the user namespace ID mappings to the image volume mount so that
+	// idmap is applied and files appear with correct ownership in the container
+	uidMaps := []*criruntime.IDMapping{{ContainerId: containerID, HostId: hostID, Length: size}}
+	gidMaps := []*criruntime.IDMapping{{ContainerId: containerID, HostId: hostID, Length: size}}
+
+	containerName := "test-container"
+	cfg := ContainerConfig(containerName, containerImage,
+		WithCommand("sleep", "1d"),
+		WithIDMapImageVolumeMount(imageVolumeImage, "", "/image-mount", uidMaps, gidMaps),
+		WithLogPath(containerName),
+		WithUserNamespace(containerID, hostID, size),
+	)
+	cnID, err := podCtx.rSvc.CreateContainer(podCtx.id, cfg, podCtx.cfg)
+	require.NoError(t, err, "failed to create container with image volume and user namespace")
+
+	require.NoError(t, podCtx.rSvc.StartContainer(cnID), "failed to start container")
+
+	// Verify that the image volume is accessible
+	stdout, stderr, err := runtimeService.ExecSync(cnID, []string{"ls", "/image-mount/pause"}, 0)
+	require.NoError(t, err, "failed to access image volume")
+	require.Len(t, stderr, 0)
+	require.Contains(t, string(stdout), "pause", "image volume should contain pause binary")
+
+	_, _, err = runtimeService.ExecSync(cnID, []string{"rm", "/image-mount/pause"}, 0)
+	require.Error(t, err, "image volume should be read-only")
+	require.Contains(t, err.Error(), "Read-only file system", "error should indicate read-only filesystem")
+
+	stdout, stderr, err = runtimeService.ExecSync(cnID, []string{"stat", "-c", "=%u=%g=", "/image-mount/pause"}, 0)
+	require.NoError(t, err, "failed to stat file in image volume")
+	require.Len(t, stderr, 0)
+	require.Contains(t, string(stdout), "=0=0=", "files in image volume should appear as owned by root in container's user namespace")
+}

internal/cri/server/container_image_mount.go

@@ -124,8 +124,14 @@ func (c *criService) mutateImageMount(
 		}
 		chainID := identity.ChainID(diffIDs).String()
 
+		// Get snapshot options with user namespace idmap labels if needed
+		snapshotOpts, err := c.getImageVolumeSnapshotOpts(ctx, extraMount)
+		if err != nil {
+			return fmt.Errorf("failed to get snapshot options for image volume: %w", err)
+		}
+
 		s := c.client.SnapshotService(snapshotter)
-		mounts, err := s.Prepare(ctx, target, chainID)
+		mounts, err := s.Prepare(ctx, target, chainID, snapshotOpts...)
 		if err != nil {
 			if errdefs.IsAlreadyExists(err) {
 				mounts, err = s.Mounts(ctx, target)
@@ -160,6 +166,15 @@ func (c *criService) mutateImageMount(
 	}
 
 	extraMount.HostPath = target
+
+	// Clear UID/GID mappings from the mount to prevent the OCI runtime from
+	// attempting idmap on the bind mount. The idmap is already applied to the
+	// overlay lower layers via the snapshotter when the image volume is prepared.
+	// This must be done regardless of whether the image volume was already mounted
+	// (e.g., by another container in the same pod).
+	extraMount.UidMappings = nil
+	extraMount.GidMappings = nil
+
 	return nil
 }
 

internal/cri/server/container_image_mount_linux.go

@@ -19,12 +19,16 @@
 package server
 
 import (
+	"context"
 	"fmt"
 	"os"
 	"sync"
 
+	containerd "github.com/containerd/containerd/v2/client"
 	"github.com/containerd/containerd/v2/core/mount"
+	"github.com/containerd/containerd/v2/core/snapshots"
 	kernel "github.com/containerd/containerd/v2/pkg/kernelversion"
+	runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
 )
 
 var (
@@ -90,3 +94,26 @@ func ensureImageVolumeMounted(target string) (bool, error) {
 	}
 	return true, nil
 }
+
+// getImageVolumeSnapshotOpts returns snapshot options with user namespace idmap labels
+// from the mount's UID/GID mappings. This ensures that image volumes work correctly
+// with user namespaces by applying idmap to the overlay lower layers.
+func (c *criService) getImageVolumeSnapshotOpts(ctx context.Context, mount *runtime.Mount) ([]snapshots.Opt, error) {
+	uids, err := parseUsernsIDMap(mount.GetUidMappings())
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse UID mappings: %w", err)
+	}
+
+	gids, err := parseUsernsIDMap(mount.GetGidMappings())
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse GID mappings: %w", err)
+	}
+
+	if len(uids) == 0 || len(gids) == 0 {
+		return nil, nil
+	}
+
+	return []snapshots.Opt{
+		containerd.WithRemapperLabels(0, uids[0].HostID, 0, gids[0].HostID, uids[0].Size),
+	}, nil
+}

internal/cri/server/container_image_mount_linux_test.go

@@ -0,0 +1,148 @@
+//go:build linux
+
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package server
+
+import (
+	"context"
+	"testing"
+
+	containerd "github.com/containerd/containerd/v2/client"
+	"github.com/containerd/containerd/v2/core/snapshots"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
+)
+
+func TestGetImageVolumeSnapshotOpts(t *testing.T) {
+	ctx := context.Background()
+
+	mappings := []*runtime.IDMapping{
+		{
+			ContainerId: 0,
+			HostId:      65536,
+			Length:      65536,
+		},
+	}
+
+	expectedOpts := optsToInfo(t, containerd.WithRemapperLabels(0, 65536, 0, 65536, 65536))
+
+	for _, test := range []struct {
+		name        string
+		mount       *runtime.Mount
+		expectInfo  *snapshots.Info
+		expectError bool
+	}{
+		{
+			name: "with user namespace mappings",
+			mount: &runtime.Mount{
+				ContainerPath: "/test",
+				UidMappings:   mappings,
+				GidMappings:   mappings,
+			},
+			expectInfo: expectedOpts,
+		},
+		{
+			name: "without mappings",
+			mount: &runtime.Mount{
+				ContainerPath: "/test",
+			},
+		},
+		{
+			name: "with empty mappings",
+			mount: &runtime.Mount{
+				ContainerPath: "/test",
+				UidMappings:   []*runtime.IDMapping{},
+				GidMappings:   []*runtime.IDMapping{},
+			},
+		},
+		{
+			name: "with only UID mappings",
+			mount: &runtime.Mount{
+				ContainerPath: "/test",
+				UidMappings:   mappings,
+			},
+		},
+		{
+			name: "with only GID mappings",
+			mount: &runtime.Mount{
+				ContainerPath: "/test",
+				GidMappings:   mappings,
+			},
+		},
+		{
+			name: "with multiple UID mapping lines",
+			mount: &runtime.Mount{
+				ContainerPath: "/test",
+				UidMappings: []*runtime.IDMapping{
+					{
+						ContainerId: 0,
+						HostId:      65536,
+						Length:      65536,
+					},
+					{
+						ContainerId: 65536,
+						HostId:      131072,
+						Length:      65536,
+					},
+				},
+				GidMappings: mappings,
+			},
+			expectError: true,
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			c := &criService{}
+
+			opts, err := c.getImageVolumeSnapshotOpts(ctx, test.mount)
+
+			if test.expectError {
+				assert.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+
+			gotInfo := optsToInfo(t, opts...)
+			if test.expectInfo == nil {
+				assert.Nil(t, gotInfo)
+			} else {
+				require.NotNil(t, gotInfo)
+				assert.Equal(t, test.expectInfo.Labels, gotInfo.Labels)
+			}
+
+			if test.mount.UidMappings != nil {
+				assert.NotNil(t, test.mount.UidMappings, "UidMappings should NOT be cleared by getImageVolumeSnapshotOpts")
+			}
+			if test.mount.GidMappings != nil {
+				assert.NotNil(t, test.mount.GidMappings, "GidMappings should NOT be cleared by getImageVolumeSnapshotOpts")
+			}
+		})
+	}
+}
+
+func optsToInfo(t *testing.T, opts ...snapshots.Opt) *snapshots.Info {
+	t.Helper()
+	if len(opts) == 0 {
+		return nil
+	}
+	var info snapshots.Info
+	for _, opt := range opts {
+		require.NoError(t, opt(&info))
+	}
+	return &info
+}

internal/cri/server/container_image_mount_other.go

@@ -19,10 +19,13 @@
 package server
 
 import (
+	"context"
 	"fmt"
 	"os"
 
 	"github.com/containerd/containerd/v2/core/mount"
+	"github.com/containerd/containerd/v2/core/snapshots"
+	runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
 )
 
 // addVolatileOptionOnImageVolumeMount is no-op on non-linux platforms.
@@ -42,3 +45,7 @@ func ensureImageVolumeMounted(target string) (bool, error) {
 	}
 	return true, nil
 }
+
+func (c *criService) getImageVolumeSnapshotOpts(ctx context.Context, mount *runtime.Mount) ([]snapshots.Opt, error) {
+	return nil, nil
+}