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

Define image manifest artifactType and guidance #1043

Merged
merged 1 commit into from
Apr 13, 2023

Conversation

sudo-bmitch
Copy link
Contributor

@sudo-bmitch sudo-bmitch commented Mar 24, 2023

This adds an artifactType field in the image manifest, defines a scratch media type, and provides guidance on using the image manifest for artifacts. This design is based on the discussions from the 2023-03-23 meeting.

manifest.md Outdated Show resolved Hide resolved
Copy link
Member

@sajayantony sajayantony left a comment

Choose a reason for hiding this comment

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

Overall, there seemed to be consensus on the call and a desire to move forward with artifactType being defined in the image manifest so that referrers can release.

I would love for OCI to remove the requirements for Layers clearer in 2.0. But that is for another day.

manifest.md Outdated Show resolved Hide resolved
sajayantony
sajayantony previously approved these changes Mar 28, 2023
@sudo-bmitch sudo-bmitch added this to the v1.1 milestone Mar 28, 2023
```json,title=Artifact%20with%20config&mediatype=application/vnd.oci.image.manifest.v1%2Bjson
{
"schemaVersion": 2,
"mediaType": "application/vnd.oci.image.manifest.v1+json",
Copy link
Member

Choose a reason for hiding this comment

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

should we add artifactType here..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When config.mediaType is defined to something other than scratch, artifactType is optional.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be slightly prescriptive about best practices, should we say that non-image artifacts SHOULD use an artifactType with the scratch config?

Not a MUST.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AaronFriel I'm open to it, but in case there are other opinions, how about we make that a separate PR. Would you like to lead that?

manifest.md Outdated Show resolved Hide resolved
manifest.md Show resolved Hide resolved
manifest.md Outdated Show resolved Hide resolved
Comment on lines +160 to +161
When this is done, the `config.mediaType` value MUST be set to a value specific to the artifact type or the [scratch value](#example-of-a-scratch-config-or-layer-descriptor).
If the `config.mediaType` is set to the scratch value, the `artifactType` MUST be defined.
If the artifact does not need layers, a single layer SHOULD be included with a non-zero size.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if it was part of the discussion I missed (but even if so, I think the explanation should stand here alone): why would I choose to use scratch+artifactType, rather than just setting config.mediaType to whatever specific value I want to use?

Copy link
Contributor

Choose a reason for hiding this comment

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

I expect any artifact producing implementation that doesn't need or want a config will use a scratch manifest.

Suppose you just want to upload an SBOM, or a cat gif (image/gif). What do you put in config.mediaType?

If you chose image/gif, some implementations might try to render the config as a gif, or save it as a .gif file on download, etc. The config descriptor however won't point to a gif blob, it'll point to {} and the first layer will be the image/gif.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonboulle the issue that was raised is the config.mediaType must reference the content of the config.digest blob according to the descriptor spec. By using that for artifacts that do not have a dedicated config blob, we have a mismatch between the media type of the blob and the artifactType of the artifact.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I might have asked the question backwards :)
In what circumstance would I set config.mediaType to something other than scratch or an actual (``application/vnd.oci.image.config.v1+json`) config? Is there a use case there we really need to support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are actual configs that are not application/vnd.oci.image.config.v1+json, e.g. Helm charts and Singularity images. These use cases are following the guidance at https://github.com/opencontainers/artifacts/blob/main/artifact-authors.md. My goal was to avoid breaking those existing artifacts unless there's a strong need to do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that's the exact reference that was missing for me. I will think about it for another moment; I would rather it's captured more explicitly here

@andaaron
Copy link

andaaron commented Apr 6, 2023

Hi all,

I have some questions on how the artifactType in this PR would be used by the referrers API /v2/<name>/referrers/<digest>.

Given this PR, I would expect /v2/<name>/referrers/<digest> to return a mix of matches on both config descriptor mediaType and artifactType (if and only if config descriptor mediaType is scratch). Is this correct?

In the server reply the artifactType field the value returned would sometimes be from config descriptor mediaType, and sometimes from artifactType, depending on the individual artifact?

Also if this is the case, I would expect the artifactType query parameter in /v2/<name>/referrers/<digest>?artifactType=<artifactType> to match sometimes the config mediaType and sometimes the artifactType.

It doesn't seem consistent for the consumer of the API.

@sudo-bmitch
Copy link
Contributor Author

@andaaron the update to the referrers API is in distribution-spec: opencontainers/distribution-spec#395

@sudo-bmitch sudo-bmitch requested a review from tianon as a code owner April 6, 2023 18:33
Copy link
Member

@sajayantony sajayantony left a comment

Choose a reason for hiding this comment

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

We can add the the case for helm or backwards as per @jonboulle comment as a follow up if needed

Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

In jq, this means consumers looking to query artifactType should use an expression something like .artifactType // .config.mediaType, but the benefit is that it allows the config blob's descriptor to be "pure" and actually describe the blob it points to instead of the object it's part of, so I'm in favor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants