image-tools
image-tools copied to clipboard
opencontainers
cmd/oci-image-tool: validation errors and warnings
From @runcom on September 9, 2016 8:33
From https://github.com/opencontainers/image-spec/pull/279#issuecomment-245717705
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
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 validateshould soften the error given when an unknown media type is encountered when validating a reference underrefs/.
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).
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.
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?
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.
From @stevvooe on September 13, 2016 19:16
@philips @wking @runcom Do we have a general UX plan for this tool?
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.