Merge pull request #8885 from kinvolk/rata/runc-abs-path

cri: Don't use rel path for image volumes

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",

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,

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

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,