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

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

Merged
merged 10 commits into from
May 18, 2022
10 changes: 6 additions & 4 deletions DEVELOPMENT.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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:

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
8 changes: 4 additions & 4 deletions cmd/executor/cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,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 {
Expand Down Expand Up @@ -152,11 +152,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 {
Expand All @@ -165,7 +165,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)
}
}
},
Expand Down
1 change: 1 addition & 0 deletions hack/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
bin
2 changes: 2 additions & 0 deletions integration/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
cache
config.json
2 changes: 1 addition & 1 deletion integration/benchmark_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
7 changes: 4 additions & 3 deletions integration/dockerfiles/Dockerfile_test_issue_1837
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
FROM registry.access.redhat.com/ubi8/ubi:8.2 AS BASE
FROM registry.access.redhat.com/ubi8/ubi:8.2 AS base
# Install ping
RUN yum --disableplugin=subscription-manager install -y iputils
RUN setcap cap_net_raw+ep /bin/ping || exit 1

FROM BASE
RUN set -e && [ ! -z "$(getcap /bin/ping)" ] || exit 1
FROM base
RUN [ ! -z "$(getcap /bin/ping)" ] || exit 1
10 changes: 5 additions & 5 deletions integration/gcs.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
Expand All @@ -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
}
34 changes: 22 additions & 12 deletions integration/images.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"time"

"github.com/GoogleContainerTools/kaniko/pkg/timing"
"github.com/GoogleContainerTools/kaniko/pkg/util"
)

const (
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand All @@ -238,6 +247,7 @@ func (d *DockerFileBuilder) BuildDockerImage(t *testing.T, imageRepo, dockerfile

dockerArgs := []string{
"build",
"--no-cache",
"-t", dockerImage,
}

Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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"}
Expand All @@ -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))
}

Expand Down Expand Up @@ -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
Expand Down
62 changes: 39 additions & 23 deletions integration/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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)
}
})
}
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion pkg/commands/copy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down
Loading