image/manifest.go: implement compression detection / mediatype validation #135
Conversation
erikh
changed the title
|
The thing is, layers now can be either uncompressed or compressed. Maybe you should look at the auto-detect-compression code here? Or we can just handle the two cases separately. |
image/manifest.go
Outdated
| return errors.Wrap(err, "error creating gzip reader") | ||
|
|
||
| reader := r | ||
| if strings.HasSuffix(mediaType, "+gzip") { |
There was a problem hiding this comment.
Oh sorry, you're already handling that. Oops.
|
So the thing about this patch that's weird is that now the images I generate, which use tar files, are validating fine against oci-image-tool. I think I did something wrong on my end. However this patch does validate the media type and switch the reader accordingly so it may be worth keeping. I'll leave it to you. |
|
@erikh I'd recommend auto-detecting compression to most compatible. |
|
@stevvooe not entirely sure what you mean by this, do you mean reading the file header? |
Yes. |
|
What's happening right now is that the media type is the source of truth
with this patch; I don't think this should change for validation purposes
at least. Perhaps we can appropriately detect and error on mismatched
compression with the media type?
This is the code used in `oci-image-tool validate` after all; it probably
shouldn't be too tolerant if it's to serve its purpose.
Happy to take it in any direction; just stating my opinion.
…-Erik
On Thu, Apr 6, 2017 at 1:46 PM Stephen Day ***@***.***> wrote:
do you mean reading the file header?
Yes.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#135 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABJ6wJ9O1LKMs5i7XkpU57IzxBXy5Ciks5rtU8tgaJpZM4Mwvt7>
.
|
|
@erikh I'm just trying to provide a practical approach that has worked for us in employing unpacking for manifests. If you want to over-validate, go ahead, but its much more flexible to autodetect the compression in practice and there isn't really downside to it. Erroring out will just frustrate people that can do nothing about it. It is much better to emit a warning and unpack the layer. As long as the digest matches, it is okay. For the most part, this will matter as people "shallow convert" existing images to the OCI formats. Mind you, this is the only exception where I would say go against the media type. In almost all other cases, you should be strict. |
|
Fair enough. I will do that. |
|
@erikh conflict, needs a rebase |
|
It seems this needs an overhaul; I will do the work in the AM PST.
…On Tue, Apr 11, 2017 at 1:08 AM Lei Jitang ***@***.***> wrote:
@erikh <https://github.com/erikh> conflict, needs a rebase
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#135 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABJ6zQ0GKsA_Te6iJdK52pDnqKsTImDks5ruzUQgaJpZM4Mwvt7>
.
|
erikh
force-pushed
the
fix-tar-files
branch
2 times, most recently
from
e5fed5b to
a7de4fc
Compare
|
PTAL and sorry for the wait. This patch now:
|
|
I'll clean things up; needs a bit of work still. I'll comment when ready. |
erikh
force-pushed
the
fix-tar-files
branch
6 times, most recently
from
4dd0d5b to
1d22ba9
Compare
|
I'll look later. Test is failing in CI but not locally. |
erikh
force-pushed
the
fix-tar-files
branch
5 times, most recently
from
96e684f to
19293e0
Compare
erikh
force-pushed
the
fix-tar-files
branch
14 times, most recently
from
02304be to
f9b29ec
Compare
…roblems Additionally: * Adjusted go test to use the -v flag to show debug statements * Imported github.com/dsnet/compress/bzip2 for bzip2 compression * Wrote several tests to test both valid and invalid media types Signed-off-by: Erik Hollensbe <[email protected]>
|
ok, this is green now. PTAL. Note I had to import github.com/dsnet/compress/bzip2 to implement bzip2 compression. the hold-up was my pipe implementation that called bzip2, which works locally but doesn't seem to on travis for some reason. |
erikh
changed the title
|
LGTM |
1 similar comment
|
LGTM |
6 participants
Previously, this would effectively panic on any tar files because it was
expecting a gzip compressed file. This enforces that the
+gzipmediatype exists before enabling gzip decompression.
I also shimmed in some tests by appropriating some others.
Signed-off-by: Erik Hollensbe [email protected]