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 <dnephin@docker.com>

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