From 1bc1ba2102e50871322273f7a7b2c3c7522b8d2f Mon Sep 17 00:00:00 2001 From: Martin Zihlmann Date: Mon, 26 May 2025 11:24:05 +0100 Subject: [PATCH 1/4] make cache tests more strict - fail on layer digest diff --- integration/integration_test.go | 51 +++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/integration/integration_test.go b/integration/integration_test.go index f52489fad..9c65faf31 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -35,6 +35,7 @@ import ( "time" "github.com/google/go-containerregistry/pkg/name" + v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/google/go-containerregistry/pkg/v1/daemon" "github.com/google/go-containerregistry/pkg/v1/remote" "github.com/pkg/errors" @@ -742,6 +743,7 @@ func verifyBuildWith(t *testing.T, cache, dockerfile string) { expected := fmt.Sprintf(emptyContainerDiff, kanikoVersion0, kanikoVersion1, kanikoVersion0, kanikoVersion1) checkContainerDiffOutput(t, diff, expected) + layerDiff(t, kanikoVersion0, kanikoVersion1) } func TestRelativePaths(t *testing.T) { @@ -957,6 +959,39 @@ func filterFileDiff(f []fileDiff) []fileDiff { return newDiffs } +func layerDiff(t *testing.T, image1, image2 string) { + t.Helper() + layers1, err := getImageLayers(image1) + if err != nil { + t.Fatalf("Couldn't get details from image reference for (%s): %s", image1, err) + } + + layers2, err := getImageLayers(image2) + if err != nil { + t.Fatalf("Couldn't get details from image reference for (%s): %s", image2, err) + } + + for idx := range min(len(layers1), len(layers2)) { + l1d, err := layers1[idx].Digest() + if err != nil { + t.Fatalf("Couldn't get digest from image layer (%s #%d): %s", image1, idx, err) + } + + l2d, err := layers2[idx].Digest() + if err != nil { + t.Fatalf("Couldn't get digest from image layer (%s #%d): %s", image2, idx, err) + } + + if l1d != l2d { + t.Errorf("Image Layers #%d differ %s != %s", idx, l1d, l2d) + } + } + + if len(layers1) != len(layers2) { + t.Errorf("Image Layer count differs %d != %d", len(layers1), len(layers2)) + } +} + func checkLayers(t *testing.T, image1, image2 string, offset int) { t.Helper() img1, err := getImageDetails(image1) @@ -975,6 +1010,22 @@ func checkLayers(t *testing.T, image1, image2 string, offset int) { } } +func getImageLayers(image string) ([]v1.Layer, error) { + ref, err := name.ParseReference(image, name.WeakValidation) + if err != nil { + return nil, fmt.Errorf("Couldn't parse referance to image %s: %w", image, err) + } + imgRef, err := daemon.Image(ref) + if err != nil { + return nil, fmt.Errorf("Couldn't get reference to image %s from daemon: %w", image, err) + } + layers, err := imgRef.Layers() + if err != nil { + return nil, fmt.Errorf("Error getting layers for image %s: %w", image, err) + } + return layers, nil +} + func getImageDetails(image string) (*imageDetails, error) { ref, err := name.ParseReference(image, name.WeakValidation) if err != nil { From 6a918a6181e8a06522459feafe59c13629dda546 Mon Sep 17 00:00:00 2001 From: Martin Zihlmann Date: Mon, 26 May 2025 12:01:22 +0100 Subject: [PATCH 2/4] add createdBy for more context --- integration/integration_test.go | 53 +++++++++++++++++++++++++++++---- 1 file changed, 48 insertions(+), 5 deletions(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index 9c65faf31..39489f42b 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -963,12 +963,12 @@ func layerDiff(t *testing.T, image1, image2 string) { t.Helper() layers1, err := getImageLayers(image1) if err != nil { - t.Fatalf("Couldn't get details from image reference for (%s): %s", image1, err) + t.Fatalf("Couldn't get layers from image reference for (%s): %s", image1, err) } layers2, err := getImageLayers(image2) if err != nil { - t.Fatalf("Couldn't get details from image reference for (%s): %s", image2, err) + t.Fatalf("Couldn't get layers from image reference for (%s): %s", image2, err) } for idx := range min(len(layers1), len(layers2)) { @@ -983,12 +983,29 @@ func layerDiff(t *testing.T, image1, image2 string) { } if l1d != l2d { - t.Errorf("Image Layers #%d differ %s != %s", idx, l1d, l2d) + command, err := resolveCreatedBy(image1, idx) + if err != nil { + t.Errorf("Image Layers #%d differ", idx) + } else { + t.Errorf("Image Layers #%d differ: %s", idx, command) + } } } - if len(layers1) != len(layers2) { - t.Errorf("Image Layer count differs %d != %d", len(layers1), len(layers2)) + if len(layers1) > len(layers2) { + command, err := resolveCreatedBy(image1, len(layers2)) + if err != nil { + t.Errorf("Image Layer count differs %d != %d", len(layers1), len(layers2)) + } else { + t.Errorf("Image Layer count differs %d != %d: %s", len(layers1), len(layers2), command) + } + } else if len(layers1) < len(layers2) { + command, err := resolveCreatedBy(image2, len(layers1)) + if err != nil { + t.Errorf("Image Layer count differs %d != %d", len(layers1), len(layers2)) + } else { + t.Errorf("Image Layer count differs %d != %d: %s", len(layers1), len(layers2), command) + } } } @@ -1010,6 +1027,32 @@ func checkLayers(t *testing.T, image1, image2 string, offset int) { } } +func resolveCreatedBy(image string, layerIndex int) (string, error) { + ref, err := name.ParseReference(image, name.WeakValidation) + if err != nil { + return "", fmt.Errorf("Couldn't parse referance to image %s: %w", image, err) + } + imgRef, err := daemon.Image(ref) + if err != nil { + return "", fmt.Errorf("Couldn't get reference to image %s from daemon: %w", image, err) + } + cfg, err := imgRef.ConfigFile() + if err != nil { + return "", fmt.Errorf("Couldn't get Config for image %s: %w", image, err) + } + idx := 0 + for _, history := range cfg.History { + if history.EmptyLayer { + continue + } + if idx == layerIndex { + return history.CreatedBy, nil + } + idx++ + } + return "", fmt.Errorf("LayerIndex %d not found in History of length %d", layerIndex, len(cfg.History)) +} + func getImageLayers(image string) ([]v1.Layer, error) { ref, err := name.ParseReference(image, name.WeakValidation) if err != nil { From 603a542f27d3348b6960c59fb633176cfe0ff3c3 Mon Sep 17 00:00:00 2001 From: Martin Zihlmann Date: Mon, 26 May 2025 12:07:54 +0100 Subject: [PATCH 3/4] typo --- integration/integration_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index 39489f42b..9a147d7f6 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -1030,7 +1030,7 @@ func checkLayers(t *testing.T, image1, image2 string, offset int) { func resolveCreatedBy(image string, layerIndex int) (string, error) { ref, err := name.ParseReference(image, name.WeakValidation) if err != nil { - return "", fmt.Errorf("Couldn't parse referance to image %s: %w", image, err) + return "", fmt.Errorf("Couldn't parse reference to image %s: %w", image, err) } imgRef, err := daemon.Image(ref) if err != nil { @@ -1056,7 +1056,7 @@ func resolveCreatedBy(image string, layerIndex int) (string, error) { func getImageLayers(image string) ([]v1.Layer, error) { ref, err := name.ParseReference(image, name.WeakValidation) if err != nil { - return nil, fmt.Errorf("Couldn't parse referance to image %s: %w", image, err) + return nil, fmt.Errorf("Couldn't parse reference to image %s: %w", image, err) } imgRef, err := daemon.Image(ref) if err != nil { @@ -1072,7 +1072,7 @@ func getImageLayers(image string) ([]v1.Layer, error) { func getImageDetails(image string) (*imageDetails, error) { ref, err := name.ParseReference(image, name.WeakValidation) if err != nil { - return nil, fmt.Errorf("Couldn't parse referance to image %s: %w", image, err) + return nil, fmt.Errorf("Couldn't parse reference to image %s: %w", image, err) } imgRef, err := daemon.Image(ref) if err != nil { @@ -1096,7 +1096,7 @@ func getImageDetails(image string) (*imageDetails, error) { func getLastLayerFiles(image string) ([]string, error) { ref, err := name.ParseReference(image, name.WeakValidation) if err != nil { - return nil, fmt.Errorf("Couldn't parse referance to image %s: %w", image, err) + return nil, fmt.Errorf("Couldn't parse reference to image %s: %w", image, err) } imgRef, err := remote.Image(ref) From 41a97540ebd01f391034b07d9133b6dd74aeeed7 Mon Sep 17 00:00:00 2001 From: Martin Zihlmann Date: Sun, 25 May 2025 22:45:14 +0100 Subject: [PATCH 4/4] drop uncacheable commands --- integration/dockerfiles/Dockerfile_test_cache | 2 -- integration/dockerfiles/Dockerfile_test_cache_install | 2 +- .../dockerfiles/Dockerfile_test_cache_install_oci | 2 +- integration/dockerfiles/Dockerfile_test_cache_oci | 2 -- integration/dockerfiles/Dockerfile_test_issue_add | 9 +++++++++ integration/dockerfiles/Dockerfile_test_issue_empty | 9 +++++++++ integration/dockerfiles/Dockerfile_test_issue_workdir | 8 ++++++++ integration/images.go | 6 ++++++ 8 files changed, 34 insertions(+), 6 deletions(-) create mode 100644 integration/dockerfiles/Dockerfile_test_issue_add create mode 100644 integration/dockerfiles/Dockerfile_test_issue_empty create mode 100644 integration/dockerfiles/Dockerfile_test_issue_workdir diff --git a/integration/dockerfiles/Dockerfile_test_cache b/integration/dockerfiles/Dockerfile_test_cache index f7ce19af3..0f80fbf21 100644 --- a/integration/dockerfiles/Dockerfile_test_cache +++ b/integration/dockerfiles/Dockerfile_test_cache @@ -18,5 +18,3 @@ FROM debian:10.13 RUN date > /date -COPY context/foo /foo -RUN echo hey diff --git a/integration/dockerfiles/Dockerfile_test_cache_install b/integration/dockerfiles/Dockerfile_test_cache_install index ae2feb195..5d044833d 100644 --- a/integration/dockerfiles/Dockerfile_test_cache_install +++ b/integration/dockerfiles/Dockerfile_test_cache_install @@ -17,7 +17,7 @@ # if the cache is implemented correctly FROM debian:10.13 +RUN mkdir /foo WORKDIR /foo RUN apt-get update && apt-get install -y make -COPY context/bar /context RUN echo "hey" > foo diff --git a/integration/dockerfiles/Dockerfile_test_cache_install_oci b/integration/dockerfiles/Dockerfile_test_cache_install_oci index ae2feb195..5d044833d 100644 --- a/integration/dockerfiles/Dockerfile_test_cache_install_oci +++ b/integration/dockerfiles/Dockerfile_test_cache_install_oci @@ -17,7 +17,7 @@ # if the cache is implemented correctly FROM debian:10.13 +RUN mkdir /foo WORKDIR /foo RUN apt-get update && apt-get install -y make -COPY context/bar /context RUN echo "hey" > foo diff --git a/integration/dockerfiles/Dockerfile_test_cache_oci b/integration/dockerfiles/Dockerfile_test_cache_oci index f7ce19af3..0f80fbf21 100644 --- a/integration/dockerfiles/Dockerfile_test_cache_oci +++ b/integration/dockerfiles/Dockerfile_test_cache_oci @@ -18,5 +18,3 @@ FROM debian:10.13 RUN date > /date -COPY context/foo /foo -RUN echo hey diff --git a/integration/dockerfiles/Dockerfile_test_issue_add b/integration/dockerfiles/Dockerfile_test_issue_add new file mode 100644 index 000000000..b50f1bfb4 --- /dev/null +++ b/integration/dockerfiles/Dockerfile_test_issue_add @@ -0,0 +1,9 @@ +FROM ubuntu AS base + +# Creates cache misses on main@1d2bff5: +# Even with --cache-copy-layers, only COPY layers are cached. +# ADD instruction is never cached. +# As a workaround use COPY instruction instead +# +# COPY context/foo foo +ADD context/foo foo diff --git a/integration/dockerfiles/Dockerfile_test_issue_empty b/integration/dockerfiles/Dockerfile_test_issue_empty new file mode 100644 index 000000000..dfbd21b92 --- /dev/null +++ b/integration/dockerfiles/Dockerfile_test_issue_empty @@ -0,0 +1,9 @@ +FROM ubuntu AS base + +# Creates cache misses on main@1d2bff5: +# When building the image directly no layer is created. +# When rebuilding from cache a layer is emitted even for empty statements, +# as we currently have no logic for skipping the layer when rebuilding from emtpy cache. +# This causes one-time cache misses as now the chain of layers has changed. +# +RUN echo blubb diff --git a/integration/dockerfiles/Dockerfile_test_issue_workdir b/integration/dockerfiles/Dockerfile_test_issue_workdir new file mode 100644 index 000000000..536942c39 --- /dev/null +++ b/integration/dockerfiles/Dockerfile_test_issue_workdir @@ -0,0 +1,8 @@ +FROM ubuntu + +# Creates cache misses on main@1d2bff5 before #3340: +# Folders created implicitly by WORKDIR are not cached. +# As a workaround create them excplicitly with RUN +# +# RUN mkdir /app +WORKDIR /app diff --git a/integration/images.go b/integration/images.go index ccf675c61..148b5d6ae 100644 --- a/integration/images.go +++ b/integration/images.go @@ -218,6 +218,12 @@ func NewDockerFileBuilder() *DockerFileBuilder { "Dockerfile_test_cache_install": {}, "Dockerfile_test_cache_perm": {}, "Dockerfile_test_cache_copy": {}, + // TODO: WORKDIR command is uncacheable + //"Dockerfile_test_issue_workdir": {}, + // TODO: ADD command is uncacheable + //"Dockerfile_test_issue_add": {}, + // TODO: Empty Layers are uncacheable + //"Dockerfile_test_issue_empty": {}, } d.TestOCICacheDockerfiles = map[string]struct{}{ "Dockerfile_test_cache_oci": {},