From fd135f1a8b8929ac26d47ed567745023714a0e8b Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Wed, 29 Jun 2022 16:50:17 +0200 Subject: [PATCH] Add Relative function to Locator interface This adds a Relative function to the Locator interface and uses this to determine the host and container paths for located files (and devices). This ensures that the root (e.g. the nvidia driver root) is stripped from the container path. Signed-off-by: Evan Lezar --- internal/discover/csv_test.go | 2 +- internal/discover/discover.go | 6 ++-- internal/discover/mounts.go | 32 +++++++++++++----- internal/discover/mounts_test.go | 56 +++++++++++++++++++++++++++----- internal/edits/device.go | 3 ++ internal/edits/mount.go | 3 +- internal/lookup/file.go | 10 ++++++ internal/lookup/locator.go | 1 + internal/lookup/locator_mock.go | 49 +++++++++++++++++++++++++++- 9 files changed, 138 insertions(+), 24 deletions(-) diff --git a/internal/discover/csv_test.go b/internal/discover/csv_test.go index 28b1e021..d1b37d0e 100644 --- a/internal/discover/csv_test.go +++ b/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", diff --git a/internal/discover/discover.go b/internal/discover/discover.go index 64dbef85..e65b360e 100644 --- a/internal/discover/discover.go +++ b/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. diff --git a/internal/discover/mounts.go b/internal/discover/mounts.go index 07650509..006c7d56 100644 --- a/internal/discover/mounts.go +++ b/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 { - d.logger.Infof("Selecting %v", path) - mount := Mount{ - Path: path, - } - mounts = append(mounts, mount) + for _, m := range uniqueMounts { + mounts = append(mounts, m) } d.cache = mounts - return mounts, nil + return d.cache, nil } diff --git a/internal/discover/mounts_test.go b/internal/discover/mounts_test.go index b443d18c..6d8e3d20 100644 --- a/internal/discover/mounts_test.go +++ b/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: "multiple0"}, - {Path: "multiple1"}, - {Path: "required1"}, + {Path: "required0", HostPath: "required0"}, + {Path: "multiple0", HostPath: "multiple0"}, + {Path: "multiple1", HostPath: "multiple1"}, + {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"}, }, }, } diff --git a/internal/edits/device.go b/internal/edits/device.go index 5e11f41d..4dd6a303 100644 --- a/internal/edits/device.go +++ b/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, } diff --git a/internal/edits/mount.go b/internal/edits/mount.go index 0697321e..46386322 100644 --- a/internal/edits/mount.go +++ b/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, - // TODO: We need to allow the container path to be customised + HostPath: d.HostPath, ContainerPath: d.Path, Options: []string{ "ro", diff --git a/internal/lookup/file.go b/internal/lookup/file.go index ab52d03d..1283f0ae 100644 --- a/internal/lookup/file.go +++ b/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) diff --git a/internal/lookup/locator.go b/internal/lookup/locator.go index 871e1b02..76ade332 100644 --- a/internal/lookup/locator.go +++ b/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) } diff --git a/internal/lookup/locator_mock.go b/internal/lookup/locator_mock.go index 0c50f345..349539ae 100644 --- a/internal/lookup/locator_mock.go +++ b/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 +}