Add annotations support to output

[jedevc]

Fixes #1220

This PR introduces support for creating and attaching annotations to the exporter. These annotations can be specified via frontends, or via the exporter output parameters (with the latter overriding the former).

For now, this is just the containerimage exporter, but we should probably add it to the other exporters as well, at the very least the oci exporter 👀

[tonistiigi]

For now, this is just the containerimage exporter, but we should probably add it to the other exporters as well, at the very least the oci exporter

Yes, we should remove the duplication in here. The imagewriter part is already shared among these exporters but we should also do a shared method for parsing the image-specific attr options and putting them to struct that can be passed to the imagewriter. Maybe we can do this on a separate PR already and then make this PR use it.

I still think we need separation between annnotations, descriptor annotations, and OCI root index annotation. For example signature annotations would need to be on descriptors.

Other than that you can also start working on the test coverage.

[jedevc]

Think this is getting closer now, need to start working on some test coverage, but think the shape of the code is about right.

I still think we need separation between annnotations, descriptor annotations, and OCI root index annotation.

Regarding annotations to OCI root indexes, personally I think we might want to explicitly not support this scenario - as mentioned in in #1220, it's fairly rare to see annotations attached like this. Additionally, if more than one platform is not supplied to buildkit, an image index isn't generated - this means that whether that annotation actually gets used is a matter of what platforms are built, which seems like a confusing user experience.

I think if we support annotation.<x> and annotation-descriptor.<x> we should be good for most use cases - we could always come back and add support for the OCI root indexes (using something like annotation-index.<x>) later down the road if it becomes apparent that such a use case is necessary.

Alternatively, is there any reason we couldn't switch to always producing indexes, even when there's only a single platform? If we're interested in signing entire indexes, then this would probably be a precursor to being able to do that.

we can just let user define all annotations with unique key. -o annotation.<key>=value

Do we want to support setting arch-specific annotations for opts specified at the command line, or just allow that for frontends? If so, what would the option look like? I was thinking something like annotation-<arch>.<key>=<value>, but then we'd probably want to update the internal frontend ref.annotation syntax to match. Although that might clash with doing annotation-descriptor.

[jedevc]

I've gone and added a test for the OCI and ContainerImage annotations, which tests annotations set from both the frontend and from options. It's quite a long test, so let me know if it needs to be split up a bit more, or if it should be in a different part of the codebase.

R.e. the two points above - I've implemented annotations on the image index (with the noted caveats), and only allowed annotations to the image manifests when using options.

[jedevc]

Have hopefully addressed the comments - except for the one about updating docs, will try and get to that one in a little bit.
Otherwise, should be ready for another review.

[johnsonshi]

Hi @jedevc, pardon me as I am not quite familiar with the exporter code. You mentioned These annotations can be specified via frontends, or via the exporter output parameters (with the latter overriding the former).

Does that mean the work in this PR will be progress towards the following scenario?

With this PR on exporter code (+ I guess future PR on the frontend/client), can a cli user can specify custom annotations such as the following?

docker buildx build \
  --annotation 'org.opencontainers.image.authors=authorName' \
  --annotation 'org.opencontainers.image.licenses=Apache' \
  -f Dockerfile -t myimage:latest .

[jedevc]

Yup this is progress towards that exact scenario - the final syntax of how this will look in buildx is still TBD, but probably quite similar to what you describe 🎉

[tonistiigi]

Could define a type for annotationType for extra safety. Keep private if nothing outside uses it.

[jedevc]

We can do this with type annotationType string but it doesn't add much extra safety since go will allow implicitly converting between string and annotationType (because it's an alias)
If we create a struct to do this and actually construct a new type, then switch-statements no longer work.

[tonistiigi]

Although this isn't an LLB feature, add a capability for it in https://github.com/moby/buildkit/blob/master/solver/pb/caps.go in case there is a need for a client to detect if this feature is supported.

[JcJinChen]

Yup this is progress towards that exact scenario - the final syntax of how this will look in buildx is still TBD, but probably quite similar to what you describe 🎉

It looks cool. I wonder when this feature will be available, such as add "annotation" flag. Thanks. I test the code from master branch. This looks like a failure.

[jedevc]

Heya @JcJinChen, the feature is currently available on buildkit master. However, the proposed syntax using the --annotation flag is not yet introduced - to track that, see docker/buildx#1171 - any comments or suggestions you might have would be really helpful. Because adding annotations using the docker builder so far hasn't been possible, we're not quite sure how users will use this, and how we should optimize for the default experience.

I've added some docs in #2975 to better explain how to use the feature as-is - if you're using buildx instead of buildctl directly you just need to add the annotation.* options to your --output flag on the buildx command line.

Hope that helps 🎉