rework containerd config merging (#1780)

CHANGELOG.md

@@ -14338,4 +14338,4 @@ Note: CNI >= 1.2.1 is required for t3 and r5 instance support.
 
 * EKS Launch AMI
 
-<!-- git log --pretty=format:"* %h %s" $(git describe --abbrev=0 --tags)..HEAD -->
+<!-- git log --pretty=format:"* %h %s" $(git describe --abbrev=0 --tags)..HEAD -->

nodeadm/go.mod

@@ -12,7 +12,8 @@ require (
 	github.com/containerd/containerd v1.7.13
 	github.com/coreos/go-systemd/v22 v22.5.0
 	github.com/integrii/flaggy v1.5.2
-	github.com/stretchr/testify v1.8.4
+	github.com/pelletier/go-toml/v2 v2.2.2
+	github.com/stretchr/testify v1.9.0
 	go.uber.org/zap v1.26.0
 	golang.org/x/mod v0.14.0
 	k8s.io/apimachinery v0.29.1
@@ -24,7 +25,7 @@ require (
 require (
 	github.com/Microsoft/go-winio v0.6.1 // indirect
 	github.com/golang/protobuf v1.5.3 // indirect
-	github.com/stretchr/objx v0.5.0 // indirect
+	github.com/stretchr/objx v0.5.2 // indirect
 	golang.org/x/tools v0.16.1 // indirect
 	google.golang.org/genproto/googleapis/rpc v0.0.0-20230822172742-b8732ec3820d // indirect
 	google.golang.org/grpc v1.58.3 // indirect

nodeadm/go.sum

@@ -98,6 +98,8 @@ github.com/onsi/ginkgo/v2 v2.14.0 h1:vSmGj2Z5YPb9JwCWT6z6ihcUvDhuXLc3sJiqd3jMKAY
 github.com/onsi/ginkgo/v2 v2.14.0/go.mod h1:JkUdW7JkN0V6rFvsHcJ478egV3XH9NxpD27Hal/PhZw=
 github.com/onsi/gomega v1.30.0 h1:hvMK7xYz4D3HapigLTeGdId/NcfQx1VHMJc60ew99+8=
 github.com/onsi/gomega v1.30.0/go.mod h1:9sxs+SwGrKI0+PWe4Fxa9tFQQBG5xSsSbMXOI8PPpoQ=
+github.com/pelletier/go-toml/v2 v2.2.2 h1:aYUidT7k73Pcl9nb2gScu7NSrKCSHIDE89b3+6Wq+LM=
+github.com/pelletier/go-toml/v2 v2.2.2/go.mod h1:1t835xjRzz80PqgE6HHgN2JOsmgYu/h4qDAS4n929Rs=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/prometheus/client_golang v1.18.0 h1:HzFfmkOzH5Q8L8G+kSJKUx5dtG87sewO+FoDDqP5Tbk=
@@ -117,13 +119,15 @@ github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA=
 github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=
-github.com/stretchr/objx v0.5.0 h1:1zr/of2m5FGMsad5YfcqgdqdWrIhu+EBEJRhR1U7z/c=
 github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo=
+github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY=
+github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA=
 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
 github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
 github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
-github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
 github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
+github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg=
+github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
 github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto=

nodeadm/internal/api/merge.go

@@ -6,30 +6,40 @@ import (
 
 	"dario.cat/mergo"
 	"github.com/awslabs/amazon-eks-ami/nodeadm/internal/util"
+	"github.com/pelletier/go-toml/v2"
 	"k8s.io/apimachinery/pkg/runtime"
 )
 
 // Merges two NodeConfigs with custom collision handling
 func (dst *NodeConfig) Merge(src *NodeConfig) error {
-	return mergo.Merge(dst, src, mergo.WithOverride, mergo.WithTransformers(kubeletTransformer{}))
+	return mergo.Merge(dst, src, mergo.WithOverride, mergo.WithTransformers(nodeConfigTransformer{}))
 }
 
 const (
 	kubeletFlagsName  = "Flags"
 	kubeletConfigName = "Config"
+
+	containerdConfigName = "Config"
 )
 
-type kubeletTransformer struct{}
+type nodeConfigTransformer struct{}
 
-func (k kubeletTransformer) Transformer(typ reflect.Type) func(dst, src reflect.Value) error {
-	if typ == reflect.TypeOf(KubeletOptions{}) {
+func (t nodeConfigTransformer) Transformer(typ reflect.Type) func(dst, src reflect.Value) error {
+	if typ == reflect.TypeOf(ContainerdOptions{}) {
 		return func(dst, src reflect.Value) error {
-			k.transformFlags(
+			return t.transformContainerdConfig(
+				dst.FieldByName(containerdConfigName),
+				src.FieldByName(containerdConfigName),
+			)
+		}
+	} else if typ == reflect.TypeOf(KubeletOptions{}) {
+		return func(dst, src reflect.Value) error {
+			t.transformKubeletFlags(
 				dst.FieldByName(kubeletFlagsName),
 				src.FieldByName(kubeletFlagsName),
 			)
 
-			if err := k.transformConfig(
+			if err := t.transformKubeletConfig(
 				dst.FieldByName(kubeletConfigName),
 				src.FieldByName(kubeletConfigName),
 			); err != nil {
@@ -42,7 +52,7 @@ func (k kubeletTransformer) Transformer(typ reflect.Type) func(dst, src reflect.
 	return nil
 }
 
-func (k kubeletTransformer) transformFlags(dst, src reflect.Value) {
+func (t nodeConfigTransformer) transformKubeletFlags(dst, src reflect.Value) {
 	if dst.CanSet() {
 		// kubelet flags are parsed using https://github.com/spf13/pflag, where
 		// flag order determines precedence. For single-value flags this is
@@ -56,15 +66,15 @@ func (k kubeletTransformer) transformFlags(dst, src reflect.Value) {
 	}
 }
 
-func (k kubeletTransformer) transformConfig(dst, src reflect.Value) error {
+func (t nodeConfigTransformer) transformKubeletConfig(dst, src reflect.Value) error {
 	if dst.CanSet() {
 		if dst.Len() <= 0 {
 			// if the destination is empty just use the source data
 			dst.Set(src)
 		} else if src.Len() > 0 {
 			// kubelet config in an inline document here, so we explicitly
 			// perform a merge with dst and src data.
-			mergedMap, err := util.DocumentMerge(dst.Interface(), src.Interface(), mergo.WithOverride)
+			mergedMap, err := util.Merge(dst.Interface(), src.Interface(), json.Marshal, json.Unmarshal)
 			if err != nil {
 				return err
 			}
@@ -78,6 +88,30 @@ func (k kubeletTransformer) transformConfig(dst, src reflect.Value) error {
 	return nil
 }
 
+func (t nodeConfigTransformer) transformContainerdConfig(dst, src reflect.Value) error {
+	if dst.CanSet() {
+		if dst.Len() <= 0 {
+			// if the destination is empty just use the source data
+			dst.Set(src)
+		} else if src.Len() > 0 {
+			// containerd config is a string an inline string here, so we
+			// explicitly perform a merge with dst and src data.
+			dstConfig := []byte(dst.String())
+			srcConfig := []byte(src.String())
+			configBytes, err := util.Merge(dstConfig, srcConfig, toml.Marshal, toml.Unmarshal)
+			if err != nil {
+				return err
+			}
+			config, err := toml.Marshal(configBytes)
+			if err != nil {
+				return err
+			}
+			dst.SetString(string(config))
+		}
+	}
+	return nil
+}
+
 func toInlineDocument(m map[string]interface{}) (InlineDocument, error) {
 	var rawMap = make(InlineDocument)
 	for key, value := range m {

nodeadm/internal/api/merge_test.go

@@ -3,6 +3,8 @@ package api
 import (
 	"reflect"
 	"testing"
+
+	"github.com/pelletier/go-toml/v2"
 )
 
 func toInlineDocumentMust(m map[string]interface{}) InlineDocument {
@@ -13,6 +15,20 @@ func toInlineDocumentMust(m map[string]interface{}) InlineDocument {
 	return d
 }
 
+// pass the toml through serialization and deserialization to get a normalized
+// payload for tests that has deterministic ordering and formatting
+func tomlNormalize(t string) string {
+	var m map[string]interface{}
+	if err := toml.Unmarshal([]byte(t), &m); err != nil {
+		panic(err)
+	}
+	s, err := toml.Marshal(m)
+	if err != nil {
+		panic(err)
+	}
+	return string(s)
+}
+
 func TestMerge(t *testing.T) {
 	var tests = []struct {
 		name         string
@@ -44,6 +60,26 @@ func TestMerge(t *testing.T) {
 				Cluster: ClusterDetails{Name: "next"},
 			},
 		},
+		{
+			name: "merge with deeply nested toml object",
+			baseSpec: NodeConfigSpec{
+				Containerd: ContainerdOptions{
+					Config: "[a.b.c.d]\nf = 0",
+				},
+			},
+			patchSpec: NodeConfigSpec{
+				Containerd: ContainerdOptions{
+					Config: "[a.b.c.d]\ne = 0",
+				},
+			},
+			// This test is primarily for clarity on what happens during the
+			// expansion of nested toml objects.
+			expectedSpec: NodeConfigSpec{
+				Containerd: ContainerdOptions{
+					Config: "[a]\n[a.b]\n[a.b.c]\n[a.b.c.d]\ne = 0\nf = 0\n",
+				},
+			},
+		},
 		{
 			name: "customer overrides orchestrator defaults",
 			baseSpec: NodeConfigSpec{
@@ -66,7 +102,33 @@ func TestMerge(t *testing.T) {
 					},
 				},
 				Containerd: ContainerdOptions{
-					Config: "base",
+					Config: tomlNormalize(`
+version = 2
+root = "/var/lib/containerd"
+state = "/run/containerd"
+
+[grpc]
+address = "/run/containerd/containerd.sock"
+
+[plugins."io.containerd.grpc.v1.cri".containerd]
+default_runtime_name = "runc"
+discard_unpacked_layers = true
+
+[plugins."io.containerd.grpc.v1.cri"]
+sandbox_image = "{{.SandboxImage}}"
+
+[plugins."io.containerd.grpc.v1.cri".registry]
+config_path = "/etc/containerd/certs.d:/etc/docker/certs.d"
+
+[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc]
+runtime_type = "io.containerd.runc.v2"
+
+[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc.options]
+SystemdCgroup = true
+
+[plugins."io.containerd.grpc.v1.cri".cni]
+bin_dir = "/opt/cni/bin"
+conf_dir = "/etc/cni/net.d"`),
 				},
 			},
 			patchSpec: NodeConfigSpec{
@@ -82,7 +144,13 @@ func TestMerge(t *testing.T) {
 					},
 				},
 				Containerd: ContainerdOptions{
-					Config: "patch",
+					Config: tomlNormalize(`
+version = 2
+[grpc]
+address = "/run/containerd/containerd.sock.2"
+
+[plugins."io.containerd.grpc.v1.cri".containerd]
+discard_unpacked_layers = false`),
 				},
 			},
 			expectedSpec: NodeConfigSpec{
@@ -107,7 +175,33 @@ func TestMerge(t *testing.T) {
 					},
 				},
 				Containerd: ContainerdOptions{
-					Config: "patch",
+					Config: tomlNormalize(`
+version = 2
+root = "/var/lib/containerd"
+state = "/run/containerd"
+
+[grpc]
+address = "/run/containerd/containerd.sock.2"
+
+[plugins."io.containerd.grpc.v1.cri".containerd]
+default_runtime_name = "runc"
+discard_unpacked_layers = false
+
+[plugins."io.containerd.grpc.v1.cri"]
+sandbox_image = "{{.SandboxImage}}"
+
+[plugins."io.containerd.grpc.v1.cri".registry]
+config_path = "/etc/containerd/certs.d:/etc/docker/certs.d"
+
+[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc]
+runtime_type = "io.containerd.runc.v2"
+
+[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc.options]
+SystemdCgroup = true
+
+[plugins."io.containerd.grpc.v1.cri".cni]
+bin_dir = "/opt/cni/bin"
+conf_dir = "/etc/cni/net.d"`),
 				},
 			},
 		},

nodeadm/internal/containerd/config.go

@@ -3,20 +3,19 @@ package containerd
 import (
 	"bytes"
 	_ "embed"
-	"path/filepath"
 	"text/template"
 
 	"github.com/awslabs/amazon-eks-ami/nodeadm/internal/api"
 	"github.com/awslabs/amazon-eks-ami/nodeadm/internal/util"
+	"github.com/pelletier/go-toml/v2"
 	"go.uber.org/zap"
 )
 
 const ContainerRuntimeEndpoint = "unix:///run/containerd/containerd.sock"
 
 const (
-	containerdConfigFile      = "/etc/containerd/config.toml"
-	containerdConfigImportDir = "/etc/containerd/config.d"
-	containerdConfigPerm      = 0644
+	containerdConfigFile = "/etc/containerd/config.toml"
+	containerdConfigPerm = 0644
 )
 
 var (
@@ -35,16 +34,22 @@ func writeContainerdConfig(cfg *api.NodeConfig) error {
 	if err != nil {
 		return err
 	}
-	zap.L().Info("Writing containerd config to file..", zap.String("path", containerdConfigFile))
-	if err := util.WriteFileWithDir(containerdConfigFile, containerdConfig, containerdConfigPerm); err != nil {
-		return err
-	}
+	// because the logic in containerd's import merge decides to completely
+	// overwrite entire sections, we want to implement this merging ourselves.
+	// see: https://github.com/containerd/containerd/blob/a91b05d99ceac46329be06eb43f7ae10b89aad45/cmd/containerd/server/config/config.go#L407-L431
 	if len(cfg.Spec.Containerd.Config) > 0 {
-		containerConfigImportPath := filepath.Join(containerdConfigImportDir, "00-nodeadm.toml")
-		zap.L().Info("Writing user containerd config to drop-in file..", zap.String("path", containerConfigImportPath))
-		return util.WriteFileWithDir(containerConfigImportPath, []byte(cfg.Spec.Containerd.Config), containerdConfigPerm)
+		containerdConfigMap, err := util.Merge(containerdConfig, []byte(cfg.Spec.Containerd.Config), toml.Marshal, toml.Unmarshal)
+		if err != nil {
+			return err
+		}
+		containerdConfig, err = toml.Marshal(containerdConfigMap)
+		if err != nil {
+			return err
+		}
 	}
-	return nil
+
+	zap.L().Info("Writing containerd config to file..", zap.String("path", containerdConfigFile))
+	return util.WriteFileWithDir(containerdConfigFile, containerdConfig, containerdConfigPerm)
 }
 
 func generateContainerdConfig(cfg *api.NodeConfig) ([]byte, error) {

nodeadm/internal/containerd/config.template.toml

@@ -1,10 +1,6 @@
 version = 2
 root = "/var/lib/containerd"
 state = "/run/containerd"
-# Users can use the following import directory to add additional
-# configuration to containerd. The imports do not behave exactly like overrides.
-# see: https://github.com/containerd/containerd/blob/main/docs/man/containerd-config.toml.5.md#format
-imports = ["/etc/containerd/config.d/*.toml"]
 
 [grpc]
 address = "/run/containerd/containerd.sock"