Add support for zstd compression #1487

LFrobeen · 2022-11-07T12:30:12Z

This PR adds support for zstandard compression to the tarball layer implementation. It adds the application/vnd.oci.image.layer.v1.tar+zstd media type, a LayerCompression type and some extra logic in the tarball package. Moreover, this PR modifies pkg/v1/partial/compressed.go so that it is able to unpack zstd compressed layers.

When creating a tarball layer, one can specify the compression algorithm using WithCompression and the compression level with WithCompressionLevel. By default it will still use gzip compression.
The changes should be fully backwards compatible.

To give some more context:
We are adding zstd support to Northflank CI/CD & Images. We have built support for multiple build engines including Buildkit, Kaniko and CNBPs. Buildkit already supports zstd, but Kaniko does not. Adding zstd support to go-containerregistry will in turn allow Kaniko to build images with zstd compression which outperformed gzip in both performance and compression ratio in our tests.

google-cla · 2022-11-07T12:30:16Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

imjasonh

Wow, thanks for this! I've just taken a quick look so far but it looks great to me.

pkg/v1/types/types.go

imjasonh · 2022-11-07T19:48:29Z

It looks like there are some conflicts preventing CI from running. Can you rebase and we can see what the tests have to say?

Squashed commits: [6563fc8] Run ./hack/update-codegen.sh [c86a013] Fix linting issues [353dd69] Remove redundant break statements [a06b4cc] Fix some comments [fb0678f] Expose compression algorithms enum [3321d0c] Update mediaType according to compression for tarball layers (cherry picked from commit 56d4df5) [5124d2c] Fix unit tests (cherry picked from commit 7ce08d1) [2958f30] Optimize compression detection (cherry picked from commit 0a3437f) [c211a61] Add support for zstd-compressedd layers (cherry picked from commit d8fb0f3) [0b9a996] Add zstd compression utilities (cherry picked from commit c645201) [7dd71ab] Add layer types with zstd compression (cherry picked from commit 328a14b) Signed-off-by: Lavrenti Frobeen <lavrenti@northflank.com>

LFrobeen · 2022-11-07T20:30:01Z

Thanks for the feedback @imjasonh. I've removed the OCIRestrictedLayerZStd type and rebased the changes onto the main branch.
So far I ran the unit tests locally and they appear to be successful. However, I should definitely add more tests for the new functionality. I was thinking about adding some more test cases to tarball/image_test.go and tarball/layer_test.go. Do you think that would be enough already, or are there other places where I should add more tests?

imjasonh · 2022-11-07T20:32:54Z

Thanks for the feedback @imjasonh. I've removed the OCIRestrictedLayerZStd type and rebased the changes onto the main branch. So far I ran the unit tests locally and they appear to be successful. However, I should definitely add more tests for the new functionality. I was thinking about adding some more test cases to tarbal/image_test.go and tarbal/layer_test.go. Do you think that would be enough already, or are there other places where I should add more tests?

That seems sufficient honestly. If this is something we expect to expose via a flag in crane it might be nice to have an e2e test for it, but I don't think we need to tackle that in this PR.

cc @jonjohnsonjr in case he has thoughts

codecov-commenter · 2022-11-08T18:25:45Z

Codecov Report

Merging #1487 (e16bec5) into main (353a117) will decrease coverage by 0.07%.
The diff coverage is 63.47%.

@@            Coverage Diff             @@
##             main    #1487      +/-   ##
==========================================
- Coverage   73.19%   73.12%   -0.08%     
==========================================
  Files         115      117       +2     
  Lines        8812     8940     +128     
==========================================
+ Hits         6450     6537      +87     
- Misses       1712     1743      +31     
- Partials      650      660      +10

Impacted Files	Coverage Δ
pkg/v1/types/types.go	`100.00% <ø> (ø)`
internal/compression/compression.go	`34.14% <34.14%> (ø)`
pkg/v1/tarball/image.go	`76.20% <50.00%> (-0.75%)`	⬇️
pkg/v1/partial/compressed.go	`70.66% <57.14%> (-0.17%)`	⬇️
pkg/v1/tarball/layer.go	`67.17% <67.34%> (-1.54%)`	⬇️
internal/zstd/zstd.go	`80.64% <80.64%> (ø)`
internal/gzip/zip.go	`90.32% <100.00%> (+17.59%)`	⬆️
pkg/v1/mutate/mutate.go	`72.06% <100.00%> (ø)`
internal/retry/retry.go	`100.00% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

jonjohnsonjr · 2022-11-08T18:38:56Z

pkg/v1/tarball/layer.go

+		if compression == ZStd {
+			l.mediaType = types.OCILayerZStd
+		} else {
+			l.mediaType = types.DockerLayer


I would expect WithCompression(None) to set l.mediaType to DockerUncompressedLayer and produce uncompressed layers. WDYT?

I am also OK with punting on this for now, returning an error or documenting that None is invalid.

That makes sense, but I'm not quite sure how easy this would be to implement. My understanding is that the decision whether layers are produced with or without compression is made by consumers of the Layer interface when they access the data via .Compressed() or .Uncompressed(). Hence I'd have to modify all the code sites that use .Compressed() to make WithCompression(None) work, right?

I was thinking about alternatively just returning uncompressed data from the .Compressed() method when WithCompression(None) is set. But I felt that would conflict with semantics of the .Compressed() method which may be a bit confusing. Then again, this PR assumes the consumers of the .Compressed() method are agnostic to the compression format anyway, so one could argue that no compression can still be regarded "some kind of very inefficient compression format".

If you agree with this idea, I would implement it that way. Otherwise, I think the best option might be to drop WithCompression(None) altogether for now.

But I felt that would conflict with semantics of the .Compressed() method which may be a bit confusing.

Yeah that's fair. Having both the Compressed and Uncompressed methods is a design mistake. It would be better if we deprecated Uncompressed and renamed Compressed, but it's too late for that. I am fine with dropping this for now, but I think we want to return an error if we don't want to support it.

I modified the WithCompression method to ignore calls the compression.None argument and use gzip instead. Additionally it will print this warning:

Compression type 'none' is not supported for tarball layers; using gzip compression.

internal/zstd/zstd.go

internal/gzip/zip.go

internal/zstd/zstd_test.go

pkg/v1/tarball/layer.go

jonjohnsonjr · 2022-11-09T18:03:49Z

pkg/v1/tarball/layer.go

+		if compression == ZStd {
+			l.mediaType = types.OCILayerZStd
+		} else {
+			l.mediaType = types.DockerLayer


But I felt that would conflict with semantics of the .Compressed() method which may be a bit confusing.

Yeah that's fair. Having both the Compressed and Uncompressed methods is a design mistake. It would be better if we deprecated Uncompressed and renamed Compressed, but it's too late for that. I am fine with dropping this for now, but I think we want to return an error if we don't want to support it.

jonjohnsonjr · 2022-11-09T18:09:21Z

pkg/v1/tarball/layer.go

+func WithCompression(compression LayerCompression) LayerOption {
+	return func(l *layer) {
+		if compression == ZStd {
+			l.mediaType = types.OCILayerZStd


This conflicts a bit with layer.WithMediaType. I might just drop the else here? We could also warn or error or document this fact.

Thanks for all the feedback so far.

I removed the mediaType override for both zstd and gzip in order to be consistent.

If WithCompression(compression.ZStd) is used, then the user should also set WithMediaType(types.OCILayerZStd). I've updated the documentation for WithCompression to reflect this. When the layer is created via LayerFromOpener, the method will check whether the layer's mediaType matches its compression and print a warning if not. What do you think about this approach?

…" compression for tarball layer

LFrobeen · 2022-11-14T12:25:29Z

I've extended the unit tests so that they cover the new zstd logic.

I noticed that the presubmit check fails for some reason, but I can't seem to figure out what the issue is. Based on the logs from one of the recent runs I thought that I'd have to run go mod tidy in the root or the krane directory but that did not solve the issue. The logs also mention Error: pkg/name/internal/must_test.go:27:33 but my understanding is that this code is expected to fail compilation based on the code comments.

Apart from getting the presubmit check to pass, is there anything else that needs to be done before this can be merged?

jonjohnsonjr · 2022-11-17T18:16:05Z

There are a handful of submodules here (sorry) -- you might need to go mod tidy some others:

$ find | grep go\.mod | grep -v \.git
./cmd/krane/go.mod
./pkg/authn/k8schain/go.mod
./pkg/authn/kubernetes/go.mod
./go.mod

LFrobeen · 2022-11-17T18:50:33Z

Oh yes, I totally overlooked those. I ran go mod tidy in all the submodules now. Indeed github.com/klauspost/compress v1.15.11 // indirect was missing from pkg/authn/k8schain/go.mod.

LFrobeen · 2022-11-29T15:16:45Z

@imjasonh @jonjohnsonjr Sorry for being annoying, but could you re-trigger the tests please? I'd really like to get this merged so that I can add zstd support to kaniko next.
Also if there's anything you'd like me to change before the PR can be merged, please let me know.
Many thanks!

imjasonh · 2022-11-29T15:17:41Z

Not annoying at all!

LFrobeen · 2022-11-29T15:22:08Z

Awesome, thanks for triggering the tests! It looks like all checks have passed this time. Can we merge the PR now @imjasonh?

jonjohnsonjr · 2022-11-30T20:19:12Z

Thanks for sticking with this -- I'd like to play with it a bit before cutting a release, so I might break the API in small ways, but it looks pretty close to what we want.

… and compression level We want to make the layer compression in kaniko configurable, so we have added two optional command line arguments “--compression” and “--compression-level”. The former allows the user to specify a compression algorithm (zstd, gzip or none) and the latter can be used to specify the compression level. Depending on the selected compression algorithm and level we modify the set of layerOptions that are used to create tarball layers in `push.go` and `build.go`. The actual implementation of the zstd support can be found in our fork of the go-containerregistry package for which we have filed this PR: google/go-containerregistry#1487 The changes should be fully backwards compatible. (cherry picked from commit 7c8b681)

…ompression level We want to make the layer compression in kaniko configurable, so we have added two optional command line arguments “--compression” and “--compression-level”. The former allows the user to specify a compression algorithm (zstd, gzip) and the latter can be used to specify the compression level. Depending on the selected compression algorithm and level we modify the set of layerOptions that are used to create tarball layers in `push.go` and `build.go`. The actual implementation of the zstd support can be found in our fork of the go-containerregistry package for which we have filed this PR: google/go-containerregistry#1487 The changes should be fully backwards compatible.

* Add support for configurable compression algorithm (gzip, zstd) and compression level We want to make the layer compression in kaniko configurable, so we have added two optional command line arguments “--compression” and “--compression-level”. The former allows the user to specify a compression algorithm (zstd, gzip) and the latter can be used to specify the compression level. Depending on the selected compression algorithm and level we modify the set of layerOptions that are used to create tarball layers in `push.go` and `build.go`. The actual implementation of the zstd support can be found in our fork of the go-containerregistry package for which we have filed this PR: google/go-containerregistry#1487 The changes should be fully backwards compatible. * Restrict inputs for compression flag to gzip and zstd This change will ensure that users can only specify supported compression algorithms (`zstd`, `gzip`) to the `--compression` flag. * Fix incorrect type for switch statements on config.Compression

* Add support for configurable compression algorithm (gzip, zstd) and compression level We want to make the layer compression in kaniko configurable, so we have added two optional command line arguments “--compression” and “--compression-level”. The former allows the user to specify a compression algorithm (zstd, gzip) and the latter can be used to specify the compression level. Depending on the selected compression algorithm and level we modify the set of layerOptions that are used to create tarball layers in `push.go` and `build.go`. The actual implementation of the zstd support can be found in our fork of the go-containerregistry package for which we have filed this PR: google/go-containerregistry#1487 The changes should be fully backwards compatible. * Restrict inputs for compression flag to gzip and zstd This change will ensure that users can only specify supported compression algorithms (`zstd`, `gzip`) to the `--compression` flag. * Fix incorrect type for switch statements on config.Compression

LFrobeen force-pushed the feature/add-zstd-support-v0.9.0 branch 2 times, most recently from 40ece4f to 8bac013 Compare November 7, 2022 12:47

LFrobeen marked this pull request as ready for review November 7, 2022 16:29

imjasonh reviewed Nov 7, 2022

View reviewed changes

pkg/v1/types/types.go Outdated Show resolved Hide resolved

LFrobeen force-pushed the feature/add-zstd-support-v0.9.0 branch from 8bac013 to 879537e Compare November 7, 2022 20:19

Remove OCIRestrictedLayerZStd from MediaType enum

d01a4d0

LFrobeen added 2 commits November 7, 2022 21:36

Remove unused checkCompression method

8d5fd39

Add missing copyright headers

a78e83f

LFrobeen mentioned this pull request Nov 8, 2022

Add support for zstd compression GoogleContainerTools/kaniko#2313

Merged

4 tasks

LFrobeen added 2 commits November 8, 2022 17:27

Fix goimport lint errors

da41cf3

Run "go mod tidy" in cmd/krane

4935ed9

jonjohnsonjr reviewed Nov 8, 2022

View reviewed changes

LFrobeen added 2 commits November 9, 2022 09:37

Rename gw to zw in zstd.go

b66bbfc

Fix typos and clean up comments

e97a970

jonjohnsonjr reviewed Nov 9, 2022

View reviewed changes

Create public compression package for Compression enum; disable "none…

7c8c437

…" compression for tarball layer

LFrobeen force-pushed the feature/add-zstd-support-v0.9.0 branch from 859aa0c to 7c8c437 Compare November 10, 2022 14:34

LFrobeen added 3 commits November 10, 2022 15:39

Fix WithCompression method

478905c

Print warning if mediaType does not match selected compression

e29dcb9

Add unit tests for new zstd compression implementation

e16bec5

LFrobeen requested review from imjasonh and jonjohnsonjr and removed request for imjasonh November 14, 2022 12:25

LFrobeen requested review from imjasonh and jonjohnsonjr and removed request for jonjohnsonjr and imjasonh November 14, 2022 12:25

Run go mod tidy in pkg/authn/k8schain and pkg/authn/kubernetes

a0afb39

LFrobeen requested review from imjasonh and removed request for jonjohnsonjr November 23, 2022 09:13

jonjohnsonjr approved these changes Nov 30, 2022

View reviewed changes

jonjohnsonjr merged commit cd77615 into google:main Nov 30, 2022

LFrobeen mentioned this pull request Apr 3, 2023

Use correct media type for zstd layers GoogleContainerTools/kaniko#2459

Merged

4 tasks

ReillyBrogan mentioned this pull request Sep 28, 2023

feat: Add zstd compression to crane append and mutate #1798

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for zstd compression #1487

Add support for zstd compression #1487

LFrobeen commented Nov 7, 2022 •

edited

google-cla bot commented Nov 7, 2022

imjasonh left a comment

imjasonh commented Nov 7, 2022

LFrobeen commented Nov 7, 2022 •

edited

imjasonh commented Nov 7, 2022

codecov-commenter commented Nov 8, 2022 •

edited

jonjohnsonjr Nov 8, 2022

LFrobeen Nov 9, 2022

jonjohnsonjr Nov 9, 2022

LFrobeen Nov 10, 2022

jonjohnsonjr Nov 9, 2022

jonjohnsonjr Nov 9, 2022

LFrobeen Nov 10, 2022 •

edited

LFrobeen commented Nov 14, 2022 •

edited

jonjohnsonjr commented Nov 17, 2022

LFrobeen commented Nov 17, 2022

LFrobeen commented Nov 29, 2022

imjasonh commented Nov 29, 2022

LFrobeen commented Nov 29, 2022 •

edited

jonjohnsonjr commented Nov 30, 2022

Add support for zstd compression #1487

Add support for zstd compression #1487

Conversation

LFrobeen commented Nov 7, 2022 • edited

google-cla bot commented Nov 7, 2022

imjasonh left a comment

Choose a reason for hiding this comment

imjasonh commented Nov 7, 2022

LFrobeen commented Nov 7, 2022 • edited

imjasonh commented Nov 7, 2022

codecov-commenter commented Nov 8, 2022 • edited

Codecov Report

jonjohnsonjr Nov 8, 2022

Choose a reason for hiding this comment

LFrobeen Nov 9, 2022

Choose a reason for hiding this comment

jonjohnsonjr Nov 9, 2022

Choose a reason for hiding this comment

LFrobeen Nov 10, 2022

Choose a reason for hiding this comment

jonjohnsonjr Nov 9, 2022

Choose a reason for hiding this comment

jonjohnsonjr Nov 9, 2022

Choose a reason for hiding this comment

LFrobeen Nov 10, 2022 • edited

Choose a reason for hiding this comment

LFrobeen commented Nov 14, 2022 • edited

jonjohnsonjr commented Nov 17, 2022

LFrobeen commented Nov 17, 2022

LFrobeen commented Nov 29, 2022

imjasonh commented Nov 29, 2022

LFrobeen commented Nov 29, 2022 • edited

jonjohnsonjr commented Nov 30, 2022

LFrobeen commented Nov 7, 2022 •

edited

LFrobeen commented Nov 7, 2022 •

edited

codecov-commenter commented Nov 8, 2022 •

edited

LFrobeen Nov 10, 2022 •

edited

LFrobeen commented Nov 14, 2022 •

edited

LFrobeen commented Nov 29, 2022 •

edited