diff --git a/pkg/cmd/copy/copy.go b/pkg/cmd/copy/copy.go index bebc1e26..6a8c0e08 100644 --- a/pkg/cmd/copy/copy.go +++ b/pkg/cmd/copy/copy.go @@ -98,7 +98,7 @@ func runCopyCommand(t *terminal.Terminal, cstore CopyStore, source, dest string, return breverrors.WrapAndTrace(err) } - err = runSCP(t, sshName, localPath, remotePath, isUpload) + err = runCopyWithFallback(t, sshName, localPath, remotePath, isUpload) if err != nil { return breverrors.WrapAndTrace(err) } @@ -209,42 +209,150 @@ func parseWorkspacePath(path string) (workspace, filePath string, err error) { return parts[0], parts[1], nil } -func runSCP(t *terminal.Terminal, sshAlias, localPath, remotePath string, isUpload bool) error { - var scpCmd *exec.Cmd - var source, dest string +type commandRunner func(name string, args ...string) ([]byte, error) + +func combinedOutputRunner(name string, args ...string) ([]byte, error) { + cmd := exec.Command(name, args...) //nolint:gosec // Command and args come from internal call sites using fixed binaries/flags (rsync/scp). + output, err := cmd.CombinedOutput() + if err != nil { + return output, fmt.Errorf("run %s command: %w", name, err) + } + return output, nil +} + +func runCopyWithFallback(t *terminal.Terminal, sshAlias, localPath, remotePath string, isUpload bool) error { + source, dest := transferEndpoints(sshAlias, localPath, remotePath, isUpload) startTime := time.Now() + err := transferWithFallback(sshAlias, localPath, remotePath, isUpload, combinedOutputRunner, rsyncInstalledLocally, func(reason string) { + t.Vprint(t.Yellow("%s\n", reason)) + }) + if err != nil { + return breverrors.WrapAndTrace(err) + } - scpArgs := []string{"scp"} + duration := time.Since(startTime) + fmt.Print("\n") + t.Vprint(t.Green(fmt.Sprintf("✓ Successfully copied %s → %s (%v)\n", source, dest, duration.Round(time.Millisecond)))) - if isUpload { - if isDirectory(localPath) { - scpArgs = append(scpArgs, "-r") + return nil +} + +func rsyncInstalledLocally() bool { + _, err := exec.LookPath("rsync") + return err == nil +} + +func transferWithFallback(sshAlias, localPath, remotePath string, isUpload bool, runner commandRunner, rsyncAvailable func() bool, onFallback func(reason string)) error { + notifyFallback := func(reason string) { + if onFallback != nil { + onFallback(reason) } - scpArgs = append(scpArgs, localPath, fmt.Sprintf("%s:%s", sshAlias, remotePath)) - source = localPath - dest = fmt.Sprintf("%s:%s", sshAlias, remotePath) + } + + // Directory sources are normalized to contents-copy form so the + // destination always mirrors the source, whether or not it already + // exists, and rsync and scp produce identical layouts. Uploads can + // stat the local source; downloads probe the remote path type. + sourceIsDir := isUpload && isDirectory(localPath) + if !isUpload { + sourceIsDir = remotePathIsDir(sshAlias, remotePath, runner) + } + + if !rsyncAvailable() { + notifyFallback("rsync not found on this machine, using scp. Install rsync for faster transfers.") + return runSCPCommand(sshAlias, localPath, remotePath, isUpload, sourceIsDir, runner) + } + + err := runRsyncCommand(sshAlias, localPath, remotePath, isUpload, sourceIsDir, runner) + if err == nil { + return nil + } + + if strings.Contains(err.Error(), "command not found") { + notifyFallback("rsync is not installed on the instance, falling back to scp. Install rsync on the instance for faster transfers.") } else { - scpArgs = append(scpArgs, "-r") - scpArgs = append(scpArgs, fmt.Sprintf("%s:%s", sshAlias, remotePath), localPath) - source = fmt.Sprintf("%s:%s", sshAlias, remotePath) - dest = localPath + notifyFallback("rsync failed, falling back to scp...") } - scpCmd = exec.Command(scpArgs[0], scpArgs[1:]...) //nolint:gosec //sshAlias is validated workspace identifier + scpErr := runSCPCommand(sshAlias, localPath, remotePath, isUpload, sourceIsDir, runner) + if scpErr != nil { + return fmt.Errorf("%v\nscp fallback failed: %w", err, scpErr) + } + + return nil +} + +func remotePathIsDir(sshAlias, remotePath string, runner commandRunner) bool { + quoted := "'" + strings.ReplaceAll(remotePath, "'", `'\''`) + "'" + _, err := runner("ssh", sshAlias, "test", "-d", quoted) + return err == nil +} - output, err := scpCmd.CombinedOutput() +func runRsyncCommand(sshAlias, localPath, remotePath string, isUpload, sourceIsDir bool, runner commandRunner) error { + rsyncArgs := buildRsyncArgs(sshAlias, localPath, remotePath, isUpload, sourceIsDir) + output, err := runner("rsync", rsyncArgs...) if err != nil { - return breverrors.WrapAndTrace(fmt.Errorf("scp failed: %s\nOutput: %s", err.Error(), string(output))) + return fmt.Errorf("rsync failed: %s\nOutput: %s", err.Error(), string(output)) } + return nil +} - duration := time.Since(startTime) - fmt.Print("\n") - t.Vprint(t.Green(fmt.Sprintf("✓ Successfully copied %s → %s (%v)\n", source, dest, duration.Round(time.Millisecond)))) - +func runSCPCommand(sshAlias, localPath, remotePath string, isUpload, sourceIsDir bool, runner commandRunner) error { + scpArgs := buildSCPArgs(sshAlias, localPath, remotePath, isUpload, sourceIsDir) + output, err := runner("scp", scpArgs...) + if err != nil { + return fmt.Errorf("scp failed: %s\nOutput: %s", err.Error(), string(output)) + } return nil } +func buildRsyncArgs(sshAlias, localPath, remotePath string, isUpload, sourceIsDir bool) []string { + source, dest := transferEndpoints(sshAlias, localPath, remotePath, isUpload) + + rsyncArgs := []string{"-z", "-e", "ssh"} + if !isUpload || sourceIsDir { + rsyncArgs = append(rsyncArgs, "-r") + } + // A trailing slash makes rsync copy the directory's contents, so the + // destination becomes the copy instead of having the source directory + // nested inside it. scp gets the same treatment via "/." below. + if sourceIsDir && !strings.HasSuffix(source, "/") { + source += "/" + } + rsyncArgs = append(rsyncArgs, source, dest) + + return rsyncArgs +} + +func buildSCPArgs(sshAlias, localPath, remotePath string, isUpload, sourceIsDir bool) []string { + source, dest := transferEndpoints(sshAlias, localPath, remotePath, isUpload) + + scpArgs := []string{} + if !isUpload || sourceIsDir { + scpArgs = append(scpArgs, "-r") + } + // "dir/." makes scp copy the directory's contents, mirroring rsync's + // trailing-slash behavior regardless of whether the destination exists. + if sourceIsDir { + if !strings.HasSuffix(source, "/") { + source += "/" + } + source += "." + } + scpArgs = append(scpArgs, source, dest) + + return scpArgs +} + +func transferEndpoints(sshAlias, localPath, remotePath string, isUpload bool) (source, dest string) { + remoteTarget := fmt.Sprintf("%s:%s", sshAlias, remotePath) + if isUpload { + return localPath, remoteTarget + } + return remoteTarget, localPath +} + func waitForSSHToBeAvailable(sshAlias string, s *spinner.Spinner) error { counter := 0 s.Suffix = " waiting for SSH connection to be available" @@ -313,7 +421,7 @@ func copyExternalNode(t *terminal.Terminal, cstore CopyStore, node *nodev1.Exter return breverrors.WrapAndTrace(err) } - return runSCP(t, alias, localPath, remotePath, isUpload) + return runCopyWithFallback(t, alias, localPath, remotePath, isUpload) } func pollUntil(s *spinner.Spinner, wsid string, state string, copyStore CopyStore, waitMsg string) error { diff --git a/pkg/cmd/copy/copy_test.go b/pkg/cmd/copy/copy_test.go index 483aa31b..3767f5e9 100644 --- a/pkg/cmd/copy/copy_test.go +++ b/pkg/cmd/copy/copy_test.go @@ -1,9 +1,66 @@ package copy import ( + "errors" "testing" + + "github.com/stretchr/testify/assert" ) +func TestBuildRsyncArgs(t *testing.T) { + t.Run("upload file", func(t *testing.T) { + args := buildRsyncArgs("ws", "/tmp/local.txt", "/remote/path", true, false) + assert.Equal(t, []string{"-z", "-e", "ssh", "/tmp/local.txt", "ws:/remote/path"}, args) + }) + + t.Run("upload directory copies contents", func(t *testing.T) { + args := buildRsyncArgs("ws", "/tmp/mydir", "/remote/path", true, true) + assert.Equal(t, []string{"-z", "-e", "ssh", "-r", "/tmp/mydir/", "ws:/remote/path"}, args) + }) + + t.Run("upload directory with trailing slash is not doubled", func(t *testing.T) { + args := buildRsyncArgs("ws", "/tmp/mydir/", "/remote/path", true, true) + assert.Equal(t, []string{"-z", "-e", "ssh", "-r", "/tmp/mydir/", "ws:/remote/path"}, args) + }) + + t.Run("download file", func(t *testing.T) { + args := buildRsyncArgs("ws", "/tmp/local.txt", "/remote/path", false, false) + assert.Equal(t, []string{"-z", "-e", "ssh", "-r", "ws:/remote/path", "/tmp/local.txt"}, args) + }) + + t.Run("download directory copies contents", func(t *testing.T) { + args := buildRsyncArgs("ws", "/tmp/local", "/remote/path", false, true) + assert.Equal(t, []string{"-z", "-e", "ssh", "-r", "ws:/remote/path/", "/tmp/local"}, args) + }) +} + +func TestBuildSCPArgs(t *testing.T) { + t.Run("upload file", func(t *testing.T) { + args := buildSCPArgs("ws", "/tmp/local.txt", "/remote/path", true, false) + assert.Equal(t, []string{"/tmp/local.txt", "ws:/remote/path"}, args) + }) + + t.Run("upload directory copies contents", func(t *testing.T) { + args := buildSCPArgs("ws", "/tmp/mydir", "/remote/path", true, true) + assert.Equal(t, []string{"-r", "/tmp/mydir/.", "ws:/remote/path"}, args) + }) + + t.Run("upload directory with trailing slash is not doubled", func(t *testing.T) { + args := buildSCPArgs("ws", "/tmp/mydir/", "/remote/path", true, true) + assert.Equal(t, []string{"-r", "/tmp/mydir/.", "ws:/remote/path"}, args) + }) + + t.Run("download file", func(t *testing.T) { + args := buildSCPArgs("ws", "/tmp/local.txt", "/remote/path", false, false) + assert.Equal(t, []string{"-r", "ws:/remote/path", "/tmp/local.txt"}, args) + }) + + t.Run("download directory copies contents", func(t *testing.T) { + args := buildSCPArgs("ws", "/tmp/local", "/remote/path", false, true) + assert.Equal(t, []string{"-r", "ws:/remote/path/.", "/tmp/local"}, args) + }) +} + func TestParseCopyArguments_Upload(t *testing.T) { ws, remotePath, localPath, isUpload, err := parseCopyArguments("./local.txt", "my-node:/tmp/dest") if err != nil { @@ -88,3 +145,156 @@ func TestParseWorkspacePath_InvalidMultipleColons(t *testing.T) { t.Fatal("expected error for multiple colons") } } + +func TestRemotePathIsDir(t *testing.T) { + t.Run("directory", func(t *testing.T) { + var gotArgs []string + runner := func(name string, args ...string) ([]byte, error) { + gotArgs = append([]string{name}, args...) + return nil, nil + } + assert.True(t, remotePathIsDir("ws", "/remote/path", runner)) + assert.Equal(t, []string{"ssh", "ws", "test", "-d", "'/remote/path'"}, gotArgs) + }) + + t.Run("not a directory", func(t *testing.T) { + runner := func(name string, args ...string) ([]byte, error) { + return nil, errors.New("exit status 1") + } + assert.False(t, remotePathIsDir("ws", "/remote/file.txt", runner)) + }) + + t.Run("quotes are escaped", func(t *testing.T) { + var gotArgs []string + runner := func(name string, args ...string) ([]byte, error) { + gotArgs = append([]string{name}, args...) + return nil, nil + } + remotePathIsDir("ws", "/it's/a/path", runner) + assert.Equal(t, `'/it'\''s/a/path'`, gotArgs[4]) + }) +} + +func TestTransferWithFallbackDownloadNormalization(t *testing.T) { + rsyncAvailable := func() bool { return true } + + t.Run("download probes remote path type and copies directory contents", func(t *testing.T) { + commands := [][]string{} + runner := func(name string, args ...string) ([]byte, error) { + commands = append(commands, append([]string{name}, args...)) + return nil, nil + } + + err := transferWithFallback("ws", "/tmp/local", "/remote/path", false, runner, rsyncAvailable, nil) + assert.NoError(t, err) + assert.Len(t, commands, 2) + assert.Equal(t, "ssh", commands[0][0]) + assert.Equal(t, []string{"rsync", "-z", "-e", "ssh", "-r", "ws:/remote/path/", "/tmp/local"}, commands[1]) + }) + + t.Run("download of a file is not normalized", func(t *testing.T) { + commands := [][]string{} + runner := func(name string, args ...string) ([]byte, error) { + if name == "ssh" { + return nil, errors.New("exit status 1") + } + commands = append(commands, append([]string{name}, args...)) + return nil, nil + } + + err := transferWithFallback("ws", "/tmp/local.txt", "/remote/file.txt", false, runner, rsyncAvailable, nil) + assert.NoError(t, err) + assert.Equal(t, [][]string{{"rsync", "-z", "-e", "ssh", "-r", "ws:/remote/file.txt", "/tmp/local.txt"}}, commands) + }) +} + +func TestTransferWithFallback(t *testing.T) { + rsyncAvailable := func() bool { return true } + + t.Run("rsync success", func(t *testing.T) { + calls := []string{} + runner := func(name string, args ...string) ([]byte, error) { + calls = append(calls, name) + return []byte("ok"), nil + } + + onFallbackCalled := false + err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner, rsyncAvailable, func(reason string) { + onFallbackCalled = true + }) + assert.NoError(t, err) + assert.False(t, onFallbackCalled) + assert.Equal(t, []string{"rsync"}, calls) + }) + + t.Run("rsync not installed locally skips straight to scp", func(t *testing.T) { + calls := []string{} + runner := func(name string, args ...string) ([]byte, error) { + calls = append(calls, name) + return []byte("ok"), nil + } + + reasons := []string{} + err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner, func() bool { return false }, func(reason string) { + reasons = append(reasons, reason) + }) + assert.NoError(t, err) + assert.Equal(t, []string{"scp"}, calls) + assert.Len(t, reasons, 1) + assert.Contains(t, reasons[0], "Install rsync for faster transfers") + }) + + t.Run("rsync missing on instance falls back with install hint", func(t *testing.T) { + calls := []string{} + runner := func(name string, args ...string) ([]byte, error) { + calls = append(calls, name) + if name == "rsync" { + return []byte("bash: rsync: command not found"), errors.New("exit status 127") + } + return []byte("scp ok"), nil + } + + reasons := []string{} + err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner, rsyncAvailable, func(reason string) { + reasons = append(reasons, reason) + }) + assert.NoError(t, err) + assert.Equal(t, []string{"rsync", "scp"}, calls) + assert.Len(t, reasons, 1) + assert.Contains(t, reasons[0], "Install rsync on the instance for faster transfers") + }) + + t.Run("rsync fails and scp succeeds", func(t *testing.T) { + calls := []string{} + runner := func(name string, args ...string) ([]byte, error) { + calls = append(calls, name) + if name == "rsync" { + return []byte("rsync failed"), errors.New("exit status 1") + } + return []byte("scp ok"), nil + } + + err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner, rsyncAvailable, func(reason string) { + calls = append(calls, "fallback") + }) + assert.NoError(t, err) + assert.Equal(t, []string{"rsync", "fallback", "scp"}, calls) + }) + + t.Run("rsync fails and scp fails", func(t *testing.T) { + runner := func(name string, args ...string) ([]byte, error) { + if name == "rsync" { + return []byte("rsync output"), errors.New("exit status 1") + } + return []byte("scp output"), errors.New("exit status 1") + } + + err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner, rsyncAvailable, func(reason string) {}) + assert.Error(t, err) + assert.Contains(t, err.Error(), "rsync failed: exit status 1") + assert.Contains(t, err.Error(), "scp fallback failed") + assert.Contains(t, err.Error(), "rsync output") + assert.Contains(t, err.Error(), "scp output") + assert.NotContains(t, err.Error(), "rsync failed: rsync failed:") + }) +}