Merge branch 'CNT-3242/strip-root-from-container-mount' into 'main'

Strip root (e.g. driver root) from located mount paths in the container

See merge request nvidia/container-toolkit/container-toolkit!177

internal/discover/csv_test.go

@@ -49,7 +49,7 @@ func TestCharDevices(t *testing.T) {
 					required: []string{"required"},
 				},
 			),
-			expectedDevices: []Device{{Path: "located"}},
+			expectedDevices: []Device{{Path: "located", HostPath: "located"}},
 		},
 		{
 			description:          "dev devices returns error for nil lookup",

internal/discover/discover.go

@@ -24,12 +24,14 @@ type Config struct {
 
 // Device represents a discovered character device.
 type Device struct {
-	Path string
+	HostPath string
+	Path     string
 }
 
 // Mount represents a discovered mount.
 type Mount struct {
-	Path string
+	HostPath string
+	Path     string
 }
 
 // Hook represents a discovered hook.

internal/discover/mounts.go

@@ -51,7 +51,7 @@ func (d *mounts) Mounts() ([]Mount, error) {
 	d.Lock()
 	defer d.Unlock()
 
-	paths := make(map[string]bool)
+	uniqueMounts := make(map[string]Mount)
 
 	for _, candidate := range d.required {
 		d.logger.Debugf("Locating %v", candidate)
@@ -66,20 +66,34 @@ func (d *mounts) Mounts() ([]Mount, error) {
 		}
 		d.logger.Debugf("Located %v as %v", candidate, located)
 		for _, p := range located {
-			paths[p] = true
+			if _, ok := uniqueMounts[p]; ok {
+				d.logger.Debugf("Skipping duplicate mount %v", p)
+				continue
+			}
+
+			r, err := d.lookup.Relative(p)
+			if err != nil {
+				d.logger.Warnf("Failed to get relative path of %v: %v", p, err)
+				continue
+			}
+			if r == "" {
+				r = p
+			}
+
+			d.logger.Infof("Selecting %v as %v", p, r)
+			uniqueMounts[p] = Mount{
+				HostPath: p,
+				Path:     r,
+			}
 		}
 	}
 
 	var mounts []Mount
-	for path := range paths {
+	for _, m := range uniqueMounts {
-		d.logger.Infof("Selecting %v", path)
+		mounts = append(mounts, m)
-		mount := Mount{
-			Path: path,
-		}
-		mounts = append(mounts, mount)
 	}
 
 	d.cache = mounts
 
-	return mounts, nil
+	return d.cache, nil
 }

internal/discover/mounts_test.go

@@ -70,7 +70,7 @@ func TestMounts(t *testing.T) {
 				},
 				required: []string{"required"},
 			},
-			expectedMounts: []Mount{{Path: "located"}},
+			expectedMounts: []Mount{{Path: "located", HostPath: "located"}},
 		},
 		{
 			description:   "mounts removes located duplicates",
@@ -83,7 +83,7 @@ func TestMounts(t *testing.T) {
 				},
 				required: []string{"required0", "required1"},
 			},
-			expectedMounts: []Mount{{Path: "located"}},
+			expectedMounts: []Mount{{Path: "located", HostPath: "located"}},
 		},
 		{
 			description: "mounts skips located errors",
@@ -98,7 +98,7 @@ func TestMounts(t *testing.T) {
 				},
 				required: []string{"required0", "error", "required1"},
 			},
-			expectedMounts: []Mount{{Path: "required0"}, {Path: "required1"}},
+			expectedMounts: []Mount{{Path: "required0", HostPath: "required0"}, {Path: "required1", HostPath: "required1"}},
 		},
 		{
 			description: "mounts skips unlocated",
@@ -113,10 +113,10 @@ func TestMounts(t *testing.T) {
 				},
 				required: []string{"required0", "empty", "required1"},
 			},
-			expectedMounts: []Mount{{Path: "required0"}, {Path: "required1"}},
+			expectedMounts: []Mount{{Path: "required0", HostPath: "required0"}, {Path: "required1", HostPath: "required1"}},
 		},
 		{
-			description: "mounts skips unlocated",
+			description: "mounts adds multiple",
 			input: &mounts{
 				lookup: &lookup.LocatorMock{
 					LocateFunc: func(s string) ([]string, error) {
@@ -129,10 +129,48 @@ func TestMounts(t *testing.T) {
 				required: []string{"required0", "multiple", "required1"},
 			},
 			expectedMounts: []Mount{
-				{Path: "required0"},
+				{Path: "required0", HostPath: "required0"},
-				{Path: "multiple0"},
+				{Path: "multiple0", HostPath: "multiple0"},
-				{Path: "multiple1"},
+				{Path: "multiple1", HostPath: "multiple1"},
-				{Path: "required1"},
+				{Path: "required1", HostPath: "required1"},
+			},
+		},
+		{
+			description: "mounts uses relative path",
+			input: &mounts{
+				lookup: &lookup.LocatorMock{
+					LocateFunc: func(s string) ([]string, error) {
+						return []string{"located"}, nil
+					},
+					RelativeFunc: func(s string) (string, error) {
+						return "relative", nil
+					},
+				},
+				required: []string{"required0", "multiple", "required1"},
+			},
+			expectedMounts: []Mount{
+				{Path: "relative", HostPath: "located"},
+			},
+		},
+		{
+			description: "mounts skips relative error",
+			input: &mounts{
+				lookup: &lookup.LocatorMock{
+					LocateFunc: func(s string) ([]string, error) {
+						return []string{s}, nil
+					},
+					RelativeFunc: func(s string) (string, error) {
+						if s == "error" {
+							return "", fmt.Errorf("no relative path")
+						}
+						return "relative" + s, nil
+					},
+				},
+				required: []string{"required0", "error", "required1"},
+			},
+			expectedMounts: []Mount{
+				{Path: "relativerequired0", HostPath: "required0"},
+				{Path: "relativerequired1", HostPath: "required1"},
 			},
 		},
 	}

internal/edits/device.go

@@ -37,6 +37,9 @@ func (d device) toEdits() *cdi.ContainerEdits {
 // toSpec converts a discovered Device to a CDI Spec Device. Note
 // that missing info is filled in when edits are applied by querying the Device node.
 func (d device) toSpec() *specs.DeviceNode {
+	// NOTE: We may want to mirror what is done in cri-o w.r.t src (Host) and dst (Container) paths
+	// to ensure that the right permissions are included.
+	// https://github.com/cri-o/cri-o/blob/ca3bb80a3dda0440659fcf8da8ed6f23211de94e/internal/config/device/device.go#L93
 	s := specs.DeviceNode{
 		Path: d.Path,
 	}

internal/edits/mount.go

@@ -38,8 +38,7 @@ func (d mount) toEdits() *cdi.ContainerEdits {
 // that missing info is filled in when edits are applied by querying the Mount node.
 func (d mount) toSpec() *specs.Mount {
 	s := specs.Mount{
-		HostPath: d.Path,
+		HostPath:      d.HostPath,
-		// TODO: We need to allow the container path to be customised
 		ContainerPath: d.Path,
 		Options: []string{
 			"ro",

internal/lookup/file.go

@@ -28,6 +28,7 @@ import (
 // prefixes. The validity of a file is determined by a filter function.
 type file struct {
 	logger   *log.Logger
+	root     string
 	prefixes []string
 	filter   func(string) error
 }
@@ -77,6 +78,15 @@ func (p file) Locate(pattern string) ([]string, error) {
 	return filenames, nil
 }
 
+// Relative returns the path relative to the root for the file locator
+func (p file) Relative(path string) (string, error) {
+	if p.root == "" || p.root == "/" {
+		return path, nil
+	}
+
+	return filepath.Rel(p.root, path)
+}
+
 // assertFile checks whether the specified path is a regular file
 func assertFile(filename string) error {
 	info, err := os.Stat(filename)

internal/lookup/locator.go

@@ -21,4 +21,5 @@ package lookup
 // Locator defines the interface for locating files on a system.
 type Locator interface {
 	Locate(string) ([]string, error)
+	Relative(string) (string, error)
 }

internal/lookup/locator_mock.go

@@ -20,6 +20,9 @@ var _ Locator = &LocatorMock{}
 // 			LocateFunc: func(s string) ([]string, error) {
 // 				panic("mock out the Locate method")
 // 			},
+// 			RelativeFunc: func(s string) (string, error) {
+// 				panic("mock out the Relative method")
+// 			},
 // 		}
 //
 // 		// use mockedLocator in code that requires Locator
@@ -30,6 +33,9 @@ type LocatorMock struct {
 	// LocateFunc mocks the Locate method.
 	LocateFunc func(s string) ([]string, error)
 
+	// RelativeFunc mocks the Relative method.
+	RelativeFunc func(s string) (string, error)
+
 	// calls tracks calls to the methods.
 	calls struct {
 		// Locate holds details about calls to the Locate method.
@@ -37,8 +43,14 @@ type LocatorMock struct {
 			// S is the s argument value.
 			S string
 		}
+		// Relative holds details about calls to the Relative method.
+		Relative []struct {
+			// S is the s argument value.
+			S string
+		}
 	}
-	lockLocate sync.RWMutex
+	lockLocate   sync.RWMutex
+	lockRelative sync.RWMutex
 }
 
 // Locate calls LocateFunc.
@@ -75,3 +87,38 @@ func (mock *LocatorMock) LocateCalls() []struct {
 	mock.lockLocate.RUnlock()
 	return calls
 }
+
+// Relative calls RelativeFunc.
+func (mock *LocatorMock) Relative(s string) (string, error) {
+	callInfo := struct {
+		S string
+	}{
+		S: s,
+	}
+	mock.lockRelative.Lock()
+	mock.calls.Relative = append(mock.calls.Relative, callInfo)
+	mock.lockRelative.Unlock()
+	if mock.RelativeFunc == nil {
+		var (
+			sOut   string
+			errOut error
+		)
+		return sOut, errOut
+	}
+	return mock.RelativeFunc(s)
+}
+
+// RelativeCalls gets all the calls that were made to Relative.
+// Check the length with:
+//     len(mockedLocator.RelativeCalls())
+func (mock *LocatorMock) RelativeCalls() []struct {
+	S string
+} {
+	var calls []struct {
+		S string
+	}
+	mock.lockRelative.RLock()
+	calls = mock.calls.Relative
+	mock.lockRelative.RUnlock()
+	return calls
+}