Fix login/logout for docker.io/user

[saschagrunert]

reference.ParseNamed() fails on docker.io/user because it expects
docker.io/library/user. We handle this special case now by changing
the domain prefix to always include /library/.

Refers to containers/buildah#3500

[vrothberg]

I wonder if c/image wouldn't be a better home for it. Preserving this logic seems crucial but if it's scattered across projects, we may run into regressions down the road.

Code LGTM. @mtrmac PTAL

[rhatdan]

/approve
I want to wait for @mtrmac opinion.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

NAK, AFAICS this changes semantics: Everywhere else, example.com/vendor would match example.com/vendor/product, but with this, docker.io/vendor would not match docker.io/vendor/product (but would match docker.io/library/vendor).

(This should be clearly visible in a unit test; this PR should add some for docker.io, however it is resolved.)

---

The fact that reference.Named can only with difficulty represent docker.io/vendor is a problem for auth.go specifically (and not most of other code, but also not exclusively to auth.io) because it uses GetCredentialsForRef to read namespaced values (GetCredentials is currently explicitly unable to do that).

[Still, to be clear, GetCredentialsForRef is what almost all callers should use to read credentials, and that API is, I think, OK for that purpose.]

I’m not 100% sure what to do here. One way would be to just work harder to create a reference.Named for docker.io/vendor[/*], probably reference.Parse + some more type checks, another would be to make GetCredentials support namespaced keys; that would be consistent with the rest of that package, and mostly EDIT reduce the need for auth.go to deal with reference.Named at all (other than a check for domain match, and tag/digest, which can be done with ParseNormalizedNamed fine); I lean towards the latter because it makes auth.go simpler and more consistent.

[mtrmac]

I wonder if c/image wouldn't be a better home for it. Preserving this logic seems crucial but if it's scattered across projects, we may run into regressions down the road.

Not for the “add a /library/ in the middle” change; we need something else.

(We have more and more instances of “repo prefix” strings, so sharing a single strict parser for that, and not using reference.Named for any related purpose, would be interesting. (Introducing a separate Go data type would be nice, but it would also require introducing new API so that we don’t break existing callers). OTOH it does not that much help in this case where we want to recognize and reject tags/digests, and with only some of the “repo prefix” code supporting domain wildcards, it’s not 100% clear that defining a single prefix syntax with a new type (effectively freezing it, unlike arbitrary strings where we can usually just relax validation) is a good idea.

But for the purposes of this PR, I don’t think it matters that much: we would still want to provide a GetCredentialsForRef, and we would still need to express “a docker.io/vendor namespace” somehow.

[mtrmac]

I lean towards the latter because it makes auth.go simpler and more consistent.

Also, while it’s possible to create a reference.Named for docker.io/vendor, as the original report with ParseNamed shows, such values can’t be, in general, round-tripped from strings and back to reference.Named, so it just seems safer not to use reference.Named for the namespaced keys.

[saschagrunert]

Folks I'd like to let you know that I'm on vacation the next three weeks until Oct 4. So either we wait until I'm back or someone else has to take it over if it's critical.

[mtrmac]

nother would be to make GetCredentials support namespaced keys

containers/image#1373 will allow that.

[mtrmac]

containers/image#1373 will allow that.

See #763 .

[saschagrunert]

containers/image#1373 will allow that.

See #763 .

Thank you! I'll close this one for now to avoid confusion.