-
Notifications
You must be signed in to change notification settings - Fork 377
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
Don't expect image sources to convert images for us #331
Comments
I agree, removing the parameter from the API does seem like the right thing to do. One thing to note, though, is that containers/image doesn’t implement conversion from manifest lists to individual manifests; in fact @runcom WDYT? |
Confusing - if a manifest list is returned by the source the code will actually go ahead and select the v2 manifest since image.FromUnparsedImage() calls manifestInstanceFromBlob which has that code, but then, yes, it looks like it fail the copy operation. The current behavior will be a mish-mash, because sometimes the manifest list type will be in the accept header and sometimes not. (Mostly not, but for the directory and oci backends, it will be listed.) So sometimes conversion happens on the server, and sometimes the copy fails. If we switch to passing a fixed set of accept headers to the docker registry - then we have a choice of including the manifest list type or not. |
(apologies for the horrible run-on-sentence; as you have figured out, neutral destinations like |
We might just fix other destinations to refuse list altogether, even if we will, at some point, figure out manifest lists. |
…meter The requestedManifestMIMETypes parameter was added because a destination might not support all manifest MIME types that the the source supports, but the original use case now passes all manifest types and lets containers/image convert internally. In generally, internal conversion may be more comprehensive, is more predictable, and avoids bypassing internal checks. Fixes: containers#331
…meter The requestedManifestMIMETypes parameter was added because a destination might not support all manifest MIME types that the the source supports, but the original use case now passes all manifest types and lets containers/image convert internally. In generally, internal conversion may be more comprehensive, is more predictable, and avoids bypassing internal checks. Fixes: containers#331
…meter The requestedManifestMIMETypes parameter was added because a destination might not support all manifest MIME types that the the source supports, but the original use case now passes all manifest types and lets containers/image convert internally. In generally, internal conversion may be more comprehensive, is more predictable, and avoids bypassing internal checks. Fixes: containers#331
…meter The requestedManifestMIMETypes parameter was added because a destination might not support all manifest MIME types that the the source supports, but the original use case now passes all manifest types and lets containers/image convert internally. In generally, internal conversion may be more comprehensive, is more predictable, and avoids bypassing internal checks. Fixes: containers#331 Signed-off-by: Owen W. Taylor <otaylor@fishsoup.net>
…meter The requestedManifestMIMETypes parameter was added because a destination might not support all manifest MIME types that the the source supports, but the original use case now passes all manifest types and lets containers/image convert internally. In generally, internal conversion may be more comprehensive, is more predictable, and avoids bypassing internal checks. Fixes: containers#331 Signed-off-by: Owen W. Taylor <otaylor@fishsoup.net>
The point of the requestedManifestMIMETypes parameter to ImageReference.NewImageSource() is to allow the destination to affect the type of image requested from the source. It looks like the inspiration for that was that the atomic registry at one point didn't accept v2 manifests - #26 (references containers/skopeo#102 (comment))
However, that usage was effectively removed in #266, with with the idea that the if necessary, conversion happens inside containers/image when pushing to the registry.
The only reason to modify the list of mime types requested from the source, is if want the source to do the conversion rather than convert inside containers/image. But what happens inside the source is very unpredictable - it depends on the details of the source implementation, and those details may not be what we want. Examples:
Accept: application/vnd.oci.image.config.v1+json
to the docker registry, and what is stored is a schema2 manifest, it doesn't return the schema2 manifest unmodified, it converts it to schema1. And schema2 => schema1 => oci is going to be lower fidelity than schema2 => oci. (schema1 => oci is not even supported in containers/image at the moment though it wouldn't be hard to add as schema1 => schema2 => oci.)Accept: application/vnd.docker.distribution.manifest.v1+json
, the docker daemon happily drops the signatures, and the code in copy.Image() which is supposed to prevent conversions that drop signatures is defeated.Concretely, for docker/distribution, the set of conversions that happen currently are quite limited:
I believe that these conversions will all happen properly with containers/image and the second is a third example of why it's better to do the conversion inside containers/image - the platform should be determined by the arch of the local system, not a default platform for the registry!
If the above argument makes, sense, I can do a PR for this - either removing requestedManifestMIMETypes parameter from ImageReference.NewImageSource() - which I guess is an API change. Or just making it do nothing locally in docker_image_src.go.
The text was updated successfully, but these errors were encountered: