Merge pull request #35429 from dnephin/build-fails-on-long-label

Fix dockerfile parser failing silently on long tokens

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
+	}
+}

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")
+}

builder/remotecontext/archive.go

@@ -79,7 +79,6 @@ func FromArchive(tarStream io.Reader) (builder.Source, error) {
 	}
 
 	tsc.sums = sum.GetSums()
-
 	return tsc, nil
 }
 

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 {

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

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) {