#version check impl details

1 messages · Page 1 of 1 (latest)

nova osprey
#

bleh

#

so, the index annotations not supported error is technically true 😦
the right call here i think is to try to attach to the manifest if we have only one platform, and to the index if we have multiple? does that work?

#

ideally there would be a way to do this in buildkit (i'll add this to my long list of things to maybe in bk)

hot phoenix
#

The thing I don't quite grasp is that we DO have multiple platforms on our image, don't we?

nova osprey
#

Not sure about Container.WithManifestAnnotation, I think WithAnnotation should be good? i really don't love that there are so many places to attach annotations in oci

#

but yes, there is a chicken-and-egg problem

#

😢 no real way around it

hot phoenix
nova osprey
nova osprey
nova osprey
hot phoenix
#

@nova osprey For the actual annotation part ...

#

Currently doing a:

            key := exptypes.AnnotationManifestDescriptorKey(&platformSpec, annotation.Key)
            opts[key] = annotation.Value

(for each variant)

nova osprey
#

ugh, the annotation api is a totalmess (i made it, such regret)

hot phoenix
#

If I try to use exptypes.AnnotationIndexKey() or AnnotationIndexDescriptorKey I get:

Error: response from query: input: failed to export: index annotations not supported for single platform export

nova osprey
#

yeah, so you should be able to have a check for len(platformVariants) - the published images would then have it set at the top-level on the index, and the other case you can do what you're currently doing

hot phoenix
#

let me clean this up and open a separate PR, will make life easier

nova osprey
#

technically this check is bad, it should also include manifest descriptors probably

#

but bleh

hot phoenix
hot phoenix
nova osprey
#

oh if this in pub lish, then i'm not sure

hot phoenix
nova osprey
hot phoenix
nova osprey
#

ah

#

not every image we publish is multi-platform

#

for example, the gpu variant is only published for linux/amd64

#

I think that could be causing it

hot phoenix
#

oh ok

#

let me try with the check

#

@nova osprey oh btw -- I should be using the Descriptor variant of the exptypes right?

e.g. AnnotationIndexDescriptorKey and AnnotationManifestDescriptorKey ?

nova osprey
#

i don't think so, we want to attach at the top-level of the manifest/index

hot phoenix
#

@nova osprey uhm ... another thing: if there ARE multiple platforms, don't we have the risk of them having different annotations?

Since Publish takes []Containers for variants?

nova osprey
#

oh you're not wrong

hot phoenix
#

Alternatively instead of Container.WithAnnotation it would be Publish taking a list of annotations

nova osprey
#

sorry i'd forgotten that

nova osprey
#

i think we don't want to move more stuff into Publish - e.g. we don't want to exacerbate the problem with needing a bunch of extra stuff you can't pass around modules

#

but yeah this is tricky

hot phoenix
hot phoenix
#

(err -- basically, keep WithAnnotation and then go with 1 or 2)

nova osprey
#

index-descriptor is an insane edge case, since it can't actually be pushed to the registry

#

we can put it in manifest-descriptor, and the manifest actually

#

that way, it's in the index, and gets caught for multi-platform

#

and it's in the manifest, for single-platform

hot phoenix
#

i don't think so, we want to attach at the top-level of the manifest/index

manifest descriptor puts it in the index, in the descriptor, right next to the annotations for each manifest

So we DO want it in the manifest descriptor, right?

#

(e.g. that's the top-level?)

nova osprey
#

yeah, i think that works

#

i think doing both is probably the least painful for implementation, and is probably not too unexpected for users

#

then we don't need the conditional about number of platforms we were talking about

hot phoenix
nova osprey
#

👍 exactly

hot phoenix
nova osprey
#

if there's demand to have it on the index, we can add it as an option in the future

hot phoenix
#

👍

#

updating

#

done

hot phoenix
#

@nova osprey UGH ... this is never ending

Is there a chance our registry in the unit test suite doesn't support annotations?

#

I'm not getting any annotations back

Tried to push instead manually to aluzzardi/test:v0.12.0 (on docker hub) and I can grab the annotations back using the same code of the unit test

nova osprey
#

oh no

#

uh that's wild

#

that would be weird, i don't think any registries should be doing that

#

is this TestManifestAnnotations?

hot phoenix
#

a modified version of that, but yeah

hot phoenix
# hot phoenix I'm not getting any annotations back Tried to push instead manually to aluzzard...

I pushed to the hub and then tried this:

package main

import (
    "fmt"
    "net/http"
    "os"

    "github.com/google/go-containerregistry/pkg/name"
    "github.com/google/go-containerregistry/pkg/v1/remote"
)

func main() {
    testRef := "aluzzardi/test:v0.12.0"

    parsedRef, err := name.ParseReference(testRef, name.Insecure)
    if err != nil {
        panic(err)
    }

    imgDesc, err := remote.Get(parsedRef, remote.WithTransport(http.DefaultTransport))
    if err != nil {
        panic(err)
    }

    img, err := imgDesc.Image()
    if err != nil {
        panic(err)
    }

    manifest, err := img.Manifest()
    if err != nil {
        panic(err)
    }

    fmt.Fprintf(os.Stderr, "%+v\n", manifest.Annotations)

}
#

for which I get:

map[org.opencontainers.image.version:v0.1.2]
#

the same print in the unit test gives me map[] 😦

nova osprey
#

hm, if for the test you set org.opencontainers.image.version does it work?

#

instead of io.dagger.foo

#

maybe something might be filtering the annotation away?

hot phoenix
#

Nah, but I'll try anyway

#

I was using io.dagger.version before and it worked (on the hub, not unit test)

#

recently switched to org.opencontainers.image.version

hot phoenix
#

@nova osprey same same 😦

nova osprey
#

what

#

hmm, we could try subbing in a different registry to the test? but also, we use these in buildkit upstream, never seen anything like this

hot phoenix
#

@nova osprey uh, got it

hot phoenix
#

and for some reason this doesn't work for single platform :?

#

For multi here's the index, with manifests containing annotations:

{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.index.v1+json",
  "manifests": [
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest": "sha256:dee65e4471e8aff605f19f1af69e78f16838f040eaf84cde4aa3ad4328270772",
      "size": 5908,
      "annotations": {
        "org.opencontainers.image.version": "v0.1.2"
      },
      "platform": {
        "architecture": "amd64",
        "os": "linux"
      }
    }
#

single however lacks the annotations:

{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.manifest.v1+json",
  "config": {
    "mediaType": "application/vnd.oci.image.config.v1+json",
    "digest": "sha256:f9ffcc1aa1e4871f64c47a55bab18e66fe245cbf0b99caff945b1078eec5673e",
    "size": 8755
  },
  "layers": [
nova osprey
nova osprey
nova osprey
hot phoenix
#

it doesn't though ... maybe I shouldn't be using both?

#

trying

        for _, annotation := range annotations {
            if len(platformVariants) == 0 {
                opts[exptypes.AnnotationManifestKey(&platformSpec, annotation.Key)] = annotation.Value
            } else {
                opts[exptypes.AnnotationManifestDescriptorKey(&platformSpec, annotation.Key)] = annotation.Value
            }
        }
#

same

# 
    fmt.Printf("%s\n", imgDesc.Manifest)

{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.manifest.v1+json",
  "config": {
    "mediaType": "application/vnd.oci.image.config.v1+json",
    "digest": "sha256:62fffa44596631db76264bd72e75cf255484e7516c50b7c9ad39dc022d20dc7f",
    "size": 430
  },
  "layers": [
    {
      "mediaType": "application/vnd.oci.image.layer.v1.tar+gzip",
      "digest": "sha256:690e87867337b8441990047e169b892933e9006bdbcbed52ab7a356945477a4d",
      "size": 4086934
    }
  ]
}
#

(in the unit test)

nova osprey
#

okay, i'm not entirely sure why that is.
it should be there. i would probably add some prints/etc in the buildkit code and inspect why it's not doing that

i can dive a bit more in tomorrow, but working on cutting the v0.13.0 release rn

hot phoenix
#

🙏

#

@nova osprey Oh, last thing:

Do you happen to know the difference between passing the annotations directly to in exporter.Resolve (e.g. expInstance, err := exporter.Resolve(ctx, 0, opts)) vs calling AddMeta?

nova osprey
#

i think they should end up in the same place

#

but they get propagated from different places

#

can't quite remember off the top of my head

hot phoenix
#

Same result with AddMeta ... help appreciated tomorrow 🙂

spice inlet
#

@nova osprey @hot phoenix some findings:

so, I created an image with this command:

docker buildx build . --output "type=image,name=ttl.sh/marcos:6,annotation.foo=bar,push=true"

Which AFAIK it's very similar to what we do in Dagger. As we call the image exporter with the publish flag in true when we publsih an image.

this is the resulting buildkit index manifest:

{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.index.v1+json",
  "manifests": [
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest": "sha256:ae055e0221b421f486348a2e1cf8d2f7a38af8549be06a2ee71234cb877152e6",
      "size": 521,
      "platform": {
        "architecture": "amd64",
        "os": "linux"
      }
    },
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest": "sha256:6028d49116f2227b58d6cb653cd9055d0b98ddb11069db8e90b3764e53eeed38",
      "size": 565,
      "annotations": {
        "vnd.docker.reference.digest": "sha256:ae055e0221b421f486348a2e1cf8d2f7a38af8549be06a2ee71234cb877152e6",
        "vnd.docker.reference.type": "attestation-manifest"
      },
      "platform": {
        "architecture": "unknown",
        "os": "unknown"
      }
    }
  ]

^ there's two things to note here:

  1. The annotation is not there.
  2. The index contains two manifest. One for the actual container image with the proper platform object, and a second one with an attestation manifest with an unknown platform
#

Now, if I call the command like this (note the annotation-index change):

docker buildx build . --output "type=image,name=ttl.sh/marcos:7,annotation-index.foo=bar,push=true"

here's the resulting manifest:

{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.index.v1+json",
  "manifests": [
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest": "sha256:465f515dfba9a241c3af16b4154e4161db1e8de1055b753434874158e7ae05d9",
      "size": 480,
      "platform": {
        "architecture": "amd64",
        "os": "linux"
      }
    },
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest": "sha256:36ab7585dd7e22bb809d4f915632b4be02b8857c49e8bce9e457b0f76af8a83d",
      "size": 565,
      "annotations": {
        "vnd.docker.reference.digest": "sha256:465f515dfba9a241c3af16b4154e4161db1e8de1055b753434874158e7ae05d9",
        "vnd.docker.reference.type": "attestation-manifest"
      },
      "platform": {
        "architecture": "unknown",
        "os": "unknown"
      }
    }
  ],
  "annotations": {
    "foo": "bar"
  }

^ you can see the annotations object in the image index

#

I haven't dug deeper but my theory at this stage is that the previous error "single platform" error that we were getting before is because as we're calling the exporter manually, we're probably not passing the attestation manifest as buildctl is currently doing and that's why the exporter is complaining

nova osprey
#

this is the difference between manifest and manifest-descriptor - manifest descriptor sets it on the index, in the descriptor for that manifest

hot phoenix
nova osprey
#

gimme 5, currently writing up some investigation into mark's issue

hot phoenix
#

no rush!

#

doing other things atm

spice inlet
nova osprey
#

the annotations shouldn't be added to the attestation manifest (which is the one with unknown/unknown)

#

digging in properly now

#

think i've found it, testing my theory now

#

@hot phoenix am I good to force push? I've rebased as well.

hot phoenix
#

Yep!

nova osprey
#

pushed

#

the annotations api is horrid, looks like we need nil platforms 😛

#

also added tests for single vs multi platform cases

hot phoenix
#

Woot!! Thank you!!

#

Wrapping up lunch, will check it out in a minute

spice inlet
#

and I thought that that was the reason we were getting that "single platform" error

nova osprey