Fix several conversions of "ocispec.Image" to "ocispec.Platform" by tianon · Pull Request #7376 · containerd/containerd

tianon · 2022-09-08T00:30:43Z

Several bits of code unmarshal image config JSON into an ocispec.Image, and then immediately create an ocispec.Platform out of it, but then discard the original image and miss several potential platform fields (most notably, variant).

Because ocispec.Platform is a strict subset of ocispec.Image, most of these can be updated to simply unmarshal the image config directly to ocispec.Platform instead, which allows these additional fields to be picked up appropriately.

We can use tianon/raspbian as a concrete reproducer to demonstrate.

Before:

$ ctr content fetch docker.io/tianon/raspbian:bullseye-slim
...

$ ctr image ls
REF                                     TYPE                                                 DIGEST                                                                  SIZE     PLATFORMS    LABELS
docker.io/tianon/raspbian:bullseye-slim application/vnd.docker.distribution.manifest.v2+json sha256:66e96f8af40691b335acc54e5f69711584ef7f926597b339e7d12ab90cc394ce 28.6 MiB linux/arm/v7 -

(Note that the PLATFORMS column lists linux/arm/v7 -- the image itself is actually linux/arm/v6, but one of these bits of code leads to only linux/arm being extracted from the image config, which platforms.Normalize then updates to an explicit v7.)

After:

$ ctr image ls
REF                                     TYPE                                                 DIGEST                                                                  SIZE     PLATFORMS    LABELS
docker.io/tianon/raspbian:bullseye-slim application/vnd.docker.distribution.manifest.v2+json sha256:66e96f8af40691b335acc54e5f69711584ef7f926597b339e7d12ab90cc394ce 28.6 MiB linux/arm/v6 -

(Tianon's back on his BS 😇 ❤️)

There's probably a pretty decent argument to be made for the OCI's Image type to embed Platform directly (which would make this more straightforward) and/or to offer a helper function that appropriately extracts a Platform object from an Image object, but I didn't want the perfect to be the enemy of the good, and this seemed like a good place to start.

AkihiroSuda · 2022-09-08T01:43:58Z

Can we have an unit test?

tianon · 2022-09-08T16:36:38Z

I'm honestly not exactly sure where I would put unit tests for these changes -- they're all reading blobs from the content store, and each one is pretty deeply nested in other functionality. 😬

With a bit more guidance, I'd be willing to try, though! Any suggestions on a good place to start?

thaJeztah · 2022-09-08T16:48:56Z

pkg/unpack/unpacker.go

+	imgPlatform := platforms.Normalize(ocispec.Platform{
+		OS:           i.OS,
+		Architecture: i.Architecture,
+		Variant:      i.Variant,
+
+		OSVersion:  i.OSVersion,
+		OSFeatures: i.OSFeatures,
+	})


We should consider updating the ocispec.Image to embed the ocispec.Platform 🤔 ;

// Image is the JSON structure which describes some basic information about the image. // This provides the `application/vnd.oci.image.config.v1+json` mediatype when marshalled to JSON. type Image struct { // Created is the combined date and time at which the image was created, formatted as defined by RFC 3339, section 5.6. Created *time.Time `json:"created,omitempty"` // Author defines the name and/or email address of the person or entity which created and is responsible for maintaining the image. Author string `json:"author,omitempty"` Platform // Config defines the execution parameters which should be used as a base when running a container using the image. Config ImageConfig `json:"config,omitempty"` // RootFS references the layer content addresses used by the image. RootFS RootFS `json:"rootfs"` // History describes the history of each layer. History []History `json:"history,omitempty"` }

Then all this copying wouldn't be needed, and we could just do

Suggested change

imgPlatform := platforms.Normalize(ocispec.Platform{

OS: i.OS,

Architecture: i.Architecture,

Variant: i.Variant,

OSVersion: i.OSVersion,

OSFeatures: i.OSFeatures,

})

imgPlatform := platforms.Normalize(i.Platform)

Violent agreement! 😅

There's probably a pretty decent argument to be made for the OCI's Image type to embed Platform directly (which would make this more straightforward) and/or to offer a helper function that appropriately extracts a Platform object from an Image object, but I didn't want the perfect to be the enemy of the good, and this seemed like a good place to start.

👀 ❤️ 😄

It would also be technically correct in that case to swap back to using Image in all these cases, but it feels like a huge amount of overkill to process the entire Image object, extract all the data, and then throw away all but at most five fields of it 😅

Oh! Missed your last comments; I was updating my fork of the image-spec; opencontainers/image-spec#949

😂

I agree that just marshaling/unmarshaling the platform makes a lot of sense; but indeed more "correct" if it's actually embedded, which ties them together to be the same thing.

Given opencontainers/image-spec#949 is merged (which further legitimizes this PR's implementation), is this ready for re-review or do we want to do #7382 first and rebase this on top of that? 🙇

cpuguy83 · 2022-11-02T00:29:34Z

pkg/unpack/unpacker.go

 	var unpack *Platform

-	imgPlatform := platforms.Normalize(ocispec.Platform{OS: i.OS, Architecture: i.Architecture})
+	imgPlatform := platforms.Normalize(ocispec.Platform{


This can just be i.Platform now, I think?

Or is that not imported yet?

Yeah, that's #7382 😅

I guess I could import that change and take over from @thaJeztah if you'd like me to? (he seems busy enough that I bet he wouldn't mind 😂)

cpuguy83

LGTM

tianon · 2022-12-16T16:56:04Z

Rebased and rolled in #7382 (with an update to the merged version of opencontainers/image-spec#949 and to use i.Platform in the place @cpuguy83 pointed out -- @thaJeztah I put you as the co-author on the commit, but if you'd rather I'm happy to adjust it however you'd like, make you the only author, remove your changes, etc, whatever 👍).

tianon · 2023-03-27T18:59:26Z

(rebased and resolved conflicts)

estesp · 2023-03-27T19:55:03Z

The project checks want integration/go.mod to match up to the changes in go.mod 😇 Otherwise looks good

thaJeztah · 2023-03-27T20:02:26Z

make vendor should probably do the trick;

containerd/Makefile

Line 460 in f7f2be7

    
           vendor: ## ensure all the go.mod/go.sum files are up-to-date including vendor/ directory

samuelkarp · 2023-03-27T23:24:12Z

/retest

samuelkarp · 2023-03-29T07:52:35Z

/retest

thaJeztah

LGTM (ci is happy now!)

thaJeztah

@tianon could you rebase this one? (also left some suggestions for the comments)

integration/client/go.mod

image.go

images/image.go

Several bits of code unmarshal image config JSON into an `ocispec.Image`, and then immediately create an `ocispec.Platform` out of it, but then discard the original image *and* miss several potential platform fields (most notably, `variant`). Because `ocispec.Platform` is a strict subset of `ocispec.Image`, most of these can be updated to simply unmarshal the image config directly to `ocispec.Platform` instead, which allows these additional fields to be picked up appropriately. We can use `tianon/raspbian` as a concrete reproducer to demonstrate. Before: ```console $ ctr content fetch docker.io/tianon/raspbian:bullseye-slim ... $ ctr image ls REF TYPE DIGEST SIZE PLATFORMS LABELS docker.io/tianon/raspbian:bullseye-slim application/vnd.docker.distribution.manifest.v2+json sha256:66e96f8af40691b335acc54e5f69711584ef7f926597b339e7d12ab90cc394ce 28.6 MiB linux/arm/v7 - ``` (Note that the `PLATFORMS` column lists `linux/arm/v7` -- the image itself is actually `linux/arm/v6`, but one of these bits of code leads to only `linux/arm` being extracted from the image config, which `platforms.Normalize` then updates to an explicit `v7`.) After: ```console $ ctr image ls REF TYPE DIGEST SIZE PLATFORMS LABELS docker.io/tianon/raspbian:bullseye-slim application/vnd.docker.distribution.manifest.v2+json sha256:66e96f8af40691b335acc54e5f69711584ef7f926597b339e7d12ab90cc394ce 28.6 MiB linux/arm/v6 - ``` Signed-off-by: Tianon Gravi <admwiggin@gmail.com> Co-authored-by: Sebastiaan van Stijn <github@gone.nl>

tianon · 2023-05-30T20:15:07Z

Rebased and resolved discussion points 👍

thaJeztah

LGTM, thanks!

thaJeztah reviewed Sep 8, 2022

View reviewed changes

This was referenced Sep 8, 2022

Embed Platform in Image opencontainers/image-spec#949

Merged

[do not merge] test image-spec with embedded Platform #7382

Closed

cpuguy83 reviewed Nov 2, 2022

View reviewed changes

cpuguy83 approved these changes Nov 2, 2022

View reviewed changes

tianon force-pushed the oci-platform branch from 582277c to 610de22 Compare December 16, 2022 16:53

thaJeztah mentioned this pull request Mar 22, 2023

c8d/list: Ignore attestation manifests moby/moby#45124

Merged

tianon force-pushed the oci-platform branch from 610de22 to 12f133a Compare March 27, 2023 18:58

cpuguy83 approved these changes Mar 27, 2023

View reviewed changes

tianon force-pushed the oci-platform branch from 12f133a to 1e4c362 Compare March 27, 2023 22:31

thaJeztah approved these changes Mar 29, 2023

View reviewed changes

thaJeztah requested changes May 30, 2023

View reviewed changes

integration/client/go.mod Outdated Show resolved Hide resolved

image.go Outdated Show resolved Hide resolved

images/image.go Outdated Show resolved Hide resolved

images/image.go Outdated Show resolved Hide resolved

thaJeztah mentioned this pull request May 30, 2023

Image: fix some minor nits #8597

Merged

tianon force-pushed the oci-platform branch from 1e4c362 to 6975ba8 Compare May 30, 2023 20:11

tianon force-pushed the oci-platform branch from 6975ba8 to 21b3318 Compare May 30, 2023 20:13

thaJeztah approved these changes May 30, 2023

View reviewed changes

samuelkarp approved these changes May 31, 2023

View reviewed changes

samuelkarp merged commit 8b66a75 into containerd:main May 31, 2023

tianon deleted the oci-platform branch May 31, 2023 01:22

thaJeztah mentioned this pull request Jun 13, 2023

Add the image platform to the images/json endpoint moby/moby#45735

Closed

thaJeztah mentioned this pull request Jul 6, 2023

Uncopypaste parsing of OCI Bundle spec file #8780

Merged

thaJeztah mentioned this pull request Nov 19, 2024

c8d/container/inspect: Return ImageManifestDescriptor moby/moby#48855

Merged

Conversation

tianon commented Sep 8, 2022

Uh oh!

AkihiroSuda commented Sep 8, 2022

Uh oh!

tianon commented Sep 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

tianon commented Dec 16, 2022

Uh oh!

tianon commented Mar 27, 2023

Uh oh!

estesp commented Mar 27, 2023

Uh oh!

thaJeztah commented Mar 27, 2023

Uh oh!

samuelkarp commented Mar 27, 2023

Uh oh!

samuelkarp commented Mar 29, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tianon commented May 30, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants