Pass along containerd config as return value

Return the containerd config as a string instead of having the configurer write it to disk. This reduces the number of struct fields required by the configurer, and makes testing less dependent on the file system. Let the containerd component write the file instead. Remove the hard-coded string literal for the paths to containerd-cri.toml and use k0s' RunDir instead. This already takes into account the differences between Windows and UNIX-like operating systems, and also takes into account the different defaults when running as non-root user. Remove the escapedPath function and inline it directly before the containerd component generates the template. Signed-off-by: Tom Wieczorek <[email protected]>
k0sproject · Mar 27, 2024 · cec9435 · cec9435
1 parent 0d821c6
commit cec9435
Show file tree

Hide file tree

Showing 4 changed files with 97 additions and 91 deletions.
diff --git a/docs/runtime.md b/docs/runtime.md
@@ -54,9 +54,22 @@ state = "/run/k0s/containerd"
 
 ## k0s managed dynamic runtime configuration
 
-From 1.27.1 onwards k0s enables dynamic configuration on containerd CRI runtimes. This works by k0s creating a special directory in `/etc/k0s/containerd.d/` where user can drop-in partial containerd configuration snippets.
-
-k0s will automatically pick up these files and adds these in containerd configuration `imports` list. If k0s sees the configuration drop-ins are CRI related configurations k0s will automatically collect all these into a single file and adds that as a single import file. This is to overcome some hard limitation on containerd 1.X versions. Read more at [containerd#8056](https://github.com/containerd/containerd/pull/8056)
+As of 1.27.1, k0s allows dynamic configuration of containerd CRI runtimes. This
+works by k0s creating a special directory in `/etc/k0s/containerd.d/` where
+users can place partial containerd configuration files.
+
+K0s will automatically pick up these files and add them as containerd
+configuration `imports`. If a partial configuration file contains a CRI plugin
+configuration section, k0s will instead treat such a file as a [merge patch] to
+k0s's default containerd configuration. This is to mitigate [containerd's
+decision] to replace rather than merge individual plugin configuration sections
+from imported configuration files. However, this behavior [may][containerd#7347]
+[change][containerd#9982] in future releases of containerd.
+
+[merge patch]: https://datatracker.ietf.org/doc/html/rfc7396
+[containerd's decision]: https://github.com/containerd/containerd/pull/3574/commits/24b9e2c1a0a72a7ad302cdce7da3abbc4e6295cb
+[containerd#7347]: https://github.com/containerd/containerd/pull/7347
+[containerd#9982]: https://github.com/containerd/containerd/pull/9982
 
 ### Examples
 
@@ -146,7 +159,7 @@ distribution=$(. /etc/os-release;echo $ID$VERSION_ID) \
 sudo apt-get update && sudo apt-get install -y nvidia-container-runtime
 ```
 
-Next, drop in the containerd runtime configuration snippet into `/etc/k0s/containerd.d/nvidia.toml`
+Next, drop in the NVIDIA runtime's configuration into into `/etc/k0s/containerd.d/nvidia.toml`:
 
 ```toml
 [plugins."io.containerd.grpc.v1.cri".containerd.runtimes.nvidia]

diff --git a/pkg/component/worker/containerd/component.go b/pkg/component/worker/containerd/component.go
@@ -196,28 +196,39 @@ func (c *Component) setupConfig() error {
 	}
 
 	configurer := &configurer{
-		loadPath:       filepath.Join(c.importsPath, "*.toml"),
-		pauseImage:     c.Profile.PauseImage.URI(),
-		log:            logrus.WithField("component", "containerd"),
-		criRuntimePath: "/run/k0s/containerd-cri.toml",
-	}
-	if runtime.GOOS == "windows" {
-		configurer.criRuntimePath = `C:\var\lib\k0s\run\containerd-cri.toml`
+		loadPath:   filepath.Join(c.importsPath, "*.toml"),
+		pauseImage: c.Profile.PauseImage.URI(),
+		log:        logrus.WithField("component", "containerd"),
 	}
 
-	imports, err := configurer.handleImports()
+	config, err := configurer.handleImports()
 	if err != nil {
 		return fmt.Errorf("can't handle imports: %w", err)
 	}
+
+	criConfigPath := filepath.Join(c.K0sVars.RunDir, "containerd-cri.toml")
+	err = file.WriteContentAtomically(criConfigPath, []byte(config.CRIConfig), 0644)
+	if err != nil {
+		return fmt.Errorf("can't create containerd CRI config: %w", err)
+	}
+
+	var data struct{ Imports []string }
+	data.Imports = append(config.ImportPaths, criConfigPath)
+
+	// double escape for windows because containerd expects
+	// double backslash in the configuration but golang templates
+	// unescape double slash to a single slash
+	if runtime.GOOS == "windows" {
+		for i := range data.Imports {
+			data.Imports[i] = strings.ReplaceAll(data.Imports[i], "\\", "\\\\")
+		}
+	}
+
 	output := bytes.NewBuffer([]byte{})
 	tw := templatewriter.TemplateWriter{
 		Name:     "containerdconfig",
 		Template: confTmpl,
-		Data: struct {
-			Imports []string
-		}{
-			Imports: imports,
-		},
+		Data:     data,
 	}
 	if err := tw.WriteToBuffer(output); err != nil {
 		return fmt.Errorf("can't create containerd config: %w", err)

diff --git a/pkg/component/worker/containerd/configurer.go b/pkg/component/worker/containerd/configurer.go
@@ -21,7 +21,6 @@ import (
 	"os"
 	"path/filepath"
 	"runtime"
-	"strings"
 
 	"github.com/mesosphere/toml-merge/pkg/patch"
 	"github.com/pelletier/go-toml"
@@ -30,31 +29,40 @@ import (
 	criconfig "github.com/containerd/containerd/pkg/cri/config"
 )
 
+// Resolved and merged containerd configuration data.
+type resolvedConfig struct {
+	// Serialized configuration including merged CRI plugin configuration data.
+	CRIConfig string
+
+	// Paths to additional partial configuration files to be imported. Those
+	// files won't contain any CRI plugin configuration data.
+	ImportPaths []string
+}
+
 type configurer struct {
-	loadPath       string
-	pauseImage     string
-	criRuntimePath string
+	loadPath   string
+	pauseImage string
 
 	log *logrus.Entry
 }
 
-// Resolves containerd imports from the import glob path.
-// If the partial config has CRI plugin enabled, it will add to the runc CRI config (single file).
-// if no CRI plugin is found, it will add the file as-is to imports list returned.
-// Once all files are processed the concatenated CRI config file is written and added to the imports list.
-func (c *configurer) handleImports() ([]string, error) {
-	var imports []string
+// Resolves partial containerd configuration files from the import glob path. If
+// a file contains a CRI plugin configuration section, it will be merged into
+// k0s's default configuration, if not, it will be added to the list of import
+// paths.
+func (c *configurer) handleImports() (*resolvedConfig, error) {
+	var importPaths []string
 
 	defaultConfig, err := generateDefaultCRIConfig(c.pauseImage)
 	if err != nil {
 		return nil, fmt.Errorf("failed to generate containerd default CRI config: %w", err)
 	}
 
-	files, err := filepath.Glob(c.loadPath)
+	filePaths, err := filepath.Glob(c.loadPath)
 	if err != nil {
 		return nil, fmt.Errorf("failed to look for containerd import files: %w", err)
 	}
-	c.log.Debugf("found containerd config files to import: %v", files)
+	c.log.Debugf("found containerd config files to import: %v", filePaths)
 
 	// Since the default config contains configuration data for the CRI plugin,
 	// and containerd has decided to replace rather than merge individual plugin
@@ -64,49 +72,32 @@ func (c *configurer) handleImports() ([]string, error) {
 	// do, treat them as merge patches to the default config, if they don't,
 	// just add them as normal imports to be handled by containerd.
 	finalConfig := string(defaultConfig)
-	for _, file := range files {
-		c.log.Debugf("Processing containerd configuration file %s", file)
+	for _, filePath := range filePaths {
+		c.log.Debugf("Processing containerd configuration file %s", filePath)
 
-		data, err := os.ReadFile(file)
+		data, err := os.ReadFile(filePath)
 		if err != nil {
 			return nil, err
 		}
 
 		hasCRI, err := hasCRIPluginConfig(data)
 		if err != nil {
-			return nil, fmt.Errorf("failed to check for CRI plugin configuration in %s: %w", file, err)
+			return nil, fmt.Errorf("failed to check for CRI plugin configuration in %s: %w", filePath, err)
 		}
 
 		if hasCRI {
-			c.log.Infof("Found CRI plugin configuration in %s, treating as merge patch", file)
-			finalConfig, err = patch.TOMLString(finalConfig, patch.FilePatches(file))
+			c.log.Infof("Found CRI plugin configuration in %s, treating as merge patch", filePath)
+			finalConfig, err = patch.TOMLString(finalConfig, patch.FilePatches(filePath))
 			if err != nil {
-				return nil, fmt.Errorf("failed to merge data from %s into containerd configuration: %w", file, err)
+				return nil, fmt.Errorf("failed to merge data from %s into containerd configuration: %w", filePath, err)
 			}
 		} else {
-			c.log.Debugf("No CRI plugin configuration found in %s, adding as-is to imports", file)
-			imports = append(imports, file)
+			c.log.Debugf("No CRI plugin configuration found in %s, adding as-is to imports", filePath)
+			importPaths = append(importPaths, filePath)
 		}
 	}
 
-	// Write the CRI config to a file and add it to imports
-	err = os.WriteFile(c.criRuntimePath, []byte(finalConfig), 0644)
-	if err != nil {
-		return nil, err
-	}
-	imports = append(imports, escapedPath(c.criRuntimePath))
-
-	return imports, nil
-}
-
-func escapedPath(s string) string {
-	// double escape for windows because containerd expects
-	// double backslash in the configuration but golang templates
-	// unescape double slash to a single slash
-	if runtime.GOOS == "windows" {
-		return strings.ReplaceAll(s, "\\", "\\\\")
-	}
-	return s
+	return &resolvedConfig{CRIConfig: finalConfig, ImportPaths: importPaths}, nil
 }
 
 // Returns the default containerd config, including only the CRI plugin

diff --git a/pkg/component/worker/containerd/configurer_test.go b/pkg/component/worker/containerd/configurer_test.go
@@ -29,9 +29,8 @@ import (
 )
 
 func TestConfigurer_HandleImports(t *testing.T) {
-	t.Run("should merge CRI config snippets", func(t *testing.T) {
+	t.Run("should merge configuration files containing CRI plugin configuration sections", func(t *testing.T) {
 		importsPath := t.TempDir()
-		criRuntimePath := filepath.Join(t.TempDir(), "cri.toml")
 		criRuntimeConfig := `
 [plugins]
   [plugins."io.containerd.grpc.v1.cri".containerd]
@@ -40,46 +39,42 @@ func TestConfigurer_HandleImports(t *testing.T) {
 		err := os.WriteFile(filepath.Join(importsPath, "foo.toml"), []byte(criRuntimeConfig), 0644)
 		require.NoError(t, err)
 		c := configurer{
-			loadPath:       filepath.Join(importsPath, "*.toml"),
-			criRuntimePath: criRuntimePath,
-			log:            logrus.New().WithField("test", t.Name()),
+			loadPath: filepath.Join(importsPath, "*.toml"),
+			log:      logrus.New().WithField("test", t.Name()),
 		}
-		imports, err := c.handleImports()
-		require.NoError(t, err)
-		require.Len(t, imports, 1)
-		require.Contains(t, imports, escapedPath(criRuntimePath))
+		criConfig, err := c.handleImports()
+		assert.NoError(t, err)
+		require.NotNil(t, criConfig)
+		assert.Empty(t, criConfig.ImportPaths, "files containing CRI plugin configuration sections should be merged, not imported")
 
 		// Dump the config for inspection
-		b, _ := os.ReadFile(criRuntimePath)
-		t.Logf("CRI config:\n%s", string(b))
+		t.Logf("CRI config:\n%s", criConfig.CRIConfig)
+
+		criConfigPath := filepath.Join(t.TempDir(), "cri.toml")
+		require.NoError(t, os.WriteFile(criConfigPath, []byte(criConfig.CRIConfig), 0644))
 
 		// Load the criRuntimeConfig and verify the settings are correct
-		containerdConfig := &serverconfig.Config{}
-		err = serverconfig.LoadConfig(criRuntimePath, containerdConfig)
-		require.NoError(t, err)
+		var containerdConfig serverconfig.Config
+		require.NoError(t, serverconfig.LoadConfig(criConfigPath, &containerdConfig))
 
-		criConfig := containerdConfig.Plugins["io.containerd.grpc.v1.cri"]
-		snapshotter := criConfig.GetPath([]string{"containerd", "snapshotter"})
+		criPluginConfig := containerdConfig.Plugins["io.containerd.grpc.v1.cri"]
+		require.NotNil(t, criPluginConfig, "No CRI plugin configuration section found")
+		snapshotter := criPluginConfig.GetPath([]string{"containerd", "snapshotter"})
 		require.Equal(t, "zfs", snapshotter)
 	})
 
-	t.Run("should have single import for CRI if there's nothing in imports dir", func(t *testing.T) {
-		criRuntimePath := filepath.Join(t.TempDir(), "cri.toml")
+	t.Run("should have no imports if imports dir is empty", func(t *testing.T) {
 		c := configurer{
-			loadPath:       filepath.Join(t.TempDir(), "*.toml"),
-			criRuntimePath: criRuntimePath,
-			log:            logrus.New().WithField("test", t.Name()),
+			loadPath: filepath.Join(t.TempDir(), "*.toml"),
+			log:      logrus.New().WithField("test", t.Name()),
 		}
-		imports, err := c.handleImports()
+		criConfig, err := c.handleImports()
 		assert.NoError(t, err)
-		if assert.Len(t, imports, 1) {
-			assert.Equal(t, escapedPath(criRuntimePath), imports[0])
-		}
+		assert.Empty(t, criConfig.ImportPaths)
 	})
 
-	t.Run("should have two imports when one non CRI snippet", func(t *testing.T) {
+	t.Run("should import configuration files not containing a CRI plugin configuration section", func(t *testing.T) {
 		importsPath := t.TempDir()
-		criRuntimePath := filepath.Join(t.TempDir(), "cri.toml")
 		criRuntimeConfig := `
 foo = "bar"
 version = 2
@@ -88,15 +83,11 @@ version = 2
 		err := os.WriteFile(nonCriConfigPath, []byte(criRuntimeConfig), 0644)
 		require.NoError(t, err)
 		c := configurer{
-			loadPath:       filepath.Join(importsPath, "*.toml"),
-			criRuntimePath: criRuntimePath,
-			log:            logrus.New().WithField("test", t.Name()),
+			loadPath: filepath.Join(importsPath, "*.toml"),
+			log:      logrus.New().WithField("test", t.Name()),
 		}
-		imports, err := c.handleImports()
+		criConfig, err := c.handleImports()
 		assert.NoError(t, err)
-		if assert.Len(t, imports, 2) {
-			assert.Contains(t, imports, escapedPath(criRuntimePath))
-			assert.Contains(t, imports, escapedPath(nonCriConfigPath))
-		}
+		assert.Equal(t, []string{nonCriConfigPath}, criConfig.ImportPaths)
 	})
 }