Unify GetDevices logic at internal/config/image

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
Signed-off-by: Evan Lezar <[email protected]>

Author: ArangoGutierrez

cmd/nvidia-container-runtime-hook/container_config.go

@@ -13,10 +13,6 @@ import (
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/config/image"
 )
 
-const (
-	capSysAdmin = "CAP_SYS_ADMIN"
-)
-
 type nvidiaConfig struct {
 	Devices            []string
 	MigConfigDevices   string
@@ -103,9 +99,9 @@ func loadSpec(path string) (spec *Spec) {
 	return
 }
 
-func isPrivileged(s *Spec) bool {
-	if s.Process.Capabilities == nil {
-		return false
+func (s *Spec) GetCapabilities() []string {
+	if s == nil || s.Process == nil || s.Process.Capabilities == nil {
+		return nil
 	}
 
 	var caps []string
@@ -118,67 +114,22 @@ func isPrivileged(s *Spec) bool {
 		if err != nil {
 			log.Panicln("could not decode Process.Capabilities in OCI spec:", err)
 		}
-		for _, c := range caps {
-			if c == capSysAdmin {
-				return true
-			}
-		}
-		return false
+		return caps
 	}
 
 	// Otherwise, parse s.Process.Capabilities as:
 	// github.com/opencontainers/runtime-spec/blob/v1.0.0/specs-go/config.go#L30-L54
-	process := specs.Process{
-		Env: s.Process.Env,
-	}
-
-	err := json.Unmarshal(*s.Process.Capabilities, &process.Capabilities)
+	capabilities := specs.LinuxCapabilities{}
+	err := json.Unmarshal(*s.Process.Capabilities, &capabilities)
 	if err != nil {
 		log.Panicln("could not decode Process.Capabilities in OCI spec:", err)
 	}
 
-	fullSpec := specs.Spec{
-		Version: *s.Version,
-		Process: &process,
-	}
-
-	return image.IsPrivileged(&fullSpec)
+	return image.OCISpecCapabilities(capabilities).GetCapabilities()
 }
 
-func getDevicesFromEnvvar(containerImage image.CUDA, swarmResourceEnvvars []string) []string {
-	// We check if the image has at least one of the Swarm resource envvars defined and use this
-	// if specified.
-	for _, envvar := range swarmResourceEnvvars {
-		if containerImage.HasEnvvar(envvar) {
-			return containerImage.DevicesFromEnvvars(swarmResourceEnvvars...).List()
-		}
-	}
-
-	return containerImage.VisibleDevicesFromEnvVar()
-}
-
-func (hookConfig *hookConfig) getDevices(image image.CUDA, privileged bool) []string {
-	// If enabled, try and get the device list from volume mounts first
-	if hookConfig.AcceptDeviceListAsVolumeMounts {
-		devices := image.VisibleDevicesFromMounts()
-		if len(devices) > 0 {
-			return devices
-		}
-	}
-
-	// Fallback to reading from the environment variable if privileges are correct
-	devices := getDevicesFromEnvvar(image, hookConfig.getSwarmResourceEnvvars())
-	if len(devices) == 0 {
-		return nil
-	}
-	if privileged || hookConfig.AcceptEnvvarUnprivileged {
-		return devices
-	}
-
-	configName := hookConfig.getConfigOption("AcceptEnvvarUnprivileged")
-	log.Printf("Ignoring devices specified in NVIDIA_VISIBLE_DEVICES (privileged=%v, %v=%v) ", privileged, configName, hookConfig.AcceptEnvvarUnprivileged)
-
-	return nil
+func isPrivileged(s *Spec) bool {
+	return image.IsPrivileged(s)
 }
 
 func getMigConfigDevices(i image.CUDA) *string {
@@ -225,7 +176,6 @@ func (hookConfig *hookConfig) getDriverCapabilities(cudaImage image.CUDA, legacy
 	// We use the default driver capabilities by default. This is filtered to only include the
 	// supported capabilities
 	supportedDriverCapabilities := image.NewDriverCapabilities(hookConfig.SupportedDriverCapabilities)
-
 	capabilities := supportedDriverCapabilities.Intersection(image.DefaultDriverCapabilities)
 
 	capsEnvSpecified := cudaImage.HasEnvvar(image.EnvVarNvidiaDriverCapabilities)
@@ -251,7 +201,7 @@ func (hookConfig *hookConfig) getDriverCapabilities(cudaImage image.CUDA, legacy
 func (hookConfig *hookConfig) getNvidiaConfig(image image.CUDA, privileged bool) *nvidiaConfig {
 	legacyImage := image.IsLegacy()
 
-	devices := hookConfig.getDevices(image, privileged)
+	devices := image.VisibleDevices()
 	if len(devices) == 0 {
 		// empty devices means this is not a GPU container.
 		return nil
@@ -306,20 +256,27 @@ func (hookConfig *hookConfig) getContainerConfig() (config containerConfig) {
 
 	s := loadSpec(path.Join(b, "config.json"))
 
-	image, err := image.New(
+	privileged := isPrivileged(s)
+
+	opts := []image.Option{
 		image.WithEnv(s.Process.Env),
 		image.WithMounts(s.Mounts),
+		image.WithPrivileged(privileged),
 		image.WithDisableRequire(hookConfig.DisableRequire),
-	)
+		image.WithAcceptDeviceListAsVolumeMounts(hookConfig.AcceptDeviceListAsVolumeMounts),
+		image.WithAcceptEnvvarUnprivileged(hookConfig.AcceptEnvvarUnprivileged),
+		image.WithAdditionalVisibleDevicesEnvVars(hookConfig.getSwarmResourceEnvvars()...),
+	}
+
+	i, err := image.New(opts...)
 	if err != nil {
 		log.Panicln(err)
 	}
 
-	privileged := isPrivileged(s)
 	return containerConfig{
 		Pid:    h.Pid,
 		Rootfs: s.Root.Path,
-		Image:  image,
-		Nvidia: hookConfig.getNvidiaConfig(image, privileged),
+		Image:  i,
+		Nvidia: hookConfig.getNvidiaConfig(i, privileged),
 	}
 }

cmd/nvidia-container-runtime-hook/container_config_test.go

@@ -477,9 +477,19 @@ func TestGetNvidiaConfig(t *testing.T) {
 	}
 	for _, tc := range tests {
 		t.Run(tc.description, func(t *testing.T) {
-			image, _ := image.New(
+			opts := []image.Option{
 				image.WithEnvMap(tc.env),
-			)
+				image.WithPrivileged(tc.privileged),
+				image.WithAcceptEnvvarUnprivileged(true),
+			}
+
+			if tc.hookConfig != nil {
+				if tc.hookConfig.SwarmResource != "" {
+					opts = append(opts, image.WithAdditionalVisibleDevicesEnvVars(tc.hookConfig.SwarmResource))
+				}
+			}
+			image, _ := image.New(opts...)
+
 			// Wrap the call to getNvidiaConfig() in a closure.
 			var cfg *nvidiaConfig
 			getConfig := func() {
@@ -622,12 +632,11 @@ func TestDeviceListSourcePriority(t *testing.T) {
 						},
 					),
 					image.WithMounts(tc.mountDevices),
+					image.WithPrivileged(tc.privileged),
+					image.WithAcceptDeviceListAsVolumeMounts(tc.acceptMounts),
+					image.WithAcceptEnvvarUnprivileged(tc.acceptUnprivileged),
 				)
-				defaultConfig, _ := config.GetDefault()
-				cfg := &hookConfig{defaultConfig}
-				cfg.AcceptEnvvarUnprivileged = tc.acceptUnprivileged
-				cfg.AcceptDeviceListAsVolumeMounts = tc.acceptMounts
-				devices = cfg.getDevices(image, tc.privileged)
+				devices = image.VisibleDevices()
 			}
 
 			// For all other tests, just grab the devices and check the results
@@ -843,10 +852,17 @@ func TestGetDevicesFromEnvvar(t *testing.T) {
 
 	for _, tc := range tests {
 		t.Run(tc.description, func(t *testing.T) {
-			image, _ := image.New(
+			opts := []image.Option{
 				image.WithEnvMap(tc.env),
-			)
-			devices := getDevicesFromEnvvar(image, tc.swarmResourceEnvvars)
+				image.WithPrivileged(true),
+				image.WithAcceptDeviceListAsVolumeMounts(false),
+				image.WithAcceptEnvvarUnprivileged(false),
+				image.WithAdditionalVisibleDevicesEnvVars(tc.swarmResourceEnvvars...),
+			}
+
+			image, _ := image.New(opts...)
+
+			devices := image.VisibleDevices()
 			require.EqualValues(t, tc.expectedDevices, devices)
 		})
 	}

internal/config/image/builder.go

@@ -24,11 +24,14 @@ import (
 )
 
 type builder struct {
-	env            map[string]string
-	mounts         []specs.Mount
+	CUDA
+
 	disableRequire bool
 }
 
+// Option is a functional option for creating a CUDA image.
+type Option func(*builder) error
+
 // New creates a new CUDA image from the input options.
 func New(opt ...Option) (CUDA, error) {
 	b := &builder{}
@@ -50,15 +53,22 @@ func (b builder) build() (CUDA, error) {
 		b.env[EnvVarNvidiaDisableRequire] = "true"
 	}
 
-	c := CUDA{
-		env:    b.env,
-		mounts: b.mounts,
+	return b.CUDA, nil
+}
+
+func WithAcceptDeviceListAsVolumeMounts(acceptDeviceListAsVolumeMounts bool) Option {
+	return func(b *builder) error {
+		b.acceptDeviceListAsVolumeMounts = acceptDeviceListAsVolumeMounts
+		return nil
 	}
-	return c, nil
 }
 
-// Option is a functional option for creating a CUDA image.
-type Option func(*builder) error
+func WithAcceptEnvvarUnprivileged(acceptEnvvarUnprivileged bool) Option {
+	return func(b *builder) error {
+		b.acceptEnvvarUnprivileged = acceptEnvvarUnprivileged
+		return nil
+	}
+}
 
 // WithDisableRequire sets the disable require option.
 func WithDisableRequire(disableRequire bool) Option {
@@ -100,3 +110,32 @@ func WithMounts(mounts []specs.Mount) Option {
 		return nil
 	}
 }
+
+// WithPrivileged sets whether an image is privileged or not.
+func WithPrivileged(isPrivileged bool) Option {
+	return func(b *builder) error {
+		b.isPrivileged = isPrivileged
+		return nil
+	}
+}
+
+// WithAdditionalVisibleDevicesEnvVars sets the visible devices environment variables.
+// For each envvar passed:
+//   - Split on commas
+//   - Trim spaces and ignore empty strings
+//   - Concatenate all of this
+func WithAdditionalVisibleDevicesEnvVars(visibleDevicesEnvVars ...string) Option {
+	return func(b *builder) error {
+		var result []string
+		for _, v := range visibleDevicesEnvVars {
+			for _, c := range strings.Split(v, ",") {
+				trimmed := strings.TrimSpace(c)
+				if trimmed != "" {
+					result = append(result, trimmed)
+				}
+			}
+		}
+		b.additionalVisibleDevicesEnvVars = result
+		return nil
+	}
+}

internal/config/image/builder_test.go

@@ -0,0 +1,69 @@
+package image
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestWithAdditionalVisibleDevicesEnvVars(t *testing.T) {
+	testCases := []struct {
+		desc   string
+		input  []string
+		expect []string
+	}{
+		{
+			desc:   "single value, no comma",
+			input:  []string{"FOO"},
+			expect: []string{"FOO"},
+		},
+		{
+			desc:   "single value, with spaces",
+			input:  []string{"  BAR  "},
+			expect: []string{"BAR"},
+		},
+		{
+			desc:   "single string, comma separated",
+			input:  []string{"A,B,C"},
+			expect: []string{"A", "B", "C"},
+		},
+		{
+			desc:   "single string, comma separated with spaces",
+			input:  []string{"  A ,  B, C  ,   "},
+			expect: []string{"A", "B", "C"},
+		},
+		{
+			desc:   "multiple values, no commas",
+			input:  []string{"A", "B", "C"},
+			expect: []string{"A", "B", "C"},
+		},
+		{
+			desc:   "multiple values, some empty",
+			input:  []string{"A", "", "C"},
+			expect: []string{"A", "C"},
+		},
+		{
+			desc:   "multiple values, one is comma separated",
+			input:  []string{"A,B", "C"},
+			expect: []string{"A", "B", "C"},
+		},
+		{
+			desc:   "single string, only spaces and commas",
+			input:  []string{" , , , "},
+			expect: nil,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			b := &builder{}
+			err := WithAdditionalVisibleDevicesEnvVars(tc.input...)(b)
+			require.NoError(t, err)
+			if tc.expect == nil {
+				require.Nil(t, b.additionalVisibleDevicesEnvVars)
+			} else {
+				require.Equal(t, tc.expect, b.additionalVisibleDevicesEnvVars)
+			}
+		})
+	}
+}