imagetools: Allow annotations for OCI image index - #1965
Conversation
6d283d0 to
42ee388
Compare
eiffel-fl
left a comment
There was a problem hiding this comment.
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.
42ee388 to
b071d65
Compare
|
@crazy-max @jedevc any thoughts on this? :) |
|
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: Would this kind of syntax work for you? I think we need consistency through buildx, so maybe good to continue the discussion there. |
|
For the sake of parity we should consider supporting labels as well in follow-up. |
Great
Agreed, we should have consistency through buildx. Also, I was already thinking how can we give user a hint that these annotations are for |
b071d65 to
e5e178f
Compare
|
@jedevc I have updated the PR to use the new syntax with |
|
I've opened #1978 that you can use as a base for adding integration tests for checking the |
e5e178f to
f8d9676
Compare
@tonistiigi great thanks. I updated the PR to include |
f8d9676 to
95b6a2f
Compare
95b6a2f to
11c0502
Compare
| indexAnnotations := make(map[string]string) | ||
| manifestDescriptorAnnotations := make(map[string]string) | ||
| for k, v := range ann { | ||
| groups := annotationRegexp.FindStringSubmatch(k) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>
11c0502 to
3ef93e0
Compare
jedevc
left a comment
There was a problem hiding this comment.
LGTM 🎉 Thanks @mqasimsarfraz!
|
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>
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>
…tion support Root cause: OCI annotations were not persisted to multi-arch manifest index, causing GitHub Container Registry to show "No description provided". The --annotation flag for docker buildx imagetools create requires BuildKit v0.12.0+ / Buildx v0.11.0+. The merge and release jobs were missing explicit BuildKit configuration, defaulting to older versions that silently ignored --annotation flags. Changes: - merge job: Add driver-opts with moby/buildkit:latest + network=host - release job: Add driver-opts with moby/buildkit:latest + network=host - Ensures consistency with build job which already had this configuration This fixes the verification step failure: ❌ ERROR: No annotations found on manifest index Related: - Fixes https://github.com/fluxo-kt/aza-pg/actions/runs/19348853813/job/55357635596#step:6:109 - Docker docs: https://docs.docker.com/build/metadata/annotations/ - BuildKit PR: docker/buildx#1965 Testing: Validated with yamllint and bun run validate (all checks passed)
…tion support Root cause: OCI annotations were not persisted to multi-arch manifest index, causing GitHub Container Registry to show "No description provided". The --annotation flag for docker buildx imagetools create requires BuildKit v0.12.0+ / Buildx v0.11.0+. The merge and release jobs were missing explicit BuildKit configuration, defaulting to older versions that silently ignored --annotation flags. Changes: - merge job: Add driver-opts with moby/buildkit:latest + network=host - release job: Add driver-opts with moby/buildkit:latest + network=host - Ensures consistency with build job which already had this configuration This fixes the verification step failure: ❌ ERROR: No annotations found on manifest index Related: - Fixes https://github.com/fluxo-kt/aza-pg/actions/runs/19348853813/job/55357635596#step:6:109 - Docker docs: https://docs.docker.com/build/metadata/annotations/ - BuildKit PR: docker/buildx#1965 Testing: Validated with yamllint and bun run validate (all checks passed)
Every fact an image release asserts — org.opencontainers.image.* config labels and index annotations — is now resolved once, in a facts job that runs before anything builds, and every image build consumes the map without deriving anything. Provenance facts (source, revision, version, created, licenses) are derived, validated and fail-closed, never caller inputs; title and description stay editorial with derived defaults, omitted rather than emitted empty. The licence value is what the OCI spec defines it to be — an SPDX licence expression — so the resolver validates grammar and id membership against a vendored spdx/license-list-data id list (offline and deterministic in the release path; audit:spdx-list is the staleness alarm). Precedence chooses which declaration speaks (workspace, package, then the licence API for manifest-less repos); the tiers are not cross-checked, because Licensee's heuristic flattens dual licences. A declared repository field must equal the derived source — that mismatch already kills npm trusted publishing at publish time, so it now fails in five seconds instead. The per-arch push exporter sets oci-mediatypes=true (a Docker manifest list has no annotations field and buildx drops index annotations silently, docker/buildx#1965; the buildx >= 0.12 floor is asserted, not assumed), and assert-image-facts.sh proves the published index at the existing pull-back points: OCI media type, index annotations and every per-arch config's labels EQUAL the map. The resolved commit epoch is also SOURCE_DATE_EPOCH, so no timestamp surface is wall clock. The artifact Dockerfile's LABEL block and its build-args are deleted — one mechanism, and its created was measuring a canon commit, not the caller's. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Carl Allen <36766173+CarlAllenn@users.noreply.github.com>
Part of #108 (closes on lab proof). Every fact an image release asserts — `org.opencontainers.image.*` config labels and index annotations — is now resolved once, in a `facts` job that runs before anything builds (`release/resolve-oci-facts.sh`), and every image build consumes the map without deriving anything. Provenance facts (source, revision, version, created, licenses) are derived, validated and fail-closed, never caller inputs; `title`/`description` stay editorial caller inputs with derived defaults, omitted rather than emitted empty. The guard is untouched: resolving is its own caller-code-free job, so the job whose identity is "nothing runs until this passes" stays `permissions: {}`. One resolution per release also means a multi-class repository cannot disagree with itself about revision, licence or created. The licence value is what the OCI spec defines it to be — an SPDX licence expression — so the resolver validates grammar and id membership against a vendored `spdx/license-list-data` id list (offline and deterministic in the release path; `audit:spdx-list` is the staleness alarm; `LicenseRef`, `NOASSERTION` and legacy `/` syntax are refused). Precedence chooses which declaration speaks — `[workspace.package].license`, `[package].license`, then the licence API for manifest-less repositories; the tiers are not cross-checked, because Licensee's heuristic flattens dual licences to a single id. A declared `repository` field must equal the derived `source`: that mismatch already kills npm trusted publishing at publish time, after images are pushed, so it now fails in five seconds with the remedy named. The per-arch push exporter sets `oci-mediatypes=true` — a Docker manifest list has no annotations field and buildx drops index annotations silently (docker/buildx#1965); the buildx >= 0.12 floor is asserted, not assumed — and `release/assert-image-facts.sh` proves the published bytes by digest at the existing pull-back points: OCI index media type, index annotations and every per-arch config's labels EQUAL the map. The resolved commit epoch doubles as `SOURCE_DATE_EPOCH`, so every timestamp surface is a function of one resolved value and none is wall clock. The artifact Dockerfile's LABEL block and its build-args are deleted — one mechanism for one fact, and its `created` was measuring a canon commit's timestamp, not the caller's (the resolver runs against the caller's checkout, so that bug is now unwritable). The per-PG title goes with it: strict equality, no editorial override plumbing, the major lives in the tag. Still owed before #108 closes, in the lab at full width: annotations surviving both index paths (imagetools create and the single multi-platform build), the signer and `gh attestation verify oci://` indifferent to the index media-type change, and the licence API's behaviour under `?ref=<sha>` for a manifest-less repository. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Carl Allen <36766173+CarlAllenn@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…63) Step 2 of the assert build (#39). Stacked on #62 (retarget to main when it merges). - **`internal/oci`** — the registry read seam: interface-shaped, production impl via go-containerregistry (no docker daemon, digest-validated pulls — the bytes judged are the bytes a stranger pulls). - **`internal/assert`** — the engine's first target: OCI index media type (the buildx silent-annotation-drop check, docker/buildx#1965), index annotations and every per-arch config's labels EQUAL the facts map with per-key findings, facts hygiene re-checked independently of the resolver, attestation manifests skipped, empty index → `CANNOT_JUDGE`. - **`stele assert image-facts`** — env contract (IMAGE, DIGEST, FACTS) unchanged from `release/assert-image-facts.sh`; `--json` emits the report document; exit 0/1/4 for PASS/FAIL/CANNOT_JUDGE. Cutover (canon-side swap + bash deletion) follows once this ships and shadow-proves against a live release. Refs #39. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Carl Allen <36766173+CarlAllenn@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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
--annotationtoimagetools createto allow adding OCI annotations.Related: #1171
Testing Done:
OCI Image Index (annotation added)
Manifest Descriptor
Docker manifest list (noop)