Refactor handling of TOML config files for runtimes

cmd/nvidia-ctk/runtime/configure/configure.go

@@ -17,7 +17,6 @@
 package configure
 
 import (
-	"encoding/json"
 	"fmt"
 	"path/filepath"
 
@@ -29,6 +28,7 @@ import (
 	"github.com/NVIDIA/nvidia-container-toolkit/pkg/config/engine/crio"
 	"github.com/NVIDIA/nvidia-container-toolkit/pkg/config/engine/docker"
 	"github.com/NVIDIA/nvidia-container-toolkit/pkg/config/ocihook"
+	"github.com/NVIDIA/nvidia-container-toolkit/pkg/config/toml"
 )
 
 const (
@@ -156,13 +156,6 @@ func (m command) build() *cli.Command {
 			Usage:       "Enable CDI in the configured runtime",
 			Destination: &config.cdi.enabled,
 		},
-		&cli.StringFlag{
-			Name:        "runtime-config-override",
-			Destination: &config.runtimeConfigOverrideJSON,
-			Usage:       "specify additional runtime options as a JSON string. The paths are relative to the runtime config.",
-			Value:       "",
-			EnvVars:     []string{"RUNTIME_CONFIG_OVERRIDE"},
-		},
 	}
 
 	return &configure
@@ -234,11 +227,13 @@ func (m command) configureConfigFile(c *cli.Context, config *config) error {
 		cfg, err = containerd.New(
 			containerd.WithLogger(m.logger),
 			containerd.WithPath(configFilePath),
+			containerd.WithConfigSource(toml.FromFile(configFilePath)),
 		)
 	case "crio":
 		cfg, err = crio.New(
 			crio.WithLogger(m.logger),
 			crio.WithPath(configFilePath),
+			crio.WithConfigSource(toml.FromFile(configFilePath)),
 		)
 	case "docker":
 		cfg, err = docker.New(
@@ -252,16 +247,10 @@ func (m command) configureConfigFile(c *cli.Context, config *config) error {
 		return fmt.Errorf("unable to load config for runtime %v: %v", config.runtime, err)
 	}
 
-	runtimeConfigOverride, err := config.runtimeConfigOverride()
-	if err != nil {
-		return fmt.Errorf("unable to parse config overrides: %w", err)
-	}
-
 	err = cfg.AddRuntime(
 		config.nvidiaRuntime.name,
 		config.nvidiaRuntime.path,
 		config.nvidiaRuntime.setAsDefault,
-		runtimeConfigOverride,
 	)
 	if err != nil {
 		return fmt.Errorf("unable to update config: %v", err)
@@ -314,20 +303,6 @@ func (c *config) getOuputConfigPath() string {
 	return c.resolveConfigFilePath()
 }
 
-// runtimeConfigOverride converts the specified runtimeConfigOverride JSON string to a map.
-func (o *config) runtimeConfigOverride() (map[string]interface{}, error) {
-	if o.runtimeConfigOverrideJSON == "" {
-		return nil, nil
-	}
-
-	runtimeOptions := make(map[string]interface{})
-	if err := json.Unmarshal([]byte(o.runtimeConfigOverrideJSON), &runtimeOptions); err != nil {
-		return nil, fmt.Errorf("failed to read %v as JSON: %w", o.runtimeConfigOverrideJSON, err)
-	}
-
-	return runtimeOptions, nil
-}
-
 // configureOCIHook creates and configures the OCI hook for the NVIDIA runtime
 func (m *command) configureOCIHook(c *cli.Context, config *config) error {
 	err := ocihook.CreateHook(config.hookFilePath, config.nvidiaRuntime.hookPath)

pkg/config/engine/api.go

@@ -19,7 +19,7 @@ package engine
 // Interface defines the API for a runtime config updater.
 type Interface interface {
 	DefaultRuntime() string
-	AddRuntime(string, string, bool, ...map[string]interface{}) error
+	AddRuntime(string, string, bool) error
 	Set(string, interface{})
 	RemoveRuntime(string) error
 	Save(string) (int64, error)

pkg/config/engine/containerd/config_v1.go

@@ -30,7 +30,7 @@ type ConfigV1 Config
 var _ engine.Interface = (*ConfigV1)(nil)
 
 // AddRuntime adds a runtime to the containerd config
-func (c *ConfigV1) AddRuntime(name string, path string, setAsDefault bool, configOverrides ...map[string]interface{}) error {
+func (c *ConfigV1) AddRuntime(name string, path string, setAsDefault bool) error {
 	if c == nil || c.Tree == nil {
 		return fmt.Errorf("config is nil")
 	}
@@ -45,12 +45,14 @@ func (c *ConfigV1) AddRuntime(name string, path string, setAsDefault bool, confi
 		runtimeNamesForConfig = append(runtimeNamesForConfig, name)
 	}
 	for _, r := range runtimeNamesForConfig {
-		if options, ok := config.GetPath([]string{"plugins", "cri", "containerd", "runtimes", r}).(*toml.Tree); ok {
-			c.Logger.Debugf("using options from runtime %v: %v", r, options.String())
-			options, _ = toml.Load(options.String())
-			config.SetPath([]string{"plugins", "cri", "containerd", "runtimes", name}, options)
-			break
+		options := config.GetSubtreeByPath([]string{"plugins", "cri", "containerd", "runtimes", r})
+		if options == nil {
+			continue
 		}
+		c.Logger.Debugf("using options from runtime %v: %v", r, options)
+		config.SetPath([]string{"plugins", "cri", "containerd", "runtimes", name}, options.Copy())
+		break
+
 	}
 
 	if config.GetPath([]string{"plugins", "cri", "containerd", "runtimes", name}) == nil {
@@ -85,16 +87,6 @@ func (c *ConfigV1) AddRuntime(name string, path string, setAsDefault bool, confi
 		}
 		config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime", "options", "BinaryName"}, path)
 		config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime", "options", "Runtime"}, path)
-
-		defaultRuntimeSubtree := subtreeAtPath(config, "plugins", "cri", "containerd", "default_runtime")
-		if err := defaultRuntimeSubtree.applyOverrides(configOverrides...); err != nil {
-			return fmt.Errorf("failed to apply config overrides to default_runtime: %w", err)
-		}
-	}
-
-	runtimeSubtree := subtreeAtPath(config, "plugins", "cri", "containerd", "runtimes", name)
-	if err := runtimeSubtree.applyOverrides(configOverrides...); err != nil {
-		return fmt.Errorf("failed to apply config overrides: %w", err)
 	}
 
 	*c.Tree = config

pkg/config/engine/containerd/config_v1_test.go

@@ -19,20 +19,20 @@ package containerd
 import (
 	"testing"
 
-	"github.com/pelletier/go-toml"
 	testlog "github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/require"
+
+	"github.com/NVIDIA/nvidia-container-toolkit/pkg/config/toml"
 )
 
 func TestAddRuntimeV1(t *testing.T) {
 	logger, _ := testlog.NewNullLogger()
 	testCases := []struct {
-		description     string
-		config          string
-		setAsDefault    bool
-		configOverrides []map[string]interface{}
-		expectedConfig  string
-		expectedError   error
+		description    string
+		config         string
+		setAsDefault   bool
+		expectedConfig string
+		expectedError  error
 	}{
 		{
 			description: "empty config not default runtime",
@@ -53,32 +53,6 @@ func TestAddRuntimeV1(t *testing.T) {
 			`,
 			expectedError: nil,
 		},
-		{
-			description: "empty config not default runtime with overrides",
-			configOverrides: []map[string]interface{}{
-				{
-					"options": map[string]interface{}{
-						"SystemdCgroup": true,
-					},
-				},
-			},
-			expectedConfig: `
-			version = 1
-			[plugins]
-			[plugins.cri]
-				[plugins.cri.containerd]
-				[plugins.cri.containerd.runtimes]
-					[plugins.cri.containerd.runtimes.test]
-					privileged_without_host_devices = false
-					runtime_engine = ""
-					runtime_root = ""
-					runtime_type = ""
-					[plugins.cri.containerd.runtimes.test.options]
-						BinaryName = "/usr/bin/test"
-						Runtime = "/usr/bin/test"
-						SystemdCgroup = true
-			`,
-		},
 		{
 			description: "options from runc are imported",
 			config: `
@@ -226,20 +200,20 @@ func TestAddRuntimeV1(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.description, func(t *testing.T) {
-			config, err := toml.Load(tc.config)
+			cfg, err := toml.Load(tc.config)
 			require.NoError(t, err)
 			expectedConfig, err := toml.Load(tc.expectedConfig)
 			require.NoError(t, err)
 
 			c := &ConfigV1{
 				Logger: logger,
-				Tree:   config,
+				Tree:   cfg,
 			}
 
-			err = c.AddRuntime("test", "/usr/bin/test", tc.setAsDefault, tc.configOverrides...)
+			err = c.AddRuntime("test", "/usr/bin/test", tc.setAsDefault)
 			require.NoError(t, err)
 
-			require.EqualValues(t, expectedConfig.String(), config.String())
+			require.EqualValues(t, expectedConfig.String(), cfg.String())
 		})
 	}
 }

pkg/config/engine/containerd/config_v2.go

@@ -19,13 +19,11 @@ package containerd
 import (
 	"fmt"
 
-	"github.com/pelletier/go-toml"
-
-	"github.com/NVIDIA/nvidia-container-toolkit/pkg/config/engine"
+	"github.com/NVIDIA/nvidia-container-toolkit/pkg/config/toml"
 )
 
 // AddRuntime adds a runtime to the containerd config
-func (c *Config) AddRuntime(name string, path string, setAsDefault bool, configOverrides ...map[string]interface{}) error {
+func (c *Config) AddRuntime(name string, path string, setAsDefault bool) error {
 	if c == nil || c.Tree == nil {
 		return fmt.Errorf("config is nil")
 	}
@@ -39,12 +37,13 @@ func (c *Config) AddRuntime(name string, path string, setAsDefault bool, configO
 		runtimeNamesForConfig = append(runtimeNamesForConfig, name)
 	}
 	for _, r := range runtimeNamesForConfig {
-		if options, ok := config.GetPath([]string{"plugins", "io.containerd.grpc.v1.cri", "containerd", "runtimes", r}).(*toml.Tree); ok {
-			c.Logger.Debugf("using options from runtime %v: %v", r, options.String())
-			options, _ = toml.Load(options.String())
-			config.SetPath([]string{"plugins", "io.containerd.grpc.v1.cri", "containerd", "runtimes", name}, options)
-			break
+		options := config.GetSubtreeByPath([]string{"plugins", "io.containerd.grpc.v1.cri", "containerd", "runtimes", r})
+		if options == nil {
+			continue
 		}
+		c.Logger.Debugf("using options from runtime %v: %v", r, options)
+		config.SetPath([]string{"plugins", "io.containerd.grpc.v1.cri", "containerd", "runtimes", name}, options.Copy())
+		break
 	}
 
 	if config.GetPath([]string{"plugins", "io.containerd.grpc.v1.cri", "containerd", "runtimes", name}) == nil {
@@ -70,11 +69,6 @@ func (c *Config) AddRuntime(name string, path string, setAsDefault bool, configO
 		config.SetPath([]string{"plugins", "io.containerd.grpc.v1.cri", "containerd", "default_runtime_name"}, name)
 	}
 
-	runtimeSubtree := subtreeAtPath(config, "plugins", "io.containerd.grpc.v1.cri", "containerd", "runtimes", name)
-	if err := runtimeSubtree.applyOverrides(configOverrides...); err != nil {
-		return fmt.Errorf("failed to apply config overrides: %w", err)
-	}
-
 	*c.Tree = config
 	return nil
 }
@@ -151,15 +145,3 @@ func (c *Config) RemoveRuntime(name string) error {
 	*c.Tree = config
 	return nil
 }
-
-// Save writes the config to the specified path
-func (c Config) Save(path string) (int64, error) {
-	config := c.Tree
-	output, err := config.Marshal()
-	if err != nil {
-		return 0, fmt.Errorf("unable to convert to TOML: %v", err)
-	}
-
-	n, err := engine.Config(path).Write(output)
-	return int64(n), err
-}

pkg/config/engine/containerd/config_v2_test.go

@@ -19,20 +19,20 @@ package containerd
 import (
 	"testing"
 
-	"github.com/pelletier/go-toml"
 	testlog "github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/require"
+
+	"github.com/NVIDIA/nvidia-container-toolkit/pkg/config/toml"
 )
 
 func TestAddRuntime(t *testing.T) {
 	logger, _ := testlog.NewNullLogger()
 	testCases := []struct {
-		description     string
-		config          string
-		setAsDefault    bool
-		configOverrides []map[string]interface{}
-		expectedConfig  string
-		expectedError   error
+		description    string
+		config         string
+		setAsDefault   bool
+		expectedConfig string
+		expectedError  error
 	}{
 		{
 			description: "empty config not default runtime",
@@ -52,31 +52,6 @@ func TestAddRuntime(t *testing.T) {
 			`,
 			expectedError: nil,
 		},
-		{
-			description: "empty config not default runtime with overrides",
-			configOverrides: []map[string]interface{}{
-				{
-					"options": map[string]interface{}{
-						"SystemdCgroup": true,
-					},
-				},
-			},
-			expectedConfig: `
-			version = 2
-			[plugins]
-			[plugins."io.containerd.grpc.v1.cri"]
-				[plugins."io.containerd.grpc.v1.cri".containerd]
-				[plugins."io.containerd.grpc.v1.cri".containerd.runtimes]
-					[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.test]
-					privileged_without_host_devices = false
-					runtime_engine = ""
-					runtime_root = ""
-					runtime_type = ""
-					[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.test.options]
-						BinaryName = "/usr/bin/test"
-						SystemdCgroup = true
-			`,
-		},
 		{
 			description: "options from runc are imported",
 			config: `
@@ -224,20 +199,20 @@ func TestAddRuntime(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.description, func(t *testing.T) {
-			config, err := toml.Load(tc.config)
+			cfg, err := toml.Load(tc.config)
 			require.NoError(t, err)
 			expectedConfig, err := toml.Load(tc.expectedConfig)
 			require.NoError(t, err)
 
 			c := &Config{
 				Logger: logger,
-				Tree:   config,
+				Tree:   cfg,
 			}
 
-			err = c.AddRuntime("test", "/usr/bin/test", tc.setAsDefault, tc.configOverrides...)
+			err = c.AddRuntime("test", "/usr/bin/test", tc.setAsDefault)
 			require.NoError(t, err)
 
-			require.EqualValues(t, expectedConfig.String(), config.String())
+			require.EqualValues(t, expectedConfig.String(), cfg.String())
 		})
 	}
 }