feat(image): return error early if total size of layers exceeds limit #8294
Conversation
…he limit Signed-off-by: nikpivkin <[email protected]>
| imagePath: "../../test/testdata/alpine-311.tar.gz", | ||
| imagePath: "../../test/testdata/image2.tar", | ||
| artifactOpt: artifact.Option{ | ||
| ImageOption: types.ImageOptions{MaxImageSize: units.MB * 4.1}, |
There was a problem hiding this comment.
It's weird, but my IDE complains 😄
There was a problem hiding this comment.
GoLand? I think the static analyzer should not show the warning because 4.1*MB is an integer.
https://go.dev/play/p/M1aB0vKfVl1
There was a problem hiding this comment.
Yeah, i agree with you.
Just wanted to share 😄
There was a problem hiding this comment.
Strange that the behavior is different from vscode, I think they use the same lsp.
| @@ -2255,11 +2255,19 @@ func TestArtifact_Inspect(t *testing.T) { | |||
| }, | |||
| { | |||
| name: "sad path, image size is larger than the maximum", | |||
| imagePath: "../../test/testdata/alpine-311.tar.gz", | |||
| imagePath: "../../test/testdata/image2.tar", | |||
There was a problem hiding this comment.
I seem to be missing something
Why are we using an existing image archive (e.g. vuln-image.tar.gz)?
There was a problem hiding this comment.
I created image2 as it is convenient for testing because it contains two 2mb layers.
There was a problem hiding this comment.
But we only test 2 cases:
- 1st layer size >
MaxImageSize. - Image size >
MaxImageSize.
Why do we need the 2nd layer?
We just don't want to add extra test files (there are too many of them anyway)
There was a problem hiding this comment.
The layers are processed in parallel and if a small layer is processed first, we will not check the first case.
There was a problem hiding this comment.
I'm not sure if this makes sense for this test, but okay, let's keep this file.
There was a problem hiding this comment.
Docker: Decimal prefixes (1000)
Docker Hub: Binary prefixes (1024)
There was a problem hiding this comment.
@nikpivkin It may be worth documenting. Otherwise, users who check the size on Docker Hub may complain about it.
There was a problem hiding this comment.
BTW I found an open issue docker/cli#4630
There was a problem hiding this comment.
Docker: Decimal prefixes (1000)
Docker Hub: Binary prefixes (1024)
Doesn't the Docker CLI display the actual binary size but uses decimal prefixes like Docker Hub?
Signed-off-by: nikpivkin <[email protected]>
Signed-off-by: nikpivkin <[email protected]>
Signed-off-by: nikpivkin <[email protected]>
Signed-off-by: nikpivkin <[email protected]>
docs/docs/target/container_image.md
Outdated
| An error is returned in the following cases: | ||
| - if the compressed image size exceeds the limit, |
There was a problem hiding this comment.
For me, the most significant difference between markdown and mkdocs is that a newline is required before bullet points.
| An error is returned in the following cases: | |
| - if the compressed image size exceeds the limit, | |
| An error is returned in the following cases: | |
| - if the compressed image size exceeds the limit, |
It's broken now.
docs/docs/target/container_image.md
Outdated
|
|
||
| An error is returned in the following cases: | ||
| - if the compressed image size exceeds the limit, | ||
| - if the total size of the layers exceeds the specified limit during their pulling, |
There was a problem hiding this comment.
| - if the total size of the layers exceeds the specified limit during their pulling, | |
| - if the total size of the uncompressed layers exceeds the specified limit during their pulling, |
Signed-off-by: nikpivkin <[email protected]>
Signed-off-by: nikpivkin <[email protected]>
Signed-off-by: knqyf263 <[email protected]>
Description
This PR optimizes the image size check by returning an error if the total size of the layers exceeds the limit, without the need to load the entire image.
Related PRs
Checklist