imagetools: Allow annotations for OCI image index #1965

mqasimsarfraz · 2023-07-20T22:05:29Z

Currently there isn't any way to add annotations to OCI image index for multi-arch. The use case is to have a description in "About this version" in GitHub. It will introduce an optional flag of --annotation to imagetools create to allow adding OCI annotations.

Related: #1171

Testing Done:

OCI Image Index (annotation added)

$ ./bin/build/buildx  imagetools create  -t ghcr.io/inspektor-gadget/inspektor-gadget:v0.18.1 ghcr.io/inspektor-gadget/inspektor-gadget:v0.18.1@sha256:c3780808973801b24c80f8b46835b882fe0d69c08a4460333f28143569665861 ghcr.io/inspektor-gadget/inspektor-gadget:v0.18.1@sha256:d6d5661b02002aa8035e035e9210e3476e0401a948a86fabf104f71c2cbe0efe  --dry-run --annotation index:org.opencontainers.image.description="I am a description"
{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.index.v1+json",
  "manifests": [
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest": "sha256:c3780808973801b24c80f8b46835b882fe0d69c08a4460333f28143569665861",
      "size": 2963,
      "platform": {
        "architecture": "amd64",
        "os": "linux"
      }
    },
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest": "sha256:d6d5661b02002aa8035e035e9210e3476e0401a948a86fabf104f71c2cbe0efe",
      "size": 2962,
      "platform": {
        "architecture": "arm64",
        "os": "linux"
      }
    }
  ],
  "annotations": {
    "org.opencontainers.image.description": "I am a description"
  }
}

Manifest Descriptor

$ ./bin/build/buildx  imagetools create  -t ghcr.io/inspektor-gadget/inspektor-gadget:v0.18.1 ghcr.io/inspektor-gadget/inspektor-gadget:v0.18.1@sha256:c3780808973801b24c80f8b46835b882fe0d69c08a4460333f28143569665861 ghcr.io/inspektor-gadget/inspektor-gadget:v0.18.1@sha256:d6d5661b02002aa8035e035e9210e3476e0401a948a86fabf104f71c2cbe0efe  --dry-run --annotation manifest-descriptor:org.opencontainers.image.description="I am a description"
{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.index.v1+json",
  "manifests": [
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest": "sha256:c3780808973801b24c80f8b46835b882fe0d69c08a4460333f28143569665861",
      "size": 2963,
      "annotations": {
        "org.opencontainers.image.description": "I am a description"
      },
      "platform": {
        "architecture": "amd64",
        "os": "linux"
      }
    },
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest": "sha256:d6d5661b02002aa8035e035e9210e3476e0401a948a86fabf104f71c2cbe0efe",
      "size": 2962,
      "annotations": {
        "org.opencontainers.image.description": "I am a description"
      },
      "platform": {
        "architecture": "arm64",
        "os": "linux"
      }
    }
  ]
}

Docker manifest list (noop)

$ /bin/build/buildx  imagetools create  -t hello-world hello-world hello-world  --dry-run --annotation org.opencontainers.image.description="I am a description"
...

eiffel-fl

Thank you for it! I tested it and it works fine:

$ ./bin/build/buildx imagetools create -t ghcr.io/eiffel-fl/inspektor-gadget:vtest ghcr.io/inspektor-gadget/inspektor-gadget:v0.18.1@sha256:c3780808973801b24c80f8b46835b882fe0d69c08a4460333f28143569665861 ghcr.io/inspektor-gadget/inspektor-gadget:v0.18.1@sha256:d6d5661b02002aa8035e035e9210e3476e0401a948a86fabf104f71c2cbe0efe --annotations foo="bar"                                       
[+] Building 2.4s (1/1) FINISHED                                                                                                                                           
 => [internal] pushing ghcr.io/eiffel-fl/inspektor-gadget:vtest
$ ./bin/build/buildx imagetools inspect --raw ghcr.io/eiffel-fl/inspektor-gadget:vtest                    qasim/oci-annotations u=
{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.index.v1+json",
  "manifests": [
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest": "sha256:c3780808973801b24c80f8b46835b882fe0d69c08a4460333f28143569665861",
      "size": 2963,
      "platform": {
        "architecture": "amd64",
        "os": "linux"
      }
    },
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest": "sha256:d6d5661b02002aa8035e035e9210e3476e0401a948a86fabf104f71c2cbe0efe",
      "size": 2962,
      "platform": {
        "architecture": "arm64",
        "os": "linux"
      }
    }
  ],
  "annotations": {
    "foo": "bar"
  }
}

I think we are OK regarding the rules, as the reversed domain name is only an advise and due to using map we cannot have two times the same key.

util/imagetools/create.go

mqasimsarfraz · 2023-07-24T08:01:03Z

@crazy-max @jedevc any thoughts on this? :)

jedevc · 2023-07-24T09:41:55Z

Hm, in general I think the idea for this is alright 🎉

However, I think we'd probably want to retain consistency with how buildkit sets annotations: https://github.com/moby/buildkit/blob/master/docs/annotations.md.

There's not really any strong consensus on how we'd expose this in buildx build, but imagetools should probably follow an identical setup to avoid confusion.

#1171 (comment) gets pretty close to an ideal syntax imo. With, that to annotate your multi-arch image, you'd instead have:

$ buildx imagetools create -t hello-world hello-world hello-world --dry-run --annotation index:org.opencontainers.image.description="I am a description"

Would this kind of syntax work for you? I think we need consistency through buildx, so maybe good to continue the discussion there.

crazy-max · 2023-07-24T10:02:22Z

For the sake of parity we should consider supporting labels as well in follow-up.

mqasimsarfraz · 2023-07-24T13:04:18Z

in general I think the idea for this is alright tada

Great

Would this kind of syntax work for you? I think we need consistency through buildx.

Agreed, we should have consistency through buildx. Also, I was already thinking how can we give user a hint that these annotations are for index and not manifest/descriptor so using index: prefix works well for that. I will proceed the PR in this direction then!

mqasimsarfraz · 2023-07-27T16:15:07Z

@jedevc I have updated the PR to use the new syntax with index:. I still kept the scope to index only. Please feel free to let me know if you think we should handle descriptors as well.

tonistiigi · 2023-07-31T14:20:17Z

I've opened #1978 that you can use as a base for adding integration tests for checking the --annotation flag behavior.

mqasimsarfraz · 2023-08-01T12:31:17Z

I've opened #1978 that you can use as a base for adding integration tests for checking the --annotation flag behavior.

@tonistiigi great thanks. I updated the PR to include testImagetoolsAnnotation. Had to use oci-mediatypes=true to ensure we push an index and not a manifest list in the test.

tests/imagetools.go

util/imagetools/create.go

tests/imagetools.go

jedevc · 2023-08-03T08:47:40Z

util/imagetools/create.go

+	indexAnnotations := make(map[string]string)
+	manifestDescriptorAnnotations := make(map[string]string)
+	for k, v := range ann {
+		groups := annotationRegexp.FindStringSubmatch(k)


Instead of using a regexp here, wdyt about using https://github.com/moby/buildkit/blob/dd0053cdce470b1355fdb0bd5a8f2b0fc506d842/exporter/containerimage/exptypes/annotations.go#L85 instead?

Obviously, it's not perfect, since we'd need to add the annotation- prefix here, which might make the error message a bit odd, but we could always upstream a buildkit fix later that would rework the function to have the caller remove the prefix there (so we wouldn't have to do that).

Instead of using a regexp here, wdyt about using https://github.com/moby/buildkit/blob/dd0053cdce470b1355fdb0bd5a8f2b0fc506d842/exporter/containerimage/exptypes/annotations.go#L85 instead?

Initially I had the same idea but we are using : as a separator for type and annotation key compared to . expected here. So it needs more work than just appending annotation- prefix.

but we could always upstream a buildkit fix later that would rework the function

Does it make sense to make buildkit parser to also handle : separator. It sounded specific to buildx so I feel we should keep it here. wdyt?

Personally, I'd prefer the conversion to buildkit's form for now, so that we only have one source of truth for parsing these keys - I'm happy to follow up in buildkit to rework the logic to be a bit more reusable for this case.

I also just noticed (sorry), that we aren't handling platforms? manifest-descriptor supports a platform, and should only be attached to the descriptor for those platforms.

I'm happy to follow up in buildkit to rework the logic to be a bit more reusable for this case.

Agreed. That should be the way moving forward. I added a comment regrading using buildkit once it supports our use-case here.

I also just noticed (sorry), that we aren't handling platforms?

Great Catch. Done.

Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

jedevc

LGTM 🎉 Thanks @mqasimsarfraz!

mqasimsarfraz · 2023-08-08T12:59:37Z

Can you please merge this ? or do we have any open items against it? :)

We were using a specific version for docker buildx to have support for adding annotaions ( docker/buildx#1965). But it has been released into stable already so removing workaround. Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

mqasimsarfraz force-pushed the qasim/oci-annotations branch from 6d283d0 to 42ee388 Compare July 20, 2023 22:06

eiffel-fl approved these changes Jul 21, 2023

View reviewed changes

util/imagetools/create.go Outdated Show resolved Hide resolved

mqasimsarfraz force-pushed the qasim/oci-annotations branch from 42ee388 to b071d65 Compare July 21, 2023 14:57

mqasimsarfraz mentioned this pull request Jul 24, 2023

Proposal: annotations helpers #1171

Closed

mqasimsarfraz force-pushed the qasim/oci-annotations branch from b071d65 to e5e178f Compare July 27, 2023 14:58

mqasimsarfraz force-pushed the qasim/oci-annotations branch from e5e178f to f8d9676 Compare August 1, 2023 12:26

eiffel-fl reviewed Aug 2, 2023

View reviewed changes

tests/imagetools.go Outdated Show resolved Hide resolved

eiffel-fl reviewed Aug 2, 2023

View reviewed changes

tests/imagetools.go Outdated Show resolved Hide resolved

mqasimsarfraz force-pushed the qasim/oci-annotations branch from f8d9676 to 95b6a2f Compare August 2, 2023 08:38

jedevc requested changes Aug 2, 2023

View reviewed changes

util/imagetools/create.go Outdated Show resolved Hide resolved

util/imagetools/create.go Outdated Show resolved Hide resolved

tonistiigi reviewed Aug 2, 2023

View reviewed changes

tests/imagetools.go Outdated Show resolved Hide resolved

tests/imagetools.go Outdated Show resolved Hide resolved

mqasimsarfraz force-pushed the qasim/oci-annotations branch from 95b6a2f to 11c0502 Compare August 3, 2023 07:55

jedevc reviewed Aug 3, 2023

View reviewed changes

jedevc added needs/docs-follow-up needs/release-notes area/imagetools labels Aug 3, 2023

mqasimsarfraz added 2 commits August 3, 2023 12:08

allow annotations for OCI image index

18894a8

Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

tests: add testImagetoolsAnnotation integration test

3ef93e0

Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

mqasimsarfraz force-pushed the qasim/oci-annotations branch from 11c0502 to 3ef93e0 Compare August 3, 2023 10:20

jedevc approved these changes Aug 3, 2023

View reviewed changes

jedevc merged commit e5cee89 into docker:master Aug 8, 2023
61 checks passed

mqasimsarfraz mentioned this pull request Aug 8, 2023

container images: Add description to image index via annotations inspektor-gadget/inspektor-gadget#1946

Merged

jedevc mentioned this pull request Aug 24, 2023

build: add --annotation shortcut flag #2020

Merged

crazy-max removed needs/docs-follow-up needs/release-notes labels Nov 17, 2023

jedevc mentioned this pull request Dec 10, 2023

imagetools: cryptic error messages for annotation flag #2148

Open

3 tasks

mqasimsarfraz mentioned this pull request Feb 14, 2024

ci: unpin docker buildx version inspektor-gadget/inspektor-gadget#2503

Merged

crazy-max mentioned this pull request Apr 5, 2024

Improve buildx test coverage #1857

Open

35 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

imagetools: Allow annotations for OCI image index #1965

imagetools: Allow annotations for OCI image index #1965

mqasimsarfraz commented Jul 20, 2023 •

edited

eiffel-fl left a comment

mqasimsarfraz commented Jul 24, 2023

jedevc commented Jul 24, 2023

crazy-max commented Jul 24, 2023 •

edited

mqasimsarfraz commented Jul 24, 2023

mqasimsarfraz commented Jul 27, 2023

tonistiigi commented Jul 31, 2023

mqasimsarfraz commented Aug 1, 2023 •

edited

jedevc Aug 3, 2023

mqasimsarfraz Aug 3, 2023

jedevc Aug 3, 2023

mqasimsarfraz Aug 3, 2023

jedevc left a comment

mqasimsarfraz commented Aug 8, 2023

imagetools: Allow annotations for OCI image index #1965

imagetools: Allow annotations for OCI image index #1965

Conversation

mqasimsarfraz commented Jul 20, 2023 • edited

Testing Done:

OCI Image Index (annotation added)

Manifest Descriptor

Docker manifest list (noop)

eiffel-fl left a comment

Choose a reason for hiding this comment

mqasimsarfraz commented Jul 24, 2023

jedevc commented Jul 24, 2023

crazy-max commented Jul 24, 2023 • edited

mqasimsarfraz commented Jul 24, 2023

mqasimsarfraz commented Jul 27, 2023

tonistiigi commented Jul 31, 2023

mqasimsarfraz commented Aug 1, 2023 • edited

jedevc Aug 3, 2023

Choose a reason for hiding this comment

mqasimsarfraz Aug 3, 2023

Choose a reason for hiding this comment

jedevc Aug 3, 2023

Choose a reason for hiding this comment

mqasimsarfraz Aug 3, 2023

Choose a reason for hiding this comment

jedevc left a comment

Choose a reason for hiding this comment

mqasimsarfraz commented Aug 8, 2023

mqasimsarfraz commented Jul 20, 2023 •

edited

crazy-max commented Jul 24, 2023 •

edited

mqasimsarfraz commented Aug 1, 2023 •

edited