Skip to content

Fix handling of remote "git@" notation#33696

Merged
vdemeester merged 2 commits intomoby:masterfrom
thaJeztah:fix-git-clone
Jun 27, 2017
Merged

Fix handling of remote "git@" notation#33696
vdemeester merged 2 commits intomoby:masterfrom
thaJeztah:fix-git-clone

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Jun 15, 2017

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.

relates to #33686

ping @tonistiigi @ijc PTAL

@thaJeztah
Copy link
Copy Markdown
Member Author

We also need to update https://github.com/docker/cli after this is merged, because the same code is used in the client

Comment thread builder/remotecontext/git/gitutils.go Outdated
Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Jun 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we can probably just add ssh:// here, but let me know what you prefer

Nevermind, that doesn't work of course because some-server:something isn't valid

@ijc
Copy link
Copy Markdown
Contributor

ijc commented Jun 15, 2017

I don't think we should be special casing the user git on a remote system, it could legitimately want to docker build ijc@mygit-server:some/project and AFAICT that would have worked with 1.7.

So it looks like we need a cleverer way to distinguish ssh type targets from URLs. Since this syntactic quirk comes from git perhaps we should follow https://git-scm.com/docs/git-fetch#URLS which says:

An alternative scp-like syntax may also be used with the ssh protocol:

[user@]host.xz:path/to/repo.git/

This syntax is only recognized if there are no slashes before the first colon. This helps differentiate a local path that contains a colon. For example the local path foo:bar could be specified as an absolute path or ./foo:bar to avoid being misinterpreted as an ssh url.

It seems like "This syntax is only recognized if there are no slashes before the first colon." is the helpful part.

@thaJeztah
Copy link
Copy Markdown
Member Author

@ijc the current code special-cases git@ already, so I kept that behaviour. I'm not against making that change (possibly separate from this one?). Alternatively, we would add support for the ssh:// variation, which is more explicit, less "fuzzy", but I saw some discussion about that (git+ssh:// vs bare ssh://)

@thaJeztah
Copy link
Copy Markdown
Member Author

@ecnerwala Go packages that are used in docker/cli are vendored. You can find the exact commit that is used in the vendor.conf file; https://github.com/docker/cli/blob/8c2f81892beb7634a97efeec053f7052c36b4039/vendor.conf#L10

And the vendored files can be found in the vendor directory; https://github.com/docker/cli/tree/master/vendor/github.com/docker/docker

The import is still named docker/docker until migration of Moby is complete and URL's are stable

Comment thread builder/remotecontext/git/gitutils.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsGitTransport already checks for IsURL, so the second check is unnecessary.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, nice catch! I had it slightly different before, and moved this around

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated 👍

Comment thread builder/remotecontext/git/gitutils.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this whole function should be in the urlutil package so that the various hardcoded assumptions made about git@ and other stuff here and in urlutil.IsGitTransport are all in the same place?

Or at least the tail should perhaps be put in a urlutil.ParseGitTransport (to match IsGitTransport)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started making this change, but looking at it, doing so would also require making gitRepo an exported type. I'm not really sure we should make it an official "interface" (type)

I did look at IsGitTransport() and it's only used in this file, so instead, perhaps we should move the function here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diff --git a/builder/remotecontext/git/gitutils.go b/builder/remotecontext/git/gitutils.go
index 91a0f5b..b94d462 100644
--- a/builder/remotecontext/git/gitutils.go
+++ b/builder/remotecontext/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/builder/remotecontext/git/gitutils_test.go b/builder/remotecontext/git/gitutils_test.go
index b6523a2..c638a49 100644
--- a/builder/remotecontext/git/gitutils_test.go
+++ b/builder/remotecontext/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",
+		"[email protected]:docker/docker.git",
+		"[email protected]: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)
+		}
+	}
+}
diff --git a/pkg/urlutil/urlutil.go b/pkg/urlutil/urlutil.go
index 4415287..cfcd582 100644
--- a/pkg/urlutil/urlutil.go
+++ b/pkg/urlutil/urlutil.go
@@ -29,12 +29,6 @@ func IsGitURL(str string) bool {
 	return checkURL(str, "git")
 }
 
-// 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 IsURL(str) || strings.HasPrefix(str, "git://") || strings.HasPrefix(str, "git@")
-}
-
 // IsTransportURL returns true if the provided str is a transport (tcp, tcp+tls, udp, unix) URL.
 func IsTransportURL(str string) bool {
 	return checkURL(str, "transport")
diff --git a/pkg/urlutil/urlutil_test.go b/pkg/urlutil/urlutil_test.go
index d84145a..e7579f5 100644
--- a/pkg/urlutil/urlutil_test.go
+++ b/pkg/urlutil/urlutil_test.go
@@ -27,20 +27,6 @@ var (
 	}
 )
 
-func TestValidGitTransport(t *testing.T) {
-	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)
-		}
-	}
-}
-
 func TestIsGIT(t *testing.T) {
 	for _, url := range gitUrls {
 		if !IsGitURL(url) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did look at IsGitTransport() and it's only used in this file, so instead, perhaps we should move the function here?

That sounds reasonable (as does the diff snippet above as far as I can tell through the comment rendering).

`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 <[email protected]>
This function was only used inside gitutils,
and is written specifically for the requirements
there.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Copy Markdown
Member Author

@ijc pushed the changes as a second commit, PTAL

@tonistiigi
Copy link
Copy Markdown
Member

LGTM

Copy link
Copy Markdown
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🦁

@vdemeester vdemeester merged commit 151c5ea into moby:master Jun 27, 2017
@thaJeztah thaJeztah deleted the fix-git-clone branch June 27, 2017 20:46
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Jul 2, 2017
Includes changes from;

- Fix handling of remote "git@" notation (moby/moby#33696)
- Move some `api` package functions away moby/moby#33798 (related to docker#236)
- Update go-connections dependency moby/moby#33814 (already vendored in docker#238)
- Set ping version even on error (moby/moby#33827)
- Do not add duplicate platform information to service spec (moby/moby#33867)
- Refactor MountPoint Setup function in volume.go (moby/moby#33890)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Jul 2, 2017
Includes changes from;

- Add a LastTagTime for images (moby/moby#31497)
- Fix handling of remote "git@" notation (moby/moby#33696)
- Move some `api` package functions away (moby/moby#33798) (related to docker#236)
- Set ping version even on error (moby/moby#33827)
- Do not add duplicate platform information to service spec (moby/moby#33867)
- Refactor MountPoint Setup function in volume.go (moby/moby#33890)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Jul 3, 2017
Includes changes from;

- Add a LastTagTime for images (moby/moby#31497)
- Fix handling of remote "git@" notation (moby/moby#33696)
- Move some `api` package functions away (moby/moby#33798) (related to docker#236)
- Set ping version even on error (moby/moby#33827)
- Do not add duplicate platform information to service spec (moby/moby#33867)
- Refactor MountPoint Setup function in volume.go (moby/moby#33890)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Jul 5, 2017
Includes changes from;

- Add a LastTagTime for images (moby/moby#31497)
- Fix handling of remote "git@" notation (moby/moby#33696)
- Move some `api` package functions away (moby/moby#33798) (related to docker#236)
- Set ping version even on error (moby/moby#33827)
- Do not add duplicate platform information to service spec (moby/moby#33867)
- Refactor MountPoint Setup function in volume.go (moby/moby#33890)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
andrewhsu pushed a commit to docker-archive/docker-ce that referenced this pull request Jul 14, 2017
Includes changes from;

- Add a LastTagTime for images (moby/moby#31497)
- Fix handling of remote "git@" notation (moby/moby#33696)
- Move some `api` package functions away (moby/moby#33798) (related to docker/cli#236)
- Set ping version even on error (moby/moby#33827)
- Do not add duplicate platform information to service spec (moby/moby#33867)
- Refactor MountPoint Setup function in volume.go (moby/moby#33890)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Upstream-commit: 366d3ec971d8007c667e8d7dc8e35a346fb19539
Component: cli
alshabib pushed a commit to alshabib/cli that referenced this pull request Aug 1, 2017
Includes changes from;

- Add a LastTagTime for images (moby/moby#31497)
- Fix handling of remote "git@" notation (moby/moby#33696)
- Move some `api` package functions away (moby/moby#33798) (related to docker#236)
- Set ping version even on error (moby/moby#33827)
- Do not add duplicate platform information to service spec (moby/moby#33867)
- Refactor MountPoint Setup function in volume.go (moby/moby#33890)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants