Skip to content

Registry configuration package#4138

Merged
mxpv merged 3 commits intocontainerd:masterfrom
dmcgowan:registry-configuration-tools
Apr 2, 2020
Merged

Registry configuration package#4138
mxpv merged 3 commits intocontainerd:masterfrom
dmcgowan:registry-configuration-tools

Conversation

@dmcgowan
Copy link
Copy Markdown
Member

@dmcgowan dmcgowan commented Apr 1, 2020

Finally finished this one up, please review the configuration file format. This builds on @AkihiroSuda 's PR which adds support for Docker certificate directory configuration. In addition to that, a hosts.toml file can exist in that directory to full specify all the registry hosts, including host specific capabilities and certificates. See the unit test for an example of the format. I can write up some documentation once there is consensus on the format for it.

Add `remotes/certutil` functions for loading `ca.crt`, `client.cert`, and `client.key` into `tls.Config` from a directory like `/etc/docker/certs.d/<hostname>.

See https://docs.docker.com/engine/security/certificates/ .

Client applications including CRI plugin are expected to configure the resolver using these functions.

As an example, the `ctr` tool is extended to support `ctr images pull --certs-dir=/etc/docker/certs.d example.com/foo/bar:baz`.

Tested with Harbor 1.8.

Signed-off-by: Akihiro Suda <[email protected]>
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Apr 1, 2020

Build succeeded.

@dmcgowan dmcgowan force-pushed the registry-configuration-tools branch 2 times, most recently from bb775e3 to 8f5a61b Compare April 1, 2020 05:16
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Apr 1, 2020

Build succeeded.

@dmcgowan dmcgowan force-pushed the registry-configuration-tools branch from 8f5a61b to 7c4229d Compare April 1, 2020 05:26
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Apr 1, 2020

Build succeeded.

Add configuration toml file format and configuration
function to configure registry hosts from a directory
based configuration. Compatible with Docker registry
certificate loading.

Signed-off-by: Derek McGowan <[email protected]>
Moved registry host configuration to the config package
and allows support of loading configurations from a
directory when the hosts are being resolved.

Signed-off-by: Derek McGowan <[email protected]>
@dmcgowan dmcgowan force-pushed the registry-configuration-tools branch from 7c4229d to 547301c Compare April 1, 2020 05:52
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Apr 1, 2020

Build succeeded.

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #4138 into master will decrease coverage by 0.32%.
The diff coverage is 27.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4138      +/-   ##
==========================================
- Coverage   42.42%   42.09%   -0.33%     
==========================================
  Files         130      133       +3     
  Lines       14884    15209     +325     
==========================================
+ Hits         6314     6402      +88     
- Misses       7651     7878     +227     
- Partials      919      929      +10     
Flag Coverage Δ
#linux 45.40% <29.36%> (-0.34%) ⬇️
#windows 37.91% <27.21%> (-0.28%) ⬇️
Impacted Files Coverage Δ
remotes/docker/config/config_unix.go 0.00% <0.00%> (ø)
remotes/docker/config/config_windows.go 0.00% <0.00%> (ø)
remotes/docker/config/hosts.go 28.85% <28.85%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e852da5...547301c. Read the comment docs.

"github.com/pkg/errors"
)

type hostConfig struct {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, if cri plugin want to use some tls configuration, cri plugin can not reach here.

I think maybe cri plugin also need this feature, and plugin just use WithSkipVerify(args) or WithHostDir(args) to finish

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CRI plugin already has its own way of turning its configuration into a registry host list. Instead of trying to expand the CRI configuration to support everything this does, I would rather allow CRI plugin to specify a hosts directory so containerd doesn't require a restart to pick up registry configuration changes. If we decide it is best to share this code in CRI configuration, we can export it then, that might not be the best route though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With users moving from dockershim I think this would be pretty desirable. Has there been any follow-up on this?

@mxpv mxpv merged commit 9ba5ea2 into containerd:master Apr 2, 2020
tlsConfig.Certificates = append(tlsConfig.Certificates, cert)
}
}
tr := defaultTransport.Clone()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this makes it break on Go < 1.13;

go get golang.org/dl/go1.12.17 && go1.12.17 download

go1.12.17 run cmd/gen-manpages/main.go ctr man/
# github.com/containerd/containerd/remotes/docker/config
../../go/src/github.com/containerd/containerd/remotes/docker/config/hosts.go:193:27: defaultTransport.Clone undefined (type *http.Transport has no field or method Clone)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.12 reached EOL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants