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.

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

image exporter: return image descriptor in response #2610

Merged
merged 1 commit into from
Feb 10, 2022

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Feb 8, 2022

fixes #2558

$ ./hack/binaries
$ sudo ./bin/buildkitd &
$ sudo ./bin/buildctl build --progress plain --frontend gateway.v0 --opt source=docker/dockerfile --metadata-file metadata.json --local context=. --local dockerfile=. --output type=oci,dest=test.tar
$ cat metadata.json  | jq
{
  "containerimage.buildinfo": {
    "sources": [
      {
        "pin": "sha256:21a3deaa0d32a8057914f36584b5288d2e5ecc984380bc0118285c70fa8c9300",
        "ref": "docker.io/library/alpine@sha256:21a3deaa0d32a8057914f36584b5288d2e5ecc984380bc0118285c70fa8c9300",
        "type": "docker-image"
      }
    ]
  },
  "containerimage.config.digest": "sha256:2937f66a9722f7f4a2df583de2f8cb97fc9196059a410e7f00072fc918930e66",
  "containerimage.descriptor": {
    "annotations": {
      "config.digest": "sha256:2937f66a9722f7f4a2df583de2f8cb97fc9196059a410e7f00072fc918930e66",
      "org.opencontainers.image.created": "2022-02-08T21:28:03Z"
    },
    "digest": "sha256:19ffeab6f8bc9293ac2c3fdf94ebe28396254c993aea0b5a542cfb02e0883fa3",
    "mediaType": "application/vnd.oci.image.manifest.v1+json",
    "size": 506
  },
  "containerimage.digest": "sha256:19ffeab6f8bc9293ac2c3fdf94ebe28396254c993aea0b5a542cfb02e0883fa3"
}

@tonistiigi About config.digest as annotation should this be removed like?:

delete(desc.Annotations, exptypes.ExporterConfigDigestKey)

Signed-off-by: CrazyMax [email protected]

cmd/buildctl/build.go Outdated Show resolved Hide resolved
cmd/buildctl/build.go Outdated Show resolved Hide resolved
cmd/buildctl/build.go Show resolved Hide resolved
@crazy-max crazy-max force-pushed the descriptor-metadata branch 2 times, most recently from 254b272 to 65ec0e5 Compare February 8, 2022 22:42
@crazy-max
Copy link
Member Author

Not sure if TestDiffOnlyMerge failed test is linked to the cni stack trace we have. Any idea @sipsma?

@sipsma
Copy link
Collaborator

sipsma commented Feb 8, 2022

Not sure if TestDiffOnlyMerge failed test is linked to the cni stack trace we have. Any idea @sipsma?

Talked with @tonistiigi about this in Slack a few days ago when he encountered a similar stack trace in the TestDiffSingleDeleteLayerOnMerge test. It's still hard to see how it could be directly related to merge/diff (it could just be that there are a ton of merge/diff tests that each do a lot of builds, but not sure) and also seems completely distinct from the CNI stack trace. I will take a closer look at it now, but shouldn't be a blocker for this PR or anything.

continue
}
var raw json.RawMessage
if err = json.Unmarshal(dt, &raw); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

RawMessage.UnmarshalJSON() doesn't actually parse any JSON https://cs.opensource.google/go/go/+/refs/tags/go1.17.6:src/encoding/json/stream.go;l=271 . This seems to still work but I'm not sure how 🤔 (or maybe just small inputs work) . But I think anyway it is safer to try parsing it to actual map interface. I think it is enough if we validate that it is a JSON object and don't handle liternals and arrays. That should minimize any possibility of randomly hitting this. Eg. "MTIK" is a valid base64 encoded JSON. (You can actually add it as a test case).

If the parsing is ok then use the data as RawMessage and discard the parsing result.

Copy link
Member

@tonistiigi tonistiigi Feb 8, 2022

Choose a reason for hiding this comment

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

it is a JSON object

Let's also make sure it is a non-zero length JSON object.

Copy link
Member

Choose a reason for hiding this comment

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

^

@crazy-max crazy-max force-pushed the descriptor-metadata branch 2 times, most recently from 34a2e1e to 3b96419 Compare February 9, 2022 10:48
return continuity.AtomicWriteFile(filename, stripJSONNullProperties(b), 0666)
}

func stripJSONNullProperties(src []byte) []byte {
Copy link
Member

Choose a reason for hiding this comment

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

That's not what I meant. Just json.Unmarshal into a map[string]interface{}. If it decodes without an error and len(map) > 0 then add the bytes to out as RawMessage. Otherwise, we show the value as the original string.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. For the strip json null props func I was thinking it could be useful for cases like {"foo":null,"bar":"baz"}.

Copy link
Member

@tonistiigi tonistiigi Feb 10, 2022

Choose a reason for hiding this comment

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

I'm not sure what case it is? If the daemon already sends JSON then we should not transform it. The question is only how do we detect what values should be shown as-is and what should be decoded.

Copy link
Member Author

Choose a reason for hiding this comment

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

Was just preventive but agree we should not change it.

require.NoError(t, err)

require.Contains(t, res.ExporterResponse, exptypes.ExporterImageDescriptorKey)
dt, err := base64.StdEncoding.DecodeString(res.ExporterResponse[exptypes.ExporterImageDescriptorKey])
Copy link
Member

Choose a reason for hiding this comment

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

There are integration tests using buildctl as well https://github.com/moby/buildkit/blob/master/cmd/buildctl/build_test.go#L48 that allow you to also cover the json decoding feature without reimplementing it in a test.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've already expanded the testBuildMetadataFile one for the descriptor: https://github.com/moby/buildkit/pull/2610/files#diff-487f6d73514b6a9c5895f8c77586ce38dc2b108fb3ed93bd0d63452667a7c3c3R157-R164. Is it not enough?

Copy link
Member

@tonistiigi tonistiigi Feb 10, 2022

Choose a reason for hiding this comment

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

No that is fine. I'm ok with just removing this test completely then. All cases seem to be covered also in that buildctl test.

@tonistiigi tonistiigi merged commit 43cdda2 into moby:master Feb 10, 2022
@crazy-max crazy-max deleted the descriptor-metadata branch February 10, 2022 02:37
@crazy-max crazy-max added this to the v0.10.0 milestone Feb 10, 2022
@tonistiigi
Copy link
Member

@crazy-max
Copy link
Member Author

@tonistiigi Done in #2476 (README and build-repro.md): adb6b1d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

image exporter: return full image descriptor in metadata
3 participants