Repository: rancher/fleet
Pull request #3430 (2025-03-07 13:01:50)

Update gitcloner/fetcher error messages for SCP/SSH GitRepo URLs
Improve error messages when handling "scp-style" Git URLs: https://github.com/rancher/fleet/issues/2495 

*  ssh/scp style URLs only work when clientSecretName is used
  * cloner (job) works with scp-style URLs
  * fetcher (controller) works with scp-style URLs*
* made error messages in cloner more distinct

Note: The new require-secrets e2e tests needs `GIT_REPO_URL` to contain a scp-style URL, like `git@gitrepo.host:path`. Other e2e tests cannot be used, since our server runs on a non-standard port and "scp" syntax doesn't support ports...

(Updated description after comments)

manno: > This could be added to `e2e/require-secrets` to work with Github.

Thanks, that helped.

* ssh/scp style URLs only work when `clientSecretName` is used
  * cloner (job) works with scp-style URLs
  * fetcher (controller) works with scp-style URLs
* made error messages in cloner more distinct

Patch:
diff --git a/internal/cmd/cli/gitcloner/cloner.go b/internal/cmd/cli/gitcloner/cloner.go
index 78286b8f..3744de69 100644
--- a/internal/cmd/cli/gitcloner/cloner.go
+++ b/internal/cmd/cli/gitcloner/cloner.go
@@ -3,6 +3,7 @@ package gitcloner
 import (
 	"fmt"
 	"os"
+	"strings"
 
 	"github.com/go-git/go-git/v5"
 	"github.com/go-git/go-git/v5/plumbing"
@@ -38,6 +39,16 @@ func New() *Cloner {
 }
 
 func (c *Cloner) CloneRepo(opts *GitCloner) error {
+	url, err := giturls.Parse(opts.Repo)
+	if err != nil {
+		return fmt.Errorf("failed to parse git URL: %w", err)
+	}
+	if strings.HasPrefix(url.String(), "ssh://") {
+		if opts.SSHPrivateKeyFile == "" {
+			return fmt.Errorf("SSH private key file is required for SSH/SCP-style URLs: %s", url)
+		}
+	}
+
 	// Azure DevOps requires capabilities multi_ack / multi_ack_detailed,
 	// which are not fully implemented and by default are included in
 	// transport.UnsupportedCapabilities.
@@ -49,11 +60,11 @@ func (c *Cloner) CloneRepo(opts *GitCloner) error {
 	}
 	auth, err := createAuthFromOpts(opts)
 	if err != nil {
-		return fmt.Errorf("%s: %w", gitClonerErrorContext(opts), err)
+		return fmt.Errorf("failed to create auth from options for %s: %w", repo(opts), err)
 	}
 	caBundle, err := getCABundleFromFile(opts.CABundleFile)
 	if err != nil {
-		return fmt.Errorf("%s: %w", gitClonerErrorContext(opts), err)
+		return fmt.Errorf("failed to read CA bundle from file for %s: %w", repo(opts), err)
 	}
 
 	if opts.Branch == "" && opts.Revision == "" {
@@ -83,7 +94,7 @@ func cloneBranch(opts *GitCloner, auth transport.AuthMethod, caBundle []byte) er
 	})
 
 	if err != nil {
-		return fmt.Errorf("%s: %w", gitClonerErrorContext(opts), err)
+		return fmt.Errorf("failed to clone repo from branch %s: %w", repo(opts), err)
 	}
 	return nil
 }
@@ -97,19 +108,19 @@ func cloneRevision(opts *GitCloner, auth transport.AuthMethod, caBundle []byte)
 		RecurseSubmodules: git.DefaultSubmoduleRecursionDepth,
 	})
 	if err != nil {
-		return fmt.Errorf("%s: %w", gitClonerErrorContext(opts), err)
+		return fmt.Errorf("failed to clone repo from revision %s: %w", repo(opts), err)
 	}
 	h, err := r.ResolveRevision(plumbing.Revision(opts.Revision))
 	if err != nil {
-		return fmt.Errorf("%s: %w", gitClonerErrorContext(opts), err)
+		return fmt.Errorf("failed to resolve revision %s: %w", repo(opts), err)
 	}
 	w, err := r.Worktree()
 	if err != nil {
-		return fmt.Errorf("%s: %w", gitClonerErrorContext(opts), err)
+		return fmt.Errorf("failed to get filesystem worktree for %s: %w", repo(opts), err)
 	}
 
 	if err := w.Checkout(&git.CheckoutOptions{Hash: *h}); err != nil {
-		return fmt.Errorf("%s: %w", gitClonerErrorContext(opts), err)
+		return fmt.Errorf("failed to checkout in worktree %s: %w", repo(opts), err)
 	}
 
 	return nil
@@ -188,12 +199,6 @@ func createKnownHostsCallBack(knownHosts []byte) (ssh.HostKeyCallback, error) {
 	return gossh.NewKnownHostsCallback(f.Name())
 }
 
-func gitClonerErrorContext(opts *GitCloner) string {
-	return fmt.Sprintf(
-		"failed checkout of: repo=[%s] branch=[%s] revision=[%s] path=[%s]",
-		opts.Repo,
-		opts.Branch,
-		opts.Revision,
-		opts.Path,
-	)
+func repo(opts *GitCloner) string {
+	return fmt.Sprintf("repo=%q branch=%q revision=%q path=%q", opts.Repo, opts.Branch, opts.Revision, opts.Path)
 }
diff --git a/pkg/git/remote.go b/pkg/git/remote.go
index 3f00c97f..75fde074 100644
--- a/pkg/git/remote.go
+++ b/pkg/git/remote.go
@@ -2,6 +2,7 @@ package git
 
 import (
 	"fmt"
+	"strings"
 	"time"
 
 	gogit "github.com/go-git/go-git/v5"
@@ -9,6 +10,7 @@ import (
 	"github.com/go-git/go-git/v5/plumbing/transport"
 	"github.com/go-git/go-git/v5/storage/memory"
 	"github.com/go-logr/logr"
+	giturls "github.com/rancher/fleet/pkg/git-urls"
 	corev1 "k8s.io/api/core/v1"
 )
 
@@ -74,6 +76,17 @@ func NewRemote(url string, opts *options) (*Remote, error) {
 	if err != nil {
 		return nil, err
 	}
+
+	// parse the url to make sure it is valid and convert scp-like urls to ssh
+	u, err := giturls.Parse(url)
+	if err != nil {
+		return nil, err
+	}
+
+	if auth == nil && strings.HasPrefix(u.String(), "ssh://") {
+		return nil, fmt.Errorf("SSH private key file is required for SSH/SCP-style URLs: %s", url)
+	}
+
 	return &Remote{
 		URL:     url,
 		Options: opts,


Test patch:
diff --git a/e2e/require-secrets/gitrepo_test.go b/e2e/require-secrets/gitrepo_test.go
index 53ed7b5e..4214eb2f 100644
--- a/e2e/require-secrets/gitrepo_test.go
+++ b/e2e/require-secrets/gitrepo_test.go
@@ -27,52 +27,92 @@ var _ = Describe("Git Repo", func() {
 		repo    *git.Repository
 	)
 
-	replace := func(path string, s string, r string) {
-		b, err := os.ReadFile(path)
-		Expect(err).ToNot(HaveOccurred())
-
-		b = bytes.ReplaceAll(b, []byte(s), []byte(r))
-
-		err = os.WriteFile(path, b, 0644)
-		Expect(err).ToNot(HaveOccurred())
-	}
-
-	BeforeEach(func() {
-		k = env.Kubectl.Namespace(env.Namespace)
-		gh = githelper.NewSSH()
-
-		out, err := k.Create(
-			"secret", "generic", "git-auth", "--type", "kubernetes.io/ssh-auth",
-			"--from-file=ssh-privatekey="+os.Getenv("GIT_SSH_KEY"),
-			"--from-file=ssh-publickey="+os.Getenv("GIT_SSH_PUBKEY"),
-		)
-		Expect(err).ToNot(HaveOccurred(), out)
-
-		err = testenv.ApplyTemplate(k, testenv.AssetPath("gitrepo/gitrepo_with_auth.yaml"), struct {
-			Repo            string
-			Branch          string
-			PollingInterval string
-		}{
-			gh.GetURL(),
-			gh.Branch,
-			"15s", // default
+	When("using scp syntax for the gitrepo URL", func() {
+		BeforeEach(func() {
+			k = env.Kubectl.Namespace(env.Namespace)
+
+			out, err := k.Create(
+				"secret", "generic", "git-auth", "--type", "kubernetes.io/ssh-auth",
+				"--from-file=ssh-privatekey="+os.Getenv("GIT_SSH_KEY"),
+				"--from-file=ssh-publickey="+os.Getenv("GIT_SSH_PUBKEY"),
+			)
+			Expect(err).ToNot(HaveOccurred(), out)
+
+			url := os.Getenv("GIT_REPO_URL")
+			Expect(url).To(HavePrefix("git@"))
+
+			err = testenv.ApplyTemplate(k, testenv.AssetPath("gitrepo/gitrepo_with_auth.yaml"), struct {
+				Repo            string
+				Branch          string
+				PollingInterval string
+			}{
+				url,
+				"main",
+				"15s", // default
+			})
+			Expect(err).ToNot(HaveOccurred(), out)
+
+			DeferCleanup(func() {
+				_, _ = k.Delete("gitrepo", "gitrepo-test")
+				_, _ = k.Delete("secret", "git-auth")
+			})
 		})
-		Expect(err).ToNot(HaveOccurred(), out)
 
-		tmpdir, _ = os.MkdirTemp("", "fleet-")
-		repodir = path.Join(tmpdir, "repo")
-		repo, err = gh.Create(repodir, testenv.AssetPath("gitrepo/sleeper-chart"), "examples")
-		Expect(err).ToNot(HaveOccurred())
-
-	})
-
-	AfterEach(func() {
-		os.RemoveAll(tmpdir)
-		_, _ = k.Delete("secret", "git-auth")
-		_, _ = k.Delete("gitrepo", "gitrepo-test")
+		It("clones the repo", func() {
+			By("checking just the clone was attempted and successful")
+			Eventually(func() string {
+				out, _ := k.Get("gitrepo", "gitrepo-test", `-o=jsonpath={.status.conditions[?(@.type=="Stalled")].message}`)
+				return out
+			}).Should(ContainSubstring("no resource found at the following paths to deploy"))
+		})
 	})
 
 	When("updating a git repository", func() {
+		replace := func(path string, s string, r string) {
+			b, err := os.ReadFile(path)
+			Expect(err).ToNot(HaveOccurred())
+
+			b = bytes.ReplaceAll(b, []byte(s), []byte(r))
+
+			err = os.WriteFile(path, b, 0644)
+			Expect(err).ToNot(HaveOccurred())
+		}
+
+		BeforeEach(func() {
+			k = env.Kubectl.Namespace(env.Namespace)
+			gh = githelper.NewSSH()
+
+			out, err := k.Create(
+				"secret", "generic", "git-auth", "--type", "kubernetes.io/ssh-auth",
+				"--from-file=ssh-privatekey="+os.Getenv("GIT_SSH_KEY"),
+				"--from-file=ssh-publickey="+os.Getenv("GIT_SSH_PUBKEY"),
+			)
+			Expect(err).ToNot(HaveOccurred(), out)
+
+			err = testenv.ApplyTemplate(k, testenv.AssetPath("gitrepo/gitrepo_with_auth.yaml"), struct {
+				Repo            string
+				Branch          string
+				PollingInterval string
+			}{
+				gh.GetURL(),
+				gh.Branch,
+				"15s", // default
+			})
+			Expect(err).ToNot(HaveOccurred(), out)
+
+			tmpdir, _ = os.MkdirTemp("", "fleet-")
+			repodir = path.Join(tmpdir, "repo")
+			repo, err = gh.Create(repodir, testenv.AssetPath("gitrepo/sleeper-chart"), "examples")
+			Expect(err).ToNot(HaveOccurred())
+
+		})
+
+		AfterEach(func() {
+			os.RemoveAll(tmpdir)
+			_, _ = k.Delete("secret", "git-auth")
+			_, _ = k.Delete("gitrepo", "gitrepo-test")
+		})
+
 		It("updates the deployment", func() {
 			By("checking the pod exists")
 			Eventually(func() string {
diff --git a/internal/cmd/cli/gitcloner/cloner_test.go b/internal/cmd/cli/gitcloner/cloner_test.go
index c268832e..8a4baadf 100644
--- a/internal/cmd/cli/gitcloner/cloner_test.go
+++ b/internal/cmd/cli/gitcloner/cloner_test.go
@@ -75,12 +75,12 @@ udiSlDctMM/X3ZM2JN5M1rtAJ2WR3ZQtmWbOjZAbG2Eq
 	}{
 		"branch no auth": {
 			opts: &GitCloner{
-				Repo:   "repo",
+				Repo:   "https://repo",
 				Path:   "path",
 				Branch: "master",
 			},
 			expectedCloneOpts: &git.CloneOptions{
-				URL:               "repo",
+				URL:               "https://repo",
 				SingleBranch:      true,
 				ReferenceName:     "master",
 				RecurseSubmodules: git.DefaultSubmoduleRecursionDepth,
@@ -88,14 +88,14 @@ udiSlDctMM/X3ZM2JN5M1rtAJ2WR3ZQtmWbOjZAbG2Eq
 		},
 		"branch basic auth": {
 			opts: &GitCloner{
-				Repo:         "repo",
+				Repo:         "https://repo",
 				Path:         "path",
 				Branch:       "master",
 				Username:     "user",
 				PasswordFile: passwordFile,
 			},
 			expectedCloneOpts: &git.CloneOptions{
-				URL:           "repo",
+				URL:           "https://repo",
 				SingleBranch:  true,
 				ReferenceName: "master",
 				Auth: &httpgit.BasicAuth{
@@ -122,31 +122,31 @@ udiSlDctMM/X3ZM2JN5M1rtAJ2WR3ZQtmWbOjZAbG2Eq
 		},
 		"password file does not exist": {
 			opts: &GitCloner{
-				Repo:         "repo",
+				Repo:         "https://repo",
 				Branch:       "master",
 				PasswordFile: "doesntexist",
 				Username:     "user",
 			},
 			expectedCloneOpts: nil,
-			expectedErr:       errors.New("failed checkout of: repo=[repo] branch=[master] revision=[] path=[]: file not found"),
+			expectedErr:       errors.New(`failed to create auth from options for repo="https://repo" branch="master" revision="" path="": file not found`),
 		},
 		"ca file does not exist": {
 			opts: &GitCloner{
-				Repo:         "repo",
+				Repo:         "https://repo",
 				Branch:       "master",
 				CABundleFile: "doesntexist",
 			},
 			expectedCloneOpts: nil,
-			expectedErr:       errors.New("failed checkout of: repo=[repo] branch=[master] revision=[] path=[]: file not found"),
+			expectedErr:       errors.New(`failed to read CA bundle from file for repo="https://repo" branch="master" revision="" path="": file not found`),
 		},
 		"ssh private key file does not exist": {
 			opts: &GitCloner{
-				Repo:              "repo",
+				Repo:              "https://repo",
 				Branch:            "master",
 				SSHPrivateKeyFile: "doesntexist",
 			},
 			expectedCloneOpts: nil,
-			expectedErr:       errors.New("failed checkout of: repo=[repo] branch=[master] revision=[] path=[]: file not found"),
+			expectedErr:       errors.New(`failed to create auth from options for repo="https://repo" branch="master" revision="" path="": file not found`),
 		},
 	}
 
diff --git a/pkg/git-urls/urls_test.go b/pkg/git-urls/urls_test.go
new file mode 100644
index 00000000..de69d462
--- /dev/null
+++ b/pkg/git-urls/urls_test.go
@@ -0,0 +1,59 @@
+package giturls_test
+
+import (
+	"testing"
+
+	giturls "github.com/rancher/fleet/pkg/git-urls"
+)
+
+func TestParse(t *testing.T) {
+	tests := []struct {
+		name string
+		url  string
+		want string
+	}{
+		{
+			name: "ssh",
+			url:  "ssh://example.com/foo/bar",
+			want: "ssh://example.com/foo/bar",
+		},
+		{
+			name: "git",
+			url:  "git://example.com/foo/bar",
+			want: "git://example.com/foo/bar",
+		},
+		{
+			name: "git+ssh",
+			url:  "git+ssh://example.com/foo/bar",
+			want: "git+ssh://example.com/foo/bar",
+		},
+		{
+			name: "http",
+			url:  "http://example.com/foo/bar",
+			want: "http://example.com/foo/bar",
+		},
+		{
+			name: "https",
+			url:  "https://example.com/foo/bar",
+
+			want: "https://example.com/foo/bar",
+		},
+		{
+			name: "scp",
+			url:  "git@example.com:foo/bar",
+			want: "ssh://git@example.com/foo/bar",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			u, err := giturls.Parse(tt.url)
+			if err != nil {
+				t.Errorf("unexpected error: %v", err)
+			}
+			if u.String() != tt.want {
+				t.Errorf("got %q, want %q", u.String(), tt.want)
+			}
+		})
+	}
+}