Skip to content
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

Allow empty OCI configs for artifacts #2279

Closed
saschagrunert opened this issue Feb 5, 2024 · 9 comments
Closed

Allow empty OCI configs for artifacts #2279

saschagrunert opened this issue Feb 5, 2024 · 9 comments

Comments

@saschagrunert
Copy link
Member

saschagrunert commented Feb 5, 2024

@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 cri-o/cri-o#7580, cri-o/cri-o#7492

@mtrmac
Copy link
Collaborator

mtrmac commented Feb 5, 2024

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
Copy link
Member Author

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
Copy link
Collaborator

mtrmac commented Feb 6, 2024

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
Copy link

sftim commented Feb 20, 2024

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

@mtrmac
Copy link
Collaborator

mtrmac commented Feb 20, 2024

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
Copy link

sftim commented Feb 20, 2024

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
Copy link
Collaborator

mtrmac commented Feb 21, 2024

Yeah, I’d like that.

@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).

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
Copy link
Member Author

saschagrunert commented Feb 21, 2024

@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
Copy link
Member Author

Proposing the change in #2306

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants