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

Add OCI recommended annotations #196

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

samj1912
Copy link
Member

@samj1912 samj1912 commented Dec 3, 2021

Signed-off-by: Sambhav Kothari skothari44@bloomberg.net

Readable

@samj1912 samj1912 requested a review from a team as a code owner December 3, 2021 19:07
@buildpack-bot
Copy link
Member

Maintainers,

As you review this RFC please queue up issues to be created using the following commands:

/queue-issue <repo> "<title>" [labels]...
/unqueue-issue <uid>

Issues

(none)

@imjasonh
Copy link
Member

imjasonh commented Dec 5, 2021

There's an important distinction between labels and annotations. OCI recommends these annotations, but AIUI images in the daemon can't (currently?) be annotated, only labeled.

Talking to @ekcasey about this a few months ago it sounded like buildpacks would have to become more remote-image-centric in order to propagate annotations through the build. Or maybe the annotations only if the build is doing --push?

That being said I'm +1 on the change, and I'd love to help however I can.

@samj1912
Copy link
Member Author

samj1912 commented Dec 5, 2021

There's an important distinction between labels and annotations. OCI recommends these annotations, but AIUI images in the daemon can't (currently?) be annotated, only labeled.

Talking to @ekcasey about this a few months ago it sounded like buildpacks would have to become more remote-image-centric in order to propagate annotations through the build. Or maybe the annotations only if the build is doing --push?

That being said I'm +1 on the change, and I'd love to help however I can.

@imjasonh all your help is appreciated! I think I got a bit confused with the b/w compatibility clause at https://github.com/opencontainers/image-spec/blob/main/annotations.md#back-compatibility-with-label-schema

but reading it more closely it looks like it recommends not just changing the domain but also moving from labels to annotations. Happy to update the rfc to use annotations and making it conditional on registry pushes directly.

That being said, we have recently been facing more and more restrictions because of the daemon use case. Especially with things like cosign/sboms etc. I have been thinking about writing an RFC about deprecating the daemon flag and instead moving to OCI layout and registry. I could really use some help there if you are interested in the RFC. Your experience/knowledge with GGCR and OCI would be really helpful! Some more details at #197 (comment)

Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
@samj1912
Copy link
Member Author

samj1912 commented Dec 7, 2021

@imjasonh - updated to annotations conditional on the registry.

@samj1912 samj1912 changed the title Add OCI recommended labels Add OCI recommended annotations Dec 7, 2021
@imjasonh
Copy link
Member

imjasonh commented Dec 7, 2021

@imjasonh - updated to annotations conditional on the registry.

Thanks, this looks great to me. 👍

Comment on lines +44 to +45
- `org.opencontainers.image.authors`: Authors of hte image. Sourced from `source.metadata.authors`.
- `org.opencontainers.image.version`: Authors of hte image. Sourced from `source.metadata.version`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `org.opencontainers.image.authors`: Authors of hte image. Sourced from `source.metadata.authors`.
- `org.opencontainers.image.version`: Authors of hte image. Sourced from `source.metadata.version`.
- `org.opencontainers.image.authors`: Authors of the image. Sourced from `source.metadata.authors`.
- `org.opencontainers.image.version`: Version of the image. Sourced from `source.metadata.version`.

Optionally adds the following annotations where applicable when lifecycle is used to publish images directly to the registry -

- `org.opencontainers.image.base.name`: Run image tag name. Currently stored in `io.buildpacks.lifecycle.metadata.run-image.reference`
- `org.opencontainers.image.base.digest`: Run image digest. Currently stored in `io.buildpacks.lifecycle.metadata.run-image.reference`
Copy link
Member

Choose a reason for hiding this comment

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

Does the run-image.reference data include both the mutable tag reference and the immutable digest?

Copy link
Member

Choose a reason for hiding this comment

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

Based on the spec this would be the immutable digest:

A digest reference to a manifest stored in an OCI image registry

Copy link
Member

@hone hone left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@hone hone requested a review from sclevine December 15, 2021 17:31

Optionally adds the following annotations where applicable when lifecycle is used to publish images directly to the registry -

- `org.opencontainers.image.base.name`: Run image tag name. Currently stored in `io.buildpacks.lifecycle.metadata.run-image.reference`
Copy link
Member

@natalieparellano natalieparellano Jan 3, 2022

Choose a reason for hiding this comment

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

I think this is actually io.buildpacks.lifecycle.metadata.stack.runImage.image

Optionally adds the following annotations where applicable when lifecycle is used to publish images directly to the registry -

- `org.opencontainers.image.base.name`: Run image tag name. Currently stored in `io.buildpacks.lifecycle.metadata.run-image.reference`
- `org.opencontainers.image.base.digest`: Run image digest. Currently stored in `io.buildpacks.lifecycle.metadata.run-image.reference`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `org.opencontainers.image.base.digest`: Run image digest. Currently stored in `io.buildpacks.lifecycle.metadata.run-image.reference`
- `org.opencontainers.image.base.digest`: Run image digest. Currently stored in `io.buildpacks.lifecycle.metadata.runImage.reference`

@hone hone requested a review from ekcasey January 12, 2022 19:44
@hone
Copy link
Member

hone commented Jan 12, 2022

@sclevine @jkutner @ekcasey ready for review please 🙏

Copy link
Member

@ekcasey ekcasey left a comment

Choose a reason for hiding this comment

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

I am of the opinion that we should table this and come back to it after implementing a proposal to fix the only source of complexity here and remove lifecycle->daemon support.

# Drawbacks
[drawbacks]: #drawbacks

In case of the daemon mode, the annotations cannot be applied and will be ignored.
Copy link
Member

Choose a reason for hiding this comment

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

Up until this point we have endeavored to make the output image identical regardless of export target (registry vs daemon). I am leary of introducing behavior differences between the daemon and remote cases. IMO it would be much better to remove the daemon as an export target first before continuing with this RFC.

Copy link
Member

@jromero jromero Jan 20, 2022

Choose a reason for hiding this comment

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

I would agree if the registry and daemon cases were identical to begin with but they are already not. Things like io.buildpacks.lifecycle.metadata.runImage.reference change depending on whether it's built for daemon or registry. The same inputs for an image don't survive round-tripping because of that and the difference in compression of layers. In other words, lifecycle -> daemon -> registry != lifecycle -> registry. IMO, it doesn't sound reasonable to block this feature on the eventual hope that we'll get rid of the daemon. At the end of the day, the core concern (discrepancy) will always exist but be pushed onto the platform.

Copy link
Member

Choose a reason for hiding this comment

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

IMO, it doesn't sound reasonable to block this feature on the eventual hope that we'll get rid of the daemon.

In order to get rid of the daemon instead of merely hoping it goes away I think we need to hold the line somewhere and actually prioritize getting rid of it. Adding remote-only features seems like the right place to draw the line to me.

Yes there is a path forward without removing the daemon first, but it is uglier and requires more compromises in the lifecycle and imgutil and caveats in our docs and specs. Instead we could invest in removing the daemon and clean up some of the uglier parts of the project.

- `org.opencontainers.image.base.name`: Run image tag name. Currently stored in `io.buildpacks.lifecycle.metadata.run-image.reference`
- `org.opencontainers.image.base.digest`: Run image digest. Currently stored in `io.buildpacks.lifecycle.metadata.run-image.reference`

The platform may also provide appropriate keys inside `project-metadata.toml` to the lifecycle so that it exports the appropriate annotations listed below.
Copy link
Member

Choose a reason for hiding this comment

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

In the ideal world I think these project metadata annotation would be better contributed by a buildpack vs by the lifecycle for the following reasons:

  1. Buildpack contributed behavior more easily upgradable accross a variable of platforms
  2. Keeping the surface area of the platform API as small as possible makes the spec and the conditional logic in lifecycle less complicated, thus making our lives easier and enabling us to actually make progress shipping things.

The way I imagine it:

  • step 1 remove daemon
  • step 2 allow buildpacks to contribute aribtrary annotations
  • step 3 provide a (utility?) buildpack that contributes these annotations based off the values in project.toml

Copy link
Member

Choose a reason for hiding this comment

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

That ordering makes sense to me. Is there a rough plan/timeline for step 1?

Copy link
Member

Choose a reason for hiding this comment

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

It ties into this RFC issue: #197

@jjbustamante is on it!

Copy link
Member

@jromero jromero Jan 20, 2022

Choose a reason for hiding this comment

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

Based on my comment here, I'm back to say that I don't think step 1 is necessary.

steps 2 and 3 can be an alternative approach to this RFC without step 1. I'd love to see it comparatively in the Alternatives section.

Copy link
Member

Choose a reason for hiding this comment

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

@ekcasey
Copy link
Member

ekcasey commented Feb 4, 2022

@samj1912 I am convinced that we can approve a version of this without blocking on the daemon removal. However I still feel pretty strongly that we shouldn't make the lifecycle aware of these specific annotations, but should instead:

  1. Provide a mechanism whereby a buildpack can set abitrary annotations
  2. Implementing this as a utility buildpack

Are you open to revising this RFC to make it address buildpack-provided annotations instead of addressing OCI specified annotations specifically?

@samj1912 samj1912 marked this pull request as draft February 16, 2022 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants