Add client support for opencontainers/distribution-spec #819

rchincha · 2020-02-14T21:15:43Z

Add a new transport with URL prefix called "dist://"

This is an implementation of an OCI image repository client, based on the
opencontainers/distribution-spec.

For a full commit history, please see https://github.com/anuvu/image/commits/master

Consequently, a container image tool like skopeo when linked with this
version of containers/image can do:

"skopeo copy dist://src-server dist://dest-server"

where src-server and dest-server are both OCI dist-spec compliant
servers.

For the current state of server-side implementations of opencontainers/distribution-spec,
please see https://oci.bloodorange.io/

Signed-off-by: Ramkumar Chinchani [email protected]
Signed-off-by: Serge Hallyn [email protected]
Signed-off-by: Tycho Andersen [email protected]

Add a new transport with URL prefix called "dist://" This is an implementation of an OCI image repository client, based on the opencontainers/distribution-spec. For a full commit history, please see https://github.com/anuvu/image/commits/master Consequently, a container image tool like skopeo when linked with this version of containers/image can do: "skopeo copy dist://src-server dist://dest-server" where src-server and dest-server are both OCI dist-spec compliant servers. For the current state of server-side implementations of opencontainers/distribution-spec, please see https://oci.bloodorange.io/ Signed-off-by: Ramkumar Chinchani <[email protected]> Signed-off-by: Serge Hallyn <[email protected]> Signed-off-by: Tycho Andersen <[email protected]>

rchincha · 2020-02-14T21:16:07Z

Additional discussion about this topic at:
https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/avpDcOncnIE

tych0 · 2020-02-14T21:55:15Z

It might be nice to call this "oci-dist" or something; it's not really clear what "dist" means.

mtrmac · 2020-02-15T00:45:15Z

Thanks for the PR. I’ll make a quick pass through the code and add a few notes in a while.

Most importantly, though, are there actually any real differences between docker:// and the proposed transport?

You yourself say in the quoted thread that

That said the actual differences between docker:// and proposed dist:// transports are very slim
The original motivation for the PR, that Content-Type used when writing manifests is Docker-specific, is simply incorrect; dockerImageDestination.SupportedManifestMIMETypes does support both OCI manifests and indexes (unlike this PR, which only considers manifests, i.e. can’t handle multi-arch images), and the OCI types will be used by c/image/copy.Image if:
- the original image is OCI, or
- OCI conversion was explicitly required by copy.Options.ForceManifestMIMEType, or
- the source image is not OCI, but the destination registry rejects Docker manifest formats.
Looking at the commit history https://github.com/opencontainers/distribution-spec/commits/master/spec.md , the changes are overwhelmingly trivial clean-ups with no functional impact (the only non-trivial change I have noticed is the renaming of the session ID parameter during uploads, but the previous code didn’t use that at all anyway).

From a maintenance point of view, it is rather burdensome to have two significantly different implementations of substantially the same functionality, including one of the two transports that are most important and most often updated, for little end-user benefit. I don’t doubt that eventually there will be a fairly significant divergence, and I’m sure the docker:// transport name should eventually be retired in favor of a name that references the OCI distribution specification in some way, but in my view, neither of that, at least currently justifies:

~doubling the maintenance overhead
introducing the OCI transport in a way that has noticeably fewer features — and that makes it unattractive for users to even start to use the new transport, when they can use docker:// that auto-detects OCI support in most cases, and has more features. E.g.: missing support for registries.conf (registry blocking, redirection, mirrors), bearer token authentication (required for anonymous access to Docker Hub at least), Retry-After, blob info cache, multi-arch image, external layer

If it makes sense to introduce the new transport name and syntax right now, at all (which is unclear, especially WRT bothering users with the transition, given ~zero end-user benefit, but I’m open to considering that), I’m very insistent that it should share 99% of the code with the existing docker:// transport (probably by moving most of the implementation into a shared c/image/internal/… subpackage, and using it in both). Taking partial copies of the docker:// transport that are stripped of much of the functionality, and linking it to a separate HTTP client implementation that does mostly the same things, but differently, and having to maintain both is just not at all attractive, in my personal opinion, right now. (Note that other maintainers like @vrothberg might well disagree.)

But my first instinct is that the current docker:// transport can technically do, or should be easy to modify to do, everything that is necessary, and there’s just no technical need for this; all that I can see so far is the PR aspect of using a different name. Sure, that’s something, but it’s also not all that compelling from a technical point of view, and I’d prefer to discuss that before delving into implementation details.

mtrmac

Most importantly, please see the other comment about the undesirability of maintaining substantially the same functionality twice.

There is also not a single line of tests; how would this code keep working? (The docker:// transport is tested mostly via skopeo integration tests — definitely not ideal, but it’s something.)

mtrmac · 2020-02-15T00:46:32Z

dist/dist_client.go

+}
+
+//nolint (funlen)
+func NewOciRepo(ref *distReference, sys *types.SystemContext) (r OciRepo, err error) {


Does this have to be public at all? The c/image transports primarily exist to provide an unified interface, and we have a lot of trouble keeping the API stable enough, so I’m very hesitant about introducing new symbols without good justification.

mtrmac · 2020-02-15T00:48:57Z

dist/dist_client.go

+	port := "8080"
+	hostName := ""
+
+	if ref.server != "" {


No, please. A localhost registry seems like a pretty rare case to me, not really worth introducing a syntax variation (and especially not worth worrying about whether the user uses localhost or 127.0.0.1 for certificate names). String parsing of references is hard enough in the best cases.

Just have the user specify the server explicitly.

mtrmac · 2020-02-15T00:50:57Z

dist/dist_client.go

+	if sys != nil {
+		if sys.DockerAuthConfig != nil {
+			a := sys.DockerAuthConfig
+			creds = base64.StdEncoding.EncodeToString([]byte(a.Username + ":" + a.Password))


Why hard-code the implementation details here, when the code could store the user name and password separately, and have req.SetBasicAuth deal with the HTTP specifics?

mtrmac · 2020-02-15T00:55:04Z

dist/dist_client.go

+			creds = base64.StdEncoding.EncodeToString([]byte(a.Username + ":" + a.Password))
+		} else {
+			registry := fmt.Sprintf("%s:%s", server, port)
+			if username, password, err := config.GetAuthentication(sys, registry); err == nil {


This is not how config typically refers to registries: if the user does not specify a port, the registry parameter should not explicitly include one. If this code does not handle input the same way, things like podman login won’t create entries that will work.

mtrmac · 2020-02-15T00:56:29Z

dist/dist_client.go

+//nolint (funlen)
+func NewOciRepo(ref *distReference, sys *types.SystemContext) (r OciRepo, err error) {
+	server := "127.0.0.1"
+	port := "8080"


Does the OCI spec say anything about the default port? Won’t changing the default require users to explicitly add ports to interact with the vast majority of currently used public registries? And, see below, that would mean that the credential items set by podman login wouldn’t be found.

mtrmac · 2020-02-15T01:23:00Z

dist/transport.go

+}
+
+func (ref distReference) DockerReference() reference.Named {
+	return nil


Why? The references, AFAIK, use exactly the same namespace, and are pretty much 100% interoperable, and the DockerReference value is important for signing and for naming images in local storage (e.g. so that podman pull tags the result with the location of the image on the registry).

mtrmac · 2020-02-15T01:23:51Z

dist/transport.go

+	port := ""
+
+	if ref.port != -1 {
+		port = fmt.Sprintf("%d/", ref.port)


This does not look right; .port == -1 will result in "//server:fullname".

mtrmac · 2020-02-15T01:24:36Z

dist/transport.go

+}
+
+func (ref distReference) PolicyConfigurationIdentity() string {
+	return ref.StringWithinTransport()


PolicyConfigurationIdentity is supposed to be canonical, i.e. adjust for the defaulted server/port values.

mtrmac · 2020-02-15T01:26:07Z

go.sum

 github.com/containerd/go-runc v0.0.0-20180907222934-5a6d9f37cfa3/go.mod h1:IV7qH3hrUgRmyYrtgEeGWJfWbgcHL9CSRruz2Vqcph0=
 github.com/containerd/ttrpc v0.0.0-20190828154514-0e0f228740de/go.mod h1:PvCDdDGpgqzQIzDW1TphrGLssLDZp2GuS+X5DkEJB8o=
 github.com/containerd/typeurl v0.0.0-20180627222232-a93fcdb778cd/go.mod h1:Cm3kwCdlkCfMSHURc+r6fwoGH6/F1hH3S4sg0rLFWPc=
+github.com/containers/image v3.0.2+incompatible h1:B1lqAE8MUPCrsBLE86J0gnXleeRq8zJnQryhiiGQNyE=


Please drop this, to make sure the non-v5 imports never get in.

mtrmac · 2020-02-15T01:26:28Z

dist/transport.go

+type distTransport struct{}
+
+func (s distTransport) Name() string {
+	return "dist"


I suppose we will need bike-shedding over this :)

mtrmac · 2020-02-15T01:57:51Z

For reference, some previous conversation about the c/image/docker client vs. OCI spec compliance: #527 .

rhatdan · 2020-02-15T12:21:25Z

If the two distribution mechanisms get merged, it would be nice if the "docker" was changed to "dist" so we could remove a company name from a distribution type.

mtrmac · 2020-02-17T17:18:21Z

Maybe one way to think about this: what’s the end-user use case / mechanism? So far, it seems to be possible to support Docker schema1+schema2 + OCI with a single syntax, single workflow, single transport, single set of mostly consistent features, mostly automatically doing the right thing (there are definitely a few cases where the code doesn’t detect limited-capability registries, even if it should be technically possible, and users have to explicitly choose a manifest format — but none of them are fundamentally impossible to fix, AFAIK).

If there are two different transports, how do users interact with this? Are they completely interchangeable? If users use CLIs that deemphasize the transport: syntax (like podman pull/podman push), or outright don’t support it, which transport is used? For direct users of the transport: syntax (e.g. users of skopeo copy), when should they switch to the OCI transport, and what is the benefit they get for that work?

Starting with a protocol implementation without being clear on how the software is going to be used seems backwards to me. Users, ultimately, care very little about specifications, specification names, and fairly little about conformance of any particular implementation to any particular standard — working UIs that don’t get in the way so often trump all that.

vrothberg · 2020-02-18T10:02:01Z

I am quite excited about conforming with the OCI dist spec. Thanks a lot for tackling that!

I generally agree with @mtrmac's assessment.

Certainly, using docker:// for OCI distribution can be confusing but that's orthogonal to this PR and can be addressed by aliasing to a more generic term (e.g., registry://) that would continue supporting the various distribution protocols.

In this PR, we should wire in the OCI support into docker:// for all aforementioned reasons (see #819 (comment)). Duplicating the two might work for Skopeo and users who know exactly what they want, but I guess that most users either don't care or simply don't know these implementation details. I don't want users to bother about these details and let consumers of c/image continue working transparently without having to "try" different transports.

rchincha · 2020-02-18T18:18:31Z

@mtrmac and others, thanks for the detailed comments on the PR.

As per suggestions, I am working on another PR to incorporate OCI support into docker:// transport code itself and let it evolve from there.

rchincha · 2020-02-19T21:08:58Z

After some more investigation, it appears that perhaps no additional changes are needed in containers/image itself.

https://github.com/containers/skopeo/blob/master/docs/skopeo-copy.1.md

--format, -f manifest-type Manifest type (oci, v2s1, or v2s2) to use when saving image to directory using the 'dir:' transport (default is manifest type of source)

$ skopeo copy --format=oci docker://busybox:latest docker://<dist-spec-server>/busybox:latest

works just fine.

The support for "format" option appears to have been added a while back.

$ git tag --contains 485a7aa33
v0.1.33
v0.1.34
v0.1.35
v0.1.36
v0.1.37
v0.1.38
v0.1.39
v0.1.40
v0.1.41

tych0 · 2020-02-19T21:10:32Z

So it seems the goal should be to get thinks working in skopeo automatically, i.e. autodetecting the --format argument?

mtrmac · 2020-02-20T12:52:26Z

c/image is autodetecting[1] whether the destination supports an image format, and a conversion happens if the destination rejects it.

In base case, in which the registry supports the source image format without modification, that image is copied without conversion unless the user explicitly asks otherwise. (That way signatures, if any, and manifest digests are preserved if possible.)

What behavior are you looking for? Always converting an image to OCI even if the registry supports storing the original format does not seem to be obviously and unambiguously the right thing.

[1] There are a few known deficiencies, IIRC e.g. one registry implementation that accepts OCI without error but can’t return it: that one could be detected but currently isn’t. But those are implementation deficiencies, not deliberate design decisions.

tych0 · 2020-02-20T15:00:52Z

What behavior are you looking for? Always converting an image to OCI even if the registry supports storing the original format does not seem to be obviously and unambiguously the right thing.

For starters, if the source is OCI and the destination speaks OCI, it seems reasonable to keep it as OCI.

But I'm not sure I generally agree with this statement. Why not use the standard instead of a custom format where available?

mtrmac · 2020-02-20T15:09:15Z

What behavior are you looking for? Always converting an image to OCI even if the registry supports storing the original format does not seem to be obviously and unambiguously the right thing.

For starters, if the source is OCI and the destination speaks OCI, it seems reasonable to keep it as OCI.

Isn’t that already happening? If it isn’t, that’s most likely a bug.

But I'm not sure I generally agree with this statement. Why not use the standard instead of a custom format where available?

Because every conversion has some small risk of breaking the image itself, and a much bigger risk of changing how the image is treated by its consumers. (And there are other small reasons like schema2, but not OCI, supporting health check commands — or maybe that is just the Podman implementation, not sure.)

tych0 · 2020-02-20T15:12:37Z

Isn’t that already happening? If it isn’t, that’s most likely a bug.

Possibly, I don't know. I do know that you can't write to zot without --format=oci, so some part of the detection is broken.

mtrmac · 2020-02-20T15:14:08Z

(Also, the distinction between ”standard” and ”a custom format” is much less clear-cut that that wording implies, given how much software out there (still) doesn’t support OCI in full/correctly. Sure, that’s a chicken-and-egg problem, but forcing the issue by defaulting to OCI if the support is not seamless is not that likely to help — it’s more likely to motivate users to sprinkle scripts with --format=docker (compare https://issues.sonatype.org/browse/NEXUS-16947 ), cementing the previous format even further.)

rhatdan · 2020-03-25T12:52:58Z

@rchincha @vrothberg @mtrmac @tych0 Any update on this?

rchincha · 2020-03-25T17:49:08Z

@rchincha @vrothberg @mtrmac @tych0 Any update on this?

@rhatdan: couple of things going on:

in the dist-spec sphere, we are organizing conformance tests as per client workflows [1], [2] (@jdolitsky and @pmengelbert are driving this). The idea being not all client tools need everything and any changes needed are small and contained.
evaluating client tools for some tasks - e.g, skopeo (docker driver) appears to work fine for image push/pull. Others are buildah, oras, etc. Evaluating those.

References:
[1] opencontainers/distribution-spec#108
[2] https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/_GP6IqefKcg
[3] https://oci.bloodorange.io/

mtrmac · 2020-08-08T04:40:24Z

I’m going to close this, at least for now. If at all possible the existing c/image/docker code should be able to automatically work with both OCI and old docker/distribution registries, without user involvement.

I’m fairly sure it’s not a 100% compliant client right now (IIRC in the authentication behavior, if nothing else); it may turn out that the goal of a single automagic implementation is not achievable, and in that case we may need to revisit this.

rchincha requested a review from mtrmac February 14, 2020 21:17

Merge branch 'master' into upstream-master

5a24e12

mtrmac reviewed Feb 15, 2020

View reviewed changes

tych0 mentioned this pull request Jun 2, 2020

docker: allow uncompressed layers #947

Closed

mtrmac closed this Aug 8, 2020

Add client support for opencontainers/distribution-spec #819

Add client support for opencontainers/distribution-spec #819

Uh oh!

Conversation

rchincha commented Feb 14, 2020

Uh oh!

rchincha commented Feb 14, 2020

Uh oh!

tych0 commented Feb 14, 2020

Uh oh!

mtrmac commented Feb 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac Feb 15, 2020 • 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!

mtrmac Feb 15, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac commented Feb 15, 2020

Uh oh!

rhatdan commented Feb 15, 2020

Uh oh!

mtrmac commented Feb 17, 2020

Uh oh!

vrothberg commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchincha commented Feb 18, 2020

Uh oh!

rchincha commented Feb 19, 2020

Uh oh!

tych0 commented Feb 19, 2020

Uh oh!

mtrmac commented Feb 20, 2020

Uh oh!

tych0 commented Feb 20, 2020

Uh oh!

mtrmac commented Feb 20, 2020

Uh oh!

tych0 commented Feb 20, 2020

Uh oh!

mtrmac commented Feb 20, 2020

Uh oh!

rhatdan commented Mar 25, 2020

Uh oh!

rchincha commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac commented Aug 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mtrmac commented Feb 15, 2020 •

edited

Loading

mtrmac Feb 15, 2020 •

edited

Loading

mtrmac Feb 15, 2020 •

edited

Loading

vrothberg commented Feb 18, 2020 •

edited

Loading

rchincha commented Mar 25, 2020 •

edited

Loading