Add support for using a host registry dir in cri by cpuguy83 · Pull Request #4978 · containerd/containerd

cpuguy83 · 2021-01-29T01:39:40Z

Exposes the hosts dir config to CRI.
This will be used instead of the cri registry config in the main config toml.

Also fixes some bugs with the docker style cert file handling.

Closes #3071
Closes #4904

theopenlab-ci · 2021-01-29T01:52:38Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 7m 56s (non-voting)

AkihiroSuda · 2021-01-29T08:34:35Z

I think this should be defined as []string to allow specifying multiple directories.
The actual implementation for multi-dir can be another PR.

agreed! hosts_dir = ["/etc/docker/certs.d", "/etc/containerd/cri/hosts"] would be great for people migrating from docker to containerd.

sounds good to have more than one.. but may be confusing with multiple disparate config locations.. which has precedence.. do we error on overwrite. Just have to consider that complexity.

I would argue against this for the sake of saving complexity and rather err on the admin to configure the system rather than trying to make containerd handle all possibilities.

Maybe its not that bad if we add a helper function like func(registryHosts ...RegistryHosts) RegistryHosts.
Precedence is first match based on order of the []string

mikebrow

see comments

mikebrow · 2021-01-29T16:21:31Z

on the prior continue when not a dir ... :-O... thx for the fix.

On the continue when it is a dir .. does docker support the use of subdirectories should we walk here?

does docker support the use of subdirectories should we walk here?

No (or at least if it does its not intentional 😅).

Thanks for finding this and adding tests :)

mikebrow · 2021-01-29T16:42:35Z

sounds good to have more than one.. but may be confusing with multiple disparate config locations.. which has precedence.. do we error on overwrite. Just have to consider that complexity.

mikebrow · 2021-01-29T16:45:29Z

if we are going to override all of this registry section.. we should consider doing it at the containerd config level and deprecate the cri registry config

What's the "right" way to do this deprecation?

TBH I tried to make both work but its pretty tricky and I thought likely not worth the effort.

gulp... typically yes.

dmcgowan · 2021-02-05T18:14:03Z

Looks like I let this one slip through the cracks as well, there were a few items in my version of this which I think would be needed here as well to have a smoother transition (containerd/cri#1533)

cpuguy83 · 2021-02-16T18:36:10Z

Pulled in @dmcgowan's changes from containerd/cri#1533

theopenlab-ci · 2021-02-16T19:26:45Z

Build succeeded.

containerd-build-arm64 : NODE_FAILURE in 0s (non-voting)

dmcgowan · 2021-02-26T18:11:04Z

Thanks for finding this and adding tests :)

theopenlab-ci · 2021-02-27T00:28:06Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 45s (non-voting)

AkihiroSuda · 2021-03-06T04:49:43Z

Can we change the path to /etc/containerd/certs.d ?

Non-CRI apps such as nerdctl will want to use the same path

Also, that should be in the default config

I think the reason we aren't making it default is because it causes us to ignore other configs.

Updated the comment to use /etc/containerd/certs.d

AkihiroSuda · 2021-03-06T04:52:11Z

Can we have a first commit as a separate PR, so that we can merge it now and cherry-pick into 1.4?

AkihiroSuda · 2021-03-12T05:22:49Z

ping @cpuguy83 I'd like to see this included in v1.5 🙏

cpuguy83 · 2021-03-12T20:48:05Z

Opened #5186 with just the fixes, as requested.

cpuguy83 · 2021-03-12T20:49:48Z

This should have all requested changes in, except for @AkihiroSuda's request to turn it on by default for cri since this ends up overwriting other configs.

theopenlab-ci · 2021-03-12T20:58:22Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 50s (non-voting)

The certs dir parsing was skipping over files instead of reading them, as such the certs would never load. It was also stating the file name rather than the full path for cert pairs. Signed-off-by: Brian Goff <[email protected]>

This will be used instead of the cri registry config in the main config toml. --- Also pulls in changes from containerd/cri@d0b4eec Signed-off-by: Brian Goff <[email protected]>

theopenlab-ci · 2021-03-12T22:50:25Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 6m 22s (non-voting)

dmcgowan

LGTM

mraghu4 · 2021-07-28T11:36:27Z

Is this supported in v1.4.4?

AkihiroSuda · 2021-07-28T11:39:50Z

No, this is in v1.5

cpuguy83 requested review from AkihiroSuda, dmcgowan and mikebrow January 29, 2021 01:39

cpuguy83 mentioned this pull request Jan 29, 2021

[bug] --hosts-dir doesn't appear to work with a docker style hosts directory #4904

Closed

cpuguy83 force-pushed the certs_dir branch from 6b60a2f to f9647c1 Compare January 29, 2021 01:43

AkihiroSuda reviewed Jan 29, 2021

View reviewed changes

mikebrow reviewed Jan 29, 2021

View reviewed changes

AkihiroSuda added this to the 1.5 milestone Feb 1, 2021

cpuguy83 force-pushed the certs_dir branch from f9647c1 to 5d54b68 Compare February 16, 2021 18:34

mikebrow mentioned this pull request Feb 23, 2021

mirror and tls configuration are mutually exclusive #5065

Closed

dmcgowan reviewed Feb 26, 2021

View reviewed changes

cpuguy83 force-pushed the certs_dir branch from 5d54b68 to 3434098 Compare February 27, 2021 00:20

AkihiroSuda requested changes Mar 6, 2021

View reviewed changes

lubronzhan mentioned this pull request Mar 11, 2021

Harbor self-signed certificate auto-injection szlabs/harbor-automation-4k8s#18

Open

cpuguy83 force-pushed the certs_dir branch from 3434098 to ffad10b Compare March 12, 2021 20:45

cpuguy83 mentioned this pull request Mar 12, 2021

Fix docker style cert loading. #5186

Merged

cpuguy83 force-pushed the certs_dir branch from ffad10b to 87ee422 Compare March 12, 2021 20:51

cpuguy83 added 2 commits March 12, 2021 22:42

Fix docker style cert loading.

1fd99e2

The certs dir parsing was skipping over files instead of reading them, as such the certs would never load. It was also stating the file name rather than the full path for cert pairs. Signed-off-by: Brian Goff <[email protected]>

Add support for using a host registry dir in cri

b0b6d9a

This will be used instead of the cri registry config in the main config toml. --- Also pulls in changes from containerd/cri@d0b4eec Signed-off-by: Brian Goff <[email protected]>

cpuguy83 force-pushed the certs_dir branch from 87ee422 to b0b6d9a Compare March 12, 2021 22:42

AkihiroSuda approved these changes Mar 12, 2021

View reviewed changes

dmcgowan approved these changes Mar 15, 2021

View reviewed changes

dmcgowan merged commit 2755ead into containerd:master Mar 15, 2021

cpuguy83 deleted the certs_dir branch March 15, 2021 20:47

This was referenced Sep 30, 2021

[EKS] [CRI]: Support for Containerd CRI aws/containers-roadmap#313

Closed

[EKS] [AMI]: Upgrade containerd to 1.5.x aws/containers-roadmap#1521

Closed

Conversation

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

Uh oh!

Uh oh!

theopenlab-ci Bot commented Jan 29, 2021

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!

Choose a reason for hiding this comment

Uh oh!

mikebrow 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

dmcgowan commented Feb 5, 2021

Uh oh!

cpuguy83 commented Feb 16, 2021

Uh oh!

theopenlab-ci Bot commented Feb 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

theopenlab-ci Bot commented Feb 27, 2021

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!

AkihiroSuda commented Mar 6, 2021

Uh oh!

AkihiroSuda commented Mar 12, 2021

Uh oh!

cpuguy83 commented Mar 12, 2021

Uh oh!

cpuguy83 commented Mar 12, 2021

Uh oh!

theopenlab-ci Bot commented Mar 12, 2021

Uh oh!

theopenlab-ci Bot commented Mar 12, 2021

Uh oh!

dmcgowan left a comment

Choose a reason for hiding this comment

Uh oh!

mraghu4 commented Jul 28, 2021

Uh oh!

AkihiroSuda commented Jul 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

cpuguy83 commented Jan 29, 2021 •

edited

Loading