Repository: spegel-org/spegel (go)

Issue:
Containerd hosts config are sorted and not in the specified order
### Spegel version

v0.0.23

### Kubernetes distribution

na

### Kubernetes version

na

### CNI

na

### Describe the bug

The order of hosts in the hosts.toml files in containerd config generated by the spegel configuration command do not preserve the order of previous config nor the order in arguments.
This is due to the fact that maps marshalled with the go-toml library are sorted by key.
The issue is that the order of the "hosts" table is important in containerd as hosts are tried in order of apparition in the file.

All current tests pass because it seems they take this fact into account.
You can try to switch the order of mirrors in input of a test without changing the expected output. The test will still pass while it shouldn't.


Pull request:
Fix Containerd host mirror ordering
This change replaces the use of toml un-marshalling with Go templating which was previously used a year ago. This is to make sure the the order the mirrors passed to Spegel are preserved in the toml file.

This change is heavily based on @vflaux work in both #550 and #557. Thank you for doing the research to figure out a path forward in solving this! 

Fixes #549 

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index db60be4..d38f0fc 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -19,6 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- [#601](https://github.com/spegel-org/spegel/pull/601) Fix Containerd host mirror ordering.
+
 ### Security
 
 ## v0.0.25
diff --git a/go.mod b/go.mod
index 5b40aea..b572158 100644
--- a/go.mod
+++ b/go.mod
@@ -25,7 +25,6 @@ require (
 	github.com/stretchr/testify v1.9.0
 	go.etcd.io/bbolt v1.3.11
 	golang.org/x/sync v0.8.0
-	golang.org/x/time v0.7.0
 	k8s.io/client-go v0.31.1
 	k8s.io/cri-api v0.31.1
 	k8s.io/klog/v2 v2.130.1
@@ -199,6 +198,7 @@ require (
 	golang.org/x/sys v0.22.0 // indirect
 	golang.org/x/term v0.22.0 // indirect
 	golang.org/x/text v0.16.0 // indirect
+	golang.org/x/time v0.7.0 // indirect
 	golang.org/x/tools v0.23.0 // indirect
 	gonum.org/v1/gonum v0.15.0 // indirect
 	google.golang.org/genproto v0.0.0-20231211222908-989df2bf70f3 // indirect
diff --git a/pkg/oci/containerd.go b/pkg/oci/containerd.go
index 76885fe..c0aff67 100644
--- a/pkg/oci/containerd.go
+++ b/pkg/oci/containerd.go
@@ -1,6 +1,7 @@
 package oci
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
@@ -11,6 +12,7 @@ import (
 	"path"
 	"path/filepath"
 	"strings"
+	"text/template"
 
 	semver "github.com/Masterminds/semver/v3"
 	"github.com/containerd/containerd"
@@ -22,6 +24,7 @@ import (
 	"github.com/opencontainers/go-digest"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/pelletier/go-toml/v2"
+	tomlu "github.com/pelletier/go-toml/v2/unstable"
 	"github.com/spf13/afero"
 	runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1"
 
@@ -330,20 +333,6 @@ func createFilters(registries []url.URL) (string, string) {
 	return listFilter, eventFilter
 }
 
-type hostFile struct {
-	HostConfigs map[string]hostConfig `toml:"host"`
-	Server      string                `toml:"server"`
-}
-
-type hostConfig struct {
-	CACert       interface{}            `toml:"ca"`
-	Client       interface{}            `toml:"client"`
-	OverridePath *bool                  `toml:"override_path"`
-	SkipVerify   *bool                  `toml:"skip_verify"`
-	Header       map[string]interface{} `toml:"header"`
-	Capabilities []string               `toml:"capabilities"`
-}
-
 // Refer to containerd registry configuration documentation for more information about required configuration.
 // https://github.com/containerd/containerd/blob/main/docs/cri/config.md#registry-configuration
 // https://github.com/containerd/containerd/blob/main/docs/hosts.md#registry-configuration---examples
@@ -372,14 +361,11 @@ func AddMirrorConfiguration(ctx context.Context, fs afero.Fs, configPath string,
 		capabilities = append(capabilities, "resolve")
 	}
 	for _, registryURL := range registryURLs {
-		hf, appending, err := getHostFile(fs, configPath, appendToBackup, registryURL)
+		existingHosts, err := existingHosts(fs, configPath, registryURL)
 		if err != nil {
 			return err
 		}
-		for _, u := range mirrorURLs {
-			hf.HostConfigs[u.String()] = hostConfig{Capabilities: capabilities}
-		}
-		b, err := toml.Marshal(&hf)
+		templatedHosts, err := templateHosts(registryURL, mirrorURLs, capabilities, existingHosts)
 		if err != nil {
 			return err
 		}
@@ -388,11 +374,11 @@ func AddMirrorConfiguration(ctx context.Context, fs afero.Fs, configPath string,
 		if err != nil {
 			return err
 		}
-		err = afero.WriteFile(fs, fp, b, 0o644)
+		err = afero.WriteFile(fs, fp, []byte(templatedHosts), 0o644)
 		if err != nil {
 			return err
 		}
-		if appending {
+		if existingHosts != "" {
 			log.Info("appending to existing Containerd mirror configuration", "registry", registryURL.String(), "path", fp)
 		} else {
 			log.Info("added Containerd mirror configuration", "registry", registryURL.String(), "path", fp)
@@ -470,29 +456,96 @@ func clearConfig(fs afero.Fs, configPath string) error {
 	return nil
 }
 
-func getHostFile(fs afero.Fs, configPath string, appendToBackup bool, registryURL url.URL) (hostFile, bool, error) {
-	if appendToBackup {
-		fp := path.Join(configPath, backupDir, registryURL.Host, "hosts.toml")
-		b, err := afero.ReadFile(fs, fp)
-		if err != nil && !errors.Is(err, afero.ErrFileNotFound) {
-			return hostFile{}, false, err
-		}
-		if err == nil {
-			hf := hostFile{}
-			err := toml.Unmarshal(b, &hf)
-			if err != nil {
-				return hostFile{}, false, err
-			}
-			return hf, true, nil
-		}
-	}
+func templateHosts(registryURL url.URL, mirrorURLs []url.URL, capabilities []string, existingHosts string) (string, error) {
 	server := registryURL.String()
 	if registryURL.String() == "https://docker.io" {
 		server = "https://registry-1.docker.io"
 	}
-	hf := hostFile{
-		Server:      server,
-		HostConfigs: map[string]hostConfig{},
+	capabilitiesStr := strings.Join(capabilities, "', '")
+	capabilitiesStr = fmt.Sprintf("['%s']", capabilitiesStr)
+	hc := struct {
+		Server       string
+		Capabilities string
+		MirrorURLs   []url.URL
+	}{
+		Server:       server,
+		Capabilities: capabilitiesStr,
+		MirrorURLs:   mirrorURLs,
+	}
+	tmpl, err := template.New("").Parse(`server = '{{ .Server }}'
+{{ range .MirrorURLs }}
+[host.'{{ .String }}']
+capabilities = {{ $.Capabilities }}
+{{ end }}`)
+	if err != nil {
+		return "", err
+	}
+	buf := bytes.NewBuffer(nil)
+	err = tmpl.Execute(buf, hc)
+	if err != nil {
+		return "", err
+	}
+	output := strings.TrimSpace(buf.String())
+	if existingHosts != "" {
+		output = output + "\n\n" + existingHosts
+	}
+	return output, nil
+}
+
+type hostFile struct {
+	Hosts map[string]interface{} `toml:"host"`
+}
+
+func existingHosts(fs afero.Fs, configPath string, registryURL url.URL) (string, error) {
+	fp := path.Join(configPath, backupDir, registryURL.Host, "hosts.toml")
+	b, err := afero.ReadFile(fs, fp)
+	if errors.Is(err, afero.ErrFileNotFound) {
+		return "", nil
+	}
+	if err != nil {
+		return "", err
+	}
+
+	var hf hostFile
+	err = toml.Unmarshal(b, &hf)
+	if err != nil {
+		return "", err
+	}
+	if len(hf.Hosts) == 0 {
+		return "", nil
+	}
+
+	hosts := []string{}
+	parser := tomlu.Parser{}
+	parser.Reset(b)
+	for parser.NextExpression() {
+		err := parser.Error()
+		if err != nil {
+			return "", err
+		}
+		e := parser.Expression()
+		if e.Kind != tomlu.Table {
+			continue
+		}
+		ki := e.Key()
+		if ki.Next() && string(ki.Node().Data) == "host" && ki.Next() && ki.IsLast() {
+			hosts = append(hosts, string(ki.Node().Data))
+		}
+	}
+
+	ehs := []string{}
+	for _, h := range hosts {
+		data := hostFile{
+			Hosts: map[string]interface{}{
+				h: hf.Hosts[h],
+			},
+		}
+		b, err := toml.Marshal(data)
+		if err != nil {
+			return "", err
+		}
+		eh := strings.TrimPrefix(string(b), "[host]\n")
+		ehs = append(ehs, eh)
 	}
-	return hf, false, nil
+	return strings.TrimSpace(strings.Join(ehs, "\n")), nil
 }


Test patch:
diff --git a/pkg/oci/containerd_test.go b/pkg/oci/containerd_test.go
index e1b6097..bff1fcd 100644
--- a/pkg/oci/containerd_test.go
+++ b/pkg/oci/containerd_test.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	iofs "io/fs"
 	"net/url"
+	"path/filepath"
 	"testing"
 
 	eventtypes "github.com/containerd/containerd/api/events"
@@ -211,17 +212,18 @@ func TestMirrorConfiguration(t *testing.T) {
 			name:        "multiple mirros",
 			resolveTags: true,
 			registries:  stringListToUrlList(t, []string{"http://foo.bar:5000"}),
-			mirrors:     stringListToUrlList(t, []string{"http://127.0.0.1:5000", "http://127.0.0.1:5001"}),
+			mirrors:     stringListToUrlList(t, []string{"http://127.0.0.1:5000", "http://127.0.0.2:5000", "http://127.0.0.1:5001"}),
 			expectedFiles: map[string]string{
 				"/etc/containerd/certs.d/foo.bar:5000/hosts.toml": `server = 'http://foo.bar:5000'
 
-[host]
 [host.'http://127.0.0.1:5000']
 capabilities = ['pull', 'resolve']
 
-[host.'http://127.0.0.1:5001']
+[host.'http://127.0.0.2:5000']
 capabilities = ['pull', 'resolve']
-`,
+
+[host.'http://127.0.0.1:5001']
+capabilities = ['pull', 'resolve']`,
 			},
 		},
 		{
@@ -232,16 +234,12 @@ capabilities = ['pull', 'resolve']
 			expectedFiles: map[string]string{
 				"/etc/containerd/certs.d/docker.io/hosts.toml": `server = 'https://registry-1.docker.io'
 
-[host]
 [host.'http://127.0.0.1:5000']
-capabilities = ['pull']
-`,
+capabilities = ['pull']`,
 				"/etc/containerd/certs.d/foo.bar:5000/hosts.toml": `server = 'http://foo.bar:5000'
 
-[host]
 [host.'http://127.0.0.1:5000']
-capabilities = ['pull']
-`,
+capabilities = ['pull']`,
 			},
 		},
 		{
@@ -253,16 +251,12 @@ capabilities = ['pull']
 			expectedFiles: map[string]string{
 				"/etc/containerd/certs.d/docker.io/hosts.toml": `server = 'https://registry-1.docker.io'
 
-[host]
 [host.'http://127.0.0.1:5000']
-capabilities = ['pull', 'resolve']
-`,
+capabilities = ['pull', 'resolve']`,
 				"/etc/containerd/certs.d/foo.bar:5000/hosts.toml": `server = 'http://foo.bar:5000'
 
-[host]
 [host.'http://127.0.0.1:5000']
-capabilities = ['pull', 'resolve']
-`,
+capabilities = ['pull', 'resolve']`,
 			},
 		},
 		{
@@ -274,16 +268,12 @@ capabilities = ['pull', 'resolve']
 			expectedFiles: map[string]string{
 				"/etc/containerd/certs.d/docker.io/hosts.toml": `server = 'https://registry-1.docker.io'
 
-[host]
 [host.'http://127.0.0.1:5000']
-capabilities = ['pull', 'resolve']
-`,
+capabilities = ['pull', 'resolve']`,
 				"/etc/containerd/certs.d/foo.bar:5000/hosts.toml": `server = 'http://foo.bar:5000'
 
-[host]
 [host.'http://127.0.0.1:5000']
-capabilities = ['pull', 'resolve']
-`,
+capabilities = ['pull', 'resolve']`,
 			},
 		},
 		{
@@ -293,24 +283,20 @@ capabilities = ['pull', 'resolve']
 			mirrors:             stringListToUrlList(t, []string{"http://127.0.0.1:5000"}),
 			createConfigPathDir: true,
 			existingFiles: map[string]string{
-				"/etc/containerd/certs.d/docker.io/hosts.toml": "Hello World",
-				"/etc/containerd/certs.d/ghcr.io/hosts.toml":   "Foo Bar",
+				"/etc/containerd/certs.d/docker.io/hosts.toml": "hello = 'world'",
+				"/etc/containerd/certs.d/ghcr.io/hosts.toml":   "foo = 'bar'",
 			},
 			expectedFiles: map[string]string{
-				"/etc/containerd/certs.d/_backup/docker.io/hosts.toml": "Hello World",
-				"/etc/containerd/certs.d/_backup/ghcr.io/hosts.toml":   "Foo Bar",
+				"/etc/containerd/certs.d/_backup/docker.io/hosts.toml": "hello = 'world'",
+				"/etc/containerd/certs.d/_backup/ghcr.io/hosts.toml":   "foo = 'bar'",
 				"/etc/containerd/certs.d/docker.io/hosts.toml": `server = 'https://registry-1.docker.io'
 
-[host]
 [host.'http://127.0.0.1:5000']
-capabilities = ['pull', 'resolve']
-`,
+capabilities = ['pull', 'resolve']`,
 				"/etc/containerd/certs.d/foo.bar:5000/hosts.toml": `server = 'http://foo.bar:5000'
 
-[host]
 [host.'http://127.0.0.1:5000']
-capabilities = ['pull', 'resolve']
-`,
+capabilities = ['pull', 'resolve']`,
 			},
 		},
 		{
@@ -320,26 +306,22 @@ capabilities = ['pull', 'resolve']
 			mirrors:             stringListToUrlList(t, []string{"http://127.0.0.1:5000"}),
 			createConfigPathDir: true,
 			existingFiles: map[string]string{
-				"/etc/containerd/certs.d/_backup/docker.io/hosts.toml": "Hello World",
-				"/etc/containerd/certs.d/_backup/ghcr.io/hosts.toml":   "Foo Bar",
+				"/etc/containerd/certs.d/_backup/docker.io/hosts.toml": "hello = 'world'",
+				"/etc/containerd/certs.d/_backup/ghcr.io/hosts.toml":   "foo = 'bar'",
 				"/etc/containerd/certs.d/test.txt":                     "test",
 				"/etc/containerd/certs.d/foo":                          "bar",
 			},
 			expectedFiles: map[string]string{
-				"/etc/containerd/certs.d/_backup/docker.io/hosts.toml": "Hello World",
-				"/etc/containerd/certs.d/_backup/ghcr.io/hosts.toml":   "Foo Bar",
+				"/etc/containerd/certs.d/_backup/docker.io/hosts.toml": "hello = 'world'",
+				"/etc/containerd/certs.d/_backup/ghcr.io/hosts.toml":   "foo = 'bar'",
 				"/etc/containerd/certs.d/docker.io/hosts.toml": `server = 'https://registry-1.docker.io'
 
-[host]
 [host.'http://127.0.0.1:5000']
-capabilities = ['pull', 'resolve']
-`,
+capabilities = ['pull', 'resolve']`,
 				"/etc/containerd/certs.d/foo.bar:5000/hosts.toml": `server = 'http://foo.bar:5000'
 
-[host]
 [host.'http://127.0.0.1:5000']
-capabilities = ['pull', 'resolve']
-`,
+capabilities = ['pull', 'resolve']`,
 			},
 		},
 		{
@@ -352,48 +334,52 @@ capabilities = ['pull', 'resolve']
 			existingFiles: map[string]string{
 				"/etc/containerd/certs.d/docker.io/hosts.toml": `server = 'https://registry-1.docker.io'
 
-[host]
 [host.'http://example.com:30020']
 capabilities = ['pull', 'resolve']
 client = ['/etc/certs/xxx/client.cert', '/etc/certs/xxx/client.key']
 
 [host.'http://example.com:30021']
-capabilities = ['pull', 'resolve']
 client = ['/etc/certs/xxx/client.cert', '/etc/certs/xxx/client.key']
-`,
+capabilities = ['pull', 'resolve']
+
+[host.'http://bar.com:30020']
+capabilities = ['pull', 'resolve']
+client = ['/etc/certs/xxx/client.cert', '/etc/certs/xxx/client.key']`,
 			},
 			expectedFiles: map[string]string{
 				"/etc/containerd/certs.d/_backup/docker.io/hosts.toml": `server = 'https://registry-1.docker.io'
 
-[host]
 [host.'http://example.com:30020']
 capabilities = ['pull', 'resolve']
 client = ['/etc/certs/xxx/client.cert', '/etc/certs/xxx/client.key']
 
 [host.'http://example.com:30021']
-capabilities = ['pull', 'resolve']
 client = ['/etc/certs/xxx/client.cert', '/etc/certs/xxx/client.key']
-`,
+capabilities = ['pull', 'resolve']
+
+[host.'http://bar.com:30020']
+capabilities = ['pull', 'resolve']
+client = ['/etc/certs/xxx/client.cert', '/etc/certs/xxx/client.key']`,
 				"/etc/containerd/certs.d/docker.io/hosts.toml": `server = 'https://registry-1.docker.io'
 
-[host]
 [host.'http://127.0.0.1:5000']
 capabilities = ['pull', 'resolve']
 
 [host.'http://example.com:30020']
-client = ['/etc/certs/xxx/client.cert', '/etc/certs/xxx/client.key']
 capabilities = ['pull', 'resolve']
+client = ['/etc/certs/xxx/client.cert', '/etc/certs/xxx/client.key']
 
 [host.'http://example.com:30021']
+capabilities = ['pull', 'resolve']
 client = ['/etc/certs/xxx/client.cert', '/etc/certs/xxx/client.key']
+
+[host.'http://bar.com:30020']
 capabilities = ['pull', 'resolve']
-`,
+client = ['/etc/certs/xxx/client.cert', '/etc/certs/xxx/client.key']`,
 				"/etc/containerd/certs.d/foo.bar:5000/hosts.toml": `server = 'http://foo.bar:5000'
 
-[host]
 [host.'http://127.0.0.1:5000']
-capabilities = ['pull', 'resolve']
-`,
+capabilities = ['pull', 'resolve']`,
 			},
 		},
 	}
@@ -456,6 +442,92 @@ func TestMirrorConfigurationInvalidMirrorURL(t *testing.T) {
 	require.EqualError(t, err, "invalid registry url user has to be empty: https://foo@docker.io")
 }
 
+func TestExistingHosts(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	u, err := url.Parse("https://ghcr.io")
+	require.NoError(t, err)
+
+	eh, err := existingHosts(fs, "", *u)
+	require.NoError(t, err)
+	require.Empty(t, eh)
+
+	tomlHosts := `server = "https://registry-1.docker.io"
+[host."https://mirror.registry"]
+  capabilities = ["pull"]
+  ca = "/etc/certs/mirror.pem"
+  skip_verify = false
+  [host."https://mirror.registry".header]
+    x-custom-2 = ["value1", "value2"]
+
+[host]
+
+[host."https://mirror-bak.registry/us"]
+  capabilities = ["pull"]
+  skip_verify = true
+
+[host."http://mirror.registry"]
+  capabilities = ["pull"]
+
+[host."https://test-3.registry"]
+  client = ["/etc/certs/client-1.pem", "/etc/certs/client-2.pem"]
+
+[host."https://test-2.registry".header]
+  x-custom-2 = ["foo"]
+
+[host."https://test-1.registry"]
+  capabilities = ["pull", "resolve", "push"]
+  ca = ["/etc/certs/test-1-ca.pem", "/etc/certs/special.pem"]
+  client = [["/etc/certs/client.cert", "/etc/certs/client.key"],["/etc/certs/client.pem", ""]]
+
+[host."https://test-2.registry"]
+  client = "/etc/certs/client.pem"
+
+[host."https://non-compliant-mirror.registry/v2/upstream"]
+  capabilities = ["pull"]
+  override_path = true`
+
+	err = afero.WriteFile(fs, filepath.Join(backupDir, u.Host, "hosts.toml"), []byte(tomlHosts), 0o644)
+	require.NoError(t, err)
+	eh, err = existingHosts(fs, "", *u)
+	require.NoError(t, err)
+	expected := `[host.'https://mirror.registry']
+ca = '/etc/certs/mirror.pem'
+capabilities = ['pull']
+skip_verify = false
+
+[host.'https://mirror.registry'.header]
+x-custom-2 = ['value1', 'value2']
+
+[host.'https://mirror-bak.registry/us']
+capabilities = ['pull']
+skip_verify = true
+
+[host.'http://mirror.registry']
+capabilities = ['pull']
+
+[host.'https://test-3.registry']
+client = ['/etc/certs/client-1.pem', '/etc/certs/client-2.pem']
+
+[host.'https://test-1.registry']
+ca = ['/etc/certs/test-1-ca.pem', '/etc/certs/special.pem']
+capabilities = ['pull', 'resolve', 'push']
+client = [['/etc/certs/client.cert', '/etc/certs/client.key'], ['/etc/certs/client.pem', '']]
+
+[host.'https://test-2.registry']
+client = '/etc/certs/client.pem'
+
+[host.'https://test-2.registry'.header]
+x-custom-2 = ['foo']
+
+[host.'https://non-compliant-mirror.registry/v2/upstream']
+capabilities = ['pull']
+override_path = true`
+	require.Equal(t, expected, eh)
+
+}
+
 func stringListToUrlList(t *testing.T, list []string) []url.URL {
 	t.Helper()
 	urls := []url.URL{}