image manifest: support use as artifact #1030

AaronFriel · 2023-03-02T02:37:57Z

Implementations lack any information to determine whether the config property of an Image Manifest describes the artifactType of the manifest, or the media type of the referenced content. This is necessitated by the removal of artifact manifest (#999) and addition of scratch descriptors (#1023) to support artifacts.

If #999 is withdrawn, this pull request should be withdrawn as well and #1028 considered instead.

sudo-bmitch · 2023-03-31T18:51:40Z

considerations.md

@@ -1,7 +1,8 @@
 # Extensibility

-Implementations that are reading/processing [manifests](manifest.md) or [image indexes](image-index.md) MUST NOT generate an error if they encounter an unknown property.
-Instead they MUST ignore unknown properties.
+Implementations storing or copying content MUST NOT generate error if they encounter an unknown manifest media type (e.g.: [image manifest](manifest.md)) or an unknown property in a manifest.


"MUST NOT generate error if they encounter an unknown manifest" implies the MANIFEST_UNKNOWN error code in distribution spec is no longer valid. I don't think we can do that since it's not how any registry I'm aware of works today, everyone validates the manifest media type.

I would like to say "Implementations storing or copying content MUST NOT modify or alter the content in a way that would change the digest of the content."

🙏 thank you for the feedback, @sudo-bmitch. Will #1043 merge today or early next week so I can rebase and incorporate these changes?

That's a question for the other maintainers. We can merge once we get two approvals.

I've pulled this out into a separate PR, replacing the line with yours

sudo-bmitch · 2023-03-31T18:53:00Z

descriptor.md

@@ -18,7 +18,8 @@ The following fields contain the primary properties that constitute a Descriptor

 - **`mediaType`** *string*

-  This REQUIRED property contains the media type of the referenced content.
+  This REQUIRED property MUST contain the media type of the referenced content unless this descriptor is the `config` property of an [image manifest](manifest.md).


I don't think we need this with the addition of artifactType and the scratch media type.

sudo-bmitch

The wording covers storage and distribution of content, but we need to include content producers and consumers (like image runtimes) in this spec.

sudo-bmitch · 2023-04-30T17:32:52Z

manifest.md

@@ -31,6 +31,7 @@ Unlike the [image index](image-index.md), which contains information about a set
  This OPTIONAL property contains the type of an artifact when the manifest is used for an artifact.
  This MUST be set when `config.mediaType` is set to the [scratch value](#example-of-a-scratch-config-or-layer-descriptor).
  If defined, the value MUST comply with [RFC 6838][rfc6838], including the [naming requirements in its section 4.2][rfc6838-s4.2], and MAY be registered with [IANA][iana].
+  An encountered `artifactType` that is unknown to the implementation MUST NOT generate an error.


This makes sense for storing and distributing artifacts. But for tooling that processes an artifact, they may reject unknown types. E.g. if a tool parses an SBOM in json formats, and the artifactType is for an xml format, that tool should be allowed to generate an error. Or helm told to deploy something that is not a helm chart.

sudo-bmitch · 2023-04-30T17:37:28Z

manifest.md

@@ -83,7 +87,7 @@ Unlike the [image index](image-index.md), which contains information about a set
        - [`application/vnd.oci.image.layer.nondistributable.v1.tar+gzip`](layer.md#gzip-media-types)

        Manifests concerned with portability SHOULD use one of the above media types.
-        An encountered `mediaType` that is unknown to the implementation MUST be ignored.
+        An encountered `mediaType` that is unknown to the implementation MUST NOT generate an error.


Image runtimes should be allowed to generate an error if they cannot process the layers. Same for artifact processing tools that encounter an unexpected layer (e.g. helm trying to process a gif layer).

How would you phrase that distinction?

Does it make sense for us to define consumers (runtimes) vs distributors (registries, proxies) somewhere?

We say "implementations storing or copying content" above. That phrasing could work here too.

Signed-off-by: Aaron Friel <[email protected]>

Outdated

sudo-bmitch · 2023-05-16T16:29:18Z

I'd want to hear from registry operators about this before adding my own review. I could understand some of them concerned that they must accept unknown content and the only blocks that are being permitted are deny lists rather than allow lists.

sajayantony · 2023-06-08T23:59:04Z

Writing up my thoughts here since I have been mulling on this for quite some time and feel I owe a response to @AaronFriel who has been on multiple calls.

We do have customers who do want to limit the types that the registry should accept. This doesn't mean much since you can masquerade anything as an image. Implementations have already shown that.

The main issue I'm reading here is that there must be a way for artifact authors to get a sense that OCI registries are the way to store artifacts and not worry that their type would be rejected and my feeling is that it's a distribution spec issue as the image-manifest already calls out how an artifact may be represented.

I'm not sure if rewording of 'MUST be ignored' to 'MUST not generate an error' would change any implementation behaviors, but https://github.com/opencontainers/image-spec/pull/1030/files#diff-8f83bd1bc119af1add2e038b7c3aeade8b3845efed06a6971d1ee8740d7a3325R150 does add the fix for OCI layout which can be used for artifacts as well.

vbatts

I'm on board with these changes. This wording has been confusing and needed fixing for a while.

vbatts · 2023-06-15T16:40:21Z

config.md

@@ -248,7 +248,7 @@ Note: Any OPTIONAL field MAY also be set to null, which is equivalent to being a
       This field is used to mark if the history item created a filesystem diff.
       It is set to true if this history item doesn't correspond to an actual layer in the rootfs section (for example, Dockerfile's [ENV](https://docs.docker.com/engine/reference/builder/#/env) command results in no change to the filesystem).

-Any extra fields in the Image JSON struct are considered implementation specific and MUST be ignored by any implementations which are unable to interpret them.
+Any extra fields in the Image JSON struct are considered implementation specific and MUST NOT generate an error by any implementations which are unable to interpret them.


👍 This has been a confusing line

vbatts · 2023-06-15T16:43:32Z

considerations.md

@@ -1,7 +1,12 @@
 # Extensibility

-Implementations that are reading/processing [manifests](manifest.md) or [image indexes](image-index.md) MUST NOT generate an error if they encounter an unknown property.
-Instead they MUST ignore unknown properties.
+Implementations storing or copying content MUST NOT modify or alter the content in a way that would change the digest of the content. Examples of these implementations include:


While I agree with this sentence regarding altering of content, it could be straying away from the topic whether to generate errors or not

Agreed, this feels like a distribution-spec concern, not an image-spec concern -- a registry that intentionally modifies content pushed to it has some interesting potential use cases, and wouldn't be compliant to the distribution spec, but absolutely could be image-spec complaint, and that would be OK, I think?

vbatts · 2023-06-15T16:50:30Z

@sajayantony interesting. It's like the customers (and their tools) would need an endpoint or HTTP headers to discover what types are accepted. If it can vary per customer or even per registry repo ...

sajayantony

If this enables sentiment of artifact types as first class +1

AaronFriel requested review from jonjohnsonjr, jonboulle, stevvooe, sudo-bmitch, sajayantony, vbatts and cyphar as code owners March 2, 2023 02:37

AaronFriel force-pushed the friel/modest-proposal branch from 4466869 to 561de2d Compare March 2, 2023 02:52

AaronFriel mentioned this pull request Mar 2, 2023

Clarify ignore language, ensure future compat #1028

Closed

AaronFriel force-pushed the friel/modest-proposal branch from 561de2d to 1164748 Compare March 16, 2023 17:20

AaronFriel force-pushed the friel/modest-proposal branch from 1164748 to 78a5b0d Compare March 30, 2023 17:03

sudo-bmitch requested changes Mar 31, 2023

View reviewed changes

sudo-bmitch added this to the v1.1 milestone Apr 14, 2023

AaronFriel force-pushed the friel/modest-proposal branch from 78a5b0d to 3128c4a Compare April 27, 2023 16:59

AaronFriel requested a review from tianon as a code owner April 27, 2023 16:59

sudo-bmitch previously requested changes Apr 30, 2023

View reviewed changes

AaronFriel force-pushed the friel/modest-proposal branch 4 times, most recently from fc5ba71 to 557926c Compare May 4, 2023 17:05

spec: clarify descriptor, align with de facto artifact usage

77efc6e

Signed-off-by: Aaron Friel <[email protected]>

AaronFriel force-pushed the friel/modest-proposal branch from 557926c to 77efc6e Compare May 4, 2023 17:06

Add language from artifacttype field to forbid allowlists of media types

e13840d

Signed-off-by: Aaron Friel <[email protected]>

vbatts approved these changes Jun 15, 2023

View reviewed changes

sajayantony approved these changes Jun 15, 2023

View reviewed changes

vbatts merged commit 9708013 into opencontainers:main Jun 15, 2023

waynr mentioned this pull request Sep 25, 2023

Approach to artifact manifest seems to be out of date youki-dev/oci-spec-rs#144

Closed

sudo-bmitch mentioned this pull request Nov 15, 2023

[Maintainer vote] Is requiring registries to accept non-existent subjects a breaking change? opencontainers/distribution-spec#490

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

image manifest: support use as artifact #1030

image manifest: support use as artifact #1030

AaronFriel commented Mar 2, 2023

sudo-bmitch Mar 31, 2023

AaronFriel Mar 31, 2023

sudo-bmitch Mar 31, 2023

AaronFriel Apr 27, 2023

sudo-bmitch Mar 31, 2023

AaronFriel Apr 27, 2023

sudo-bmitch left a comment

sudo-bmitch Apr 30, 2023

sudo-bmitch Apr 30, 2023

AaronFriel May 1, 2023

sudo-bmitch May 1, 2023

sudo-bmitch commented May 16, 2023

sajayantony commented Jun 8, 2023

vbatts left a comment

vbatts Jun 15, 2023

vbatts Jun 15, 2023

tianon Jun 15, 2023

vbatts commented Jun 15, 2023

sajayantony left a comment

image manifest: support use as artifact #1030

image manifest: support use as artifact #1030

Conversation

AaronFriel commented Mar 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudo-bmitch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudo-bmitch commented May 16, 2023

sajayantony commented Jun 8, 2023

vbatts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbatts commented Jun 15, 2023

sajayantony left a comment

Choose a reason for hiding this comment