-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a new reference package abstracting tags and digests #690
Conversation
// repository := hostname ['/' component]* | ||
// hostname := alpha-numeric [separator alpha-numeric]* [port] | ||
// alpha-numeric := /[a-z0-9]+/ | ||
// separator := /[._-]/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevvooe I had to remove :
from separator, hope that's okay.
5180d67
to
02432cc
Compare
if p.err != nil { | ||
return | ||
} | ||
repository, tail = p.parseHostname(s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function doesn't appear that it would handle a single component repository with a numeric tag correctly. The port should only be considered if there are remaining components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, not sure how to deal with this, there seems to be ambiguity there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no tail or the tail starts with @
then don't count the port
) | ||
|
||
var ( | ||
tagRegexp = regexp.MustCompile(`[\w][\w.-]{0,127}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use value from names.go. Please let's not have multiple definitions of this. These definitions should probably be just moved here.
@thaJeztah Let's keep our eye on this one. It impacts the image naming material in the docker/docker docs. Already talked with @tiborvass about updating those when this merges. (See this for some areas to hit: moby/moby#14252) |
Thanks for the ping Mary |
} | ||
} | ||
|
||
func TestUrlRepository(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestURLRepository
There seems to be a need for a type that represents a way of pointing to an image, irrespective of the implementation. This patch defines a Reference interface and provides 3 implementations: - TagReference: when only a tag is provided - DigestReference: when a digest (according to the digest package) is provided, can include optional tag as well Validation of references are purely syntactic. There is also a strong type for tags, analogous to digests, as well as a strong type for Repository from which clients can access the hostname alone, or the repository name without the hostname, or both together via the String() method. For Repository, the files names.go and names_test.go were moved from the v2 package. Signed-off-by: Tibor Vass <tibor@docker.com>
) | ||
|
||
// Repository represents a reference to a Repository. | ||
type Repository struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just do this as a string type. The Hostname
is an interpretation and most repositories don't have this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just have a Hostname
and Repository
method on a string type. Repository
also implements Reference
.
|
||
// RepositoryNameRegexp builds on RepositoryNameComponentRegexp to allow | ||
// multiple path components, separated by a forward slash. | ||
var RepositoryNameRegexp = regexp.MustCompile(`(?:` + RepositoryNameHostnameRegexp.String() + `/` + RepositoryNameComponentRegexp.String() + `/)*` + RepositoryNameComponentRegexp.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These regexps seem to behave very differently from the existing regexps in api/v2. I tried to use this RepositoryNameRegexp in notary and ran into some problems.
It looks like RepositoryNameRegexp is set up to allow multiple "hostname/component" matches instead of multiple components. When I tested the regexp with regexr.com, I found that:
- docker.com/notary does not match
- docker.com/notary/notary does match
- docker.com/notary/notary/notary does not match
The exact regexp I tested was (?:[a-zA-Z0-9]+(?:[._-][a-z0-9]+)*(?::[0-9]+)?/[a-zA-Z0-9]+(?:[._-][a-z0-9]+)*/)*[a-zA-Z0-9]+(?:[._-][a-z0-9]+)*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaronlehmann Do mind making a small PR to add some of these testcases to names_test.go and routes_test.go? We'll add some coverage here in the form a table-based test in reference, but let's make sure we have some strong second order tests to ensure we don't mess this up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #724
…meRegexp This was inspired by problems found with new regexps proposed in PR distribution#690 Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
@tiborvass can you rebase? |
@tiborvass ping :) |
Needs rebase, going to try and push for this to get merged since now we want it again 😉 |
Will try updating tonight, @dmcgowan any help is appreciated :) |
@tiborvass the rebase is pretty nasty because of some test changes, I will help out with it |
@tiborvass any movement there? |
Let's clean up these types for common use cases. To do this, we define a root object called a // Reference is an opaque object reference identifier that may include
// modifiers such as a hostname, name, tag, and digest.
type Reference interface {
// String returns the full reference
String() string
}
// Parse returns the reference contained in s. An error will be returned
// if s is not a valid reference.
func Parse(s string) (Reference, error) { /* ... } To mark the most common image references, we define a reference with a name: type Named interface {
Name() string
} To allow access to a tag or a digest, we define an interface for that, as well: type Tagged interface {
Tag() string
}
type Digested interface {
Digest() digest.Digest
} Most functions operating on a remote would use a type like this: func Pull(ref reference.Named) error This effectively says this function requires a reference with at least a name, but a tag is not required. Making decisions on what is available within that function becomes a simple type switch: log.Println("pulling", ref.Name())
switch ref.(type) {
case Digested:
//.. handle digest ref
case Tagged:
//.. handle tagged
} Note the careful ordering here: we say that we want a To enforce areas we must have a fully qualified name and digest, we define the following: type Canonical interface {
Reference
Named
Digested
} Functions that declare they take or return canonical digests must have all these components as part of the reference. An example would be resolving a digest from a trust system: // Resolve takes an untrusted reference and attempts to identify the
// canonical reference value.
func Resolve(ref reference.Reference) (reference.Canonical, error) |
Carried |
…meRegexp This was inspired by problems found with new regexps proposed in PR #690 Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
…meRegexp This was inspired by problems found with new regexps proposed in PR distribution#690 Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
…meRegexp This was inspired by problems found with new regexps proposed in PR distribution#690 Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
…meRegexp This was inspired by problems found with new regexps proposed in PR distribution#690 Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
There seems to be a need for a type that represents a way of pointing
to an image, irrespective of the implementation.
This patch defines a Reference interface and provides 3 implementations:
provided
(currently not in use). This could be used to verify that for this
reference
ubuntu:14.04@sha256:ffffff...
, the14.04
tag from theubuntu
repository has a digest matching the one passed. This issimply an (intended) example, and no higher-level verification logic is
included in this package.
digests.
Validation of references are purely syntactic.
Signed-off-by: Tibor Vass tibor@docker.com
Ping @dmcgowan @stevvooe @RichardScothern