From e907d54fe6d16f364d34b28264162c6d61bfa06a Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 1 Jun 2017 17:15:13 -0400 Subject: [PATCH 01/39] Move pkg/gitutils to remotecontext/git Signed-off-by: Daniel Nephin --- .../image/build/internal/git/gitutils.go | 122 ++++++++++++ .../image/build/internal/git/gitutils_test.go | 180 ++++++++++++++++++ 2 files changed, 302 insertions(+) create mode 100644 cli/command/image/build/internal/git/gitutils.go create mode 100644 cli/command/image/build/internal/git/gitutils_test.go diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go new file mode 100644 index 000000000..fd8250dbc --- /dev/null +++ b/cli/command/image/build/internal/git/gitutils.go @@ -0,0 +1,122 @@ +package git + +import ( + "fmt" + "io/ioutil" + "net/http" + "net/url" + "os" + "os/exec" + "path/filepath" + "strings" + + "github.com/docker/docker/pkg/symlink" + "github.com/docker/docker/pkg/urlutil" + "github.com/pkg/errors" +) + +// Clone clones a repository into a newly created directory which +// will be under "docker-build-git" +func Clone(remoteURL string) (string, error) { + if !urlutil.IsGitTransport(remoteURL) { + remoteURL = "https://" + remoteURL + } + root, err := ioutil.TempDir("", "docker-build-git") + if err != nil { + return "", err + } + + u, err := url.Parse(remoteURL) + if err != nil { + return "", err + } + + if out, err := gitWithinDir(root, "init"); err != nil { + return "", errors.Wrapf(err, "failed to init repo at %s: %s", root, out) + } + + ref, subdir := getRefAndSubdir(u.Fragment) + fetch := fetchArgs(u, ref) + + u.Fragment = "" + + // Add origin remote for compatibility with previous implementation that + // used "git clone" and also to make sure local refs are created for branches + if out, err := gitWithinDir(root, "remote", "add", "origin", u.String()); err != nil { + return "", errors.Wrapf(err, "failed add origin repo at %s: %s", u.String(), out) + } + + if output, err := gitWithinDir(root, fetch...); err != nil { + return "", errors.Wrapf(err, "error fetching: %s", output) + } + + return checkoutGit(root, ref, subdir) +} + +func getRefAndSubdir(fragment string) (ref string, subdir string) { + refAndDir := strings.SplitN(fragment, ":", 2) + ref = "master" + if len(refAndDir[0]) != 0 { + ref = refAndDir[0] + } + if len(refAndDir) > 1 && len(refAndDir[1]) != 0 { + subdir = refAndDir[1] + } + return +} + +func fetchArgs(remoteURL *url.URL, ref string) []string { + args := []string{"fetch", "--recurse-submodules=yes"} + shallow := true + + if strings.HasPrefix(remoteURL.Scheme, "http") { + res, err := http.Head(fmt.Sprintf("%s/info/refs?service=git-upload-pack", remoteURL)) + if err != nil || res.Header.Get("Content-Type") != "application/x-git-upload-pack-advertisement" { + shallow = false + } + } + + if shallow { + args = append(args, "--depth", "1") + } + + return append(args, "origin", ref) +} + +func checkoutGit(root, ref, subdir string) (string, error) { + // Try checking out by ref name first. This will work on branches and sets + // .git/HEAD to the current branch name + if output, err := gitWithinDir(root, "checkout", ref); err != nil { + // If checking out by branch name fails check out the last fetched ref + if _, err2 := gitWithinDir(root, "checkout", "FETCH_HEAD"); err2 != nil { + return "", errors.Wrapf(err, "error checking out %s: %s", ref, output) + } + } + + if subdir != "" { + newCtx, err := symlink.FollowSymlinkInScope(filepath.Join(root, subdir), root) + if err != nil { + return "", errors.Wrapf(err, "error setting git context, %q not within git root", subdir) + } + + fi, err := os.Stat(newCtx) + if err != nil { + return "", err + } + if !fi.IsDir() { + return "", errors.Errorf("error setting git context, not a directory: %s", newCtx) + } + root = newCtx + } + + return root, nil +} + +func gitWithinDir(dir string, args ...string) ([]byte, error) { + a := []string{"--work-tree", dir, "--git-dir", filepath.Join(dir, ".git")} + return git(append(a, args...)...) +} + +func git(args ...string) ([]byte, error) { + return exec.Command("git", args...).CombinedOutput() +} diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go new file mode 100644 index 000000000..eb771f8c9 --- /dev/null +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -0,0 +1,180 @@ +package git + +import ( + "fmt" + "io/ioutil" + "net/http" + "net/http/httptest" + "net/url" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCloneArgsSmartHttp(t *testing.T) { + mux := http.NewServeMux() + server := httptest.NewServer(mux) + serverURL, _ := url.Parse(server.URL) + + serverURL.Path = "/repo.git" + + mux.HandleFunc("/repo.git/info/refs", func(w http.ResponseWriter, r *http.Request) { + q := r.URL.Query().Get("service") + w.Header().Set("Content-Type", fmt.Sprintf("application/x-%s-advertisement", q)) + }) + + args := fetchArgs(serverURL, "master") + exp := []string{"fetch", "--recurse-submodules=yes", "--depth", "1", "origin", "master"} + assert.Equal(t, exp, args) +} + +func TestCloneArgsDumbHttp(t *testing.T) { + mux := http.NewServeMux() + server := httptest.NewServer(mux) + serverURL, _ := url.Parse(server.URL) + + serverURL.Path = "/repo.git" + + mux.HandleFunc("/repo.git/info/refs", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/plain") + }) + + args := fetchArgs(serverURL, "master") + exp := []string{"fetch", "--recurse-submodules=yes", "origin", "master"} + assert.Equal(t, exp, args) +} + +func TestCloneArgsGit(t *testing.T) { + u, _ := url.Parse("git://github.com/docker/docker") + args := fetchArgs(u, "master") + exp := []string{"fetch", "--recurse-submodules=yes", "--depth", "1", "origin", "master"} + assert.Equal(t, exp, args) +} + +func gitGetConfig(name string) string { + b, err := git([]string{"config", "--get", name}...) + if err != nil { + // since we are interested in empty or non empty string, + // we can safely ignore the err here. + return "" + } + return strings.TrimSpace(string(b)) +} + +func TestCheckoutGit(t *testing.T) { + root, err := ioutil.TempDir("", "docker-build-git-checkout") + require.NoError(t, err) + defer os.RemoveAll(root) + + autocrlf := gitGetConfig("core.autocrlf") + if !(autocrlf == "true" || autocrlf == "false" || + autocrlf == "input" || autocrlf == "") { + t.Logf("unknown core.autocrlf value: \"%s\"", autocrlf) + } + eol := "\n" + if autocrlf == "true" { + eol = "\r\n" + } + + gitDir := filepath.Join(root, "repo") + _, err = git("init", gitDir) + require.NoError(t, err) + + _, err = gitWithinDir(gitDir, "config", "user.email", "test@docker.com") + require.NoError(t, err) + + _, err = gitWithinDir(gitDir, "config", "user.name", "Docker test") + require.NoError(t, err) + + err = ioutil.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch"), 0644) + require.NoError(t, err) + + subDir := filepath.Join(gitDir, "subdir") + require.NoError(t, os.Mkdir(subDir, 0755)) + + err = ioutil.WriteFile(filepath.Join(subDir, "Dockerfile"), []byte("FROM scratch\nEXPOSE 5000"), 0644) + require.NoError(t, err) + + if runtime.GOOS != "windows" { + if err = os.Symlink("../subdir", filepath.Join(gitDir, "parentlink")); err != nil { + t.Fatal(err) + } + + if err = os.Symlink("/subdir", filepath.Join(gitDir, "absolutelink")); err != nil { + t.Fatal(err) + } + } + + _, err = gitWithinDir(gitDir, "add", "-A") + require.NoError(t, err) + + _, err = gitWithinDir(gitDir, "commit", "-am", "First commit") + require.NoError(t, err) + + _, err = gitWithinDir(gitDir, "checkout", "-b", "test") + require.NoError(t, err) + + err = ioutil.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch\nEXPOSE 3000"), 0644) + require.NoError(t, err) + + err = ioutil.WriteFile(filepath.Join(subDir, "Dockerfile"), []byte("FROM busybox\nEXPOSE 5000"), 0644) + require.NoError(t, err) + + _, err = gitWithinDir(gitDir, "add", "-A") + require.NoError(t, err) + + _, err = gitWithinDir(gitDir, "commit", "-am", "Branch commit") + require.NoError(t, err) + + _, err = gitWithinDir(gitDir, "checkout", "master") + require.NoError(t, err) + + type singleCase struct { + frag string + exp string + fail bool + } + + cases := []singleCase{ + {"", "FROM scratch", false}, + {"master", "FROM scratch", false}, + {":subdir", "FROM scratch" + eol + "EXPOSE 5000", false}, + {":nosubdir", "", true}, // missing directory error + {":Dockerfile", "", true}, // not a directory error + {"master:nosubdir", "", true}, + {"master:subdir", "FROM scratch" + eol + "EXPOSE 5000", false}, + {"master:../subdir", "", true}, + {"test", "FROM scratch" + eol + "EXPOSE 3000", false}, + {"test:", "FROM scratch" + eol + "EXPOSE 3000", false}, + {"test:subdir", "FROM busybox" + eol + "EXPOSE 5000", false}, + } + + if runtime.GOOS != "windows" { + // Windows GIT (2.7.1 x64) does not support parentlink/absolutelink. Sample output below + // git --work-tree .\repo --git-dir .\repo\.git add -A + // error: readlink("absolutelink"): Function not implemented + // error: unable to index file absolutelink + // fatal: adding files failed + cases = append(cases, singleCase{frag: "master:absolutelink", exp: "FROM scratch" + eol + "EXPOSE 5000", fail: false}) + cases = append(cases, singleCase{frag: "master:parentlink", exp: "FROM scratch" + eol + "EXPOSE 5000", fail: false}) + } + + for _, c := range cases { + ref, subdir := getRefAndSubdir(c.frag) + r, err := checkoutGit(gitDir, ref, subdir) + + if c.fail { + assert.Error(t, err) + continue + } + + b, err := ioutil.ReadFile(filepath.Join(r, "Dockerfile")) + require.NoError(t, err) + assert.Equal(t, c.exp, string(b)) + } +} From a6cc6cd878a2a06281cc1d547da5594e69923778 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 15 Jun 2017 16:29:18 +0200 Subject: [PATCH 02/39] Fix handling of remote "git@" notation `docker build` accepts remote repositories using either the `git://` notation, or `git@`. Docker attempted to parse both as an URL, however, `git@` is not an URL, but an argument to `git clone`. Go 1.7 silently ignored this, and managed to extract the needed information from these remotes, however, Go 1.8 does a more strict validation, and invalidated these. This patch adds a different path for `git@` remotes, to prevent them from being handled as URL (and invalidated). A test is also added, because there were no tests for handling of `git@` remotes. Signed-off-by: Sebastiaan van Stijn --- .../image/build/internal/git/gitutils.go | 61 ++++++++++++++----- .../image/build/internal/git/gitutils_test.go | 39 ++++++++++-- 2 files changed, 81 insertions(+), 19 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index fd8250dbc..91a0f5bae 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -15,18 +15,24 @@ import ( "github.com/pkg/errors" ) +type gitRepo struct { + remote string + ref string + subdir string +} + // Clone clones a repository into a newly created directory which // will be under "docker-build-git" func Clone(remoteURL string) (string, error) { - if !urlutil.IsGitTransport(remoteURL) { - remoteURL = "https://" + remoteURL - } - root, err := ioutil.TempDir("", "docker-build-git") + repo, err := parseRemoteURL(remoteURL) + if err != nil { return "", err } - u, err := url.Parse(remoteURL) + fetch := fetchArgs(repo.remote, repo.ref) + + root, err := ioutil.TempDir("", "docker-build-git") if err != nil { return "", err } @@ -35,22 +41,47 @@ func Clone(remoteURL string) (string, error) { return "", errors.Wrapf(err, "failed to init repo at %s: %s", root, out) } - ref, subdir := getRefAndSubdir(u.Fragment) - fetch := fetchArgs(u, ref) - - u.Fragment = "" - // Add origin remote for compatibility with previous implementation that // used "git clone" and also to make sure local refs are created for branches - if out, err := gitWithinDir(root, "remote", "add", "origin", u.String()); err != nil { - return "", errors.Wrapf(err, "failed add origin repo at %s: %s", u.String(), out) + if out, err := gitWithinDir(root, "remote", "add", "origin", repo.remote); err != nil { + return "", errors.Wrapf(err, "failed add origin repo at %s: %s", repo.remote, out) } if output, err := gitWithinDir(root, fetch...); err != nil { return "", errors.Wrapf(err, "error fetching: %s", output) } - return checkoutGit(root, ref, subdir) + return checkoutGit(root, repo.ref, repo.subdir) +} + +func parseRemoteURL(remoteURL string) (gitRepo, error) { + repo := gitRepo{} + + if !urlutil.IsGitTransport(remoteURL) { + remoteURL = "https://" + remoteURL + } + + var fragment string + if strings.HasPrefix(remoteURL, "git@") { + // git@.. is not an URL, so cannot be parsed as URL + parts := strings.SplitN(remoteURL, "#", 2) + + repo.remote = parts[0] + if len(parts) == 2 { + fragment = parts[1] + } + repo.ref, repo.subdir = getRefAndSubdir(fragment) + } else { + u, err := url.Parse(remoteURL) + if err != nil { + return repo, err + } + + repo.ref, repo.subdir = getRefAndSubdir(u.Fragment) + u.Fragment = "" + repo.remote = u.String() + } + return repo, nil } func getRefAndSubdir(fragment string) (ref string, subdir string) { @@ -65,11 +96,11 @@ func getRefAndSubdir(fragment string) (ref string, subdir string) { return } -func fetchArgs(remoteURL *url.URL, ref string) []string { +func fetchArgs(remoteURL string, ref string) []string { args := []string{"fetch", "--recurse-submodules=yes"} shallow := true - if strings.HasPrefix(remoteURL.Scheme, "http") { + if urlutil.IsURL(remoteURL) { res, err := http.Head(fmt.Sprintf("%s/info/refs?service=git-upload-pack", remoteURL)) if err != nil || res.Header.Get("Content-Type") != "application/x-git-upload-pack-advertisement" { shallow = false diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index eb771f8c9..b6523a223 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -16,6 +16,38 @@ import ( "github.com/stretchr/testify/require" ) +func TestParseRemoteURL(t *testing.T) { + dir, err := parseRemoteURL("git://github.com/user/repo.git") + require.NoError(t, err) + assert.NotEmpty(t, dir) + assert.Equal(t, gitRepo{"git://github.com/user/repo.git", "master", ""}, dir) + + dir, err = parseRemoteURL("git://github.com/user/repo.git#mybranch:mydir/mysubdir/") + require.NoError(t, err) + assert.NotEmpty(t, dir) + assert.Equal(t, gitRepo{"git://github.com/user/repo.git", "mybranch", "mydir/mysubdir/"}, dir) + + dir, err = parseRemoteURL("https://github.com/user/repo.git") + require.NoError(t, err) + assert.NotEmpty(t, dir) + assert.Equal(t, gitRepo{"https://github.com/user/repo.git", "master", ""}, dir) + + dir, err = parseRemoteURL("https://github.com/user/repo.git#mybranch:mydir/mysubdir/") + require.NoError(t, err) + assert.NotEmpty(t, dir) + assert.Equal(t, gitRepo{"https://github.com/user/repo.git", "mybranch", "mydir/mysubdir/"}, dir) + + dir, err = parseRemoteURL("git@github.com:user/repo.git") + require.NoError(t, err) + assert.NotEmpty(t, dir) + assert.Equal(t, gitRepo{"git@github.com:user/repo.git", "master", ""}, dir) + + dir, err = parseRemoteURL("git@github.com:user/repo.git#mybranch:mydir/mysubdir/") + require.NoError(t, err) + assert.NotEmpty(t, dir) + assert.Equal(t, gitRepo{"git@github.com:user/repo.git", "mybranch", "mydir/mysubdir/"}, dir) +} + func TestCloneArgsSmartHttp(t *testing.T) { mux := http.NewServeMux() server := httptest.NewServer(mux) @@ -28,7 +60,7 @@ func TestCloneArgsSmartHttp(t *testing.T) { w.Header().Set("Content-Type", fmt.Sprintf("application/x-%s-advertisement", q)) }) - args := fetchArgs(serverURL, "master") + args := fetchArgs(serverURL.String(), "master") exp := []string{"fetch", "--recurse-submodules=yes", "--depth", "1", "origin", "master"} assert.Equal(t, exp, args) } @@ -44,14 +76,13 @@ func TestCloneArgsDumbHttp(t *testing.T) { w.Header().Set("Content-Type", "text/plain") }) - args := fetchArgs(serverURL, "master") + args := fetchArgs(serverURL.String(), "master") exp := []string{"fetch", "--recurse-submodules=yes", "origin", "master"} assert.Equal(t, exp, args) } func TestCloneArgsGit(t *testing.T) { - u, _ := url.Parse("git://github.com/docker/docker") - args := fetchArgs(u, "master") + args := fetchArgs("git://github.com/docker/docker", "master") exp := []string{"fetch", "--recurse-submodules=yes", "--depth", "1", "origin", "master"} assert.Equal(t, exp, args) } From 9450481b7e7ab60c23b5bfd40c24692e729bff2e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 26 Jun 2017 10:07:04 -0700 Subject: [PATCH 03/39] Move IsGitTransport() to gitutils This function was only used inside gitutils, and is written specifically for the requirements there. Signed-off-by: Sebastiaan van Stijn --- .../image/build/internal/git/gitutils.go | 8 +++++- .../image/build/internal/git/gitutils_test.go | 27 +++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index 91a0f5bae..b94d462cd 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -57,7 +57,7 @@ func Clone(remoteURL string) (string, error) { func parseRemoteURL(remoteURL string) (gitRepo, error) { repo := gitRepo{} - if !urlutil.IsGitTransport(remoteURL) { + if !isGitTransport(remoteURL) { remoteURL = "https://" + remoteURL } @@ -151,3 +151,9 @@ func gitWithinDir(dir string, args ...string) ([]byte, error) { func git(args ...string) ([]byte, error) { return exec.Command("git", args...).CombinedOutput() } + +// isGitTransport returns true if the provided str is a git transport by inspecting +// the prefix of the string for known protocols used in git. +func isGitTransport(str string) bool { + return urlutil.IsURL(str) || strings.HasPrefix(str, "git://") || strings.HasPrefix(str, "git@") +} diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index b6523a223..c638a498f 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -209,3 +209,30 @@ func TestCheckoutGit(t *testing.T) { assert.Equal(t, c.exp, string(b)) } } + +func TestValidGitTransport(t *testing.T) { + gitUrls := []string{ + "git://github.com/docker/docker", + "git@github.com:docker/docker.git", + "git@bitbucket.org:atlassianlabs/atlassian-docker.git", + "https://github.com/docker/docker.git", + "http://github.com/docker/docker.git", + "http://github.com/docker/docker.git#branch", + "http://github.com/docker/docker.git#:dir", + } + incompleteGitUrls := []string{ + "github.com/docker/docker", + } + + for _, url := range gitUrls { + if !isGitTransport(url) { + t.Fatalf("%q should be detected as valid Git prefix", url) + } + } + + for _, url := range incompleteGitUrls { + if isGitTransport(url) { + t.Fatalf("%q should not be detected as valid Git prefix", url) + } + } +} From e9831d75e2d1bd1a7aa9b757fae2363ca89da4ad Mon Sep 17 00:00:00 2001 From: Andrew He Date: Thu, 15 Jun 2017 12:15:02 -0700 Subject: [PATCH 04/39] Fix shallow git clone in docker-build If the HEAD request fails, use a GET request to properly test if git server is smart-http. Signed-off-by: Andrew He --- .../image/build/internal/git/gitutils.go | 43 ++++++++++++++----- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index b94d462cd..7bc226815 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -1,7 +1,6 @@ package git import ( - "fmt" "io/ioutil" "net/http" "net/url" @@ -98,22 +97,46 @@ func getRefAndSubdir(fragment string) (ref string, subdir string) { func fetchArgs(remoteURL string, ref string) []string { args := []string{"fetch", "--recurse-submodules=yes"} - shallow := true - if urlutil.IsURL(remoteURL) { - res, err := http.Head(fmt.Sprintf("%s/info/refs?service=git-upload-pack", remoteURL)) - if err != nil || res.Header.Get("Content-Type") != "application/x-git-upload-pack-advertisement" { - shallow = false - } - } - - if shallow { + if supportsShallowClone(remoteURL) { args = append(args, "--depth", "1") } return append(args, "origin", ref) } +// Check if a given git URL supports a shallow git clone, +// i.e. it is a non-HTTP server or a smart HTTP server. +func supportsShallowClone(remoteURL string) bool { + if urlutil.IsURL(remoteURL) { + // Check if the HTTP server is smart + + // Smart servers must correctly respond to a query for the git-upload-pack service + serviceURL := remoteURL + "/info/refs?service=git-upload-pack" + + // Try a HEAD request and fallback to a Get request on error + res, err := http.Head(serviceURL) + if err != nil || res.StatusCode != http.StatusOK { + res, err = http.Get(serviceURL) + if err == nil { + res.Body.Close() + } + if err != nil || res.StatusCode != http.StatusOK { + // request failed + return false + } + } + + if res.Header.Get("Content-Type") != "application/x-git-upload-pack-advertisement" { + // Fallback, not a smart server + return false + } + return true + } + // Non-HTTP protocols always support shallow clones + return true +} + func checkoutGit(root, ref, subdir string) (string, error) { // Try checking out by ref name first. This will work on branches and sets // .git/HEAD to the current branch name From e2cc22d076153e2ea4ad674aae52714a66f5e0c5 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Thu, 7 Dec 2017 13:44:08 -0800 Subject: [PATCH 05/39] gitutils: fix checking out submodules Signed-off-by: Tonis Tiigi --- .../image/build/internal/git/gitutils.go | 20 ++++- .../image/build/internal/git/gitutils_test.go | 79 ++++++++++++++----- 2 files changed, 79 insertions(+), 20 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index 7bc226815..b579db976 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -29,6 +29,10 @@ func Clone(remoteURL string) (string, error) { return "", err } + return cloneGitRepo(repo) +} + +func cloneGitRepo(repo gitRepo) (string, error) { fetch := fetchArgs(repo.remote, repo.ref) root, err := ioutil.TempDir("", "docker-build-git") @@ -50,7 +54,19 @@ func Clone(remoteURL string) (string, error) { return "", errors.Wrapf(err, "error fetching: %s", output) } - return checkoutGit(root, repo.ref, repo.subdir) + root, err = checkoutGit(root, repo.ref, repo.subdir) + if err != nil { + return "", err + } + + cmd := exec.Command("git", "submodule", "update", "--init", "--recursive", "--depth=1") + cmd.Dir = root + output, err := cmd.CombinedOutput() + if err != nil { + return "", errors.Wrapf(err, "error initializing submodules: %s", output) + } + + return root, nil } func parseRemoteURL(remoteURL string) (gitRepo, error) { @@ -96,7 +112,7 @@ func getRefAndSubdir(fragment string) (ref string, subdir string) { } func fetchArgs(remoteURL string, ref string) []string { - args := []string{"fetch", "--recurse-submodules=yes"} + args := []string{"fetch"} if supportsShallowClone(remoteURL) { args = append(args, "--depth", "1") diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index c638a498f..b4c49195f 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "net/url" "os" + "os/exec" "path/filepath" "runtime" "strings" @@ -61,7 +62,7 @@ func TestCloneArgsSmartHttp(t *testing.T) { }) args := fetchArgs(serverURL.String(), "master") - exp := []string{"fetch", "--recurse-submodules=yes", "--depth", "1", "origin", "master"} + exp := []string{"fetch", "--depth", "1", "origin", "master"} assert.Equal(t, exp, args) } @@ -77,13 +78,13 @@ func TestCloneArgsDumbHttp(t *testing.T) { }) args := fetchArgs(serverURL.String(), "master") - exp := []string{"fetch", "--recurse-submodules=yes", "origin", "master"} + exp := []string{"fetch", "origin", "master"} assert.Equal(t, exp, args) } func TestCloneArgsGit(t *testing.T) { args := fetchArgs("git://github.com/docker/docker", "master") - exp := []string{"fetch", "--recurse-submodules=yes", "--depth", "1", "origin", "master"} + exp := []string{"fetch", "--depth", "1", "origin", "master"} assert.Equal(t, exp, args) } @@ -165,24 +166,55 @@ func TestCheckoutGit(t *testing.T) { _, err = gitWithinDir(gitDir, "checkout", "master") require.NoError(t, err) + // set up submodule + subrepoDir := filepath.Join(root, "subrepo") + _, err = git("init", subrepoDir) + require.NoError(t, err) + + _, err = gitWithinDir(subrepoDir, "config", "user.email", "test@docker.com") + require.NoError(t, err) + + _, err = gitWithinDir(subrepoDir, "config", "user.name", "Docker test") + require.NoError(t, err) + + err = ioutil.WriteFile(filepath.Join(subrepoDir, "subfile"), []byte("subcontents"), 0644) + require.NoError(t, err) + + _, err = gitWithinDir(subrepoDir, "add", "-A") + require.NoError(t, err) + + _, err = gitWithinDir(subrepoDir, "commit", "-am", "Subrepo initial") + require.NoError(t, err) + + cmd := exec.Command("git", "submodule", "add", subrepoDir, "sub") // this command doesn't work with --work-tree + cmd.Dir = gitDir + require.NoError(t, cmd.Run()) + + _, err = gitWithinDir(gitDir, "add", "-A") + require.NoError(t, err) + + _, err = gitWithinDir(gitDir, "commit", "-am", "With submodule") + require.NoError(t, err) + type singleCase struct { - frag string - exp string - fail bool + frag string + exp string + fail bool + submodule bool } cases := []singleCase{ - {"", "FROM scratch", false}, - {"master", "FROM scratch", false}, - {":subdir", "FROM scratch" + eol + "EXPOSE 5000", false}, - {":nosubdir", "", true}, // missing directory error - {":Dockerfile", "", true}, // not a directory error - {"master:nosubdir", "", true}, - {"master:subdir", "FROM scratch" + eol + "EXPOSE 5000", false}, - {"master:../subdir", "", true}, - {"test", "FROM scratch" + eol + "EXPOSE 3000", false}, - {"test:", "FROM scratch" + eol + "EXPOSE 3000", false}, - {"test:subdir", "FROM busybox" + eol + "EXPOSE 5000", false}, + {"", "FROM scratch", false, true}, + {"master", "FROM scratch", false, true}, + {":subdir", "FROM scratch" + eol + "EXPOSE 5000", false, false}, + {":nosubdir", "", true, false}, // missing directory error + {":Dockerfile", "", true, false}, // not a directory error + {"master:nosubdir", "", true, false}, + {"master:subdir", "FROM scratch" + eol + "EXPOSE 5000", false, false}, + {"master:../subdir", "", true, false}, + {"test", "FROM scratch" + eol + "EXPOSE 3000", false, false}, + {"test:", "FROM scratch" + eol + "EXPOSE 3000", false, false}, + {"test:subdir", "FROM busybox" + eol + "EXPOSE 5000", false, false}, } if runtime.GOOS != "windows" { @@ -197,11 +229,22 @@ func TestCheckoutGit(t *testing.T) { for _, c := range cases { ref, subdir := getRefAndSubdir(c.frag) - r, err := checkoutGit(gitDir, ref, subdir) + r, err := cloneGitRepo(gitRepo{remote: gitDir, ref: ref, subdir: subdir}) if c.fail { assert.Error(t, err) continue + } else { + require.NoError(t, err) + if c.submodule { + b, err := ioutil.ReadFile(filepath.Join(r, "sub/subfile")) + require.NoError(t, err) + assert.Equal(t, "subcontents", string(b)) + } else { + _, err := os.Stat(filepath.Join(r, "sub/subfile")) + require.Error(t, err) + require.True(t, os.IsNotExist(err)) + } } b, err := ioutil.ReadFile(filepath.Join(r, "Dockerfile")) From 7bc503344ab57051fed940824d504243c57ae10b Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Fri, 8 Dec 2017 11:51:10 -0800 Subject: [PATCH 06/39] gitutils: remove checkout directory on error Signed-off-by: Tonis Tiigi --- .../image/build/internal/git/gitutils.go | 12 ++++++++--- .../image/build/internal/git/gitutils_test.go | 20 +++++++++---------- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index b579db976..67cff594a 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -32,7 +32,7 @@ func Clone(remoteURL string) (string, error) { return cloneGitRepo(repo) } -func cloneGitRepo(repo gitRepo) (string, error) { +func cloneGitRepo(repo gitRepo) (checkoutDir string, err error) { fetch := fetchArgs(repo.remote, repo.ref) root, err := ioutil.TempDir("", "docker-build-git") @@ -40,6 +40,12 @@ func cloneGitRepo(repo gitRepo) (string, error) { return "", err } + defer func() { + if err != nil { + os.RemoveAll(root) + } + }() + if out, err := gitWithinDir(root, "init"); err != nil { return "", errors.Wrapf(err, "failed to init repo at %s: %s", root, out) } @@ -54,7 +60,7 @@ func cloneGitRepo(repo gitRepo) (string, error) { return "", errors.Wrapf(err, "error fetching: %s", output) } - root, err = checkoutGit(root, repo.ref, repo.subdir) + checkoutDir, err = checkoutGit(root, repo.ref, repo.subdir) if err != nil { return "", err } @@ -66,7 +72,7 @@ func cloneGitRepo(repo gitRepo) (string, error) { return "", errors.Wrapf(err, "error initializing submodules: %s", output) } - return root, nil + return checkoutDir, nil } func parseRemoteURL(remoteURL string) (gitRepo, error) { diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index b4c49195f..fd58d6bd9 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -234,17 +234,17 @@ func TestCheckoutGit(t *testing.T) { if c.fail { assert.Error(t, err) continue - } else { + } + require.NoError(t, err) + defer os.RemoveAll(r) + if c.submodule { + b, err := ioutil.ReadFile(filepath.Join(r, "sub/subfile")) require.NoError(t, err) - if c.submodule { - b, err := ioutil.ReadFile(filepath.Join(r, "sub/subfile")) - require.NoError(t, err) - assert.Equal(t, "subcontents", string(b)) - } else { - _, err := os.Stat(filepath.Join(r, "sub/subfile")) - require.Error(t, err) - require.True(t, os.IsNotExist(err)) - } + assert.Equal(t, "subcontents", string(b)) + } else { + _, err := os.Stat(filepath.Join(r, "sub/subfile")) + require.Error(t, err) + require.True(t, os.IsNotExist(err)) } b, err := ioutil.ReadFile(filepath.Join(r, "Dockerfile")) From 6ea4877cff075c815cdddac0d27db4eb239e3715 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 5 Feb 2018 16:05:59 -0500 Subject: [PATCH 07/39] Add canonical import comment Signed-off-by: Daniel Nephin --- cli/command/image/build/internal/git/gitutils.go | 2 +- cli/command/image/build/internal/git/gitutils_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index 67cff594a..77a45beff 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -1,4 +1,4 @@ -package git +package git // import "github.com/docker/docker/builder/remotecontext/git" import ( "io/ioutil" diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index fd58d6bd9..4f4d83350 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -1,4 +1,4 @@ -package git +package git // import "github.com/docker/docker/builder/remotecontext/git" import ( "fmt" From 242f176825780db1a0ac59b38621c9e35df95a33 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 13 Mar 2018 15:28:34 -0400 Subject: [PATCH 08/39] Automated migration using gty-migrate-from-testify --ignore-build-tags Signed-off-by: Daniel Nephin --- .../image/build/internal/git/gitutils_test.go | 110 +++++++++--------- 1 file changed, 55 insertions(+), 55 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index 4f4d83350..cb2adb8c2 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -13,40 +13,40 @@ import ( "strings" "testing" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" + "github.com/gotestyourself/gotestyourself/assert" + is "github.com/gotestyourself/gotestyourself/assert/cmp" ) func TestParseRemoteURL(t *testing.T) { dir, err := parseRemoteURL("git://github.com/user/repo.git") - require.NoError(t, err) - assert.NotEmpty(t, dir) - assert.Equal(t, gitRepo{"git://github.com/user/repo.git", "master", ""}, dir) + assert.NilError(t, err) + assert.Check(t, len(dir) != 0) + assert.Check(t, is.DeepEqual(gitRepo{"git://github.com/user/repo.git", "master", ""}, dir)) dir, err = parseRemoteURL("git://github.com/user/repo.git#mybranch:mydir/mysubdir/") - require.NoError(t, err) - assert.NotEmpty(t, dir) - assert.Equal(t, gitRepo{"git://github.com/user/repo.git", "mybranch", "mydir/mysubdir/"}, dir) + assert.NilError(t, err) + assert.Check(t, len(dir) != 0) + assert.Check(t, is.DeepEqual(gitRepo{"git://github.com/user/repo.git", "mybranch", "mydir/mysubdir/"}, dir)) dir, err = parseRemoteURL("https://github.com/user/repo.git") - require.NoError(t, err) - assert.NotEmpty(t, dir) - assert.Equal(t, gitRepo{"https://github.com/user/repo.git", "master", ""}, dir) + assert.NilError(t, err) + assert.Check(t, len(dir) != 0) + assert.Check(t, is.DeepEqual(gitRepo{"https://github.com/user/repo.git", "master", ""}, dir)) dir, err = parseRemoteURL("https://github.com/user/repo.git#mybranch:mydir/mysubdir/") - require.NoError(t, err) - assert.NotEmpty(t, dir) - assert.Equal(t, gitRepo{"https://github.com/user/repo.git", "mybranch", "mydir/mysubdir/"}, dir) + assert.NilError(t, err) + assert.Check(t, len(dir) != 0) + assert.Check(t, is.DeepEqual(gitRepo{"https://github.com/user/repo.git", "mybranch", "mydir/mysubdir/"}, dir)) dir, err = parseRemoteURL("git@github.com:user/repo.git") - require.NoError(t, err) - assert.NotEmpty(t, dir) - assert.Equal(t, gitRepo{"git@github.com:user/repo.git", "master", ""}, dir) + assert.NilError(t, err) + assert.Check(t, len(dir) != 0) + assert.Check(t, is.DeepEqual(gitRepo{"git@github.com:user/repo.git", "master", ""}, dir)) dir, err = parseRemoteURL("git@github.com:user/repo.git#mybranch:mydir/mysubdir/") - require.NoError(t, err) - assert.NotEmpty(t, dir) - assert.Equal(t, gitRepo{"git@github.com:user/repo.git", "mybranch", "mydir/mysubdir/"}, dir) + assert.NilError(t, err) + assert.Check(t, len(dir) != 0) + assert.Check(t, is.DeepEqual(gitRepo{"git@github.com:user/repo.git", "mybranch", "mydir/mysubdir/"}, dir)) } func TestCloneArgsSmartHttp(t *testing.T) { @@ -63,7 +63,7 @@ func TestCloneArgsSmartHttp(t *testing.T) { args := fetchArgs(serverURL.String(), "master") exp := []string{"fetch", "--depth", "1", "origin", "master"} - assert.Equal(t, exp, args) + assert.Check(t, is.DeepEqual(exp, args)) } func TestCloneArgsDumbHttp(t *testing.T) { @@ -79,13 +79,13 @@ func TestCloneArgsDumbHttp(t *testing.T) { args := fetchArgs(serverURL.String(), "master") exp := []string{"fetch", "origin", "master"} - assert.Equal(t, exp, args) + assert.Check(t, is.DeepEqual(exp, args)) } func TestCloneArgsGit(t *testing.T) { args := fetchArgs("git://github.com/docker/docker", "master") exp := []string{"fetch", "--depth", "1", "origin", "master"} - assert.Equal(t, exp, args) + assert.Check(t, is.DeepEqual(exp, args)) } func gitGetConfig(name string) string { @@ -100,7 +100,7 @@ func gitGetConfig(name string) string { func TestCheckoutGit(t *testing.T) { root, err := ioutil.TempDir("", "docker-build-git-checkout") - require.NoError(t, err) + assert.NilError(t, err) defer os.RemoveAll(root) autocrlf := gitGetConfig("core.autocrlf") @@ -115,22 +115,22 @@ func TestCheckoutGit(t *testing.T) { gitDir := filepath.Join(root, "repo") _, err = git("init", gitDir) - require.NoError(t, err) + assert.NilError(t, err) _, err = gitWithinDir(gitDir, "config", "user.email", "test@docker.com") - require.NoError(t, err) + assert.NilError(t, err) _, err = gitWithinDir(gitDir, "config", "user.name", "Docker test") - require.NoError(t, err) + assert.NilError(t, err) err = ioutil.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch"), 0644) - require.NoError(t, err) + assert.NilError(t, err) subDir := filepath.Join(gitDir, "subdir") - require.NoError(t, os.Mkdir(subDir, 0755)) + assert.NilError(t, os.Mkdir(subDir, 0755)) err = ioutil.WriteFile(filepath.Join(subDir, "Dockerfile"), []byte("FROM scratch\nEXPOSE 5000"), 0644) - require.NoError(t, err) + assert.NilError(t, err) if runtime.GOOS != "windows" { if err = os.Symlink("../subdir", filepath.Join(gitDir, "parentlink")); err != nil { @@ -143,58 +143,58 @@ func TestCheckoutGit(t *testing.T) { } _, err = gitWithinDir(gitDir, "add", "-A") - require.NoError(t, err) + assert.NilError(t, err) _, err = gitWithinDir(gitDir, "commit", "-am", "First commit") - require.NoError(t, err) + assert.NilError(t, err) _, err = gitWithinDir(gitDir, "checkout", "-b", "test") - require.NoError(t, err) + assert.NilError(t, err) err = ioutil.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch\nEXPOSE 3000"), 0644) - require.NoError(t, err) + assert.NilError(t, err) err = ioutil.WriteFile(filepath.Join(subDir, "Dockerfile"), []byte("FROM busybox\nEXPOSE 5000"), 0644) - require.NoError(t, err) + assert.NilError(t, err) _, err = gitWithinDir(gitDir, "add", "-A") - require.NoError(t, err) + assert.NilError(t, err) _, err = gitWithinDir(gitDir, "commit", "-am", "Branch commit") - require.NoError(t, err) + assert.NilError(t, err) _, err = gitWithinDir(gitDir, "checkout", "master") - require.NoError(t, err) + assert.NilError(t, err) // set up submodule subrepoDir := filepath.Join(root, "subrepo") _, err = git("init", subrepoDir) - require.NoError(t, err) + assert.NilError(t, err) _, err = gitWithinDir(subrepoDir, "config", "user.email", "test@docker.com") - require.NoError(t, err) + assert.NilError(t, err) _, err = gitWithinDir(subrepoDir, "config", "user.name", "Docker test") - require.NoError(t, err) + assert.NilError(t, err) err = ioutil.WriteFile(filepath.Join(subrepoDir, "subfile"), []byte("subcontents"), 0644) - require.NoError(t, err) + assert.NilError(t, err) _, err = gitWithinDir(subrepoDir, "add", "-A") - require.NoError(t, err) + assert.NilError(t, err) _, err = gitWithinDir(subrepoDir, "commit", "-am", "Subrepo initial") - require.NoError(t, err) + assert.NilError(t, err) cmd := exec.Command("git", "submodule", "add", subrepoDir, "sub") // this command doesn't work with --work-tree cmd.Dir = gitDir - require.NoError(t, cmd.Run()) + assert.NilError(t, cmd.Run()) _, err = gitWithinDir(gitDir, "add", "-A") - require.NoError(t, err) + assert.NilError(t, err) _, err = gitWithinDir(gitDir, "commit", "-am", "With submodule") - require.NoError(t, err) + assert.NilError(t, err) type singleCase struct { frag string @@ -232,24 +232,24 @@ func TestCheckoutGit(t *testing.T) { r, err := cloneGitRepo(gitRepo{remote: gitDir, ref: ref, subdir: subdir}) if c.fail { - assert.Error(t, err) + assert.Check(t, is.ErrorContains(err, "")) continue } - require.NoError(t, err) + assert.NilError(t, err) defer os.RemoveAll(r) if c.submodule { b, err := ioutil.ReadFile(filepath.Join(r, "sub/subfile")) - require.NoError(t, err) - assert.Equal(t, "subcontents", string(b)) + assert.NilError(t, err) + assert.Check(t, is.Equal("subcontents", string(b))) } else { _, err := os.Stat(filepath.Join(r, "sub/subfile")) - require.Error(t, err) - require.True(t, os.IsNotExist(err)) + assert.Assert(t, is.ErrorContains(err, "")) + assert.Assert(t, os.IsNotExist(err)) } b, err := ioutil.ReadFile(filepath.Join(r, "Dockerfile")) - require.NoError(t, err) - assert.Equal(t, c.exp, string(b)) + assert.NilError(t, err) + assert.Check(t, is.Equal(c.exp, string(b))) } } From db857b5d9c44670d06771cd59a618cfcf3d05d13 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 22 Dec 2017 16:30:49 -0500 Subject: [PATCH 09/39] Post migration assertion fixes Signed-off-by: Daniel Nephin --- .../image/build/internal/git/gitutils_test.go | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index cb2adb8c2..a46675b22 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -13,6 +13,7 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" "github.com/gotestyourself/gotestyourself/assert" is "github.com/gotestyourself/gotestyourself/assert/cmp" ) @@ -20,35 +21,31 @@ import ( func TestParseRemoteURL(t *testing.T) { dir, err := parseRemoteURL("git://github.com/user/repo.git") assert.NilError(t, err) - assert.Check(t, len(dir) != 0) - assert.Check(t, is.DeepEqual(gitRepo{"git://github.com/user/repo.git", "master", ""}, dir)) + assert.Check(t, is.DeepEqual(gitRepo{"git://github.com/user/repo.git", "master", ""}, dir, cmpGitRepoOpt)) dir, err = parseRemoteURL("git://github.com/user/repo.git#mybranch:mydir/mysubdir/") assert.NilError(t, err) - assert.Check(t, len(dir) != 0) - assert.Check(t, is.DeepEqual(gitRepo{"git://github.com/user/repo.git", "mybranch", "mydir/mysubdir/"}, dir)) + assert.Check(t, is.DeepEqual(gitRepo{"git://github.com/user/repo.git", "mybranch", "mydir/mysubdir/"}, dir, cmpGitRepoOpt)) dir, err = parseRemoteURL("https://github.com/user/repo.git") assert.NilError(t, err) - assert.Check(t, len(dir) != 0) - assert.Check(t, is.DeepEqual(gitRepo{"https://github.com/user/repo.git", "master", ""}, dir)) + assert.Check(t, is.DeepEqual(gitRepo{"https://github.com/user/repo.git", "master", ""}, dir, cmpGitRepoOpt)) dir, err = parseRemoteURL("https://github.com/user/repo.git#mybranch:mydir/mysubdir/") assert.NilError(t, err) - assert.Check(t, len(dir) != 0) - assert.Check(t, is.DeepEqual(gitRepo{"https://github.com/user/repo.git", "mybranch", "mydir/mysubdir/"}, dir)) + assert.Check(t, is.DeepEqual(gitRepo{"https://github.com/user/repo.git", "mybranch", "mydir/mysubdir/"}, dir, cmpGitRepoOpt)) dir, err = parseRemoteURL("git@github.com:user/repo.git") assert.NilError(t, err) - assert.Check(t, len(dir) != 0) - assert.Check(t, is.DeepEqual(gitRepo{"git@github.com:user/repo.git", "master", ""}, dir)) + assert.Check(t, is.DeepEqual(gitRepo{"git@github.com:user/repo.git", "master", ""}, dir, cmpGitRepoOpt)) dir, err = parseRemoteURL("git@github.com:user/repo.git#mybranch:mydir/mysubdir/") assert.NilError(t, err) - assert.Check(t, len(dir) != 0) - assert.Check(t, is.DeepEqual(gitRepo{"git@github.com:user/repo.git", "mybranch", "mydir/mysubdir/"}, dir)) + assert.Check(t, is.DeepEqual(gitRepo{"git@github.com:user/repo.git", "mybranch", "mydir/mysubdir/"}, dir, cmpGitRepoOpt)) } +var cmpGitRepoOpt = cmp.AllowUnexported(gitRepo{}) + func TestCloneArgsSmartHttp(t *testing.T) { mux := http.NewServeMux() server := httptest.NewServer(mux) From 71672ece9c4df05e16971e8a0f6350474ba9ac0f Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Mon, 11 Jun 2018 15:32:11 +0200 Subject: [PATCH 10/39] =?UTF-8?q?Update=20tests=20to=20use=20gotest.tools?= =?UTF-8?q?=20=F0=9F=91=BC?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Vincent Demeester --- cli/command/image/build/internal/git/gitutils_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index a46675b22..8c3967908 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -14,8 +14,8 @@ import ( "testing" "github.com/google/go-cmp/cmp" - "github.com/gotestyourself/gotestyourself/assert" - is "github.com/gotestyourself/gotestyourself/assert/cmp" + "gotest.tools/assert" + is "gotest.tools/assert/cmp" ) func TestParseRemoteURL(t *testing.T) { From 04e2a24a9e28e12908f3d95c42eee780836876f4 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Wed, 6 Feb 2019 11:58:40 -0800 Subject: [PATCH 11/39] gitutils: add validation for ref Signed-off-by: Tonis Tiigi (cherry picked from commit 723b107ca4fba14580a6cd971e63d8af2e7d2bbe) Signed-off-by: Andrew Hsu --- .../image/build/internal/git/gitutils.go | 7 ++++++- .../image/build/internal/git/gitutils_test.go | 21 ++++++++++++++++--- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index 77a45beff..6213963db 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -102,6 +102,11 @@ func parseRemoteURL(remoteURL string) (gitRepo, error) { u.Fragment = "" repo.remote = u.String() } + + if strings.HasPrefix(repo.ref, "-") { + return gitRepo{}, errors.Errorf("invalid refspec: %s", repo.ref) + } + return repo, nil } @@ -124,7 +129,7 @@ func fetchArgs(remoteURL string, ref string) []string { args = append(args, "--depth", "1") } - return append(args, "origin", ref) + return append(args, "origin", "--", ref) } // Check if a given git URL supports a shallow git clone, diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index 8c3967908..34dd495b5 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -59,7 +59,7 @@ func TestCloneArgsSmartHttp(t *testing.T) { }) args := fetchArgs(serverURL.String(), "master") - exp := []string{"fetch", "--depth", "1", "origin", "master"} + exp := []string{"fetch", "--depth", "1", "origin", "--", "master"} assert.Check(t, is.DeepEqual(exp, args)) } @@ -75,13 +75,13 @@ func TestCloneArgsDumbHttp(t *testing.T) { }) args := fetchArgs(serverURL.String(), "master") - exp := []string{"fetch", "origin", "master"} + exp := []string{"fetch", "origin", "--", "master"} assert.Check(t, is.DeepEqual(exp, args)) } func TestCloneArgsGit(t *testing.T) { args := fetchArgs("git://github.com/docker/docker", "master") - exp := []string{"fetch", "--depth", "1", "origin", "master"} + exp := []string{"fetch", "--depth", "1", "origin", "--", "master"} assert.Check(t, is.DeepEqual(exp, args)) } @@ -276,3 +276,18 @@ func TestValidGitTransport(t *testing.T) { } } } + +func TestGitInvalidRef(t *testing.T) { + gitUrls := []string{ + "git://github.com/moby/moby#--foo bar", + "git@github.com/moby/moby#--upload-pack=sleep;:", + "git@g.com:a/b.git#-B", + "git@g.com:a/b.git#with space", + } + + for _, url := range gitUrls { + _, err := Clone(url) + assert.Assert(t, err != nil) + assert.Check(t, is.Contains(strings.ToLower(err.Error()), "invalid refspec")) + } +} From 70aef9f502a705f3a358f4b18c761d4728f60708 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 28 Aug 2019 17:07:29 +0200 Subject: [PATCH 12/39] gosec: add ignore comments for reported issues that can be ignored ``` builder/remotecontext/remote.go:48: G107: Potential HTTP request made with variable url (gosec) builder/remotecontext/git/gitutils.go:145: G107: Potential HTTP request made with variable url (gosec) builder/remotecontext/git/gitutils.go:147: G107: Potential HTTP request made with variable url (gosec) pkg/fileutils/fileutils_test.go:185: G303: File creation in shared tmp directory without using ioutil.Tempfile (gosec) pkg/tarsum/tarsum_test.go:7: G501: Blacklisted import `crypto/md5`: weak cryptographic primitive (gosec) pkg/tarsum/tarsum_test.go:9: G505: Blacklisted import `crypto/sha1`: weak cryptographic primitive (gosec) ``` Signed-off-by: Sebastiaan van Stijn --- cli/command/image/build/internal/git/gitutils.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index 6213963db..cfcf64872 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -142,9 +142,9 @@ func supportsShallowClone(remoteURL string) bool { serviceURL := remoteURL + "/info/refs?service=git-upload-pack" // Try a HEAD request and fallback to a Get request on error - res, err := http.Head(serviceURL) + res, err := http.Head(serviceURL) // #nosec G107 if err != nil || res.StatusCode != http.StatusOK { - res, err = http.Get(serviceURL) + res, err = http.Get(serviceURL) // #nosec G107 if err == nil { res.Body.Close() } From a0d9b0cf0d741121677ed6d1c617a3dda1a40044 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 4 Nov 2019 17:36:49 -0800 Subject: [PATCH 13/39] TestParseRemoteURL: use subtests Signed-off-by: Sebastiaan van Stijn --- .../image/build/internal/git/gitutils_test.go | 89 ++++++++++++++----- 1 file changed, 65 insertions(+), 24 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index 34dd495b5..02d6c63ea 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -19,33 +19,74 @@ import ( ) func TestParseRemoteURL(t *testing.T) { - dir, err := parseRemoteURL("git://github.com/user/repo.git") - assert.NilError(t, err) - assert.Check(t, is.DeepEqual(gitRepo{"git://github.com/user/repo.git", "master", ""}, dir, cmpGitRepoOpt)) + tests := []struct { + doc string + url string + expected gitRepo + }{ + { + doc: "git scheme, no url-fragment", + url: "git://github.com/user/repo.git", + expected: gitRepo{ + remote: "git://github.com/user/repo.git", + ref: "master", + }, + }, + { + doc: "git scheme, with url-fragment", + url: "git://github.com/user/repo.git#mybranch:mydir/mysubdir/", + expected: gitRepo{ + remote: "git://github.com/user/repo.git", + ref: "mybranch", + subdir: "mydir/mysubdir/", + }, + }, + { + doc: "https scheme, no url-fragment", + url: "https://github.com/user/repo.git", + expected: gitRepo{ + remote: "https://github.com/user/repo.git", + ref: "master", + }, + }, + { + doc: "https scheme, with url-fragment", + url: "https://github.com/user/repo.git#mybranch:mydir/mysubdir/", + expected: gitRepo{ + remote: "https://github.com/user/repo.git", + ref: "mybranch", + subdir: "mydir/mysubdir/", + }, + }, + { + doc: "git@, no url-fragment", + url: "git@github.com:user/repo.git", + expected: gitRepo{ + remote: "git@github.com:user/repo.git", + ref: "master", + }, + }, + { + doc: "git@, with url-fragment", + url: "git@github.com:user/repo.git#mybranch:mydir/mysubdir/", + expected: gitRepo{ + remote: "git@github.com:user/repo.git", + ref: "mybranch", + subdir: "mydir/mysubdir/", + }, + }, + } - dir, err = parseRemoteURL("git://github.com/user/repo.git#mybranch:mydir/mysubdir/") - assert.NilError(t, err) - assert.Check(t, is.DeepEqual(gitRepo{"git://github.com/user/repo.git", "mybranch", "mydir/mysubdir/"}, dir, cmpGitRepoOpt)) - - dir, err = parseRemoteURL("https://github.com/user/repo.git") - assert.NilError(t, err) - assert.Check(t, is.DeepEqual(gitRepo{"https://github.com/user/repo.git", "master", ""}, dir, cmpGitRepoOpt)) - - dir, err = parseRemoteURL("https://github.com/user/repo.git#mybranch:mydir/mysubdir/") - assert.NilError(t, err) - assert.Check(t, is.DeepEqual(gitRepo{"https://github.com/user/repo.git", "mybranch", "mydir/mysubdir/"}, dir, cmpGitRepoOpt)) - - dir, err = parseRemoteURL("git@github.com:user/repo.git") - assert.NilError(t, err) - assert.Check(t, is.DeepEqual(gitRepo{"git@github.com:user/repo.git", "master", ""}, dir, cmpGitRepoOpt)) - - dir, err = parseRemoteURL("git@github.com:user/repo.git#mybranch:mydir/mysubdir/") - assert.NilError(t, err) - assert.Check(t, is.DeepEqual(gitRepo{"git@github.com:user/repo.git", "mybranch", "mydir/mysubdir/"}, dir, cmpGitRepoOpt)) + for _, tc := range tests { + tc := tc + t.Run(tc.doc, func(t *testing.T) { + repo, err := parseRemoteURL(tc.url) + assert.NilError(t, err) + assert.Check(t, is.DeepEqual(tc.expected, repo, cmp.AllowUnexported(gitRepo{}))) + }) + } } -var cmpGitRepoOpt = cmp.AllowUnexported(gitRepo{}) - func TestCloneArgsSmartHttp(t *testing.T) { mux := http.NewServeMux() server := httptest.NewServer(mux) From 2d0d4ce4af19800c960c95fd3fd8ab6cb4b9f167 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 5 Nov 2019 09:26:30 -0800 Subject: [PATCH 14/39] builder/remotecontext: use net/url instead of urlutil urlutil.IsUrl() was merely checking if the url had a http(s):// prefix, which is just as well handled through using url.Parse() Signed-off-by: Sebastiaan van Stijn --- .../image/build/internal/git/gitutils.go | 24 ++++++++++++++++--- .../image/build/internal/git/gitutils_test.go | 8 +++++++ 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index cfcf64872..8c39d33c6 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -10,7 +10,6 @@ import ( "strings" "github.com/docker/docker/pkg/symlink" - "github.com/docker/docker/pkg/urlutil" "github.com/pkg/errors" ) @@ -135,7 +134,7 @@ func fetchArgs(remoteURL string, ref string) []string { // Check if a given git URL supports a shallow git clone, // i.e. it is a non-HTTP server or a smart HTTP server. func supportsShallowClone(remoteURL string) bool { - if urlutil.IsURL(remoteURL) { + if scheme := getScheme(remoteURL); scheme == "http" || scheme == "https" { // Check if the HTTP server is smart // Smart servers must correctly respond to a query for the git-upload-pack service @@ -205,5 +204,24 @@ func git(args ...string) ([]byte, error) { // isGitTransport returns true if the provided str is a git transport by inspecting // the prefix of the string for known protocols used in git. func isGitTransport(str string) bool { - return urlutil.IsURL(str) || strings.HasPrefix(str, "git://") || strings.HasPrefix(str, "git@") + if strings.HasPrefix(str, "git@") { + return true + } + + switch getScheme(str) { + case "git", "http", "https": + return true + } + + return false +} + +// getScheme returns addresses' scheme in lowercase, or an empty +// string in case address is an invalid URL. +func getScheme(address string) string { + u, err := url.Parse(address) + if err != nil { + return "" + } + return u.Scheme } diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index 02d6c63ea..aa9e05247 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -24,6 +24,14 @@ func TestParseRemoteURL(t *testing.T) { url string expected gitRepo }{ + { + doc: "git scheme uppercase, no url-fragment", + url: "GIT://github.com/user/repo.git", + expected: gitRepo{ + remote: "git://github.com/user/repo.git", + ref: "master", + }, + }, { doc: "git scheme, no url-fragment", url: "git://github.com/user/repo.git", From ea850377cdbdd0381fc32359316d84968de57049 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 5 Nov 2019 13:58:58 -0800 Subject: [PATCH 15/39] builder/remotecontext: allow ssh:// urls for remote context Signed-off-by: Sebastiaan van Stijn --- .../image/build/internal/git/gitutils.go | 2 +- .../image/build/internal/git/gitutils_test.go | 26 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index 8c39d33c6..ffd512379 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -209,7 +209,7 @@ func isGitTransport(str string) bool { } switch getScheme(str) { - case "git", "http", "https": + case "git", "http", "https", "ssh": return true } diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index aa9e05247..fbbe03076 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -83,6 +83,32 @@ func TestParseRemoteURL(t *testing.T) { subdir: "mydir/mysubdir/", }, }, + { + doc: "ssh, no url-fragment", + url: "ssh://github.com/user/repo.git", + expected: gitRepo{ + remote: "ssh://github.com/user/repo.git", + ref: "master", + }, + }, + { + doc: "ssh, with url-fragment", + url: "ssh://github.com/user/repo.git#mybranch:mydir/mysubdir/", + expected: gitRepo{ + remote: "ssh://github.com/user/repo.git", + ref: "mybranch", + subdir: "mydir/mysubdir/", + }, + }, + { + doc: "ssh, with url-fragment and user", + url: "ssh://foo%40barcorp.com@github.com/user/repo.git#mybranch:mydir/mysubdir/", + expected: gitRepo{ + remote: "ssh://foo%40barcorp.com@github.com/user/repo.git", + ref: "mybranch", + subdir: "mydir/mysubdir/", + }, + }, } for _, tc := range tests { From 5896d383ca26d5c35119304fca6014ac134d8a7c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 7 Feb 2020 14:39:24 +0100 Subject: [PATCH 16/39] bump gotest.tools v3.0.1 for compatibility with Go 1.14 full diff: https://github.com/gotestyourself/gotest.tools/compare/v2.3.0...v3.0.1 Signed-off-by: Sebastiaan van Stijn --- cli/command/image/build/internal/git/gitutils_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index fbbe03076..e5dc05323 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -14,8 +14,8 @@ import ( "testing" "github.com/google/go-cmp/cmp" - "gotest.tools/assert" - is "gotest.tools/assert/cmp" + "gotest.tools/v3/assert" + is "gotest.tools/v3/assert/cmp" ) func TestParseRemoteURL(t *testing.T) { From a4c8c72411917849158307ca87c2091ad93c5136 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 30 Oct 2020 00:54:10 +0100 Subject: [PATCH 17/39] replace pkg/symlink with github.com/moby/sys/symlink Signed-off-by: Sebastiaan van Stijn --- cli/command/image/build/internal/git/gitutils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index ffd512379..b3aba7f58 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -9,7 +9,7 @@ import ( "path/filepath" "strings" - "github.com/docker/docker/pkg/symlink" + "github.com/moby/sys/symlink" "github.com/pkg/errors" ) From 389ada718888631f6ec73e7cb3033b1bf4392ff7 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Mon, 25 Jan 2021 17:30:29 +0000 Subject: [PATCH 18/39] Use golang.org/x/sys/execabs Signed-off-by: Tibor Vass --- cli/command/image/build/internal/git/gitutils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index b3aba7f58..c0f68f8f8 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -5,12 +5,12 @@ import ( "net/http" "net/url" "os" - "os/exec" "path/filepath" "strings" "github.com/moby/sys/symlink" "github.com/pkg/errors" + exec "golang.org/x/sys/execabs" ) type gitRepo struct { From 6d2a9011183bbd57832c9a1589bab159f965ad09 Mon Sep 17 00:00:00 2001 From: Eng Zer Jun Date: Tue, 24 Aug 2021 18:10:50 +0800 Subject: [PATCH 19/39] refactor: move from io/ioutil to io and os package The io/ioutil package has been deprecated in Go 1.16. This commit replaces the existing io/ioutil functions with their new definitions in io and os packages. Signed-off-by: Eng Zer Jun --- .../image/build/internal/git/gitutils.go | 3 +-- .../image/build/internal/git/gitutils_test.go | 17 ++++++++--------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index c0f68f8f8..1dd07851e 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -1,7 +1,6 @@ package git // import "github.com/docker/docker/builder/remotecontext/git" import ( - "io/ioutil" "net/http" "net/url" "os" @@ -34,7 +33,7 @@ func Clone(remoteURL string) (string, error) { func cloneGitRepo(repo gitRepo) (checkoutDir string, err error) { fetch := fetchArgs(repo.remote, repo.ref) - root, err := ioutil.TempDir("", "docker-build-git") + root, err := os.MkdirTemp("", "docker-build-git") if err != nil { return "", err } diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index e5dc05323..17df6fa86 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -2,7 +2,6 @@ package git // import "github.com/docker/docker/builder/remotecontext/git" import ( "fmt" - "io/ioutil" "net/http" "net/http/httptest" "net/url" @@ -171,7 +170,7 @@ func gitGetConfig(name string) string { } func TestCheckoutGit(t *testing.T) { - root, err := ioutil.TempDir("", "docker-build-git-checkout") + root, err := os.MkdirTemp("", "docker-build-git-checkout") assert.NilError(t, err) defer os.RemoveAll(root) @@ -195,13 +194,13 @@ func TestCheckoutGit(t *testing.T) { _, err = gitWithinDir(gitDir, "config", "user.name", "Docker test") assert.NilError(t, err) - err = ioutil.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch"), 0644) + err = os.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch"), 0644) assert.NilError(t, err) subDir := filepath.Join(gitDir, "subdir") assert.NilError(t, os.Mkdir(subDir, 0755)) - err = ioutil.WriteFile(filepath.Join(subDir, "Dockerfile"), []byte("FROM scratch\nEXPOSE 5000"), 0644) + err = os.WriteFile(filepath.Join(subDir, "Dockerfile"), []byte("FROM scratch\nEXPOSE 5000"), 0644) assert.NilError(t, err) if runtime.GOOS != "windows" { @@ -223,10 +222,10 @@ func TestCheckoutGit(t *testing.T) { _, err = gitWithinDir(gitDir, "checkout", "-b", "test") assert.NilError(t, err) - err = ioutil.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch\nEXPOSE 3000"), 0644) + err = os.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch\nEXPOSE 3000"), 0644) assert.NilError(t, err) - err = ioutil.WriteFile(filepath.Join(subDir, "Dockerfile"), []byte("FROM busybox\nEXPOSE 5000"), 0644) + err = os.WriteFile(filepath.Join(subDir, "Dockerfile"), []byte("FROM busybox\nEXPOSE 5000"), 0644) assert.NilError(t, err) _, err = gitWithinDir(gitDir, "add", "-A") @@ -249,7 +248,7 @@ func TestCheckoutGit(t *testing.T) { _, err = gitWithinDir(subrepoDir, "config", "user.name", "Docker test") assert.NilError(t, err) - err = ioutil.WriteFile(filepath.Join(subrepoDir, "subfile"), []byte("subcontents"), 0644) + err = os.WriteFile(filepath.Join(subrepoDir, "subfile"), []byte("subcontents"), 0644) assert.NilError(t, err) _, err = gitWithinDir(subrepoDir, "add", "-A") @@ -310,7 +309,7 @@ func TestCheckoutGit(t *testing.T) { assert.NilError(t, err) defer os.RemoveAll(r) if c.submodule { - b, err := ioutil.ReadFile(filepath.Join(r, "sub/subfile")) + b, err := os.ReadFile(filepath.Join(r, "sub/subfile")) assert.NilError(t, err) assert.Check(t, is.Equal("subcontents", string(b))) } else { @@ -319,7 +318,7 @@ func TestCheckoutGit(t *testing.T) { assert.Assert(t, os.IsNotExist(err)) } - b, err := ioutil.ReadFile(filepath.Join(r, "Dockerfile")) + b, err := os.ReadFile(filepath.Join(r, "Dockerfile")) assert.NilError(t, err) assert.Check(t, is.Equal(c.exp, string(b))) } From 9e39630a052c265991dc15fef71e7d3b7c5735d3 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 10 Apr 2022 16:57:45 +0200 Subject: [PATCH 20/39] pkg/urlutil: deprecate, and move to builder/remotecontext/urlutil pkg/urlutil (despite its poorly chosen name) is not really intended as a generic utility to handle URLs, and should only be used by the builder to handle (remote) build contexts. - IsURL() only does a very rudimentary check for http(s):// prefixes, without any other validation, but due to its name may give incorrect expectations. - IsGitURL() is written specifically with docker build remote git contexts in mind, and has handling for backward-compatibility, where strings that are not URLs, but start with "github.com/" are accepted. Because of the above, this patch: - moves the package inside builder/remotecontext, close to where it's intended to be used (ideally this would be part of build/remotecontext itself, but this package imports many other dependencies, which would introduce those as extra dependencies in the CLI). - deprecates pkg/urlutil, but adds aliases as there are some external consumers. Signed-off-by: Sebastiaan van Stijn --- .../image/build/internal/urlutil/urlutil.go | 46 +++++++++++++++++++ .../build/internal/urlutil/urlutil_test.go | 41 +++++++++++++++++ 2 files changed, 87 insertions(+) create mode 100644 cli/command/image/build/internal/urlutil/urlutil.go create mode 100644 cli/command/image/build/internal/urlutil/urlutil_test.go diff --git a/cli/command/image/build/internal/urlutil/urlutil.go b/cli/command/image/build/internal/urlutil/urlutil.go new file mode 100644 index 000000000..01827d925 --- /dev/null +++ b/cli/command/image/build/internal/urlutil/urlutil.go @@ -0,0 +1,46 @@ +// Package urlutil provides helper function to check urls kind. +// It supports http and git urls. +package urlutil // import "github.com/docker/docker/builder/remotecontext/urlutil" + +import ( + "regexp" + "strings" +) + +var ( + validPrefixes = map[string][]string{ + "url": {"http://", "https://"}, + + // The github.com/ prefix is a special case used to treat context-paths + // starting with `github.com` as a git URL if the given path does not + // exist locally. The "github.com/" prefix is kept for backward compatibility, + // and is a legacy feature. + // + // Going forward, no additional prefixes should be added, and users should + // be encouraged to use explicit URLs (https://github.com/user/repo.git) instead. + "git": {"git://", "github.com/", "git@"}, + } + urlPathWithFragmentSuffix = regexp.MustCompile(".git(?:#.+)?$") +) + +// IsURL returns true if the provided str is an HTTP(S) URL. +func IsURL(str string) bool { + return checkURL(str, "url") +} + +// IsGitURL returns true if the provided str is a git repository URL. +func IsGitURL(str string) bool { + if IsURL(str) && urlPathWithFragmentSuffix.MatchString(str) { + return true + } + return checkURL(str, "git") +} + +func checkURL(str, kind string) bool { + for _, prefix := range validPrefixes[kind] { + if strings.HasPrefix(str, prefix) { + return true + } + } + return false +} diff --git a/cli/command/image/build/internal/urlutil/urlutil_test.go b/cli/command/image/build/internal/urlutil/urlutil_test.go new file mode 100644 index 000000000..690611832 --- /dev/null +++ b/cli/command/image/build/internal/urlutil/urlutil_test.go @@ -0,0 +1,41 @@ +package urlutil // import "github.com/docker/docker/builder/remotecontext/urlutil" + +import "testing" + +var ( + gitUrls = []string{ + "git://github.com/docker/docker", + "git@github.com:docker/docker.git", + "git@bitbucket.org:atlassianlabs/atlassian-docker.git", + "https://github.com/docker/docker.git", + "http://github.com/docker/docker.git", + "http://github.com/docker/docker.git#branch", + "http://github.com/docker/docker.git#:dir", + } + incompleteGitUrls = []string{ + "github.com/docker/docker", + } + invalidGitUrls = []string{ + "http://github.com/docker/docker.git:#branch", + } +) + +func TestIsGIT(t *testing.T) { + for _, url := range gitUrls { + if !IsGitURL(url) { + t.Fatalf("%q should be detected as valid Git url", url) + } + } + + for _, url := range incompleteGitUrls { + if !IsGitURL(url) { + t.Fatalf("%q should be detected as valid Git url", url) + } + } + + for _, url := range invalidGitUrls { + if IsGitURL(url) { + t.Fatalf("%q should not be detected as valid Git prefix", url) + } + } +} From 26a11366a7efce0e741695a055bab40f208e6582 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 10 Apr 2022 20:08:13 +0200 Subject: [PATCH 21/39] builder/remotecontext/urlutil: simplify and improve documentation Simplify some of the logic, and add documentation about the package, as well as warnings that this package should not be used as a general- purpose utility. Signed-off-by: Sebastiaan van Stijn --- .../image/build/internal/urlutil/urlutil.go | 92 ++++++++++++++----- 1 file changed, 67 insertions(+), 25 deletions(-) diff --git a/cli/command/image/build/internal/urlutil/urlutil.go b/cli/command/image/build/internal/urlutil/urlutil.go index 01827d925..d4078942e 100644 --- a/cli/command/image/build/internal/urlutil/urlutil.go +++ b/cli/command/image/build/internal/urlutil/urlutil.go @@ -1,5 +1,8 @@ -// Package urlutil provides helper function to check urls kind. -// It supports http and git urls. +// Package urlutil provides helper function to check if a given build-context +// location should be considered a URL or a remote Git repository. +// +// This package is specifically written for use with docker build contexts, and +// should not be used as a general-purpose utility. package urlutil // import "github.com/docker/docker/builder/remotecontext/urlutil" import ( @@ -7,37 +10,76 @@ import ( "strings" ) -var ( - validPrefixes = map[string][]string{ - "url": {"http://", "https://"}, +// urlPathWithFragmentSuffix matches fragments to use as Git reference and build +// context from the Git repository. See IsGitURL for details. +var urlPathWithFragmentSuffix = regexp.MustCompile(".git(?:#.+)?$") - // The github.com/ prefix is a special case used to treat context-paths - // starting with `github.com` as a git URL if the given path does not - // exist locally. The "github.com/" prefix is kept for backward compatibility, - // and is a legacy feature. - // - // Going forward, no additional prefixes should be added, and users should - // be encouraged to use explicit URLs (https://github.com/user/repo.git) instead. - "git": {"git://", "github.com/", "git@"}, - } - urlPathWithFragmentSuffix = regexp.MustCompile(".git(?:#.+)?$") -) - -// IsURL returns true if the provided str is an HTTP(S) URL. +// IsURL returns true if the provided str is an HTTP(S) URL by checking if it +// has a http:// or https:// scheme. No validation is performed to verify if the +// URL is well-formed. func IsURL(str string) bool { - return checkURL(str, "url") + return strings.HasPrefix(str, "https://") || strings.HasPrefix(str, "http://") } -// IsGitURL returns true if the provided str is a git repository URL. +// IsGitURL returns true if the provided str is a remote git repository "URL". +// +// This function only performs a rudimentary check (no validation is performed +// to ensure the URL is well-formed), and is written specifically for use with +// docker build, with some logic for backward compatibility with older versions +// of docker: do not use this function as a general-purpose utility. +// +// The following patterns are considered to be a Git URL: +// +// - https://(.*).git(?:#.+)?$ git repository URL with optional fragment, as +// known to be used by GitHub and GitLab. +// - http://(.*).git(?:#.+)?$ same, but non-TLS +// - git://(.*) URLs using git:// scheme +// - git@(.*) +// - github.com/ see description below +// +// The github.com/ prefix is a special case used to treat context-paths +// starting with "github.com/" as a git URL if the given path does not +// exist locally. The "github.com/" prefix is kept for backward compatibility, +// and is a legacy feature. +// +// Going forward, no additional prefixes should be added, and users should +// be encouraged to use explicit URLs (https://github.com/user/repo.git) instead. +// +// Note that IsGitURL does not check if "github.com/" prefixes exist as a local +// path. Code using this function should check if the path exists locally before +// using it as a URL. +// +// Fragments +// +// Git URLs accept context configuration in their fragment section, separated by +// a colon (`:`). The first part represents the reference to check out, and can +// be either a branch, a tag, or a remote reference. The second part represents +// a subdirectory inside the repository to use as the build context. +// +// For example,the following URL uses a directory named "docker" in the branch +// "container" in the https://github.com/myorg/my-repo.git repository: +// +// https://github.com/myorg/my-repo.git#container:docker +// +// The following table represents all the valid suffixes with their build +// contexts: +// +// | Build Syntax Suffix | Git reference used | Build Context Used | +// |--------------------------------|----------------------|--------------------| +// | my-repo.git | refs/heads/master | / | +// | my-repo.git#mytag | refs/tags/my-tag | / | +// | my-repo.git#mybranch | refs/heads/my-branch | / | +// | my-repo.git#pull/42/head | refs/pull/42/head | / | +// | my-repo.git#:directory | refs/heads/master | /directory | +// | my-repo.git#master:directory | refs/heads/master | /directory | +// | my-repo.git#mytag:directory | refs/tags/my-tag | /directory | +// | my-repo.git#mybranch:directory | refs/heads/my-branch | /directory | +// func IsGitURL(str string) bool { if IsURL(str) && urlPathWithFragmentSuffix.MatchString(str) { return true } - return checkURL(str, "git") -} - -func checkURL(str, kind string) bool { - for _, prefix := range validPrefixes[kind] { + for _, prefix := range []string{"git://", "github.com/", "git@"} { if strings.HasPrefix(str, prefix) { return true } From a12090d78786c9a423748b5021fb73735539fa39 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 8 Jul 2022 18:27:07 +0200 Subject: [PATCH 22/39] gofmt GoDoc comments with go1.19 Older versions of Go don't format comments, so committing this as a separate commit, so that we can already make these changes before we upgrade to Go 1.19. Signed-off-by: Sebastiaan van Stijn --- .../image/build/internal/urlutil/urlutil.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/cli/command/image/build/internal/urlutil/urlutil.go b/cli/command/image/build/internal/urlutil/urlutil.go index d4078942e..e38988a30 100644 --- a/cli/command/image/build/internal/urlutil/urlutil.go +++ b/cli/command/image/build/internal/urlutil/urlutil.go @@ -30,12 +30,11 @@ func IsURL(str string) bool { // // The following patterns are considered to be a Git URL: // -// - https://(.*).git(?:#.+)?$ git repository URL with optional fragment, as -// known to be used by GitHub and GitLab. -// - http://(.*).git(?:#.+)?$ same, but non-TLS -// - git://(.*) URLs using git:// scheme -// - git@(.*) -// - github.com/ see description below +// - https://(.*).git(?:#.+)?$ git repository URL with optional fragment, as known to be used by GitHub and GitLab. +// - http://(.*).git(?:#.+)?$ same, but non-TLS +// - git://(.*) URLs using git:// scheme +// - git@(.*) +// - github.com/ see description below // // The github.com/ prefix is a special case used to treat context-paths // starting with "github.com/" as a git URL if the given path does not @@ -49,7 +48,7 @@ func IsURL(str string) bool { // path. Code using this function should check if the path exists locally before // using it as a URL. // -// Fragments +// # Fragments // // Git URLs accept context configuration in their fragment section, separated by // a colon (`:`). The first part represents the reference to check out, and can @@ -74,7 +73,6 @@ func IsURL(str string) bool { // | my-repo.git#master:directory | refs/heads/master | /directory | // | my-repo.git#mytag:directory | refs/tags/my-tag | /directory | // | my-repo.git#mybranch:directory | refs/heads/my-branch | /directory | -// func IsGitURL(str string) bool { if IsURL(str) && urlPathWithFragmentSuffix.MatchString(str) { return true From 3f4cc89f64bc15940c949b8711a9e5771c22f4cc Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Wed, 27 Jul 2022 15:46:46 -0400 Subject: [PATCH 23/39] builder: modernize TestCheckoutGit Make the test more debuggable by logging all git command output and running each table-driven test case as a subtest. Signed-off-by: Cory Snider --- .../image/build/internal/git/gitutils_test.go | 139 +++++++----------- 1 file changed, 54 insertions(+), 85 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index 17df6fa86..47b897fa2 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -170,9 +170,7 @@ func gitGetConfig(name string) string { } func TestCheckoutGit(t *testing.T) { - root, err := os.MkdirTemp("", "docker-build-git-checkout") - assert.NilError(t, err) - defer os.RemoveAll(root) + root := t.TempDir() autocrlf := gitGetConfig("core.autocrlf") if !(autocrlf == "true" || autocrlf == "false" || @@ -184,88 +182,57 @@ func TestCheckoutGit(t *testing.T) { eol = "\r\n" } + must := func(out []byte, err error) { + t.Helper() + if len(out) > 0 { + t.Logf("%s", out) + } + assert.NilError(t, err) + } + gitDir := filepath.Join(root, "repo") - _, err = git("init", gitDir) - assert.NilError(t, err) - - _, err = gitWithinDir(gitDir, "config", "user.email", "test@docker.com") - assert.NilError(t, err) - - _, err = gitWithinDir(gitDir, "config", "user.name", "Docker test") - assert.NilError(t, err) - - err = os.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch"), 0644) - assert.NilError(t, err) + must(git("-c", "init.defaultBranch=master", "init", gitDir)) + must(gitWithinDir(gitDir, "config", "user.email", "test@docker.com")) + must(gitWithinDir(gitDir, "config", "user.name", "Docker test")) + assert.NilError(t, os.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch"), 0644)) subDir := filepath.Join(gitDir, "subdir") assert.NilError(t, os.Mkdir(subDir, 0755)) - - err = os.WriteFile(filepath.Join(subDir, "Dockerfile"), []byte("FROM scratch\nEXPOSE 5000"), 0644) - assert.NilError(t, err) + assert.NilError(t, os.WriteFile(filepath.Join(subDir, "Dockerfile"), []byte("FROM scratch\nEXPOSE 5000"), 0644)) if runtime.GOOS != "windows" { - if err = os.Symlink("../subdir", filepath.Join(gitDir, "parentlink")); err != nil { - t.Fatal(err) - } - - if err = os.Symlink("/subdir", filepath.Join(gitDir, "absolutelink")); err != nil { - t.Fatal(err) - } + assert.NilError(t, os.Symlink("../subdir", filepath.Join(gitDir, "parentlink"))) + assert.NilError(t, os.Symlink("/subdir", filepath.Join(gitDir, "absolutelink"))) } - _, err = gitWithinDir(gitDir, "add", "-A") - assert.NilError(t, err) + must(gitWithinDir(gitDir, "add", "-A")) + must(gitWithinDir(gitDir, "commit", "-am", "First commit")) + must(gitWithinDir(gitDir, "checkout", "-b", "test")) - _, err = gitWithinDir(gitDir, "commit", "-am", "First commit") - assert.NilError(t, err) + assert.NilError(t, os.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch\nEXPOSE 3000"), 0644)) + assert.NilError(t, os.WriteFile(filepath.Join(subDir, "Dockerfile"), []byte("FROM busybox\nEXPOSE 5000"), 0644)) - _, err = gitWithinDir(gitDir, "checkout", "-b", "test") - assert.NilError(t, err) - - err = os.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch\nEXPOSE 3000"), 0644) - assert.NilError(t, err) - - err = os.WriteFile(filepath.Join(subDir, "Dockerfile"), []byte("FROM busybox\nEXPOSE 5000"), 0644) - assert.NilError(t, err) - - _, err = gitWithinDir(gitDir, "add", "-A") - assert.NilError(t, err) - - _, err = gitWithinDir(gitDir, "commit", "-am", "Branch commit") - assert.NilError(t, err) - - _, err = gitWithinDir(gitDir, "checkout", "master") - assert.NilError(t, err) + must(gitWithinDir(gitDir, "add", "-A")) + must(gitWithinDir(gitDir, "commit", "-am", "Branch commit")) + must(gitWithinDir(gitDir, "checkout", "master")) // set up submodule subrepoDir := filepath.Join(root, "subrepo") - _, err = git("init", subrepoDir) - assert.NilError(t, err) + must(git("-c", "init.defaultBranch=master", "init", subrepoDir)) + must(gitWithinDir(subrepoDir, "config", "user.email", "test@docker.com")) + must(gitWithinDir(subrepoDir, "config", "user.name", "Docker test")) - _, err = gitWithinDir(subrepoDir, "config", "user.email", "test@docker.com") - assert.NilError(t, err) + assert.NilError(t, os.WriteFile(filepath.Join(subrepoDir, "subfile"), []byte("subcontents"), 0644)) - _, err = gitWithinDir(subrepoDir, "config", "user.name", "Docker test") - assert.NilError(t, err) - - err = os.WriteFile(filepath.Join(subrepoDir, "subfile"), []byte("subcontents"), 0644) - assert.NilError(t, err) - - _, err = gitWithinDir(subrepoDir, "add", "-A") - assert.NilError(t, err) - - _, err = gitWithinDir(subrepoDir, "commit", "-am", "Subrepo initial") - assert.NilError(t, err) + must(gitWithinDir(subrepoDir, "add", "-A")) + must(gitWithinDir(subrepoDir, "commit", "-am", "Subrepo initial")) cmd := exec.Command("git", "submodule", "add", subrepoDir, "sub") // this command doesn't work with --work-tree cmd.Dir = gitDir - assert.NilError(t, cmd.Run()) + must(cmd.CombinedOutput()) - _, err = gitWithinDir(gitDir, "add", "-A") - assert.NilError(t, err) - - _, err = gitWithinDir(gitDir, "commit", "-am", "With submodule") - assert.NilError(t, err) + must(gitWithinDir(gitDir, "add", "-A")) + must(gitWithinDir(gitDir, "commit", "-am", "With submodule")) type singleCase struct { frag string @@ -299,28 +266,30 @@ func TestCheckoutGit(t *testing.T) { } for _, c := range cases { - ref, subdir := getRefAndSubdir(c.frag) - r, err := cloneGitRepo(gitRepo{remote: gitDir, ref: ref, subdir: subdir}) + t.Run(c.frag, func(t *testing.T) { + ref, subdir := getRefAndSubdir(c.frag) + r, err := cloneGitRepo(gitRepo{remote: gitDir, ref: ref, subdir: subdir}) - if c.fail { - assert.Check(t, is.ErrorContains(err, "")) - continue - } - assert.NilError(t, err) - defer os.RemoveAll(r) - if c.submodule { - b, err := os.ReadFile(filepath.Join(r, "sub/subfile")) + if c.fail { + assert.Check(t, is.ErrorContains(err, "")) + return + } assert.NilError(t, err) - assert.Check(t, is.Equal("subcontents", string(b))) - } else { - _, err := os.Stat(filepath.Join(r, "sub/subfile")) - assert.Assert(t, is.ErrorContains(err, "")) - assert.Assert(t, os.IsNotExist(err)) - } + defer os.RemoveAll(r) + if c.submodule { + b, err := os.ReadFile(filepath.Join(r, "sub/subfile")) + assert.NilError(t, err) + assert.Check(t, is.Equal("subcontents", string(b))) + } else { + _, err := os.Stat(filepath.Join(r, "sub/subfile")) + assert.Assert(t, is.ErrorContains(err, "")) + assert.Assert(t, os.IsNotExist(err)) + } - b, err := os.ReadFile(filepath.Join(r, "Dockerfile")) - assert.NilError(t, err) - assert.Check(t, is.Equal(c.exp, string(b))) + b, err := os.ReadFile(filepath.Join(r, "Dockerfile")) + assert.NilError(t, err) + assert.Check(t, is.Equal(c.exp, string(b))) + }) } } From 3bfb30acd73b4e55bdcf21c4145e4795a7013ec5 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Wed, 27 Jul 2022 16:05:13 -0400 Subject: [PATCH 24/39] builder: explicitly set CWD for all git commands Keep It Simple! Set the working directory for git commands by...setting the git process's working directory. Git commands can be run in the parent process's working directory by passing the empty string. Signed-off-by: Cory Snider --- cli/command/image/build/internal/git/gitutils.go | 9 +++------ .../image/build/internal/git/gitutils_test.go | 12 ++++-------- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index 1dd07851e..bfcb6ab08 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -192,12 +192,9 @@ func checkoutGit(root, ref, subdir string) (string, error) { } func gitWithinDir(dir string, args ...string) ([]byte, error) { - a := []string{"--work-tree", dir, "--git-dir", filepath.Join(dir, ".git")} - return git(append(a, args...)...) -} - -func git(args ...string) ([]byte, error) { - return exec.Command("git", args...).CombinedOutput() + cmd := exec.Command("git", args...) + cmd.Dir = dir + return cmd.CombinedOutput() } // isGitTransport returns true if the provided str is a git transport by inspecting diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index 47b897fa2..a91a6a084 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -6,7 +6,6 @@ import ( "net/http/httptest" "net/url" "os" - "os/exec" "path/filepath" "runtime" "strings" @@ -160,7 +159,7 @@ func TestCloneArgsGit(t *testing.T) { } func gitGetConfig(name string) string { - b, err := git([]string{"config", "--get", name}...) + b, err := gitWithinDir("", "config", "--get", name) if err != nil { // since we are interested in empty or non empty string, // we can safely ignore the err here. @@ -191,7 +190,7 @@ func TestCheckoutGit(t *testing.T) { } gitDir := filepath.Join(root, "repo") - must(git("-c", "init.defaultBranch=master", "init", gitDir)) + must(gitWithinDir(root, "-c", "init.defaultBranch=master", "init", gitDir)) must(gitWithinDir(gitDir, "config", "user.email", "test@docker.com")) must(gitWithinDir(gitDir, "config", "user.name", "Docker test")) assert.NilError(t, os.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch"), 0644)) @@ -218,7 +217,7 @@ func TestCheckoutGit(t *testing.T) { // set up submodule subrepoDir := filepath.Join(root, "subrepo") - must(git("-c", "init.defaultBranch=master", "init", subrepoDir)) + must(gitWithinDir(root, "-c", "init.defaultBranch=master", "init", subrepoDir)) must(gitWithinDir(subrepoDir, "config", "user.email", "test@docker.com")) must(gitWithinDir(subrepoDir, "config", "user.name", "Docker test")) @@ -227,10 +226,7 @@ func TestCheckoutGit(t *testing.T) { must(gitWithinDir(subrepoDir, "add", "-A")) must(gitWithinDir(subrepoDir, "commit", "-am", "Subrepo initial")) - cmd := exec.Command("git", "submodule", "add", subrepoDir, "sub") // this command doesn't work with --work-tree - cmd.Dir = gitDir - must(cmd.CombinedOutput()) - + must(gitWithinDir(gitDir, "submodule", "add", subrepoDir, "sub")) must(gitWithinDir(gitDir, "add", "-A")) must(gitWithinDir(gitDir, "commit", "-am", "With submodule")) From 876fc1dac4625d1cce4974a0a6337551c8773074 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Thu, 28 Jul 2022 13:17:42 -0400 Subject: [PATCH 25/39] builder: isolate git from local system Prevent git commands we run from reading the user or system configuration, or cloning submodules from the local filesystem. Signed-off-by: Cory Snider --- .../image/build/internal/git/gitutils.go | 6 +++ .../image/build/internal/git/gitutils_test.go | 51 ++++++++++++++++++- 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index bfcb6ab08..b24dd52bc 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -192,8 +192,14 @@ func checkoutGit(root, ref, subdir string) (string, error) { } func gitWithinDir(dir string, args ...string) ([]byte, error) { + args = append([]string{"-c", "protocol.file.allow=never"}, args...) // Block sneaky repositories from using repos from the filesystem as submodules. cmd := exec.Command("git", args...) cmd.Dir = dir + cmd.Env = append(cmd.Env, + "GIT_PROTOCOL_FROM_USER=0", // Disable unsafe remote protocols. + "GIT_CONFIG_NOSYSTEM=1", // Disable reading from system gitconfig. + "HOME=/dev/null", // Disable reading from user gitconfig. + ) return cmd.CombinedOutput() } diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index a91a6a084..929030e17 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -1,11 +1,14 @@ package git // import "github.com/docker/docker/builder/remotecontext/git" import ( + "bytes" "fmt" "net/http" + "net/http/cgi" "net/http/httptest" "net/url" "os" + "os/exec" "path/filepath" "runtime" "strings" @@ -171,6 +174,49 @@ func gitGetConfig(name string) string { func TestCheckoutGit(t *testing.T) { root := t.TempDir() + gitpath, err := exec.LookPath("git") + assert.NilError(t, err) + gitversion, _ := exec.Command(gitpath, "version").CombinedOutput() + t.Logf("%s", gitversion) // E.g. "git version 2.30.2" + + // Serve all repositories under root using the Smart HTTP protocol so + // they can be cloned. The Dumb HTTP protocol is incompatible with + // shallow cloning but we unconditionally shallow-clone submodules, and + // we explicitly disable the file protocol. + // (Another option would be to use `git daemon` and the Git protocol, + // but that listens on a fixed port number which is a recipe for + // disaster in CI. Funnily enough, `git daemon --port=0` works but there + // is no easy way to discover which port got picked!) + + // Associate git-http-backend logs with the current (sub)test. + // Incompatible with parallel subtests. + currentSubtest := t + githttp := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var logs bytes.Buffer + (&cgi.Handler{ + Path: gitpath, + Args: []string{"http-backend"}, + Dir: root, + Env: []string{ + "GIT_PROJECT_ROOT=" + root, + "GIT_HTTP_EXPORT_ALL=1", + }, + Stderr: &logs, + }).ServeHTTP(w, r) + if logs.Len() == 0 { + return + } + for { + line, err := logs.ReadString('\n') + currentSubtest.Log("git-http-backend: " + line) + if err != nil { + break + } + } + }) + server := httptest.NewServer(&githttp) + defer server.Close() + autocrlf := gitGetConfig("core.autocrlf") if !(autocrlf == "true" || autocrlf == "false" || autocrlf == "input" || autocrlf == "") { @@ -226,7 +272,7 @@ func TestCheckoutGit(t *testing.T) { must(gitWithinDir(subrepoDir, "add", "-A")) must(gitWithinDir(subrepoDir, "commit", "-am", "Subrepo initial")) - must(gitWithinDir(gitDir, "submodule", "add", subrepoDir, "sub")) + must(gitWithinDir(gitDir, "submodule", "add", server.URL+"/subrepo", "sub")) must(gitWithinDir(gitDir, "add", "-A")) must(gitWithinDir(gitDir, "commit", "-am", "With submodule")) @@ -263,8 +309,9 @@ func TestCheckoutGit(t *testing.T) { for _, c := range cases { t.Run(c.frag, func(t *testing.T) { + currentSubtest = t ref, subdir := getRefAndSubdir(c.frag) - r, err := cloneGitRepo(gitRepo{remote: gitDir, ref: ref, subdir: subdir}) + r, err := cloneGitRepo(gitRepo{remote: server.URL + "/repo", ref: ref, subdir: subdir}) if c.fail { assert.Check(t, is.ErrorContains(err, "")) From bcd6c45731d59420497b3a8ad772003d0b4c8470 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Thu, 13 Oct 2022 17:28:02 -0400 Subject: [PATCH 26/39] builder: make git config isolation opt-in While it is undesirable for the system or user git config to be used when the daemon clones a Git repo, it could break workflows if it was unconditionally applied to docker/cli as well. Signed-off-by: Cory Snider --- .../image/build/internal/git/gitutils.go | 61 +++++++++++++------ .../image/build/internal/git/gitutils_test.go | 38 ++++++------ 2 files changed, 60 insertions(+), 39 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index b24dd52bc..e042e2397 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -16,21 +16,37 @@ type gitRepo struct { remote string ref string subdir string + + isolateConfig bool +} + +type CloneOption func(*gitRepo) + +// WithIsolatedConfig disables reading the user or system gitconfig files when +// performing Git operations. +func WithIsolatedConfig(v bool) CloneOption { + return func(gr *gitRepo) { + gr.isolateConfig = v + } } // Clone clones a repository into a newly created directory which // will be under "docker-build-git" -func Clone(remoteURL string) (string, error) { +func Clone(remoteURL string, opts ...CloneOption) (string, error) { repo, err := parseRemoteURL(remoteURL) if err != nil { return "", err } - return cloneGitRepo(repo) + for _, opt := range opts { + opt(&repo) + } + + return repo.clone() } -func cloneGitRepo(repo gitRepo) (checkoutDir string, err error) { +func (repo gitRepo) clone() (checkoutDir string, err error) { fetch := fetchArgs(repo.remote, repo.ref) root, err := os.MkdirTemp("", "docker-build-git") @@ -44,21 +60,21 @@ func cloneGitRepo(repo gitRepo) (checkoutDir string, err error) { } }() - if out, err := gitWithinDir(root, "init"); err != nil { + if out, err := repo.gitWithinDir(root, "init"); err != nil { return "", errors.Wrapf(err, "failed to init repo at %s: %s", root, out) } // Add origin remote for compatibility with previous implementation that // used "git clone" and also to make sure local refs are created for branches - if out, err := gitWithinDir(root, "remote", "add", "origin", repo.remote); err != nil { + if out, err := repo.gitWithinDir(root, "remote", "add", "origin", repo.remote); err != nil { return "", errors.Wrapf(err, "failed add origin repo at %s: %s", repo.remote, out) } - if output, err := gitWithinDir(root, fetch...); err != nil { + if output, err := repo.gitWithinDir(root, fetch...); err != nil { return "", errors.Wrapf(err, "error fetching: %s", output) } - checkoutDir, err = checkoutGit(root, repo.ref, repo.subdir) + checkoutDir, err = repo.checkout(root) if err != nil { return "", err } @@ -162,20 +178,20 @@ func supportsShallowClone(remoteURL string) bool { return true } -func checkoutGit(root, ref, subdir string) (string, error) { +func (repo gitRepo) checkout(root string) (string, error) { // Try checking out by ref name first. This will work on branches and sets // .git/HEAD to the current branch name - if output, err := gitWithinDir(root, "checkout", ref); err != nil { + if output, err := repo.gitWithinDir(root, "checkout", repo.ref); err != nil { // If checking out by branch name fails check out the last fetched ref - if _, err2 := gitWithinDir(root, "checkout", "FETCH_HEAD"); err2 != nil { - return "", errors.Wrapf(err, "error checking out %s: %s", ref, output) + if _, err2 := repo.gitWithinDir(root, "checkout", "FETCH_HEAD"); err2 != nil { + return "", errors.Wrapf(err, "error checking out %s: %s", repo.ref, output) } } - if subdir != "" { - newCtx, err := symlink.FollowSymlinkInScope(filepath.Join(root, subdir), root) + if repo.subdir != "" { + newCtx, err := symlink.FollowSymlinkInScope(filepath.Join(root, repo.subdir), root) if err != nil { - return "", errors.Wrapf(err, "error setting git context, %q not within git root", subdir) + return "", errors.Wrapf(err, "error setting git context, %q not within git root", repo.subdir) } fi, err := os.Stat(newCtx) @@ -191,15 +207,20 @@ func checkoutGit(root, ref, subdir string) (string, error) { return root, nil } -func gitWithinDir(dir string, args ...string) ([]byte, error) { +func (repo gitRepo) gitWithinDir(dir string, args ...string) ([]byte, error) { args = append([]string{"-c", "protocol.file.allow=never"}, args...) // Block sneaky repositories from using repos from the filesystem as submodules. cmd := exec.Command("git", args...) cmd.Dir = dir - cmd.Env = append(cmd.Env, - "GIT_PROTOCOL_FROM_USER=0", // Disable unsafe remote protocols. - "GIT_CONFIG_NOSYSTEM=1", // Disable reading from system gitconfig. - "HOME=/dev/null", // Disable reading from user gitconfig. - ) + // Disable unsafe remote protocols. + cmd.Env = append(cmd.Env, "GIT_PROTOCOL_FROM_USER=0") + + if repo.isolateConfig { + cmd.Env = append(cmd.Env, + "GIT_CONFIG_NOSYSTEM=1", // Disable reading from system gitconfig. + "HOME=/dev/null", // Disable reading from user gitconfig. + ) + } + return cmd.CombinedOutput() } diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index 929030e17..f3c67b302 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -162,7 +162,7 @@ func TestCloneArgsGit(t *testing.T) { } func gitGetConfig(name string) string { - b, err := gitWithinDir("", "config", "--get", name) + b, err := gitRepo{}.gitWithinDir("", "config", "--get", name) if err != nil { // since we are interested in empty or non empty string, // we can safely ignore the err here. @@ -236,9 +236,9 @@ func TestCheckoutGit(t *testing.T) { } gitDir := filepath.Join(root, "repo") - must(gitWithinDir(root, "-c", "init.defaultBranch=master", "init", gitDir)) - must(gitWithinDir(gitDir, "config", "user.email", "test@docker.com")) - must(gitWithinDir(gitDir, "config", "user.name", "Docker test")) + must(gitRepo{}.gitWithinDir(root, "-c", "init.defaultBranch=master", "init", gitDir)) + must(gitRepo{}.gitWithinDir(gitDir, "config", "user.email", "test@docker.com")) + must(gitRepo{}.gitWithinDir(gitDir, "config", "user.name", "Docker test")) assert.NilError(t, os.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch"), 0644)) subDir := filepath.Join(gitDir, "subdir") @@ -250,31 +250,31 @@ func TestCheckoutGit(t *testing.T) { assert.NilError(t, os.Symlink("/subdir", filepath.Join(gitDir, "absolutelink"))) } - must(gitWithinDir(gitDir, "add", "-A")) - must(gitWithinDir(gitDir, "commit", "-am", "First commit")) - must(gitWithinDir(gitDir, "checkout", "-b", "test")) + must(gitRepo{}.gitWithinDir(gitDir, "add", "-A")) + must(gitRepo{}.gitWithinDir(gitDir, "commit", "-am", "First commit")) + must(gitRepo{}.gitWithinDir(gitDir, "checkout", "-b", "test")) assert.NilError(t, os.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch\nEXPOSE 3000"), 0644)) assert.NilError(t, os.WriteFile(filepath.Join(subDir, "Dockerfile"), []byte("FROM busybox\nEXPOSE 5000"), 0644)) - must(gitWithinDir(gitDir, "add", "-A")) - must(gitWithinDir(gitDir, "commit", "-am", "Branch commit")) - must(gitWithinDir(gitDir, "checkout", "master")) + must(gitRepo{}.gitWithinDir(gitDir, "add", "-A")) + must(gitRepo{}.gitWithinDir(gitDir, "commit", "-am", "Branch commit")) + must(gitRepo{}.gitWithinDir(gitDir, "checkout", "master")) // set up submodule subrepoDir := filepath.Join(root, "subrepo") - must(gitWithinDir(root, "-c", "init.defaultBranch=master", "init", subrepoDir)) - must(gitWithinDir(subrepoDir, "config", "user.email", "test@docker.com")) - must(gitWithinDir(subrepoDir, "config", "user.name", "Docker test")) + must(gitRepo{}.gitWithinDir(root, "-c", "init.defaultBranch=master", "init", subrepoDir)) + must(gitRepo{}.gitWithinDir(subrepoDir, "config", "user.email", "test@docker.com")) + must(gitRepo{}.gitWithinDir(subrepoDir, "config", "user.name", "Docker test")) assert.NilError(t, os.WriteFile(filepath.Join(subrepoDir, "subfile"), []byte("subcontents"), 0644)) - must(gitWithinDir(subrepoDir, "add", "-A")) - must(gitWithinDir(subrepoDir, "commit", "-am", "Subrepo initial")) + must(gitRepo{}.gitWithinDir(subrepoDir, "add", "-A")) + must(gitRepo{}.gitWithinDir(subrepoDir, "commit", "-am", "Subrepo initial")) - must(gitWithinDir(gitDir, "submodule", "add", server.URL+"/subrepo", "sub")) - must(gitWithinDir(gitDir, "add", "-A")) - must(gitWithinDir(gitDir, "commit", "-am", "With submodule")) + must(gitRepo{}.gitWithinDir(gitDir, "submodule", "add", server.URL+"/subrepo", "sub")) + must(gitRepo{}.gitWithinDir(gitDir, "add", "-A")) + must(gitRepo{}.gitWithinDir(gitDir, "commit", "-am", "With submodule")) type singleCase struct { frag string @@ -311,7 +311,7 @@ func TestCheckoutGit(t *testing.T) { t.Run(c.frag, func(t *testing.T) { currentSubtest = t ref, subdir := getRefAndSubdir(c.frag) - r, err := cloneGitRepo(gitRepo{remote: server.URL + "/repo", ref: ref, subdir: subdir}) + r, err := gitRepo{remote: server.URL + "/repo", ref: ref, subdir: subdir}.clone() if c.fail { assert.Check(t, is.ErrorContains(err, "")) From 212213e81e08901a04cc7420d15441cb8d5534c3 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Wed, 19 Oct 2022 20:11:27 -0400 Subject: [PATCH 27/39] builder: fix running git commands on Windows Setting cmd.Env overrides the default of passing through the parent process' environment, which works out fine most of the time, except when it doesn't. For whatever reason, leaving out all the environment causes git-for-windows sh.exe subprocesses to enter an infinite loop of access violations during Cygwin initialization in certain environments (specifically, our very own dev container image). Signed-off-by: Cory Snider --- cli/command/image/build/internal/git/gitutils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index e042e2397..25c6107bb 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -212,7 +212,7 @@ func (repo gitRepo) gitWithinDir(dir string, args ...string) ([]byte, error) { cmd := exec.Command("git", args...) cmd.Dir = dir // Disable unsafe remote protocols. - cmd.Env = append(cmd.Env, "GIT_PROTOCOL_FROM_USER=0") + cmd.Env = append(cmd.Environ(), "GIT_PROTOCOL_FROM_USER=0") if repo.isolateConfig { cmd.Env = append(cmd.Env, From 5c21ec520efd6b58b4cc7b1ef1a68f0bc8eb593a Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Thu, 20 Oct 2022 14:03:36 -0400 Subject: [PATCH 28/39] builder: add missing doc comment Signed-off-by: Cory Snider --- cli/command/image/build/internal/git/gitutils.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index 25c6107bb..6af957c40 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -20,6 +20,7 @@ type gitRepo struct { isolateConfig bool } +// CloneOption changes the behaviour of Clone(). type CloneOption func(*gitRepo) // WithIsolatedConfig disables reading the user or system gitconfig files when From 66713384c3b9464c987afd4641ecafc05ee303b4 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 21 Oct 2022 17:41:41 +0200 Subject: [PATCH 29/39] builder/remotecontext/git: allow building on go1.18 cmd.Environ() is new in go1.19, and not needed for this specific case. Without this, trying to use this package in code that uses go1.18 will fail; builder/remotecontext/git/gitutils.go:216:23: cmd.Environ undefined (type *exec.Cmd has no field or method Environ) Signed-off-by: Sebastiaan van Stijn --- cli/command/image/build/internal/git/gitutils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index 6af957c40..c20f8da75 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -213,7 +213,7 @@ func (repo gitRepo) gitWithinDir(dir string, args ...string) ([]byte, error) { cmd := exec.Command("git", args...) cmd.Dir = dir // Disable unsafe remote protocols. - cmd.Env = append(cmd.Environ(), "GIT_PROTOCOL_FROM_USER=0") + cmd.Env = append(os.Environ(), "GIT_PROTOCOL_FROM_USER=0") if repo.isolateConfig { cmd.Env = append(cmd.Env, From 60b326f81496d6fe1f8f3830726609f3cf70048e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 9 Nov 2022 12:27:33 +0100 Subject: [PATCH 30/39] builder/remotecontext/gitutils: switch back to os/exec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a partial revert of 389ada718888631f6ec73e7cb3033b1bf4392ff7, which switched from os/exec to the golang.org/x/sys/execabs package to mitigate security issues (mainly on Windows) with lookups resolving to binaries in the current directory. from the go1.19 release notes https://go.dev/doc/go1.19#os-exec-path > ## PATH lookups > > Command and LookPath no longer allow results from a PATH search to be found > relative to the current directory. This removes a common source of security > problems but may also break existing programs that depend on using, say, > exec.Command("prog") to run a binary named prog (or, on Windows, prog.exe) in > the current directory. See the os/exec package documentation for information > about how best to update such programs. > > On Windows, Command and LookPath now respect the NoDefaultCurrentDirectoryInExePath > environment variable, making it possible to disable the default implicit search > of “.” in PATH lookups on Windows systems. Signed-off-by: Sebastiaan van Stijn --- cli/command/image/build/internal/git/gitutils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index c20f8da75..8ca6918ae 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -4,12 +4,12 @@ import ( "net/http" "net/url" "os" + "os/exec" "path/filepath" "strings" "github.com/moby/sys/symlink" "github.com/pkg/errors" - exec "golang.org/x/sys/execabs" ) type gitRepo struct { From 6291744fa437e98c57da6d63d9194f5879f9868a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 31 Oct 2022 15:21:23 +0100 Subject: [PATCH 31/39] builder/remotecontext/git: use strings.Cut() Signed-off-by: Sebastiaan van Stijn --- .../image/build/internal/git/gitutils.go | 21 ++++++------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index 8ca6918ae..f9b2b4b9c 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -97,15 +97,10 @@ func parseRemoteURL(remoteURL string) (gitRepo, error) { remoteURL = "https://" + remoteURL } - var fragment string if strings.HasPrefix(remoteURL, "git@") { // git@.. is not an URL, so cannot be parsed as URL - parts := strings.SplitN(remoteURL, "#", 2) - - repo.remote = parts[0] - if len(parts) == 2 { - fragment = parts[1] - } + var fragment string + repo.remote, fragment, _ = strings.Cut(remoteURL, "#") repo.ref, repo.subdir = getRefAndSubdir(fragment) } else { u, err := url.Parse(remoteURL) @@ -126,15 +121,11 @@ func parseRemoteURL(remoteURL string) (gitRepo, error) { } func getRefAndSubdir(fragment string) (ref string, subdir string) { - refAndDir := strings.SplitN(fragment, ":", 2) - ref = "master" - if len(refAndDir[0]) != 0 { - ref = refAndDir[0] + ref, subdir, _ = strings.Cut(fragment, ":") + if ref == "" { + ref = "master" } - if len(refAndDir) > 1 && len(refAndDir[1]) != 0 { - subdir = refAndDir[1] - } - return + return ref, subdir } func fetchArgs(remoteURL string, ref string) []string { From 8f865184a62728d66795ad1bf95b63aca6d00d97 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 20 Jan 2022 13:48:28 +0100 Subject: [PATCH 32/39] builder/remotecontext: format code with gofumpt Formatting the code with https://github.com/mvdan/gofumpt Signed-off-by: Sebastiaan van Stijn --- cli/command/image/build/internal/git/gitutils.go | 1 - .../image/build/internal/git/gitutils_test.go | 12 ++++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index f9b2b4b9c..4270e86ef 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -35,7 +35,6 @@ func WithIsolatedConfig(v bool) CloneOption { // will be under "docker-build-git" func Clone(remoteURL string, opts ...CloneOption) (string, error) { repo, err := parseRemoteURL(remoteURL) - if err != nil { return "", err } diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index f3c67b302..e09a7601f 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -239,11 +239,11 @@ func TestCheckoutGit(t *testing.T) { must(gitRepo{}.gitWithinDir(root, "-c", "init.defaultBranch=master", "init", gitDir)) must(gitRepo{}.gitWithinDir(gitDir, "config", "user.email", "test@docker.com")) must(gitRepo{}.gitWithinDir(gitDir, "config", "user.name", "Docker test")) - assert.NilError(t, os.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch"), 0644)) + assert.NilError(t, os.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch"), 0o644)) subDir := filepath.Join(gitDir, "subdir") - assert.NilError(t, os.Mkdir(subDir, 0755)) - assert.NilError(t, os.WriteFile(filepath.Join(subDir, "Dockerfile"), []byte("FROM scratch\nEXPOSE 5000"), 0644)) + assert.NilError(t, os.Mkdir(subDir, 0o755)) + assert.NilError(t, os.WriteFile(filepath.Join(subDir, "Dockerfile"), []byte("FROM scratch\nEXPOSE 5000"), 0o644)) if runtime.GOOS != "windows" { assert.NilError(t, os.Symlink("../subdir", filepath.Join(gitDir, "parentlink"))) @@ -254,8 +254,8 @@ func TestCheckoutGit(t *testing.T) { must(gitRepo{}.gitWithinDir(gitDir, "commit", "-am", "First commit")) must(gitRepo{}.gitWithinDir(gitDir, "checkout", "-b", "test")) - assert.NilError(t, os.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch\nEXPOSE 3000"), 0644)) - assert.NilError(t, os.WriteFile(filepath.Join(subDir, "Dockerfile"), []byte("FROM busybox\nEXPOSE 5000"), 0644)) + assert.NilError(t, os.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte("FROM scratch\nEXPOSE 3000"), 0o644)) + assert.NilError(t, os.WriteFile(filepath.Join(subDir, "Dockerfile"), []byte("FROM busybox\nEXPOSE 5000"), 0o644)) must(gitRepo{}.gitWithinDir(gitDir, "add", "-A")) must(gitRepo{}.gitWithinDir(gitDir, "commit", "-am", "Branch commit")) @@ -267,7 +267,7 @@ func TestCheckoutGit(t *testing.T) { must(gitRepo{}.gitWithinDir(subrepoDir, "config", "user.email", "test@docker.com")) must(gitRepo{}.gitWithinDir(subrepoDir, "config", "user.name", "Docker test")) - assert.NilError(t, os.WriteFile(filepath.Join(subrepoDir, "subfile"), []byte("subcontents"), 0644)) + assert.NilError(t, os.WriteFile(filepath.Join(subrepoDir, "subfile"), []byte("subcontents"), 0o644)) must(gitRepo{}.gitWithinDir(subrepoDir, "add", "-A")) must(gitRepo{}.gitWithinDir(subrepoDir, "commit", "-am", "Subrepo initial")) From 52c62bd13b265c7ac24690080f48717c071a5f77 Mon Sep 17 00:00:00 2001 From: David Dooling Date: Thu, 18 Jan 2024 14:10:15 -0600 Subject: [PATCH 33/39] Fix isGitURL regular expression Escape period (.) so regular expression does not match any character before "git". Signed-off-by: David Dooling --- cli/command/image/build/internal/urlutil/urlutil.go | 2 +- cli/command/image/build/internal/urlutil/urlutil_test.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/cli/command/image/build/internal/urlutil/urlutil.go b/cli/command/image/build/internal/urlutil/urlutil.go index e38988a30..e8459cc82 100644 --- a/cli/command/image/build/internal/urlutil/urlutil.go +++ b/cli/command/image/build/internal/urlutil/urlutil.go @@ -12,7 +12,7 @@ import ( // urlPathWithFragmentSuffix matches fragments to use as Git reference and build // context from the Git repository. See IsGitURL for details. -var urlPathWithFragmentSuffix = regexp.MustCompile(".git(?:#.+)?$") +var urlPathWithFragmentSuffix = regexp.MustCompile(`\.git(?:#.+)?$`) // IsURL returns true if the provided str is an HTTP(S) URL by checking if it // has a http:// or https:// scheme. No validation is performed to verify if the diff --git a/cli/command/image/build/internal/urlutil/urlutil_test.go b/cli/command/image/build/internal/urlutil/urlutil_test.go index 690611832..ed39f5f22 100644 --- a/cli/command/image/build/internal/urlutil/urlutil_test.go +++ b/cli/command/image/build/internal/urlutil/urlutil_test.go @@ -17,6 +17,7 @@ var ( } invalidGitUrls = []string{ "http://github.com/docker/docker.git:#branch", + "https://github.com/docker/dgit", } ) From 45f09a15046c854d81d710c7ba0b19cbca890f7a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 12 Nov 2024 13:01:58 +0100 Subject: [PATCH 34/39] builder/remotecontext/git: remove redundant capturing of loop vars (copyloopvar) builder/remotecontext/git/gitutils_test.go:116:3: The copy of the 'for' variable "tc" can be deleted (Go 1.22+) (copyloopvar) tc := tc ^ Signed-off-by: Sebastiaan van Stijn --- cli/command/image/build/internal/git/gitutils_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index e09a7601f..d8dcdb085 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -113,7 +113,6 @@ func TestParseRemoteURL(t *testing.T) { } for _, tc := range tests { - tc := tc t.Run(tc.doc, func(t *testing.T) { repo, err := parseRemoteURL(tc.url) assert.NilError(t, err) From 8d3c0fb6dcf7f5bb789304a9cb86f8f51bd45734 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Fri, 22 Nov 2024 12:12:30 +1100 Subject: [PATCH 35/39] tests: migrate to assert.ErrorContains when possible If we have an error type that we're checking a substring against, we should really be checking using ErrorContains to indicate the right semantics to assert. Mostly done using these transforms: find . -type f -name "*_test.go" | \ xargs gofmt -w -r 'assert.Assert(t, is.ErrorContains(e, s)) -> assert.ErrorContains(t, e, s)' find . -type f -name "*_test.go" | \ xargs gofmt -w -r 'assert.Assert(t, is.Contains(err.Error(), s)) -> assert.ErrorContains(t, err, s)' find . -type f -name "*_test.go" | \ xargs gofmt -w -r 'assert.Check(t, is.Contains(err.Error(), s)) -> assert.Check(t, is.ErrorContains(err, s))' As well as some small fixups to helpers that were doing strings.Contains explicitly. Signed-off-by: Aleksa Sarai --- cli/command/image/build/internal/git/gitutils_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index d8dcdb085..923a6240e 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -324,7 +324,7 @@ func TestCheckoutGit(t *testing.T) { assert.Check(t, is.Equal("subcontents", string(b))) } else { _, err := os.Stat(filepath.Join(r, "sub/subfile")) - assert.Assert(t, is.ErrorContains(err, "")) + assert.ErrorContains(t, err, "") assert.Assert(t, os.IsNotExist(err)) } @@ -373,6 +373,8 @@ func TestGitInvalidRef(t *testing.T) { for _, url := range gitUrls { _, err := Clone(url) assert.Assert(t, err != nil) + // On Windows, git has different case for the "invalid refspec" error, + // so we can't use ErrorContains. assert.Check(t, is.Contains(strings.ToLower(err.Error()), "invalid refspec")) } } From 75f791d904218089c8a75c5ba0ec5dd0c4da4742 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 15 Jul 2024 16:22:53 +0200 Subject: [PATCH 36/39] builder: use lazyregexp to compile regexes on first use Signed-off-by: Sebastiaan van Stijn --- cli/command/image/build/internal/urlutil/urlutil.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cli/command/image/build/internal/urlutil/urlutil.go b/cli/command/image/build/internal/urlutil/urlutil.go index e8459cc82..34c603cbc 100644 --- a/cli/command/image/build/internal/urlutil/urlutil.go +++ b/cli/command/image/build/internal/urlutil/urlutil.go @@ -6,13 +6,14 @@ package urlutil // import "github.com/docker/docker/builder/remotecontext/urlutil" import ( - "regexp" "strings" + + "github.com/docker/docker/internal/lazyregexp" ) // urlPathWithFragmentSuffix matches fragments to use as Git reference and build // context from the Git repository. See IsGitURL for details. -var urlPathWithFragmentSuffix = regexp.MustCompile(`\.git(?:#.+)?$`) +var urlPathWithFragmentSuffix = lazyregexp.New(`\.git(?:#.+)?$`) // IsURL returns true if the provided str is an HTTP(S) URL by checking if it // has a http:// or https:// scheme. No validation is performed to verify if the From a10a1e619bbbbd82fc784c552e434e5b19593fbc Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 28 Dec 2023 11:58:52 +0100 Subject: [PATCH 37/39] builder/remotecontext: remove unused named and "naked" returns Also renamed some vars for clarity, renamed a error-returns to prevent shadowing, and fixed some linter warnings about unhandled errors. Signed-off-by: Sebastiaan van Stijn --- cli/command/image/build/internal/git/gitutils.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index 4270e86ef..d23ebe337 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -46,7 +46,7 @@ func Clone(remoteURL string, opts ...CloneOption) (string, error) { return repo.clone() } -func (repo gitRepo) clone() (checkoutDir string, err error) { +func (repo gitRepo) clone() (checkoutDir string, retErr error) { fetch := fetchArgs(repo.remote, repo.ref) root, err := os.MkdirTemp("", "docker-build-git") @@ -55,8 +55,8 @@ func (repo gitRepo) clone() (checkoutDir string, err error) { } defer func() { - if err != nil { - os.RemoveAll(root) + if retErr != nil { + _ = os.RemoveAll(root) } }() From 09a3c93f968e9da7de5b28dbc44b1cb5d2f191d7 Mon Sep 17 00:00:00 2001 From: Matthieu MOREL Date: Sun, 27 Apr 2025 08:57:01 +0200 Subject: [PATCH 38/39] =?UTF-8?q?fix(QF1001):=20Apply=20De=20Morgan?= =?UTF-8?q?=E2=80=99s=20law?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Matthieu MOREL Signed-off-by: Sebastiaan van Stijn --- .../image/build/internal/git/gitutils_test.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index 923a6240e..a37e6ad4a 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -216,14 +216,15 @@ func TestCheckoutGit(t *testing.T) { server := httptest.NewServer(&githttp) defer server.Close() - autocrlf := gitGetConfig("core.autocrlf") - if !(autocrlf == "true" || autocrlf == "false" || - autocrlf == "input" || autocrlf == "") { - t.Logf("unknown core.autocrlf value: \"%s\"", autocrlf) - } eol := "\n" - if autocrlf == "true" { + autocrlf := gitGetConfig("core.autocrlf") + switch autocrlf { + case "true": eol = "\r\n" + case "false", "input", "": + // accepted values + default: + t.Logf(`unknown core.autocrlf value: "%s"`, autocrlf) } must := func(out []byte, err error) { From 342f8bca2565fa8227891e7627270b438498afe9 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 30 May 2025 12:27:29 +0200 Subject: [PATCH 39/39] builder: remove // import comments These comments were added to enforce using the correct import path for our packages ("github.com/docker/docker", not "github.com/moby/moby"). However, when working in go module mode (not GOPATH / vendor), they have no effect, so their impact is limited. Remove these imports in preparation of migrating our code to become an actual go module. Signed-off-by: Sebastiaan van Stijn --- cli/command/image/build/internal/git/gitutils.go | 2 +- cli/command/image/build/internal/git/gitutils_test.go | 2 +- cli/command/image/build/internal/urlutil/urlutil.go | 2 +- cli/command/image/build/internal/urlutil/urlutil_test.go | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cli/command/image/build/internal/git/gitutils.go b/cli/command/image/build/internal/git/gitutils.go index d23ebe337..b02993511 100644 --- a/cli/command/image/build/internal/git/gitutils.go +++ b/cli/command/image/build/internal/git/gitutils.go @@ -1,4 +1,4 @@ -package git // import "github.com/docker/docker/builder/remotecontext/git" +package git import ( "net/http" diff --git a/cli/command/image/build/internal/git/gitutils_test.go b/cli/command/image/build/internal/git/gitutils_test.go index a37e6ad4a..204c2477e 100644 --- a/cli/command/image/build/internal/git/gitutils_test.go +++ b/cli/command/image/build/internal/git/gitutils_test.go @@ -1,4 +1,4 @@ -package git // import "github.com/docker/docker/builder/remotecontext/git" +package git import ( "bytes" diff --git a/cli/command/image/build/internal/urlutil/urlutil.go b/cli/command/image/build/internal/urlutil/urlutil.go index 34c603cbc..a2225b596 100644 --- a/cli/command/image/build/internal/urlutil/urlutil.go +++ b/cli/command/image/build/internal/urlutil/urlutil.go @@ -3,7 +3,7 @@ // // This package is specifically written for use with docker build contexts, and // should not be used as a general-purpose utility. -package urlutil // import "github.com/docker/docker/builder/remotecontext/urlutil" +package urlutil import ( "strings" diff --git a/cli/command/image/build/internal/urlutil/urlutil_test.go b/cli/command/image/build/internal/urlutil/urlutil_test.go index ed39f5f22..f6d0a35de 100644 --- a/cli/command/image/build/internal/urlutil/urlutil_test.go +++ b/cli/command/image/build/internal/urlutil/urlutil_test.go @@ -1,4 +1,4 @@ -package urlutil // import "github.com/docker/docker/builder/remotecontext/urlutil" +package urlutil import "testing"