containers / podman

Podman: A tool for managing OCI containers and pods.
https://podman.io
Apache License 2.0
23.69k stars 2.41k forks source link

[Feature]: digest support for OCI transport #17308

Open mwopfner opened 1 year ago

mwopfner commented 1 year ago

Feature request description

Using digest in the oci transport currently does not work (podman pull oci:/tmp/local/alpine@sha256:3d426b0bfc361d6e8303f51459f17782b219dece42a1c7fe463b6014b189c86d) because the parsing only handles the :tag notation. From the documentation (https://github.com/containers/image/blob/main/docs/containers-transports.5.md#ocipathreference) I'm not sure if this is desired behavior or a bug. I looked into the problem and found that it is not much needed to support it (see patch below). I'm not sure if this is the right place to request the feature, because all changes have to be made in vendor libs.

Suggest potential solution

I created a path oci_digest_support.txt and tested it on my system:

diff --git i/vendor/github.com/containers/common/libimage/pull.go w/vendor/github.com/containers/common/libimage/pull.go
index 51712bb3b..70703e1d4 100644
--- i/vendor/github.com/containers/common/libimage/pull.go
+++ w/vendor/github.com/containers/common/libimage/pull.go
@@ -227,7 +227,11 @@ func (r *Runtime) copyFromDefault(ctx context.Context, ref types.ImageReference,
        storageName = imageName

    case ociTransport.Transport.Name():
-       split := strings.SplitN(ref.StringWithinTransport(), ":", 2)
+       split := strings.SplitN(ref.StringWithinTransport(), "@", 2)
+
+       if len(split) == 1 {
+           split = strings.SplitN(ref.StringWithinTransport(), ":", 2)
+       }
        storageName = toLocalImageName(split[0])
        imageName = storageName

diff --git i/vendor/github.com/containers/image/v5/oci/internal/oci_util.go w/vendor/github.com/containers/image/v5/oci/internal/oci_util.go
index 148bc12fa..6849459f5 100644
--- i/vendor/github.com/containers/image/v5/oci/internal/oci_util.go
+++ w/vendor/github.com/containers/image/v5/oci/internal/oci_util.go
@@ -58,7 +58,14 @@ func splitPathAndImageWindows(reference string) (string, string) {
 }

 func splitPathAndImageNonWindows(reference string) (string, string) {
-   sep := strings.SplitN(reference, ":", 2)
+
+   sep := strings.SplitN(reference, "@", 2)
+
+   if len(sep) == 1 {
+       sep = strings.SplitN(reference, ":", 2)
+   }
+
+   // sep := strings.SplitN(reference, ":", 2)
    path := sep[0]

    var image string
diff --git i/vendor/github.com/containers/image/v5/oci/layout/oci_transport.go w/vendor/github.com/containers/image/v5/oci/layout/oci_transport.go
index be22bed6d..527cadbb8 100644
--- i/vendor/github.com/containers/image/v5/oci/layout/oci_transport.go
+++ w/vendor/github.com/containers/image/v5/oci/layout/oci_transport.go
@@ -103,7 +103,15 @@ func (ref ociReference) Transport() types.ImageTransport {
 // e.g. default attribute values omitted by the user may be filled in in the return value, or vice versa.
 // WARNING: Do not use the return value
[oci_digest_support.txt](https://github.com/containers/podman/files/10555452/oci_digest_support.txt)
 in the UI to describe an image, it does not contain the Transport().Name() prefix.
 func (ref ociReference) StringWithinTransport() string {
-   return fmt.Sprintf("%s:%s", ref.dir, ref.image)
+
+   // if we use a digest for the image
+   _, err := digest.Parse(ref.image)
+   if err == nil {
+       return fmt.Sprintf("%s@%s", ref.dir, ref.image)
+   } else {
+       // if we use a normal tag for the image
+       return fmt.Sprintf("%s:%s", ref.dir, ref.image)
+   }
 }

 // DockerReference returns a Docker reference associated with this reference
@@ -194,13 +202,26 @@ func (ref ociReference) getManifestDescriptor() (imgspecv1.Descriptor, error) {
            if md.MediaType != imgspecv1.MediaTypeImageManifest && md.MediaType != imgspecv1.MediaTypeImageIndex {
                continue
            }
-           refName, ok := md.Annotations[imgspecv1.AnnotationRefName]
-           if !ok {
-               continue
-           }
-           if refName == ref.image {
-               d = &md
-               break
+
+           // if we use a digest for the image
+           digest, err := digest.Parse(ref.image)
+           if err == nil {
+               if md.Digest == digest {
+                   d = &md
+                   break
+               } else {
+                   continue
+               }
+           } else {
+               // if we use a tag
+               refName, ok := md.Annotations[imgspecv1.AnnotationRefName]
+               if !ok {
+                   continue
+               }
+               if refName == ref.image {
+                   d = &md
+                   break
+               }
            }
        }
    }

Have you considered any alternatives?

I considered using :tag but i don't have any tags. I only have digests.

Additional context

The version section of podman info

version:
  APIVersion: 4.3.1
  Built: 1668026638
  BuiltTime: Wed Nov  9 20:43:58 2022
  GitCommit: 814b7b003cc630bf6ab188274706c383f9fb9915-dirty
  GoVersion: go1.17.13
  Os: linux
  OsArch: linux/amd64
  Version: 4.3.1
vrothberg commented 1 year ago

Thanks for reaching out!

@mtrmac WDYT?

mtrmac commented 1 year ago

(FWIW, there is the https://github.com/containers/image/pull/1381 / https://github.com/containers/image/pull/1677 which adds path:@index. That might be an alternative to a digest reference in some situations, but certainly not a 100% replacement. We should get that finished eventually…)

Sure, that feature seems desirable to provide.

Some specific concerns to deal with:

So fully implementing this would be a somewhat larger effort than the diff above, but it’s a good idea and something that should happen.

mwopfner commented 1 year ago

@mtrmac: Thanks for your feedback. I was pretty sure the patch is not a proper solution :wink: (especially the splitting of the reference into its parts). Here I also got a bit lost with the naming.

If I can help implementing this feature in any way please let me know.

mtrmac commented 1 year ago

The “reference” naming is an unfortunate consequence of several layers of indirection/abstraction. ociReference is an instance of a “c/image image reference”, and it refers to any single image (text form oci:…). That has up to three parts:

… and the last part is, by the OCI spec, described as “name of the reference”, let’s call that “OCI reference”. Hence the man page names that part reference, meaning OCI reference; OTOH the man page does not use the “reference” word to refer to ”c/image image reference”; it usually says “image name”.

So, the org.opencontainers.image.ref.name syntax applies to the third part of the image reference (the field ociReference.image)


We can’t use oci:path@digest because right now the @digest part (or at least the prefix up to the : in @sha256:) is interpreted as _path. E.g. oci:/foo@sha256:abc… currently refers to file /foo@sha256, with the …ref.name annotation value abc….

The path refers to a local filesystem, so quay.io restrictions on repository names are not relevant.

Sure, @ in path names is somewhat unusual, but we are already in enough trouble :) for making it impossible to use : in the path part.


WRT the way to implement this, most of the work will need to happen in the https://github.com/containers/image repository ; we can discuss both the detailed syntax and implementation there.

Similarly, the containers/common part would be a PR in that repository; and both will flow into Podman.

I have, at least, filed https://github.com/containers/image/issues/1828 , so that this is tracked in the relevant repo.

mwopfner commented 1 year ago

@mtrmac Thanks for openning https://github.com/containers/image/issues/1828. I will add the proposal details there.

Regarding the use of oci:path@digest

github-actions[bot] commented 1 year ago

A friendly reminder that this issue had no activity for 30 days.

rhatdan commented 1 year ago

@mtrmac I think this is waiting for a response from you?

mtrmac commented 1 year ago

@rhatdan This will happen in c/image, via the tracked RFE, when/if it is implemented there.

github-actions[bot] commented 1 year ago

A friendly reminder that this issue had no activity for 30 days.