fix: cleanup after upload of broken plugin

apps/emqx_management/src/emqx_mgmt_api_plugins.erl

@@ -17,7 +17,6 @@
 
 -behaviour(minirest_api).
 
--include_lib("kernel/include/file.hrl").
 -include_lib("typerefl/include/types.hrl").
 -include_lib("emqx/include/logger.hrl").
 %%-include_lib("emqx_plugins/include/emqx_plugins.hrl").
@@ -326,7 +325,8 @@ upload_install(post, #{body := #{<<"plugin">> := Plugin}}) when is_map(Plugin) -
     %% File bin is too large, we use rpc:multicall instead of cluster_rpc:multicall
     %% TODO what happens when a new node join in?
     %% emqx_plugins_monitor should copy plugins from other core node when boot-up.
-    case emqx_plugins:describe(string:trim(FileName, trailing, ".tar.gz")) of
+    NameVsn = string:trim(FileName, trailing, ".tar.gz"),
+    case emqx_plugins:describe(NameVsn) of
         {error, #{error := "bad_info_file", return := {enoent, _}}} ->
             case emqx_plugins:parse_name_vsn(FileName) of
                 {ok, AppName, _Vsn} ->
@@ -346,6 +346,7 @@ upload_install(post, #{body := #{<<"plugin">> := Plugin}}) when is_map(Plugin) -
                             }}
                     end;
                 {error, Reason} ->
+                    emqx_plugins:delete_package(NameVsn),
                     {400, #{
                         code => 'BAD_PLUGIN_INFO',
                         message => iolist_to_binary([Reason, ":", FileName])
@@ -367,9 +368,24 @@ upload_install(post, #{}) ->
 do_install_package(FileName, Bin) ->
     %% TODO: handle bad nodes
     {[_ | _] = Res, []} = emqx_mgmt_api_plugins_proto_v1:install_package(FileName, Bin),
-    %% TODO: handle non-OKs
-    [] = lists:filter(fun(R) -> R =/= ok end, Res),
-    {200}.
+    case lists:filter(fun(R) -> R =/= ok end, Res) of
+        [] ->
+            {200};
+        Filtered ->
+            %% crash if we have unexpected errors or results
+            [] = lists:filter(
+                fun
+                    ({error, {failed, _}}) -> true;
+                    ({error, _}) -> false
+                end,
+                Filtered
+            ),
+            {error, #{error := Reason}} = hd(Filtered),
+            {400, #{
+                code => 'BAD_PLUGIN_INFO',
+                message => iolist_to_binary([Reason, ":", FileName])
+            }}
+    end.
 
 plugin(get, #{bindings := #{name := Name}}) ->
     {Plugins, _} = emqx_mgmt_api_plugins_proto_v1:describe_package(Name),
@@ -408,7 +424,15 @@ install_package(FileName, Bin) ->
     File = filename:join(emqx_plugins:install_dir(), FileName),
     ok = file:write_file(File, Bin),
     PackageName = string:trim(FileName, trailing, ".tar.gz"),
-    emqx_plugins:ensure_installed(PackageName).
+    case emqx_plugins:ensure_installed(PackageName) of
+        {error, #{return := not_found}} = NotFound ->
+            NotFound;
+        {error, _Reason} = Error ->
+            _ = file:delete(File),
+            Error;
+        Result ->
+            Result
+    end.
 
 %% For RPC plugin get
 describe_package(Name) ->

apps/emqx_plugins/src/emqx_plugins.app.src

@@ -1,7 +1,7 @@
 %% -*- mode: erlang -*-
 {application, emqx_plugins, [
     {description, "EMQX Plugin Management"},
-    {vsn, "0.1.0"},
+    {vsn, "0.1.1"},
     {modules, []},
     {mod, {emqx_plugins_app, []}},
     {applications, [kernel, stdlib, emqx]},

apps/emqx_plugins/src/emqx_plugins.erl

@@ -16,8 +16,13 @@
 
 -module(emqx_plugins).
 
--include_lib("emqx/include/emqx.hrl").
 -include_lib("emqx/include/logger.hrl").
+-include_lib("emqx/include/logger.hrl").
+-include("emqx_plugins.hrl").
+
+-ifdef(TEST).
+-include_lib("eunit/include/eunit.hrl").
+-endif.
 
 -export([
     ensure_installed/1,
@@ -56,10 +61,6 @@
 -compile(nowarn_export_all).
 -endif.
 
--include_lib("emqx/include/emqx.hrl").
--include_lib("emqx/include/logger.hrl").
--include("emqx_plugins.hrl").
-
 %% "my_plugin-0.1.0"
 -type name_vsn() :: binary() | string().
 %% the parse result of the JSON info file
@@ -87,14 +88,15 @@ ensure_installed(NameVsn) ->
 
 do_ensure_installed(NameVsn) ->
     TarGz = pkg_file(NameVsn),
-    case erl_tar:extract(TarGz, [{cwd, install_dir()}, compressed]) of
-        ok ->
+    case erl_tar:extract(TarGz, [compressed, memory]) of
+        {ok, TarContent} ->
+            ok = write_tar_file_content(install_dir(), TarContent),
             case read_plugin(NameVsn, #{}) of
                 {ok, _} ->
                     ok;
                 {error, Reason} ->
                     ?SLOG(warning, Reason#{msg => "failed_to_read_after_install"}),
-                    _ = ensure_uninstalled(NameVsn),
+                    ok = delete_tar_file_content(install_dir(), TarContent),
                     {error, Reason}
             end;
         {error, {_, enoent}} ->
@@ -111,6 +113,66 @@ do_ensure_installed(NameVsn) ->
             }}
     end.
 
+write_tar_file_content(BaseDir, TarContent) ->
+    lists:foreach(
+        fun({Name, Bin}) ->
+            Filename = filename:join(BaseDir, Name),
+            ok = filelib:ensure_dir(Filename),
+            ok = file:write_file(Filename, Bin)
+        end,
+        TarContent
+    ).
+
+delete_tar_file_content(BaseDir, TarContent) ->
+    lists:foreach(
+        fun({Name, _}) ->
+            Filename = filename:join(BaseDir, Name),
+            case filelib:is_file(Filename) of
+                true ->
+                    TopDirOrFile = top_dir(BaseDir, Filename),
+                    ok = file:del_dir_r(TopDirOrFile);
+                false ->
+                    %% probably already deleted
+                    ok
+            end
+        end,
+        TarContent
+    ).
+
+top_dir(BaseDir0, DirOrFile) ->
+    BaseDir = normalize_dir(BaseDir0),
+    case filename:dirname(DirOrFile) of
+        RockBottom when RockBottom =:= "/" orelse RockBottom =:= "." ->
+            throw({out_of_bounds, DirOrFile});
+        BaseDir ->
+            DirOrFile;
+        Parent ->
+            top_dir(BaseDir, Parent)
+    end.
+
+normalize_dir(Dir) ->
+    %% Get rid of possible trailing slash
+    filename:join([Dir, ""]).
+
+-ifdef(TEST).
+normalize_dir_test_() ->
+    [
+        ?_assertEqual("foo", normalize_dir("foo")),
+        ?_assertEqual("foo", normalize_dir("foo/")),
+        ?_assertEqual("/foo", normalize_dir("/foo")),
+        ?_assertEqual("/foo", normalize_dir("/foo/"))
+    ].
+
+top_dir_test_() ->
+    [
+        ?_assertEqual("base/foo", top_dir("base", filename:join(["base", "foo", "bar"]))),
+        ?_assertEqual("/base/foo", top_dir("/base", filename:join(["/", "base", "foo", "bar"]))),
+        ?_assertEqual("/base/foo", top_dir("/base/", filename:join(["/", "base", "foo", "bar"]))),
+        ?_assertThrow({out_of_bounds, _}, top_dir("/base", filename:join(["/", "base"]))),
+        ?_assertThrow({out_of_bounds, _}, top_dir("/base", filename:join(["/", "foo", "bar"])))
+    ].
+-endif.
+
 %% @doc Ensure files and directories for the given plugin are being deleted.
 %% If a plugin is running, or enabled, an error is returned.
 -spec ensure_uninstalled(name_vsn()) -> ok | {error, any()}.