Default config_path if legacy registry options are not set. #6488

evankanderson · 2022-01-28T00:01:13Z

k8s-ci-robot · 2022-01-28T00:01:22Z

Hi @evankanderson. Thanks for your PR.

I'm waiting for a containerd 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 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.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

evankanderson · 2022-01-28T00:08:36Z

pkg/cri/server/image_pull.go

+	if len(cR.Mirrors) == 0 && len(cR.Configs) == 0 && len(cR.Auths) == 0 && len(cR.Headers) == 0 {
+		// No old-style configuration, default to docker-compatible defaults
+		// See https://github.com/containerd/containerd/issues/6485
+		c.config.Registry.ConfigPath = "/etc/containerd/certs.d:/etc/docker/certs.d"


This is Linux-specific. I don't know if I need to do something cross-platform here or not.

@jterry75 @kevpar - Do you know Docker's default in Windows?

Windows server directions are here; the TL;DR is that the certificates need to be installed into the system cert store on Windows, I think: https://docs.docker.com/registry/insecure/#use-self-signed-certificates

In addition, it seems like the Windows cert directions require restarting Docker, so it's not as seamless as the Linux process of putting certificates under the registry name in /etc/docker/certs.d.

would it help to set it here?https://github.com/containerd/containerd/blob/main/pkg/cri/config/config_unix.go#L29?

Sorry, needed to do a little research.

Setting the default in config_unix causes the default config to be merged with any config in the paths, which means that you can hit the error condition on line 400 of config.go by setting any of the (deprecated) mirrors or tls parameters and not explicitly setting config_path to the empty string.

It is still best to catch the conflict there. This is kind of deep in the path to start applying default values. Where is it checking whether ConfigPath is already set?

Good point!

I've moved this to the end of ValidatePluginConfig and updated the appropriate tests in config_test.go.

evankanderson · 2022-01-28T00:20:09Z

(DCO-signed)

evankanderson · 2022-01-28T05:53:55Z

/assign @cpuguy83

evankanderson · 2022-01-31T06:00:41Z

/assign @kzys

kzys

Thanks @evankanderson. LGTM!

kzys · 2022-02-07T23:51:31Z

/ok-to-test

adisky

Thanks @evankanderson LGTM
can you update the docs as well?
https://github.com/containerd/containerd/blob/main/docs/cri/registry.md
https://github.com/containerd/containerd/blob/main/docs/cri/config.md

Signed-off-by: Evan Anderson <evana@vmware.com>

evankanderson · 2022-02-08T18:31:41Z

Updated docs

adisky · 2022-02-10T06:57:03Z

LGTM

evankanderson · 2022-02-17T12:17:18Z

Is there anything else I need to do before this can be merged? (I don't have permissions to merge it myself.)

dmcgowan · 2022-02-17T16:23:53Z

pkg/cri/config/config.go

 	// Validation for deprecated auths options and mapping it to configs.
 	if len(c.Registry.Auths) != 0 {
+		if useConfigPath {
+			return errors.New("`auths` is deprecated, please use `config_path` instead")


Authorization via config_path is intentionally not supported today

The code looks like it supports setting an authorizer, but I've reverted this portion of the change and updated the tests.

evankanderson

Let me know if you want me to squash the commits before merging the PR.

evankanderson · 2022-02-18T01:23:21Z

pkg/cri/config/config.go

 		log.G(ctx).Warning("`auths` is deprecated, please use `configs` instead")
 	}

+	if len(c.Registry.Mirrors) == 0 && !hasDeprecatedTLS && len(c.Registry.Auths) == 0 {


Removed Registry.Auths here as well.

evankanderson · 2022-02-18T01:33:01Z

pkg/cri/config/config.go

 	// Validation for deprecated auths options and mapping it to configs.
 	if len(c.Registry.Auths) != 0 {
+		if useConfigPath {
+			return errors.New("`auths` is deprecated, please use `config_path` instead")


The code looks like it supports setting an authorizer, but I've reverted this portion of the change and updated the tests.

theopenlab-ci · 2022-02-18T02:00:01Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 48s (non-voting)
containerd-test-arm64 : FAILURE in 6m 01s (non-voting)
containerd-integration-test-arm64 : FAILURE in 25m 34s (non-voting)

theopenlab-ci · 2022-02-21T10:00:55Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 30s (non-voting)
containerd-test-arm64 : FAILURE in 7m 12s (non-voting)
containerd-integration-test-arm64 : FAILURE in 26m 09s (non-voting)

evankanderson · 2022-02-25T20:48:45Z

Anything else I need to do?

estesp · 2022-02-25T21:40:08Z

Can you squash your commits? We have one LGTM from a containerd maintainer so will need at least one more before merging. I've marked it as ready for review since you had already made the updates @dmcgowan asked for.

evankanderson · 2022-03-01T05:47:39Z

Squashed. Ready for another look.

theopenlab-ci · 2022-03-01T06:14:00Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 26s (non-voting)
containerd-test-arm64 : FAILURE in 5m 35s (non-voting)
containerd-integration-test-arm64 : FAILURE in 25m 08s (non-voting)

mikebrow

thinking this over...

right now default for configpath is "" .. (containerd config default > config.toml)

if a user edits the configpath in the config.toml the old mirror/tls has to have already been removed (checked by validator)

with this change we introduce a behavior change

if the configpath is "" and if mirrors/tls isn't set.. then nil configpath means use "/etc/containerd/certs.d:/etc/docker/certs.d"

an admin isn't informed or directing containerd to do that .. it just happens... default is now promoted to auto config from docker/certs.d + containerd/certs.d

if they don't want that to happen they have to add a dummy mirror..

this would be a behavior difference between 1.5 and main

it should be noted that the default config does not have mirrors/tls in it

...

my gut says it would be better, in main, to change the actual default value for config_path in new default configs from "" to the proposed, than to come up with a behavior change for what config_path = "" means retroactively against older configs...

something like:

diff --git a/pkg/cri/config/config_unix.go b/pkg/cri/config/config_unix.go
index ed75bb41c..6bf3d2c1e 100644
--- a/pkg/cri/config/config_unix.go
+++ b/pkg/cri/config/config_unix.go
@@ -69,6 +69,9 @@ func DefaultConfig() PluginConfig {
                        NetworkPluginMaxConfNum:   1, // only one CNI plugin config file will be loaded
                        NetworkPluginConfTemplate: "",
                },
+               Registry: Registry{
+                       ConfigPath: "/etc/containerd/certs.d:/etc/docker/certs.d",
+               },
                ContainerdConfig: ContainerdConfig{
                        Snapshotter:        containerd.DefaultSnapshotter,
                        DefaultRuntimeName: "runc",

and whatever is needed for config_windows.go

Signed-off-by: Evan Anderson <evan.k.anderson@gmail.com>

dmcgowan · 2025-10-17T00:53:30Z

Rebased to run another round of CI on latest main

evankanderson · 2025-10-17T19:40:39Z

I'm willing to do some bounded additional amount of work here, but I'm not sure that I'll be able to debug the current issues, as I don't understand the test harnesses and environments well enough to recognize flakes vs failures.

mikebrow

LGTM

Default config_path if legacy registry options are not set.

AkihiroSuda · 2025-10-23T23:25:01Z

internal/cri/config/config_unix.go

What about Windows ?

I don't know enough about Windows (or how to test) to suggest anything here.

Default config_path if legacy registry options are not set.

k8s-ci-robot added the needs-ok-to-test label Jan 28, 2022

evankanderson commented Jan 28, 2022

View reviewed changes

evankanderson force-pushed the main branch from 986d570 to cc32c58 Compare January 28, 2022 00:19

k8s-ci-robot assigned cpuguy83 Jan 28, 2022

k8s-ci-robot assigned kzys Jan 31, 2022

kzys approved these changes Jan 31, 2022

View reviewed changes

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Feb 7, 2022

adisky reviewed Feb 8, 2022

View reviewed changes

evankanderson pushed a commit to evankanderson/containerd that referenced this pull request Feb 8, 2022

Update documentation for containerd#6488

411743b

Signed-off-by: Evan Anderson <evana@vmware.com>

evankanderson force-pushed the main branch from 87286e1 to 411743b Compare February 8, 2022 18:31

dmcgowan reviewed Feb 17, 2022

View reviewed changes

evankanderson commented Feb 18, 2022

View reviewed changes

evankanderson force-pushed the main branch from d4fdcb0 to 28e251d Compare February 21, 2022 09:32

evankanderson force-pushed the main branch from 28e251d to 7db819b Compare March 1, 2022 05:47

mikebrow reviewed Mar 7, 2022

View reviewed changes

dmcgowan added this to the 2.2 milestone Apr 25, 2025

dmcgowan approved these changes Oct 17, 2025

View reviewed changes

Default config_path if legacy registry options are not set.

5386802

Signed-off-by: Evan Anderson <evan.k.anderson@gmail.com>

dmcgowan force-pushed the main branch from 567d4ec to 5386802 Compare October 17, 2025 00:52

dmcgowan moved this from Review In Progress to Merge on Green in Pull Request Review Oct 17, 2025

dmcgowan added this pull request to the merge queue Oct 17, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 17, 2025

mikebrow approved these changes Oct 17, 2025

View reviewed changes

github-project-automation bot moved this from Merge on Green to Review In Progress in Pull Request Review Oct 17, 2025

mikebrow added this pull request to the merge queue Oct 17, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 17, 2025

mikebrow added this pull request to the merge queue Oct 19, 2025

github-merge-queue bot pushed a commit that referenced this pull request Oct 19, 2025

Merge pull request #6488 from evankanderson/main

84de989

Default config_path if legacy registry options are not set.

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 19, 2025

mxpv added this pull request to the merge queue Oct 23, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 23, 2025

AkihiroSuda reviewed Oct 23, 2025

View reviewed changes

AkihiroSuda approved these changes Oct 23, 2025

View reviewed changes

AkihiroSuda added this pull request to the merge queue Oct 23, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 24, 2025

dmcgowan added this pull request to the merge queue Oct 24, 2025

github-merge-queue bot pushed a commit that referenced this pull request Oct 24, 2025

Merge pull request #6488 from evankanderson/main

c274dab

Default config_path if legacy registry options are not set.

dmcgowan moved this from Review In Progress to Merge on Green in Pull Request Review Oct 24, 2025

Merged via the queue into containerd:main with commit 9a43ee6 Oct 24, 2025
95 of 96 checks passed

github-project-automation bot moved this from In Progress to Done in Feature Tracking Oct 24, 2025

github-project-automation bot moved this from Merge on Green to Done in Pull Request Review Oct 24, 2025

akhilerm mentioned this pull request Dec 2, 2025

cri plugin fails to load when using registry.mirrors with config version 2 #12612

Open

Default config_path if legacy registry options are not set. #6488

Default config_path if legacy registry options are not set. #6488

Uh oh!

Conversation

evankanderson commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jan 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kzys Jan 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisky Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evankanderson commented Jan 28, 2022

Uh oh!

evankanderson commented Jan 28, 2022

Uh oh!

evankanderson commented Jan 31, 2022

Uh oh!

kzys left a comment

Choose a reason for hiding this comment

Uh oh!

kzys commented Feb 7, 2022

Uh oh!

adisky left a comment

Choose a reason for hiding this comment

Uh oh!

evankanderson commented Feb 8, 2022

Uh oh!

adisky commented Feb 10, 2022

Uh oh!

evankanderson commented Feb 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theopenlab-ci bot commented Feb 18, 2022

Uh oh!

theopenlab-ci bot commented Feb 21, 2022

Uh oh!

evankanderson commented Feb 25, 2022

Uh oh!

estesp commented Feb 25, 2022

Uh oh!

evankanderson commented Mar 1, 2022

Uh oh!

theopenlab-ci bot commented Mar 1, 2022

Uh oh!

mikebrow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmcgowan commented Oct 17, 2025

Uh oh!

Uh oh!

evankanderson commented Oct 17, 2025

evankanderson commented Jan 28, 2022 •

edited

Loading

kzys Jan 29, 2022 •

edited

Loading

adisky Jan 31, 2022 •

edited

Loading

mikebrow left a comment •

edited

Loading