-
Notifications
You must be signed in to change notification settings - Fork 663
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 standard base image annotations #822
Conversation
09eb802
to
7eeb35c
Compare
After talking it over with @jonjohnsonjr, he convinced me that it would probably be better to allow multiple digests and refs. This enables potential use cases like marking the version of the The latest draft also references a semi-official grammar for what a "reference" is (linking to here), but it'd be great to get that more formalized in distribution-spec instead of in a comment in some Go code. When/if that happens we can update this to point there instead. Lemme know what you think! 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need some finality with this comment I think: #821 (comment)
It might be worth putting that in the spec itself. Something like "These annotations MAY contain references or digests of manifests that are shared with an ancestor or references or digests of manifests that are used in producing build artifacts." This might not be the best way to phrase it, but I think having some indication of what the data could be would be worthwhile. |
Some other versions:
Is any of that more/less clear about the intention? I really don't want to be even a little prescriptive about what "derived from" means, because it might end up being very content-dependent and new ideas might show up over time. |
My concern is that once you add something to the image that says "this image has some relation with these other images" then the question becomes "what is the relation and how do I find out?". Personally, I think this lies in the realm of an SBoM not the image spec. |
As discussed on the call, I like the idea of a base image annotation. I'd just suggest we tighten the scope of exactly what it means and how to use it; For the annotation to be generally useful, it MUST have a strict definition so it can be used consistently across all uses. How can someone say their tooling around the annotation isn't working because an image isn't using the annotation properly? |
Yeah, @SteveLasker I think I'm coming (back 😛 ) around to your viewpoint here. It'd be great to have annotations that describes all the images, etc., that were used to produce this image, and for whom changes to those images might be interesting and trigger a rebuild/rebase, but for the specific use cases described in #821, we can get by with a simpler more narrowly-scoped annotation. That lets us more narrowly specify what the annotation values should be and mean. It also reduces the complexity of having to describe how two parallel lists of values should interact, which I think is useful. If folks want a more general-purpose "this image is somehow related to [these images]" annotation, that could always be a separate annotation, or wrapped up into ongoing SBoM work somehow. Either way, that seems like a reasonable separate proposal, with fewer specified guarantees about what the annotation values mean. If there aren't any strong objections I can change this back to the single-value forms, with more specifics about what the values should be. |
Updated the PR to reflect the latest language around single base images. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can address the feedback for examples, I would approve this narrow scope of base reference.
This PR was discussed at the March 17 TOB meeting (thanks for brining it up @jonjohnsonjr and sorry again I couldn't attend). I'd like to try to summarize the discussion so not everyone has to watch the video. Basically, there seem to be two camps forming:
To be clear, I absolutely want both, and I think there's value in being able to easily signal "an update to I think the argument for (1) is that it's a smaller, simpler, focused change to make to the spec, and can be defined more narrowly and easily. Basically every image has a single "base image" under that definition of a "base image" (i.e., "shares zero-indexed layers") -- if it's (2) is much more powerful and flexible, but as a result can make it hard to consume effectively. Tooling that can understand multiple "bases" (I think this term is bad when used this way) can have potentially many different actions to take when those bases change. When From a practical standpoint, (2) requires us to define "base image" very vaguely to include supporting images in a multi-stage build for instance, and I don't think that's a way the term is used in common practice. We could change the annotation to Again, to be clear, I definitely want (2) to exist, and for systems built on (2) to be defined and built, I'm just not sure I want to be the one to define them. 😄 I just want (1) to exist and to build tooling that consumes it. cc @SteveLasker @jonjohnsonjr @samuelkarp @vbatts who were involved in the discussion and can fact-check my summary |
I think there's two orthogonal issues:
For the first, I'm okay with only allowing a single "base image" per annotation. I can hack around this. For the second, I'm less okay with defining exactly what a "base image" means. I'm fine with us referencing the common usage of "base image" as referring to the contiguous zero-indexed layers (via MAY language), but I think this being a SHOULD is a little strong. It's encoding a lot of docker-specific implementation details that aren't really required for the usefulness of this annotation. It's also trivial to check whether or not the base image does conform to that pattern, so there is no loss of information. Any use case that only applies to Dockerfile-style base images can do a very simple check before performing the operation. In fact, we might want to add language that tools SHOULD verify that the base image matches their expectations before taking any action. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general framing I have around annotations are:
- is the value intended for humans to read/parse and possibly do something
- is the value intended for computers to parse, which should be very specific. We shouldn't need an AI/ML process to iterate all the possible permutations to understand the intent.
If the value is for humans, it's less important, because humans are very good about inferring meaning, and there's no automation. If it's for automation, we should be very specific and actionable for public software and any user to automate around.
If it's for specific companies then Acme Rockets can create their own annotations and use them any way they'd like.
Putting the grammar info in the ref-name seems less important than an actual example, as the example implies whether this is intended to be a fully qualified registry name (docker.io/library/ubuntu), a default docker hub reference (ubuntu - please don't as we need to kill the default docker hub concept), or a human value that says ubuntu
but is only interesting for the reader to think, oh, that's the ubuntu image, but I don't know where exactly they got it from, or where I monitor if it's still this digest.
So, how do we expect users to use these annotations - automation or human interesting, but not necessarily ready for automation?
If this example is insufficient:
...could you suggest one that you'd find more clear?
I'd like to aim to use these annotations for automation, and have these annotations targeted at computers. From the initial proposal, use cases include, in order of increasing complexity/utility:
(bonus points in all cases if the old base has a CVE that the new base doesn't) Another annotation, or series of annotations (or maybe |
Correct me if I'm wrong, but in this week's meeting, it almost sounded like we were approaching rough consensus about this. (The recording's not up yet, you'll have to take my word for it 😆 ) If I can summarize the final sticking points:
I don't know what the process is for calling for a vote of "close enough", but in the absence of any better ideas, and in the presence of what seems like mostly positive reception, I'd like to hereby call for a vote of "close enough", and kindly ask for approval. 🙏 |
While the calls and feedback on this issue from community members is valuable, ultimately that decision falls to the maintainers of the image spec. |
LGTM. For those looking to write different tooling with different goals, there's always the option to add their own annotation. This optional annotation covers a large number of users and use cases that makes sense to provide a standard. |
FYI I'm still hoping to get this approved and merged, it's mostly in a holding pattern while opencontainers/tob#95 is being resolved and new maintainers identified and sworn in. My understanding is this will still only need 2+ approvers from among those maintainers once they've been identified, someone please let me know if that's not the case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to add these strings here: https://github.com/opencontainers/image-spec/blob/master/specs-go/v1/annotations.go
Based on discussion with @jonjohnsonjr I've added another point describing how the annotations should be used when the base image is an image index (i.e., a multi-platform image like This is important because tooling that wants to detect and perform rebuilds/rebases needs to be able to detect whether the thing referenced by tag and digest has diverged, and the only way to do that is to have them point to the same thing. In the case of multi-platform images, the platform-specific image within the index ( |
Signed-off-by: Jason Hall <[email protected]>
Signed-off-by: Jason Hall <[email protected]>
Friendly ping for more reviews and approvals from image-spec maintainers 🤞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'm not a maintainer but this has my LGTM :) |
Since opencontainers/image-spec/pull/822/ the OCI spec supports two new annotations to set the fully-qualified name and the digest of the base image. Signed-off-by: Valentin Rothberg <[email protected]>
Since opencontainers/image-spec/pull/822/ the OCI spec supports two new annotations to set the fully-qualified name and the digest of the base image. Signed-off-by: Valentin Rothberg <[email protected]>
Since opencontainers/image-spec/pull/822/ the OCI spec supports two new annotations to set the fully-qualified name and the digest of the base image. Signed-off-by: Valentin Rothberg <[email protected]>
Fixes #821 and #783
As discussed in yesterday's call, the next step was to open a PR to nail down specific wording.