`podman manifest inspect` showing unexpected values for mixed-schema image structures

containers / podman

Podman: A tool for managing OCI containers and pods.

https://podman.io

Apache License 2.0

23.71k stars 2.41k forks source link

`podman manifest inspect` showing unexpected values for mixed-schema image structures #22196

Open codingben opened 7 months ago

codingben commented 7 months ago

Issue Description

We have pushed OCI-based image index and manifests. Podman returns wrong results about media type of image index.

Steps to reproduce the issue

Build OCI image index that will contain OCI image manifests, and push it to any container registry.

╰─$ podman pull quay.io/boukhano/cirros:6.1            
Trying to pull quay.io/boukhano/cirros:6.1...
Error: copying system image from manifest list: parsing image configuration: invalid mixed OCI image with Docker v2s2 config ("application/vnd.docker.container.image.v1+json")

Describe the results you received

╰─$ podman manifest inspect quay.io/boukhano/cirros:6.1                   
{
    "schemaVersion": 2,
    "mediaType": "application/vnd.docker.distribution.manifest.list.v2+json",
    "manifests": [
        {
            "mediaType": "application/vnd.oci.image.manifest.v1+json",
            "size": 418,
            "digest": "sha256:3a37dc3fd4e8460225ef5040308838ae344327c4901cffa653f3e157aae6c4f7",
            "platform": {
                "architecture": "amd64",
                "os": "linux"
            }
        },
        {
            "mediaType": "application/vnd.oci.image.manifest.v1+json",
            "size": 418,
            "digest": "sha256:cfefc66dad4addd779ff91774c1592f7a8cc87d3a905ea86bbcd9441045fe143",
            "platform": {
                "architecture": "arm64",
                "os": "linux"
            }
        }
    ]
}

Describe the results you expected

╰─$ docker manifest inspect quay.io/boukhano/cirros:6.1                                         1 ↵
{
   "schemaVersion": 2,
   "mediaType": "application/vnd.oci.image.index.v1+json",
   "manifests": [
      {
         "mediaType": "application/vnd.oci.image.manifest.v1+json",
         "size": 418,
         "digest": "sha256:3a37dc3fd4e8460225ef5040308838ae344327c4901cffa653f3e157aae6c4f7",
         "platform": {
            "architecture": "amd64",
            "os": "linux"
         }
      },
      {
         "mediaType": "application/vnd.oci.image.manifest.v1+json",
         "size": 418,
         "digest": "sha256:cfefc66dad4addd779ff91774c1592f7a8cc87d3a905ea86bbcd9441045fe143",
         "platform": {
            "architecture": "arm64",
            "os": "linux"
         }
      }
   ]
}

podman info output

If you are unable to run podman info for any reason, please provide the podman version, operating system and its version and the architecture you are running.

Podman in a container

Privileged Or Rootless

None

Upstream Latest Release

Yes

Additional environment details

N/A

Additional information

We have pushed OCI image index and manifests:

╰─$ curl -H "Accept: application/vnd.oci.image.index.v1+json" "https://quay.io/v2/boukhano/cirros/manifests/6.1"   

{"schemaVersion":2,"mediaType":"application/vnd.oci.image.index.v1+json","manifests":[{"mediaType":"application/vnd.oci.image.manifest.v1+json","size":418,"digest":"sha256:3a37dc3fd4e8460225ef5040308838ae344327c4901cffa653f3e157aae6c4f7","platform":{"architecture":"amd64","os":"linux"}},{"mediaType":"application/vnd.oci.image.manifest.v1+json","size":418,"digest":"sha256:cfefc66dad4addd779ff91774c1592f7a8cc87d3a905ea86bbcd9441045fe143","platform":{"architecture":"arm64","os":"linux"}}]}

codingben commented 7 months ago

@nalind Can you please take a look? Thanks!

mheon commented 7 months ago

@mtrmac PTAL

codingben commented 7 months ago

Thanks for looking on it. @nalind debugged and saw that layer/config still using Docker and not OCI, and that's why Podman doesn't work:

╰─$ skopeo inspect --raw docker://quay.io/boukhano/cirros@sha256:e96e9fe082ec1c9c056144f58b52de10314adb79353f35298caa2cd25c79d83c | jq
{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.manifest.v1+json",
  "config": {
    "mediaType": "application/vnd.docker.container.image.v1+json",
    "size": 329,
    "digest": "sha256:7760637f74788fa570057d4c2e7b5c21d996863744d6049807c218f01154fa74"
  },
  "layers": [
    {
      "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
      "size": 20787697,
      "digest": "sha256:97565d0bbd9530b3349cabff269322e8686a4f3add243e72814ba51630e49be9"
    }
  ]
}

While image index and image manifests are based on OCI schemas.

nalind commented 7 months ago

Out of band, we have an image with an OCI media type, but which is using Docker media types to describe its config blob and layers:

{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.manifest.v1+json",
  "config": {
    "mediaType": "application/vnd.docker.container.image.v1+json",
    "size": 329,
    "digest": "sha256:7760637f74788fa570057d4c2e7b5c21d996863744d6049807c218f01154fa74"
  },
  "layers": [
    {
      "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
      "size": 20787697,
      "digest": "sha256:97565d0bbd9530b3349cabff269322e8686a4f3add243e72814ba51630e49be9"
    }
  ]
}

being generated by https://github.com/kubevirt/containerdisks/blob/main/pkg/build/build.go with changes from https://github.com/kubevirt/containerdisks/compare/40fc44a45797c5e02bfdf72d1f161fc0bb155254..c39ddf6c0af1b76817541c0cdae8e744003b252d applied.

It looks like passing tarball.WithMediaType() as the second argument to the tarball.LayerFromOpener() call would change the layer's media type and running img through mutate.ConfigMediaType() would change its config blob's type.

But that leaves the question of whether or not we should be balking at such an image anyway. The spec seems to advise that images that are concerned with portability should be using OCI types for these things, and that seems easy enough to fix here, but the spec notes that implementations mustn't error out when encountering unknown types, and it looks to me like we're doing that here (for certain values of "unknown", anyway).

nalind commented 7 months ago

being generated by https://github.com/kubevirt/containerdisks/blob/main/pkg/build/build.go with changes from https://github.com/kubevirt/containerdisks/compare/40fc44a45797c5e02bfdf72d1f161fc0bb155254..c39ddf6c0af1b76817541c0cdae8e744003b252d applied.

... or reverted, as it may be.

mtrmac commented 7 months ago

But that leaves the question of whether or not we should be balking at such an image anyway. The spec seems to advise that images that are concerned with portability should be using OCI types for these things, and that seems easy enough to fix here, but the spec notes that implementations mustn't error out when encountering unknown types, and it looks to me like we're doing that here (for certain values of "unknown", anyway).

The spec says

Implementations storing or copying image manifests MUST NOT error on encountering a value that is unknown to the implementation.

That applies to skopeo copy, but not really to pulls which must consume the data.

It would be ~easy enough to handle some of the code paths; e.g. Inspect can ignore the manifest media type and choose an implementation based on just the config’s media type.

But I don’t think it’s longer-term reasonable, because the matrix of combinations blows up and capabilities can no longer be relied on.

E.g., (until recent Docker’s vendor-specific extension), “OCI images don’t support health checks” was true. If we support “OCI images with v2s2 configs”… what does that mean?

If we support “OCI images with v2s2 configs”, do we need to implement a “convert an OCI image to really-follows-spec-OCI-image” operation?

Hypothetically, if OCI config annotations are defined: What does it mean to have a v2s2 config with a an OCI-config-specific annotation?

baude commented 7 months ago

@codingben @nalind @mtrmac thanks for all the write ups ... i wanted to make sure we captured this for the future. it also seems to me that the title of the issue no longer reflects what is going on ... should it be updated for clarity?

mtrmac commented 7 months ago

All that said, this output

$ podman manifest inspect quay.io/boukhano/cirros:6.1                   
{
    "schemaVersion": 2,
    "mediaType": "application/vnd.docker.distribution.manifest.list.v2+json",

is either a bug, or maybe an intentionally-permissive implementation somehow trying to fall back to v2s2 based on the inconsistent per-platform instances.

nalind commented 7 months ago

I expect that was built using https://github.com/kubevirt/containerdisks/pull/81, where the mediaType values being used are all hard-coded.

mtrmac commented 7 months ago

Was the image under that tag overwritten / replaced? At least right now, skopeo inspect --raw shows the image to use an OCI index media type.

codingben commented 7 months ago

Was the image under that tag overwritten / replaced? At least right now, skopeo inspect --raw shows the image to use an OCI index media type.

It should be OCI index media type and OCI image manifests, I just updated it again:

╰─$ docker manifest inspect quay.io/boukhano/cirros:6.1     
{
   "schemaVersion": 2,
   "mediaType": "application/vnd.oci.image.index.v1+json",
   "manifests": [
      {
         "mediaType": "application/vnd.oci.image.manifest.v1+json",
         "size": 418,
         "digest": "sha256:a0e5b0df033c10034371d9711d90771816f349b57e246cf95b17b52b086a0d26",
         "platform": {
            "architecture": "amd64",
            "os": "linux"
         }
      },
      {
         "mediaType": "application/vnd.oci.image.manifest.v1+json",
         "size": 418,
         "digest": "sha256:7aa5762d40d1bbc545210629f9ff5a2036179d63551a5abbfe13f614f6cb4962",
         "platform": {
            "architecture": "arm64",
            "os": "linux"
         }
      }
   ]
}

If you run podman manifest inspect quay.io/boukhano/cirros:6.1, it'll return Docker manifest list instead of OCI image index now.

codingben commented 7 months ago

@mtrmac Because of it, we decided that in our production registry quay.io/organization/containerdisks - We'll use Docker schemas and not OCI because of this issue [1].

So I'll leave quay.io/boukhano/cirros:6.1 to use OCI image index and OCI image manifests for testing purposes of this issue.

[1] https://github.com/kubevirt/containerdisks/compare/40fc44a45797c5e02bfdf72d1f161fc0bb155254..c39ddf6c0af1b76817541c0cdae8e744003b252d

mtrmac commented 7 months ago

I have gone ahead and retitled this to only describe the podman manifest inspect behavior, under the assumption that we don’t want to commit to fully supporting mixed-schema images. (Fixing this might mean just refusing the operation, or some earlier operation, not necessarily adding support.)

I’m open to the argument that this should actually be supported, and that this issue should track that instead.

nalind commented 7 months ago

It would be ~easy enough to handle some of the code paths; e.g. Inspect can ignore the manifest media type and choose an implementation based on just the config’s media type.

But I don’t think it’s longer-term reasonable, because the matrix of combinations blows up and capabilities can no longer be relied on.

I'm not advocating that we generate such images.

E.g., (until recent Docker’s vendor-specific extension), “OCI images don’t support health checks” was true. If we support “OCI images with v2s2 configs”… what does that mean?

If podman can parse the configuration blob (in whatever format that it's in), and that format provides settings that podman understands, it should pay attention to them. It already has to vary how it parses manifests and config blobs in order to avoid discarding information that's specific to one format or another. The main question for me is whether it does so by directly consulting the mediaType in the config descriptor, or if it just assumes based on the mediaType of the image manifest as a whole.

If we support “OCI images with v2s2 configs”, do we need to implement a “convert an OCI image to really-follows-spec-OCI-image” operation?

It's been a while since I've dug down into the image library's copy logic, but I thought that it was already prepared to convert data from one format to another, and to do so at multiple levels, when the destination doesn't accept an image in its as-is form. Granted, I don't remember it trying to convert an image into "the format the manifest says it's in, with anything that might be considered inconsistent ironed out all the way down", because it's not something I thought it might need to try.

github.com/moby/docker-image-spec/specs-go/v1.DockerOCIImageMediaType has the same value as github.com/containers/image/v5/manifest.DockerV2Schema2ConfigMediaType, and I think that means we're probably going to see OCI images with that type in their config descriptors at some point.

Oh no, maybe I am advocating that we start generating such images.

Hypothetically, if OCI config annotations are defined: What does it mean to have a v2s2 config with a an OCI-config-specific annotation?

Annotations in the config descriptor that are applicable only to config data of a different mediaType than the one in the descriptor? I might want the implementation to feign ignorance of that annotation in that case.

mtrmac commented 7 months ago

github.com/moby/docker-image-spec/specs-go/v1.DockerOCIImageMediaType has the same value as github.com/containers/image/v5/manifest.DockerV2Schema2ConfigMediaType, and I think that means we're probably going to see OCI images with that type in their config descriptors at some point.

Great catch, I didn’t notice that constant.

But that constant is actually not used anywhere in the docker/docker or docker/cli codebase - they use the OCI config media type for configs generated using docker-image-spec.

Oh no, maybe I am advocating that we start generating such images.

I do agree that Docker compatibility, or similar concerns, might lead us to support some JSON fields, and some MIME types, not in the official OCI specification.

I’d prefer not to generalize that to the ambition to support any possible combination of MIME types that shows up once. Just saying “no, that’s a bug, fix it before your first release” is, I think, frequently the better option for the ecosystem as a whole. It’s probably a fairly small one-time fix in one codebase, vs. adding support for that quirk not just in c/image, but also in any other possible consumer over time, and maintaining that part of the test matrix for a long time.

nalind commented 7 months ago

github.com/moby/docker-image-spec/specs-go/v1.DockerOCIImageMediaType has the same value as github.com/containers/image/v5/manifest.DockerV2Schema2ConfigMediaType, and I think that means we're probably going to see OCI images with that type in their config descriptors at some point.

Great catch, I didn’t notice that constant.

But that constant is actually not used anywhere in the docker/docker or docker/cli codebase - they use the OCI config media type for configs generated using docker-image-spec.

I hadn't checked on that, thanks for doing the legwork. Something to keep an eye out for.

Oh no, maybe I am advocating that we start generating such images.

I do agree that Docker compatibility, or similar concerns, might lead us to support some JSON fields, and some MIME types, not in the official OCI specification.

I’d prefer not to generalize that to the ambition to support any possible combination of MIME types that shows up once. Just saying “no, that’s a bug, fix it before your first release” is, I think, frequently the better option for the ecosystem as a whole. It’s probably a fairly small one-time fix in one codebase, vs. adding support for that quirk not just in c/image, but also in any other possible consumer over time, and maintaining that part of the test matrix for a long time.

That's entirely reasonable, since I'd now characterize the origin of this issue as an attempt to change the mediaType values in images that the containerdisks package produces, an attempt that didn't quite catch everything that needed to be changed.

The possible future compatibility concern is my main concern as well. Is there anything that you think we can or should do in the near-term to be better prepared for a case where this happens? I can easily imagine wanting to be able to provide a healthcheck configuration in an OCI image and trying to swap in a different config blob type to make it work. I haven't checked if the various registry implementations would accept such a thing, though, so that may not be an option yet.

mtrmac commented 7 months ago

As a vague guess based on past experience, I agree I’d expect strict Quay validation to be one of the main constraints.

The thing is, “being prepared” can be argued both ways:

completely ignore MIME types and just parse the fields that exist, and (due to Go’s questionable-but-pragmatically-convenient default behavior of silently ignoring unknown JSON fields) that will probably work when encountering future formats — work somehow
or, strictly enforce MIME format match (and reject unknown fields?), so that unknown critical fields are not ignored, and users are told that they must upgrade.

I think, on the net, for forward compatibility, a combination of strictly enforcing MIME formats, and silently ignoring unknown fields, is a good option, because it gives future formats all the tools necessary: they can represent non-critical features as new fields in existing MIME types (and they will be ignored by old consumers), and critical features as new MIME types (and they will cause old consumers to fail).

(Note that skopeo copy will copy known image MIME types, unknown artifacts, and unknown image MIME types, all the same way; using an unknown image config “only” causes the layer compression/decompression code to disengage. So, in that sense, we are forward-compatible; but podman pull / podman run does refuse to work with unexpected MIME types. And, now that non-image artifacts are becoming increasingly prevalent, I think it rather has to continue refusing that.)

But then, none of the above is an argument one way or the other for the v2s2-config-in-OCI-manifest situation. That’s not really a “forward compatibility” case, and we know it’s not an artifact.

(WRT the specific health check example, as it happens, the c/image abstraction of Inspect, for probably path dependency reasons without a deeper cause, doesn’t contain the health check field at all, and it is handled in Buildah/Podman/common.

If we want to add support Docker’s healthcheck-in-OCI extension (and we probably do, eventually), c/image will have to learn about the field, so that c/image’s conversions between v2s2 and OCI don’t discard it. And that would be a natural time to add the field to types.Image.Inspect; then the Podman etc. consumers can migrate to that future c/image abstraction, instead of adding another handler for OCI specifically.)

github-actions[bot] commented 6 months ago

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

mtrmac commented 6 months ago

Looking at the manifest inspect behavior, the explanation is:

Internally, podman manifest is keeping the WIP manifests in-memory in both v2s2 and OCI formats. That’s similar to what Buildah generally does. Compare https://github.com/containers/buildah/pull/5452 .
And then, manifest inspect is supposed to return a MIME type. The logic to do that is designed this way: https://github.com/containers/common/blob/1fd0f881f63b23f86fce32740bf35d920ccae1ab/libimage/manifest_list.go#L278-L281 i.e. it is choosing based on whether the WIP manifest requires one or the other

I think the underlying issue here is that podman manifest … is designed to create multi-arch images; there is no podman manifest pull.

So, combining podman pull + podman manifest inspect does not fail but it seems to not have been a part of the initially-contemplated design, at least to the extent that inspect command intentionally optimizes for the case of pushing newly-created images, at the cost of fidelity when showing data about existing ones.

And, like in Buildah, changing the “WIP objects exist in both formats simultaneously” assumption would be a fairly invasive change.

Meanwhile, skopeo inspect --raw is available to inspect external on-registry objects, so podman manifest inspect does not need to prioritize the use case.

So, overall, leaving things as they are seems broadly reasonable to me, except perhaps that podman-manifest-inspect.1 should document what the command is doing in a bit more detail.