Merge pull request #1745 from NVIDIA/fix-additional-gids

fix: Additional GIDs are dropped due to file mode mask

Author: cdesiniotis

internal/edits/device.go

@@ -130,9 +130,6 @@ func (d *device) getAdditionalGIDs(dn *specs.DeviceNode) []uint32 {
 	if dn.FileMode == nil {
 		return nil
 	}
-	if dn.FileMode.Type()&os.ModeCharDevice == 0 {
-		return nil
-	}
 	if permission := dn.FileMode.Perm(); isWorldReadable(permission) && isWorldWriteable(permission) {
 		return nil
 	}

internal/edits/device_test.go

@@ -23,13 +23,90 @@ import (
 
 	"github.com/opencontainers/cgroups/devices/config"
 	"github.com/stretchr/testify/require"
+	"tags.cncf.io/container-device-interface/pkg/cdi"
 	"tags.cncf.io/container-device-interface/specs-go"
 
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/devices"
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/discover"
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/test/to"
 )
 
+func TestDeviceToEdits(t *testing.T) {
+	testCases := []struct {
+		description string
+		device      discover.Device
+		deviceslib  devices.Interface
+		expected    *cdi.ContainerEdits
+	}{
+		{
+			device: discover.Device{
+				Path: "/foo",
+			},
+			expected: &cdi.ContainerEdits{
+				ContainerEdits: &specs.ContainerEdits{
+					DeviceNodes: []*specs.DeviceNode{
+						{Path: "/foo"},
+					},
+				},
+			},
+		},
+		{
+			description: "device with additional GIDs",
+			device: discover.Device{
+				Path: "/foo",
+			},
+			deviceslib: &devices.InterfaceMock{
+				DeviceFromPathFunc: func(path, permissions string) (*devices.Device, error) {
+					if path != "/foo" {
+						return nil, fmt.Errorf("not found %v", path)
+					}
+					cd := &config.Device{
+						Rule: config.Rule{
+							Major:       100,
+							Minor:       200,
+							Permissions: config.Permissions("w"),
+						},
+						// The bits which indicate this is a character device in the filemode
+						// have been masked. This mimics the behavior of the real DeviceFromPath
+						// function.
+						FileMode: 0660 & os.ModePerm,
+						Uid:      11,
+						Gid:      44,
+					}
+
+					return (*devices.Device)(cd), nil
+				},
+			},
+			expected: &cdi.ContainerEdits{
+				ContainerEdits: &specs.ContainerEdits{
+					DeviceNodes: []*specs.DeviceNode{
+						{
+							Path:        "/foo",
+							HostPath:    "",
+							Permissions: "w",
+							Major:       100,
+							Minor:       200,
+							FileMode:    to.Ptr(0660 & os.ModePerm),
+							GID:         ptrIfNonZero[uint32](44),
+						},
+					},
+					AdditionalGIDs: []uint32{44},
+				},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		f := factory{}
+		t.Run(tc.description, func(t *testing.T) {
+			defer devices.SetInterfaceForTests(tc.deviceslib)()
+			edits, err := f.device(tc.device).toEdits()
+			require.NoError(t, err)
+			require.EqualValues(t, tc.expected, edits)
+		})
+	}
+}
+
 func TestDeviceToSpec(t *testing.T) {
 	testCases := []struct {
 		description string

pkg/nvcdi/lib-csv_test.go

@@ -182,6 +182,7 @@ func TestDeviceSpecGenerators(t *testing.T) {
 				{
 					Name: "0",
 					ContainerEdits: specs.ContainerEdits{
+						AdditionalGIDs: []uint32{44},
 						DeviceNodes: []*specs.DeviceNode{
 							{Path: "/dev/nvidia0", HostPath: "/dev/nvidia0"},
 							{Path: "/dev/nvidiactl", HostPath: "/dev/nvidiactl"},
@@ -193,6 +194,7 @@ func TestDeviceSpecGenerators(t *testing.T) {
 				{
 					Name: "1",
 					ContainerEdits: specs.ContainerEdits{
+						AdditionalGIDs: []uint32{44},
 						DeviceNodes: []*specs.DeviceNode{
 							{Path: "/dev/nvidia1", HostPath: "/dev/nvidia1"},
 							{Path: "/dev/nvidiactl", HostPath: "/dev/nvidiactl"},