From 30a5cfaa83ac5e90c1ce2a80a43a22c2f6dcee78 Mon Sep 17 00:00:00 2001 From: Erik Timan Date: Tue, 10 Jan 2023 15:58:34 +0100 Subject: [PATCH] fix(emqx_management): remove trace files after zip download We only deleted the resulting zip after a trace file download, not the actual trace files. This adds a deletion of the uncompressed trace files as well. It also creates unique directories when collecting trace files so that concurrent downloads doesn't overwrite files in transit. --- .../src/emqx_mgmt_api_trace.erl | 25 ++++++++--- .../test/emqx_mgmt_api_trace_SUITE.erl | 44 ++++++++++++++++++- 2 files changed, 61 insertions(+), 8 deletions(-) diff --git a/apps/emqx_management/src/emqx_mgmt_api_trace.erl b/apps/emqx_management/src/emqx_mgmt_api_trace.erl index d90aea9ef..5ad04a68c 100644 --- a/apps/emqx_management/src/emqx_mgmt_api_trace.erl +++ b/apps/emqx_management/src/emqx_mgmt_api_trace.erl @@ -20,6 +20,7 @@ -include_lib("kernel/include/file.hrl"). -include_lib("typerefl/include/types.hrl"). -include_lib("emqx/include/logger.hrl"). +-include_lib("snabbkaffe/include/snabbkaffe.hrl"). -export([ api_spec/0, @@ -461,16 +462,26 @@ download_trace_log(get, #{bindings := #{name := Name}, query_string := Query}) - case parse_node(Query, undefined) of {ok, Node} -> TraceFiles = collect_trace_file(Node, TraceLog), - ZipDir = emqx_trace:zip_dir(), + %% We generate a session ID so that we name files + %% with unique names. Then we won't cause + %% overwrites for concurrent requests. + SessionId = emqx_misc:gen_id(), + ZipDir = filename:join([emqx_trace:zip_dir(), SessionId]), + ok = file:make_dir(ZipDir), + %% Write files to ZipDir and create an in-memory zip file Zips = group_trace_file(ZipDir, TraceLog, TraceFiles), - FileName = binary_to_list(Name) ++ ".zip", - ZipFileName = filename:join([ZipDir, FileName]), - {ok, ZipFile} = zip:zip(ZipFileName, Zips, [{cwd, ZipDir}]), + ZipName = binary_to_list(Name) ++ ".zip", + {ok, {ZipName, Binary}} = zip:zip(ZipName, Zips, [memory, {cwd, ZipDir}]), %% emqx_trace:delete_files_after_send(ZipFileName, Zips), %% TODO use file replace file_binary.(delete file after send is not ready now). - {ok, Binary} = file:read_file(ZipFile), - ZipName = filename:basename(ZipFile), - _ = file:delete(ZipFile), + ok = file:del_dir_r(ZipDir), + ?tp(trace_api_download_trace_log, #{ + files => Zips, + name => Name, + session_id => SessionId, + zip_dir => ZipDir, + zip_name => ZipName + }), Headers = #{ <<"content-type">> => <<"application/x-zip">>, <<"content-disposition">> => iolist_to_binary( diff --git a/apps/emqx_management/test/emqx_mgmt_api_trace_SUITE.erl b/apps/emqx_management/test/emqx_mgmt_api_trace_SUITE.erl index 33240b64f..6962a9043 100644 --- a/apps/emqx_management/test/emqx_mgmt_api_trace_SUITE.erl +++ b/apps/emqx_management/test/emqx_mgmt_api_trace_SUITE.erl @@ -168,7 +168,8 @@ t_create_failed(_Config) -> ), %% clear ?assertMatch({ok, _}, request_api(delete, api_path("trace"), [])), - {ok, Create} = request_api(post, api_path("trace"), [GoodName | Trace]), + {ok, Create1} = request_api(post, api_path("trace"), [GoodName | Trace]), + ?assertMatch(#{<<"name">> := <<"test-name-0">>}, json(Create1)), %% new name but same trace GoodName2 = {<<"name">>, <<"test-name-1">>}, ?assertMatch( @@ -314,6 +315,47 @@ t_stream_log(_Config) -> unload(), ok. +t_trace_files_are_deleted_after_download(_Config) -> + ClientId = <<"client-test-delete-after-download">>, + Now = erlang:system_time(second), + Name = <<"test_client_id">>, + load(), + create_trace(Name, ClientId, Now), + {ok, Client} = emqtt:start_link([{clean_start, true}, {clientid, ClientId}]), + {ok, _} = emqtt:connect(Client), + [ + begin + _ = emqtt:ping(Client) + end + || _ <- lists:seq(1, 5) + ], + ok = emqtt:disconnect(Client), + ok = emqx_trace_handler_SUITE:filesync(Name, clientid), + + %% Check that files have been removed after download and that zip + %% directories uses unique session ids + ?check_trace( + begin + %% Download two zip files + Path = api_path(["trace/", binary_to_list(Name), "/download"]), + {ok, Binary1} = request_api(get, Path), + {ok, Binary2} = request_api(get, Path), + ?assertMatch({ok, _}, zip:table(Binary1)), + ?assertMatch({ok, _}, zip:table(Binary2)) + end, + fun(Trace) -> + [ + #{session_id := SessionId1, zip_dir := ZipDir1}, + #{session_id := SessionId2, zip_dir := ZipDir2} + ] = ?of_kind(trace_api_download_trace_log, Trace), + ?assertEqual({error, enoent}, file:list_dir(ZipDir1)), + ?assertEqual({error, enoent}, file:list_dir(ZipDir2)), + ?assertNotEqual(SessionId1, SessionId2), + ?assertNotEqual(ZipDir1, ZipDir2) + end + ), + ok. + to_rfc3339(Second) -> list_to_binary(calendar:system_time_to_rfc3339(Second)).