refactor splitDockerDomain to include more documentation

[thaJeztah]

• relates to New repo who dis? distribution/reference kubernetes/kubernetes#120316 (comment)
• relates to client: TestImageTagInvalidSourceImageName remove invalid test-case moby/moby#46378

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"); containerd/cri@25fdf72

Let keep it as a further exercise, but add a "TODO" to remind us doing so.

[thaJeztah]

I have some follow-ups, but some may need discussion, so starting with this one

/cc @milosgajdos @Jamstah

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +1.20% 🎉

Comparison is base (a3fb784) 82.51% compared to head (89ee7ec) 83.71%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main       #7      +/-   ##
==========================================
+ Coverage   82.51%   83.71%   +1.20%     
==========================================
  Files           5        5              
  Lines         366      393      +27     
==========================================
+ Hits          302      329      +27     
  Misses         54       54              
  Partials       10       10              

Files Changed	Coverage Δ
normalize.go	85.60% <100.00%> (+3.70%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[milosgajdos]

I only have a question and a random wondering which we can ignore. But otherwise LGTM 🤔

[milosgajdos]

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

[thaJeztah]

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

[milosgajdos]

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

[thaJeztah]

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

[thaJeztah]

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

[Jamstah]

Thanks :)

[liggitt]

We're pulling in an update to kubernetes that includes this change (in https://github.com/kubernetes/kubernetes/pull/126799/files#r1750753594)

Is this a refactor without a behavior change?

[thaJeztah]

@liggitt sorry for the delay 🙈

It's been a while since I worked on this, but I think it was all without change, but I don't think this one made a change in behavior;

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

I see I have some follow-up changes stashed that I never opened a PR for around matching domain-names not being case-insensitive; #7 (comment)

The only one that can be problematic in some cases is;

• remove deprecated SplitHostname #5

That removes a function that was deprecated, and should no longer be used, but there was one oversight on my hand that the current non-beta release of distribution provides an alias for the deprecated reference functions, and one of those uses that function; https://github.com/distribution/distribution/blob/v2.8.3/reference/reference_deprecated.go#L115-L123

So in code-bases that import the github.com/docker/distribution and still use the deprecated reference package, that can cause a failure to compile.

[liggitt]

So in code-bases that import the github.com/docker/distribution and still use the deprecated reference package, that can cause a failure to compile.

Yeah, that's not my concern in https://github.com/kubernetes/kubernetes/pull/126799/files#r1750753594, I was only trying to figure out if code that still compiled would behave differently due to the refactor of splitDockerDomain. Sounds like no :)