libimage: Return the full registry domain when searching

[jelly]

Searching for images in registry.fedoraproject.org returns
fedoraproject.org as registry in the search results. When relying on the
Index to group results from different registries this is an issue.

Signed-off-by: Jelle van der Waa [email protected]

[jelly]

After some more thought, I am wondering if the string splitting is still required? If this is intentionally done for docker.io, when a user specified index.docker.io as registry then this creates a new issue by returning an invalid index if a user specifies index.docker.io:

    "Index": "docker.io",
    "Name": "index.docker.io/binhex/arch-minidlna",

I'm happy to fix that in this PR, I'm just not 100% sure if that was the intention of the index variable or that I am missing some history.

Additionally if this is intended for index.docker.io, I'd add a comment about it in the PR or maybe we can add a unit test so at least the behaviour stays the same?

[vrothberg]

Thanks for opening the PR!

Let's first check whether it's actually a problem.

I am honestly not sure whether it is. The behavior this PR attempts to change is documented in the man pages (see man podman-search). Though that doesn't necessarily imply that it's correct.

Trying to approach it with a fresh mind, I agree that the "INDEX" which is documented to be the registry is returning the registry as is without stripping off some parts. I'd guess that if there are two registries on the same domain (a.registry.com and b.registry.com) that we'd end up showing it as one.

The search code is really old and has been moved over to libimage from podman. To me it looks like the code is working around using docker/reference. @mtrmac WDYT?

[jelly]

Thanks for opening the PR!

Let's first check whether it's actually a problem.

I am honestly not sure whether it is. The behavior this PR attempts to change is documented in the man pages (see man podman-search). Though that doesn't necessarily imply that it's correct.

Ah, that's something I didn't have in mind, good catch as that would make it inconsistent. Thinking more about it, in my application (cockpit-podman) we could show our filter options with the same logic as it is applied here, showing registry.fedoraproject.org as fedoraproject.org. A user is not likely to care much about the full qualified domain.

Trying to approach it with a fresh mind, I agree that the "INDEX" which is documented to be the registry is returning the registry as is without stripping off some parts. I'd guess that if there are two registries on the same domain (a.registry.com and b.registry.com) that we'd end up showing it as one.

As the Name field still contains the full registry url this might not be an issue. So I'm kind of leaning towards keeping it as is, as I don't like inconsistency between the podman search output and REST output.

[vrothberg]

As the Name field still contains the full registry url this might not be an issue. So I'm kind of leaning towards keeping it as is, as I don't like inconsistency between the podman search output and REST output.

I agree that podman and the REST API should behave the same way. But we can still change it :^) Let's wait for @mtrmac's thoughts. I am very open to change the output as you suggested.

[mtrmac]

The behavior this PR attempts to change is documented in the man pages (see man podman-search).

I’m missing something — which part says that .index is the two-label deep domain instead of all of the registry hostname?

---

The search code is really old and has been moved over to libimage from podman. To me it looks like the code is working around using docker/reference. @mtrmac WDYT?

I don’t know anything about the history; IIRC this is the first time I have seen that. I can trace it back to containers/podman@0d7e6fa , but there isn’t a PR, and I can’t find any record of an earlier conversation.

---

Either way I am very skeptical about the index computation; as a guess, it looks like a half-hearted way to support all of docker.io, index.docker.io, registry-1.docker.io (in a way that is unlikely to actually work with c/image, at least WRT credential lookup), for some reason. (The three-label domains should never be used: they rarely work as expected, don’t use the right credentials, and don’t have the docker.io normalization semantics.)

Ideally it would be nice to run an experiment to confirm that trying any of the three-label hostnames would always fail (I haven’t tried), and then we could discount the code path and replace that check just by registry == "docker.io". If any of the three-label names works, I’m not sure whether we want to break them. Maybe we can, for Podman 4.0.

---

Yes, the other user of index to inject /library/ should conceptually be replaced by reference.ParseNormalizedNamed(host+"/"+repo).Name(). OTOH it’s a change to a user-observable output vs. the current index logic, and using ParseNormalizedNamed will also fail on invalid values, which is semantically correct but I’m not 100% sure it’s safe in the wild.

[mtrmac]

To be clear: In general I support the PR, and we should probably drop the index value entirely and replace it with something correct. I’m not quite sure about the risk of breaking anything.

[vrothberg]

I’m missing something — which part says that .index is the two-label deep domain instead of all of the registry hostname?

The examples at the bottom show/indicate this behavior but it is not explicitly mentioned in the text AFAICS.

@rhatdan @baude what's your take? It seems we're at a good place to change the podman search output. I would even be in favor of dropping the INDEX column altogether. The output in the NAME column always bundles the results by registry, so it seems redundant.

[rhatdan]

I would be fine with dropping it, I would also like to drop the STARS OFFICIAL AUTOMATED
by default since only docker.io gives this data.

NAME DISCRIPTION SHOULD be default.

[vrothberg]

LGTM, thank you!
@mtrmac PTAL

We can do the remainder in Podman.

[rhatdan]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jelly, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mtrmac]

/lgtm

[openshift-ci]

@mtrmac: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[rhatdan]

/lgtm