cmd/oci-image-tool: validation errors and warnings

[philips]

From @runcom on September 9, 2016 8:33

From opencontainers/image-spec#279 (comment)

Action: oci-image-tool validate should soften the error given when an unknown media type is encountered when validating a reference under refs/.

Discussion: does the above work with media types found in manifests as well? Should we just print a warning and exit 0 from the tool? Should we a flag which specifies the warning level to suppress? So, should we have more than just one warning level here? (the second level being a manifest, third being media types in descriptors , etc)

/cc @stevvooe @vbatts @philips

Copied from original issue: opencontainers/image-spec#286

[philips]

From @wking on September 9, 2016 8:52

On Fri, Sep 09, 2016 at 01:33:06AM -0700, Antonio Murdaca wrote:

Action: oci-image-tool validate should soften the error given when
an unknown media type is encountered when validating a reference
under refs/.

I think the softening should go behind a flag like --unpackable [1](which makes more sense than my alternative
--ignore-unknown-media-types [2]). This gives you a way to toggle (b)
in the following cases:

a) The media type is recognized and an expected target of the source
location (always good).
b) The media type is unrecognized, but the source location does not
have media-type restrictions (good unless --unpackable)
c) The media type is recognized and the source location does not have
media-type restrictions, but the media type does not make sense in
this location (e.g. an image/png in manifest.layers. Always bad).
d) The media type is not in the set of types allowed by that source
location (always bad).

The spec does not currently place restrictions on allowed types for
manifest-list.manifests, manifest.layers, or manifest.config, which
makes gradual extensibility easy but leaves the “does not make sense”
call a bit vague (maybe an implementation thinks it does know how to
unpack an image/png layer). If we do restrict those types, we
wouldn't have any instances of (b) or (c), and there would be no need
of a softening flag.

So, should we have more than just one warning level here? (the
second level being a manifest, third being media types in
descriptors , etc)

I don't think that scales as the number of supported media types grows
(as it presumably will in the future).

[philips]

From @wking on September 9, 2016 8:55

On Fri, Sep 09, 2016 at 01:50:04AM -0700, W. Trevor King wrote:

If we do restrict those types, we wouldn't have any instances of (b)
or (c), and there would be no need of a softening flag.

Scratch that, we would still have instances of (b) in the refs. So
we'd need --unpackable (or some such) to ignore cases where someone
added an image/png ref (or whatever). This is exactly the case that
@stevvoe was concerned about ([optionally?] not erroring for unknown
ref media types) 1, and I agree that we want to cover that case.

[philips]

From @runcom on September 9, 2016 8:59

@wking I really struggle at following you comments full of references to other issues/place of your paragraph. I personally find it really confusing. Could you please concisely re-explain what are you suggesting to have here?

[philips]

From @wking on September 9, 2016 10:21

On Fri, Sep 09, 2016 at 01:59:05AM -0700, Antonio Murdaca wrote:

Could you please concisely re-explain what are you suggesting to
have here?

Some of the things are not actionable suggestions, but just raising
points for discussion. For example, “should manifest.layers require
descriptors to be application/vnd.oci.image.layer.tar+gzip?”. This is
(like most decisions) a balancing act between competing interests. I
tried to lay out those interests in the “The spec does not currently
place restrictions…” paragraph, and am fine with whatever the
maintainer consensus is on which interest is more important.

But that's about spec development. I have opinions about the
validation ;). I haven't been able to work out a way to explain my
four cases more concisely. Maybe I can be clearer with an example.

Say the spec has evolved so manifest.config requires …config.v1+json
but manifest.layers does not restrict media types. Then an
image-layout like:

refs:
v1.0 (…manifest.v1+json, sha256-a…)
v2.0 (…manifest.v1+json, sha256-a…)
cover (image/png, sha256-d…)
blobs:
sha256-a…:
.mediaType …manifest.v1+json
.config (…config.v1+json, sha256-b…)
.layers[0](…layer.tar+gzip, sha256-c…)
.layers[1](…config.v1+json, sha256-b…)
.layers[2](image/png, sha256-d…)
sha256-e…:
.mediaType …manifest.v1+json
.config (image/png, sha256-d…)
…
…

should validate (without --unpackable) as:

ok 1 - v1.0 recognized media type: …manifest.v1+json
ok 2 - v1.0 fetched sha256-a…
ok 3 - sha256-a… has the expected media type
… other local manifest checks, I won't bother counting …
ok 4 - sha256-a… config has a legal media type: …config.v1+json
ok 5 - sha256-a… layers[0] has a recognized media type: …layer.tar+gzip
not ok 6 - sha256-a… layers1 has a inappropriate media type: …config.v1+json not in […layer.tar+gzip]
ok 7 # skip sha256-a… layers[2] has an unknown media type: image/png
ok 8 - sha256-a… config fetched sha256-b…
… other local config checks, I won't bother counting …
ok 9 - fetched sha256-c…
… other local layer checks, I won't bother counting …
ok 10 - v2.0 recognized media type: …manifest.v1+json
ok 11 - v2.0 fetched sha256-e…
ok 12 - sha256-e… has the expected media type
not ok 13 - sha256-e… config has an illegal media type: image/png not in […config.v1+json]
… other sha256-e… and children checks, I won't bother counting …
ok 14 # skip cover unknown media type: image/png

We do not attempt to fetch sha256-a…'s ‘layers1’ (because we know
it's an illegal type) or ‘layers[2]’ (because we wouldn't know what to
do with it if we got it). With --unpackable, the skips in tests 7 and
14 would become:

not ok 7 - sha256-a… layers[2] cannot unpack unknown media type: image/png
…
not ok 14 - cover cannot unpack unknown media type: image/png

Sorting the results into the cases from 1:

a) Recognized and expected: test 4.
b) Unrecognized, no source restrictions: tests 7 and 14.
c) Recognized, no source restrictions, type not compatible with the
location: test 6.
d) Source restrictions not satisfied: test 13.

[philips]

From @stevvooe on September 13, 2016 19:16

@philips @wking @runcom Do we have a general UX plan for this tool?

[philips]

From @runcom on September 13, 2016 19:19

@stevvooe I have something in mind and I started working on it already but don't want to create more issues and PRs here since the repo will be split shortly to avoid further work to do when moving. I'm doing a light refactor focused on validation stages though, the order will be discussed separately after I submit a PR.

[philips]

From @stevvooe on September 13, 2016 21:19

@runcom Sounds great!