Merge pull request #34217 from yongtang/34208-http-add-root

Fix build with `ADD` urls without any sub path

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

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