Merge branch 'smh-fix-update-remote-mirror' into 'master'

Set SSH credentials when listing remote repos references in UpdateRemoteMirror

Closes gitlab#332861

See merge request gitlab-org/gitaly!3579

5 files changed, 91 insertions, 40 deletions

diff --git a/internal/git/localrepo/refs.go b/internal/git/localrepo/refs.go
index d4125f7e5..12f4c7ef7 100644
--- a/internal/git/localrepo/refs.go
+++ b/internal/git/localrepo/refs.go
@@ -161,8 +161,9 @@ func (repo *Repo) UpdateRef(ctx context.Context, reference git.ReferenceName, ne
 }
 
 type getRemoteReferenceConfig struct {
-	patterns []string
-	config   []git.ConfigPair
+	patterns   []string
+	config     []git.ConfigPair
+	sshCommand string
 }
 
 // GetRemoteReferencesOption is an option which can be passed to GetRemoteReferences.
@@ -176,6 +177,13 @@ func WithPatterns(patterns ...string) GetRemoteReferencesOption {
 	}
 }
 
+// WithSSHCommand sets the SSH invocation to use when communicating with the remote.
+func WithSSHCommand(cmd string) GetRemoteReferencesOption {
+	return func(cfg *getRemoteReferenceConfig) {
+		cfg.sshCommand = cmd
+	}
+}
+
 // WithConfig sets up Git configuration for the git-ls-remote(1) invocation. The config pairs are
 // set up via `WithConfigEnv()` and are thus not exposed via the command line.
 func WithConfig(config ...git.ConfigPair) GetRemoteReferencesOption {
@@ -192,6 +200,11 @@ func (repo *Repo) GetRemoteReferences(ctx context.Context, remote string, opts .
 		opt(&cfg)
 	}
 
+	var env []string
+	if cfg.sshCommand != "" {
+		env = append(env, envGitSSHCommand(cfg.sshCommand))
+	}
+
 	stdout := &bytes.Buffer{}
 	stderr := &bytes.Buffer{}
 	if err := repo.ExecAndWait(ctx,
@@ -204,6 +217,7 @@ func (repo *Repo) GetRemoteReferences(ctx context.Context, remote string, opts .
 		},
 		git.WithStdout(stdout),
 		git.WithStderr(stderr),
+		git.WithEnv(env...),
 		git.WithConfigEnv(cfg.config...),
 	); err != nil {
 		return nil, fmt.Errorf("create git ls-remote: %w, stderr: %q", err, stderr)
diff --git a/internal/git/localrepo/refs_test.go b/internal/git/localrepo/refs_test.go
index ce4074f20..b4a7c449b 100644
--- a/internal/git/localrepo/refs_test.go
+++ b/internal/git/localrepo/refs_test.go
@@ -1,6 +1,8 @@
 package localrepo
 
 import (
+	"io/ioutil"
+	"os"
 	"path/filepath"
 	"testing"
 
@@ -215,6 +217,9 @@ func TestRepo_GetRemoteReferences(t *testing.T) {
 
 	cfg := testcfg.Build(t)
 
+	wrappedGit, gitSSHCommandFile := captureGitSSHCommand(t, cfg.Git.BinPath)
+	cfg.Git.BinPath = wrappedGit
+
 	storagePath, ok := cfg.StoragePath("default")
 	require.True(t, ok)
 
@@ -245,10 +250,11 @@ func TestRepo_GetRemoteReferences(t *testing.T) {
 		cfg,
 	)
 	for _, tc := range []struct {
-		desc     string
-		remote   string
-		opts     []GetRemoteReferencesOption
-		expected []git.Reference
+		desc                  string
+		remote                string
+		opts                  []GetRemoteReferencesOption
+		expected              []git.Reference
+		expectedGitSSHCommand string
 	}{
 		{
 			desc:   "not found",
@@ -295,11 +301,30 @@ func TestRepo_GetRemoteReferences(t *testing.T) {
 				{Name: "refs/heads/master", Target: commit},
 			},
 		},
+		{
+			desc:   "with custom ssh command",
+			remote: repoPath,
+			opts: []GetRemoteReferencesOption{
+				WithPatterns("refs/heads/master"),
+				WithSSHCommand("custom-ssh -with-creds"),
+			},
+			expected: []git.Reference{
+				{Name: "refs/heads/master", Target: commit},
+			},
+			expectedGitSSHCommand: "custom-ssh -with-creds",
+		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			refs, err := repo.GetRemoteReferences(ctx, tc.remote, tc.opts...)
 			require.NoError(t, err)
 			require.Equal(t, tc.expected, refs)
+
+			gitSSHCommand, err := ioutil.ReadFile(gitSSHCommandFile)
+			if !os.IsNotExist(err) {
+				require.NoError(t, err)
+			}
+
+			require.Equal(t, tc.expectedGitSSHCommand, string(gitSSHCommand))
 		})
 	}
 }
diff --git a/internal/git/localrepo/remote.go b/internal/git/localrepo/remote.go
index d5f3b0bf0..bb659f6d2 100644
--- a/internal/git/localrepo/remote.go
+++ b/internal/git/localrepo/remote.go
@@ -274,6 +274,10 @@ func validateNotBlank(val, name string) error {
 	return nil
 }
 
+func envGitSSHCommand(cmd string) string {
+	return "GIT_SSH_COMMAND=" + cmd
+}
+
 // PushOptions are options that can be configured for a push.
 type PushOptions struct {
 	// SSHCommand is the command line to use for git's SSH invocation. The command line is used
@@ -293,7 +297,7 @@ func (repo *Repo) Push(ctx context.Context, remote string, refspecs []string, op
 
 	var env []string
 	if options.SSHCommand != "" {
-		env = append(env, "GIT_SSH_COMMAND="+options.SSHCommand)
+		env = append(env, envGitSSHCommand(options.SSHCommand))
 	}
 
 	stderr := &bytes.Buffer{}
diff --git a/internal/git/localrepo/remote_test.go b/internal/git/localrepo/remote_test.go
index 33505f5f3..158f3ea49 100644
--- a/internal/git/localrepo/remote_test.go
+++ b/internal/git/localrepo/remote_test.go
@@ -438,13 +438,11 @@ func TestRepo_FetchRemote(t *testing.T) {
 	})
 }
 
-func TestRepo_Push(t *testing.T) {
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
-	cfg, sourceRepoPb, _ := testcfg.BuildWithRepo(t)
-
-	tmpDir := testhelper.TempDir(t)
+// captureGitSSHCommand returns a path to a script that wraps the git at the passed in path
+// and records the GIT_SSH_COMMAND that was passed to it. It returns also a path to the file
+// that contains the captured GIT_SSH_COMMAND value.
+func captureGitSSHCommand(t testing.TB, gitBinaryPath string) (string, string) {
+	tmpDir := t.TempDir()
 
 	gitPath := filepath.Join(tmpDir, "git-hook")
 	envPath := filepath.Join(tmpDir, "GIT_SSH_PATH")
@@ -455,12 +453,23 @@ func TestRepo_Push(t *testing.T) {
 if [ -z ${GIT_SSH_COMMAND+x} ];then rm -f %q ;else echo -n "$GIT_SSH_COMMAND" > %q; fi
 %s "$@"`,
 			envPath, envPath,
-			cfg.Git.BinPath,
+			gitBinaryPath,
 		)),
 		os.ModePerm),
 	)
 
-	cfg.Git.BinPath = gitPath
+	return gitPath, envPath
+}
+
+func TestRepo_Push(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	cfg, sourceRepoPb, _ := testcfg.BuildWithRepo(t)
+
+	wrappedGit, gitSSHCommandFile := captureGitSSHCommand(t, cfg.Git.BinPath)
+
+	cfg.Git.BinPath = wrappedGit
 	sourceRepo := NewTestRepo(t, cfg, sourceRepoPb)
 
 	setupPushRepo := func(t testing.TB) (*Repo, string, []git.ConfigPair) {
@@ -563,7 +572,7 @@ if [ -z ${GIT_SSH_COMMAND+x} ];then rm -f %q ;else echo -n "$GIT_SSH_COMMAND" >
 			}
 			require.NoError(t, err)
 
-			gitSSHCommand, err := ioutil.ReadFile(envPath)
+			gitSSHCommand, err := ioutil.ReadFile(gitSSHCommandFile)
 			if !os.IsNotExist(err) {
 				require.NoError(t, err)
 			}
diff --git a/internal/gitaly/service/remote/update_remote_mirror.go b/internal/gitaly/service/remote/update_remote_mirror.go
index df1fe4886..1f1131ce5 100644
--- a/internal/gitaly/service/remote/update_remote_mirror.go
+++ b/internal/gitaly/service/remote/update_remote_mirror.go
@@ -144,9 +144,16 @@ func (s *server) goUpdateRemoteMirror(stream gitalypb.RemoteService_UpdateRemote
 		}
 	}
 
+	sshCommand, clean, err := git.BuildSSHInvocation(ctx, firstRequest.GetSshKey(), firstRequest.GetKnownHosts())
+	if err != nil {
+		return fmt.Errorf("build ssh invocation: %w", err)
+	}
+	defer clean()
+
 	remoteRefsSlice, err := repo.GetRemoteReferences(ctx, remoteName,
 		localrepo.WithPatterns("refs/heads/*", "refs/tags/*"),
 		localrepo.WithConfig(remoteConfig...),
+		localrepo.WithSSHCommand(sshCommand),
 	)
 	if err != nil {
 		return fmt.Errorf("get remote references: %w", err)
@@ -257,31 +264,23 @@ func (s *server) goUpdateRemoteMirror(stream gitalypb.RemoteService_UpdateRemote
 		}
 	}
 
-	if len(refspecs) > 0 {
-		sshCommand, clean, err := git.BuildSSHInvocation(ctx, firstRequest.GetSshKey(), firstRequest.GetKnownHosts())
-		if err != nil {
-			return fmt.Errorf("build ssh invocation: %w", err)
+	for len(refspecs) > 0 {
+		batch := refspecs
+		if len(refspecs) > pushBatchSize {
+			batch = refspecs[:pushBatchSize]
 		}
-		defer clean()
 
-		for len(refspecs) > 0 {
-			batch := refspecs
-			if len(refspecs) > pushBatchSize {
-				batch = refspecs[:pushBatchSize]
-			}
-
-			refspecs = refspecs[len(batch):]
-
-			// The refs could have been modified on the mirror during after we fetched them.
-			// This could cause divergent refs to be force pushed over even with keep_divergent_refs set.
-			// This could be addressed by force pushing only if the current ref still matches what
-			// we received in the original fetch. https://gitlab.com/gitlab-org/gitaly/-/issues/3505
-			if err := repo.Push(ctx, remoteName, batch, localrepo.PushOptions{
-				SSHCommand: sshCommand,
-				Config:     remoteConfig,
-			}); err != nil {
-				return fmt.Errorf("push to mirror: %w", err)
-			}
+		refspecs = refspecs[len(batch):]
+
+		// The refs could have been modified on the mirror during after we fetched them.
+		// This could cause divergent refs to be force pushed over even with keep_divergent_refs set.
+		// This could be addressed by force pushing only if the current ref still matches what
+		// we received in the original fetch. https://gitlab.com/gitlab-org/gitaly/-/issues/3505
+		if err := repo.Push(ctx, remoteName, batch, localrepo.PushOptions{
+			SSHCommand: sshCommand,
+			Config:     remoteConfig,
+		}); err != nil {
+			return fmt.Errorf("push to mirror: %w", err)
 		}
 	}