From ac38bbaf63f0bd3e6c18951a79e96374ab75aec3 Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Fri, 21 Jul 2017 23:54:29 +0000 Subject: [PATCH 1/2] Fix build with `ADD` urls without any sub path This fix tries to address the issue raised in #34208 where in Dockerfile an `ADD` followed by an url without any sub path will cause an error. The issue is because the temporary filename relies on the sub path. An integration test has been added. This fix fixes #34208. Signed-off-by: Yong Tang Upstream-commit: bea0a072d86604071c99e9b6989b19ca4fe22032 Component: engine --- components/engine/builder/dockerfile/copy.go | 57 +++++++++++++++---- .../integration-cli/docker_cli_build_test.go | 16 ++++++ 2 files changed, 62 insertions(+), 11 deletions(-) diff --git a/components/engine/builder/dockerfile/copy.go b/components/engine/builder/dockerfile/copy.go index 223623ccd6..3de63d542e 100644 --- a/components/engine/builder/dockerfile/copy.go +++ b/components/engine/builder/dockerfile/copy.go @@ -3,6 +3,7 @@ package dockerfile import ( "fmt" "io" + "mime" "net/http" "net/url" "os" @@ -24,6 +25,8 @@ import ( "github.com/pkg/errors" ) +const unnamedFilename = "__unnamed__" + type pathCache interface { Load(key interface{}) (value interface{}, ok bool) Store(key, value interface{}) @@ -85,8 +88,7 @@ func (o *copier) createCopyInstruction(args []string, cmdName string) (copyInstr // Work in daemon-specific filepath semantics inst.dest = filepath.FromSlash(args[last]) - - infos, err := o.getCopyInfosForSourcePaths(args[0:last]) + infos, err := o.getCopyInfosForSourcePaths(args[0:last], inst.dest) if err != nil { return inst, errors.Wrapf(err, "%s failed", cmdName) } @@ -99,10 +101,11 @@ func (o *copier) createCopyInstruction(args []string, cmdName string) (copyInstr // getCopyInfosForSourcePaths iterates over the source files and calculate the info // needed to copy (e.g. hash value if cached) -func (o *copier) getCopyInfosForSourcePaths(sources []string) ([]copyInfo, error) { +// The dest is used in case source is URL (and ends with "/") +func (o *copier) getCopyInfosForSourcePaths(sources []string, dest string) ([]copyInfo, error) { var infos []copyInfo for _, orig := range sources { - subinfos, err := o.getCopyInfoForSourcePath(orig) + subinfos, err := o.getCopyInfoForSourcePath(orig, dest) if err != nil { return nil, err } @@ -115,7 +118,7 @@ func (o *copier) getCopyInfosForSourcePaths(sources []string) ([]copyInfo, error return infos, nil } -func (o *copier) getCopyInfoForSourcePath(orig string) ([]copyInfo, error) { +func (o *copier) getCopyInfoForSourcePath(orig, dest string) ([]copyInfo, error) { if !urlutil.IsURL(orig) { return o.calcCopyInfo(orig, true) } @@ -123,6 +126,14 @@ func (o *copier) getCopyInfoForSourcePath(orig string) ([]copyInfo, error) { if err != nil { return nil, err } + // If path == "" then we are unable to determine filename from src + // We have to make sure dest is available + if path == "" { + if strings.HasSuffix(dest, "/") { + return nil, errors.Errorf("cannot determine filename for source %s", orig) + } + path = unnamedFilename + } o.tmpPaths = append(o.tmpPaths, remote.Root()) hash, err := remote.Hash(path) @@ -301,22 +312,40 @@ func errOnSourceDownload(_ string) (builder.Source, string, error) { return nil, "", errors.New("source can't be a URL for COPY") } +func getFilenameForDownload(path string, resp *http.Response) string { + // Guess filename based on source + if path != "" && !strings.HasSuffix(path, "/") { + if filename := filepath.Base(filepath.FromSlash(path)); filename != "" { + return filename + } + } + + // Guess filename based on Content-Disposition + if contentDisposition := resp.Header.Get("Content-Disposition"); contentDisposition != "" { + if _, params, err := mime.ParseMediaType(contentDisposition); err == nil { + if params["filename"] != "" && !strings.HasSuffix(params["filename"], "/") { + if filename := filepath.Base(filepath.FromSlash(params["filename"])); filename != "" { + return filename + } + } + } + } + return "" +} + func downloadSource(output io.Writer, stdout io.Writer, srcURL string) (remote builder.Source, p string, err error) { u, err := url.Parse(srcURL) if err != nil { return } - filename := filepath.Base(filepath.FromSlash(u.Path)) // Ensure in platform semantics - if filename == "" { - err = errors.Errorf("cannot determine filename from url: %s", u) - return - } resp, err := remotecontext.GetWithStatusError(srcURL) if err != nil { return } + filename := getFilenameForDownload(u.Path, resp) + // Prepare file in a tmp dir tmpDir, err := ioutils.TempDir("", "docker-remote") if err != nil { @@ -327,7 +356,13 @@ func downloadSource(output io.Writer, stdout io.Writer, srcURL string) (remote b os.RemoveAll(tmpDir) } }() - tmpFileName := filepath.Join(tmpDir, filename) + // If filename is empty, the returned filename will be "" but + // the tmp filename will be created as "__unnamed__" + tmpFileName := filename + if filename == "" { + tmpFileName = unnamedFilename + } + tmpFileName = filepath.Join(tmpDir, tmpFileName) tmpFile, err := os.OpenFile(tmpFileName, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0600) if err != nil { return diff --git a/components/engine/integration-cli/docker_cli_build_test.go b/components/engine/integration-cli/docker_cli_build_test.go index 59213e5404..3dedf45f31 100644 --- a/components/engine/integration-cli/docker_cli_build_test.go +++ b/components/engine/integration-cli/docker_cli_build_test.go @@ -6506,3 +6506,19 @@ RUN touch /foop c.Assert(err, check.IsNil) c.Assert(d.String(), checker.Equals, getIDByName(c, name)) } + +// Test case for #34208 +func (s *DockerSuite) TestBuildAddHTTPRoot(c *check.C) { + testRequires(c, Network, DaemonIsLinux) + buildImageSuccessfully(c, "buildaddhttproot", build.WithDockerfile(` + FROM scratch + ADD http://example.com/index.html /example1 + ADD http://example.com /example2 + ADD http://example.com /example3`)) + buildImage("buildaddhttprootfailure", build.WithDockerfile(` + FROM scratch + ADD http://example.com/ /`)).Assert(c, icmd.Expected{ + ExitCode: 1, + Err: "cannot determine filename for source http://example.com/", + }) +} From b3c31e9800cd6e709af55661f288780c50f1c8c8 Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Tue, 15 Aug 2017 00:33:31 +0000 Subject: [PATCH 2/2] Add unit test to cover changes. Signed-off-by: Yong Tang Upstream-commit: 027297a60f5f5a70e1e3eb3a68280f64c66bc877 Component: engine --- .../engine/builder/dockerfile/copy_test.go | 101 ++++++++++++++++++ .../integration-cli/docker_cli_build_test.go | 16 --- 2 files changed, 101 insertions(+), 16 deletions(-) diff --git a/components/engine/builder/dockerfile/copy_test.go b/components/engine/builder/dockerfile/copy_test.go index ed384fd20b..86f4dcff30 100644 --- a/components/engine/builder/dockerfile/copy_test.go +++ b/components/engine/builder/dockerfile/copy_test.go @@ -1,6 +1,7 @@ package dockerfile import ( + "net/http" "testing" "github.com/gotestyourself/gotestyourself/fs" @@ -43,3 +44,103 @@ func TestIsExistingDirectory(t *testing.T) { assert.Equal(t, testcase.expected, result, testcase.doc) } } + +func TestGetFilenameForDownload(t *testing.T) { + var testcases = []struct { + path string + disposition string + expected string + }{ + { + path: "http://www.example.com/", + expected: "", + }, + { + path: "http://www.example.com/xyz", + expected: "xyz", + }, + { + path: "http://www.example.com/xyz.html", + expected: "xyz.html", + }, + { + path: "http://www.example.com/xyz/", + expected: "", + }, + { + path: "http://www.example.com/xyz/uvw", + expected: "uvw", + }, + { + path: "http://www.example.com/xyz/uvw.html", + expected: "uvw.html", + }, + { + path: "http://www.example.com/xyz/uvw/", + expected: "", + }, + { + path: "/", + expected: "", + }, + { + path: "/xyz", + expected: "xyz", + }, + { + path: "/xyz.html", + expected: "xyz.html", + }, + { + path: "/xyz/", + expected: "", + }, + { + path: "/xyz/", + disposition: "attachment; filename=xyz.html", + expected: "xyz.html", + }, + { + disposition: "", + expected: "", + }, + { + disposition: "attachment; filename=xyz", + expected: "xyz", + }, + { + disposition: "attachment; filename=xyz.html", + expected: "xyz.html", + }, + { + disposition: "attachment; filename=\"xyz\"", + expected: "xyz", + }, + { + disposition: "attachment; filename=\"xyz.html\"", + expected: "xyz.html", + }, + { + disposition: "attachment; filename=\"/xyz.html\"", + expected: "xyz.html", + }, + { + disposition: "attachment; filename=\"/xyz/uvw\"", + expected: "uvw", + }, + { + disposition: "attachment; filename=\"Naïve file.txt\"", + expected: "Naïve file.txt", + }, + } + for _, testcase := range testcases { + resp := http.Response{ + Header: make(map[string][]string), + } + if testcase.disposition != "" { + resp.Header.Add("Content-Disposition", testcase.disposition) + } + filename := getFilenameForDownload(testcase.path, &resp) + assert.Equal(t, testcase.expected, filename) + } +} diff --git a/components/engine/integration-cli/docker_cli_build_test.go b/components/engine/integration-cli/docker_cli_build_test.go index 3dedf45f31..59213e5404 100644 --- a/components/engine/integration-cli/docker_cli_build_test.go +++ b/components/engine/integration-cli/docker_cli_build_test.go @@ -6506,19 +6506,3 @@ RUN touch /foop c.Assert(err, check.IsNil) c.Assert(d.String(), checker.Equals, getIDByName(c, name)) } - -// Test case for #34208 -func (s *DockerSuite) TestBuildAddHTTPRoot(c *check.C) { - testRequires(c, Network, DaemonIsLinux) - buildImageSuccessfully(c, "buildaddhttproot", build.WithDockerfile(` - FROM scratch - ADD http://example.com/index.html /example1 - ADD http://example.com /example2 - ADD http://example.com /example3`)) - buildImage("buildaddhttprootfailure", build.WithDockerfile(` - FROM scratch - ADD http://example.com/ /`)).Assert(c, icmd.Expected{ - ExitCode: 1, - Err: "cannot determine filename for source http://example.com/", - }) -}