From 34a211113fcfea0a0a10717dd9b153fa57a6b929 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Thu, 16 Nov 2023 14:06:08 +0530 Subject: [PATCH] buildah: add heredoc support for RUN, COPY and ADD Following PR is a attempt to add `Heredoc` support to buildah. Once this PR is merged buildah is supposed to honor heredoc syntax while processing containerfiles Expected syntax to work ```Dockerfile FROM docker.io/library/python:latest RUN <> /hello echo "World!" >> /hello EOF RUN python3 < --- go.mod | 2 +- imagebuildah/stage_executor.go | 119 +++++++++++++++++- tests/bud.bats | 35 ++++++ tests/bud/heredoc/Containerfile | 59 +++++++++ tests/bud/heredoc/Containerfile.bash_file | 15 +++ .../heredoc/Containerfile.verify_mount_leak | 17 +++ tests/conformance/conformance_test.go | 20 +++ .../testdata/heredoc/Dockerfile.heredoc_copy | 23 ++++ tests/conformance/testdata/heredoc/file | 1 + 9 files changed, 288 insertions(+), 3 deletions(-) create mode 100644 tests/bud/heredoc/Containerfile create mode 100644 tests/bud/heredoc/Containerfile.bash_file create mode 100644 tests/bud/heredoc/Containerfile.verify_mount_leak create mode 100644 tests/conformance/testdata/heredoc/Dockerfile.heredoc_copy create mode 100644 tests/conformance/testdata/heredoc/file diff --git a/go.mod b/go.mod index c627a6fce77..cee3a18bc57 100644 --- a/go.mod +++ b/go.mod @@ -18,6 +18,7 @@ require ( github.com/fsouza/go-dockerclient v1.9.7 github.com/hashicorp/go-multierror v1.1.1 github.com/mattn/go-shellwords v1.0.12 + github.com/moby/buildkit v0.10.6 github.com/onsi/ginkgo v1.16.5 github.com/onsi/gomega v1.29.0 github.com/opencontainers/go-digest v1.0.0 @@ -98,7 +99,6 @@ require ( github.com/miekg/pkcs11 v1.1.1 // indirect github.com/mistifyio/go-zfs/v3 v3.0.1 // indirect github.com/mitchellh/mapstructure v1.5.0 // indirect - github.com/moby/buildkit v0.10.6 // indirect github.com/moby/patternmatcher v0.5.0 // indirect github.com/moby/sys/mountinfo v0.6.2 // indirect github.com/moby/sys/sequential v0.5.0 // indirect diff --git a/imagebuildah/stage_executor.go b/imagebuildah/stage_executor.go index f7f7e6467cc..bbff7ce1067 100644 --- a/imagebuildah/stage_executor.go +++ b/imagebuildah/stage_executor.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "os" + "path" "path/filepath" "sort" "strconv" @@ -35,6 +36,7 @@ import ( "github.com/containers/storage/pkg/chrootarchive" "github.com/containers/storage/pkg/unshare" docker "github.com/fsouza/go-dockerclient" + buildkitparser "github.com/moby/buildkit/frontend/dockerfile/parser" digest "github.com/opencontainers/go-digest" v1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/opencontainers/runtime-spec/specs-go" @@ -348,6 +350,7 @@ func (s *StageExecutor) volumeCacheRestore() error { // imagebuilder tells us the instruction was "ADD" and not "COPY". func (s *StageExecutor) Copy(excludes []string, copies ...imagebuilder.Copy) error { s.builder.ContentDigester.Restart() + copiesExtend := []imagebuilder.Copy{} for _, copy := range copies { if err := s.volumeCacheInvalidate(copy.Dest); err != nil { return err @@ -362,7 +365,61 @@ func (s *StageExecutor) Copy(excludes []string, copies ...imagebuilder.Copy) err stripSetgid := false preserveOwnership := false contextDir := s.executor.contextDir - if len(copy.From) > 0 { + // If we are copying files via heredoc syntax, then + // its time to create these temporary files on host + // and copy these to container + if len(copy.Files) > 0 { + // If we are copying files from heredoc syntax, there + // maybe regular files from context as well so split and + // process them differently + if len(copy.Src) > len(copy.Files) { + regularSources := []string{} + for _, src := range copy.Src { + // If this source is not a heredoc, then it is a regular file from + // build context or from another stage (`--from=`) so treat this differently. + if !strings.HasPrefix(src, "<<") { + regularSources = append(regularSources, src) + } + } + copyEntry := copy + // Remove heredoc if any, since we are already processing them + // so create new entry with sources containing regular files + // only, since regular files can have different context then + // heredoc files. + copyEntry.Files = nil + copyEntry.Src = regularSources + copiesExtend = append(copiesExtend, copyEntry) + } + copySources := []string{} + for _, file := range copy.Files { + data := file.Data + // remove first break line added while parsing heredoc + data = strings.TrimPrefix(data, "\n") + // add breakline when heredoc ends for docker compat + data = data + "\n" + tmpFile, err := os.Create(filepath.Join(parse.GetTempDir(), path.Base(filepath.ToSlash(file.Name)))) + if err != nil { + return fmt.Errorf("unable to create tmp file for COPY instruction at %q: %w", parse.GetTempDir(), err) + } + err = tmpFile.Chmod(0644) // 644 is consistent with buildkit + if err != nil { + tmpFile.Close() + return fmt.Errorf("unable to chmod tmp file created for COPY instruction at %q: %w", tmpFile.Name(), err) + } + defer os.Remove(tmpFile.Name()) + _, err = tmpFile.WriteString(data) + if err != nil { + tmpFile.Close() + return fmt.Errorf("unable to write contents of heredoc file at %q: %w", tmpFile.Name(), err) + } + copySources = append(copySources, filepath.Base(tmpFile.Name())) + tmpFile.Close() + } + contextDir = parse.GetTempDir() + copy.Src = copySources + } + + if len(copy.From) > 0 && len(copy.Files) == 0 { // If from has an argument within it, resolve it to its // value. Otherwise just return the value found. from, fromErr := imagebuilder.ProcessWord(copy.From, s.stage.Builder.Arguments()) @@ -485,6 +542,13 @@ func (s *StageExecutor) Copy(excludes []string, copies ...imagebuilder.Copy) err return err } } + if len(copiesExtend) > 0 { + // If we found heredocs and regularfiles together + // in same statement then we produced new copies to + // process regular files separately since they need + // different context. + return s.Copy(excludes, copiesExtend...) + } return nil } @@ -590,10 +654,59 @@ func (s *StageExecutor) runStageMountPoints(mountList []string) (map[string]inte return stageMountPoints, nil } +func (s *StageExecutor) createNeededHeredocMountsForRun(files []imagebuilder.File) ([]Mount, error) { + mountResult := []Mount{} + for _, file := range files { + f, err := os.CreateTemp("", "buildahheredoc") + if err != nil { + return nil, err + } + if _, err := f.WriteString(file.Data); err != nil { + f.Close() + return nil, err + } + err = f.Chmod(0755) + if err != nil { + f.Close() + return nil, err + } + // dest path is same as buildkit for compat + dest := filepath.Join("/dev/pipes/", filepath.Base(f.Name())) + mount := Mount{Destination: dest, Type: define.TypeBind, Source: f.Name(), Options: append(define.BindOptions, "rprivate", "z", "Z")} + mountResult = append(mountResult, mount) + f.Close() + } + return mountResult, nil +} + // Run executes a RUN instruction using the stage's current working container // as a root directory. func (s *StageExecutor) Run(run imagebuilder.Run, config docker.Config) error { logrus.Debugf("RUN %#v, %#v", run, config) + args := run.Args + heredocMounts := []Mount{} + if len(run.Files) > 0 { + if heredoc := buildkitparser.MustParseHeredoc(args[0]); heredoc != nil { + if strings.HasPrefix(run.Files[0].Data, "#!") || strings.HasPrefix(run.Files[0].Data, "\n#!") { + // This is a single heredoc with a shebang, so create a file + // and run it. + heredocMount, err := s.createNeededHeredocMountsForRun(run.Files) + if err != nil { + return err + } + args = []string{heredocMount[0].Destination} + heredocMounts = append(heredocMounts, heredocMount...) + } else { + args = []string{run.Files[0].Data} + } + } else { + full := args[0] + for _, file := range run.Files { + full += file.Data + "\n" + file.Name + } + args = []string{full} + } + } stageMountPoints, err := s.runStageMountPoints(run.Mounts) if err != nil { return err @@ -657,7 +770,6 @@ func (s *StageExecutor) Run(run imagebuilder.Run, config docker.Config) error { options.ConfigureNetwork = buildah.NetworkDisabled } - args := run.Args if run.Shell { if len(config.Shell) > 0 && s.builder.Format == define.Dockerv2ImageManifest { args = append(config.Shell, args...) @@ -670,6 +782,9 @@ func (s *StageExecutor) Run(run imagebuilder.Run, config docker.Config) error { return err } options.Mounts = append(options.Mounts, mounts...) + if len(heredocMounts) > 0 { + options.Mounts = append(options.Mounts, heredocMounts...) + } err = s.builder.Run(args, options) if err2 := s.volumeCacheRestore(); err2 != nil { if err == nil { diff --git a/tests/bud.bats b/tests/bud.bats index a4060f9226a..d9981b049e2 100644 --- a/tests/bud.bats +++ b/tests/bud.bats @@ -267,6 +267,41 @@ _EOF run_buildah 1 run myctr ls -l subdir/ } +@test "bud build with heredoc content" { + run_buildah build -t heredoc $WITH_POLICY_JSON -f $BUDFILES/heredoc/Containerfile . + expect_output --substring "print first line from heredoc" + expect_output --substring "print second line from heredoc" + expect_output --substring "Heredoc writing first file" + expect_output --substring "some text of first file" + expect_output --substring "file2 from python" + expect_output --substring "(your index page goes here)" + expect_output --substring "(robots content)" + expect_output --substring "(humans content)" + expect_output --substring "this is the output of test6 part1" + expect_output --substring "this is the output of test6 part2" + expect_output --substring "this is the output of test7 part1" + expect_output --substring "this is the output of test7 part2" + expect_output --substring "this is the output of test7 part3" + expect_output --substring "this is the output of test8 part1" + expect_output --substring "this is the output of test8 part2" +} + +@test "bud build with heredoc content which is a bash file" { + skip_if_in_container + _prefetch busybox + run_buildah build -t heredoc $WITH_POLICY_JSON -f $BUDFILES/heredoc/Containerfile.bash_file . + expect_output --substring "this is the output of test9" + expect_output --substring "this is the output of test10" +} + +@test "bud build with heredoc verify mount leak" { + skip_if_in_container + _prefetch alpine + run_buildah 1 build -t heredoc $WITH_POLICY_JSON -f $BUDFILES/heredoc/Containerfile.verify_mount_leak . + expect_output --substring "this is the output of test" + expect_output --substring "ls: /dev/pipes: No such file or directory" +} + @test "bud with .containerignore" { _prefetch alpine busybox run_buildah 125 build -t testbud $WITH_POLICY_JSON -f $BUDFILES/containerignore/Dockerfile $BUDFILES/containerignore diff --git a/tests/bud/heredoc/Containerfile b/tests/bud/heredoc/Containerfile new file mode 100644 index 00000000000..788b2ffa763 --- /dev/null +++ b/tests/bud/heredoc/Containerfile @@ -0,0 +1,59 @@ +FROM docker.io/library/python:latest + +RUN <> /file1 +echo "some text of first file" >> /file1 +EOF + +RUN cat file1 + +RUN python3 < test6.txt +this is the output of test6 part1 +FILE1 +this is the output of test6 part2 +FILE2 + +RUN 5< test7.txt +this is the output of test7 part1 +file +this is the output of test7 part2 +FILE +this is the output of test7 part3 +File + +RUN < test8.1 && < test8.2 +this is the output of test8 part1 +FILE1 +this is the output of test8 part2 +FILE2 + +RUN cat /test/robots.txt +RUN cat /test/humans.txt +RUN cat test6.txt +RUN cat test7.txt +RUN cat test8.1 +RUN cat test8.2 diff --git a/tests/bud/heredoc/Containerfile.bash_file b/tests/bud/heredoc/Containerfile.bash_file new file mode 100644 index 00000000000..89032a11135 --- /dev/null +++ b/tests/bud/heredoc/Containerfile.bash_file @@ -0,0 +1,15 @@ +FROM busybox +RUN < test9.txt +EOF + +RUN <<-EOF +#!/bin/sh +echo " + this is the output of test10" > test10.txt +EOF + +RUN cat test9.txt +RUN cat test10.txt diff --git a/tests/bud/heredoc/Containerfile.verify_mount_leak b/tests/bud/heredoc/Containerfile.verify_mount_leak new file mode 100644 index 00000000000..753fd892b37 --- /dev/null +++ b/tests/bud/heredoc/Containerfile.verify_mount_leak @@ -0,0 +1,17 @@ +FROM alpine + +RUN < test.txt +# Mount of this file must exists till this run step +# so this `ls` command should not fail +ls -a /dev/pipes/ +EOF + +RUN cat test.txt + +# This ls command must fail, since mount is removed in this step +RUN ls -a /dev/pipes + + diff --git a/tests/conformance/conformance_test.go b/tests/conformance/conformance_test.go index 0de897206b8..6ec8f38371b 100644 --- a/tests/conformance/conformance_test.go +++ b/tests/conformance/conformance_test.go @@ -3003,6 +3003,26 @@ var internalTestCases = []testCase{ dockerUseBuildKit: true, }, + { + name: "heredoc-copy", + dockerfile: "Dockerfile.heredoc_copy", + dockerUseBuildKit: true, + contextDir: "heredoc", + fsSkip: []string{"(dir):test:mtime", + "(dir):test2:mtime", + "(dir):test:(dir):humans.txt:mtime", + "(dir):test:(dir):robots.txt:mtime", + "(dir):test:(dir):file:mtime", + "(dir):test:(dir):file:uid", /* buildkit's regular COPY sets UID which is inconsistent with docker and buildah*/ + "(dir):test:(dir):file:gid", /* buildkit's regular COPY sets GID which is inconsistent with docker and buildah*/ + "(dir):test2:(dir):humans.txt:mtime", + "(dir):test2:(dir):robots.txt:mtime", + "(dir):test2:(dir):image_file:mtime", + "(dir):test2:(dir):image_file:uid", /* buildkit's regular COPY sets UID which is inconsistent with docker and buildah*/ + "(dir):test2:(dir):image_file:gid", /* buildkit's regular COPY sets GID which is inconsistent with docker and buildah*/ + "(dir):etc:(dir):hostname" /* buildkit does not contains /etc/hostname like buildah */}, + }, + { name: "replace-symlink-with-directory", contextDir: "replace/symlink-with-directory", diff --git a/tests/conformance/testdata/heredoc/Dockerfile.heredoc_copy b/tests/conformance/testdata/heredoc/Dockerfile.heredoc_copy new file mode 100644 index 00000000000..227f71b5eea --- /dev/null +++ b/tests/conformance/testdata/heredoc/Dockerfile.heredoc_copy @@ -0,0 +1,23 @@ +# syntax=docker/dockerfile:1.3-labs +FROM busybox as one +RUN echo helloworld > image_file +FROM busybox +RUN echo hello +# copy two heredoc and one from context +COPY <