image/save: Fix untagged images not present in index.json by vvoland · Pull Request #47232 · moby/moby

vvoland · 2024-01-26T17:32:58Z

fixes: [v25] docker save <IMAGE>@<DIGEST> produces index.json with "manifest": null #47210
closes: integration test: Check OCI Manifest layers order #47165

Saving an image via digested reference, ID or truncated ID doesn't store the image reference in the archive. This also causes the save code to not add the image's manifest to the index.json.
This PR explicitly adds the untagged manifests to the index.json if no tagged manifests were added.

- How to verify it

TestSaveOCI integration test

By digest

$ docker save hello-world@sha256:4bd78111b6914a99dbc560e6a20eab57ff6655aea4a80c50b0c5491968cbc2e6 | tar xO index.json | jq
{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.index.v1+json",
  "manifests": [
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest": "sha256:411caf340c828657e915a83ed561a79d2b8150dabad4dc079d881cbfe6f86afe",
      "size": 447,
      "platform": {
        "architecture": "arm64",
        "os": "linux"
      }
    }
  ]
}

By tag

$ docker save hello-world | tar xO index.json | jq
{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.index.v1+json",
  "manifests": [
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest": "sha256:411caf340c828657e915a83ed561a79d2b8150dabad4dc079d881cbfe6f86afe",
      "size": 447,
      "annotations": {
        "io.containerd.image.name": "docker.io/library/hello-world:latest",
        "org.opencontainers.image.ref.name": "latest"
      },
      "platform": {
        "architecture": "arm64",
        "os": "linux"
      }
    }
  ]
}

Tag and digest

$ docker tag hello-world my:hello
$ docker save hello-world my:hello \
   hello-world@sha256:4bd78111b6914a99dbc560e6a20eab57ff6655aea4a80c50b0c5491968cbc2e6 | \
   tar xO index.json | jq
{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.index.v1+json",
  "manifests": [
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest": "sha256:411caf340c828657e915a83ed561a79d2b8150dabad4dc079d881cbfe6f86afe",
      "size": 447,
      "annotations": {
        "io.containerd.image.name": "docker.io/library/hello-world:latest",
        "org.opencontainers.image.ref.name": "latest"
      },
      "platform": {
        "architecture": "arm64",
        "os": "linux"
      }
    },
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest": "sha256:411caf340c828657e915a83ed561a79d2b8150dabad4dc079d881cbfe6f86afe",
      "size": 447,
      "annotations": {
        "io.containerd.image.name": "docker.io/library/my:hello",
        "org.opencontainers.image.ref.name": "hello"
      },
      "platform": {
        "architecture": "arm64",
        "os": "linux"
      }
    }
  ]
}

- Description for the changelog

- Fix `docker save <image>@<digest>` producing an OCI archive with index without manifests

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

Saving an image via digested reference, ID or truncated ID doesn't store the image reference in the archive. This also causes the save code to not add the image's manifest to the index.json. This commit explicitly adds the untagged manifests to the index.json if no tagged manifests were added. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

When saving an image treat `image@sha256:abcdef...` the same as `abcdef...`, this makes it: - Not export the digested tag as the image name - Not try to export all tags from the image repository Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

pkg/archive/archive.go

cpuguy83 · 2024-02-01T17:32:39Z

internal/testutils/specialimage/multilayer.go

+	}
+
+	manifestDesc.Annotations = map[string]string{
+		"io.containerd.image.name": "docker.io/library/multilayer:latest",


I would really like to see these hard-coded values moved up the stack. With it like this it makes the test more confusing, for me, to work through.
Likewise with the layer contents.

Extracted another function and moved the hardcoded values up. Let me know if that works for you!

thaJeztah · 2024-02-02T18:06:03Z

@vvoland could you have a peek at the review comment above?

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

thaJeztah · 2024-02-05T13:12:38Z

Failures are Windows + containerd snapshotter, and are known to still fail, so can be ignored

thaJeztah

quick blurb, as I started to look (so far nothing blocking) but had to drop off, so let me post what I had

thaJeztah · 2024-02-05T16:49:39Z

image/tarexport/save.go

+			taggedManifest := untaggedMfstDesc
+			taggedManifest.Annotations = map[string]string{
+				images.AnnotationImageName: ref.String(),
+				ocispec.AnnotationRefName:  ref.Tag(),
+			}
+			manifestDescriptors = append(manifestDescriptors, taggedManifest)


I honestly somewhat more liked the struct-literal here, to prevent any possible confusion that things / fields could be used by-reference 🤔

It would result in some small amount of duplication (constructing the struct twice), just have been bit by maps or slices not being properly de-referenced on some occasions.

thaJeztah

LGTM

tianon · 2024-12-06T00:15:42Z

image/tarexport/save.go

+			taggedManifest := untaggedMfstDesc
+			taggedManifest.Annotations = map[string]string{
+				images.AnnotationImageName: ref.String(),
+				ocispec.AnnotationRefName:  ref.Tag(),


FWIW, it's fully valid for this to include the full reference:

https://github.com/opencontainers/image-spec/blob/5325ec48851022d6ded604199a3566254e72842a/annotations.md#:~:text=org.opencontainers.image.ref.name

Hm.. interesting; so that would mean duplicating the same information (and implicitly; requiring the reader of that information having to parse the format to get the tag only?).

One thing I notice in that section is that it looks like separator as part of components seems to have been conflated with separator between name and tag (or digest);

org.opencontainers.image.ref.name Name of the reference for a target (string).

SHOULD only be considered valid when on descriptors on index.json within image layout.

Character set of the value SHOULD conform to alphanum of A-Za-z0-9 and separator set of -._:@/+

A valid reference matches the following grammar:

ref ::= component ("/" component)* component ::= alphanum (separator alphanum)* alphanum ::= [A-Za-z0-9]+ separator ::= [-._:@+] | "--"

The distribution/reference grammar uses a different definition;

separator := /[_.]|__|[-]*/

it only allows -._ as separators

2 consecutive underscores (__)

zero or more hypens (-)

The :, @ would be to separate tag or digest, and

If I read the OCI spec there correctly, something like docker.io/github.com/he:llo@sha256+fo--b_ar would be a valid value, but not parseable with the distribution/reference package 😬

Hm.. for org.opencontainers.image.base.name, there's a mention that it should be in the format described in distribution/reference; should that also be done for the above?

org.opencontainers.image.base.name Image reference of the image this image is based on (string)

This SHOULD be image references in the format defined by [distribution/distribution][distribution-reference].

vvoland added status/2-code-review process/cherry-pick area/images Image Distribution kind/bugfix PR's that fix bugs process/cherry-pick/25.0 labels Jan 26, 2024

vvoland added this to the 26.0.0 milestone Jan 26, 2024

vvoland self-assigned this Jan 26, 2024

vvoland added 2 commits January 29, 2024 16:26

vvoland force-pushed the fix-save-manifests branch 3 times, most recently from da0e203 to fbe269f Compare January 29, 2024 19:00

vvoland marked this pull request as ready for review January 29, 2024 19:39

vvoland requested a review from cpuguy83 January 29, 2024 19:39

thaJeztah reviewed Jan 30, 2024

View reviewed changes

pkg/archive/archive.go Outdated Show resolved Hide resolved

thaJeztah reviewed Jan 30, 2024

View reviewed changes

pkg/archive/archive.go Outdated Show resolved Hide resolved

vvoland force-pushed the fix-save-manifests branch from fbe269f to 364316c Compare February 1, 2024 13:39

vvoland added the containerd-integration Issues and PRs related to containerd integration label Feb 1, 2024

vvoland mentioned this pull request Feb 1, 2024

[25.0 backport] image/save: Fix untagged images not present in index.json #47294

Merged

cpuguy83 requested changes Feb 1, 2024

View reviewed changes

integration/save: Add tests checking OCI archive output

2ef0b53

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland force-pushed the fix-save-manifests branch from 364316c to 2ef0b53 Compare February 5, 2024 10:18

thaJeztah mentioned this pull request Feb 5, 2024

OCI export should only put the tag into org.opencontainers.image.ref.name in index.json moby/buildkit#4615

Open

vvoland requested a review from cpuguy83 February 5, 2024 16:47

cpuguy83 approved these changes Feb 5, 2024

View reviewed changes

thaJeztah reviewed Feb 5, 2024

View reviewed changes

thaJeztah approved these changes Feb 5, 2024

View reviewed changes

thaJeztah merged commit 2156635 into moby:master Feb 5, 2024

thaJeztah added process/cherry-picked and removed process/cherry-pick process/cherry-pick/25.0 labels Feb 12, 2024

tianon reviewed Dec 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

image/save: Fix untagged images not present in index.json#47232

image/save: Fix untagged images not present in index.json#47232
thaJeztah merged 3 commits intomoby:masterfrom
vvoland:fix-save-manifests

vvoland commented Jan 26, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

cpuguy83 Feb 1, 2024

Uh oh!

vvoland Feb 5, 2024

Uh oh!

thaJeztah commented Feb 2, 2024

Uh oh!

thaJeztah commented Feb 5, 2024

Uh oh!

thaJeztah left a comment

Uh oh!

thaJeztah Feb 5, 2024

Uh oh!

thaJeztah left a comment

Uh oh!

tianon Dec 6, 2024

Uh oh!

thaJeztah Dec 6, 2024 •

edited

Loading

Uh oh!

thaJeztah Dec 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

vvoland commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

By digest

By tag

Tag and digest

Uh oh!

Uh oh!

Uh oh!

cpuguy83 Feb 1, 2024

Choose a reason for hiding this comment

Uh oh!

vvoland Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Feb 2, 2024

Uh oh!

thaJeztah commented Feb 5, 2024

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

tianon Dec 6, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah Dec 6, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vvoland commented Jan 26, 2024 •

edited

Loading

thaJeztah Dec 6, 2024 •

edited

Loading