I think this is a typo in the test, because imageName has local-only prefix already:
https://github.com/apple/container/blob/main/Tests/CLITests/Subcommands/Build/CLIBuilderTest.swift#L63

This is a little tricky - yeah adding this check would fix the issue we are seeing, but we dont feel its right that an image we build without a domain in its tag, automatically gets assigned one.

I think we should change the behavior so that the reference we get from the build container does not have the domain prepended if its a locally built image

And I think that happens here :
https://github.com/apple/container-builder-shim/blob/main/pkg/build/frontend.go#L144

Tagging @katiewasnothere for some more input

What do you mean saying that an image automatically gets domain assigned? It's a fully qualified reference and all normalized names are with domain, so ParseAnyReference returns it in this format.

I don't have a strong opinion here, so it you decide to get rid of the domain here, I can make corresponding changes.

For example if the Dockerfile being built is

FROM local-image:test
...

The expectation is that somewhere in the build process, the container-image-service will be asked about the existence of local-image:test by the builder.

But because the builder is parsing that base image reference using reference. ParseAnyReference (which has the assumption that an image should get the default domain of docker.io) - the container-image-service ends up getting a request for docker.io/local-image:test

And this diff makes it such that we intercept the request b/w the build container and image service to work around that assumption

To add to the above - we still want the nice UX of something like this still working

FROM alpine:latest
...

And to support that container does some normalization of its own - at a high level it does

• Check if "alpine:latest" exists as an image in the daemon
• If not normalize it using the configured default registry (see container registry default set)
• Check if an image with normalized reference exists in the daemon (will be of the form <domain>/alpine:latest)
• If that fails - pull the image from the registry

Yeah, I got the idea. And I agree that the decision about the default registry should be made on the container-image-service side, not on container-build-shim.
I made some changes in the PR, that should both support the flow you described and fix the initial bug. Next I'm going to make a PR to the shim repo to change the reference parsing and get rid of the default docker domain in the request. Does it sound like a good plan for you?

Okay after lengthy investigation, I think there's not really a way here for us to avoid buildkit adding the docker.io/library prefix to an image reference other than us modifying the dockerfile that we send to builder, which seems like a bad plan. Given that, I think our only option is to match the reference to either the container.name or containerization.name as you've done here. I don't think we need the builder side changes though.

@adityaramani is out for a few days, but I'd like to wait for his input here.

I think we call Dockerfile2LLB after we resolve images: https://github.com/apple/container-builder-shim/blob/35406d303bc165a090df812d75971d90d4e24f9f/pkg/build/frontend.go#L75-L80

But I agree that it becomes too complex and fragile. We can skip container-builder-shim changes then and keep this PR.

The image resolving we do in the builder shim itself is only used by buildkit for named contexts (see here and here)

Do you think its possible to make a variation of this change in BuildImageResolver.swift when we try to fetch the image?

Something like this?

        let img = try await {
            if ref.startsWith("docker.io/library") {
               trimmedRef = ref["docker.io/library".count + 1 : ]
                if i = ClientImage.get(trimmedRef) {
		return i
                 }
    
           guard let img = try? await ClientImage.pull(reference: ref, platform: platform) else {
                return try await ClientImage.fetch(reference: ref, platform: platform)
            }
            return img              
}
        }()

The idea behind this is to restrict this change only to affect the build flow. Do ya'll think this would work?

I think I'd rather we use the containerization.name as the source of truth. It's possible that someone has the images alpine (local) and docker.io/library/alpine (pulled). If we always stripped out docker.io/library for the image on a build, we'd always return the local alpine image.