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

Handle multiple matching index entries #880

Merged
merged 1 commit into from
Jan 14, 2022

Conversation

sudo-bmitch
Copy link
Contributor

@sudo-bmitch sudo-bmitch commented Nov 30, 2021

The goal is to handle an index where there are multiple entries that look the same to older clients, but a newer client may see a feature it supports in the annotations. This would allow an SBoM to be packaged as an OCI-artifact and pushed in an index along side the image. It would also give a way to support different layer encodings for the same image without breaking older clients.

I say "SHOULD", but if there are no use cases where this change would break, then I'd be tempted to say "MUST".

Fixes #862, fixes #776

Signed-off-by: Brandon Mitchell git@bmitch.net

@vrothberg
Copy link
Contributor

LGTM

I say "SHOULD", but if there are no use cases where this change would break, then I'd be tempted to say "MUST".

Wouldn't "MUST" break use cases where clients would discriminate multiple matching entries via annotations? Annotations are sometimes used, for instance in HPC, to add information on compiler flags. A client may look for these annotations and select the ones optimized for the local platform. Using MUST would break that use case since annotations are not part of the platform.

@sudo-bmitch
Copy link
Contributor Author

Wouldn't "MUST" break use cases where clients would discriminate multiple matching entries via annotations? Annotations are sometimes used, for instance in HPC, to add information on compiler flags. A client may look for these annotations and select the ones optimized for the local platform. Using MUST would break that use case since annotations are not part of the platform.

This is certainly a use case I'm thinking this would support. I didn't call out what the "client or runtime's requirements" were in that line, so my assumption is these would also include annotations if a client was created that looked for annotations (or other fields in the descriptor).

Perhaps better phrasing may be "if multiple manifest entries are effectively equivalent to the client" and newer clients that handle features identified in annotations wouldn't see the entries as equivalent. Happy to find the best phrasing that gets across this idea.

@vrothberg
Copy link
Contributor

vrothberg commented Nov 30, 2021

The current phrasing looks good to me since clients can still decide to not pick the first matching entry if they have good reasons for it.

@SteveLasker
Copy link
Contributor

I agree with the wording in the PR. But, I struggle with the example of an SBoM in an index. This is the forward and reverse indexing topic that's part of the Reference Type working groups.

@sudo-bmitch
Copy link
Contributor Author

I agree with the wording in the PR. But, I struggle with the example of an SBoM in an index. This is the forward and reverse indexing topic that's part of the Reference Type working groups.

The Reference Type proposals would be a welcome addition and would likely handle more scenarios than injecting the artifact along side the image in an index's manifest list. However it will be some time before that is approved and available in all the registries I work with, so I'm looking at intermediate solutions that allow me to package artifacts with an image today. The other option would be something like the cosign model of a tag that's a digest and short type, but those tags clutter the tag listing in a less than ideal way.

stevvooe
stevvooe previously approved these changes Dec 2, 2021
Copy link
Contributor

@stevvooe stevvooe left a comment

Choose a reason for hiding this comment

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

LGTM

@sajayantony
Copy link
Member

sajayantony commented Dec 2, 2021

Should we handle this in down conversion in distribution conformance? @jdolitsky

@jdolitsky
Copy link
Member

@sajayantony yes, but prior to that we need to be testing indexes in general (larger issue)

@@ -83,6 +83,8 @@ For the media type(s) that this document is compatible with, see the [matrix][ma

This property is RESERVED for future versions of the specification.

If multiple manifests match a client or runtime's requirements, the first matching entry SHOULD be used.
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
If multiple manifests match a client or runtime's requirements, the first matching entry SHOULD be used.
If multiple manifests match a client or runtime's `platform` requirements, the first matching entry SHOULD be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was intentionally leaving that out since some clients may have different requirements, like checking annotations to see if a different layer compression format is available.

Signed-off-by: Brandon Mitchell <git@bmitch.net>
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM.

@cyphar cyphar closed this in 8b9d41f Jan 14, 2022
@cyphar cyphar merged commit 8b9d41f into opencontainers:main Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants