Drop c/i/docker/reference (again) #221

mtrmac · 2017-01-20T08:37:37Z

This is a follow-up on #220 , which minimally updated us for the changed API.

This wholesale transitions containers/image to rely on the docker.io/library canonicalization support now available in docker/distribution/reference, removing our c/i/docker/reference fork of docker/docker/reference. That’s a nice simplification, but also a blind bet on how projectatomic/docker will/will not port its support for unqualified references to the new docker/distribution/reference.

There are three ways to review this PR:

As the resulting diff, which simply removes c/i/docker/distribution/reference and replaces all users in one go, somewhat difficult to follow for a total of +184-702 lines.
As a sequence of 37 commits, with a total of +864-1382 lines, but with a good explanation of every step, and tests (apart from golint) passing along the way
Just give up and reject it outright :)

Choose your poison.

NOTE: This changes behavior. Before, docker/reference.ParseNamed would silently turn name:tag@digest into name@digest, dropping the tag. Now, distribution/reference.ParseNormalizedNamed preserves all of the information, which triggers several code paths expecting such complex values and rejecting them outright.

I don’t think that is likely to hurt, and it is usually not quite trivial to see what the behavior on a tag+digest should be (especially because further consumers are or may not be ready to correctly handle this). We could, though, define and implement some semantics for such values.

runcom · 2017-01-20T08:46:44Z

but also a blind bet on how projectatomic/docker will/will not port its support for unqualified references to the new docker/distribution/reference.

@mtrmac what do you mean with this? Should I start just using docker/distribution/reference directly here https://github.com/projectatomic/docker/blob/docker-1.12.6/distribution/pull_v2_unix.go#L52

mtrmac · 2017-01-20T08:51:34Z

but also a blind bet on how projectatomic/docker will/will not port its support for unqualified references to the new docker/distribution/reference.

@mtrmac what do you mean with this?

We have had to make a copy of docker/docker/reference because projectatomic/docker was patching its version in a way which broke signatures. (#123).

AFAICT the plan with the new docker/distribution/reference is to eliminate docker/docker/reference entirely (see e.g. dmcgowan/docker#27); if that happens, projectatomic/docker will have to change something to keep its current behavior of searching across registries; and it is currently unknown whether that would involve changes to docker/distribution/reference, and if so, whether they would change the semantics / break our use cases again.

update opencontainers/image-spec

…ture After containers#220, and especially future containers#221, signing docker/distribution/reference.Named.String() would use the new fully-expanded normalization (as opposed to containers/image/docker/reference.Named.String(), which is minimized). For interoperability between various versions and signers, parse and normalize the expected and signed references before comparing them. This should be equivalent to prmMatchExact.matchesDockerReference(). Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac · 2017-01-31T19:06:24Z

@runcom Opinions on this? Rebasing this branch over time is doable, but it is rather painful.

Looking at upstream https://github.com/docker/docker/blob/master/reference/reference.go , it has now become a pretty trivial forwarding layer to docker/distribution/reference—with docker/docker/reference.Named conforming to the docker/distribution/reference.Named interface but having different normalization semantics, which seems pretty confusing; it is much better to eliminate the docker/docker/reference semantics IMO, as this PR is doing.

Separately, per the conversation in #207 , due to version drift we probably can’t avoid either vendoring or forking the docker/distribution/reference package. My proposal is to keep this PR as is, and land a single “copy docker/distribution/reference to containers/image/docker/reference` commit on top. Does that sound acceptable?

runcom · 2017-02-01T08:31:11Z

Separately, per the conversation in #207 , due to version drift we probably can’t avoid either vendoring or forking the docker/distribution/reference package. My proposal is to keep this PR as is, and land a single “copy docker/distribution/reference to containers/image/docker/reference` commit on top. Does that sound acceptable?

works for me. The only thing I fear right now is the huge amount of work I think I need to do in projectatomic/docker for the multi registries story. But yeah, that sounds fine to me.

mtrmac · 2017-02-01T15:44:07Z

The only thing I fear right now is the huge amount of work I think I need to do in projectatomic/docker for the multi registries story.

I appreciate that that will be painful for projectatomic/docker; does that mean something should be changed in containers/image, though? It seems to me that projectatomic/docker does the search / resolution in any way it wants, and then it just submits the resolved name to containers/image. At worst the reference will have to go though a “format as string → parse into the containers/image-normalized format” roundtrip, which IIRC is currently happening as well.

runcom · 2017-02-01T15:46:01Z

I appreciate that that will be painful for projectatomic/docker; does that mean something should be changed in containers/image, though?

nope, I didn't mean this, sorry

mtrmac · 2017-02-01T21:58:33Z

OK, updated:

Updated for another API change in reference: Replace EnsureTagged with TagNameOnly distribution/distribution#2173
Dropped a no longer necessary workaround for Infinite recursion in digestReference.String() distribution/distribution#2163
Added a final copy which adds a containers/image/docker/reference as a fork of docker/distribution/reference, with the digestset dependency removed.

runcom · 2017-02-07T13:27:22Z

Added a final copy which adds a containers/image/docker/reference as a fork of docker/distribution/reference, with the digestset dependency removed.

so, with this bullet in place I'm free to patch projectatomic/docker whatever I want wrt references and that wouldn't affect c/image signatures, correct? I'm still trying to find a way not to patch projectatomic/docker but I'm struggling doing that...we'll see.

mtrmac · 2017-02-07T14:17:27Z

so, with this bullet in place I'm free to patch projectatomic/docker whatever I want wrt references and that wouldn't affect c/image signatures, correct?

Assuming a docker/docker/reference.Named→string→containers/image/docker/reference.Named roundtrip (which IIRC the current verification patch is already doing), sure.

runcom · 2017-02-07T14:18:27Z

lgtm

…/reference This is an intermediate step which will eventually go away. The goal of this PR is to get rid of c/i/docker/daemon/reference and to replace uses of it by direct calls to docker/distribution/reference. We can't do that safely and easily, because the two have different semantics for reference.Named.Name() and reference.Named.String(): we return a minimized version, e.g. "busybox", upstream returns an expanded version, e.g. "docker.io/library/busybox". BEFORE this commit the difference is hidden by using docker/distribution/reference.WithName, which allows using the minimized version, and works with it correctly; but because we want to use the upstream canonicalization code, which will change semantics, we can't just mix and match. To make the distinction explicit, this commmit adds an X to ALL public names from c/i/docker/daemon/reference. E.g. a reference.XNamed type, which has methods XName and XString. This is pretty large, but does not change behavior at all. By inspection it is clear to see that reference.XNamed and subtypes does not expose any of the non-X, conflicting, method names. Using e.g. > git diff --word-diff-regex=.|grep -F '{+'|grep -v '^$[^{]\|{+X+}$*{\?$' it is possible to see that most lines in this diff only add a single X letter, and manually inspect the few lines which don't match the regexp. The only REALLY new code is an explicit definition of namedRef.XName() and namedRef.XString(), and two newly added casts to namedRef in cases where we need to use the underlying distreference.Reference within a reference.XNamed value. Strictly speaking these changes change behavior, in that third-party implementations of reference.XNamed are no longer accepted; but we broke them by renaming at all. Signed-off-by: Miloslav Trmač <[email protected]>

To start a transition to the upstream distreference.Named canonicalization semantics, first start computing the upstream value: In namedRef (and its subtypes), carry BOTH an "our" field (with existing semantics, canonical = minimal) and "upstream" field (with the upstream semantics, canonical = fully explicit). .upstream is currently essentially write-only: it is used _only_ to compute further .upstream values. Therefore, this does not change behavior (perhaps apart from a bit more error checking which now happens on the upstream value). To make this reasonably possible, some of the public methods return a *namedRef instead of a public type, which breaks golint. This is temporary. Signed-off-by: Miloslav Trmač <[email protected]>

Start transitioning from .our uses to .upstream. First in the simplest cases: taggedRef.Tag() and canonicalRef.Digest() are values in principle unaffected by the name canonicalization, so this should be an obviously correct change which does not change behavior. Starting with this one to demostrate the principle of moving step by step. Signed-off-by: Miloslav Trmač <[email protected]>

In the “new” methods introduced in docker/reference.[X]Named, to return the fully expanded host/path/both, instead of using .our and expanding it in splitHostname, rely on the fully-expanded .upstream and its fully-expanded .Name(), and the newly introduced distreference.Domain() and distreference.Path() helpers. Signed-off-by: Miloslav Trmač <[email protected]>

…ormalization Call the newly provided distreference.FamilarName and distreference.FamiliarString instead of using our minimal canonical version. This removes the last “externally-visible” uses of .our. Signed-off-by: Miloslav Trmač <[email protected]>

Now that namedRef.our values are only used for computing other namedRef.our values, drop the struct member and all code computing it, including the entirety of our normalization code. We still keep .upstream as a private member instead of using distreference.Named directly, or making namedRef an implementation of distreference.Named. BEHAVIOR CHANGE: We used to minimize the input and then check whether it is a 64-char hex string, now distreference.ParseNormalizedNamed first checks for a 64-char hext string and then normalized (and by expanding, not minimizing). Hence, things like docker.io/$64hexchars are now accepted, which is a behavior change (noticed by the tests). Though, there is really no risk of confusing such a value with a digest reference, so this behavior change seems quite acceptable. Signed-off-by: Miloslav Trmač <[email protected]>

Instead of embedding a distreference.Named as a private field, embed it as an anonymous field, making namedRef a valid distreference.Named implementation. This is EXTREMELY ugly. In theory, docker/distribution/reference should be able to work with any valid input implementing distreference.Named() equally, based on only what the public method implementations return. In practice, the code expects specific implementations of internal interfaces, and merely embeding a distreference.Named into our struct makes our struct _not_ implement these internal interfaces. We are forced to explicitly define forwarding methods, using an undocumented knowledge that the returned distreference.Named implements them. Soon enough we will completely eiliminate namedRef and use a distreference.Named directly, and then distreference can keep playing these ugly games without us having to care. Signed-off-by: Miloslav Trmač <[email protected]>

Instead call distreference.Named.Name() in all users. Signed-off-by: Miloslav Trmač <[email protected]>

Now that canonicalRef merely wraps a distreference.Canonical, adding no functionality, just use a distreference.Canonical directly. Signed-off-by: Miloslav Trmač <[email protected]>

Instead use distreference.Canonical directly. Signed-off-by: Miloslav Trmač <[email protected]>

The two functions are line-by-line identical now. Signed-off-by: Miloslav Trmač <[email protected]>

Instead call distreference.IsNameOnly directly. Signed-off-by: Miloslav Trmač <[email protected]>

…ultTag Signed-off-by: Miloslav Trmač <[email protected]>

Instead use distreference.TagNameOnly directly. Signed-off-by: Miloslav Trmač <[email protected]>

(This could have been done a few commits ago.) Now that namedRef merely wraps a distreference.Named, adding no functionality, just use a distreference.Named directly. Signed-off-by: Miloslav Trmač <[email protected]>

Instead use distreference.ParseNormalizedNamedDirectly (and update obsolete comments). Signed-off-by: Miloslav Trmač <[email protected]>

We have _just_ normalized it, no need to do it again. (distreference.WithName does no checking; we could also call distreference.ParseNamed which does, but that does the checking by calling ParseNormalizedNamed anyway, again. We will eliminate this soon anyway…) Signed-off-by: Miloslav Trmač <[email protected]>

Signed-off-by: Miloslav Trmač <[email protected]>

Instead of rebuilding it as name/name+digest/name+tag, just use the return value from distreference.ParseNormalizedName without modification. THIS CHANGES BEHAVIOR: before, name@tag:digest inputs were silently trated as name:digest, dropping the tag; now the semantics is correctly preserved. We already anticipate such strings as references in docker: and docker-daemon: (where they are now rejected) and in signature verification (where, unless we check repository names only, they must match exactly). Signed-off-by: Miloslav Trmač <[email protected]>

Instead call distreference.ParseNormalizedNamed directly. (This looks bigger than it really is because so many files now don't need c/i/docker/reference, so they are dropping the “distreference” qualifier for docker/distribution/reference.) Signed-off-by: Miloslav Trmač <[email protected]>

…ParseAnyReference Signed-off-by: Miloslav Trmač <[email protected]>

Use distreference.ParseAnyReference directly. Signed-off-by: Miloslav Trmač <[email protected]>

Signed-off-by: Miloslav Trmač <[email protected]>

This replaces the copy of github.com/docker/docker/reference in the same place, which we have just gotten rid of, and allows using this package even in consumers which insist on an incompatible version of docker/distribution. The copy has been edited to drop a reference to github.com/docker/distribution/digestset . Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac · 2017-02-07T14:47:36Z

👍

…ture After containers/image#220, and especially future containers/image#221, signing docker/distribution/reference.Named.String() would use the new fully-expanded normalization (as opposed to containers/image/docker/reference.Named.String(), which is minimized). For interoperability between various versions and signers, parse and normalize the expected and signed references before comparing them. This should be equivalent to prmMatchExact.matchesDockerReference(). Signed-off-by: Miloslav Trmač <[email protected]>

giuseppe pushed a commit to giuseppe/image that referenced this pull request Jan 24, 2017

Merge pull request containers#221 from runcom/fix-oci-image-spec

f69a78f

update opencontainers/image-spec

mtrmac mentioned this pull request Jan 24, 2017

Canonicalize docker references in signature.VerifyDockerManifestSignature #227

Merged

mtrmac force-pushed the no-docker-reference branch from 428d5d4 to 3415753 Compare January 31, 2017 19:00

mtrmac force-pushed the no-docker-reference branch 3 times, most recently from 6f7e5d7 to 45e6cf3 Compare February 1, 2017 21:57

mtrmac force-pushed the no-docker-reference branch 3 times, most recently from 11157b6 to 8a51683 Compare February 6, 2017 20:55

mtrmac added 8 commits February 7, 2017 15:24

API transition: Drop XNamed.XFullName

df65181

Instead call distreference.Named.Name() in all users. Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac added 16 commits February 7, 2017 15:25

Implementation collapse: Drop canonicalRef

c91c7c2

Now that canonicalRef merely wraps a distreference.Canonical, adding no functionality, just use a distreference.Canonical directly. Signed-off-by: Miloslav Trmač <[email protected]>

API transition: Drop reference.XCanonical

cc0f48a

Instead use distreference.Canonical directly. Signed-off-by: Miloslav Trmač <[email protected]>

Implementation transition: Use distreference.IsNameOnly in XIsNameOnly

b55e51d

The two functions are line-by-line identical now. Signed-off-by: Miloslav Trmač <[email protected]>

API transition: Drop reference.XIsNameOnly

c2d8ac2

Instead call distreference.IsNameOnly directly. Signed-off-by: Miloslav Trmač <[email protected]>

Implementation transition: Use distreference.TagNameOnly in XWithDefa…

b330f50

…ultTag Signed-off-by: Miloslav Trmač <[email protected]>

API transition: Drop reference.XWithDefaultTag

05f35b9

Instead use distreference.TagNameOnly directly. Signed-off-by: Miloslav Trmač <[email protected]>

Implementation collapse: Drop namedRef

7c038c2

(This could have been done a few commits ago.) Now that namedRef merely wraps a distreference.Named, adding no functionality, just use a distreference.Named directly. Signed-off-by: Miloslav Trmač <[email protected]>

API transition: Drop reference.XWithName

32d33ac

Instead use distreference.ParseNormalizedNamedDirectly (and update obsolete comments). Signed-off-by: Miloslav Trmač <[email protected]>

Drop unused constants from docker/reference.

e928b40

Signed-off-by: Miloslav Trmač <[email protected]>

Implementation transition: Base XParseIDOrReference on distreference.…

2f1d5df

…ParseAnyReference Signed-off-by: Miloslav Trmač <[email protected]>

API transition: Drop reference.XParseIDOrReference

6e4f15a

Use distreference.ParseAnyReference directly. Signed-off-by: Miloslav Trmač <[email protected]>

Finally, get rid of the last remains of c/i/docker/reference.

74c24ed

Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac force-pushed the no-docker-reference branch from 8a51683 to ecdd233 Compare February 7, 2017 14:27

mtrmac merged commit 4f14180 into containers:master Feb 7, 2017

mtrmac deleted the no-docker-reference branch February 7, 2017 14:48

mtrmac mentioned this pull request Feb 7, 2017

docker: add docker-archive transport #148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Drop c/i/docker/reference (again) #221

Drop c/i/docker/reference (again) #221

Uh oh!

mtrmac commented Jan 20, 2017

Uh oh!

runcom commented Jan 20, 2017 •

edited

Loading

Uh oh!

mtrmac commented Jan 20, 2017

Uh oh!

mtrmac commented Jan 31, 2017 •

edited

Loading

Uh oh!

runcom commented Feb 1, 2017

Uh oh!

mtrmac commented Feb 1, 2017

Uh oh!

runcom commented Feb 1, 2017

Uh oh!

mtrmac commented Feb 1, 2017

Uh oh!

runcom commented Feb 7, 2017

Uh oh!

mtrmac commented Feb 7, 2017

Uh oh!

runcom commented Feb 7, 2017 •

edited

Loading

Uh oh!

mtrmac commented Feb 7, 2017 •

edited by runcom

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Drop c/i/docker/reference (again) #221

Drop c/i/docker/reference (again) #221

Uh oh!

Conversation

mtrmac commented Jan 20, 2017

Uh oh!

runcom commented Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac commented Jan 20, 2017

Uh oh!

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

Uh oh!

Uh oh!

runcom commented Feb 1, 2017

Uh oh!

mtrmac commented Feb 1, 2017

Uh oh!

runcom commented Feb 1, 2017

Uh oh!

mtrmac commented Feb 1, 2017

Uh oh!

runcom commented Feb 7, 2017

Uh oh!

mtrmac commented Feb 7, 2017

Uh oh!

runcom commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac commented Feb 7, 2017 • edited by runcom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

runcom commented Jan 20, 2017 •

edited

Loading

mtrmac commented Jan 31, 2017 •

edited

Loading

runcom commented Feb 7, 2017 •

edited

Loading

mtrmac commented Feb 7, 2017 •

edited by runcom

Loading