Add OCI support to manifest subcommand

[jedevc]

Fixes moby/moby#43126.

- What I did

Introduced support for OCI images to the docker manifest command.

- How I did it

https://github.com/distribution/distribution/ already support oci schemas, so all we need to do is connect into it. A few minor changes to the on-disk storage format are also required to correct whitespace issues 😢

This is incredibly not beautiful set of patches, but given docker manifest doesn't see active development, I'm not sure it's worth refactoring too much of the structure of the code.

Ideally, as a follow-up I'd like to add support to merge manifest lists together - this is proving a little tricky though.

- How to verify it

Use docker manifest with an OCI image!

- Description for the changelog

Add support for OCI images to docker manifest

- A picture of a cute animal (not mandatory but encouraged)

cc @thaJeztah @crazy-max

[jedevc]

I've started working on manifest merging: it's a bit trickier, we need some more logic in the store, but it works 🎉

I won't put it into this PR, since it's some more substantial changes, but here's the initial sketch: jedevc/cli@manifest-oci...jedevc:cli:manifest-merging.

[thaJeztah]

Silly question; would JSON-in-JSON (raw data, but as string) work? I realise this data is not primarily intended for users to read, but having something to hold-onto when looking at it could still be useful (instead of having to decode)

[jedevc]

Yeah, so we still have the content from the registry in the file as a sub-object, since deprecating that would probably be way more work 😢

As long as we have that, and Raw just duplicates that content full with indentation, I'm happy to have it be unreadable base64 😄 Especially as the content fed to the hashing algorithm isn't a string, it's a bytes array, keeping in as close format to the original is easier IMO.

No super strong preferences here, though I admit I'm not a fan of JSON-in-JSON 👀

[thaJeztah]

Still some discussion happening on base64 vs JSON-in-JSON 😅

[tianon]

After @neersighted describing a preference for base64 here, I'm convinced -- the data is still available, and the Go struct should mostly match anyways for users. Additionally, jq has a base64 decoder built-in, so it's still very very accessible (~ jq '.Raw | @base64d | fromjson').

[rumpl]

base64 is great 👍

[thaJeztah]

LOL, so there even was a TODO for this (added in 1fd2d66 / #1156)

Looks like there even was a PR that did so, but got closed? 🤔 #1144

[thaJeztah]

LGTM

[neersighted]

LGTM, I actually prefer the base64 encoding as the it makes it more obvious this is an opaque blob.

[thaJeztah]

It just occurred to me; as this is "net new"; should we error in this case (no raw data present) instead of re-constructing the manifest data?

[neersighted]

I think I prefer the symmetry with the manifest v2 -- or at the very least, if we reconstruct for one and not the other, we should make it more obvious why that happens in the code.

[jedevc]

Agreed, I think the symmetry makes sense.

I'm curious though - here I've used 2-spaces as "the default". In hindsight, 3 spaces might make more sense? BuildKit did used to produce 3-space OCI images, so maybe it makes more sense to use the same 3 spaces as for schema2?

[thaJeztah]

I'm curious though - here I've used 2-spaces as "the default". In hindsight, 3 spaces might make more sense? BuildKit did used to produce 3-space OCI images, so maybe it makes more sense to use the same 3 spaces as for schema2?

I guess there's no "good" answer for that one. Quick thoughts;

• In practice, we should never® hit this branch; existing ~/.docker/manifests/.. JSON files would not have OCI manifests, and new files will have the Raw field propagated
• If we want to keep symmetry; we could also consider going the reverse path; for existing files that have the "struct", but don't have the Raw field propagated;
  • add the Raw data (based on the struct), using the "old" formatting (3 spaces)
  • or (worst case); try 3 spaces, and if that doesn't match the digest, try 2 spaces, and otherwise bail out?
  • deprecate this code-path (only use .Raw)

I guess it's fine to leave those for a follow-up.

[thaJeztah]

LGTM, let's bring this one in

[TalWeisler]

Is the change supported in RHEL8?

[thaJeztah]

@TalWeisler it's part of the docker cli v23.0.0 and up, so if you have that package installed, then it should be supported. We don't provide packages for RHEL (other than for s390x architectures), but you should be able to install the CentOS 8 packages (see https://docs.docker.com/engine/install/centos/)

[TalWeisler]

Thanks @thaJeztah.
After looking further, the command docker manifest inspect seems to work fine from v23.0.0, but the API request GET /v2/<name>/manifests/<reference> returns 404 Not Found :

{
    "errors": [
        {
            "code": "MANIFEST_UNKNOWN",
            "message": "OCI index found, but accept header does not support OCI indexes"
        }
    ]
}

• The request include the Accept headers:
  application/vnd.oci.image.manifest.v2+json, application/vnd.oci.image.manifest.v1+json
• Tested on ubuntu2004 and rhel8
• Image: ubuntu:latest

[thaJeztah]

@TalWeisler ah, thanks for that; that looks like a possible bug. What registry are you testing this against? Is that Docker Hub, or another registry?

I can see two (potential) issues here;

1. docker CLI (for docker manifest inspect) not setting all the Accept headers; is possible that not all registries look at that (the OCI distribution spec does not really define Content-Negotiation (or not properly))
2. The error response is a bit odd; for content-negotiation, I'd expect a 415 status to be returned (not a 404); see RFC9110, section 15.5.1

[TalWeisler]

@thaJeztah
https://registry-1.docker.io/v2/library/ubuntu/manifests/latest

Headers:

• Accept: application/vnd.oci.image.manifest.v1+json
• Accept: application/vnd.docker.distribution.manifest.v1+json
• Accept: application/vnd.docker.distribution.manifest.v1+prettyjws
• Accept: application/json
• Accept: application/vnd.oci.image.manifest.v2+json
• Authorization

[tianon]

application/vnd.oci.image.manifest.v2+json isn't a thing - that was probably supposed to be application/vnd.oci.image.index.v1+json? 😅

[thaJeztah]

I did a quick check to see what headers it passes; here's a quick "registry" to debug the requests;

package main

import (
	"fmt"
	"net/http"
)

func main() {
	http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
		fmt.Println(r.Method, r.URL, r.Header)
		w.Write([]byte("{}"))
	})
	_ = http.ListenAndServe(":5001", nil)
}

Now running docker manifest inspect;

docker manifest inspect --insecure localhost:5001/foo/bar

I get these logs:

GET /v2/ map[Accept-Encoding:[gzip] Connection:[close] User-Agent:[Docker-Client/24.0.2 (darwin)]]
GET /v2/foo/bar/manifests/latest map[Accept:[application/vnd.docker.distribution.manifest.list.v2+json application/vnd.oci.image.index.v1+json application/vnd.oci.image.manifest.v1+json application/vnd.docker.distribution.manifest.v2+json] Accept-Encoding:[gzip] Connection:[close] User-Agent:[Docker-Client/24.0.2 (darwin)]]
GET /v2/ map[Accept-Encoding:[gzip] Connection:[close] User-Agent:[Docker-Client/24.0.2 (darwin)]]
GET /v2/foo/bar/manifests/latest map[Accept:[application/vnd.docker.distribution.manifest.v2+json application/vnd.docker.distribution.manifest.list.v2+json application/vnd.oci.image.index.v1+json application/vnd.oci.image.manifest.v1+json] Accept-Encoding:[gzip] Connection:[close] User-Agent:[Docker-Client/24.0.2 (darwin)]]

So (besides the /v2/ endpoint, which is for detecting registry version and authenticatoin), the following "Accept" headers are sent:

• application/vnd.docker.distribution.manifest.list.v2+json ("Docker Image Manifest Version 2, Schema 2" manifest list (pre-dates the OCI manifest lists)
• application/vnd.oci.image.index.v1+json (OCI manifest list)
• application/vnd.oci.image.manifest.v1+json (OCI image manifest)
• application/vnd.docker.distribution.manifest.v2+json ("Docker Image Manifest Version 2, Schema 2" image manifest)

So, I think that looks good.

application/vnd.oci.image.manifest.v2+json isn't a thing - that was probably supposed to be application/vnd.oci.image.index.v1+json? 😅

Yes, that looks odd. I did a quick search, and found a reference to that (invalid) mediatype in 3 locations;

2 in github.com/anchore/harbor-scanner-adapter;

https://github.com/anchore/harbor-scanner-adapter/blob/aafc46f138503405e4419afe758d3b6173a5624e/pkg/adapter/adapter.go#L10
https://github.com/anchore/harbor-scanner-adapter/blob/aafc46f138503405e4419afe758d3b6173a5624e/pkg/http/api/v1/handler.go#L25

	DockerImageMimeType           = "application/vnd.oci.image.manifest.v2+json"

1 in some comment describing that Buildah must produce that mediatype for Docker
https://github.com/smart-edge-open/sriov-fec-operator/blob/45fa1e052bdcf93653cdbb8c5916cbe519f5f273/Makefile#L6-L7

# Container format for podman. Required to build containers with "ManifestType": "application/vnd.oci.image.manifest.v2+json",
export BUILDAH_FORMAT=docker

{
    "errors": [
        {
            "code": "MANIFEST_UNKNOWN",
            "message": "OCI index found, but accept header does not support OCI indexes"
        }
    ]
}

I did find code in the CNCF distribution repository about that error, but don't see it handling wrong media types.
https://github.com/distribution/distribution/blob/87b280718d3898c8c78ddf8f358c58f6e92e3c9e/registry/handlers/manifests.go#L169-L176

I do think it's odd to return a 404 (and not a 415), but that may have been for backward compatibility. It's worth looking though if that's defined somewhere in the spec.
https://github.com/distribution/distribution/blob/87b280718d3898c8c78ddf8f358c58f6e92e3c9e/registry/api/v2/errors.go#L75C1-L82

[TalWeisler]

@thaJeztah Thank you!