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

Fix podman search for docker.io/library images #2133

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

boaz0
Copy link
Contributor

@boaz0 boaz0 commented Oct 5, 2023

This PR attempts to fix this issue containers/podman#20230.

Looking at moby, it looks like they are doing a hacky trick when we add library to the image.
As seen here:

	if index.Official {
		// If pull "library/foo", it's stored locally under "foo"
		remoteName = strings.TrimPrefix(remoteName, "library/")
	}

If the registry is official then we need to trim "library/" from the image name.
Now what registries are set as official? Looking further at the newIndexInfo function:
If the registry name is set in the configuration we will return it.

	// Return any configured index info, first.
	if index, ok := config.IndexConfigs[indexName]; ok {
		return index, nil
	}

Otherwise we return a new IndexInfo which is not official:

	// Construct a non-configured index info.
	return &registry.IndexInfo{
		Name:     indexName,
		Mirrors:  make([]string, 0),
		Secure:   config.isSecureIndex(indexName),
		Official: false,
	}, nil

Okay, now what is left to understand is: what registries are set to official at the configuration.
Getting here where we create a new ServiceConfig which is used in newIndexInfo.
This function creates a ServiceConfig and what I was interested in was how the IndexConfigs is set.
It looks like these two functions mutate the IndexConfigs: loadInsecureRegistries and loadMirrors

In both of them the only official IndexServer is IndexName which is equal to docker.io.

	IndexName = "docker.io"
...

	// Configure public registry since mirrors may have changed.
	config.IndexConfigs = map[string]*registry.IndexInfo{
		IndexName: {
			Name:     IndexName,
			Mirrors:  unique,
			Secure:   true,
			Official: true,
		},
	}
...
			// Assume `host:port` if not CIDR.
			indexConfigs[r] = &registry.IndexInfo{
				Name:     r,
				Mirrors:  make([]string, 0),
				Secure:   false,
				Official: false,
			}
...
	// Configure public registry.
	indexConfigs[IndexName] = &registry.IndexInfo{
		Name:     IndexName,
		Mirrors:  config.Mirrors,
		Secure:   true,
		Official: true,
	}
	config.InsecureRegistryCIDRs = insecureRegistryCIDRs
	config.IndexConfigs = indexConfigs

So all in all - only docker.io is the official registry. Thus, to adjust docker behavior in podman, I trimmed the library/ when registry is docker hostname.

/cc @rhatdan

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks!

I could see an argument that “search is just submitting the term to the remote API and we make no guarantee about what it means, anyway, so making edits can only be counterproductive” — but, on balance, finding images is more convenient than not finding images, and, meh, we already have a special case for docker.io. So, sure, let’s do it.

@@ -363,6 +363,8 @@ func SearchRegistry(ctx context.Context, sys *types.SystemContext, registry, ima
hostname := registry
if registry == dockerHostname {
hostname = dockerV1Hostname
// On docker.io if image is "library/foo", it's stored under "foo"
Copy link
Collaborator

Choose a reason for hiding this comment

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

(I realize this comment is motivated by the Docker one, still…)

  • I don’t know what “stored under” means here (the Docker code says “stored locally”, which is even more confusing). docker.io/library/busybox and docker.io/busybox/busybox would be different names, so it’s can not really be true that the library one is “stored under” the other one — and anyway where an image is stored on a registry an internal registry detail, ~irrelevant to invoking a search.
  • What we care about is that (as of today, Oct 2023) a search term of library/foo does not find the library/foo image on the docker.io servers, which is surprising - and that Docker is modifying the search term client-side this same way, and it seems convenient to do the same thing. So I think the comment should say something like that.

@rhatdan
Copy link
Member

rhatdan commented Oct 9, 2023

/approve
/lgtm

Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
@boaz0
Copy link
Contributor Author

boaz0 commented Oct 10, 2023

@mtrmac updated the comment, hope it's OK with you.

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

That works, thanks!

@mtrmac mtrmac merged commit 06f8d2a into containers:main Oct 10, 2023
9 checks passed
@boaz0 boaz0 deleted the closes_podman_20230 branch October 11, 2023 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants