feat: add possibility to customize containerd with additional files

[alegrey91]

Description

Based on the feature request opened here: #2550 the PR add the possibility to customize containerd without editing the original /etc/k0s/containerd.toml file. This is possible due to containerd's feature that allows importing additional configurations using the import keyword.

Fixes #2550

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update

How Has This Been Tested?

• Manual test
• Auto test added

Checklist:

• My code follows the style guidelines of this project
• My commit messages are signed-off
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
  (since this is a containerd feature, I don't think tests are needed)
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules
• I have checked my code and corrected any misspellings

[juanluisvaladas]

First of all, thanks for the PR. We appreciate contributions a lot.

This looks good to me but before approving @jnummelin should check this because he was checking something related to imports last week. Although I don´t think this is incompatible.

[alegrey91]

Hi @jnummelin, did you have chance to check the PR?

[jnummelin]

From k0s point of view this looks ok. But there's some weirdness, for example:

bash-5.1# cat /etc/containerd/config.toml 

version = 2
imports = [
        "/etc/containerd_imports/*.toml"
]

As a drop-in config snippet I have:

bash-5.1# cat /etc/containerd_imports/foo.toml 
[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.foo]
    runtime_type = "io.containerd.runc.v2"
    [plugins."io.containerd.grpc.v1.cri".containerd.runtimes.foo.options]
        BinaryName = "runfoo"

But the drop-in config is not really fully merged as expected:

bash-5.1# containerd config dump > combined.toml
bash-5.1# containerd config default > default.toml
bash-5.1# diff default.toml combined.toml
--- default.toml
+++ combined.toml
@@ -1,5 +1,5 @@
 disabled_plugins = []
-imports = []
+imports = ["/etc/containerd/config.toml", "/etc/containerd_imports/foo.toml"]
 oom_score = 0
 plugin_dir = ""
 required_plugins = []
@@ -99,7 +99,7 @@
 
       [plugins."io.containerd.grpc.v1.cri".containerd.runtimes]
 
-        [plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc]
+        [plugins."io.containerd.grpc.v1.cri".containerd.runtimes.foo]
           base_runtime_spec = ""
           cni_conf_dir = ""
           cni_max_conf_num = 0
@@ -111,18 +111,8 @@
           runtime_root = ""
           runtime_type = "io.containerd.runc.v2"
 
-          [plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc.options]
-            BinaryName = ""
-            CriuImagePath = ""
-            CriuPath = ""
-            CriuWorkPath = ""
-            IoGid = 0
-            IoUid = 0
-            NoNewKeyring = false
-            NoPivotRoot = false
-            Root = ""
-            ShimCgroup = ""
-            SystemdCgroup = false
+          [plugins."io.containerd.grpc.v1.cri".containerd.runtimes.foo.options]
+            BinaryName = "runfoo"
 
       [plugins."io.containerd.grpc.v1.cri".containerd.untrusted_workload_runtime]
         base_runtime_spec = ""

I've tried to reach out to the containerd devs at Slack but to no avail.

[juanluisvaladas]

This is about how containerd merges the configuration and how it replaces *toml.Tree rather than merging it.
I've spent some time on fixing it but debugging the problems is being complicated... Anyway I think it wouldn't be unreasonable to merge this despite of the problems merging the configs...

[jnummelin]

I'm slightly hesitant to merge this untill there's some resolution on upstream on the config import handling. What I'm worried is that as we know the imports behave bit oddly in some cases (the main use case why we'd want them) and that might lead to lot of struggle on end users.

[alegrey91]

Ok, I got your point. I took a glance to containerd's codebase.
I think the "problem" could be there: https://github.com/containerd/containerd/blob/main/cmd/containerd/command/config.go#L57
They are initializing the Plugins struct in this way: config.Plugins = make(map[string]interface{}). So they are assigning zero values to Plugins types. Isn't it?

[juanluisvaladas]

@alegrey91 No that's not the issue. The issue happens here:
https://github.com/containerd/containerd/blob/main/services/server/config/config.go#L298

Essentially the problem is that config.Plugins is a map[string]toml.Tree

If you have a file with the contents configuration:

      [plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc]
	runtime_type = "io.containerd.runc.v2"
	sandbox_mode = "podsandbox"
	[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc.options]
	  BinaryName = "/var/lib/k0s/bin/runc"

And another file with:

  [plugins."io.containerd.grpc.v1.cri".containerd.runtimes.foo]
	runtime_type = "io.containerd.runc.v2"
	[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.foo.options]
		BinaryName = "/var/lib/k0s/bin/runfoo"

In both config.Plugins the key of the map will be "io.containerd.grpc.v1.cri", which means it will get replaced and only the newer one will work.

I have a patch in progress but I'm struggling to make it work I haven't worked in it for a couple days because I was stuck and decided to leave it for a while, but I'm going to continue today: containerd/containerd@6e445f2

[alegrey91]

Great! Thanks for your explanation @juanluisvaladas

[github-actions]

The PR is marked as stale since no activity has been recorded in 30 days

[juanluisvaladas]

Related:
containerd/containerd#5837, containerd/containerd#7347, containerd/containerd#7784 and containerd/containerd#8056

[github-actions]

The PR is marked as stale since no activity has been recorded in 30 days

[juanluisvaladas]

@jnummelin I think since we're about to merge #2637 we can close this one. WDYT?

[mikhail-sakhnov]

I think it is obsolete with the #2637 merged, closing the current PR. Feel free to re-open in case if I am wrong.