Bug 1873636

After a bit more research pkg/credentialprovider/secrets.MakeDockerKeyring prioritizes Pod.Spec.ImagePullSecrets in the order given, and then uses the global pull credentials in /var/lib/kubernetes; in each case it only cares about credential entries matching the specific registry, and a secret that does not mention the registry is ignored.

This clearly makes sense for the Pod.Spec.ImagePullSecrets entries (if the first pull secret did not contain an entry for a specific registry, and that were interpreted as “use no credentials”, the any further entries in ImagePullSecrets would be always ignored).

It’s not _inevitable_ that if 1) ImagePullSecrets exist and 2) don’t contain anything about the registry in question, that the global pull credentials are used; it might be possible to only use the global pull credentials if Pod.Spec.ImagePullSecrets is empty, but IMHO treating the entries of Pod.Spec.ImagePullSecrets and /var/lib/kubernetes the same is most consistent (e.g. adding a pull secret that does not contain a credential for the credential in question is _always_ a no-op).

> ... and a secret that does not mention the registry is ignored.

This seems like a poor match for mirrored images using ImageContentSourcePolicies [1] which configure registries.conf mirrors [2], because CRI-O side mirrors are opaque to the kubelet, aren't they?  What if you skip over a secret with only a b.example.com token because it's an a.example.com pullspec, but b.example.com is a configured mirror for a.example.com and the pull would have worked?

[1]: https://docs.openshift.com/container-platform/4.5/updating/updating-restricted-network-cluster.html#images-configuration-registry-mirror_updating-restricted-network-cluster
[2]: https://github.com/containers/image/blob/v2.0.0/docs/containers-registries.conf.5.md#remapping-and-mirroring-registries

(In reply to W. Trevor King from comment #2)
> What if
> you skip over a secret with only a b.example.com token because it's an
> a.example.com pullspec, but b.example.com is a configured mirror for
> a.example.com and the pull would have worked?

That’s a fairly orthogonal problem. The CRI only allows passing _one_ set of credentials, so the Kubelet can never pass credentials for both [ab].example.com; indeed, that’s why the mirrors need to be inserted into the global pull secret at all (at the time and given the time constraints, the effort to extend CRI instead was considered impractical).

I agree that if it were possible for the CRI to pass multiple sets of credentials, it would make sense to provide _all_ of the Pod.Spec secrets _and_ the Kubelet pull secret, for every pull (or rather, I guess, the first matching credentials for every registry, instead of all matches). But 1) it isn’t currently possible, and 2) that would be an _even stronger_ case for not stopping the search when encountering a pull secret that does not mention the registry in the pod pullSpec.

> The CRI only allows passing _one_ set of credentials...

Ah, I had the impression that the kubelet would walk through the pull-secret fallback chain and provide CRI-O with a different secret if there was a pull failure with the previous secret.  If this is really a single shot at picking the best pull secret in the absence of mirror-config information... good luck to us :).

You’re right, I’m afraid have misread the code. The kubelet does try, one at a time, all pull secrets _for the matching registry_. But it doesn’t try pulling without credentials if at least one set of credentials is available, and it _only_ passes credentials for the matching registry, not any mirrors (the Kubelet currently doesn’t know what the mirrors are).

(It seems ill-specified whether it is possible at all to pass credentials for a different registry via the CRI, technically there is a ServerAddress field that could contain the registry name, but the CRI does not really document it. I suspect that’s a Docker file parser implementation artifact that similarly just leaks through the Docker API types, and IIRC is ignored by the actual code.

Anyway the Kubelet does not pass a server name in the field, CRI-O does not use the server name field, and IIRC neither does Docker. And still, it could only work if there were no credentials for the primary and there were a single mirror, because we are still limited to a single set of credentials.)

I'm not convinced this can be solved in the scope of a bug.  There is something systemic about this and it isn't apparent to be how this can be resolved.  We've had issues like this in the past were we basically decided we painted ourselves into a corner and there wasn't a way to resolve it without breaking existing behavior.

https://bugzilla.redhat.com/show_bug.cgi?id=1561989

Miloslav helped on that one too.  It was a total mess.

Tempted to close as WONTFIX, and maybe open a Jira story to look into this.  Any objections?

+1 from me to moving this issue into some long-term RFE-ish effort.  My impression is that this will need CRI improvements, and so probably significant upstream effort, and also that it's not a regression.

I’d be fine with WONTFIX; if the global pull secret is
1) necessary at all, and
2) invalid (does not allow pulling images),
then I would expect the cluster to encounter difficulties running either way; focusing on the special case where a specific pod tries to _override_ a pull secret with “nothing” seems to overemphasize how the cluster _did_ fail over how it _would fail anyway_.

If you want to track a long-term RFE to better support mirrors in the CRI, sure, it would be a fair amount of effort and time, but it is architecturally the right thing to do.

As for tracking a RFE to allow overriding the global pull secret with “be anonymous”, that seems like a rare corner case (it does not really make sense to _intentionally design_ an authorization system so that unauthenticated users have more access than authenticated). It might make more sense for the registry to be taught that expired/invalid tokens should have at least the “anonymous” level of access.

The needinfo request[s] on this closed bug have been removed as they have been unresolved for 1000 days