fix: Refactor LayersMap to correct old strange code behavior (#2066)

* fix: Correct flatten function in layers

- Added a test.
- Cache current image, track deletes in `whiteouts` as well as normal adds in `layers`.
- Fix ugly delete behavior of `layerHashCache`.
  Delete it when crerating a new snapshot.
- Slight cleanup in `snapshot.go`.
- Format ugly `WalkFS` function.

* fix: Add symbolic link changes  to Hasher and CacheHasher

* fix: Better log messages

* fix(ci): Integration tests

* fix(ci): Add `--no-cache` to docker builds

* fix(ci): Pass credentials for error integration test

* np: Missing .gitignore in `hack`

* np: Capitalize every log message

- Correct some linting.

* fix: Key function

- Merge only last layer onto `currentImage`.

* fix: Remove old obsolete `cacheHasher`

DEVELOPMENT.md

@@ -74,7 +74,7 @@ The helper script to install and run lint is placed here at the root of project.
 
 ```shell
 ./hack/linter.sh
-``` 
+```
 
 To fix any `gofmt` issues, you can simply run `gofmt` with `-w` flag like this
 
@@ -99,6 +99,8 @@ To run integration tests with your GCloud Storage, you will also need the follow
 * A bucket in [GCS](https://cloud.google.com/storage/) which you have write access to via
   the user currently logged into `gcloud`
 * An image repo which you have write access to via the user currently logged into `gcloud`
+* A docker account and a `~/.docker/config.json` with login credentials if you run
+  into rate limiting problems during tests.
 
 Once this step done, you must override the project using environment variables:
 
@@ -162,9 +164,9 @@ These tests will be kicked off by [reviewers](#reviews) for submitted PRs using
 
 ### Benchmarking
 
-The goal is for Kaniko to be at least as fast at building Dockerfiles as Docker is, and to that end, we've built 
+The goal is for Kaniko to be at least as fast at building Dockerfiles as Docker is, and to that end, we've built
 in benchmarking to check the speed of not only each full run, but also how long each step of each run takes. To turn
-on benchmarking, just set the `BENCHMARK_FILE` environment variable, and kaniko will output all the benchmark info 
+on benchmarking, just set the `BENCHMARK_FILE` environment variable, and kaniko will output all the benchmark info
 of each run to that file location.
 
 ```shell
@@ -174,7 +176,7 @@ gcr.io/kaniko-project/executor:latest \
 --dockerfile=<path to Dockerfile> --context=/workspace \
 --destination=gcr.io/my-repo/my-image
 ```
-Additionally, the integration tests can output benchmarking information to a `benchmarks` directory under the 
+Additionally, the integration tests can output benchmarking information to a `benchmarks` directory under the
 `integration` directory if the `BENCHMARK` environment variable is set to `true.`
 
 ```shell

cmd/executor/cmd/root.go

@@ -128,7 +128,7 @@ var RootCmd = &cobra.Command{
 			if !force {
 				exit(errors.New("kaniko should only be run inside of a container, run with the --force flag if you are sure you want to continue"))
 			}
-			logrus.Warn("kaniko is being run outside of a container. This can have dangerous effects on your system")
+			logrus.Warn("Kaniko is being run outside of a container. This can have dangerous effects on your system")
 		}
 		if !opts.NoPush || opts.CacheRepo != "" {
 			if err := executor.CheckPushPermissions(opts); err != nil {
@@ -158,11 +158,11 @@ var RootCmd = &cobra.Command{
 				return
 			}
 			if strings.HasPrefix(benchmarkFile, "gs://") {
-				logrus.Info("uploading to gcs")
+				logrus.Info("Uploading to gcs")
 				if err := buildcontext.UploadToBucket(strings.NewReader(s), benchmarkFile); err != nil {
 					logrus.Infof("Unable to upload %s due to %v", benchmarkFile, err)
 				}
-				logrus.Infof("benchmark file written at %s", benchmarkFile)
+				logrus.Infof("Benchmark file written at %s", benchmarkFile)
 			} else {
 				f, err := os.Create(benchmarkFile)
 				if err != nil {
@@ -171,7 +171,7 @@ var RootCmd = &cobra.Command{
 				}
 				defer f.Close()
 				f.WriteString(s)
-				logrus.Infof("benchmark file written at %s", benchmarkFile)
+				logrus.Infof("Benchmark file written at %s", benchmarkFile)
 			}
 		}
 	},

hack/.gitignore

@@ -0,0 +1 @@
+bin

integration/.gitignore

@@ -0,0 +1,2 @@
+cache
+config.json

integration/benchmark_test.go

@@ -169,7 +169,7 @@ func runInGcloud(dir string, num int) (string, error) {
 	copyCommand := exec.Command("gsutil", "cp", src, dest)
 	_, err = RunCommandWithoutTest(copyCommand)
 	if err != nil {
-		return "", fmt.Errorf("failed to download file to GCS bucket %s: %s", src, err)
+		return "", fmt.Errorf("failed to download file to GCS bucket %s: %w", src, err)
 	}
 	return tmpDir, nil
 }

integration/gcs.go

@@ -31,17 +31,17 @@ func CreateIntegrationTarball() (string, error) {
 	log.Println("Creating tarball of integration test files to use as build context")
 	dir, err := os.Getwd()
 	if err != nil {
-		return "", fmt.Errorf("Failed find path to integration dir: %s", err)
+		return "", fmt.Errorf("Failed find path to integration dir: %w", err)
 	}
 	tempDir, err := ioutil.TempDir("", "")
 	if err != nil {
-		return "", fmt.Errorf("Failed to create temporary directory to hold tarball: %s", err)
+		return "", fmt.Errorf("Failed to create temporary directory to hold tarball: %w", err)
 	}
 	contextFile := fmt.Sprintf("%s/context_%d.tar.gz", tempDir, time.Now().UnixNano())
 	cmd := exec.Command("tar", "-C", dir, "-zcvf", contextFile, ".")
 	_, err = RunCommandWithoutTest(cmd)
 	if err != nil {
-		return "", fmt.Errorf("Failed to create build context tarball from integration dir: %s", err)
+		return "", fmt.Errorf("Failed to create build context tarball from integration dir: %w", err)
 	}
 	return contextFile, err
 }
@@ -57,7 +57,7 @@ func UploadFileToBucket(gcsBucket string, filePath string, gcsPath string) (stri
 	if err != nil {
 		log.Printf("Error uploading file %s to GCS at %s: %s", filePath, dst, err)
 		log.Println(string(out))
-		return "", fmt.Errorf("Failed to copy tarball to GCS bucket %s: %s", gcsBucket, err)
+		return "", fmt.Errorf("Failed to copy tarball to GCS bucket %s: %w", gcsBucket, err)
 	}
 
 	return dst, nil
@@ -69,7 +69,7 @@ func DeleteFromBucket(path string) error {
 	cmd := exec.Command("gsutil", "rm", path)
 	_, err := RunCommandWithoutTest(cmd)
 	if err != nil {
-		return fmt.Errorf("Failed to delete file %s from GCS: %s", path, err)
+		return fmt.Errorf("Failed to delete file %s from GCS: %w", path, err)
 	}
 	return err
 }

integration/images.go

@@ -31,6 +31,7 @@ import (
 	"time"
 
 	"github.com/GoogleContainerTools/kaniko/pkg/timing"
+	"github.com/GoogleContainerTools/kaniko/pkg/util"
 )
 
 const (
@@ -82,7 +83,7 @@ var additionalKanikoFlagsMap = map[string][]string{
 	"Dockerfile_test_scratch":                {"--single-snapshot"},
 	"Dockerfile_test_maintainer":             {"--single-snapshot"},
 	"Dockerfile_test_target":                 {"--target=second"},
-	"Dockerfile_test_snapshotter_ignorelist": {"--use-new-run=true", "-v=debug"},
+	"Dockerfile_test_snapshotter_ignorelist": {"--use-new-run=true", "-v=trace"},
 }
 
 // output check to do when building with kaniko
@@ -98,8 +99,8 @@ var outputChecks = map[string]func(string, []byte) error{
 		}
 
 		for _, s := range []string{
-			"resolved symlink /hello to /dev/null",
-			"path /dev/null is ignored, ignoring it",
+			"Resolved symlink /hello to /dev/null",
+			"Path /dev/null is ignored, ignoring it",
 		} {
 			if !strings.Contains(string(out), s) {
 				return fmt.Errorf("output must contain %s", s)
@@ -162,7 +163,7 @@ func GetVersionedKanikoImage(imageRepo, dockerfile string, version int) string {
 func FindDockerFiles(dockerfilesPath string) ([]string, error) {
 	allDockerfiles, err := filepath.Glob(path.Join(dockerfilesPath, "Dockerfile_test*"))
 	if err != nil {
-		return []string{}, fmt.Errorf("Failed to find docker files at %s: %s", dockerfilesPath, err)
+		return []string{}, fmt.Errorf("Failed to find docker files at %s: %w", dockerfilesPath, err)
 	}
 
 	var dockerfiles []string
@@ -218,7 +219,15 @@ func addServiceAccountFlags(flags []string, serviceAccount string) []string {
 			"GOOGLE_APPLICATION_CREDENTIALS=/secret/"+filepath.Base(serviceAccount),
 			"-v", filepath.Dir(serviceAccount)+":/secret/")
 	} else {
-		flags = append(flags, "-v", os.Getenv("HOME")+"/.config/gcloud:/root/.config/gcloud")
+		gcloudConfig := os.Getenv("HOME") + "/.config/gcloud"
+		if util.FilepathExists(gcloudConfig) {
+			flags = append(flags, "-v", gcloudConfig+":/root/.config/gcloud")
+		}
+
+		dockerConfig := os.Getenv("HOME") + "/.docker/config.json"
+		if util.FilepathExists(dockerConfig) {
+			flags = append(flags, "-v", dockerConfig+":/root/.docker/config.json", "-e", "DOCKER_CONFIG=/root/.docker")
+		}
 	}
 	return flags
 }
@@ -238,6 +247,7 @@ func (d *DockerFileBuilder) BuildDockerImage(t *testing.T, imageRepo, dockerfile
 
 	dockerArgs := []string{
 		"build",
+		"--no-cache",
 		"-t", dockerImage,
 	}
 
@@ -255,7 +265,7 @@ func (d *DockerFileBuilder) BuildDockerImage(t *testing.T, imageRepo, dockerfile
 
 	out, err := RunCommandWithoutTest(dockerCmd)
 	if err != nil {
-		return fmt.Errorf("Failed to build image %s with docker command \"%s\": %s %s", dockerImage, dockerCmd.Args, err, string(out))
+		return fmt.Errorf("Failed to build image %s with docker command \"%s\": %w %s", dockerImage, dockerCmd.Args, err, string(out))
 	}
 	t.Logf("Build image for Dockerfile %s as %s. docker build output: %s \n", dockerfile, dockerImage, out)
 	return nil
@@ -333,7 +343,7 @@ func populateVolumeCache() error {
 	)
 
 	if _, err := RunCommandWithoutTest(warmerCmd); err != nil {
-		return fmt.Errorf("Failed to warm kaniko cache: %s", err)
+		return fmt.Errorf("Failed to warm kaniko cache: %w", err)
 	}
 
 	return nil
@@ -373,7 +383,7 @@ func (d *DockerFileBuilder) buildCachedImages(config *integrationTestConfig, cac
 
 		_, err := RunCommandWithoutTest(kanikoCmd)
 		if err != nil {
-			return fmt.Errorf("Failed to build cached image %s with kaniko command \"%s\": %s", kanikoImage, kanikoCmd.Args, err)
+			return fmt.Errorf("Failed to build cached image %s with kaniko command \"%s\": %w", kanikoImage, kanikoCmd.Args, err)
 		}
 	}
 	return nil
@@ -399,7 +409,7 @@ func (d *DockerFileBuilder) buildRelativePathsImage(imageRepo, dockerfile, servi
 	out, err := RunCommandWithoutTest(dockerCmd)
 	timing.DefaultRun.Stop(timer)
 	if err != nil {
-		return fmt.Errorf("Failed to build image %s with docker command \"%s\": %s %s", dockerImage, dockerCmd.Args, err, string(out))
+		return fmt.Errorf("Failed to build image %s with docker command \"%s\": %w %s", dockerImage, dockerCmd.Args, err, string(out))
 	}
 
 	dockerRunFlags := []string{"run", "--net=host", "-v", cwd + ":/workspace"}
@@ -417,7 +427,7 @@ func (d *DockerFileBuilder) buildRelativePathsImage(imageRepo, dockerfile, servi
 
 	if err != nil {
 		return fmt.Errorf(
-			"Failed to build relative path image %s with kaniko command \"%s\": %s\n%s",
+			"Failed to build relative path image %s with kaniko command \"%s\": %w\n%s",
 			kanikoImage, kanikoCmd.Args, err, string(out))
 	}
 
@@ -485,11 +495,11 @@ func buildKanikoImage(
 
 	out, err := RunCommandWithoutTest(kanikoCmd)
 	if err != nil {
-		return "", fmt.Errorf("Failed to build image %s with kaniko command \"%s\": %s\n%s", kanikoImage, kanikoCmd.Args, err, string(out))
+		return "", fmt.Errorf("Failed to build image %s with kaniko command \"%s\": %w\n%s", kanikoImage, kanikoCmd.Args, err, string(out))
 	}
 	if outputCheck := outputChecks[dockerfile]; outputCheck != nil {
 		if err := outputCheck(dockerfile, out); err != nil {
-			return "", fmt.Errorf("Output check failed for image %s with kaniko command : %s\n%s", kanikoImage, err, string(out))
+			return "", fmt.Errorf("Output check failed for image %s with kaniko command : %w\n%s", kanikoImage, err, string(out))
 		}
 	}
 	return benchmarkDir, nil

integration/integration_test.go

@@ -496,14 +496,16 @@ func TestLayers(t *testing.T) {
 	offset := map[string]int{
 		"Dockerfile_test_add":     12,
 		"Dockerfile_test_scratch": 3,
+	}
 
+	if os.Getenv("CI") == "true" {
 		// TODO: tejaldesai fix this!
-		"Dockerfile_test_meta_arg":                  1,
-		"Dockerfile_test_copy_same_file_many_times": 47,
-		"Dockerfile_test_arg_multi_with_quotes":     1,
-		"Dockerfile_test_arg_multi":                 1,
-		"Dockerfile_test_arg_blank_with_quotes":     1,
+		// This files build locally with difference 0, on CI docker
+		// produces a different amount of layers (?).
+		offset["Dockerfile_test_copy_same_file_many_times"] = 47
+		offset["Dockerfile_test_meta_arg"] = 1
 	}
+
 	for _, dockerfile := range allDockerfiles {
 		t.Run("test_layer_"+dockerfile, func(t *testing.T) {
 			dockerfileTest := dockerfile
@@ -531,6 +533,8 @@ func TestLayers(t *testing.T) {
 }
 
 func buildImage(t *testing.T, dockerfile string, imageBuilder *DockerFileBuilder) {
+	t.Logf("Building image '%v'...", dockerfile)
+
 	if err := imageBuilder.BuildImage(t, config, dockerfilesPath, dockerfile); err != nil {
 		t.Errorf("Error building image: %s", err)
 		t.FailNow()
@@ -619,47 +623,59 @@ func TestExitCodePropagation(t *testing.T) {
 	t.Run("test error code propagation", func(t *testing.T) {
 		// building the image with docker should fail with exit code 42
 		dockerImage := GetDockerImage(config.imageRepo, "Dockerfile_exit_code_propagation")
-		dockerCmd := exec.Command("docker",
-			append([]string{"build",
-				"-t", dockerImage,
-				"-f", dockerfile,
-				context})...)
-		_, kanikoErr := RunCommandWithoutTest(dockerCmd)
+		dockerFlags := []string{
+			"build",
+			"-t", dockerImage,
+			"-f", dockerfile}
+		dockerCmd := exec.Command("docker", append(dockerFlags, context)...)
+
+		out, kanikoErr := RunCommandWithoutTest(dockerCmd)
 		if kanikoErr == nil {
-			t.Fatal("docker build did not produce an error")
+			t.Fatalf("docker build did not produce an error:\n%s", out)
 		}
 		var dockerCmdExitErr *exec.ExitError
 		var dockerExitCode int
 
 		if errors.As(kanikoErr, &dockerCmdExitErr) {
 			dockerExitCode = dockerCmdExitErr.ExitCode()
 			testutil.CheckDeepEqual(t, 42, dockerExitCode)
+			if t.Failed() {
+				t.Fatalf("Output was:\n%s", out)
+			}
 		} else {
-			t.Fatalf("did not produce the expected error")
+			t.Fatalf("did not produce the expected error:\n%s", out)
 		}
 
 		//try to build the same image with kaniko the error code should match with the one from the plain docker build
 		contextVolume := fmt.Sprintf("%s:/workspace", context)
-		dockerCmdWithKaniko := exec.Command("docker", append([]string{
+
+		dockerFlags = []string{
 			"run",
 			"-v", contextVolume,
-			ExecutorImage,
+		}
+		dockerFlags = addServiceAccountFlags(dockerFlags, "")
+		dockerFlags = append(dockerFlags, ExecutorImage,
 			"-c", "dir:///workspace/",
 			"-f", "./Dockerfile_exit_code_propagation",
 			"--no-push",
 			"--force", // TODO: detection of whether kaniko is being run inside a container might be broken?
-		})...)
+		)
+
+		dockerCmdWithKaniko := exec.Command("docker", dockerFlags...)
 
-		_, kanikoErr = RunCommandWithoutTest(dockerCmdWithKaniko)
+		out, kanikoErr = RunCommandWithoutTest(dockerCmdWithKaniko)
 		if kanikoErr == nil {
-			t.Fatal("the kaniko build did not produce the expected error")
+			t.Fatalf("the kaniko build did not produce the expected error:\n%s", out)
 		}
 
 		var kanikoExitErr *exec.ExitError
 		if errors.As(kanikoErr, &kanikoExitErr) {
 			testutil.CheckDeepEqual(t, dockerExitCode, kanikoExitErr.ExitCode())
+			if t.Failed() {
+				t.Fatalf("Output was:\n%s", out)
+			}
 		} else {
-			t.Fatalf("did not produce the expected error")
+			t.Fatalf("did not produce the expected error:\n%s", out)
 		}
 	})
 }
@@ -798,19 +814,19 @@ func checkLayers(t *testing.T, image1, image2 string, offset int) {
 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: %s", image, err)
+		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: %s", image, err)
+		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: %s", image, err)
+		return nil, fmt.Errorf("Error getting layers for image %s: %w", image, err)
 	}
 	digest, err := imgRef.Digest()
 	if err != nil {
-		return nil, fmt.Errorf("Error getting digest for image %s: %s", image, err)
+		return nil, fmt.Errorf("Error getting digest for image %s: %w", image, err)
 	}
 	return &imageDetails{
 		name:      image,

pkg/commands/copy.go

@@ -191,7 +191,7 @@ func (cr *CachingCopyCommand) ExecuteCommand(config *v1.Config, buildArgs *docke
 	cr.layer = layers[0]
 	cr.extractedFiles, err = util.GetFSFromLayers(kConfig.RootDir, layers, util.ExtractFunc(cr.extractFn), util.IncludeWhiteout())
 
-	logrus.Debugf("extractedFiles: %s", cr.extractedFiles)
+	logrus.Debugf("ExtractedFiles: %s", cr.extractedFiles)
 	if err != nil {
 		return errors.Wrap(err, "extracting fs from image")
 	}