-
Notifications
You must be signed in to change notification settings - Fork 677
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
Working Group Proposal for Reference Types #934
Conversation
OHMAN |
artifact.md
Outdated
|
||
Also, see [Pre-Defined Annotation Keys](annotations.md#pre-defined-annotation-keys). | ||
|
||
User defined annotations MAY be used to filter various artifact types, e.g. signature public key hash, attestation type, and SBOM schema. |
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.
As the spec allows user defined annotations, do we want to cap the number of annotations stored or returned as part of referrers
API ?
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 current proposal is that all annotations would be returned. If we were to trim down the list, then we need to define the criteria. For example, only the first 10 annotations would be returned. That still doesn't cap the size of the payload since a single key could actually be large enough.
I think it would be good to raise this issue in the distribution spec since the referrers response payload and registry implementations needs to now ensure that data from all manifests are returned. opencontainers/distribution-spec#335 /cc @michaelb990
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 index we return is rather minimal, so having enough annotations to cause this to exceed the recommended 4MB limit is likely to also exceed that limit in the referrer's manifest too. I think any limits we impose should be set on the original manifest, and not this API, to minimize complexity.
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.
Lets say worst case, there are like 20 artifacts in the referrers API reponse and each had say about ~3MB of annotations, then the API reponse payload would be very large. In these cases what should the API return? May be capping the reponse size per artifact?
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.
At that point, the response would trigger pagination, with one descriptor per page. It also wouldn't be supported to include more than one of those descriptors in the tag schema fallback. My hope is that manifests like that result in a bad enough UX, that bug reports get opened on the source and alternatives are used.
The downside of capping what is returned in the API response, is that any partial descriptor would then need to be pulled as a full manifest to determine if the client side filters match (the filtered annotation could be one that was trimmed). We would also need to provide an indicate that the descriptor is a partial response so clients know when they need to fall back to pulling the full manifest directly. When thinking of client implementations, that would probably be one pull for the API, one of the manifest by the artifact listing method, and then another pull of the manifest by the artifact get method, unless the manifest is cached between those methods.
@opencontainers/image-spec-maintainers given the size of this, we are giving this 2 weeks for any objections before merging. It was presented on the OCI call today and hopefully a recording will be posted soon. After merging it still won't be a tagged release, so if there are any issues raised, we can always open another PR. |
We would kindly like to ask for reviews and/or approval from maintainers by August 25th, 2022. Please let us know if you don't think this is attainable. |
Adding link to similar comment on distribution-spec PR. opencontainers/distribution-spec#335 (comment) |
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.
can we, should we add something over in image-index.md, such as another example, for using the image.index manifest to hold artifact manifests in the list of manifests? See example provided in distribution api..
manifest.md
Outdated
@@ -65,6 +65,11 @@ Unlike the [image index](image-index.md), which contains information about a set | |||
|
|||
Entries in this field will frequently use the `+gzip` types. | |||
|
|||
- **`refers`** *[descriptor](descriptor.md)* | |||
|
|||
This OPTIONAL property specifies a [descriptor](descriptor.md) of another image or [artifact](artifact.md). |
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.
can we clarify if there is a restriction or not for media type of the refers descriptor.. image manifest and image index manifest for example..
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 shouldn't have a restriction here. An artifact shipped with an Artifact manifest or this Image manifest can refer to any other manifest, including an Index. (I.e. You can attach a signature on a multi-platform manifest.)
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.
before suggesting text changes here to include that clarification.. Do we have to worry about/restrict dependency loops here? E.g. an image manifest with a refers to it's own image.index.. an image manifest with a refers to an artifact that refers to this image manifest... a manifest of any type with a refers to itself?
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.
For loops, I'm looking at whether the refers descriptor can point to a normal child of the manifest. If all children are blobs (as with Image and Artifact manifests), and not manifests (like we have with an Index), you can't point to a manifest that's a child.
It's a reverse pointer, so we aren't worried about parents (both pointers would be going the same direction). And you can't know the digest of the parent in advance without a hash collision/prediction.
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.
there may be an opportunity in platform image index.. where an artifact manifest included in the platform image index could be to an artifact that does not have a refers.. and in that case the artifact could be implied to be for that index (refer) by inclusion... thus allowing an index to have an artifact that refers to itself without incurring the loop restriction.. thinking out loud.
|
||
- **`refers`** *[descriptor](descriptor.md)* | ||
|
||
This OPTIONAL property specifies a [descriptor](descriptor.md) of another manifest. |
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.
cc @vbatts @sudo-bmitch - Does this provide more clarity in response to https://github.com/opencontainers/image-spec/pull/934/files/5240cdaf5cfb2f62f1b6b8860e5561332437270e#r955050333
This OPTIONAL property specifies a [descriptor](descriptor.md) of another manifest. | |
This OPTIONAL property specifies a [descriptor](descriptor.md) of another manifest. If defined, the artifact `refers` to, or has a reference to the specified manifest |
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.
I think so. (please put that sentence on its own newline)
during discussion on the call, this has enough features and fields that it merits a point release, but not a major version change. |
Thanks to all for the work on this in the WG and getting all this together (and dealing with all the feedback) and ready! IANAM but LGTM! 🎉 |
This is a culmination of many people's work over the last few months to introduce reference types to OCI as part of the [OCI Reference Types WG](oci-ref-type-wg). This PR contains the changes from our chosen proposal, [Proposal E][proposal-e]. [oci-ref-type-wg]: https://github.com/opencontainers/tob/blob/main/proposals/wg-reference-types.md [proposal-e]: https://github.com/opencontainers/wg-reference-types/blob/main/docs/proposals/PROPOSAL_E.md Co-authored-by: nisha <nisha@ctlfsh.tech> Co-authored-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com> Co-authored-by: Sajay Antony <sajaya@microsoft.com> Co-authored-by: Lachlan Evenson <lachlan.evenson@microsoft.com> Co-authored-by: Michael Brown <brownxmi@amazon.com> Signed-off-by: Brandon Mitchell <git@bmitch.net>
This is a culmination of many people's work over the last few months to introduce reference types to OCI as part of the OCI Reference Types WG.
This PR contains the changes from our chosen proposal, Proposal E, relevant to the image-spec. We're looking forward to hearing feedback from image-spec maintainers on these changes!
Associated distribution-spec PR - opencontainers/distribution-spec#335