shortnames: mechanism to enforce resolving to Docker Hub

[vrothberg]

Podman's Docker-compatible REST API is in need of a mechanism to enforce
resolving to Docker Hub only. Yet there is the desire for the rest of
the stack to continue honoring the system's registries.conf.

We recently added a new field to containers.conf [1] which allows for
opting out from enforcing Docker Hub for Podman's compat API but we
still lack a way of enforcement when resolving short names; which
ultimately is the place to do that.

This change does the necessary plumbing. The compat REST handlers will
set the new field in the types.SystemContext and pass that down to
libimage and buildah.

[1] containers/common@e698b8c

Context: containers/podman/issues/12320
Signed-off-by: Valentin Rothberg [email protected]

[vrothberg]

@mtrmac @rhatdan PTAL

[rhatdan]

LGTM

[mtrmac]

I’m not really a fan.

To the extent short names are a primarily a UI feature, with user-targeted interactions and semantics, it’s user-hostile for those names to have option-dependent semantics.

The way I think about it, the case this is targeting we claim that the users didn’t want to use short names with alias/search semantics, their client software didn’t want to represent users’s wishes over the AF_UNIX socket with alias/search semantics, and the API server understands / is configured to understand the AF_UNIX request to not have alias/search semantics. So it seems natural to me that the API server shouldn’t call the alias/search semantics code, then.

We don’t have docker:// call shortnames.Resolve(ResolveShortNamesToDockerHub). Why is the API server client a different case?

(Yeah, to an extent this is just aesthetics; we are going to have the ugly special case somewhere. Still… we don’t want anyone else, ever, to turn this option on, do we?)

The one reason I can see for doing it this way is the ResolveLocally path, where localhost/ is still preferred. I’m not sure that is a good enough reason, though opinions on that can certainly differ — and come to think of it, I’m not even sure the API should prefer localhost/ in that case, then. If, in the API, “short names mean docker.io”, shouldn’t that equally apply to everything, including names of pulled images, outcomes of local builds, local tag canonicalization, and everything else? And shouldn’t the localhost/ lookup then be unnecessary at least, or quite possibly actively unwanted?

[vrothberg]

I’m not really a fan.

Me neither but I do not see any other long-term maintainable solution.

The code is scattered across at least three different repositories: c/podman, c/common, c/buildah. I want to avoid having the logic (and the decision of running in compat-mode) scattered across all these repositories. c/image, and the shortnames package in particular, is the heart of short-names resolution so it seemed natural to me to move it. Setting one bool in the SystemsContext (which is used across the stack) in the compat API and leave the rest of the stack untouched seems very attractive to me.

Note that am still convinced users (or distributions) should edit registries.conf if they desire the specific behavior.

(Yeah, to an extent this is just aesthetics; we are going to have the ugly special case somewhere. Still… we don’t want anyone else, ever, to turn this option on, do we?)

The one reason I can see for doing it this way is the ResolveLocally path, where localhost/ is still preferred. I’m not sure that is a good enough reason, though opinions on that can certainly differ — and come to think of it, I’m not even sure the API should prefer localhost/ in that case, then. If, in the API, “short names mean docker.io”, shouldn’t that equally apply to everything, including names of pulled images, outcomes of local builds, local tag canonicalization, and everything else? And shouldn’t the localhost/ lookup then be unnecessary at least, or quite possibly actively unwanted?

Yes, good point. localhost/ should be excluded as well and the tag/untag behavior in the compat API and the commit logic in Buildah had to be updated as well.

[mtrmac]

Me neither but I do not see any other long-term maintainable solution.

I’d expect this to be a boolean handled by all the API handlers. Sure, there are quite a few, but it’s routine work that only needs to be done once because the API is frozen, isn’t it?

In particular, a “build” request which contains a plain-text Dockerfile with FROM fedora is not a case of the “in the API, a short name might mean docker.io” situation, because there isn’t a Docker Engine API client passing explicitly fully-qualified user input through FamiliarName to turn it into a short name against user wishes.

If that case should also interpret short names as docker.io (which I assume/guess is the primary hard case motivating putting this into SystemContext), it’s completely unclear to me why that should be so.

[mtrmac]

and the tag/untag behavior in the compat API and the commit logic in Buildah had to be updated as well.

Using pkg/shortnames on those paths really doesn’t make sense to me in the general case, because Resolve returns a list of candidates. It would work better with this proposed new flag than without!

[vrothberg]

I’d expect this to be a boolean handled by all the API handlers. Sure, there are quite a few, but it’s routine work that only needs to be done once because the API is frozen, isn’t it?

That's what I hoped for yes, but as you suspect, build is the hairy part.

In particular, a “build” request which contains a plain-text Dockerfile with FROM fedora is not a case of the “in the API, a short name might mean docker.io” situation, because there isn’t a Docker Engine API client passing explicitly fully-qualified user input through FamiliarName to turn it into a short name against user wishes.

I am not sure I understand you correctly. Do you mean that building should continue using search registries and only the parts of the REST API with images in parameters would be updated?

[mtrmac]

Do you mean that building should continue using search registries and only the parts of the REST API with images in parameters would be updated?

Yes. I see a reason for interpreting the API-native reference values (where e.g. docker/docker/client accepts reference.Named values and calls FamiliarName) to imply docker.io; I don’t see one for the plain text Dockerfiles. (Users can always set U-S-R to docker.io to get that behavior, sure.)

It’s admittedly all very messy whatever we do.

[vrothberg]

I would feel very uncomfortable with build resolving differently than the rest. Note sure if it could be security relevant but at the very least it would be inconsistent and probably annoying. I wanted to avoid doing this (or a similar) PR here in c/image but I do not see an alternative without introducing new fields/parameters to large parts Podman's call stack.

Especially, I don't want future generations having to worry too much about that stuff. "Flip this knob in types.SystemContext" seems very attractive. The only alternative I see is trying to wire it into libimage but that would require updating many APIs that I really want to avoid.

It’s admittedly all very messy whatever we do.

I know 😭

[mtrmac]

So per an off-GitHub conversation, this is considered to the easiest way to centralize the logic (because the “always docker.io” semantics will be configurable — and maybe c/common/libimage will read the new SystemContext boolean as a switch for its own code that doesn’t involve pkg/shortnames), and we are probably doing it…

[vrothberg]

Thanks for reviewing, @mtrmac. Here's an updated version.

[vrothberg]

Converting to draft for now. I need to spend some more time in libimage

[mtrmac]

(I know another approach is being investigated; just getting this off my plate.)

[vrothberg]

(I know another approach is being investigated; just getting this off my plate.)

Thanks! I am almost done with the REST API and am currently working on the last tests which are on builds.

[vrothberg]

Ready from my side

[mtrmac]

LGTM. So is the approach that is going to be used? A clean split between the build code and the rest? Some kind of ad-hoc mix?

[vrothberg]

LGTM. So is the approach that is going to be used?

Yes, I opened containers/podman#12435.

A clean split between the build code and the rest? Some kind of ad-hoc mix?

I have a hard time considering it to be "clean" since I'd still prefer a change in registries.conf but "ad-hoc mix" sounds accurate to me. At the time of writing, containers/podman#12435 normalizes all parameters it can and leaves the rest to c/buildah via an updated types.SystemContext.

[vrothberg]

@mtrmac good to merge?

[mtrmac]

A clean split between the build code and the rest? Some kind of ad-hoc mix?

I have a hard time considering it to be "clean"

Very fair :)

[mtrmac]

*sigh*