containerd handle localhost and ports in registry hosts

[chrisplo]

localhost is a valid server reference but does not have a '.'
server reference may also have a port and be a locally
resolvable hostname

[kzantow]

Hi, and thanks for the contribution @chrisplo! Apologies, but I had a little bit of a challenging time sorting out the logic here. Is the primary change you're looking for here that using localhost or host-without-dots:<port> is allowed?

Looking at this PR, I think parts[0] == "127.0.0.1" won't ever be a valid case, since the dot will cause it to fall into the previous case? I also don't really think we care about the len == 0 case not returning docker/library/

Could this be simplified to 3 cases?

switch {
case len(parts) == 1:
  return fmt.Sprintf("docker.io/library/%s", imageName)
case strings.Contains(parts[0], ".") || strings.Contains(parts[0], ":") || parts[0] == "localhost":
  return imageName
default:
  return fmt.Sprintf("docker.io/%s", imageName)
}

[chrisplo]

I'm able to tag image with ctr with simple foo:latest, len==0 appears to be valid
you're right on the 127.0.0.1, don't need to call it out, will remove
Do you know where a spec is for the naming that containerd follows? oci has an spec for the image but not the name as far as I can tell. docker registry v2 has a loose spec, and suggests that daemons may have tighter rules.

[chrisplo]

i split a couple cases with same return values to keep the column width not too wide (readability?) can adjust for style as needed

[chrisplo]

as a side note, I'm using this in context of syft, and replacing master with my PR fixes the issue I saw for using localhost: tagged images in containerd

could not determine source: an error occurred attempting to resolve 'localhost:32000/:0.1.0-7749': containerd: unable to parse registry reference="docker.io/localhost:32000/:0.1.0-7749": could not parse reference: docker.io/localhost:32000/:0.1.0-7749

[kzantow]

But there's no case where len(parts) == 0. Even strings.Split("", "/") results in []string{""}, which is len() == 1.

[kzantow]

How about let's try to make this even more clear what the intent is by adding a helper function called something like isHostname.

Then we'd have something like:

switch {
case len(parts) == 1:
  return fmt.Sprintf("docker.io/library/%s", imageName)
case isHostname(parts[0]):
  return imageName
default:
  return fmt.Sprintf("docker.io/%s", imageName)
}

and

func isHostname(s string) bool {
  return s == "localhost" || strings.Contains(s, ".") || strings.Contains(s, ":")
}

[chrisplo]

ah, right . . (len != 0)

made changes and switched from case+return to return fast style, I had thought there was room to grow more cases over time but seems not be needed. I changed the name as well as it's an altered return vs a boolean status.

should the daemon be checked for image existance before prefixing the image name with hope it can be pulled?

func (p *daemonImageProvider) pullImageIfMissing(ctx context.Context, client *containerd.Client) (string, *platforms.Platform, error) {
	p.imageStr = ensureRegistryHostPrefix(p.imageStr)

	// try to get the image first before pulling
	resolvedImage, resolvedPlatform, err := p.resolveImage(ctx, client, p.imageStr)

i could open this as a separate PR if you like, it would probably also fix my issue in a different way

[chrisplo]

noticed we don't need to test for len > 1 as we know can't be zero and we test early for len == 1

[chrisplo]

hi @kzantow, is there more I can do for this PR?