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.

Sign up for GitHub

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

Add OCI support to manifest subcommand #3990

Merged
merged 3 commits into from
Jan 31, 2023
Merged

Add OCI support to manifest subcommand #3990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
070825b
manifest: add support for oci image types
jedevc Jan 25, 2023
285e137
manifest: explicitly error if whitespace reconstruction has failed
jedevc Jan 27, 2023
67b9617
manifest: save raw manifest content on download
jedevc Jan 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 47 additions & 11 deletions cli/command/manifest/push.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
registryclient "github.com/docker/cli/cli/registry/client"
"github.com/docker/distribution"
"github.com/docker/distribution/manifest/manifestlist"
"github.com/docker/distribution/manifest/ocischema"
"github.com/docker/distribution/manifest/schema2"
"github.com/docker/distribution/reference"
"github.com/docker/docker/registry"
Expand Down Expand Up @@ -217,18 +218,53 @@ func buildPutManifestRequest(imageManifest types.ImageManifest, targetRef refere
return mountRequest{}, err
}

// This indentation has to be added to ensure sha parity with the registry
v2ManifestBytes, err := json.MarshalIndent(imageManifest.SchemaV2Manifest, "", " ")
if err != nil {
return mountRequest{}, err
}
// indent only the DeserializedManifest portion of this, in order to maintain parity with the registry
// and not alter the sha
var v2Manifest schema2.DeserializedManifest
if err = v2Manifest.UnmarshalJSON(v2ManifestBytes); err != nil {
return mountRequest{}, err
// Attempt to reconstruct indentation of the manifest to ensure sha parity
// with the registry - if we haven't preserved the raw content.
//
// This is necessary because our previous internal storage format did not
// preserve whitespace. If we don't have the newer format present, we can
// attempt the reconstruction like before, but explicitly error if the
// reconstruction failed!
switch {
case imageManifest.SchemaV2Manifest != nil:
dt := imageManifest.Raw
if len(dt) == 0 {
dt, err = json.MarshalIndent(imageManifest.SchemaV2Manifest, "", " ")
if err != nil {
return mountRequest{}, err
}
}

dig := imageManifest.Descriptor.Digest
if dig2 := dig.Algorithm().FromBytes(dt); dig != dig2 {
return mountRequest{}, errors.Errorf("internal digest mismatch for %s: expected %s, got %s", imageManifest.Ref, dig, dig2)
}

var manifest schema2.DeserializedManifest
if err = manifest.UnmarshalJSON(dt); err != nil {
return mountRequest{}, err
}
imageManifest.SchemaV2Manifest = &manifest
case imageManifest.OCIManifest != nil:
dt := imageManifest.Raw
if len(dt) == 0 {
dt, err = json.MarshalIndent(imageManifest.OCIManifest, "", " ")
if err != nil {
return mountRequest{}, err
}
Comment on lines +251 to +254
Copy link
Member

@thaJeztah thaJeztah Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

dig := imageManifest.Descriptor.Digest
if dig2 := dig.Algorithm().FromBytes(dt); dig != dig2 {
return mountRequest{}, errors.Errorf("internal digest mismatch for %s: expected %s, got %s", imageManifest.Ref, dig, dig2)
}

var manifest ocischema.DeserializedManifest
if err = manifest.UnmarshalJSON(dt); err != nil {
return mountRequest{}, err
}
imageManifest.OCIManifest = &manifest
}
imageManifest.SchemaV2Manifest = &v2Manifest

return mountRequest{ref: mountRef, manifest: imageManifest}, err
}
Expand Down
1 change: 1 addition & 0 deletions cli/command/manifest/testdata/inspect-annotate.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"variant": "v7"
}
},
"Raw": "ewogICAic2NoZW1hVmVyc2lvbiI6IDIsCiAgICJtZWRpYVR5cGUiOiAiYXBwbGljYXRpb24vdm5kLmRvY2tlci5kaXN0cmlidXRpb24ubWFuaWZlc3QudjIranNvbiIsCiAgICJjb25maWciOiB7CiAgICAgICJtZWRpYVR5cGUiOiAiYXBwbGljYXRpb24vdm5kLmRvY2tlci5jb250YWluZXIuaW1hZ2UudjEranNvbiIsCiAgICAgICJzaXplIjogMTUyMCwKICAgICAgImRpZ2VzdCI6ICJzaGEyNTY6NzMyOGY2ZjhiNDE4OTA1OTc1NzVjYmFhZGM4ODRlNzM4NmFlMGFjYzUzYjc0NzQwMWViY2U1Y2YwZDYyNDU2MCIKICAgfSwKICAgImxheWVycyI6IFsKICAgICAgewogICAgICAgICAibWVkaWFUeXBlIjogImFwcGxpY2F0aW9uL3ZuZC5kb2NrZXIuaW1hZ2Uucm9vdGZzLmRpZmYudGFyLmd6aXAiLAogICAgICAgICAic2l6ZSI6IDE5OTA0MDIsCiAgICAgICAgICJkaWdlc3QiOiAic2hhMjU2Ojg4Mjg2ZjQxNTMwZTkzZGZmZDRiOTY0ZTFkYjIyY2U0OTM5ZmZmYTRhNGM2NjVkYWI4NTkxZmJhYjAzZDQ5MjYiCiAgICAgIH0KICAgXQp9",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👀

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

base64 is great 👍

"SchemaV2Manifest": {
"schemaVersion": 2,
"mediaType": "application/vnd.docker.distribution.manifest.v2+json",
Expand Down
2 changes: 2 additions & 0 deletions cli/command/manifest/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ func getManifest(ctx context.Context, dockerCli command.Cli, listRef, namedRef r
return dockerCli.RegistryClient(insecure).GetManifest(ctx, namedRef)
case err != nil:
return types.ImageManifest{}, err
case len(data.Raw) == 0:
return dockerCli.RegistryClient(insecure).GetManifest(ctx, namedRef)
jedevc marked this conversation as resolved.
Show resolved Hide resolved
default:
return data, nil
}
Expand Down
42 changes: 39 additions & 3 deletions cli/manifest/types/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

"github.com/docker/distribution"
"github.com/docker/distribution/manifest/manifestlist"
"github.com/docker/distribution/manifest/ocischema"
"github.com/docker/distribution/manifest/schema2"
"github.com/docker/distribution/reference"
"github.com/opencontainers/go-digest"
Expand All @@ -16,10 +17,12 @@ import (
type ImageManifest struct {
Ref *SerializableNamed
Descriptor ocispec.Descriptor
Raw []byte `json:",omitempty"`

// SchemaV2Manifest is used for inspection
// TODO: Deprecate this and store manifest blobs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

SchemaV2Manifest *schema2.DeserializedManifest `json:",omitempty"`
// OCIManifest is used for inspection
OCIManifest *ocischema.DeserializedManifest `json:",omitempty"`
}

// OCIPlatform creates an OCI platform from a manifest list platform spec
Expand Down Expand Up @@ -53,8 +56,15 @@ func PlatformSpecFromOCI(p *ocispec.Platform) *manifestlist.PlatformSpec {
// Blobs returns the digests for all the blobs referenced by this manifest
func (i ImageManifest) Blobs() []digest.Digest {
digests := []digest.Digest{}
for _, descriptor := range i.SchemaV2Manifest.References() {
digests = append(digests, descriptor.Digest)
switch {
case i.SchemaV2Manifest != nil:
for _, descriptor := range i.SchemaV2Manifest.References() {
digests = append(digests, descriptor.Digest)
}
case i.OCIManifest != nil:
for _, descriptor := range i.OCIManifest.References() {
digests = append(digests, descriptor.Digest)
}
}
return digests
}
Expand All @@ -65,6 +75,8 @@ func (i ImageManifest) Payload() (string, []byte, error) {
switch {
case i.SchemaV2Manifest != nil:
return i.SchemaV2Manifest.Payload()
case i.OCIManifest != nil:
return i.OCIManifest.Payload()
default:
return "", nil, errors.Errorf("%s has no payload", i.Ref)
}
Expand All @@ -76,6 +88,8 @@ func (i ImageManifest) References() []distribution.Descriptor {
switch {
case i.SchemaV2Manifest != nil:
return i.SchemaV2Manifest.References()
case i.OCIManifest != nil:
return i.OCIManifest.References()
default:
return nil
}
Expand All @@ -84,13 +98,35 @@ func (i ImageManifest) References() []distribution.Descriptor {
// NewImageManifest returns a new ImageManifest object. The values for Platform
// are initialized from those in the image
func NewImageManifest(ref reference.Named, desc ocispec.Descriptor, manifest *schema2.DeserializedManifest) ImageManifest {
raw, err := manifest.MarshalJSON()
if err != nil {
raw = nil
}

return ImageManifest{
Ref: &SerializableNamed{Named: ref},
Descriptor: desc,
Raw: raw,
SchemaV2Manifest: manifest,
}
}

// NewOCIImageManifest returns a new ImageManifest object. The values for
// Platform are initialized from those in the image
func NewOCIImageManifest(ref reference.Named, desc ocispec.Descriptor, manifest *ocischema.DeserializedManifest) ImageManifest {
raw, err := manifest.MarshalJSON()
if err != nil {
raw = nil
}

return ImageManifest{
Ref: &SerializableNamed{Named: ref},
Descriptor: desc,
Raw: raw,
OCIManifest: manifest,
}
}

// SerializableNamed is a reference.Named that can be serialized and deserialized
// from JSON
type SerializableNamed struct {
Expand Down
44 changes: 39 additions & 5 deletions cli/registry/client/fetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/docker/cli/cli/manifest/types"
"github.com/docker/distribution"
"github.com/docker/distribution/manifest/manifestlist"
"github.com/docker/distribution/manifest/ocischema"
"github.com/docker/distribution/manifest/schema2"
"github.com/docker/distribution/reference"
"github.com/docker/distribution/registry/api/errcode"
Expand Down Expand Up @@ -35,6 +36,12 @@ func fetchManifest(ctx context.Context, repo distribution.Repository, ref refere
return types.ImageManifest{}, err
}
return imageManifest, nil
case *ocischema.DeserializedManifest:
imageManifest, err := pullManifestOCISchema(ctx, ref, repo, *v)
if err != nil {
return types.ImageManifest{}, err
}
return imageManifest, nil
case *manifestlist.DeserializedManifestList:
return types.ImageManifest{}, errors.Errorf("%s is a manifest list", ref)
}
Expand Down Expand Up @@ -94,6 +101,28 @@ func pullManifestSchemaV2(ctx context.Context, ref reference.Named, repo distrib
return types.NewImageManifest(ref, manifestDesc, &mfst), nil
}

func pullManifestOCISchema(ctx context.Context, ref reference.Named, repo distribution.Repository, mfst ocischema.DeserializedManifest) (types.ImageManifest, error) {
manifestDesc, err := validateManifestDigest(ref, mfst)
if err != nil {
return types.ImageManifest{}, err
}
configJSON, err := pullManifestSchemaV2ImageConfig(ctx, mfst.Target().Digest, repo)
if err != nil {
return types.ImageManifest{}, err
}

if manifestDesc.Platform == nil {
manifestDesc.Platform = &ocispec.Platform{}
}

// Fill in os and architecture fields from config JSON
if err := json.Unmarshal(configJSON, manifestDesc.Platform); err != nil {
return types.ImageManifest{}, err
}

return types.NewOCIImageManifest(ref, manifestDesc, &mfst), nil
}

func pullManifestSchemaV2ImageConfig(ctx context.Context, dgst digest.Digest, repo distribution.Repository) ([]byte, error) {
blobs := repo.Blobs(ctx)
configJSON, err := blobs.Get(ctx, dgst)
Expand Down Expand Up @@ -153,16 +182,21 @@ func pullManifestList(ctx context.Context, ref reference.Named, repo distributio
if err != nil {
return nil, err
}
v, ok := manifest.(*schema2.DeserializedManifest)
if !ok {
return nil, errors.Errorf("unsupported manifest format: %v", v)
}

manifestRef, err := reference.WithDigest(ref, manifestDescriptor.Digest)
if err != nil {
return nil, err
}
imageManifest, err := pullManifestSchemaV2(ctx, manifestRef, repo, *v)

var imageManifest types.ImageManifest
switch v := manifest.(type) {
case *schema2.DeserializedManifest:
imageManifest, err = pullManifestSchemaV2(ctx, manifestRef, repo, *v)
case *ocischema.DeserializedManifest:
imageManifest, err = pullManifestOCISchema(ctx, manifestRef, repo, *v)
default:
err = errors.Errorf("unsupported manifest type: %T", manifest)
}
if err != nil {
return nil, err
}
Expand Down
107 changes: 107 additions & 0 deletions vendor/github.com/docker/distribution/manifest/ocischema/builder.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading