containers / image

Work with containers' images
Apache License 2.0
845 stars 366 forks source link

Allow empty OCI configs for artifacts #2279

Closed saschagrunert closed 4 months ago

saschagrunert commented 5 months ago

@containers/image-maintainers I'm wondering if we can allow the mediaType application/vnd.oci.empty.v1+json when parsing the OCI configs. This would make pushing OCI artifacts a bit simpler for users of ORAS, which uses this as default configuration.

For example in a manifest would look like this:

{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.manifest.v1+json",
  "artifactType": "application/vnd.unknown.artifact.v1",
  "config": {
    "mediaType": "application/vnd.oci.empty.v1+json",
    "digest": "sha256:44136fa355b3678a1146ad16f7e8649e94fb4fc21fe77e8310c060f61caaff8a",
    "size": 2,
    "data": "e30="
  },
  "layers": [ … ],

To allow this media type, we would have to mainly change the checks preventing it:

diff --git a/internal/image/oci.go b/internal/image/oci.go
index df0e8e41..dc4df568 100644
--- a/internal/image/oci.go
+++ b/internal/image/oci.go
@@ -87,7 +87,7 @@ func (m *manifestOCI1) ConfigBlob(ctx context.Context) ([]byte, error) {
 // layers in the resulting configuration isn't guaranteed to be returned to due how
 // old image manifests work (docker v2s1 especially).
 func (m *manifestOCI1) OCIConfig(ctx context.Context) (*imgspecv1.Image, error) {
-   if m.m.Config.MediaType != imgspecv1.MediaTypeImageConfig {
+   if m.m.Config.MediaType != imgspecv1.MediaTypeImageConfig && m.m.Config.MediaType != imgspecv1.MediaTypeEmptyJSON {
        return nil, internalManifest.NewNonImageArtifactError(&m.m.Manifest)
    }

diff --git a/manifest/oci.go b/manifest/oci.go
index 6d5acb45..4770f769 100644
--- a/manifest/oci.go
+++ b/manifest/oci.go
@@ -198,7 +198,7 @@ func (m *OCI1) Serialize() ([]byte, error) {

 // Inspect returns various information for (skopeo inspect) parsed from the manifest and configuration.
 func (m *OCI1) Inspect(configGetter func(types.BlobInfo) ([]byte, error)) (*types.ImageInspectInfo, error) {
-   if m.Config.MediaType != imgspecv1.MediaTypeImageConfig {
+   if m.Config.MediaType != imgspecv1.MediaTypeImageConfig && m.Config.MediaType != imgspecv1.MediaTypeEmptyJSON {
        // We could return at least the layers, but that’s already available in a better format via types.Image.LayerInfos.
        // Most software calling this without human intervention is going to expect the values to be realistic and relevant,
        // and is probably better served by failing; we can always re-visit that later if we fail now, but

Is there a concern that this would affect all images and is this something we can generally consider?

Refers to https://github.com/cri-o/cri-o/issues/7580, https://github.com/cri-o/cri-o/issues/7492

mtrmac commented 5 months ago

No, parsing the empty artifact as an image type just doesn’t make sense. What is failing? That should probably not be calling Inspect or OCIConfig in the first place.

(I’m not at all saying that the current code is correct or sufficient, it’s just that without any more context this seems to be not the right approach.)

saschagrunert commented 5 months ago

So workflow wise, a current artifact push using ORAS:

> oras push quay.io/saschagrunert/seccomp:v1 seccomp.tar
Uploading bd931af693a3 seccomp.tar
Uploaded  bd931af693a3 seccomp.tar
Pushed [registry] quay.io/saschagrunert/seccomp:v1
Digest: sha256:854fc40f989f928f44152269ddc845945bc1ce81c360e5a5645e85e92cc5d602

Results in the following manifest:

> skopeo inspect --raw docker://quay.io/saschagrunert/seccomp:v1 | jq .
{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.manifest.v1+json",
  "artifactType": "application/vnd.unknown.artifact.v1",
  "config": {
    "mediaType": "application/vnd.oci.empty.v1+json",
    "digest": "sha256:44136fa355b3678a1146ad16f7e8649e94fb4fc21fe77e8310c060f61caaff8a",
    "size": 2,
    "data": "e30="
  },
  "layers": [
    {
      "mediaType": "application/vnd.oci.image.layer.v1.tar",
      "digest": "sha256:bd931af693a3a815420d7bafc751f87609bb47520a8f3c2ecc5fbfc8404578ee",
      "size": 10240,
      "annotations": {
        "org.opencontainers.image.title": "seccomp.tar"
      }
    }
  ],
  "annotations": {
    "org.opencontainers.image.created": "2024-02-06T08:11:44Z"
  }
}

Then an example image pull using libimage like this:

package main

import (
    "context"

    "github.com/containers/common/libimage"
    "github.com/containers/common/pkg/config"
    "github.com/containers/storage"
)

func main() {
    storeOpts, err := storage.DefaultStoreOptions()
    if err != nil {
        panic(err)
    }
    store, err := storage.GetStore(storeOpts)
    if err != nil {
        panic(err)
    }
    runtime, err := libimage.RuntimeFromStore(store, &libimage.RuntimeOptions{})
    if err != nil {
        panic(err)
    }
    _, err = runtime.Pull(context.Background(), "quay.io/saschagrunert/seccomp:v1", config.PullPolicyAlways, &libimage.PullOptions{})
    if err != nil {
        panic(err)
    }
}

Will result in:

panic: parsing image configuration: unsupported image-specific operation on artifact with type "application/vnd.unknown.artifact.v1"

goroutine 1 [running]:
main.main()
        /home/sascha/downloads/main.go:26 +0x185

With the call stack:

goroutine 1 [running]:
runtime/debug.Stack()
        /nix/store/cw9dqybf9w6wp7827h23pb3ym8gs8h47-go-1.21.6/share/go/src/runtime/debug/stack.go:24 +0x5e
runtime/debug.PrintStack()
        /nix/store/cw9dqybf9w6wp7827h23pb3ym8gs8h47-go-1.21.6/share/go/src/runtime/debug/stack.go:16 +0x13
github.com/containers/image/v5/internal/manifest.NonImageArtifactError.Error({{0xc00027eed0?, 0xc000036ca0?}})
        /home/sascha/downloads/vendor/github.com/containers/image/v5/internal/manifest/errors.go:56 +0x45
fmt.(*pp).handleMethods(0xc000564270, 0xa0c00?)
        /nix/store/cw9dqybf9w6wp7827h23pb3ym8gs8h47-go-1.21.6/share/go/src/fmt/print.go:667 +0x1b2
fmt.(*pp).printArg(0xc000564270, {0x109a580?, 0xc000036ca0}, 0x77)
        /nix/store/cw9dqybf9w6wp7827h23pb3ym8gs8h47-go-1.21.6/share/go/src/fmt/print.go:756 +0x630
fmt.(*pp).doPrintf(0xc000564270, {0x11c04df, 0x1f}, {0xc0003e8208?, 0x1, 0x1})
        /nix/store/cw9dqybf9w6wp7827h23pb3ym8gs8h47-go-1.21.6/share/go/src/fmt/print.go:1077 +0x39e
fmt.Errorf({0x11c04df, 0x1f}, {0xc0003e8208?, 0x1, 0x1})
        /nix/store/cw9dqybf9w6wp7827h23pb3ym8gs8h47-go-1.21.6/share/go/src/fmt/errors.go:25 +0x8b
github.com/containers/image/v5/copy.checkImageDestinationForCurrentRuntime({0x1345b58, 0x1b91a00}, 0xc000139680, {0x134d3e0, 0xc000389c40}, {0x134d468?, 0xc00020f8c0?})
        /home/sascha/downloads/vendor/github.com/containers/image/v5/copy/single.go:300 +0xd3
github.com/containers/image/v5/copy.(*copier).copySingleImage(0xc00030d720, {0x1345b58, 0x1b91a00}, 0xc0000c7140, 0x0, {0x20?, 0x0?, 0x0?})
        /home/sascha/downloads/vendor/github.com/containers/image/v5/copy/single.go:110 +0x50b
github.com/containers/image/v5/copy.Image({0x1345b58, 0x1b91a00}, 0xc00019da28, {0x134b1c8, 0xc0001d7620}, {0x134b228?, 0xc00019da40?}, 0x0?)
        /home/sascha/downloads/vendor/github.com/containers/image/v5/copy/copy.go:291 +0x15a5
github.com/containers/common/libimage.(*copier).copy.func3()
        /home/sascha/downloads/vendor/github.com/containers/common/libimage/copier.go:456 +0x172
github.com/containers/common/pkg/retry.IfNecessary({0x1345b58, 0x1b91a00}, 0xc0003e9278, 0xc000290bc8)
        /home/sascha/downloads/vendor/github.com/containers/common/pkg/retry/retry.go:41 +0x57
github.com/containers/common/libimage.(*copier).copy(0xc000290a80, {0x1345b58?, 0x1b91a00}, {0x134b228, 0xc00019da40}, {0x134b1c8, 0xc0001d7620})
        /home/sascha/downloads/vendor/github.com/containers/common/libimage/copier.go:462 +0x858
github.com/containers/common/libimage.(*Runtime).copySingleImageFromRegistry(0xc000221c00, {0x1345b58, 0x1b91a00}, {0xc0001169c0, 0x20}, 0x0, 0xc0003e9d50)
        /home/sascha/downloads/vendor/github.com/containers/common/libimage/pull.go:651 +0x13ab
github.com/containers/common/libimage.(*Runtime).copyFromRegistry(0xc000221c00, {0x1345b58, 0x1b91a00}, {0x134b228, 0xc00019d608}, {0xc0001169c0?, 0x0?}, 0x0, 0xc00063fd50)
        /home/sascha/downloads/vendor/github.com/containers/common/libimage/pull.go:394 +0x208
github.com/containers/common/libimage.(*Runtime).Pull(0xc000221c00, {0x1345b58, 0x1b91a00}, {0x11c1f45, 0x20}, 0x0, 0xc00063fd50)
mtrmac commented 5 months ago

Yeah, but pulling that artifact into c/storage (interpreting the tarball as a container-image-like tar with whiteouts, and creating a a graph driver backing layer for the item) seems to me to be not moving us any closer to usefully consuming the artifact.

Wouldn’t it be typically more useful to just refer to the included blob, and stream it as an io.Reader, possibly consuming just the one included file without any on-disk storage of the tar stream at all? Or, I don’t know, if this artifact were associated with an image, turning the blob (either as is, or just the included seccomp.json file) into data recorded using storage.Image.SetImageBigData along with the “primary” image, so that it is named/reference/deleted as a single unit, not as an unrelated user-visible image object?

Now, c/image probably should provide more utilities for that kind of usage (at the very least, making it easy to validate the blob digest, and very hard to forget), and the API for that doesn’t exist.

sftim commented 4 months ago

How about getting a dedicated media type (echoing https://github.com/cri-o/cri-o/issues/7580#issuecomment-1954721735)? There's no actual registration cost, although I appreciate it means filling out a form and asking.

mtrmac commented 4 months ago

Why not, but that’s not really related to the current PR’s (proposed) goal of allowing non-images to use specifically the image MIME type. If the thing in question were using any other MIME type, it would already be treated by as an artifact by c/image, regardless of the specific MIME type value.

sftim commented 4 months ago

What I mean is: drop this PR, change CRI-O to expect a [new kind of] OCI object containing a seccomp profile, register a media type for that new kind of OCI object.

The code in this repo need not change at all to achieve the overall outcome, which is to be able to store seccomp profiles in an OCI-compatible registry (and then specify them using URLs, in the Docker sense of URL).

mtrmac commented 4 months ago

Yeah, I’d like that.

@saschagrunert where are we on the design? My reading of https://github.com/cri-o/cri-o/pull/7719 and https://github.com/kubernetes/website/pull/45121 is that CRI-O is currently forging ahead with pulling the artifact into c/storage (and bypassing the need for this PR by using image-like MIME types and config contents).

Are you saying that that’s the committed approach? Is the conversation of how non-image artifacts are going to be stored/pulled locally still ongoing perhaps?

saschagrunert commented 4 months ago

@saschagrunert where are we on the design? My reading of cri-o/cri-o#7719 and kubernetes/website#45121 is that CRI-O is currently forging ahead with pulling the artifact into c/storage (and bypassing the need for this PR by using image-like MIME types and config contents).

Yes the blog post on k/website represents the current implementation state in CRI-O. Having an easier way to produce those artifacts would be great. I assume having a dedicated config media type makes it more explicit, but not necessary easier to use. I'm still in favor of that and we may close this issue as already suggested.

Are you saying that that’s the committed approach? Is the conversation of how non-image artifacts are going to be stored/pulled locally still ongoing perhaps?

We can still move into that direction if that's simpler, I assume it would not change anything from the consuming perspective.

saschagrunert commented 4 months ago

Proposing the change in https://github.com/containers/image/pull/2306