Disable chmod-hook by default

Add enable-chmod-hook option to cdi generate CLI
Update runtime factory to pass the enable-chmod-hook feature flag to the hook creator
Clean up With(En|Dis)abledHooks functions
In the nvcdi package, we prefer functions that accept
variadic arguments over ones that accept single arguments.
In the internal discover options, there is no need to expose
a generic optional function since we are already calling this
with typed parameters.
implement unit tests for NewHookCreator and methods
Raise an error if 'all' is included in enabled hooks
Add WithDebugLogging option
Update Hook creator to add WithEnabledChmodHook option

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
 }