image: make layer validation work #111
Conversation
Most recently, the validation was broken because the image-spec was re-vendored but the code handling MediaType validation was not updated. This change (among others) broke umoci's tests because we run the validation code to ensure our images are always valid, and the validation incorrectly flagged our images as invalid. Fixes: ff477b3 ("vendor:update") Signed-off-by: Aleksa Sarai <[email protected]>
|
This is also broken inside the image specification, which I'm fixing separately... Here's the image spec PR: opencontainers/image-spec#560 |
|
Can we please be more careful next time? |
|
This is a dupe of #108 (though, this PR is correct...) |
|
LGTM |
| @@ -75,8 +75,15 @@ func (m *manifest) validate(w walker) error { | |||
| return errors.Wrap(err, "config validation failed") | |||
| } | |||
|
|
|||
| validLayerMediaTypes := []string{ | |||
There was a problem hiding this comment.
Is there a reason to make this private? If a third party wants to support an extension layer type (which we explicitly allow), that would give them a way to do it.
There was a problem hiding this comment.
it can stay private until asked for. :-)
|
Taking a step back, isn't this already handled by image-spec (which is was why it was broken in two places)? The DRY path forward seems like it would be dropping this image-tools code completely and using the image-spec validation instead. |
|
image-spec calls that descriptor-type check (see here and here), so |
|
Refer to #62 (comment). This had better to be done in image spec repo. |
|
@wking Sure, but at the moment that code doesn't actually return an error if the validation fails. |
|
On Thu, Feb 09, 2017 at 12:43:56AM -0800, Aleksa Sarai wrote:
Sure, but at the moment that code doesn't actually return an error
if the validation fails.
That's because an unknown type is a SHOULD violation [1], not a MUST
violation, and the image-spec maintainers [2] and image-tools
maintainers (#66) haven't decided how they want to handle those yet.
Assuming we only error on MUST violations (until folks decide how to
handle SHOULD violations), there's no reason to error-out on unknown
types here, so using the image-spec code is fine.
Layer *unpacking*, on the other hand, should be erroring out on
unknown media types (that's violating the ‘unpackable’ condition I
floated in #66). We'll want code around here [3], and that code will
have to start checking media types anyway now that both compressed and
uncompressed layers MUST be supported [4].
[1]: https://github.com/opencontainers/image-spec/blame/v1.0.0-rc4/manifest.md#L62
[2]: opencontainers/image-spec#491 (comment)
[3]: https://github.com/opencontainers/image-tools/blob/9386a7f89afec9215762e592aa054b7834c89aa4/image/manifest.go#L105-L106
[4]: https://github.com/opencontainers/image-spec/blame/v1.0.0-rc4/manifest.md#L55
|
|
LGTM |
1 similar comment
|
LGTM |
Most recently, the validation was broken because the image-spec was
re-vendored but the code handling MediaType validation was not updated.
This change (among others) broke umoci's tests because we run the
validation code to ensure our images are always valid, and the
validation incorrectly flagged our images as invalid.
Fixes: ff477b3 ("vendor:update")
Signed-off-by: Aleksa Sarai [email protected]