Merge branch 'add-options-to-mounts' into 'main'

Add Options to mounts to refactor IPC CDI spec generation

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

cmd/nvidia-ctk/cdi/generate/driver.go

@@ -41,12 +41,18 @@ func NewDriverDiscoverer(logger *logrus.Logger, driverRoot string, nvidiaCTKPath
 		return nil, fmt.Errorf("failed to create discoverer for driver libraries: %v", err)
 	}
 
+	ipcs, err := discover.NewIPCDiscoverer(logger, driverRoot)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create discoverer for IPC sockets: %v", err)
+	}
+
 	firmwares := NewDriverFirmwareDiscoverer(logger, driverRoot, version)
 
 	binaries := NewDriverBinariesDiscoverer(logger, driverRoot)
 
 	d := discover.Merge(
 		libraries,
+		ipcs,
 		firmwares,
 		binaries,
 	)

cmd/nvidia-ctk/cdi/generate/generate.go

@@ -232,24 +232,6 @@ func (m command) generateSpec(driverRoot string, nvidiaCTKPath string, namer dev
 
 	deviceSpecs = append(deviceSpecs, allDevice)
 
-	allEdits := edits.NewContainerEdits()
-
-	ipcs, err := NewIPCDiscoverer(m.logger, driverRoot)
-	if err != nil {
-		return nil, fmt.Errorf("failed to create discoverer for IPC sockets: %v", err)
-	}
-
-	ipcEdits, err := edits.FromDiscoverer(ipcs)
-	if err != nil {
-		return nil, fmt.Errorf("failed to create container edits for IPC sockets: %v", err)
-	}
-	// TODO: We should not have to update this after the fact
-	for _, s := range ipcEdits.Mounts {
-		s.Options = append(s.Options, "noexec")
-	}
-
-	allEdits.Append(ipcEdits)
-
 	common, err := NewCommonDiscoverer(m.logger, driverRoot, nvidiaCTKPath, nvmllib)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create discoverer for common entities: %v", err)
@@ -265,14 +247,12 @@ func (m command) generateSpec(driverRoot string, nvidiaCTKPath string, namer dev
 		return nil, fmt.Errorf("failed to create container edits for common entities: %v", err)
 	}
 
-	allEdits.Append(commonEdits)
-
 	// We construct the spec and determine the minimum required version based on the specification.
 	spec := specs.Spec{
 		Version:        "NOT_SET",
 		Kind:           "nvidia.com/gpu",
 		Devices:        deviceSpecs,
-		ContainerEdits: *allEdits.ContainerEdits,
+		ContainerEdits: *commonEdits.ContainerEdits,
 	}
 
 	minVersion, err := cdi.MinimumRequiredVersion(&spec)

internal/discover/char_devices.go

@@ -58,7 +58,11 @@ func (d *charDevices) Devices() ([]Device, error) {
 	}
 	var devices []Device
 	for _, mount := range devicesAsMounts {
-		devices = append(devices, Device(mount))
+		device := Device{
+			HostPath: mount.HostPath,
+			Path:     mount.Path,
+		}
+		devices = append(devices, device)
 	}
 
 	return devices, nil

internal/discover/discover.go

@@ -32,6 +32,7 @@ type Device struct {
 type Mount struct {
 	HostPath string
 	Path     string
+	Options  []string
 }
 
 // Hook represents a discovered hook.

internal/discover/icp_test.go

@@ -0,0 +1,60 @@
+/**
+# Copyright (c) NVIDIA CORPORATION.  All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+**/
+
+package discover
+
+import (
+	"testing"
+
+	"github.com/NVIDIA/nvidia-container-toolkit/internal/lookup"
+	"github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/require"
+)
+
+func TestIPCMounts(t *testing.T) {
+	l := ipcMounts(
+		mounts{
+			logger: logrus.New(),
+			lookup: &lookup.LocatorMock{
+				LocateFunc: func(path string) ([]string, error) {
+					return []string{"/host/path"}, nil
+				},
+			},
+			required: []string{"target"},
+		},
+	)
+
+	mounts, err := l.Mounts()
+	require.NoError(t, err)
+
+	require.EqualValues(
+		t,
+		[]Mount{
+			{
+				HostPath: "/host/path",
+				Path:     "/host/path",
+				Options: []string{
+					"ro",
+					"nosuid",
+					"nodev",
+					"bind",
+					"noexec",
+				},
+			},
+		},
+		mounts,
+	)
+}

internal/discover/ipc.go

@@ -14,17 +14,18 @@
 # limitations under the License.
 **/
 
-package generate
+package discover
 
 import (
-	"github.com/NVIDIA/nvidia-container-toolkit/internal/discover"
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/lookup"
 	"github.com/sirupsen/logrus"
 )
 
+type ipcMounts mounts
+
 // NewIPCDiscoverer creats a discoverer for NVIDIA IPC sockets.
-func NewIPCDiscoverer(logger *logrus.Logger, driverRoot string) (discover.Discover, error) {
+func NewIPCDiscoverer(logger *logrus.Logger, driverRoot string) (Discover, error) {
-	d := discover.NewMounts(
+	d := newMounts(
 		logger,
 		lookup.NewFileLocator(
 			lookup.WithLogger(logger),
@@ -38,5 +39,22 @@ func NewIPCDiscoverer(logger *logrus.Logger, driverRoot string) (discover.Discov
 		},
 	)
 
-	return d, nil
+	return (*ipcMounts)(d), nil
+}
+
+// Mounts returns the discovered mounts with "noexec" added to the mount options.
+func (d *ipcMounts) Mounts() ([]Mount, error) {
+	mounts, err := (*mounts)(d).Mounts()
+	if err != nil {
+		return nil, err
+	}
+
+	var modifiedMounts []Mount
+	for _, m := range mounts {
+		mount := m
+		mount.Options = append(m.Options, "noexec")
+		modifiedMounts = append(modifiedMounts, mount)
+	}
+
+	return modifiedMounts, nil
 }

internal/discover/mounts.go

@@ -43,6 +43,11 @@ var _ Discover = (*mounts)(nil)
 
 // NewMounts creates a discoverer for the required mounts using the specified locator.
 func NewMounts(logger *logrus.Logger, lookup lookup.Locator, root string, required []string) Discover {
+	return newMounts(logger, lookup, root, required)
+}
+
+// newMounts creates a discoverer for the required mounts using the specified locator.
+func newMounts(logger *logrus.Logger, lookup lookup.Locator, root string, required []string) *mounts {
 	return &mounts{
 		logger:   logger,
 		lookup:   lookup,
@@ -93,6 +98,12 @@ func (d *mounts) Mounts() ([]Mount, error) {
 			uniqueMounts[p] = Mount{
 				HostPath: p,
 				Path:     r,
+				Options: []string{
+					"ro",
+					"nosuid",
+					"nodev",
+					"bind",
+				},
 			}
 		}
 	}

internal/discover/mounts_test.go

@@ -35,6 +35,14 @@ func TestMountsReturnsEmptyDevices(t *testing.T) {
 }
 
 func TestMounts(t *testing.T) {
+
+	mountOptions := []string{
+		"ro",
+		"nosuid",
+		"nodev",
+		"bind",
+	}
+
 	logger, logHook := testlog.NewNullLogger()
 
 	testCases := []struct {
@@ -70,7 +78,7 @@ func TestMounts(t *testing.T) {
 				},
 				required: []string{"required"},
 			},
-			expectedMounts: []Mount{{Path: "located", HostPath: "located"}},
+			expectedMounts: []Mount{{Path: "located", HostPath: "located", Options: mountOptions}},
 		},
 		{
 			description:   "mounts removes located duplicates",
@@ -83,7 +91,7 @@ func TestMounts(t *testing.T) {
 				},
 				required: []string{"required0", "required1"},
 			},
-			expectedMounts: []Mount{{Path: "located", HostPath: "located"}},
+			expectedMounts: []Mount{{Path: "located", HostPath: "located", Options: mountOptions}},
 		},
 		{
 			description: "mounts skips located errors",
@@ -98,7 +106,7 @@ func TestMounts(t *testing.T) {
 				},
 				required: []string{"required0", "error", "required1"},
 			},
-			expectedMounts: []Mount{{Path: "required0", HostPath: "required0"}, {Path: "required1", HostPath: "required1"}},
+			expectedMounts: []Mount{{Path: "required0", HostPath: "required0", Options: mountOptions}, {Path: "required1", HostPath: "required1", Options: mountOptions}},
 		},
 		{
 			description: "mounts skips unlocated",
@@ -113,7 +121,7 @@ func TestMounts(t *testing.T) {
 				},
 				required: []string{"required0", "empty", "required1"},
 			},
-			expectedMounts: []Mount{{Path: "required0", HostPath: "required0"}, {Path: "required1", HostPath: "required1"}},
+			expectedMounts: []Mount{{Path: "required0", HostPath: "required0", Options: mountOptions}, {Path: "required1", HostPath: "required1", Options: mountOptions}},
 		},
 		{
 			description: "mounts adds multiple",
@@ -129,10 +137,10 @@ func TestMounts(t *testing.T) {
 				required: []string{"required0", "multiple", "required1"},
 			},
 			expectedMounts: []Mount{
-				{Path: "required0", HostPath: "required0"},
+				{Path: "required0", HostPath: "required0", Options: mountOptions},
-				{Path: "multiple0", HostPath: "multiple0"},
+				{Path: "multiple0", HostPath: "multiple0", Options: mountOptions},
-				{Path: "multiple1", HostPath: "multiple1"},
+				{Path: "multiple1", HostPath: "multiple1", Options: mountOptions},
-				{Path: "required1", HostPath: "required1"},
+				{Path: "required1", HostPath: "required1", Options: mountOptions},
 			},
 		},
 		{
@@ -147,7 +155,7 @@ func TestMounts(t *testing.T) {
 				required: []string{"required0", "multiple", "required1"},
 			},
 			expectedMounts: []Mount{
-				{Path: "/located", HostPath: "/some/root/located"},
+				{Path: "/located", HostPath: "/some/root/located", Options: mountOptions},
 			},
 		},
 	}

internal/edits/mount.go

@@ -40,12 +40,7 @@ func (d mount) toSpec() *specs.Mount {
 	s := specs.Mount{
 		HostPath:      d.HostPath,
 		ContainerPath: d.Path,
-		Options: []string{
+		Options:       d.Options,
-			"ro",
-			"nosuid",
-			"nodev",
-			"bind",
-		},
 	}
 
 	return &s