git: update parsing to clarify between scp-style urls

This should also resolve the ports parsing issue. Co-authored-by: Aaron Lehmann <[email protected]> Signed-off-by: Justin Chadwell <[email protected]>
jedevc · Aug 22, 2023 · 50e75e3 · 50e75e3
1 parent e84cc9e
commit 50e75e3
Show file tree

Hide file tree

Showing 9 changed files with 276 additions and 173 deletions.
diff --git a/client/llb/source.go b/client/llb/source.go
@@ -5,6 +5,7 @@ import (
 	_ "crypto/sha256" // for opencontainers/go-digest
 	"encoding/json"
 	"os"
+	"path"
 	"strconv"
 	"strings"
 
@@ -237,29 +238,30 @@ type ImageInfo struct {
 //
 // By default the git repository is cloned with `--depth=1` to reduce the amount of data downloaded.
 // Additionally the ".git" directory is removed after the clone, you can keep ith with the [KeepGitDir] [GitOption].
-func Git(remote, ref string, opts ...GitOption) State {
-	url := strings.Split(remote, "#")[0]
-
-	var protocolType int
-	remote, protocolType = gitutil.ParseProtocol(remote)
-
-	var sshHost string
-	if protocolType == gitutil.SSHProtocol {
-		parts := strings.SplitN(remote, ":", 2)
-		if len(parts) == 2 {
-			sshHost = parts[0]
-			// keep remote consistent with http(s) version
-			remote = parts[0] + "/" + parts[1]
-		}
-	}
-	if protocolType == gitutil.UnknownProtocol {
+func Git(url, ref string, opts ...GitOption) State {
+	remote, err := gitutil.ParseURL(url)
+	if errors.Is(err, gitutil.ErrUnknownProtocol) {
 		url = "https://" + url
+		remote, err = gitutil.ParseURL(url)
+	}
+	if remote != nil {
+		remote.Fragment = ""
+		url = remote.String()
 	}
 
-	id := remote
-
-	if ref != "" {
-		id += "#" + ref
+	var id string
+	if err != nil {
+		// If we can't parse the URL, just use the full URL as the ID. The git
+		// operation will fail later on.
+		id = url
+	} else {
+		// We construct the ID manually here, so that we can create the same ID
+		// for different protocols (e.g. https and ssh) that have the same
+		// host/path/fragment combination.
+		id = remote.Host + path.Join("/", remote.Path)
+		if ref != "" {
+			id += "#" + ref
+		}
 	}
 
 	gi := &GitInfo{
@@ -290,11 +292,11 @@ func Git(remote, ref string, opts ...GitOption) State {
 			addCap(&gi.Constraints, pb.CapSourceGitHTTPAuth)
 		}
 	}
-	if protocolType == gitutil.SSHProtocol {
+	if remote != nil && remote.Scheme == gitutil.SSHProtocol {
 		if gi.KnownSSHHosts != "" {
 			attrs[pb.AttrKnownSSHHosts] = gi.KnownSSHHosts
-		} else if sshHost != "" {
-			keyscan, err := sshutil.SSHKeyScan(sshHost)
+		} else {
+			keyscan, err := sshutil.SSHKeyScan(remote.Host)
 			if err == nil {
 				// best effort
 				attrs[pb.AttrKnownSSHHosts] = keyscan

diff --git a/source/git/identifier.go b/source/git/identifier.go
@@ -1,13 +1,13 @@
 package git
 
 import (
-	"net/url"
 	"path"
 	"strings"
 
 	"github.com/moby/buildkit/solver/llbsolver/provenance"
 	"github.com/moby/buildkit/source"
 	srctypes "github.com/moby/buildkit/source/types"
+	"github.com/moby/buildkit/util/gitutil"
 	"github.com/moby/buildkit/util/sshutil"
 )
 
@@ -23,32 +23,19 @@ type GitIdentifier struct {
 }
 
 func NewGitIdentifier(remoteURL string) (*GitIdentifier, error) {
-	repo := GitIdentifier{}
-
 	if !isGitTransport(remoteURL) {
 		remoteURL = "https://" + remoteURL
 	}
+	u, err := gitutil.ParseURL(remoteURL)
+	if err != nil {
+		return nil, err
+	}
 
-	var fragment string
-	if sshutil.IsImplicitSSHTransport(remoteURL) {
-		// implicit ssh urls such as "git@.." are not actually a 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 nil, err
-		}
+	repo := GitIdentifier{}
+	repo.Ref, repo.Subdir = gitutil.SplitGitFragment(u.Fragment)
+	u.Fragment = ""
+	repo.Remote = u.String()
 
-		repo.Ref, repo.Subdir = getRefAndSubdir(u.Fragment)
-		u.Fragment = ""
-		repo.Remote = u.String()
-	}
 	if sd := path.Clean(repo.Subdir); sd == "/" || sd == "." {
 		repo.Subdir = ""
 	}
@@ -96,15 +83,3 @@ func (id *GitIdentifier) Capture(c *provenance.Capture, pin string) error {
 func isGitTransport(str string) bool {
 	return strings.HasPrefix(str, "http://") || strings.HasPrefix(str, "https://") || strings.HasPrefix(str, "git://") || strings.HasPrefix(str, "ssh://") || sshutil.IsImplicitSSHTransport(str)
 }
-
-func getRefAndSubdir(fragment string) (ref string, subdir string) {
-	refAndDir := strings.SplitN(fragment, ":", 2)
-	ref = ""
-	if len(refAndDir[0]) != 0 {
-		ref = refAndDir[0]
-	}
-	if len(refAndDir) > 1 && len(refAndDir[1]) != 0 {
-		subdir = refAndDir[1]
-	}
-	return
-}
diff --git a/source/git/identifier_test.go b/source/git/identifier_test.go
@@ -27,7 +27,7 @@ func TestNewGitIdentifier(t *testing.T) {
 		{
 			url: "[email protected]:moby/buildkit.git",
 			expected: GitIdentifier{
-				Remote: "[email protected]:moby/buildkit.git",
+				Remote: "ssh://[email protected]/moby/buildkit.git",
 			},
 		},
 		{
@@ -75,13 +75,13 @@ func TestNewGitIdentifier(t *testing.T) {
 		{
 			url: "[email protected]:user/repo.git",
 			expected: GitIdentifier{
-				Remote: "[email protected]:user/repo.git",
+				Remote: "ssh://[email protected]/user/repo.git",
 			},
 		},
 		{
 			url: "[email protected]:user/repo.git#mybranch:mydir/mysubdir/",
 			expected: GitIdentifier{
-				Remote: "[email protected]:user/repo.git",
+				Remote: "ssh://[email protected]/user/repo.git",
 				Ref:    "mybranch",
 				Subdir: "mydir/mysubdir/",
 			},

diff --git a/util/gitutil/git_protocol.go b/util/gitutil/git_protocol.go
diff --git a/util/gitutil/git_protocol_test.go b/util/gitutil/git_protocol_test.go
diff --git a/util/gitutil/git_ref.go b/util/gitutil/git_ref.go
@@ -1,10 +1,12 @@
 package gitutil
 
 import (
+	"net/url"
 	"regexp"
 	"strings"
 
 	"github.com/containerd/containerd/errdefs"
+	"github.com/pkg/errors"
 )
 
 // GitRef represents a git ref.
@@ -51,19 +53,33 @@ type GitRef struct {
 func ParseGitRef(ref string) (*GitRef, error) {
 	res := &GitRef{}
 
+	var (
+		remote *url.URL
+		err    error
+	)
+
 	if strings.HasPrefix(ref, "github.com/") {
 		res.IndistinguishableFromLocal = true // Deprecated
+		remote = &url.URL{
+			Scheme: "https",
+			Host:   "github.com",
+			Path:   strings.TrimPrefix(ref, "github.com/"),
+		}
 	} else {
-		_, proto := ParseProtocol(ref)
-		switch proto {
-		case UnknownProtocol:
-			return nil, errdefs.ErrInvalidArgument
+		remote, err = ParseURL(ref)
+		if errors.Is(err, ErrUnknownProtocol) {
+			remote, err = ParseURL("https://" + ref)
 		}
-		switch proto {
+		if err != nil {
+			return nil, err
+		}
+
+		switch remote.Scheme {
 		case HTTPProtocol, GitProtocol:
 			res.UnencryptedTCP = true // Discouraged, but not deprecated
 		}
-		switch proto {
+
+		switch remote.Scheme {
 		// An HTTP(S) URL is considered to be a valid git ref only when it has the ".git[...]" suffix.
 		case HTTPProtocol, HTTPSProtocol:
 			var gitURLPathWithFragmentSuffix = regexp.MustCompile(`\.git(?:#.+)?$`)
@@ -73,13 +89,16 @@ func ParseGitRef(ref string) (*GitRef, error) {
 		}
 	}
 
-	var fragment string
-	res.Remote, fragment, _ = strings.Cut(ref, "#")
-	if len(res.Remote) == 0 {
-		return res, errdefs.ErrInvalidArgument
+	res.Commit, res.SubDir = SplitGitFragment(remote.Fragment)
+	remote.Fragment = ""
+
+	res.Remote = remote.String()
+	if res.IndistinguishableFromLocal {
+		_, res.Remote, _ = strings.Cut(res.Remote, "://")
 	}
-	res.Commit, res.SubDir, _ = strings.Cut(fragment, ":")
+
 	repoSplitBySlash := strings.Split(res.Remote, "/")
 	res.ShortName = strings.TrimSuffix(repoSplitBySlash[len(repoSplitBySlash)-1], ".git")
+
 	return res, nil
 }
diff --git a/util/gitutil/git_ref_test.go b/util/gitutil/git_ref_test.go
@@ -66,6 +66,13 @@ func TestParseGitRef(t *testing.T) {
 				IndistinguishableFromLocal: true,
 			},
 		},
+		{
+			ref: "custom.xyz/moby/buildkit.git",
+			expected: &GitRef{
+				Remote:    "https://custom.xyz/moby/buildkit.git",
+				ShortName: "buildkit",
+			},
+		},
 		{
 			ref:      "https://github.com/moby/buildkit",
 			expected: nil,
@@ -77,24 +84,31 @@ func TestParseGitRef(t *testing.T) {
 				ShortName: "buildkit",
 			},
 		},
+		{
+			ref: "https://foo:[email protected]/moby/buildkit.git",
+			expected: &GitRef{
+				Remote:    "https://foo:[email protected]/moby/buildkit.git",
+				ShortName: "buildkit",
+			},
+		},
 		{
 			ref: "[email protected]:moby/buildkit",
 			expected: &GitRef{
-				Remote:    "[email protected]:moby/buildkit",
+				Remote:    "ssh://[email protected]/moby/buildkit",
 				ShortName: "buildkit",
 			},
 		},
 		{
 			ref: "[email protected]:moby/buildkit.git",
 			expected: &GitRef{
-				Remote:    "[email protected]:moby/buildkit.git",
+				Remote:    "ssh://[email protected]/moby/buildkit.git",
 				ShortName: "buildkit",
 			},
 		},
 		{
 			ref: "[email protected]:atlassianlabs/atlassian-docker.git",
 			expected: &GitRef{
-				Remote:    "[email protected]:atlassianlabs/atlassian-docker.git",
+				Remote:    "ssh://[email protected]/atlassianlabs/atlassian-docker.git",
 				ShortName: "atlassian-docker",
 			},
 		},