Repository: containerd/containerd
Pull request #10121 (2024-04-23 12:48:24)

fix migrateConfig for io.containerd.cri.v1.images
Adjust code sequence in `migrateConfig` to migrate `pinned_images` and `registry` first even when `containerd` does not exist.

Fix: #10120


k8s-ci-robot: Hi @ZhangShuaiyi. Thanks for your PR.

I'm waiting for a [containerd](https://github.com/orgs/containerd/people) member to verify that this patch is reasonable to test. If it is, they should reply with `/ok-to-test` on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should [join the org](https://github.com/containerd/project/blob/main/MAINTAINERS) to skip this step.

Once the patch is verified, the new status will be reflected by the `ok-to-test` label.

I understand the commands that are listed [here](https://go.k8s.io/bot-commands?repo=containerd%2Fcontainerd).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>


Patch:
diff --git a/plugins/cri/images/plugin.go b/plugins/cri/images/plugin.go
index ac00b9b16..70b999077 100644
--- a/plugins/cri/images/plugin.go
+++ b/plugins/cri/images/plugin.go
@@ -176,6 +176,33 @@ func configMigration(ctx context.Context, configVersion int, pluginConfigs map[s
 	return nil
 }
 func migrateConfig(dst, src map[string]interface{}) {
+	var pinnedImages map[string]interface{}
+	if v, ok := dst["pinned_images"]; ok {
+		pinnedImages = v.(map[string]interface{})
+	} else {
+		pinnedImages = map[string]interface{}{}
+	}
+
+	if simage, ok := src["sandbox_image"]; ok {
+		pinnedImages["sandbox"] = simage
+	}
+	if len(pinnedImages) > 0 {
+		dst["pinned_images"] = pinnedImages
+	}
+
+	for _, key := range []string{
+		"registry",
+		"image_decryption",
+		"max_concurrent_downloads",
+		"image_pull_progress_timeout",
+		"image_pull_with_sync_fs",
+		"stats_collect_period",
+	} {
+		if val, ok := src[key]; ok {
+			dst[key] = val
+		}
+	}
+
 	containerdConf, ok := src["containerd"]
 	if !ok {
 		return
@@ -205,20 +232,6 @@ func migrateConfig(dst, src map[string]interface{}) {
 		dst["runtime_platform"] = runtimePlatforms
 	}
 
-	var pinnedImages map[string]interface{}
-	if v, ok := dst["pinned_images"]; ok {
-		pinnedImages = v.(map[string]interface{})
-	} else {
-		pinnedImages = map[string]interface{}{}
-	}
-
-	if simage, ok := src["sandbox_image"]; ok {
-		pinnedImages["sandbox"] = simage
-	}
-	if len(pinnedImages) > 0 {
-		dst["pinned_images"] = pinnedImages
-	}
-
 	for _, key := range []string{
 		"snapshotter",
 		"disable_snapshot_annotations",
@@ -228,17 +241,4 @@ func migrateConfig(dst, src map[string]interface{}) {
 			dst[key] = val
 		}
 	}
-
-	for _, key := range []string{
-		"registry",
-		"image_decryption",
-		"max_concurrent_downloads",
-		"image_pull_progress_timeout",
-		"image_pull_with_sync_fs",
-		"stats_collect_period",
-	} {
-		if val, ok := src[key]; ok {
-			dst[key] = val
-		}
-	}
 }


Test patch:
diff --git a/plugins/cri/images/plugin_test.go b/plugins/cri/images/plugin_test.go
new file mode 100644
index 000000000..811a3e187
--- /dev/null
+++ b/plugins/cri/images/plugin_test.go
@@ -0,0 +1,70 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package images
+
+import (
+	"context"
+	"testing"
+
+	"github.com/containerd/containerd/v2/plugins"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestSandboxImageConfigMigration(t *testing.T) {
+	image := "rancher/mirrored-pause:3.9-amd64"
+	grpcCri := map[string]interface{}{
+		"sandbox_image": image,
+	}
+	pluginConfigs := map[string]interface{}{
+		string(plugins.GRPCPlugin) + ".cri": grpcCri,
+	}
+	configMigration(context.Background(), 2, pluginConfigs)
+	v, ok := pluginConfigs[string(plugins.CRIServicePlugin)+".images"]
+	images := v.(map[string]interface{})
+	require.True(t, ok)
+	v, ok = images["pinned_images"]
+	require.True(t, ok)
+	pinnedImages := v.(map[string]interface{})
+	v, ok = pinnedImages["sandbox"]
+	require.True(t, ok)
+	sandbox := v.(string)
+	assert.Equal(t, image, sandbox)
+}
+
+func TestRegistryConfigMigration(t *testing.T) {
+	path := "/etc/containerd/certs.d"
+	grpcCri := map[string]interface{}{
+		"registry": map[string]interface{}{
+			"config_path": path,
+		},
+	}
+	pluginConfigs := map[string]interface{}{
+		string(plugins.GRPCPlugin) + ".cri": grpcCri,
+	}
+	configMigration(context.Background(), 2, pluginConfigs)
+	v, ok := pluginConfigs[string(plugins.CRIServicePlugin)+".images"]
+	images := v.(map[string]interface{})
+	require.True(t, ok)
+	v, ok = images["registry"]
+	require.True(t, ok)
+	registry := v.(map[string]interface{})
+	v, ok = registry["config_path"]
+	require.True(t, ok)
+	configPath := v.(string)
+	assert.Equal(t, path, configPath)
+}