From ec2bec6481d5ad53a2a21a7ad1a3c48efca3999f Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Thu, 6 Apr 2023 16:57:32 -0700 Subject: [PATCH 1/5] Fix non C volumes on Windows Images may be created with a VOLUME stanza pointed to drive letters that are not C:. Currently, an image that has such VOLUMEs defined, will cause containerd to error out when starting a container. This change skips copying existing contents to volumes that are not C:. as an image can only hold files that are destined for the C: drive of a container. Signed-off-by: Gabriel Adrian Samfira --- pkg/cri/opts/container.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/cri/opts/container.go b/pkg/cri/opts/container.go index c7f226b13..3995975ef 100644 --- a/pkg/cri/opts/container.go +++ b/pkg/cri/opts/container.go @@ -97,6 +97,12 @@ func WithVolumes(volumeMounts map[string]string) containerd.NewContainerOpts { }() for host, volume := range volumeMounts { + // On Windows, volumes may be defined as different drive letters than C:. For non-C: volumes + // we need to skip trying to copy existing contents, as an image can only hold files + // destined for volumes hosted on drive C:. + if len(volume) >= 2 && string(volume[1]) == ":" && !strings.EqualFold(string(volume[0]), "c") { + continue + } // The volume may have been defined with a C: prefix, which we can't use here. volume = strings.TrimPrefix(volume, "C:") src, err := fs.RootPath(root, volume) From c7ec95caf4a85ee54df2491b3e65f4fc2af38510 Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Thu, 11 May 2023 10:34:06 +0300 Subject: [PATCH 2/5] Reword comment and make slight change to code Signed-off-by: Gabriel Adrian Samfira --- pkg/cri/opts/container.go | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/pkg/cri/opts/container.go b/pkg/cri/opts/container.go index 3995975ef..cb89412da 100644 --- a/pkg/cri/opts/container.go +++ b/pkg/cri/opts/container.go @@ -97,14 +97,22 @@ func WithVolumes(volumeMounts map[string]string) containerd.NewContainerOpts { }() for host, volume := range volumeMounts { - // On Windows, volumes may be defined as different drive letters than C:. For non-C: volumes - // we need to skip trying to copy existing contents, as an image can only hold files - // destined for volumes hosted on drive C:. - if len(volume) >= 2 && string(volume[1]) == ":" && !strings.EqualFold(string(volume[0]), "c") { - continue + // Windows allows volume mounts in subfolders under C: and as any other drive letter like D:, E:, etc. + // An image may contain files inside a folder defined as a VOLUME in a Dockerfile. On Windows, images + // can only contain pre-existing files for volumes situated on the root filesystem, which is C:. + // For any other volumes, we need to skip attempting to copy existing contents. + // + // C:\some\volume --> \some\volume + // D:\some\volume --> skip + if len(volume) >= 2 && string(volume[1]) == ":" { + // Perform a case insensitive comparison to "C", and skip non-C mounted volumes. + if !strings.EqualFold(string(volume[0]), "c") { + continue + } + // This is a volume mounted somewhere under C:\. We strip the drive letter and allow fs.RootPath() + // to append the remaining path to the rootfs path as seen by the host OS. + volume = volume[2:] } - // The volume may have been defined with a C: prefix, which we can't use here. - volume = strings.TrimPrefix(volume, "C:") src, err := fs.RootPath(root, volume) if err != nil { return fmt.Errorf("rootpath on mountPath %s, volume %s: %w", root, volume, err) From 88a3e25b3dac43f3c1593cf3b11427593d5058b5 Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Tue, 16 May 2023 10:55:10 +0300 Subject: [PATCH 3/5] Add targetOS to WithVolumes() Windows systems are capable of running both Windows Containers and Linux containers. For windows containers we need to sanitize the volume path and skip non-C volumes from the copy existing contents code path. Linux containers running on Windows and Linux must not have the path sanitized in any way. Supplying the targetOS of the container allows us to proprely decide when to activate that code path. Signed-off-by: Gabriel Adrian Samfira --- pkg/cri/opts/container.go | 32 +++++++++++++++------------- pkg/cri/sbserver/container_create.go | 2 +- pkg/cri/server/container_create.go | 2 +- 3 files changed, 19 insertions(+), 17 deletions(-) diff --git a/pkg/cri/opts/container.go b/pkg/cri/opts/container.go index cb89412da..754088ef2 100644 --- a/pkg/cri/opts/container.go +++ b/pkg/cri/opts/container.go @@ -55,7 +55,7 @@ func WithNewSnapshot(id string, i containerd.Image, opts ...snapshots.Opt) conta // WithVolumes copies ownership of volume in rootfs to its corresponding host path. // It doesn't update runtime spec. // The passed in map is a host path to container path map for all volumes. -func WithVolumes(volumeMounts map[string]string) containerd.NewContainerOpts { +func WithVolumes(volumeMounts map[string]string, targetOS string) containerd.NewContainerOpts { return func(ctx context.Context, client *containerd.Client, c *containers.Container) (err error) { if c.Snapshotter == "" { return errors.New("no snapshotter set for container") @@ -97,21 +97,23 @@ func WithVolumes(volumeMounts map[string]string) containerd.NewContainerOpts { }() for host, volume := range volumeMounts { - // Windows allows volume mounts in subfolders under C: and as any other drive letter like D:, E:, etc. - // An image may contain files inside a folder defined as a VOLUME in a Dockerfile. On Windows, images - // can only contain pre-existing files for volumes situated on the root filesystem, which is C:. - // For any other volumes, we need to skip attempting to copy existing contents. - // - // C:\some\volume --> \some\volume - // D:\some\volume --> skip - if len(volume) >= 2 && string(volume[1]) == ":" { - // Perform a case insensitive comparison to "C", and skip non-C mounted volumes. - if !strings.EqualFold(string(volume[0]), "c") { - continue + if targetOS == "windows" { + // Windows allows volume mounts in subfolders under C: and as any other drive letter like D:, E:, etc. + // An image may contain files inside a folder defined as a VOLUME in a Dockerfile. On Windows, images + // can only contain pre-existing files for volumes situated on the root filesystem, which is C:. + // For any other volumes, we need to skip attempting to copy existing contents. + // + // C:\some\volume --> \some\volume + // D:\some\volume --> skip + if len(volume) >= 2 && string(volume[1]) == ":" { + // Perform a case insensitive comparison to "C", and skip non-C mounted volumes. + if !strings.EqualFold(string(volume[0]), "c") { + continue + } + // This is a volume mounted somewhere under C:\. We strip the drive letter and allow fs.RootPath() + // to append the remaining path to the rootfs path as seen by the host OS. + volume = volume[2:] } - // This is a volume mounted somewhere under C:\. We strip the drive letter and allow fs.RootPath() - // to append the remaining path to the rootfs path as seen by the host OS. - volume = volume[2:] } src, err := fs.RootPath(root, volume) if err != nil { diff --git a/pkg/cri/sbserver/container_create.go b/pkg/cri/sbserver/container_create.go index f274e671f..435792ca4 100644 --- a/pkg/cri/sbserver/container_create.go +++ b/pkg/cri/sbserver/container_create.go @@ -223,7 +223,7 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta for _, v := range volumeMounts { mountMap[filepath.Clean(v.HostPath)] = v.ContainerPath } - opts = append(opts, customopts.WithVolumes(mountMap)) + opts = append(opts, customopts.WithVolumes(mountMap, image.ImageSpec.OS)) } meta.ImageRef = image.ID meta.StopSignal = image.ImageSpec.Config.StopSignal diff --git a/pkg/cri/server/container_create.go b/pkg/cri/server/container_create.go index 9483778d1..06b7fa948 100644 --- a/pkg/cri/server/container_create.go +++ b/pkg/cri/server/container_create.go @@ -203,7 +203,7 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta for _, v := range volumeMounts { mountMap[filepath.Clean(v.HostPath)] = v.ContainerPath } - opts = append(opts, customopts.WithVolumes(mountMap)) + opts = append(opts, customopts.WithVolumes(mountMap, image.ImageSpec.OS)) } meta.ImageRef = image.ID meta.StopSignal = image.ImageSpec.Config.StopSignal From b9dfd29b73a9ddfb316d9af6ff99ac18c8a70ba9 Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Wed, 24 May 2023 22:12:47 +0000 Subject: [PATCH 4/5] Update tests to use volume-copy-up:2.2 Signed-off-by: Gabriel Adrian Samfira --- integration/images/image_list.go | 2 +- integration/main_test.go | 1 - integration/volume_copy_up_test.go | 158 ++++++++++++++++++++++------- script/test/cri-integration.sh | 2 - 4 files changed, 120 insertions(+), 43 deletions(-) diff --git a/integration/images/image_list.go b/integration/images/image_list.go index d7cce9524..453ca9099 100644 --- a/integration/images/image_list.go +++ b/integration/images/image_list.go @@ -52,7 +52,7 @@ func initImages(imageListFile string) { BusyBox: "ghcr.io/containerd/busybox:1.36", Pause: "registry.k8s.io/pause:3.8", ResourceConsumer: "registry.k8s.io/e2e-test-images/resource-consumer:1.10", - VolumeCopyUp: "ghcr.io/containerd/volume-copy-up:2.1", + VolumeCopyUp: "ghcr.io/containerd/volume-copy-up:2.2", VolumeOwnership: "ghcr.io/containerd/volume-ownership:2.1", ArgsEscaped: "cplatpublic.azurecr.io/args-escaped-test-image-ns:1.0", } diff --git a/integration/main_test.go b/integration/main_test.go index 5e86ec934..a6d5d2d4f 100644 --- a/integration/main_test.go +++ b/integration/main_test.go @@ -66,7 +66,6 @@ var ( ) var criEndpoint = flag.String("cri-endpoint", "unix:///run/containerd/containerd.sock", "The endpoint of cri plugin.") -var criRoot = flag.String("cri-root", "/var/lib/containerd/io.containerd.grpc.v1.cri", "The root directory of cri plugin.") var runtimeHandler = flag.String("runtime-handler", "", "The runtime handler to use in the test.") var containerdBin = flag.String("containerd-bin", "containerd", "The containerd binary name. The name is used to restart containerd during test.") diff --git a/integration/volume_copy_up_test.go b/integration/volume_copy_up_test.go index 00a539050..32d24eabf 100644 --- a/integration/volume_copy_up_test.go +++ b/integration/volume_copy_up_test.go @@ -17,16 +17,20 @@ package integration import ( + "context" + "encoding/json" "fmt" "os" "path/filepath" - goruntime "runtime" + "runtime" "testing" "time" "github.com/containerd/containerd/integration/images" + specs "github.com/opencontainers/runtime-spec/specs-go" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + v1 "k8s.io/cri-api/pkg/apis/runtime/v1" ) const ( @@ -36,6 +40,16 @@ const ( containerUserSID = "S-1-5-93-2-2" ) +type volumeFile struct { + fileName string + contents string +} + +type containerVolume struct { + containerPath string + files []volumeFile +} + func TestVolumeCopyUp(t *testing.T) { var ( testImage = images.Get(images.VolumeCopyUp) @@ -59,23 +73,85 @@ func TestVolumeCopyUp(t *testing.T) { t.Logf("Start the container") require.NoError(t, runtimeService.StartContainer(cn)) - // ghcr.io/containerd/volume-copy-up:2.1 contains a test_dir - // volume, which contains a test_file with content "test_content". - t.Logf("Check whether volume contains the test file") - stdout, stderr, err := runtimeService.ExecSync(cn, []string{ - "cat", - "/test_dir/test_file", - }, execTimeout) + expectedVolumes := []containerVolume{ + { + containerPath: "/test_dir", + files: []volumeFile{ + { + fileName: "test_file", + contents: "test_content\n", + }, + }, + }, + { + containerPath: "/:colon_prefixed", + files: []volumeFile{ + { + fileName: "colon_prefixed_file", + contents: "test_content\n", + }, + }, + }, + { + containerPath: "C:/weird_test_dir", + files: []volumeFile{ + { + fileName: "weird_test_file", + contents: "test_content\n", + }, + }, + }, + } + + if runtime.GOOS == "windows" { + expectedVolumes = []containerVolume{ + { + containerPath: "C:\\test_dir", + files: []volumeFile{ + { + fileName: "test_file", + contents: "test_content\n", + }, + }, + }, + { + containerPath: "D:", + files: []volumeFile{}, + }, + } + } + + volumeMappings, err := getContainerBindVolumes(t, cn) require.NoError(t, err) - assert.Empty(t, stderr) - assert.Equal(t, "test_content\n", string(stdout)) t.Logf("Check host path of the volume") - volumePaths, err := getHostPathForVolumes(*criRoot, cn) - require.NoError(t, err) - assert.Equal(t, len(volumePaths), 1, "expected exactly 1 volume") + for _, vol := range expectedVolumes { + _, ok := volumeMappings[vol.containerPath] + assert.Equalf(t, true, ok, "expected to find volume %s", vol.containerPath) + } - testFilePath := filepath.Join(volumePaths[0], "test_file") + // ghcr.io/containerd/volume-copy-up:2.2 contains 3 volumes on Linux and 2 volumes on Windows. + // On linux, each of the volumes contains a single file, all with the same conrent. On Windows, + // non C volumes defined in the image start out as empty. + for _, vol := range expectedVolumes { + files, err := os.ReadDir(volumeMappings[vol.containerPath]) + require.NoError(t, err) + assert.Equal(t, len(vol.files), len(files)) + + for _, file := range vol.files { + t.Logf("Check whether volume %s contains the test file %s", vol.containerPath, file.fileName) + stdout, stderr, err := runtimeService.ExecSync(cn, []string{ + "cat", + filepath.ToSlash(filepath.Join(vol.containerPath, file.fileName)), + }, execTimeout) + require.NoError(t, err) + assert.Empty(t, stderr) + assert.Equal(t, file.contents, string(stdout)) + } + } + + testFilePath := filepath.Join(volumeMappings[expectedVolumes[0].containerPath], expectedVolumes[0].files[0].fileName) + inContainerPath := filepath.Join(expectedVolumes[0].containerPath, expectedVolumes[0].files[0].fileName) contents, err := os.ReadFile(testFilePath) require.NoError(t, err) assert.Equal(t, "test_content\n", string(contents)) @@ -84,7 +160,7 @@ func TestVolumeCopyUp(t *testing.T) { _, _, err = runtimeService.ExecSync(cn, []string{ "sh", "-c", - "echo new_content > /test_dir/test_file", + fmt.Sprintf("echo new_content > %s", filepath.ToSlash(inContainerPath)), }, execTimeout) require.NoError(t, err) @@ -124,15 +200,17 @@ func TestVolumeOwnership(t *testing.T) { // exist inside the container that returns the owner in the form of USERNAME:SID. t.Logf("Check ownership of test directory inside container") + volumePath := "/test_dir" cmd := []string{ - "stat", "-c", "%u:%g", "/test_dir", + "stat", "-c", "%u:%g", volumePath, } expectedContainerOutput := "65534:65534\n" expectedHostOutput := "65534:65534\n" - if goruntime.GOOS == "windows" { + if runtime.GOOS == "windows" { + volumePath = "C:\\volumes\\test_dir" cmd = []string{ "C:\\bin\\get_owner.exe", - "C:\\volumes\\test_dir", + volumePath, } expectedContainerOutput = fmt.Sprintf("%s:%s", containerUserName, containerUserSID) // The username is unknown on the host, but we can still get the SID. @@ -144,34 +222,36 @@ func TestVolumeOwnership(t *testing.T) { assert.Equal(t, expectedContainerOutput, string(stdout)) t.Logf("Check ownership of test directory on the host") - volumePaths, err := getHostPathForVolumes(*criRoot, cn) + volumePaths, err := getContainerBindVolumes(t, cn) require.NoError(t, err) - assert.Equal(t, len(volumePaths), 1, "expected exactly 1 volume") - output, err := getOwnership(volumePaths[0]) + output, err := getOwnership(volumePaths[volumePath]) require.NoError(t, err) assert.Equal(t, expectedHostOutput, output) } -func getHostPathForVolumes(criRoot, containerID string) ([]string, error) { - hostPath := filepath.Join(criRoot, "containers", containerID, "volumes") - if _, err := os.Stat(hostPath); err != nil { - return nil, err +func getContainerBindVolumes(t *testing.T, containerID string) (map[string]string, error) { + client, err := RawRuntimeClient() + require.NoError(t, err, "failed to get raw grpc runtime service client") + request := &v1.ContainerStatusRequest{ + ContainerId: containerID, + Verbose: true, } + response, err := client.ContainerStatus(context.TODO(), request) + require.NoError(t, err) + ret := make(map[string]string) - volumes, err := os.ReadDir(hostPath) - if err != nil { - return nil, err + mounts := struct { + RuntimeSpec struct { + Mounts []specs.Mount `json:"mounts"` + } `json:"runtimeSpec"` + }{} + + info := response.Info["info"] + err = json.Unmarshal([]byte(info), &mounts) + require.NoError(t, err) + for _, mount := range mounts.RuntimeSpec.Mounts { + ret[mount.Destination] = mount.Source } - - if len(volumes) == 0 { - return []string{}, nil - } - - volumePaths := make([]string, len(volumes)) - for idx, volume := range volumes { - volumePaths[idx] = filepath.Join(hostPath, volume.Name()) - } - - return volumePaths, nil + return ret, nil } diff --git a/script/test/cri-integration.sh b/script/test/cri-integration.sh index 8d1a2a29a..442054a4b 100755 --- a/script/test/cri-integration.sh +++ b/script/test/cri-integration.sh @@ -37,7 +37,6 @@ fi # RUNTIME is the runtime handler to use in the test. RUNTIME=${RUNTIME:-""} -CRI_ROOT="${CONTAINERD_ROOT}/io.containerd.grpc.v1.cri" mkdir -p "${REPORT_DIR}" test_setup "${REPORT_DIR}" @@ -54,7 +53,6 @@ CMD+="${PWD}/bin/cri-integration.test" ${CMD} --test.run="${FOCUS}" --test.v \ --cri-endpoint="${CONTAINERD_SOCK}" \ - --cri-root="${CRI_ROOT}" \ --runtime-handler="${RUNTIME}" \ --containerd-bin="${CONTAINERD_BIN}" \ --image-list="${TEST_IMAGE_LIST:-}" && test_exit_code=$? || test_exit_code=$? From 6dd529e400bff93e651bf29179e50e58060a7577 Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Thu, 8 Jun 2023 11:25:10 +0300 Subject: [PATCH 5/5] Pass in imagespec.Platform to WithVolumes() Signed-off-by: Gabriel Adrian Samfira --- pkg/cri/opts/container.go | 5 +++-- pkg/cri/sbserver/container_create.go | 2 +- pkg/cri/server/container_create.go | 9 ++++++++- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/pkg/cri/opts/container.go b/pkg/cri/opts/container.go index 754088ef2..1840d6cc5 100644 --- a/pkg/cri/opts/container.go +++ b/pkg/cri/opts/container.go @@ -24,6 +24,7 @@ import ( "strings" "github.com/containerd/continuity/fs" + imagespec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/containerd/containerd" "github.com/containerd/containerd/containers" @@ -55,7 +56,7 @@ func WithNewSnapshot(id string, i containerd.Image, opts ...snapshots.Opt) conta // WithVolumes copies ownership of volume in rootfs to its corresponding host path. // It doesn't update runtime spec. // The passed in map is a host path to container path map for all volumes. -func WithVolumes(volumeMounts map[string]string, targetOS string) containerd.NewContainerOpts { +func WithVolumes(volumeMounts map[string]string, platform imagespec.Platform) containerd.NewContainerOpts { return func(ctx context.Context, client *containerd.Client, c *containers.Container) (err error) { if c.Snapshotter == "" { return errors.New("no snapshotter set for container") @@ -97,7 +98,7 @@ func WithVolumes(volumeMounts map[string]string, targetOS string) containerd.New }() for host, volume := range volumeMounts { - if targetOS == "windows" { + if platform.OS == "windows" { // Windows allows volume mounts in subfolders under C: and as any other drive letter like D:, E:, etc. // An image may contain files inside a folder defined as a VOLUME in a Dockerfile. On Windows, images // can only contain pre-existing files for volumes situated on the root filesystem, which is C:. diff --git a/pkg/cri/sbserver/container_create.go b/pkg/cri/sbserver/container_create.go index 435792ca4..5d0ebc516 100644 --- a/pkg/cri/sbserver/container_create.go +++ b/pkg/cri/sbserver/container_create.go @@ -223,7 +223,7 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta for _, v := range volumeMounts { mountMap[filepath.Clean(v.HostPath)] = v.ContainerPath } - opts = append(opts, customopts.WithVolumes(mountMap, image.ImageSpec.OS)) + opts = append(opts, customopts.WithVolumes(mountMap, platform)) } meta.ImageRef = image.ID meta.StopSignal = image.ImageSpec.Config.StopSignal diff --git a/pkg/cri/server/container_create.go b/pkg/cri/server/container_create.go index 06b7fa948..52d18aee8 100644 --- a/pkg/cri/server/container_create.go +++ b/pkg/cri/server/container_create.go @@ -203,7 +203,14 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta for _, v := range volumeMounts { mountMap[filepath.Clean(v.HostPath)] = v.ContainerPath } - opts = append(opts, customopts.WithVolumes(mountMap, image.ImageSpec.OS)) + platform := imagespec.Platform{ + OS: image.ImageSpec.OS, + Architecture: image.ImageSpec.Architecture, + OSVersion: image.ImageSpec.OSVersion, + OSFeatures: image.ImageSpec.OSFeatures, + Variant: image.ImageSpec.Variant, + } + opts = append(opts, customopts.WithVolumes(mountMap, platform)) } meta.ImageRef = image.ID meta.StopSignal = image.ImageSpec.Config.StopSignal