Disable chmod-hook by default

This pull request introduces enhanced configurability for nvidia-ctk cdi generate, allowing users to explicitly enable or disable specific hooks, with enabled hooks taking precedence over disabled ones.
If "enable-hook" is set to "all" an error will be displayed as this is not supported.
The Hook CHMOD is now disabled by default, and can be enabled via the new cdi generate flag "enable-hook".
This patch also implement unit tests for NewHookCreator and methods.

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

Author: ArangoGutierrez

cmd/nvidia-ctk-installer/toolkit/toolkit_test.go

@@ -84,6 +84,8 @@ devices:
               hostPath: /host/driver/root/dev/nvidia-caps-imex-channels/channel1
             - path: /dev/nvidia-caps-imex-channels/channel2047
               hostPath: /host/driver/root/dev/nvidia-caps-imex-channels/channel2047
+            - path: /dev/nvidia-caps/nvidia-cap1
+              hostPath: /host/driver/root/dev/nvidia-caps/nvidia-cap1
 containerEdits:
     env:
         - NVIDIA_CTK_LIBCUDA_DIR=/lib/x86_64-linux-gnu

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

@@ -62,6 +62,7 @@ type options struct {
 	configSearchPaths  []string
 	librarySearchPaths []string
 	disabledHooks      []string
+	enabledHooks       []string
 
 	csv struct {
 		files          []string
@@ -214,6 +215,13 @@ func (m command) build() *cli.Command {
 				Destination: &opts.disabledHooks,
 				Sources:     cli.EnvVars("NVIDIA_CTK_CDI_GENERATE_DISABLED_HOOKS"),
 			},
+			&cli.StringSliceFlag{
+				Name:        "enable-hook",
+				Aliases:     []string{"enable-hooks"},
+				Usage:       "Explicitly enable a hook in the generated CDI specification. This overrides disabled hooks. This can be specified multiple times.",
+				Destination: &opts.enabledHooks,
+				Sources:     cli.EnvVars("NVIDIA_CTK_CDI_GENERATE_ENABLED_HOOKS"),
+			},
 		},
 	}
 
@@ -258,6 +266,12 @@ func (m command) validateFlags(c *cli.Command, opts *options) error {
 	if err := cdi.ValidateClassName(opts.class); err != nil {
 		return fmt.Errorf("invalid CDI class name: %v", err)
 	}
+
+	for _, hook := range opts.enabledHooks {
+		if hook == "all" {
+			return fmt.Errorf("enabling all hooks is not supported")
+		}
+	}
 	return nil
 }
 
@@ -313,14 +327,12 @@ func (m command) generateSpec(opts *options) (spec.Interface, error) {
 		nvcdi.WithLibrarySearchPaths(opts.librarySearchPaths),
 		nvcdi.WithCSVFiles(opts.csv.files),
 		nvcdi.WithCSVIgnorePatterns(opts.csv.ignorePatterns),
+		nvcdi.WithDisabledHooks(opts.disabledHooks...),
+		nvcdi.WithEnabledHooks(opts.enabledHooks...),
 		// We set the following to allow for dependency injection:
 		nvcdi.WithNvmlLib(opts.nvmllib),
 	}
 
-	for _, hook := range opts.disabledHooks {
-		cdiOptions = append(cdiOptions, nvcdi.WithDisabledHook(hook))
-	}
-
 	cdilib, err := nvcdi.New(cdiOptions...)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create CDI library: %v", err)

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

@@ -359,6 +359,91 @@ containerEdits:
             - nodev
             - rbind
             - rprivate
+`,
+		},
+		{
+			description: "enableChmodHook",
+			options: options{
+				format:        "yaml",
+				mode:          "management",
+				vendor:        "example.com",
+				class:         "device",
+				driverRoot:    driverRoot,
+				enabledHooks:  []string{"chmod"},
+				disabledHooks: []string{"enable-cuda-compat", "update-ldcache", "disable-device-node-modification"},
+			},
+			expectedOptions: options{
+				format:            "yaml",
+				mode:              "management",
+				vendor:            "example.com",
+				class:             "device",
+				nvidiaCDIHookPath: "/usr/bin/nvidia-cdi-hook",
+				driverRoot:        driverRoot,
+				enabledHooks:      []string{"chmod"},
+				disabledHooks:     []string{"enable-cuda-compat", "update-ldcache", "disable-device-node-modification"},
+			},
+			expectedSpec: `---
+cdiVersion: 0.5.0
+kind: example.com/device
+devices:
+    - name: all
+      containerEdits:
+        deviceNodes:
+            - path: /dev/nvidia0
+              hostPath: {{ .driverRoot }}/dev/nvidia0
+            - path: /dev/nvidiactl
+              hostPath: {{ .driverRoot }}/dev/nvidiactl
+            - path: /dev/nvidia-caps-imex-channels/channel0
+              hostPath: {{ .driverRoot }}/dev/nvidia-caps-imex-channels/channel0
+            - path: /dev/nvidia-caps-imex-channels/channel1
+              hostPath: {{ .driverRoot }}/dev/nvidia-caps-imex-channels/channel1
+            - path: /dev/nvidia-caps-imex-channels/channel2047
+              hostPath: {{ .driverRoot }}/dev/nvidia-caps-imex-channels/channel2047
+            - path: /dev/nvidia-caps/nvidia-cap1
+              hostPath: {{ .driverRoot }}/dev/nvidia-caps/nvidia-cap1
+        hooks:
+            - hookName: createContainer
+              path: /usr/bin/nvidia-cdi-hook
+              args:
+                - nvidia-cdi-hook
+                - chmod
+                - --mode
+                - "755"
+                - --path
+                - /dev/nvidia-caps
+              env:
+                - NVIDIA_CTK_DEBUG=false
+containerEdits:
+    env:
+        - NVIDIA_CTK_LIBCUDA_DIR=/lib/x86_64-linux-gnu
+        - NVIDIA_VISIBLE_DEVICES=void
+    hooks:
+        - hookName: createContainer
+          path: /usr/bin/nvidia-cdi-hook
+          args:
+            - nvidia-cdi-hook
+            - create-symlinks
+            - --link
+            - libcuda.so.1::/lib/x86_64-linux-gnu/libcuda.so
+          env:
+            - NVIDIA_CTK_DEBUG=false
+    mounts:
+        - hostPath: {{ .driverRoot }}/lib/x86_64-linux-gnu/libcuda.so.999.88.77
+          containerPath: /lib/x86_64-linux-gnu/libcuda.so.999.88.77
+          options:
+            - ro
+            - nosuid
+            - nodev
+            - rbind
+            - rprivate
+        - hostPath: {{ .driverRoot }}/lib/x86_64-linux-gnu/vdpau/libvdpau_nvidia.so.999.88.77
+          containerPath: /lib/x86_64-linux-gnu/vdpau/libvdpau_nvidia.so.999.88.77
+          options:
+            - ro
+            - nosuid
+            - nodev
+            - rbind
+            - rprivate
 `,
 		},
 	}

internal/discover/hooks.go

@@ -50,6 +50,14 @@ const (
 	defaultNvidiaCDIHookPath = "/usr/bin/nvidia-cdi-hook"
 )
 
+// defaultDisabledHooks defines hooks that are disabled by default.
+// These hooks can be explicitly enabled using the WithEnabledHooks option.
+var defaultDisabledHooks = []HookName{
+	// ChmodHook is disabled by default as it was a workaround for older
+	// versions of crun that has since been fixed.
+	ChmodHook,
+}
+
 var _ Discover = (*Hook)(nil)
 
 // Devices returns an empty list of devices for a Hook discoverer.
@@ -77,7 +85,14 @@ func (h *Hook) Hooks() ([]Hook, error) {
 	return []Hook{*h}, nil
 }
 
-type Option func(*cdiHookCreator)
+type hookCreatorOptions struct {
+	nvidiaCDIHookPath string
+	disabledHooks     []HookName
+	enabledHooks      []HookName
+	debugLogging      bool
+}
+
+type Option func(*hookCreatorOptions)
 
 type cdiHookCreator struct {
 	nvidiaCDIHookPath string
@@ -100,39 +115,66 @@ type HookCreator interface {
 	Create(HookName, ...string) *Hook
 }
 
-// WithDisabledHooks sets the set of hooks that are disabled for the CDI hook creator.
+func WithDebugLogging(debugLogging bool) Option {
+	return func(hco *hookCreatorOptions) {
+		hco.debugLogging = debugLogging
+	}
+}
+
+// WithDisabledHooks explicitly disables the specified hooks.
 // This can be specified multiple times.
 func WithDisabledHooks(hooks ...HookName) Option {
-	return func(c *cdiHookCreator) {
-		for _, hook := range hooks {
-			c.disabledHooks[hook] = true
-		}
+	return func(c *hookCreatorOptions) {
+		c.disabledHooks = append(c.disabledHooks, hooks...)
+	}
+}
+
+// WithEnabledHooks explicitly enables the specified hooks.
+// This is useful for enabling hooks that are disabled by default.
+func WithEnabledHooks(hooks ...HookName) Option {
+	return func(c *hookCreatorOptions) {
+		c.enabledHooks = append(c.enabledHooks, hooks...)
 	}
 }
 
 // WithNVIDIACDIHookPath sets the path to the nvidia-cdi-hook binary.
 func WithNVIDIACDIHookPath(nvidiaCDIHookPath string) Option {
-	return func(c *cdiHookCreator) {
+	return func(c *hookCreatorOptions) {
 		c.nvidiaCDIHookPath = nvidiaCDIHookPath
 	}
 }
 
 func NewHookCreator(opts ...Option) HookCreator {
-	cdiHookCreator := &cdiHookCreator{
+	o := &hookCreatorOptions{
 		nvidiaCDIHookPath: defaultNvidiaCDIHookPath,
-		disabledHooks:     make(map[HookName]bool),
 	}
 	for _, opt := range opts {
-		opt(cdiHookCreator)
+		opt(o)
 	}
 
-	if cdiHookCreator.disabledHooks[AllHooks] {
+	o.disabledHooks = append(o.disabledHooks, defaultDisabledHooks...)
+
+	disabledHooks := make(map[HookName]bool)
+	for _, h := range o.disabledHooks {
+		disabledHooks[h] = true
+	}
+
+	if disabledHooks[AllHooks] && len(o.enabledHooks) == 0 {
 		return &allDisabledHookCreator{}
 	}
 
-	cdiHookCreator.fixedArgs = getFixedArgsForCDIHookCLI(cdiHookCreator.nvidiaCDIHookPath)
+	for _, h := range o.enabledHooks {
+		disabledHooks[h] = false
+	}
+
+	c := &cdiHookCreator{
+		nvidiaCDIHookPath: o.nvidiaCDIHookPath,
+		disabledHooks:     disabledHooks,
+		fixedArgs:         getFixedArgsForCDIHookCLI(o.nvidiaCDIHookPath),
+		debugLogging:      o.debugLogging,
+	}
 
-	return cdiHookCreator
+	return c
 }
 
 // Create creates a new hook with the given name and arguments.
@@ -150,21 +192,19 @@ func (c cdiHookCreator) Create(name HookName, args ...string) *Hook {
 	}
 }
 
-// isDisabled checks if the specified hook name is disabled.
 func (c cdiHookCreator) isDisabled(name HookName, args ...string) bool {
-	if c.disabledHooks[name] {
+	disabled, ok := c.disabledHooks[name]
+	if ok {
+		return disabled
+	}
+	if c.disabledHooks[AllHooks] {
 		return true
 	}
 
+	// still reject hooks that require args if none were provided
 	switch name {
-	case CreateSymlinksHook:
-		if len(args) == 0 {
-			return true
-		}
-	case ChmodHook:
-		if len(args) == 0 {
-			return true
-		}
+	case CreateSymlinksHook, ChmodHook:
+		return len(args) == 0
 	}
 	return false
 }