From a74cc833450dfc48cc95b2b109cbcb24feff4929 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 8 Nov 2017 13:06:57 -0500 Subject: [PATCH] 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 --- builder/remotecontext/archive.go | 1 - builder/remotecontext/detect.go | 33 +++++++------- builder/remotecontext/remote.go | 65 ++++++++-------------------- builder/remotecontext/remote_test.go | 49 +++++---------------- 4 files changed, 46 insertions(+), 102 deletions(-) diff --git a/builder/remotecontext/archive.go b/builder/remotecontext/archive.go index b62d9dd0b7..c670235b61 100644 --- a/builder/remotecontext/archive.go +++ b/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/builder/remotecontext/detect.go b/builder/remotecontext/detect.go index 38aff67985..fe5cd967fb 100644 --- a/builder/remotecontext/detect.go +++ b/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/builder/remotecontext/remote.go b/builder/remotecontext/remote.go index 6c4073bd4c..ee0282f706 100644 --- a/builder/remotecontext/remote.go +++ b/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/builder/remotecontext/remote_test.go b/builder/remotecontext/remote_test.go index c38433b340..35b105f550 100644 --- a/builder/remotecontext/remote_test.go +++ b/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) {