From 4a960bd1ec6e4b749cf7bd28a3df2d3c18b04ab7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Kn=C3=B6pfle?= Date: Wed, 14 Oct 2020 19:05:36 +0200 Subject: [PATCH 1/2] Better error handling for scp uploads --- lib/sshkit/scp/upload.ex | 10 ++++++++++ test/sshkit_functional_test.exs | 22 ++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/lib/sshkit/scp/upload.ex b/lib/sshkit/scp/upload.ex index f48059b..bc88658 100644 --- a/lib/sshkit/scp/upload.ex +++ b/lib/sshkit/scp/upload.ex @@ -107,6 +107,9 @@ defmodule SSHKit.SCP.Upload do {:data, _, 0, data} -> handle_error_data(state, options, data) + {:data, _, 1, data} -> + fatal(options, state, data) + {:exit_status, _, status} -> exited(options, state, status) @@ -223,6 +226,13 @@ defmodule SSHKit.SCP.Upload do end end + # There are warnings that we don't understand correctly. If that is the case + # we treat them as errors. + # For example a permission denied error will be treated as such. + defp warning(options, state, buffer) do + fatal(options, state, buffer) + end + defp fatal(_, state, buffer) do if String.last(buffer) == "\n" do {:halt, {:error, String.trim(buffer)}} diff --git a/test/sshkit_functional_test.exs b/test/sshkit_functional_test.exs index 4027c48..bc335b5 100644 --- a/test/sshkit_functional_test.exs +++ b/test/sshkit_functional_test.exs @@ -159,6 +159,28 @@ defmodule SSHKitFunctionalTest do assert verify_transfer(context, local, Path.basename(local)) end + test "uploads a file to a directory that does not exist", %{hosts: hosts} do + local = "test/fixtures/local.txt" + + context = + hosts + |> SSHKit.context() + |> SSHKit.path("/otp/releases") + + assert [error: _, error: _] = SSHKit.upload(context, local) + end + + test "uploads a file to a directory we have no access to", %{hosts: hosts} do + local = "test/fixtures/local.txt" + + context = + hosts + |> SSHKit.context() + |> SSHKit.path("/") + + assert [error: _, error: _] = SSHKit.upload(context, local) + end + test "recursive: true", %{hosts: [host | _] = hosts} do local = "test/fixtures" remote = "/home/#{host.options[:user]}/fixtures" From e3c6d446e71137028ca4d3a6d502f3361af4bfa7 Mon Sep 17 00:00:00 2001 From: Max Mulatz Date: Wed, 11 Nov 2020 19:49:55 +0100 Subject: [PATCH 2/2] Update handling upload warnings Continue to collect warnings instead of stopping at the first. --- lib/sshkit/scp/upload.ex | 15 ++++++--------- test/sshkit/scp/upload_test.exs | 29 +++++++++++++++++++++++++++-- test/sshkit/scp_functional_test.exs | 11 +++++++++++ test/sshkit_functional_test.exs | 10 ++++++++-- 4 files changed, 52 insertions(+), 13 deletions(-) diff --git a/lib/sshkit/scp/upload.ex b/lib/sshkit/scp/upload.ex index bc88658..e7830a7 100644 --- a/lib/sshkit/scp/upload.ex +++ b/lib/sshkit/scp/upload.ex @@ -218,21 +218,18 @@ defmodule SSHKit.SCP.Upload do {:cont, {:error, "SCP channel closed before completing the transfer"}} end - defp warning(_, state = {name, cwd, stack, errs}, buffer) do + defp warning(options, {name, _file, _stat, cwd, stack, errs}, buffer) do + warning(options, {name, cwd, stack, errs}, buffer) + end + + defp warning(options, state = {_name, cwd, stack, errs}, buffer) do if String.last(buffer) == "\n" do - {:cont, {name, cwd, stack, errs ++ [String.trim(buffer)]}} + next(options, cwd, stack, errs ++ [String.trim(buffer)]) else {:cont, {:warning, state, buffer}} end end - # There are warnings that we don't understand correctly. If that is the case - # we treat them as errors. - # For example a permission denied error will be treated as such. - defp warning(options, state, buffer) do - fatal(options, state, buffer) - end - defp fatal(_, state, buffer) do if String.last(buffer) == "\n" do {:halt, {:error, String.trim(buffer)}} diff --git a/test/sshkit/scp/upload_test.exs b/test/sshkit/scp/upload_test.exs index 8ac5ff2..593250e 100644 --- a/test/sshkit/scp/upload_test.exs +++ b/test/sshkit/scp/upload_test.exs @@ -155,7 +155,6 @@ defmodule SSHKit.SCP.UploadTest do channel: channel } do error_msg = "error part 1 error part 2 error part 3" - {name, cwd, stack, _errs} = state msg1 = {:data, channel, 0, <<1, "error part 1 ">>} state1 = {:warning, state, "error part 1 "} @@ -166,7 +165,33 @@ defmodule SSHKit.SCP.UploadTest do assert {:cont, state2} == handler.(msg2, state1) msg3 = {:data, channel, 0, <<"error part 3\n">>} - assert {:cont, {name, cwd, stack, [error_msg]}} == handler.(msg3, state2) + assert {:cont, _directive, {_name, _cwd, _stack, [^error_msg]}} = handler.(msg3, state2) + end + + test "proceeds with next file in stack after warning", %{ + upload: %Upload{handler: handler, state: state}, + channel: channel + } do + {name, cwd, _stack, _errs} = state + msg = {:data, channel, 0, <<1, "error message\n">>} + + assert {:cont, 'D0755 0 local_dir\n', + {name, cwd <> "/local_dir", [["other.txt"], []], ["error message"]}} == + handler.(msg, state) + end + + test "collects warning when in write state", %{ + upload: %Upload{handler: handler, state: state}, + channel: channel + } do + {name, cwd, stack, _errs} = state + + state = {:write, "local.txt", %{}, cwd, stack, []} + msg = {:data, channel, 0, <<1, "error message\n">>} + + assert {:cont, 'D0755 0 local_dir\n', + {name, cwd <> "/local_dir", [["other.txt"], []], ["error message"]}} == + handler.(msg, state) end test "aggregates connection errors in the state and halts", %{ diff --git a/test/sshkit/scp_functional_test.exs b/test/sshkit/scp_functional_test.exs index 4ded22d..56acdc9 100644 --- a/test/sshkit/scp_functional_test.exs +++ b/test/sshkit/scp_functional_test.exs @@ -52,6 +52,17 @@ defmodule SSHKit.SCPFunctionalTest do assert verify_mtime(conn, local, remote) end) end + + @tag boot: [@bootconf] + test "uploads to nonexistent target directory (recursive: true)", %{hosts: [host]} do + local = "test/fixtures/local_dir" + remote = "/some/nonexistent/destination" + + SSH.connect(host.name, host.options, fn conn -> + assert {:error, msg} = SCP.upload(conn, local, remote, recursive: true) + assert msg =~ "scp: /some/nonexistent/destination: No such file or directory" + end) + end end describe "download/4" do diff --git a/test/sshkit_functional_test.exs b/test/sshkit_functional_test.exs index bc335b5..244720b 100644 --- a/test/sshkit_functional_test.exs +++ b/test/sshkit_functional_test.exs @@ -167,7 +167,10 @@ defmodule SSHKitFunctionalTest do |> SSHKit.context() |> SSHKit.path("/otp/releases") - assert [error: _, error: _] = SSHKit.upload(context, local) + assert [ + error: "sh: cd: line 1: can't cd to /otp/releases", + error: "sh: cd: line 1: can't cd to /otp/releases" + ] = SSHKit.upload(context, local) end test "uploads a file to a directory we have no access to", %{hosts: hosts} do @@ -178,7 +181,10 @@ defmodule SSHKitFunctionalTest do |> SSHKit.context() |> SSHKit.path("/") - assert [error: _, error: _] = SSHKit.upload(context, local) + assert [ + error: "SCP exited with non-zero exit code 1: scp: local.txt: Permission denied", + error: "SCP exited with non-zero exit code 1: scp: local.txt: Permission denied" + ] = SSHKit.upload(context, local) end test "recursive: true", %{hosts: [host | _] = hosts} do