cri: Don't use rel path for image volumes

rata · rata · commit 2d64ab8d79a0 · 2023-07-31T12:33:54.000+02:00
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 &lt;rodrigoca@microsoft.com&gt;
diff --git 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
@@ -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
@@ -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,14 +281,30 @@ 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) {
 			config := &imagespec.ImageConfig{
 				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
@@ -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,

Original file line number	Diff line number	Diff line change
`@@ -93,7 +93,7 @@ func TestVolumeCopyUp(t *testing.T) {`
`93`	`93`	`},`
`94`	`94`	`},`
`95`	`95`	`{`
`96`		`- containerPath: "C:/weird_test_dir",`
	`96`	`+ containerPath: "/C:/weird_test_dir",`
`97`	`97`	`files: []volumeFile{`
`98`	`98`	`{`
`99`	`99`	`fileName: "weird_test_file",`