From 2d64ab8d79a0bb18a089de37a5fbfc8eb5ab720d Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Wed, 26 Jul 2023 18:20:22 +0200 Subject: [PATCH] cri: Don't use rel path for image volumes Runc 1.1 throws a warning when using rel destination paths, and runc 1.2 is planning to thow an error (i.e. won't start the container). Let's just make this an abs path in the only place it might not be: the mounts created due to `VOLUME` directives in the Dockerfile. Signed-off-by: Rodrigo Campos --- integration/volume_copy_up_test.go | 2 +- pkg/cri/sbserver/container_create.go | 25 ++++++++++++++++------- pkg/cri/sbserver/container_create_test.go | 19 ++++++++++++++++- pkg/cri/server/container_create.go | 6 ++++++ 4 files changed, 43 insertions(+), 9 deletions(-) diff --git a/integration/volume_copy_up_test.go b/integration/volume_copy_up_test.go index 32d24eabf..086f2021d 100644 --- a/integration/volume_copy_up_test.go +++ b/integration/volume_copy_up_test.go @@ -93,7 +93,7 @@ func TestVolumeCopyUp(t *testing.T) { }, }, { - containerPath: "C:/weird_test_dir", + containerPath: "/C:/weird_test_dir", files: []volumeFile{ { fileName: "weird_test_file", diff --git a/pkg/cri/sbserver/container_create.go b/pkg/cri/sbserver/container_create.go index 8ad47107c..f3a241369 100644 --- a/pkg/cri/sbserver/container_create.go +++ b/pkg/cri/sbserver/container_create.go @@ -22,6 +22,7 @@ import ( "fmt" "path/filepath" "strconv" + "strings" "time" "github.com/containerd/typeurl/v2" @@ -148,10 +149,15 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta } }() + platform, err := controller.Platform(ctx, sandboxID) + if err != nil { + return nil, fmt.Errorf("failed to query sandbox platform: %w", err) + } + var volumeMounts []*runtime.Mount if !c.config.IgnoreImageDefinedVolumes { // Create container image volumes mounts. - volumeMounts = c.volumeMounts(containerRootDir, config.GetMounts(), &image.ImageSpec.Config) + volumeMounts = c.volumeMounts(platform, containerRootDir, config.GetMounts(), &image.ImageSpec.Config) } else if len(image.ImageSpec.Config.Volumes) != 0 { log.G(ctx).Debugf("Ignoring volumes defined in image %v because IgnoreImageDefinedVolumes is set", image.ID) } @@ -162,11 +168,6 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta } log.G(ctx).Debugf("Use OCI runtime %+v for sandbox %q and container %q", ociRuntime, sandboxID, id) - platform, err := controller.Platform(ctx, sandboxID) - if err != nil { - return nil, fmt.Errorf("failed to query sandbox platform: %w", err) - } - spec, err := c.buildContainerSpec( platform, id, @@ -340,7 +341,7 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta // volumeMounts sets up image volumes for container. Rely on the removal of container // root directory to do cleanup. Note that image volume will be skipped, if there is criMounts // specified with the same destination. -func (c *criService) volumeMounts(containerRootDir string, criMounts []*runtime.Mount, config *imagespec.ImageConfig) []*runtime.Mount { +func (c *criService) volumeMounts(platform platforms.Platform, containerRootDir string, criMounts []*runtime.Mount, config *imagespec.ImageConfig) []*runtime.Mount { if len(config.Volumes) == 0 { return nil } @@ -355,6 +356,16 @@ func (c *criService) volumeMounts(containerRootDir string, criMounts []*runtime. } volumeID := util.GenerateID() src := filepath.Join(containerRootDir, "volumes", volumeID) + // When the platform OS is Linux, ensure dst is a _Linux_ abs path. + // We can't use filepath.IsAbs() because, when executing on Windows, it checks for + // Windows abs paths. + if platform.OS == "linux" && !strings.HasPrefix(dst, "/") { + // On Windows, ToSlash() is needed to ensure the path is a valid Linux path. + // On Linux, ToSlash() is a no-op. + oldDst := dst + dst = filepath.ToSlash(filepath.Join("/", dst)) + log.L.Debugf("Volume destination %q is not absolute, converted to %q", oldDst, dst) + } // addOCIBindMounts will create these volumes. mounts = append(mounts, &runtime.Mount{ ContainerPath: dst, diff --git a/pkg/cri/sbserver/container_create_test.go b/pkg/cri/sbserver/container_create_test.go index 5c7d47075..3f6cb067c 100644 --- a/pkg/cri/sbserver/container_create_test.go +++ b/pkg/cri/sbserver/container_create_test.go @@ -233,6 +233,7 @@ func TestVolumeMounts(t *testing.T) { testContainerRootDir := "test-container-root" for _, test := range []struct { desc string + platform platforms.Platform criMounts []*runtime.Mount imageVolumes map[string]struct{} expectedMountDest []string @@ -280,6 +281,22 @@ func TestVolumeMounts(t *testing.T) { "/test-volume-2/", }, }, + { + desc: "should make relative paths absolute on Linux", + platform: platforms.Platform{OS: "linux"}, + imageVolumes: map[string]struct{}{ + "./test-volume-1": {}, + "C:/test-volume-2": {}, + "../../test-volume-3": {}, + "/abs/test-volume-4": {}, + }, + expectedMountDest: []string{ + "/test-volume-1", + "/C:/test-volume-2", + "/test-volume-3", + "/abs/test-volume-4", + }, + }, } { test := test t.Run(test.desc, func(t *testing.T) { @@ -287,7 +304,7 @@ func TestVolumeMounts(t *testing.T) { Volumes: test.imageVolumes, } c := newTestCRIService() - got := c.volumeMounts(testContainerRootDir, test.criMounts, config) + got := c.volumeMounts(test.platform, testContainerRootDir, test.criMounts, config) assert.Len(t, got, len(test.expectedMountDest)) for _, dest := range test.expectedMountDest { found := false diff --git a/pkg/cri/server/container_create.go b/pkg/cri/server/container_create.go index 52d18aee8..4cc8defae 100644 --- a/pkg/cri/server/container_create.go +++ b/pkg/cri/server/container_create.go @@ -21,6 +21,7 @@ import ( "errors" "fmt" "path/filepath" + goruntime "runtime" "time" "github.com/containerd/typeurl/v2" @@ -332,6 +333,11 @@ func (c *criService) volumeMounts(containerRootDir string, criMounts []*runtime. } volumeID := util.GenerateID() src := filepath.Join(containerRootDir, "volumes", volumeID) + if !filepath.IsAbs(dst) && goruntime.GOOS != "windows" { + oldDst := dst + dst = filepath.Join("/", dst) + log.L.Debugf("Volume destination %q is not absolute, converted to %q", oldDst, dst) + } // addOCIBindMounts will create these volumes. mounts = append(mounts, &runtime.Mount{ ContainerPath: dst,