From ca892e4f1a95f16c88bc6a7246970405fc04a3d7 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 7 Nov 2017 18:27:49 -0500 Subject: [PATCH 1/2] Fix dockerfile parser failing silently on long tokens Signed-off-by: Daniel Nephin Upstream-commit: 59ad3a36e2684bd36a4b02179949bd17f1406918 Component: engine --- .../engine/builder/dockerfile/parser/parser.go | 11 ++++++++++- .../engine/builder/dockerfile/parser/parser_test.go | 13 +++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/components/engine/builder/dockerfile/parser/parser.go b/components/engine/builder/dockerfile/parser/parser.go index 822c42b41a..a4bce1cca6 100644 --- a/components/engine/builder/dockerfile/parser/parser.go +++ b/components/engine/builder/dockerfile/parser/parser.go @@ -321,7 +321,7 @@ func Parse(rwc io.Reader) (*Result, error) { Warnings: warnings, EscapeToken: d.escapeToken, OS: d.platformToken, - }, nil + }, handleScannerError(scanner.Err()) } func trimComments(src []byte) []byte { @@ -358,3 +358,12 @@ func processLine(d *Directive, token []byte, stripLeftWhitespace bool) ([]byte, } return trimComments(token), d.possibleParserDirective(string(token)) } + +func handleScannerError(err error) error { + switch err { + case bufio.ErrTooLong: + return errors.Errorf("dockerfile line greater than max allowed size of %d", bufio.MaxScanTokenSize-1) + default: + return err + } +} diff --git a/components/engine/builder/dockerfile/parser/parser_test.go b/components/engine/builder/dockerfile/parser/parser_test.go index 5f354bb139..7bfbee92e4 100644 --- a/components/engine/builder/dockerfile/parser/parser_test.go +++ b/components/engine/builder/dockerfile/parser/parser_test.go @@ -1,12 +1,14 @@ package parser import ( + "bufio" "bytes" "fmt" "io/ioutil" "os" "path/filepath" "runtime" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -159,3 +161,14 @@ RUN indented \ assert.Contains(t, warnings[1], "RUN another thing") assert.Contains(t, warnings[2], "will become errors in a future release") } + +func TestParseReturnsScannerErrors(t *testing.T) { + label := strings.Repeat("a", bufio.MaxScanTokenSize) + + dockerfile := strings.NewReader(fmt.Sprintf(` + FROM image + LABEL test=%s +`, label)) + _, err := Parse(dockerfile) + assert.EqualError(t, err, "dockerfile line greater than max allowed size of 65535") +} From 84fe2b7d9a236ee8b9cd9fce4f46c41ad485be13 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 8 Nov 2017 13:06:57 -0500 Subject: [PATCH 2/2] Fix remote build target as Dockerfile The test was passing previously because the preamble was already buffered. After the change to return Scanner.Err() the final read error on the buffer was no longer being ignored. Signed-off-by: Daniel Nephin Upstream-commit: a74cc833450dfc48cc95b2b109cbcb24feff4929 Component: engine --- .../engine/builder/remotecontext/archive.go | 1 - .../engine/builder/remotecontext/detect.go | 33 +++++----- .../engine/builder/remotecontext/remote.go | 65 ++++++------------- .../builder/remotecontext/remote_test.go | 49 ++++---------- 4 files changed, 46 insertions(+), 102 deletions(-) diff --git a/components/engine/builder/remotecontext/archive.go b/components/engine/builder/remotecontext/archive.go index b62d9dd0b7..c670235b61 100644 --- a/components/engine/builder/remotecontext/archive.go +++ b/components/engine/builder/remotecontext/archive.go @@ -79,7 +79,6 @@ func FromArchive(tarStream io.Reader) (builder.Source, error) { } tsc.sums = sum.GetSums() - return tsc, nil } diff --git a/components/engine/builder/remotecontext/detect.go b/components/engine/builder/remotecontext/detect.go index 38aff67985..fe5cd967fb 100644 --- a/components/engine/builder/remotecontext/detect.go +++ b/components/engine/builder/remotecontext/detect.go @@ -97,26 +97,23 @@ func newGitRemote(gitURL string, dockerfilePath string) (builder.Source, *parser } func newURLRemote(url string, dockerfilePath string, progressReader func(in io.ReadCloser) io.ReadCloser) (builder.Source, *parser.Result, error) { - var dockerfile io.ReadCloser - dockerfileFoundErr := errors.New("found-dockerfile") - c, err := MakeRemoteContext(url, map[string]func(io.ReadCloser) (io.ReadCloser, error){ - mimeTypes.TextPlain: func(rc io.ReadCloser) (io.ReadCloser, error) { - dockerfile = rc - return nil, dockerfileFoundErr - }, - // fallback handler (tar context) - "": func(rc io.ReadCloser) (io.ReadCloser, error) { - return progressReader(rc), nil - }, - }) - switch { - case err == dockerfileFoundErr: - res, err := parser.Parse(dockerfile) - return nil, res, err - case err != nil: + contentType, content, err := downloadRemote(url) + if err != nil { return nil, nil, err } - return withDockerfileFromContext(c.(modifiableContext), dockerfilePath) + defer content.Close() + + switch contentType { + case mimeTypes.TextPlain: + res, err := parser.Parse(progressReader(content)) + return nil, res, err + default: + source, err := FromArchive(progressReader(content)) + if err != nil { + return nil, nil, err + } + return withDockerfileFromContext(source.(modifiableContext), dockerfilePath) + } } func removeDockerfile(c modifiableContext, filesToRemove ...string) error { diff --git a/components/engine/builder/remotecontext/remote.go b/components/engine/builder/remotecontext/remote.go index 6c4073bd4c..ee0282f706 100644 --- a/components/engine/builder/remotecontext/remote.go +++ b/components/engine/builder/remotecontext/remote.go @@ -10,7 +10,7 @@ import ( "net/url" "regexp" - "github.com/docker/docker/builder" + "github.com/docker/docker/pkg/ioutils" "github.com/pkg/errors" ) @@ -22,50 +22,23 @@ const acceptableRemoteMIME = `(?:application/(?:(?:x\-)?tar|octet\-stream|((?:x\ var mimeRe = regexp.MustCompile(acceptableRemoteMIME) -// MakeRemoteContext downloads a context from remoteURL and returns it. -// -// If contentTypeHandlers is non-nil, then the Content-Type header is read along with a maximum of -// maxPreambleLength bytes from the body to help detecting the MIME type. -// Look at acceptableRemoteMIME for more details. -// -// If a match is found, then the body is sent to the contentType handler and a (potentially compressed) tar stream is expected -// to be returned. If no match is found, it is assumed the body is a tar stream (compressed or not). -// In either case, an (assumed) tar stream is passed to FromArchive whose result is returned. -func MakeRemoteContext(remoteURL string, contentTypeHandlers map[string]func(io.ReadCloser) (io.ReadCloser, error)) (builder.Source, error) { - f, err := GetWithStatusError(remoteURL) +// downloadRemote context from a url and returns it, along with the parsed content type +func downloadRemote(remoteURL string) (string, io.ReadCloser, error) { + response, err := GetWithStatusError(remoteURL) if err != nil { - return nil, fmt.Errorf("error downloading remote context %s: %v", remoteURL, err) - } - defer f.Body.Close() - - var contextReader io.ReadCloser - if contentTypeHandlers != nil { - contentType := f.Header.Get("Content-Type") - clen := f.ContentLength - - contentType, contextReader, err = inspectResponse(contentType, f.Body, clen) - if err != nil { - return nil, fmt.Errorf("error detecting content type for remote %s: %v", remoteURL, err) - } - defer contextReader.Close() - - // This loop tries to find a content-type handler for the detected content-type. - // If it could not find one from the caller-supplied map, it tries the empty content-type `""` - // which is interpreted as a fallback handler (usually used for raw tar contexts). - for _, ct := range []string{contentType, ""} { - if fn, ok := contentTypeHandlers[ct]; ok { - defer contextReader.Close() - if contextReader, err = fn(contextReader); err != nil { - return nil, err - } - break - } - } + return "", nil, fmt.Errorf("error downloading remote context %s: %v", remoteURL, err) } - // Pass through - this is a pre-packaged context, presumably - // with a Dockerfile with the right name inside it. - return FromArchive(contextReader) + contentType, contextReader, err := inspectResponse( + response.Header.Get("Content-Type"), + response.Body, + response.ContentLength) + if err != nil { + response.Body.Close() + return "", nil, fmt.Errorf("error detecting content type for remote %s: %v", remoteURL, err) + } + + return contentType, ioutils.NewReadCloserWrapper(contextReader, response.Body.Close), nil } // GetWithStatusError does an http.Get() and returns an error if the @@ -110,7 +83,7 @@ func GetWithStatusError(address string) (resp *http.Response, err error) { // - an io.Reader for the response body // - an error value which will be non-nil either when something goes wrong while // reading bytes from r or when the detected content-type is not acceptable. -func inspectResponse(ct string, r io.Reader, clen int64) (string, io.ReadCloser, error) { +func inspectResponse(ct string, r io.Reader, clen int64) (string, io.Reader, error) { plen := clen if plen <= 0 || plen > maxPreambleLength { plen = maxPreambleLength @@ -119,14 +92,14 @@ func inspectResponse(ct string, r io.Reader, clen int64) (string, io.ReadCloser, preamble := make([]byte, plen) rlen, err := r.Read(preamble) if rlen == 0 { - return ct, ioutil.NopCloser(r), errors.New("empty response") + return ct, r, errors.New("empty response") } if err != nil && err != io.EOF { - return ct, ioutil.NopCloser(r), err + return ct, r, err } preambleR := bytes.NewReader(preamble[:rlen]) - bodyReader := ioutil.NopCloser(io.MultiReader(preambleR, r)) + bodyReader := io.MultiReader(preambleR, r) // Some web servers will use application/octet-stream as the default // content type for files without an extension (e.g. 'Dockerfile') // so if we receive this value we better check for text content diff --git a/components/engine/builder/remotecontext/remote_test.go b/components/engine/builder/remotecontext/remote_test.go index c38433b340..35b105f550 100644 --- a/components/engine/builder/remotecontext/remote_test.go +++ b/components/engine/builder/remotecontext/remote_test.go @@ -11,7 +11,7 @@ import ( "github.com/docker/docker/builder" "github.com/docker/docker/internal/testutil" - "github.com/docker/docker/pkg/archive" + "github.com/gotestyourself/gotestyourself/fs" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -174,11 +174,10 @@ func TestUnknownContentLength(t *testing.T) { } } -func TestMakeRemoteContext(t *testing.T) { - contextDir, cleanup := createTestTempDir(t, "", "builder-tarsum-test") - defer cleanup() - - createTestTempFile(t, contextDir, builder.DefaultDockerfileName, dockerfileContents, 0777) +func TestDownloadRemote(t *testing.T) { + contextDir := fs.NewDir(t, "test-builder-download-remote", + fs.WithFile(builder.DefaultDockerfileName, dockerfileContents)) + defer contextDir.Remove() mux := http.NewServeMux() server := httptest.NewServer(mux) @@ -187,39 +186,15 @@ func TestMakeRemoteContext(t *testing.T) { serverURL.Path = "/" + builder.DefaultDockerfileName remoteURL := serverURL.String() - mux.Handle("/", http.FileServer(http.Dir(contextDir))) + mux.Handle("/", http.FileServer(http.Dir(contextDir.Path()))) - remoteContext, err := MakeRemoteContext(remoteURL, map[string]func(io.ReadCloser) (io.ReadCloser, error){ - mimeTypes.TextPlain: func(rc io.ReadCloser) (io.ReadCloser, error) { - dockerfile, err := ioutil.ReadAll(rc) - if err != nil { - return nil, err - } + contentType, content, err := downloadRemote(remoteURL) + require.NoError(t, err) - r, err := archive.Generate(builder.DefaultDockerfileName, string(dockerfile)) - if err != nil { - return nil, err - } - return ioutil.NopCloser(r), nil - }, - }) - - if err != nil { - t.Fatalf("Error when executing DetectContextFromRemoteURL: %s", err) - } - - if remoteContext == nil { - t.Fatal("Remote context should not be nil") - } - - h, err := remoteContext.Hash(builder.DefaultDockerfileName) - if err != nil { - t.Fatalf("failed to compute hash %s", err) - } - - if expected, actual := "7b6b6b66bee9e2102fbdc2228be6c980a2a23adf371962a37286a49f7de0f7cc", h; expected != actual { - t.Fatalf("There should be file named %s %s in fileInfoSums", expected, actual) - } + assert.Equal(t, mimeTypes.TextPlain, contentType) + raw, err := ioutil.ReadAll(content) + require.NoError(t, err) + assert.Equal(t, dockerfileContents, string(raw)) } func TestGetWithStatusError(t *testing.T) {