From 89ee7ec63e17bd74e6766ecac7d8ff2bd4b8dd8a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 1 Sep 2023 15:39:49 +0200 Subject: [PATCH] refactor splitDockerDomain to include more documentation The splitDockerDomain attempts to determine whether the given name contains a domain, or should be used as a "remote-name". The logic used in doing so is based on (legacy) conventions, which have not always been properly documented. The logic used in this function was also optimized for "brevity", but not easy to ready, due to the combination of multiple boolean conditions combined on a single line, and some "double negatives". More documentation may still be needed, but let's start with documenting the logic used in this function; - Use `strings.Cut()` instead of `strings.IndexRune()`, which allows us to use descriptive variable names, and prevents use of the magic `-1` value. - Split the conditions into a switch, so that each of them can be documented separately. While this makes the code more verbose (and introduces some duplication), it should not impact performance, as only one condition would ever be met (performance may even be better, as the old code combined multiple conditions with `&&`). - Introduce a fast-path for single-element ("familiar") names. These names can be canonicalized early, without doing further handling. While working on the code, I also discovered an existing bug (or omission) where the code would not handle bare _domain names_. Ironically, the TestParseDockerRef test has a test-case name "hostname only", but which does not cover that case. THat test-case was transferred from containerd/cri, and does not describe this scenario (possibly was left as a "further exercise"); https://github.com/containerd/cri/commit/25fdf726923b607d0155550c5e85a911bb04bad1 Let keep it as a further exercise, but add a "TODO" to remind us doing so. Signed-off-by: Sebastiaan van Stijn --- normalize.go | 59 ++++++++++++++++++++++++++++++++++++----------- normalize_test.go | 32 +++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 14 deletions(-) diff --git a/normalize.go b/normalize.go index a30229d..f412831 100644 --- a/normalize.go +++ b/normalize.go @@ -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. + return defaultDomain, officialRepoPrefix + name + } + + switch { + case maybeDomain == localhost: + // localhost is a reserved namespace and always considered a domain. + domain, remoteName = maybeDomain, maybeRemoteName + 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: + // 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 diff --git a/normalize_test.go b/normalize_test.go index fbaa630..129aaf1 100644 --- a/normalize_test.go +++ b/normalize_test.go @@ -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", @@ -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",