Skip to content
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

refactor splitDockerDomain to include more documentation #7

Merged
merged 1 commit into from
Sep 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 45 additions & 14 deletions normalize.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,20 +123,51 @@ func ParseDockerRef(ref string) (Named, error) {
// splitDockerDomain splits a repository name to domain and remote-name.
// If no valid domain is found, the default domain is used. Repository name
// needs to be already validated before.
func splitDockerDomain(name string) (domain, remainder string) {
i := strings.IndexRune(name, '/')
if i == -1 || (!strings.ContainsAny(name[:i], ".:") && name[:i] != localhost && strings.ToLower(name[:i]) == name[:i]) {
domain, remainder = defaultDomain, name
} else {
domain, remainder = name[:i], name[i+1:]
}
if domain == legacyDefaultDomain {
domain = defaultDomain
}
if domain == defaultDomain && !strings.ContainsRune(remainder, '/') {
remainder = officialRepoPrefix + remainder
}
return
func splitDockerDomain(name string) (domain, remoteName string) {
maybeDomain, maybeRemoteName, ok := strings.Cut(name, "/")
if !ok {
// Fast-path for single element ("familiar" names), such as "ubuntu"
// or "ubuntu:latest". Familiar names must be handled separately, to
// prevent them from being handled as "hostname:port".
//
// Canonicalize them as "docker.io/library/name[:tag]"

// FIXME(thaJeztah): account for bare "localhost" or "example.com" names, which SHOULD be considered a domain.
Copy link
Member

Choose a reason for hiding this comment

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

is the worry in this FIXME with someone doing a pull on docker pull localhost:foobar?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's not a real worry, other than a correctness fix; currently, docker.io as name would get converted to docker.io/library/docker.io (and similarly, localhost becomes docker.io/library/localhost)

both would probably not be useful without a fix, and rejected by the registry, so it's not a big concern

return defaultDomain, officialRepoPrefix + name
}

switch {
case maybeDomain == localhost:
// localhost is a reserved namespace and always considered a domain.
domain, remoteName = maybeDomain, maybeRemoteName
Jamstah marked this conversation as resolved.
Show resolved Hide resolved
case maybeDomain == legacyDefaultDomain:
// canonicalize the Docker Hub and legacy "Docker Index" domains.
domain, remoteName = defaultDomain, maybeRemoteName
case strings.ContainsAny(maybeDomain, ".:"):
// Likely a domain or IP-address:
//
// - contains a "." (e.g., "example.com" or "127.0.0.1")
// - contains a ":" (e.g., "example:5000", "::1", or "[::1]:5000")
domain, remoteName = maybeDomain, maybeRemoteName
case strings.ToLower(maybeDomain) != maybeDomain:
Copy link
Member

Choose a reason for hiding this comment

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

Feel free to ignore this comment. This sent me down the rabbit hole of RFCs for domain names; if the unicode cars are allowed in domain names (I'm not entirely clear on some of the mini-research I've undertaken 🙃) then we could use strings.EqualFolds here

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, my follow-up would keep the original case, but would compare all domain name cases (localhost and the "index.docker.io" domain with strings.EqualFold

Copy link
Member Author

Choose a reason for hiding this comment

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

My initial intent here was to keep the existing behavior (warts and all), but just make the code more readable/explained, as it was a lot of logic condensed into a single line

// Uppercase namespaces are not allowed, so if the first element
// is not lowercase, we assume it to be a domain-name.
domain, remoteName = maybeDomain, maybeRemoteName
default:
// None of the above: it's not a domain, so use the default, and
// use the name input the remote-name.
domain, remoteName = defaultDomain, name
}

if domain == defaultDomain && !strings.ContainsRune(remoteName, '/') {
// Canonicalize "familiar" names, but only on Docker Hub, not
// on other domains:
//
// "docker.io/ubuntu[:tag]" => "docker.io/library/ubuntu[:tag]"
remoteName = officialRepoPrefix + remoteName
}

return domain, remoteName
}

// familiarizeName returns a shortened version of the name familiar
Expand Down
32 changes: 32 additions & 0 deletions normalize_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ func TestValidateReferenceName(t *testing.T) {
"docker/docker",
"library/debian",
"debian",
"localhost/library/debian",
"localhost/debian",
"LOCALDOMAIN/library/debian",
"LOCALDOMAIN/debian",
"docker.io/docker/docker",
"docker.io/library/debian",
"docker.io/debian",
Expand Down Expand Up @@ -147,6 +151,34 @@ func TestParseRepositoryInfo(t *testing.T) {
}

tests := []tcase{
{
RemoteName: "fooo",
FamiliarName: "localhost/fooo",
FullName: "localhost/fooo",
AmbiguousName: "localhost/fooo",
Domain: "localhost",
},
{
RemoteName: "fooo/bar",
FamiliarName: "localhost/fooo/bar",
FullName: "localhost/fooo/bar",
AmbiguousName: "localhost/fooo/bar",
Domain: "localhost",
},
{
RemoteName: "fooo",
FamiliarName: "LOCALDOMAIN/fooo",
FullName: "LOCALDOMAIN/fooo",
AmbiguousName: "LOCALDOMAIN/fooo",
Domain: "LOCALDOMAIN",
},
{
RemoteName: "fooo/bar",
FamiliarName: "LOCALDOMAIN/fooo/bar",
FullName: "LOCALDOMAIN/fooo/bar",
AmbiguousName: "LOCALDOMAIN/fooo/bar",
Domain: "LOCALDOMAIN",
},
{
RemoteName: "fooo/bar",
FamiliarName: "fooo/bar",
Expand Down