Use artifactType from image manifest #395

sudo-bmitch · 2023-03-24T21:41:48Z

This updates the guidance to use the artifactType value first, and fall back to the config.mediaType when determining the artifactType to set in the referrers response. This should be merged after opencontainers/image-spec#1043 is approved.

mikebrow

some thoughts ...

mikebrow · 2023-03-28T21:35:45Z

spec.md

@@ -489,7 +489,8 @@ When pushing a manifest with the `subject` field and the [referrers API](#listin
 1. If the tag returns a 404, the client MUST begin with an empty image index.
 1. Verify the descriptor for the manifest is not already in the referrers list (duplicate entries SHOULD NOT be created).
 1. Append a descriptor for the pushed manifest to the manifests in the referrers list.
-   The value of the `artifactType` MUST be set to the config descriptor `mediaType` in the image manifest.
+   The value of the `artifactType` MUST be set to the `artifactType` value in the image manifest.


seems confusing that the artifactType MUST be set to the artifactType field of the image manifest .. or if nil/unset must be set to the config descriptor mediaType..

Indicates it may be set in both places and that they can be inconsistent..

If both we should talk about the rules here .. such that you can't have both and sometimes one is used and other times the other is used.. depending. Does set/unset logic apply? I didn't check the schema. I'm thinking if both are set they must be the same or it's an error.

are we saying/proposing at least one of these must be set and not nil (in this push manifest with subject case)

I'm looking for the right wording to say artifactType must be set, and the value is the manifest artifactType if defined, otherwise it falls back to the config.mediaType, so there's a precedence to document. The config.mediaType is always defined in the image-spec.

kk let's go to chat.. :-)

spec.md

rchincha · 2023-03-30T17:27:49Z

lgtm

spec.md

andaaron

From the perspective of the consumer of the API does it matter if the match is made on the config descriptor mediaType or the artifactType?
I guess if someone codes a client which handles a limited types of artifacts it doesn't matter, because he can handle each case individually by verifying the artifactType value, but I think there should be a way to distinguish the source of the artifact type value in the reply itself to handle more generic cases.

andaaron · 2023-04-06T17:22:57Z

spec.md

@@ -555,7 +556,8 @@ If the request is invalid, such as a `<digest>` with an invalid syntax, a `400 B

 Upon success, the response MUST be a JSON body with an image index containing a list of descriptors.
 Each descriptor is of an image manifest in the same `<name>` namespace with a `subject` field that specifies the value of `<digest>`.
-The descriptors MUST include an `artifactType` field that is set to the value the configuration descriptor's `mediaType` for an image manifest.
+The descriptors MUST include an `artifactType` field that is set to the value of the `artifactType` in the image manifest.


This part doesn't seem to be consistent with the image spec proposal. In the image spec the 'artifactType' is only mandatory if the config descriptor mediaType is scratch.

So if an image has a non-scratch config descriptor mediaType and an artifactType, it makes sense for anyone reading the manifest to ignore the artifactType altogether, including the registry implementation.

Or else the registry may return an artifactType matching the artifactType, and after the artifact is copied locally, another tool with a different implementation may only look at the config descriptor.

That is unless we make sure the artifactType is absent from any image having a non-scratch config descriptor mediaType. Meaning the image spec mandates artifactType and non-scratch config descriptor mediaTypes are mutually exclusive.

sudo-bmitch · 2023-04-06T18:17:07Z

@andaaron

From the perspective of the consumer of the API does it matter if the match is made on the config descriptor mediaType or the artifactType? I guess if someone codes a client which handles a limited types of artifacts it doesn't matter, because he can handle each case individually by verifying the artifactType value, but I think there should be a way to distinguish the source of the artifact type value in the reply itself to handle more generic cases.

I'm looking at it from a progressive upgrade with backwards compatibility. An existing artifact that has a dedicated config blob and media type can continue using that to avoid impacting existing clients. A new artifact that needs a config blob, but also wants a specific artifactType can do that, and find itself using that artifactType. And a new artifact without a config blob is required to specify an artifactType because "scratch" isn't the real artifact type.

I'm hoping that artifacts with a specific type will be created according to some spec from that artifact producer, and have the fields set consistently according to their spec (e.g. all Helm charts follow a Helm spec). From the listing we shouldn't care how the artifactType value was determined, only that it has a specific value that we want.

andaaron · 2023-04-06T18:50:16Z

@sudo-bmitch
If you put it like this (all tooling will eventually converge to check artifactType first, or will have special handling for individual artifact specs), then it makes sense.

sudo-bmitch · 2023-04-06T18:53:47Z

@andaaron the goal of the phrasing is to say "check artifactType first, and fall back to using config.mediaType". Suggestions for better wording in the spec is welcome, because I'm not happy with it either.

andaaron · 2023-04-06T19:16:49Z

@andaaron the goal of the phrasing is to say "check artifactType first, and fall back to using config.mediaType". Suggestions for better wording in the spec is welcome, because I'm not happy with it either.

I'm fine with the wording as long as the other consumers of the image spec use the same rationale when reading a manifest. That was my concern.

spec.md

mikebrow

LGTM with if present language on the MUST requirement

jdolitsky · 2023-04-13T17:49:49Z

@mikebrow can I get a re-approve? (adding jons suggestion dismissed your review)

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

sudo-bmitch · 2023-04-14T00:03:06Z

This has been updated with the changes from today's meeting. PTAL.

mikebrow

LGTM

sudo-bmitch requested review from dmcgowan, jzelinskie, jonjohnsonjr, jdolitsky, mikebrow, stevvooe and vbatts as code owners March 24, 2023 21:41

jdolitsky previously approved these changes Mar 25, 2023

View reviewed changes

sudo-bmitch added this to the v1.1.0 milestone Mar 28, 2023

mikebrow reviewed Mar 28, 2023

View reviewed changes

jonjohnsonjr reviewed Mar 30, 2023

View reviewed changes

spec.md Outdated Show resolved Hide resolved

sudo-bmitch dismissed jdolitsky’s stale review via ac7c527 March 31, 2023 18:42

sudo-bmitch force-pushed the pr-image-manifest-artifact-type branch from 76c05c6 to ac7c527 Compare March 31, 2023 18:42

sudo-bmitch mentioned this pull request Apr 6, 2023

Define image manifest artifactType and guidance opencontainers/image-spec#1043

Merged

andaaron reviewed Apr 6, 2023

View reviewed changes

jdolitsky previously approved these changes Apr 6, 2023

View reviewed changes

andaaron previously approved these changes Apr 6, 2023

View reviewed changes

jonjohnsonjr reviewed Apr 13, 2023

View reviewed changes

spec.md Outdated Show resolved Hide resolved

jonjohnsonjr reviewed Apr 13, 2023

View reviewed changes

spec.md Outdated Show resolved Hide resolved

jonjohnsonjr reviewed Apr 13, 2023

View reviewed changes

spec.md Outdated Show resolved Hide resolved

mikebrow previously approved these changes Apr 13, 2023

View reviewed changes

sajayantony previously approved these changes Apr 13, 2023

View reviewed changes

jdolitsky dismissed stale reviews from sajayantony, mikebrow, and andaaron via b1f810e April 13, 2023 17:48

jdolitsky dismissed their stale review via b1f810e April 13, 2023 17:48

jdolitsky previously approved these changes Apr 13, 2023

View reviewed changes

Use artifactType from image manifest

72a5455

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

sudo-bmitch dismissed jdolitsky’s stale review via 72a5455 April 13, 2023 23:59

sudo-bmitch force-pushed the pr-image-manifest-artifact-type branch from c6e8f6e to 72a5455 Compare April 13, 2023 23:59

mikebrow approved these changes Apr 18, 2023

View reviewed changes

jdolitsky approved these changes Apr 18, 2023

View reviewed changes

jdolitsky merged commit 79ee8e2 into opencontainers:main Apr 18, 2023

sudo-bmitch deleted the pr-image-manifest-artifact-type branch April 18, 2023 21:18

jdolitsky mentioned this pull request Apr 19, 2023

Release 1.1.0 rc2 #402

Closed

sudo-bmitch mentioned this pull request Feb 1, 2024

Release v1.1.0 #507

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use artifactType from image manifest #395

Use artifactType from image manifest #395

sudo-bmitch commented Mar 24, 2023

mikebrow left a comment

mikebrow Mar 28, 2023

mikebrow Mar 28, 2023 •

edited

Loading

sudo-bmitch Mar 28, 2023

mikebrow Mar 28, 2023

rchincha commented Mar 30, 2023

andaaron left a comment •

edited

Loading

andaaron Apr 6, 2023

sudo-bmitch commented Apr 6, 2023

andaaron commented Apr 6, 2023

sudo-bmitch commented Apr 6, 2023

andaaron commented Apr 6, 2023

mikebrow left a comment

jdolitsky commented Apr 13, 2023

sudo-bmitch commented Apr 14, 2023

mikebrow left a comment

Use artifactType from image manifest #395

Use artifactType from image manifest #395

Conversation

sudo-bmitch commented Mar 24, 2023

mikebrow left a comment

Choose a reason for hiding this comment

mikebrow Mar 28, 2023

Choose a reason for hiding this comment

mikebrow Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

sudo-bmitch Mar 28, 2023

Choose a reason for hiding this comment

mikebrow Mar 28, 2023

Choose a reason for hiding this comment

rchincha commented Mar 30, 2023

andaaron left a comment • edited Loading

Choose a reason for hiding this comment

andaaron Apr 6, 2023

Choose a reason for hiding this comment

sudo-bmitch commented Apr 6, 2023

andaaron commented Apr 6, 2023

sudo-bmitch commented Apr 6, 2023

andaaron commented Apr 6, 2023

mikebrow left a comment

Choose a reason for hiding this comment

jdolitsky commented Apr 13, 2023

sudo-bmitch commented Apr 14, 2023

mikebrow left a comment

Choose a reason for hiding this comment

mikebrow Mar 28, 2023 •

edited

Loading

andaaron left a comment •

edited

Loading