From efca9855e7fef327a3f5ccc98b6642c702d96d0e Mon Sep 17 00:00:00 2001 From: Lutz Justen Date: Wed, 18 Jan 2023 17:49:52 +0100 Subject: [PATCH] [cdc_rsync] [cdc_stream] Switch from scp to sftp (#66) Use sftp for deploying remote components instead of scp. sftp has the advantage that it can also create directries, chmod files etc., so that we can do everything in one call of sftp instead of mixing scp and ssh calls. The downside of sftp is that it can't switch to ~ resp. %userprofile% for the remote side, and we have to assume that sftp starts in the user's home dir. This is the default and works on my machines! cdc_rsync and cdc_stream check the CDC_SFTP_COMMAND env var now and accept --sftp-command flags. If they are not set, the corresponding scp flag and env var is still used, with scp replaced by sftp. This is most likely correct as sftp and scp usually reside in the same directory and share largely identical parameters. --- README.md | 30 +++---- cdc_rsync/cdc_rsync_client.cc | 28 ++---- cdc_rsync/cdc_rsync_client.h | 6 +- cdc_rsync/params.cc | 42 +++++++-- cdc_rsync/params_test.cc | 66 +++++++++++--- cdc_rsync/server_arch.cc | 87 ++++++++----------- cdc_rsync/server_arch.h | 26 +++--- cdc_rsync/server_arch_test.cc | 38 +++----- cdc_stream/asset_stream_config.cc | 8 +- cdc_stream/cdc_fuse_manager.cc | 40 +++++---- .../local_assets_stream_manager_client.cc | 4 +- .../local_assets_stream_manager_client.h | 4 +- ...ocal_assets_stream_manager_service_impl.cc | 6 +- cdc_stream/session.cc | 4 +- cdc_stream/session.h | 4 +- cdc_stream/start_command.cc | 41 +++++++-- cdc_stream/start_command.h | 4 +- cdc_stream/start_service_command.cc | 2 +- proto/local_assets_stream_manager.proto | 6 +- 19 files changed, 253 insertions(+), 193 deletions(-) diff --git a/README.md b/README.md index 4562cfd..949eeca 100644 --- a/README.md +++ b/README.md @@ -191,7 +191,7 @@ To build the tools from source, the following steps have to be executed on ``` Finally, install an SSH client on the Windows device if not present. -The file transfer tools require `ssh.exe` and `scp.exe`. +The file transfer tools require `ssh.exe` and `sftp.exe`. ## Building @@ -227,24 +227,24 @@ The two tools can be built and used independently. ## Usage -The tools require a setup where you can use SSH and SCP from the Windows machine -to the Linux device without entering a password, e.g. by using key-based +The tools require a setup where you can use SSH and SFTP from the Windows +machine to the Linux device without entering a password, e.g. by using key-based authentication. -### Configuring SSH and SCP +### Configuring SSH and SFTP -By default, the tools search `ssh.exe` and `scp.exe` from the path environment +By default, the tools search `ssh.exe` and `sftp.exe` from the path environment variable. If you can run the following commands in a Windows cmd without entering your password, you are all set: ``` ssh user@linux.device.com -scp somefile.txt user@linux.device.com: +sftp user@linux.device.com ``` Here, `user` is the Linux user and `linux.device.com` is the Linux host to SSH into or copy the file to. If additional arguments are required, it is recommended to provide an SSH config -file. By default, both `ssh.exe` and `scp.exe` use the file at +file. By default, both `ssh.exe` and `sftp.exe` use the file at `%USERPROFILE%\.ssh\config` on Windows, if it exists. A possible config file that sets a username, a port, an identity file and a known host file could look as follows: @@ -256,21 +256,21 @@ Host linux_device IdentityFile C:\path\to\id_rsa UserKnownHostsFile C:\path\to\known_hosts ``` -If `ssh.exe` or `scp.exe` cannot be found, you can specify the full paths via -the command line arguments `--ssh-command` and `--scp-command` for `cdc_rsync` +If `ssh.exe` or `sftp.exe` cannot be found, you can specify the full paths via +the command line arguments `--ssh-command` and `--sftp-command` for `cdc_rsync` and `cdc_stream start` (see below), or set the environment variables -`CDC_SSH_COMMAND` and `CDC_SCP_COMMAND`, e.g. +`CDC_SSH_COMMAND` and `CDC_SFTP_COMMAND`, e.g. ``` set CDC_SSH_COMMAND="C:\path with space\to\ssh.exe" -set CDC_SCP_COMMAND="C:\path with space\to\scp.exe" +set CDC_SFTP_COMMAND="C:\path with space\to\sftp.exe" ``` Note that you can also specify SSH configuration via the environment variables instead of using a config file: ``` set CDC_SSH_COMMAND=C:\path\to\ssh.exe -p 12345 -i C:\path\to\id_rsa -oUserKnownHostsFile=C:\path\to\known_hosts -set CDC_SCP_COMMAND=C:\path\to\scp.exe -P 12345 -i C:\path\to\id_rsa -oUserKnownHostsFile=C:\path\to\known_hosts +set CDC_SFTP_COMMAND=C:\path\to\sftp.exe -P 12345 -i C:\path\to\id_rsa -oUserKnownHostsFile=C:\path\to\known_hosts ``` -Note the small `-p` for `ssh.exe` and the capital `-P` for `scp.exe`. +Note the small `-p` for `ssh.exe` and the capital `-P` for `sftp.exe`. #### Google Specific @@ -278,7 +278,7 @@ For Google internal usage, set the following environment variables to enable SSH authentication using a Google security key: ``` set CDC_SSH_COMMAND=C:\gnubby\bin\ssh.exe -set CDC_SCP_COMMAND=C:\gnubby\bin\scp.exe +set CDC_SFTP_COMMAND=C:\gnubby\bin\sftp.exe ``` Note that you will have to touch the security key multiple times during the first run. Subsequent runs only require a single touch. @@ -352,7 +352,7 @@ instead of to the file. `cdc_rsync` always logs to the console. To increase log verbosity, pass `-vvv` for debug logs or `-vvvv` for verbose logs. -For both sync and stream, the debug logs contain all SSH and SCP commands that +For both sync and stream, the debug logs contain all SSH and SFTP commands that are attempted to run, which is very useful for troubleshooting. If a command fails unexpectedly, copy it and run it in isolation. Pass `-vv` or `-vvv` for additional debug output. diff --git a/cdc_rsync/cdc_rsync_client.cc b/cdc_rsync/cdc_rsync_client.cc index 677f7ec..06cea47 100644 --- a/cdc_rsync/cdc_rsync_client.cc +++ b/cdc_rsync/cdc_rsync_client.cc @@ -109,8 +109,8 @@ CdcRsyncClient::CdcRsyncClient(const Options& options, if (!options_.ssh_command.empty()) { remote_util_.SetSshCommand(options_.ssh_command); } - if (!options_.scp_command.empty()) { - remote_util_.SetScpCommand(options_.scp_command); + if (!options_.sftp_command.empty()) { + remote_util_.SetSftpCommand(options_.sftp_command); } } @@ -413,26 +413,10 @@ absl::Status CdcRsyncClient::DeployServer(const ServerArch& arch) { } printer_.Print(deploy_msg, true, Util::GetConsoleWidth()); - // scp cdc_rsync_server to a temp location on the gamelet. - std::string remoteServerPath = - arch.RemoteToolsBinDir(ServerArch::UseCase::kScp) + - arch.CdcServerFilename(); - std::string remoteServerTmpPath = remoteServerPath + Util::GenerateUniqueId(); - std::string localServerPath = path::Join(exe_dir, arch.CdcServerFilename()); - status = remote_util_.Scp({localServerPath}, remoteServerTmpPath, - /*compress=*/true); - if (!status.ok()) { - return WrapStatus(status, "Failed to copy cdc_rsync_server to instance"); - } - - // Replace cdc_rsync_server file by the temp file. - std::string replace_cmd = - arch.GetDeployReplaceCommand(remoteServerPath, remoteServerTmpPath); - status = remote_util_.Run(replace_cmd, "replace"); - if (!status.ok()) { - return WrapStatus(status, - "Failed to replace old cdc_rsync_server by new one"); - } + // sftp cdc_rsync_server to the target. + std::string commands = arch.GetDeploySftpCommands(); + RETURN_IF_ERROR(remote_util_.Sftp(commands, exe_dir, /*compress=*/false), + "Failed to deploy cdc_rsync_server"); return absl::OkStatus(); } diff --git a/cdc_rsync/cdc_rsync_client.h b/cdc_rsync/cdc_rsync_client.h index d6e9b63..0af1635 100644 --- a/cdc_rsync/cdc_rsync_client.h +++ b/cdc_rsync/cdc_rsync_client.h @@ -54,10 +54,14 @@ class CdcRsyncClient { int forward_port_first = 44450; int forward_port_last = 44459; std::string ssh_command; - std::string scp_command; + std::string sftp_command; std::string sources_dir; // Base dir for files loaded for --files-from. PathFilter filter; + // Backwards compatibility for switching from scp to sftp. + // Used internally, do not use. + std::string deprecated_scp_command; + // Compression level 0 is invalid. static constexpr int kMinCompressLevel = -5; static constexpr int kMaxCompressLevel = 22; diff --git a/cdc_rsync/params.cc b/cdc_rsync/params.cc index 241f6a7..69be6ac 100644 --- a/cdc_rsync/params.cc +++ b/cdc_rsync/params.cc @@ -21,6 +21,7 @@ #include "absl/strings/str_split.h" #include "common/path.h" #include "common/port_range_parser.h" +#include "common/remote_util.h" #include "lib/zstd.h" namespace cdc_ft { @@ -75,9 +76,9 @@ Options: --ssh-command Path and arguments of ssh command to use, e.g. "C:\path\to\ssh.exe -p 12345 -i id_rsa -oUserKnownHostsFile=known_hosts" Can also be specified by the CDC_SSH_COMMAND environment variable. - --scp-command Path and arguments of scp command to use, e.g. - "C:\path\to\scp.exe -P 12345 -i id_rsa -oUserKnownHostsFile=known_hosts" - Can also be specified by the CDC_SCP_COMMAND environment variable. + --sftp-command Path and arguments of sftp command to use, e.g. + "C:\path\to\sftp.exe -P 12345 -i id_rsa -oUserKnownHostsFile=known_hosts" + Can also be specified by the CDC_SFTP_COMMAND environment variable. --forward-port TCP port or range used for SSH port forwarding (default: 44450-44459). If a range is specified, searches for available ports (slower). -h --help Help for cdc_rsync @@ -85,12 +86,15 @@ Options: constexpr char kSshCommandEnvVar[] = "CDC_SSH_COMMAND"; constexpr char kScpCommandEnvVar[] = "CDC_SCP_COMMAND"; +constexpr char kSftpCommandEnvVar[] = "CDC_SFTP_COMMAND"; // Populates some parameters from environment variables. void PopulateFromEnvVars(Parameters* parameters) { path::GetEnv(kSshCommandEnvVar, ¶meters->options.ssh_command) .IgnoreError(); - path::GetEnv(kScpCommandEnvVar, ¶meters->options.scp_command) + path::GetEnv(kScpCommandEnvVar, ¶meters->options.deprecated_scp_command) + .IgnoreError(); + path::GetEnv(kSftpCommandEnvVar, ¶meters->options.sftp_command) .IgnoreError(); } @@ -287,8 +291,15 @@ OptionResult HandleParameter(const std::string& key, const char* value, } if (key == "scp-command") { + // Backwards compatibility. Note that this flag is hidden from the help. if (!ValidateValue(key, value)) return OptionResult::kError; - params->options.scp_command = value; + params->options.deprecated_scp_command = value; + return OptionResult::kConsumedKeyValue; + } + + if (key == "sftp-command") { + if (!ValidateValue(key, value)) return OptionResult::kError; + params->options.sftp_command = value; return OptionResult::kConsumedKeyValue; } @@ -491,6 +502,27 @@ bool Parse(int argc, const char* const* argv, Parameters* parameters) { PopUserHost(¶meters->destination, ¶meters->user_host); + // Backwards compabitility after switching to sftp. Convert scp to sftp + // command Note that this flag is hidden from the help. + if (parameters->options.sftp_command.empty() && + !parameters->options.deprecated_scp_command.empty()) { + LOG_WARNING( + "The CDC_SCP_COMMAND environment variable and the --scp-command flag " + "are deprecated. Please set CDC_SFTP_COMMAND or --sftp-command " + "instead."); + + parameters->options.sftp_command = RemoteUtil::ScpToSftpCommand( + parameters->options.deprecated_scp_command); + if (!parameters->options.sftp_command.empty()) { + LOG_WARNING("Converted scp command '%s' to sftp command '%s'.", + parameters->options.deprecated_scp_command, + parameters->options.sftp_command); + } else { + LOG_WARNING("Failed to convert scp command '%s' to sftp command.", + parameters->options.deprecated_scp_command); + } + } + if (!ValidateParameters(*parameters, help)) { return false; } diff --git a/cdc_rsync/params_test.cc b/cdc_rsync/params_test.cc index 24ae2fa..9b8a2f7 100644 --- a/cdc_rsync/params_test.cc +++ b/cdc_rsync/params_test.cc @@ -32,6 +32,10 @@ constexpr char kUserHostDst[] = "user@host:destination"; constexpr char kUserHost[] = "user@host"; constexpr char kDst[] = "destination"; +constexpr char kSshCommandEnvVar[] = "CDC_SSH_COMMAND"; +constexpr char kScpCommandEnvVar[] = "CDC_SCP_COMMAND"; +constexpr char kSftpCommandEnvVar[] = "CDC_SFTP_COMMAND"; + class TestLog : public Log { public: explicit TestLog() : Log(LogLevel::kInfo) {} @@ -60,6 +64,11 @@ class ParamsTest : public ::testing::Test { void TearDown() override { std::cout.rdbuf(prev_stdout_); std::cerr.rdbuf(prev_stderr_); + + // Clear env. They seem to be sticky sometimes and leak into other tests. + path::SetEnv(kSshCommandEnvVar, ""); + path::SetEnv(kScpCommandEnvVar, ""); + path::SetEnv(kSftpCommandEnvVar, ""); } protected: @@ -152,24 +161,57 @@ TEST_F(ParamsTest, ParseFailsOnContimeoutEqualsNoValue) { ExpectError(NeedsValueError("contimeout")); } -TEST_F(ParamsTest, ParseSucceedsWithSshScpCommands) { - const char* argv[] = {"cdc_rsync.exe", kSrc, - kUserHostDst, "--ssh-command=sshcmd", - "--scp-command=scpcmd", NULL}; +TEST_F(ParamsTest, ParseSucceedsWithSshSftpCommands) { + const char* argv[] = { + "cdc_rsync.exe", kSrc, kUserHostDst, "--ssh-command=sshcmd", + "--sftp-command=sftpcmd", NULL}; EXPECT_TRUE(Parse(static_cast(std::size(argv)) - 1, argv, ¶meters_)); - EXPECT_EQ(parameters_.options.scp_command, "scpcmd"); + EXPECT_EQ(parameters_.options.sftp_command, "sftpcmd"); EXPECT_EQ(parameters_.options.ssh_command, "sshcmd"); } -TEST_F(ParamsTest, ParseSucceedsWithSshScpCommandsByEnvVars) { - EXPECT_OK(path::SetEnv("CDC_SSH_COMMAND", "sshcmd")); - EXPECT_OK(path::SetEnv("CDC_SCP_COMMAND", "scpcmd")); +TEST_F(ParamsTest, ParseSucceedsWithSshSftpCommandsByEnvVars) { + EXPECT_OK(path::SetEnv(kSshCommandEnvVar, "sshcmd")); + EXPECT_OK(path::SetEnv(kSftpCommandEnvVar, "sftpcmd")); const char* argv[] = {"cdc_rsync.exe", kSrc, kUserHostDst, NULL}; EXPECT_TRUE(Parse(static_cast(std::size(argv)) - 1, argv, ¶meters_)); - EXPECT_EQ(parameters_.options.scp_command, "scpcmd"); + EXPECT_EQ(parameters_.options.sftp_command, "sftpcmd"); EXPECT_EQ(parameters_.options.ssh_command, "sshcmd"); } +TEST_F(ParamsTest, ParseSucceedsWithScpCommandFallback) { + const char* argv[] = {"cdc_rsync.exe", kSrc, kUserHostDst, + "--scp-command=C:\\scp.exe foo", NULL}; + EXPECT_TRUE(Parse(static_cast(std::size(argv)) - 1, argv, ¶meters_)); + EXPECT_EQ(parameters_.options.sftp_command, "C:\\sftp.exe foo"); +} + +TEST_F(ParamsTest, ParseSucceedsWithScpCommandFallbackByEnvVar) { + EXPECT_OK(path::SetEnv(kScpCommandEnvVar, "C:\\scp.exe foo")); + const char* argv[] = {"cdc_rsync.exe", kSrc, kUserHostDst, NULL}; + EXPECT_TRUE(Parse(static_cast(std::size(argv)) - 1, argv, ¶meters_)); + EXPECT_EQ(parameters_.options.sftp_command, "C:\\sftp.exe foo"); +} + +TEST_F(ParamsTest, ParseSucceedsWithSftpOverwritingScp) { + const char* argv[] = {"cdc_rsync.exe", + kSrc, + kUserHostDst, + "--scp-command=C:\\scp.exe foo", + "--sftp-command=sftpcmd", + NULL}; + EXPECT_TRUE(Parse(static_cast(std::size(argv)) - 1, argv, ¶meters_)); + EXPECT_EQ(parameters_.options.sftp_command, "sftpcmd"); +} + +TEST_F(ParamsTest, ParseSucceedsWithSftpEnvVarOverwritingScp) { + EXPECT_OK(path::SetEnv(kSftpCommandEnvVar, "sftpcmd")); + const char* argv[] = {"cdc_rsync.exe", kSrc, kUserHostDst, + "--scp-command=C:\\scp.exe foo", NULL}; + EXPECT_TRUE(Parse(static_cast(std::size(argv)) - 1, argv, ¶meters_)); + EXPECT_EQ(parameters_.options.sftp_command, "sftpcmd"); +} + TEST_F(ParamsTest, ParseSucceedsWithNoSshCommand) { const char* argv[] = {"cdc_rsync.exe", kSrc, kUserHostDst, "--ssh-command=", NULL}; @@ -178,12 +220,12 @@ TEST_F(ParamsTest, ParseSucceedsWithNoSshCommand) { ExpectError(NeedsValueError("ssh-command")); } -TEST_F(ParamsTest, ParseSucceedsWithNoScpCommand) { - const char* argv[] = {"cdc_rsync.exe", kSrc, kUserHostDst, "--scp-command", +TEST_F(ParamsTest, ParseSucceedsWithNoSftpCommand) { + const char* argv[] = {"cdc_rsync.exe", kSrc, kUserHostDst, "--sftp-command", NULL}; EXPECT_FALSE( Parse(static_cast(std::size(argv)) - 1, argv, ¶meters_)); - ExpectError(NeedsValueError("scp-command")); + ExpectError(NeedsValueError("sftp-command")); } TEST_F(ParamsTest, ParseFailsOnNoUserHost) { diff --git a/cdc_rsync/server_arch.cc b/cdc_rsync/server_arch.cc index d0fb0a5..26070a7 100644 --- a/cdc_rsync/server_arch.cc +++ b/cdc_rsync/server_arch.cc @@ -18,8 +18,10 @@ #include "absl/strings/match.h" #include "absl/strings/str_format.h" +#include "absl/strings/str_split.h" #include "common/path.h" #include "common/remote_util.h" +#include "common/util.h" namespace cdc_ft { @@ -72,19 +74,13 @@ std::string ServerArch::CdcServerFilename() const { } } -std::string ServerArch::RemoteToolsBinDir(UseCase use_case) const { +std::string ServerArch::RemoteToolsBinDir() const { switch (type_) { case Type::kWindows: { - // TODO(ljusten): Unfortunately, scp doesn't seem to support shell var - // expansion, so %AppData% can't be used. This relies on scp copying - // files relative to %UserProfile% and %AppData% mapping to - // "AppData\\Roaming" relative to that. - std::string app_data = - use_case == UseCase::kScp ? "AppData\\Roaming" : "$env:appdata"; - return app_data + "\\cdc-file-transfer\\bin\\"; + return "AppData\\Roaming\\cdc-file-transfer\\bin\\"; } case Type::kLinux: - return "~/.cache/cdc-file-transfer/bin/"; + return ".cache/cdc-file-transfer/bin/"; default: assert(!kErrorArchTypeUnhandled); return std::string(); @@ -93,11 +89,7 @@ std::string ServerArch::RemoteToolsBinDir(UseCase use_case) const { std::string ServerArch::GetStartServerCommand(int exit_code_not_found, const std::string& args) const { - std::string server_dir = RemoteToolsBinDir(UseCase::kSsh); - std::string server_path = - RemoteUtil::QuoteForSsh(server_dir + CdcServerFilename()); - path::EnsureDoesNotEndWithPathSeparator(&server_dir); - server_dir = RemoteUtil::QuoteForSsh(server_dir); + std::string server_path = RemoteToolsBinDir() + CdcServerFilename(); switch (type_) { case Type::kWindows: @@ -106,53 +98,50 @@ std::string ServerArch::GetStartServerCommand(int exit_code_not_found, // a minor issue and means we display "Deploying server..." instead of // "Server not deployed. Deploying..."; return RemoteUtil::QuoteForWindows( - absl::StrFormat("Set-StrictMode -Version 2; " + absl::StrFormat("powershell -Command \" " + "Set-StrictMode -Version 2; " "$ErrorActionPreference = 'Stop'; " - "$server_dir = %s; " - "$server_path = %s; " - "$u=New-Item $server_dir -ItemType Directory -Force; " - "if (-not (Test-Path -Path $server_path)) { " + "if (-not (Test-Path -Path '%s')) { " " exit %i; " "} " - "& $server_path %s", - server_dir, server_path, exit_code_not_found, args)); + "%s %s " + "\"", + server_path, exit_code_not_found, server_path, args)); case Type::kLinux: - return absl::StrFormat( - "mkdir -p %s; if [ ! -f %s ]; then exit %i; fi; %s %s", server_dir, - server_path, exit_code_not_found, server_path, args); + return absl::StrFormat("if [ ! -f %s ]; then exit %i; fi; %s %s", + server_path, exit_code_not_found, server_path, + args); default: assert(!kErrorArchTypeUnhandled); return std::string(); } } -std::string ServerArch::GetDeployReplaceCommand( - const std::string& old_server_path, - const std::string& new_server_path) const { - const std::string old_path = RemoteUtil::QuoteForSsh(old_server_path); - const std::string new_path = RemoteUtil::QuoteForSsh(new_server_path); +std::string ServerArch::GetDeploySftpCommands() const { + std::string commands; - switch (type_) { - case Type::kWindows: - return RemoteUtil::QuoteForWindows(absl::StrFormat( - "Set-StrictMode -Version 2; " - "$ErrorActionPreference = 'Stop'; " - "$old_path = %s; " - "$new_path = %s; " - "if (Test-Path -Path $old_path) { " - " Get-Item -Path $old_path | " - " Set-ItemProperty -Name IsReadOnly -Value $false; " - "} " - "Move-Item -Path $new_path -Destination $old_path -Force", - old_path, new_path)); - case Type::kLinux: - return absl::StrFormat( - "([ ! -f %s ] || chmod u+w %s) && chmod a+x %s && mv %s %s", old_path, - old_path, new_path, new_path, old_path); - default: - assert(!kErrorArchTypeUnhandled); - return std::string(); + // Create the remote tools bin dir if it doesn't exist yet. + // This assumes that sftp's remote startup directory is the home directory. + const std::string server_dir = path::ToUnix(RemoteToolsBinDir()); + std::vector dir_parts = + absl::StrSplit(server_dir, '/', absl::SkipEmpty()); + for (const std::string& dir : dir_parts) { + // Use -mkdir to ignore errors if the directory already exists. + commands += absl::StrFormat("-mkdir %s\ncd %s\n", dir, dir); } + + // Copy the server binary to a temp location. This assumes that sftp's local + // startup directory is cdc_rsync's exe dir. + const std::string server_file = CdcServerFilename(); + const std::string server_temp_file = server_file + Util::GenerateUniqueId(); + commands += absl::StrFormat("put %s %s\n", server_file, server_temp_file); + + // Restore permissions in case they changed and propagate temp file. + commands += absl::StrFormat("-chmod 755 %s\n", server_file); + commands += absl::StrFormat("chmod 755 %s\n", server_temp_file); + commands += absl::StrFormat("rename %s %s\n", server_temp_file, server_file); + + return commands; } } // namespace cdc_ft diff --git a/cdc_rsync/server_arch.h b/cdc_rsync/server_arch.h index 7be2ca0..40f9da0 100644 --- a/cdc_rsync/server_arch.h +++ b/cdc_rsync/server_arch.h @@ -29,8 +29,6 @@ class ServerArch { kWindows = 1, }; - enum class UseCase { kSsh = 0, kScp = 1 }; - // Detects the architecture type based on the destination path, e.g. path // starting with C: indicate Windows. static Type Detect(const std::string& destination); @@ -42,16 +40,10 @@ class ServerArch { std::string CdcServerFilename() const; // Returns the arch-specific directory where cdc_rsync_server is deployed. - // On Windows, |use_case| determines what type of env variables to use: - // - kSsh uses $env:appdata and works for ssh commands. - // - kScp uses AppData\\Roaming and works for scp commands. - // On Linux, this flag is ignored. - std::string RemoteToolsBinDir(UseCase use_case) const; + std::string RemoteToolsBinDir() const; // Returns an arch-specific SSH shell command that gets invoked in order to // start cdc_rsync_server. The command - // - creates RemoteToolsBinDir() if it does not exist (so that the server can - // be deployed there subsequently; scp can't create directories), // - returns |exit_code_not_found| if cdc_rsync_server does not exist (to // prevent the confusing bash output message // "bash: .../cdc_rsync_server: No such file or directory"), and @@ -59,13 +51,15 @@ class ServerArch { std::string GetStartServerCommand(int exit_code_not_found, const std::string& args) const; - // Returns an arch-specific SSH shell command that gets invoked after - // cdc_rsync_server has been copied to a temp location. The command - // - makes the old cdc_rsync_server writable (if it exists), - // - makes the new cdc_rsync_server executable (Linux only) and - // - replaces the old cdc_rsync_server by the new one. - std::string GetDeployReplaceCommand(const std::string& old_server_path, - const std::string& new_server_path) const; + // Returns an arch-specific SFTP command sequence that deploys the server + // component on the target gets invoked after + // cdc_rsync_server has been copied to a temp location. The commands + // - create the cdc-file-transfer/bin folder if it doesn't exist yet, + // - make the old cdc_rsync_server writable if it exists, + // - copy cdc_rsync_server to a temp location, + // - make the new cdc_rsync_server executable (Linux only) and + // - replaces the existing cdc_rsync_server by the temp one. + std::string GetDeploySftpCommands() const; private: Type type_; diff --git a/cdc_rsync/server_arch_test.cc b/cdc_rsync/server_arch_test.cc index 0cb6f5b..6518082 100644 --- a/cdc_rsync/server_arch_test.cc +++ b/cdc_rsync/server_arch_test.cc @@ -60,47 +60,29 @@ TEST(ServerArchTest, CdcServerFilename) { } TEST(ServerArchTest, RemoteToolsBinDir) { - const std::string linux_ssh_dir = - ServerArch(kLinux).RemoteToolsBinDir(ServerArch::UseCase::kSsh); - EXPECT_TRUE(absl::StrContains(linux_ssh_dir, "/")); + const std::string linux_dir = ServerArch(kLinux).RemoteToolsBinDir(); + EXPECT_TRUE(absl::StrContains(linux_dir, ".cache/")); - const std::string linux_scp_dir = - ServerArch(kLinux).RemoteToolsBinDir(ServerArch::UseCase::kScp); - EXPECT_EQ(linux_ssh_dir, linux_scp_dir); - - const std::string win_ssh_dir = - ServerArch(kWindows).RemoteToolsBinDir(ServerArch::UseCase::kSsh); - EXPECT_TRUE(absl::StrContains(win_ssh_dir, "\\")); - EXPECT_TRUE(absl::StrContains(win_ssh_dir, "$env:appdata")); - - std::string win_scp_dir = - ServerArch(kWindows).RemoteToolsBinDir(ServerArch::UseCase::kScp); - EXPECT_TRUE(absl::StrContains(win_scp_dir, "\\")); - EXPECT_TRUE(absl::StrContains(win_scp_dir, "AppData\\Roaming")); + std::string win_dir = ServerArch(kWindows).RemoteToolsBinDir(); + EXPECT_TRUE(absl::StrContains(win_dir, "AppData\\Roaming\\")); } TEST(ServerArchTest, GetStartServerCommand) { std::string cmd = ServerArch(kWindows).GetStartServerCommand(123, "foo bar"); EXPECT_TRUE(absl::StrContains(cmd, "123")); - EXPECT_TRUE(absl::StrContains(cmd, "foo bar")); - EXPECT_TRUE(absl::StrContains(cmd, "New-Item ")); + EXPECT_TRUE(absl::StrContains(cmd, "cdc_rsync_server.exe foo bar")); cmd = ServerArch(kLinux).GetStartServerCommand(123, "foo bar"); EXPECT_TRUE(absl::StrContains(cmd, "123")); - EXPECT_TRUE(absl::StrContains(cmd, "foo bar")); - EXPECT_TRUE(absl::StrContains(cmd, "mkdir -p")); + EXPECT_TRUE(absl::StrContains(cmd, "cdc_rsync_server foo bar")); } TEST(ServerArchTest, GetDeployReplaceCommand) { - std::string cmd = ServerArch(kWindows).GetDeployReplaceCommand("aaa", "bbb"); - EXPECT_TRUE(absl::StrContains(cmd, "aaa")); - EXPECT_TRUE(absl::StrContains(cmd, "bbb")); - EXPECT_TRUE(absl::StrContains(cmd, "Move-Item ")); + std::string cmd = ServerArch(kWindows).GetDeploySftpCommands(); + EXPECT_TRUE(absl::StrContains(cmd, "cdc_rsync_server.exe")); - cmd = ServerArch(kLinux).GetDeployReplaceCommand("aaa", "bbb"); - EXPECT_TRUE(absl::StrContains(cmd, "aaa")); - EXPECT_TRUE(absl::StrContains(cmd, "bbb")); - EXPECT_TRUE(absl::StrContains(cmd, "mv ")); + cmd = ServerArch(kLinux).GetDeploySftpCommands(); + EXPECT_TRUE(absl::StrContains(cmd, "cdc_rsync_server")); } } // namespace diff --git a/cdc_stream/asset_stream_config.cc b/cdc_stream/asset_stream_config.cc index 41995df..bb62ea8 100644 --- a/cdc_stream/asset_stream_config.cc +++ b/cdc_stream/asset_stream_config.cc @@ -157,9 +157,9 @@ void AssetStreamConfig::RegisterCommandLineFlags(lyra::command& cmd, "connection to the host. See also --dev-src-dir.")); cmd.add_argument( - lyra::opt(dev_target_.scp_command, "cmd") - .name("--dev-scp-command") - .help("Scp command and extra flags to use for the " + lyra::opt(dev_target_.sftp_command, "cmd") + .name("--dev-sftp-command") + .help("Sftp command and extra flags to use for the " "connection to the host. See also --dev-src-dir.")); cmd.add_argument( @@ -258,7 +258,7 @@ std::string AssetStreamConfig::ToString() { ss << "dev-user-host = " << dev_target_.user_host << std::endl; ss << "dev-ssh-command = " << dev_target_.ssh_command << std::endl; - ss << "dev-scp-command = " << dev_target_.scp_command + ss << "dev-sftp-command = " << dev_target_.sftp_command << std::endl; ss << "dev-mount-dir = " << dev_target_.mount_dir << std::endl; return ss.str(); diff --git a/cdc_stream/cdc_fuse_manager.cc b/cdc_stream/cdc_fuse_manager.cc index 28c4c0f..5e411c9 100644 --- a/cdc_stream/cdc_fuse_manager.cc +++ b/cdc_stream/cdc_fuse_manager.cc @@ -16,6 +16,7 @@ #include "absl/strings/match.h" #include "absl/strings/str_format.h" +#include "absl/strings/str_split.h" #include "cdc_fuse_fs/constants.h" #include "common/gamelet_component.h" #include "common/log.h" @@ -30,7 +31,7 @@ constexpr char kExeFilename[] = "cdc_stream.exe"; constexpr char kFuseFilename[] = "cdc_fuse_fs"; constexpr char kLibFuseFilename[] = "libfuse.so"; constexpr char kFuseStdoutPrefix[] = "cdc_fuse_fs_stdout"; -constexpr char kRemoteToolsBinDir[] = "~/.cache/cdc-file-transfer/bin/"; +constexpr char kRemoteToolsBinDir[] = ".cache/cdc-file-transfer/bin/"; // Cache directory on the gamelet to store data chunks. constexpr char kCacheDir[] = "~/.cache/cdc-file-transfer/chunks"; @@ -54,23 +55,25 @@ absl::Status CdcFuseManager::Deploy() { std::string exe_dir; RETURN_IF_ERROR(path::GetExeDir(&exe_dir), "Failed to get exe directory"); - // Set the cwd to the exe dir and pass the filenames to scp. Otherwise, some - // scp implementations can get confused and create the wrong remote filenames. - path::SetCwd(exe_dir); + // Create the remote tools bin dir if it doesn't exist yet. + // This assumes that sftp's remote startup directory is the home directory. + std::vector dir_parts = + absl::StrSplit(kRemoteToolsBinDir, '/', absl::SkipEmpty()); + std::string sftp_commands; + for (const std::string& dir : dir_parts) { + // Use -mkdir to ignore errors if the directory already exists. + sftp_commands += absl::StrFormat("-mkdir %s\ncd %s\n", dir, dir); + } - // Copy FUSE to the gamelet. - LOG_DEBUG("Copying FUSE"); - RETURN_IF_ERROR(remote_util_->Scp({kFuseFilename, kLibFuseFilename}, - kRemoteToolsBinDir, /*compress=*/false), - "Failed to copy FUSE to gamelet"); - LOG_DEBUG("Copying FUSE succeeded"); + sftp_commands += absl::StrFormat("put %s\n", kFuseFilename); + sftp_commands += absl::StrFormat("put %s\n", kLibFuseFilename); + sftp_commands += absl::StrFormat("chmod 755 %s\n", kFuseFilename); - // Make FUSE executable. Note that sync does it automatically. - LOG_DEBUG("Making FUSE executable"); - std::string remotePath = path::JoinUnix(kRemoteToolsBinDir, kFuseFilename); - RETURN_IF_ERROR(remote_util_->Chmod("a+x", remotePath), - "Failed to set executable flag on FUSE"); - LOG_DEBUG("Making FUSE succeeded"); + LOG_DEBUG("Deploying FUSE"); + RETURN_IF_ERROR( + remote_util_->Sftp(sftp_commands, exe_dir, /*compress=*/false), + "Failed to deploy FUSE"); + LOG_DEBUG("Deploying FUSE succeeded"); return absl::OkStatus(); } @@ -104,14 +107,13 @@ absl::Status CdcFuseManager::Start(const std::string& mount_dir, // Build the remote command. std::string remotePath = path::JoinUnix(kRemoteToolsBinDir, kFuseFilename); std::string remote_command = absl::StrFormat( - "mkdir -p %s; LD_LIBRARY_PATH=%s %s " + "LD_LIBRARY_PATH=%s %s " "--instance=%s " "--components=%s --port=%i --cache_dir=%s " "--verbosity=%i --cleanup_timeout=%i --access_idle_timeout=%i --stats=%i " "--check=%i --cache_capacity=%u -- -o allow_root -o ro -o nonempty -o " "auto_unmount %s%s%s", - kRemoteToolsBinDir, kRemoteToolsBinDir, remotePath, - RemoteUtil::QuoteForSsh(instance_), + kRemoteToolsBinDir, remotePath, RemoteUtil::QuoteForSsh(instance_), RemoteUtil::QuoteForSsh(component_args), remote_port, kCacheDir, verbosity, cleanup_timeout_sec, access_idle_timeout_sec, enable_stats, check, cache_capacity, debug ? "-d " : "", singlethreaded ? "-s " : "", diff --git a/cdc_stream/local_assets_stream_manager_client.cc b/cdc_stream/local_assets_stream_manager_client.cc index 181cb35..e816d51 100644 --- a/cdc_stream/local_assets_stream_manager_client.cc +++ b/cdc_stream/local_assets_stream_manager_client.cc @@ -38,13 +38,13 @@ LocalAssetsStreamManagerClient::~LocalAssetsStreamManagerClient() = default; absl::Status LocalAssetsStreamManagerClient::StartSession( const std::string& src_dir, const std::string& user_host, const std::string& mount_dir, const std::string& ssh_command, - const std::string& scp_command) { + const std::string& sftp_command) { StartSessionRequest request; request.set_workstation_directory(src_dir); request.set_user_host(user_host); request.set_mount_dir(mount_dir); request.set_ssh_command(ssh_command); - request.set_scp_command(scp_command); + request.set_sftp_command(sftp_command); grpc::ClientContext context; StartSessionResponse response; diff --git a/cdc_stream/local_assets_stream_manager_client.h b/cdc_stream/local_assets_stream_manager_client.h index 15dfdeb..b92fab1 100644 --- a/cdc_stream/local_assets_stream_manager_client.h +++ b/cdc_stream/local_assets_stream_manager_client.h @@ -43,12 +43,12 @@ class LocalAssetsStreamManagerClient { // |user_host| is the Linux host, formatted as [user@:host]. // |mount_dir| is the Linux target directory to stream to. // |ssh_command| is the ssh command and extra arguments to use. - // |scp_command| is the scp command and extra arguments to use. + // |sftp_command| is the sftp command and extra arguments to use. absl::Status StartSession(const std::string& src_dir, const std::string& user_host, const std::string& mount_dir, const std::string& ssh_command, - const std::string& scp_command); + const std::string& sftp_command); // Stops the streaming session to the Linux target |user_host|:|mount_dir|. // |user_host| is the Linux host, formatted as [user@:host]. diff --git a/cdc_stream/local_assets_stream_manager_service_impl.cc b/cdc_stream/local_assets_stream_manager_service_impl.cc index c4e033b..ec55f37 100644 --- a/cdc_stream/local_assets_stream_manager_service_impl.cc +++ b/cdc_stream/local_assets_stream_manager_service_impl.cc @@ -208,7 +208,7 @@ LocalAssetsStreamManagerServiceImpl::GetTargetForStadia( SessionTarget target; target.mount_dir = request.mount_dir(); target.ssh_command = request.ssh_command(); - target.scp_command = request.scp_command(); + target.sftp_command = request.sftp_command(); // Parse instance/project/org id. if (!ParseInstanceName(request.gamelet_name(), instance_id, project_id, @@ -223,7 +223,7 @@ LocalAssetsStreamManagerServiceImpl::GetTargetForStadia( InitSsh(*instance_id, *project_id, *organization_id)); target.user_host = "cloudcast@" + instance_ip; - // Note: Port must be set with ssh_command (-p) and scp_command (-P). + // Note: Port must be set with ssh_command (-p) and sftp_command (-P). return target; } @@ -233,7 +233,7 @@ SessionTarget LocalAssetsStreamManagerServiceImpl::GetTarget( target.user_host = request.user_host(); target.mount_dir = request.mount_dir(); target.ssh_command = request.ssh_command(); - target.scp_command = request.scp_command(); + target.sftp_command = request.sftp_command(); *instance_id = absl::StrCat(target.user_host, ":", target.mount_dir); return target; diff --git a/cdc_stream/session.cc b/cdc_stream/session.cc index e6c0cd9..e1237c4 100644 --- a/cdc_stream/session.cc +++ b/cdc_stream/session.cc @@ -55,8 +55,8 @@ Session::Session(std::string instance_id, const SessionTarget& target, if (!target.ssh_command.empty()) { remote_util_.SetSshCommand(target.ssh_command); } - if (!target.scp_command.empty()) { - remote_util_.SetScpCommand(target.scp_command); + if (!target.sftp_command.empty()) { + remote_util_.SetSftpCommand(target.sftp_command); } } diff --git a/cdc_stream/session.h b/cdc_stream/session.h index 14e6695..5db3733 100644 --- a/cdc_stream/session.h +++ b/cdc_stream/session.h @@ -38,8 +38,8 @@ struct SessionTarget { std::string user_host; // Ssh command to use to connect to the remote target. std::string ssh_command; - // Scp command to use to copy files to the remote target. - std::string scp_command; + // Sftp command to use to copy files to the remote target. + std::string sftp_command; // Directory on the remote target where to mount the streamed directory. std::string mount_dir; }; diff --git a/cdc_stream/start_command.cc b/cdc_stream/start_command.cc index 0fdd19c..c5559d2 100644 --- a/cdc_stream/start_command.cc +++ b/cdc_stream/start_command.cc @@ -22,6 +22,7 @@ #include "common/log.h" #include "common/path.h" #include "common/process.h" +#include "common/remote_util.h" #include "common/status_macros.h" #include "common/stopwatch.h" #include "common/util.h" @@ -73,17 +74,28 @@ void StartCommand::RegisterCommandLineFlags(lyra::command& cmd) { path::GetEnv("CDC_SSH_COMMAND", &ssh_command_).IgnoreError(); cmd.add_argument( - lyra::opt(ssh_command_, "ssh_command") + lyra::opt(ssh_command_, "cmd") .name("--ssh-command") .help("Path and arguments of ssh command to use, e.g. " "\"C:\\path\\to\\ssh.exe -F config_file -p 1234\". Can also be " "specified by the CDC_SSH_COMMAND environment variable.")); - path::GetEnv("CDC_SCP_COMMAND", &scp_command_).IgnoreError(); + path::GetEnv("CDC_SFTP_COMMAND", &sftp_command_).IgnoreError(); cmd.add_argument( - lyra::opt(scp_command_, "scp_command") + lyra::opt(sftp_command_, "cmd") + .name("--sftp-command") + .help( + "Path and arguments of sftp command to use, e.g. " + "\"C:\\path\\to\\sftp.exe -F config_file -P 1234\". Can also be " + "specified by the CDC_SFTP_COMMAND environment variable.")); + + path::GetEnv("CDC_SCP_COMMAND", &deprecated_scp_command_).IgnoreError(); + cmd.add_argument( + lyra::opt(deprecated_scp_command_, "cmd") .name("--scp-command") - .help("Path and arguments of scp command to use, e.g. " + .help("[Deprecated, use --sftp-command] Path and arguments of scp " + "command to " + "use, e.g. " "\"C:\\path\\to\\scp.exe -F config_file -P 1234\". Can also be " "specified by the CDC_SCP_COMMAND environment variable.")); @@ -106,9 +118,26 @@ absl::Status StartCommand::Run() { RETURN_IF_ERROR(LocalAssetsStreamManagerClient::ParseUserHostDir( user_host_dir_, &user_host, &mount_dir)); + // Backwards compatibility after switching from scp to sftp. + if (sftp_command_.empty() && !deprecated_scp_command_.empty()) { + LOG_WARNING( + "The CDC_SCP_COMMAND environment variable and the --scp-command flag " + "are deprecated. Please set CDC_SFTP_COMMAND or --sftp-command " + "instead."); + + sftp_command_ = RemoteUtil::ScpToSftpCommand(deprecated_scp_command_); + if (!sftp_command_.empty()) { + LOG_WARNING("Converted scp command '%s' to sftp command '%s'.", + deprecated_scp_command_, sftp_command_); + } else { + LOG_WARNING("Failed to convert scp command '%s' to sftp command.", + deprecated_scp_command_); + } + } + LocalAssetsStreamManagerClient client(CreateChannel(service_port_)); absl::Status status = client.StartSession(full_src_dir, user_host, mount_dir, - ssh_command_, scp_command_); + ssh_command_, sftp_command_); if (absl::IsUnavailable(status)) { LOG_DEBUG("StartSession status: %s", status.ToString()); @@ -122,7 +151,7 @@ absl::Status StartCommand::Run() { // state. LocalAssetsStreamManagerClient new_client(CreateChannel(service_port_)); status = new_client.StartSession(full_src_dir, user_host, mount_dir, - ssh_command_, scp_command_); + ssh_command_, sftp_command_); } } diff --git a/cdc_stream/start_command.h b/cdc_stream/start_command.h index a4eb01f..2ed4070 100644 --- a/cdc_stream/start_command.h +++ b/cdc_stream/start_command.h @@ -44,9 +44,11 @@ class StartCommand : public BaseCommand { int verbosity_ = 0; uint16_t service_port_ = 0; std::string ssh_command_; - std::string scp_command_; + std::string sftp_command_; std::string src_dir_; std::string user_host_dir_; + + std::string deprecated_scp_command_; }; } // namespace cdc_ft diff --git a/cdc_stream/start_service_command.cc b/cdc_stream/start_service_command.cc index 5035143..cd54369 100644 --- a/cdc_stream/start_service_command.cc +++ b/cdc_stream/start_service_command.cc @@ -141,7 +141,7 @@ absl::Status StartServiceCommand::RunService() { request.set_user_host(cfg_.dev_target().user_host); request.set_mount_dir(cfg_.dev_target().mount_dir); request.set_ssh_command(cfg_.dev_target().ssh_command); - request.set_scp_command(cfg_.dev_target().scp_command); + request.set_sftp_command(cfg_.dev_target().sftp_command); localassetsstreammanager::StartSessionResponse response; RETURN_ABSL_IF_ERROR( session_service.StartSession(nullptr, &request, &response)); diff --git a/proto/local_assets_stream_manager.proto b/proto/local_assets_stream_manager.proto index 4e4b55f..a23a431 100644 --- a/proto/local_assets_stream_manager.proto +++ b/proto/local_assets_stream_manager.proto @@ -48,9 +48,9 @@ message StartSessionRequest { // SSH command to connect to the remote instance. // Optional, falls back to searching ssh. string ssh_command = 10; - // SCP command to copy files to the remote instance. - // Optional, falls back to searching scp. - string scp_command = 11; + // SFTP command to copy files to the remote instance. + // Optional, falls back to searching sftp. + string sftp_command = 11; reserved 1, 3, 4, 9; }