Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Layer digests don't match pushed digests using docker #572

Closed
jromero opened this issue Oct 16, 2019 · 3 comments · Fixed by #574
Closed

Layer digests don't match pushed digests using docker #572

jromero opened this issue Oct 16, 2019 · 3 comments · Fixed by #574

Comments

@jromero
Copy link
Contributor

jromero commented Oct 16, 2019

It seems like there have been a few issues regarding Digest and compression (#150, #153, #158) yet I've come here with an unexpected issue brought on by the current compression.

It seems like the current compression used is great in regards of performance but our expectation was that it would produce a layer digest which resembles that of what docker would produce when pushing to a registry. That is not the case. docker uses default compression making calls to layer.Digest() unusable in our case and requires us to compress the layers yet again to get the appropriate and expected layer digest.

This issue appears affects both tarball.Layer and stream.Layer.

Proposal: Allow configuration of compression used at digest computation but default to gzip.DefaultCompression.

@jonjohnsonjr
Copy link
Collaborator

our expectation was that it would produce a layer digest which resembles that of what docker would produce when pushing to a registry

I don't think matching exactly what docker produces is something we are interested in trying to maintain, given that any changes to docker would break this assumption.

Proposal: Allow configuration of compression used at digest computation but default to gzip.DefaultCompression.

On the other hand, this is completely reasonable and something we should definitely have 😄.

I haven't looked into it much, so I'm not sure how hard it would be to plumb this through everywhere. Do you have an idea of how you'd want to implement this?

@jromero
Copy link
Contributor Author

jromero commented Oct 16, 2019

There was a PR that was going to address part of this #155 for images and the discussions revolved around using functional options. It seems like most agreed on that strategy. (#155 (comment))

On the plus side would maintain the method signatures backwards compatible.

Here's a quick prototype of what that would look like.
layer-compress.patch.txt

@jonjohnsonjr
Copy link
Collaborator

Sorry I didn't think of this before, but I'm wondering if we can solve #531 at the same time? If we're already making the compression level pluggable, perhaps we should make the entire compression algorithm pluggable?

We would need some way to synthesize the appropriate mediaType for the given compression algorithm as well. Perhaps something like:

func defaultCompressor(rc io.ReadCloser, mt types.MediaType) (io.ReadCloser, types.MediaType) {
  // TODO: map docker media types and OCI media types to the right compressed versions
  mt = convertMediaType(mt, "gzip")
  return v1util.GzipReadCloser(rc), mt
}

func WithCompressor(c Compressor) LayerOption {
  return func(l *layer) error {
    l.compressor = c
  }
}

type Compressor func(io.ReadCloser, types.MediaType) (io.ReadCloser, types.MediaType)

Maybe returning the whole mediatype is the wrong thing here and we should just return "gzip" -- some generic interface would be useful.

Somewhat related are containerd/containerd#3649 and opencontainers/image-spec#791

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants