From 97382f92888bd415f03d82132ca2e379d74a1bde Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Thu, 11 Apr 2024 15:48:57 -0400 Subject: [PATCH 1/2] Don't expand RUN heredocs ourselves, let the shell do it When handling RUN instructions that use heredoc syntax, don't bother interpolating environment variables and argument values, and let the command that's running handle it. Signed-off-by: Nalin Dahyabhai --- docker/types.go | 7 +- go.mod | 4 +- go.sum | 8 +- tests/conformance/conformance_test.go | 7 + .../testdata/Dockerfile.heredoc-quoting | 215 ++++++++++++++++++ .../fsouza/go-dockerclient/container.go | 8 +- .../openshift/imagebuilder/.travis.yml | 4 + .../openshift/imagebuilder/dispatchers.go | 18 +- .../imagebuilder/dockerclient/client.go | 2 +- .../openshift/imagebuilder/imagebuilder.spec | 4 +- vendor/modules.txt | 6 +- 11 files changed, 260 insertions(+), 23 deletions(-) create mode 100644 tests/conformance/testdata/Dockerfile.heredoc-quoting diff --git a/docker/types.go b/docker/types.go index b0ed2e4c02..275951d039 100644 --- a/docker/types.go +++ b/docker/types.go @@ -60,9 +60,10 @@ type HealthConfig struct { Test []string `json:",omitempty"` // Zero means to inherit. Durations are expressed as integer nanoseconds. - Interval time.Duration `json:",omitempty"` // Interval is the time to wait between checks. - Timeout time.Duration `json:",omitempty"` // Timeout is the time to wait before considering the check to have hung. - StartPeriod time.Duration `json:",omitempty"` // Time to wait after the container starts before running the first check. + Interval time.Duration `json:",omitempty"` // Interval is the time to wait between checks. + Timeout time.Duration `json:",omitempty"` // Timeout is the time to wait before considering the check to have hung. + StartPeriod time.Duration `json:",omitempty"` // Time to wait after the container starts before running the first check. + StartInterval time.Duration `json:",omitempty"` // Time to wait between checks during the StartPeriod. // Retries is the number of consecutive failures needed to consider a container as unhealthy. // Zero means inherit. diff --git a/go.mod b/go.mod index c51f7988f3..1e2c1109f7 100644 --- a/go.mod +++ b/go.mod @@ -29,7 +29,7 @@ require ( github.com/docker/distribution v2.8.3+incompatible github.com/docker/docker v26.1.2+incompatible github.com/docker/go-units v0.5.0 - github.com/fsouza/go-dockerclient v1.10.1 + github.com/fsouza/go-dockerclient v1.11.0 github.com/hashicorp/go-multierror v1.1.1 github.com/mattn/go-shellwords v1.0.12 github.com/moby/buildkit v0.12.5 @@ -41,7 +41,7 @@ require ( github.com/opencontainers/runtime-spec v1.2.0 github.com/opencontainers/runtime-tools v0.9.1-0.20230914150019-408c51e934dc github.com/opencontainers/selinux v1.11.0 - github.com/openshift/imagebuilder v1.2.6 + github.com/openshift/imagebuilder v1.2.9 github.com/seccomp/libseccomp-golang v0.10.0 github.com/sirupsen/logrus v1.9.3 github.com/spf13/cobra v1.8.0 diff --git a/go.sum b/go.sum index c6eee6c539..c044a30a08 100644 --- a/go.sum +++ b/go.sum @@ -116,8 +116,8 @@ github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMo github.com/fsnotify/fsnotify v1.4.9/go.mod h1:znqG4EE+3YCdAaPaxE2ZRY/06pZUdp0tY4IgpuI1SZQ= github.com/fsnotify/fsnotify v1.7.0 h1:8JEhPFa5W2WU7YfeZzPNqzMP6Lwt7L2715Ggo0nosvA= github.com/fsnotify/fsnotify v1.7.0/go.mod h1:40Bi/Hjc2AVfZrqy+aj+yEI+/bRxZnMJyTJwOpGvigM= -github.com/fsouza/go-dockerclient v1.10.1 h1:bSU5Wu2ARdub+iv9VtoDsN8yBUI0vgflmshbeQLKhvc= -github.com/fsouza/go-dockerclient v1.10.1/go.mod h1:dyzGriw6v3pK4O4O1u/X+vXxDDsrnLLkCqYkcLsDq2k= +github.com/fsouza/go-dockerclient v1.11.0 h1:4ZAk6W7rPAtPXm7198EFqA5S68rwnNQORxlOA5OurCA= +github.com/fsouza/go-dockerclient v1.11.0/go.mod h1:0I3TQCRseuPTzqlY4Y3ajfsg2VAdMQoazrkxJTiJg8s= github.com/go-jose/go-jose/v3 v3.0.3 h1:fFKWeig/irsp7XD2zBxvnmA/XaRWp5V3CBsZXJF7G7k= github.com/go-jose/go-jose/v3 v3.0.3/go.mod h1:5b+7YgP7ZICgJDBdfjZaIt+H/9L9T/YQrVfLAMboGkQ= github.com/go-logr/logr v0.1.0/go.mod h1:ixOQHD9gLJUVQQ2ZOR7zLEifBX6tGkNJF4QyIY7sIas= @@ -294,8 +294,8 @@ github.com/opencontainers/runtime-tools v0.9.1-0.20230914150019-408c51e934dc h1: github.com/opencontainers/runtime-tools v0.9.1-0.20230914150019-408c51e934dc/go.mod h1:8tx1helyqhUC65McMm3x7HmOex8lO2/v9zPuxmKHurs= github.com/opencontainers/selinux v1.11.0 h1:+5Zbo97w3Lbmb3PeqQtpmTkMwsW5nRI3YaLpt7tQ7oU= github.com/opencontainers/selinux v1.11.0/go.mod h1:E5dMC3VPuVvVHDYmi78qvhJp8+M586T4DlDRYpFkyec= -github.com/openshift/imagebuilder v1.2.6 h1:ge+HILDVaB3c65KhH0nrM/Z1f9EdN8NUqxigd4qGqqo= -github.com/openshift/imagebuilder v1.2.6/go.mod h1:6VbTJ5CK7+OOTWcQlc/Cp86ML7pKlxOwCJNESQPbtgw= +github.com/openshift/imagebuilder v1.2.9 h1:830/kg5FWtpLsQ6JcCQ23qOeb/KfzMK66pai544rAUI= +github.com/openshift/imagebuilder v1.2.9/go.mod h1:KkkXOyRjJlZEXWQtHNBNzVHqh4vf/0xX5cDIQ2gr+5I= github.com/ostreedev/ostree-go v0.0.0-20210805093236-719684c64e4f h1:/UDgs8FGMqwnHagNDPGOlts35QkhAZ8by3DR7nMih7M= github.com/ostreedev/ostree-go v0.0.0-20210805093236-719684c64e4f/go.mod h1:J6OG6YJVEWopen4avK3VNQSnALmmjvniMmni/YFYAwc= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= diff --git a/tests/conformance/conformance_test.go b/tests/conformance/conformance_test.go index 6432cc19db..8de8bdd773 100644 --- a/tests/conformance/conformance_test.go +++ b/tests/conformance/conformance_test.go @@ -3102,6 +3102,13 @@ var internalTestCases = []testCase{ contextDir: "multistage/copyback", dockerUseBuildKit: true, }, + + { + name: "heredoc-quoting", + dockerfile: "Dockerfile.heredoc-quoting", + dockerUseBuildKit: true, + fsSkip: []string{"(dir):etc:(dir):hostname"}, // buildkit does not create a phantom /etc/hostname + }, } func TestCommit(t *testing.T) { diff --git a/tests/conformance/testdata/Dockerfile.heredoc-quoting b/tests/conformance/testdata/Dockerfile.heredoc-quoting new file mode 100644 index 0000000000..cf83b36439 --- /dev/null +++ b/tests/conformance/testdata/Dockerfile.heredoc-quoting @@ -0,0 +1,215 @@ +FROM busybox +ARG argA=argvA +ENV varA=valueA + +# An argument, an environment variable, and one set in the heredoc +RUN <, --chown=, and --checksum= flags") } } - files, err := processHereDocs(original, heredocs, userArgs) + files, err := processHereDocs(buildkitcommand.Add, original, heredocs, userArgs) if err != nil { return err } @@ -256,7 +257,7 @@ func dispatchCopy(b *Builder, args []string, attributes map[string]bool, flagArg return fmt.Errorf("COPY only supports the --chmod= --chown= and the --from= flags") } } - files, err := processHereDocs(original, heredocs, userArgs) + files, err := processHereDocs(buildkitcommand.Copy, original, heredocs, userArgs) if err != nil { return err } @@ -422,7 +423,7 @@ func run(b *Builder, args []string, attributes map[string]bool, flagArgs []strin } } - files, err := processHereDocs(original, heredocs, userArgs) + files, err := processHereDocs(buildkitcommand.Run, original, heredocs, userArgs) if err != nil { return err } @@ -606,6 +607,7 @@ func healthcheck(b *Builder, args []string, attributes map[string]bool, flagArgs flags := flag.NewFlagSet("", flag.ContinueOnError) flags.String("start-period", "", "") + flags.String("start-interval", "", "") flags.String("interval", "", "") flags.String("timeout", "", "") flRetries := flags.String("retries", "", "") @@ -642,6 +644,12 @@ func healthcheck(b *Builder, args []string, attributes map[string]bool, flagArgs } healthcheck.Interval = interval + startInterval, err := parseOptInterval(flags.Lookup("start-interval")) + if err != nil { + return err + } + healthcheck.StartInterval = startInterval + timeout, err := parseOptInterval(flags.Lookup("timeout")) if err != nil { return err diff --git a/vendor/github.com/openshift/imagebuilder/dockerclient/client.go b/vendor/github.com/openshift/imagebuilder/dockerclient/client.go index 1e8c83f869..7e9f35681a 100644 --- a/vendor/github.com/openshift/imagebuilder/dockerclient/client.go +++ b/vendor/github.com/openshift/imagebuilder/dockerclient/client.go @@ -1058,7 +1058,7 @@ func (e *ClientExecutor) CopyContainer(container *docker.Container, excludes []s } chmod = func(h *tar.Header, r io.Reader) (data []byte, update bool, skip bool, err error) { mode := h.Mode &^ 0o777 - mode |= parsed & 0o777 + mode |= parsed & 0o7777 h.Mode = mode return nil, false, false, nil } diff --git a/vendor/github.com/openshift/imagebuilder/imagebuilder.spec b/vendor/github.com/openshift/imagebuilder/imagebuilder.spec index 5d4f2b7013..7819bdc8ba 100644 --- a/vendor/github.com/openshift/imagebuilder/imagebuilder.spec +++ b/vendor/github.com/openshift/imagebuilder/imagebuilder.spec @@ -11,8 +11,8 @@ # Customize from here. # -%global golang_version 1.8.1 -%{!?version: %global version 1.2.6} +%global golang_version 1.19 +%{!?version: %global version 1.2.9} %{!?release: %global release 1} %global package_name imagebuilder %global product_name Container Image Builder diff --git a/vendor/modules.txt b/vendor/modules.txt index 736b2c76cb..b228576f5e 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -377,8 +377,8 @@ github.com/felixge/httpsnoop # github.com/fsnotify/fsnotify v1.7.0 ## explicit; go 1.17 github.com/fsnotify/fsnotify -# github.com/fsouza/go-dockerclient v1.10.1 -## explicit; go 1.20 +# github.com/fsouza/go-dockerclient v1.11.0 +## explicit; go 1.21 github.com/fsouza/go-dockerclient # github.com/go-jose/go-jose/v3 v3.0.3 ## explicit; go 1.12 @@ -644,7 +644,7 @@ github.com/opencontainers/selinux/go-selinux github.com/opencontainers/selinux/go-selinux/label github.com/opencontainers/selinux/pkg/pwalk github.com/opencontainers/selinux/pkg/pwalkdir -# github.com/openshift/imagebuilder v1.2.6 +# github.com/openshift/imagebuilder v1.2.9 ## explicit; go 1.19 github.com/openshift/imagebuilder github.com/openshift/imagebuilder/dockerclient From d9191e17cdc504d8df9328fa156071dc5ac1dbff Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Wed, 8 May 2024 10:07:11 -0400 Subject: [PATCH 2/2] copierWithSubprocess(): try to capture stderr on io.ErrClosedPipe When we get a tried-to-write-to-closed-pipe error while encoding something for a coprocess, try to capture error output from the coprocess and add it to the error message, to hopefully catch a flake we're seeing in CI. Signed-off-by: Nalin Dahyabhai --- copier/copier.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/copier/copier.go b/copier/copier.go index babab38886..0dfdca3970 100644 --- a/copier/copier.go +++ b/copier/copier.go @@ -18,6 +18,7 @@ import ( "sync" "syscall" "time" + "unicode" "github.com/containers/image/v5/pkg/compression" "github.com/containers/storage/pkg/archive" @@ -633,6 +634,15 @@ func copierWithSubprocess(bulkReader io.Reader, bulkWriter io.Writer, req reques if err2 := cmd.Process.Kill(); err2 != nil { return nil, fmt.Errorf("killing subprocess: %v; %s: %w", err2, step, err) } + if errors.Is(err, io.ErrClosedPipe) || errors.Is(err, syscall.EPIPE) { + err2 := cmd.Wait() + if errorText := strings.TrimFunc(errorBuffer.String(), unicode.IsSpace); errorText != "" { + err = fmt.Errorf("%s: %w", errorText, err) + } + if err2 != nil { + return nil, fmt.Errorf("waiting on subprocess: %v; %s: %w", err2, step, err) + } + } return nil, fmt.Errorf("%v: %w", step, err) } if err = encoder.Encode(req); err != nil {