Repository: spegel-org/spegel
Pull request #603 (2024-10-12 10:26:18)

Fix append to backup always happening
The change in #601 fixed mirror ordering but introduced a bug which caused the mirror configuration to always append on the backup. This change fixes the regression and adds a test to verify configuration is only appended when enabled.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8a6ce64..7fc1de0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -17,6 +17,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- [#603](https://github.com/spegel-org/spegel/pull/603) Fix append to backup always happening.
+
 ### Security
 
 ## v0.0.26
diff --git a/pkg/oci/containerd.go b/pkg/oci/containerd.go
index c0aff67..f812646 100644
--- a/pkg/oci/containerd.go
+++ b/pkg/oci/containerd.go
@@ -361,13 +361,19 @@ func AddMirrorConfiguration(ctx context.Context, fs afero.Fs, configPath string,
 		capabilities = append(capabilities, "resolve")
 	}
 	for _, registryURL := range registryURLs {
-		existingHosts, err := existingHosts(fs, configPath, registryURL)
+		templatedHosts, err := templateHosts(registryURL, mirrorURLs, capabilities)
 		if err != nil {
 			return err
 		}
-		templatedHosts, err := templateHosts(registryURL, mirrorURLs, capabilities, existingHosts)
-		if err != nil {
-			return err
+		if appendToBackup {
+			existingHosts, err := existingHosts(fs, configPath, registryURL)
+			if err != nil {
+				return err
+			}
+			if existingHosts != "" {
+				templatedHosts = templatedHosts + "\n\n" + existingHosts
+			}
+			log.Info("appending to existing Containerd mirror configuration", "registry", registryURL.String())
 		}
 		fp := path.Join(configPath, registryURL.Host, "hosts.toml")
 		err = fs.MkdirAll(path.Dir(fp), 0o755)
@@ -378,11 +384,7 @@ func AddMirrorConfiguration(ctx context.Context, fs afero.Fs, configPath string,
 		if err != nil {
 			return err
 		}
-		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)
-		}
+		log.Info("added Containerd mirror configuration", "registry", registryURL.String(), "path", fp)
 	}
 	return nil
 }
@@ -456,7 +458,7 @@ func clearConfig(fs afero.Fs, configPath string) error {
 	return nil
 }
 
-func templateHosts(registryURL url.URL, mirrorURLs []url.URL, capabilities []string, existingHosts string) (string, error) {
+func templateHosts(registryURL url.URL, mirrorURLs []url.URL, capabilities []string) (string, error) {
 	server := registryURL.String()
 	if registryURL.String() == "https://docker.io" {
 		server = "https://registry-1.docker.io"
@@ -485,11 +487,7 @@ capabilities = {{ $.Capabilities }}
 	if err != nil {
 		return "", err
 	}
-	output := strings.TrimSpace(buf.String())
-	if existingHosts != "" {
-		output = output + "\n\n" + existingHosts
-	}
-	return output, nil
+	return strings.TrimSpace(buf.String()), nil
 }
 
 type hostFile struct {


Test patch:
diff --git a/pkg/oci/containerd_test.go b/pkg/oci/containerd_test.go
index bff1fcd..5fa4d82 100644
--- a/pkg/oci/containerd_test.go
+++ b/pkg/oci/containerd_test.go
@@ -209,10 +209,11 @@ func TestMirrorConfiguration(t *testing.T) {
 		appendToBackup      bool
 	}{
 		{
-			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.2:5000", "http://127.0.0.1:5001"}),
+			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.2:5000", "http://127.0.0.1:5001"}),
+			appendToBackup: false,
 			expectedFiles: map[string]string{
 				"/etc/containerd/certs.d/foo.bar:5000/hosts.toml": `server = 'http://foo.bar:5000'
 
@@ -227,10 +228,11 @@ capabilities = ['pull', 'resolve']`,
 			},
 		},
 		{
-			name:        "resolve tags disabled",
-			resolveTags: false,
-			registries:  stringListToUrlList(t, []string{"https://docker.io", "http://foo.bar:5000"}),
-			mirrors:     stringListToUrlList(t, []string{"http://127.0.0.1:5000"}),
+			name:           "resolve tags disabled",
+			resolveTags:    false,
+			registries:     stringListToUrlList(t, []string{"https://docker.io", "http://foo.bar:5000"}),
+			mirrors:        stringListToUrlList(t, []string{"http://127.0.0.1:5000"}),
+			appendToBackup: false,
 			expectedFiles: map[string]string{
 				"/etc/containerd/certs.d/docker.io/hosts.toml": `server = 'https://registry-1.docker.io'
 
@@ -248,6 +250,7 @@ capabilities = ['pull']`,
 			registries:          stringListToUrlList(t, []string{"https://docker.io", "http://foo.bar:5000"}),
 			mirrors:             stringListToUrlList(t, []string{"http://127.0.0.1:5000"}),
 			createConfigPathDir: false,
+			appendToBackup:      false,
 			expectedFiles: map[string]string{
 				"/etc/containerd/certs.d/docker.io/hosts.toml": `server = 'https://registry-1.docker.io'
 
@@ -265,6 +268,7 @@ capabilities = ['pull', 'resolve']`,
 			registries:          stringListToUrlList(t, []string{"https://docker.io", "http://foo.bar:5000"}),
 			mirrors:             stringListToUrlList(t, []string{"http://127.0.0.1:5000"}),
 			createConfigPathDir: true,
+			appendToBackup:      false,
 			expectedFiles: map[string]string{
 				"/etc/containerd/certs.d/docker.io/hosts.toml": `server = 'https://registry-1.docker.io'
 
@@ -282,6 +286,7 @@ capabilities = ['pull', 'resolve']`,
 			registries:          stringListToUrlList(t, []string{"https://docker.io", "http://foo.bar:5000"}),
 			mirrors:             stringListToUrlList(t, []string{"http://127.0.0.1:5000"}),
 			createConfigPathDir: true,
+			appendToBackup:      false,
 			existingFiles: map[string]string{
 				"/etc/containerd/certs.d/docker.io/hosts.toml": "hello = 'world'",
 				"/etc/containerd/certs.d/ghcr.io/hosts.toml":   "foo = 'bar'",
@@ -305,6 +310,7 @@ capabilities = ['pull', 'resolve']`,
 			registries:          stringListToUrlList(t, []string{"https://docker.io", "http://foo.bar:5000"}),
 			mirrors:             stringListToUrlList(t, []string{"http://127.0.0.1:5000"}),
 			createConfigPathDir: true,
+			appendToBackup:      false,
 			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'",
@@ -378,6 +384,52 @@ 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.'http://127.0.0.1:5000']
+capabilities = ['pull', 'resolve']`,
+			},
+		},
+		{
+			name:                "append to backup disabled",
+			resolveTags:         true,
+			registries:          stringListToUrlList(t, []string{"https://docker.io", "http://foo.bar:5000"}),
+			mirrors:             stringListToUrlList(t, []string{"http://127.0.0.1:5000"}),
+			createConfigPathDir: true,
+			appendToBackup:      false,
+			existingFiles: map[string]string{
+				"/etc/containerd/certs.d/docker.io/hosts.toml": `server = 'https://registry-1.docker.io'
+
+[host.'http://example.com:30020']
+capabilities = ['pull', 'resolve']
+client = ['/etc/certs/xxx/client.cert', '/etc/certs/xxx/client.key']
+
+[host.'http://example.com:30021']
+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.'http://example.com:30020']
+capabilities = ['pull', 'resolve']
+client = ['/etc/certs/xxx/client.cert', '/etc/certs/xxx/client.key']
+
+[host.'http://example.com:30021']
+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.'http://127.0.0.1:5000']
+capabilities = ['pull', 'resolve']`,
+				"/etc/containerd/certs.d/foo.bar:5000/hosts.toml": `server = 'http://foo.bar:5000'
+
 [host.'http://127.0.0.1:5000']
 capabilities = ['pull', 'resolve']`,
 			},