Integrate docker reference changes by dmcgowan · Pull Request #1778 · distribution/distribution

dmcgowan · 2016-06-09T18:34:34Z

Allows having other parsers which are capable of unambiguously keeping hostname and name separated in a Reference type.

This is intended to address comments made for #1777 and lay the foundation for upstreaming changes from docker/docker. Leaving this as WIP so some of those changes can be included.

codecov-io · 2016-06-09T19:24:31Z

Current coverage is 51.41% (diff: 72.36%)

Merging #1778 into master will decrease coverage by 9.82%

@@             master      #1778   diff @@
==========================================
  Files           129        129           
  Lines         11416      11531    +115   
  Methods           0          0           
  Messages          0          0           
  Branches          0          0           
==========================================
- Hits           6991       5929   -1062   
- Misses         3534       4851   +1317   
+ Partials        891        751    -140

Powered by Codecov. Last update 534b155...429c75f

stevvooe · 2016-06-09T21:44:21Z

@dmcgowan This is looking okay. Are there are few demonstrative test cases? For example, does the case from #1777 now pass?

dmcgowan · 2016-06-09T22:17:26Z

@stevvooe adding the downstream functionality now. The test case from #1777 must always fail unless we change the grammar. However by adding the downstream parsers it should be supported and able to be parsed in an unambiguous way.

dmcgowan · 2016-06-09T22:44:34Z

Added commit with functionality needed to replace github.com/docker/docker/reference. Another pass on the interface and naming should get us close to what we want.

Also considering adding an interface which has methods for Hostname() and Path() (or whatever the correct name for "Path" is). This would allow some simplification of methods to split out the values and make the functionality less reliant on type asserting an unexported type.

dmcgowan · 2016-06-09T22:47:52Z

reference/normalize.go

+// Normalizer removes ambiguity in values to ensure
+// parsing is done correctly and without partial valus.
+type Normalizer interface {
+	Canonicalize(string) (string, error)


The use of the term canonical here and CanonicalName is confusing with the idea of the Canonical reference. Not sure what would be a more appropriate word.

Also would it make more sense for this interface just to take in a string and do the Named parsing. Originally I wanted to leave it as a package function so it could possible access unexported values without trying the docker implementation to the package anymore. However that is not currently the case and the implementation is already accessing unexported types.

dmcgowan · 2016-06-13T22:01:33Z

Ping @stevvooe @aaronlehmann for feedback

dmcgowan · 2016-06-14T21:52:04Z

Discussed more with Stephen and came to a few decisions...

Get rid of the normalizer interface, only one implementation of normalize
Normalize takes any name and produces the longest form ie redis -> docker.io/library/redis
Introduce a FamiliarName function which always produces a shortened version for UI, called familiar because this is intended to be the name that is familiar to users, not always shortened.
Rename Hostname to Authority as the first component of the Name may be a DNS name, it does not necessarily represent a host or the hostname of a registry. Rather the first component of a name is intended to represent the authority for that name for trust, authentication, and registry host resolution.
Add support for fully resolving string references which may be an ID identifier. When just an identifier is found with no name, a digested type with no name will be returned. Also have a version which could take in a shortened identifier and a digest.Set and returned a reference.
Refer to RemoteName as Path. A Name is then always made up of an Authority and a Path.
Add all existing helper functions used from docker/docker and docker/engine-api to guarantee that the reference package does not get forked.

I am going to start implementing these changes, we can have a discussion of their individual merit while I get these changes implemented. Ping @aaronlehmann @RichardScothern

aaronlehmann · 2016-06-14T22:42:32Z

I like most of these changes.

Rename Hostname to Authority as the first component of the Name may be a DNS name, it does not necessarily represent a host or the hostname of a registry. Rather the first component of a name is intended to represent the authority for that name for trust, authentication, and registry host resolution.

I'm not sure I like the name Authority, but I'm not sure what would be a better choice.

Add support for fully resolving string references which may be an ID identifier. When just an identifier is found with no name, a digested type with no name will be returned

How will the grammar distinguish between IDs and names that look like IDs?

dmcgowan · 2016-06-14T23:09:16Z

How will the grammar distinguish between IDs and names that look like IDs?

I am not sure we can have a top level grammar to represent the rules for ID parsing as well as normalization. I would prefer to keep the grammar as is along with the existing Parse function and just add functions which can parsing IDs and normalization.

stevvooe · 2016-06-15T05:06:00Z

@aaronlehmann Authority works here because it is non-committal to the actual role and differentiates the term from namespace, which is too overloaded.

How will the grammar distinguish between IDs and names that look like IDs?

The role of the grammar to be to identify best effort structure. If a name may be an ID, its really up to the caller to run that throw the ID filter before treating it as a name. The main general drawback to this approach is that a secure process requires knowing the entire ID set.

dmcgowan · 2016-06-15T05:16:16Z

I would also consider Domain as an alternative to Authority, has a similar meaning but slightly more specific to its use.

stevvooe · 2016-06-15T05:46:55Z

Domain is fine, but it still has that DNS flavor...

dmcgowan · 2016-06-15T23:05:36Z

PTAL, this now has additions to the grammar so I want everyone looking at it. Function naming may still still does not feel right (such as NormalizedName is similar to ParseNamed). Also I added helpers from engine-api but may get rid of the SplitName helper, it really does not add anything useful nor save many lines of code. Plus I only see the original version used twice in engine-api.

aaronlehmann · 2016-06-15T23:16:07Z

reference/helpers.go

+
+// SplitName is a helper function which parses the
+// input as a name and returns name plus tag or digest
+func SplitName(s string) (string, string, error) {


This isn't used internally, and I don't think we want to encourage outside callers to conflate digests and tags.

I agree, I got it from here https://github.com/docker/engine-api/blob/master/types/reference/image_reference.go#L10 but even its use looks trivial. It would be better to be more explicit about parsing into a Named. It is possible for engine-api to use the normalized versions now as well.

dmcgowan · 2016-06-15T23:36:04Z

Changed WithDefaultTag to EnsureTagged and dropped last commit with changes from engine-api. I agree those changes are too specific to the api and we don't want them widely used.

aaronlehmann · 2016-06-15T23:50:13Z

LGTM

stevvooe · 2016-06-16T01:04:26Z

LGTM

aaronlehmann · 2016-06-19T21:07:09Z

I suggest we hold off on merging this until we go through the exercise of porting Docker Engine to use it, to make sure we got the details right.

dmcgowan · 2016-06-21T04:32:44Z

@aaronlehmann that is fair, I will link a branch of Docker with the integration soon, no rush on merging this

dmcgowan · 2016-06-21T19:31:26Z

Made a PR in my fork for reviewing the changes dmcgowan/docker#27.

Noticeable changes:

"convenience" methods adding to RepositoryInfo, just call corresponding reference function with the Named value
"WithDefaultTag" has different behavior than EnsureTagged. EnsureTagged always adds a tag if not tagged while WithDefaultTag would not add the tag is the type was canonical. I prefer the more explicit change to only call EnsureTagged on a named only reference, but if that is not the consensus we can update the behavior.
ParseAnyReference causes slightly different behavior than the function it was replacing since it does explicitly return a Named type. While the return type is always Named or Digested I am not sure that logic translates well, perhaps another helper function would be better here.

dmcgowan · 2016-06-27T23:53:01Z

Added NormalizedNamed interface to allow referencing without ambiguity. Note that all underlying Named types implement the interface since even non-normalized types may be familiarized. Was thinking of also adding an IsNormalized function which just needs to ensure the domain is not empty. The caller still has the potential to call ParseNamed and type assert to NormalizedNamed but I don't see that as a being a problematic pattern since ParseNamed should only be called on fully normalized values. The docker update will only use ParseNormalizedNamed.

aaronlehmann · 2016-06-28T05:38:02Z

I like the addition of the NormalizedNamed intrerface. I think it's the right direction because without it, there's no way to distinguish a normalized reference by type. For example in the first attempt at porting the Docker Engine codebase to this unified version of the reference library, I believe push/pull functions expected normalized references, but since the types were reference.Named, it would be easy to accidentally pass another type of named reference.

Allows having other parsers which are capable of unambiguously keeping domain and path separated in a Reference type. Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)

Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)

Add normalization functions and Docker specific domain splitting to reference package. Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)

Add interface for for a normalized name and corresponding parser for that type. New normalized versions of all interfaces are not added since all type information is preserved on calls to Familiar. Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)

dmcgowan · 2017-01-09T20:38:11Z

Rebased

Added 2 helper functions to make easy to integrate into Docker

aaronlehmann · 2017-01-09T20:50:58Z

Whoah, nice

stevvooe · 2017-01-09T21:40:05Z

LGTM

aaronlehmann · 2017-01-09T21:55:39Z

reference/normalize.go

+	legacyDefaultDomain = "index.docker.io"
+	defaultDomain       = "docker.io"
+	defaultRepoPrefix   = "library/"
+	defaultTag          = "latest"


Any thoughts on exporting these? If we don't, downstream code will likely need to redefine them.

I would prefer not to, ideally they are only used through functions and hidden from the callers. The places the default tag are used today should rather be using EnsureTagged. Other use is for logging, but the "default" tag is irrelevant to the logs, the useful information is that a reference was altered along with before/after.

Only other types used are here https://github.com/docker/docker/blob/master/registry/config.go#L258. That function will likely get replaced to use the parsers in this package to validate.

aaronlehmann · 2017-01-09T23:17:00Z

LGTM

I think it would be a nice simplification to merge Named and NamedRepository. Named doesn't ever appear to be backed by anything that doesn't implement NamedRepository. Formalizing the fact that Named exposes Domain and Path methods would simplify the code a bit, avoiding some type assertions that use these methods if available and otherwise do parsing.

However, this would change the Named interface, and I think it's better not to do that yet. This could be a followup for after the docker and distribution reference packages are merged.

GordonTheTurtle added the status/0-triage label Jun 9, 2016

dmcgowan reviewed Jun 9, 2016
View reviewed changes

RichardScothern modified the milestones: Registry/2.6, Registry/2.5 Jun 13, 2016

dmcgowan changed the title ~~[WIP] Split apart repository reference into hostname and name~~ Split apart repository reference into hostname and name Jun 13, 2016

dmcgowan changed the title ~~Split apart repository reference into hostname and name~~ Integrate docker reference changes Jun 14, 2016

dmcgowan force-pushed the reference-with-split-hostname branch 2 times, most recently from b79674e to 3a2a544 Compare June 15, 2016 23:00

aaronlehmann reviewed Jun 15, 2016
View reviewed changes

dmcgowan force-pushed the reference-with-split-hostname branch from 3a2a544 to dfe593b Compare June 15, 2016 23:34

dmcgowan mentioned this pull request Jun 21, 2016

Upstream reference updates dmcgowan/docker#27

Closed

stevvooe mentioned this pull request Jun 22, 2016

Feature request: separate registry from tag moby/moby#23748

Closed

aaronlehmann mentioned this pull request Aug 16, 2016

Improve "invalid reference" error message #1900

Merged

RichardScothern modified the milestones: Registry/2.6, Registry/2.5 Aug 16, 2016

dmcgowan mentioned this pull request Aug 23, 2016

inconsistent mapping of images names when using fully qualified name moby/moby#25827

Closed

dmcgowan mentioned this pull request Sep 14, 2016

Reference package should not be forked moby/moby#18731

Closed

thaJeztah mentioned this pull request Nov 10, 2016

login and push make inconsistent result moby/moby#28225

Closed

dmcgowan mentioned this pull request Nov 11, 2016

Adding utility functions - IsNameOnly and EnsureTagged #2062

Merged

dmcgowan modified the milestones: Registry/2.7, Registry/2.6 Nov 14, 2016

dmcgowan added in progress and removed status/0-triage labels Nov 14, 2016

dmcgowan added the status/2-code-review label Nov 23, 2016

dmcgowan added 4 commits January 5, 2017 15:41

Split apart repository reference into domain and path

9a43b8f

Allows having other parsers which are capable of unambiguously keeping domain and path separated in a Reference type. Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)

Add identifier grammar

21db8e8

Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)

Move docker reference functionality to reference package

042fe9b

Add normalization functions and Docker specific domain splitting to reference package. Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)

dmcgowan force-pushed the reference-with-split-hostname branch from 6741755 to 429c75f Compare January 6, 2017 00:59

aaronlehmann reviewed Jan 9, 2017

View reviewed changes

aaronlehmann merged commit dbc336e into distribution:master Jan 9, 2017

dmcgowan mentioned this pull request Jan 10, 2017

reference: upstreams additions from forked docker/reference package #1277

Closed

Conversation

dmcgowan commented Jun 9, 2016

Uh oh!

codecov-io commented Jun 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 51.41% (diff: 72.36%)

Uh oh!

stevvooe commented Jun 9, 2016

Uh oh!

dmcgowan commented Jun 9, 2016

Uh oh!

dmcgowan commented Jun 9, 2016

Uh oh!

dmcgowan Jun 9, 2016

Choose a reason for hiding this comment

Uh oh!

dmcgowan commented Jun 13, 2016

Uh oh!

dmcgowan commented Jun 14, 2016

Uh oh!

aaronlehmann commented Jun 14, 2016

Uh oh!

dmcgowan commented Jun 14, 2016

Uh oh!

stevvooe commented Jun 15, 2016

Uh oh!

dmcgowan commented Jun 15, 2016

Uh oh!

stevvooe commented Jun 15, 2016

Uh oh!

dmcgowan commented Jun 15, 2016

Uh oh!

aaronlehmann Jun 15, 2016

Choose a reason for hiding this comment

Uh oh!

dmcgowan Jun 15, 2016

Choose a reason for hiding this comment

Uh oh!

dmcgowan commented Jun 15, 2016

Uh oh!

aaronlehmann commented Jun 15, 2016

Uh oh!

stevvooe commented Jun 16, 2016

Uh oh!

aaronlehmann commented Jun 19, 2016

Uh oh!

dmcgowan commented Jun 21, 2016

Uh oh!

dmcgowan commented Jun 21, 2016

Uh oh!

dmcgowan commented Jun 27, 2016

Uh oh!

aaronlehmann commented Jun 28, 2016

Uh oh!

dmcgowan commented Jan 9, 2017

Uh oh!

aaronlehmann commented Jan 9, 2017

Uh oh!

stevvooe commented Jan 9, 2017

Uh oh!

aaronlehmann Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

dmcgowan Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Jan 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Jun 9, 2016 •

edited

Loading