From 997a262c96d91c538a82786a80ebb3514b4dcac3 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Fri, 21 Oct 2022 15:57:30 -0300 Subject: [PATCH 1/4] fix(mgmt_api): return 503 when emqx is not running in `/status` (5.0) --- .../src/emqx_mgmt_api_status.erl | 7 +- .../test/emqx_mgmt_api_status_SUITE.erl | 88 ++++++++++++++++++- changes/v5.0.10-en.md | 2 + changes/v5.0.10-zh.md | 2 + 4 files changed, 94 insertions(+), 5 deletions(-) diff --git a/apps/emqx_management/src/emqx_mgmt_api_status.erl b/apps/emqx_management/src/emqx_mgmt_api_status.erl index 68bad03e4..37a8274f8 100644 --- a/apps/emqx_management/src/emqx_mgmt_api_status.erl +++ b/apps/emqx_management/src/emqx_mgmt_api_status.erl @@ -45,7 +45,12 @@ running_status() -> BrokerStatus = broker_status(), AppStatus = application_status(), Body = io_lib:format("Node ~ts is ~ts~nemqx is ~ts", [node(), BrokerStatus, AppStatus]), - {200, #{<<"content-type">> => <<"text/plain">>}, list_to_binary(Body)}; + StatusCode = + case AppStatus of + running -> 200; + not_running -> 503 + end, + {StatusCode, #{<<"content-type">> => <<"text/plain">>}, list_to_binary(Body)}; false -> {503, #{<<"retry-after">> => <<"15">>}, <<>>} end. diff --git a/apps/emqx_management/test/emqx_mgmt_api_status_SUITE.erl b/apps/emqx_management/test/emqx_mgmt_api_status_SUITE.erl index b725e37b2..4051bd62c 100644 --- a/apps/emqx_management/test/emqx_mgmt_api_status_SUITE.erl +++ b/apps/emqx_management/test/emqx_mgmt_api_status_SUITE.erl @@ -20,6 +20,12 @@ -include_lib("eunit/include/eunit.hrl"). +-define(HOST, "http://127.0.0.1:18083/"). + +%%--------------------------------------------------------------------------------------- +%% CT boilerplate +%%--------------------------------------------------------------------------------------- + all() -> emqx_common_test_helpers:all(?MODULE). @@ -30,8 +36,82 @@ init_per_suite(Config) -> end_per_suite(_) -> emqx_mgmt_api_test_util:end_suite(). -t_status(_Config) -> - Path = emqx_mgmt_api_test_util:api_path_without_base_path(["/status"]), - Status = io_lib:format("Node ~ts is ~ts~nemqx is ~ts", [node(), started, running]), - {ok, Status} = emqx_mgmt_api_test_util:request_api(get, Path), +init_per_testcase(t_status_not_ok, Config) -> + ok = application:stop(emqx), + Config; +init_per_testcase(_TestCase, Config) -> + Config. + +end_per_testcase(t_status_not_ok, _Config) -> + {ok, _} = application:ensure_all_started(emqx), + ok; +end_per_testcase(_TestCase, _Config) -> + ok. + +%%--------------------------------------------------------------------------------------- +%% Helper fns +%%--------------------------------------------------------------------------------------- + +do_request(Opts) -> + #{ + path := Path, + method := Method, + headers := Headers, + body := Body0 + } = Opts, + URL = ?HOST ++ filename:join(Path), + Request = + case Body0 of + no_body -> {URL, Headers}; + {Encoding, Body} -> {URL, Headers, Encoding, Body} + end, + ct:pal("Method: ~p, Request: ~p", [Method, Request]), + case httpc:request(Method, Request, [], []) of + {error, socket_closed_remotely} -> + {error, socket_closed_remotely}; + {ok, {{_, StatusCode, _}, Headers1, Body1}} -> + Body2 = + case emqx_json:safe_decode(Body1, [return_maps]) of + {ok, Json} -> Json; + {error, _} -> Body1 + end, + {ok, #{status_code => StatusCode, headers => Headers1, body => Body2}} + end. + +%%--------------------------------------------------------------------------------------- +%% Test cases +%%--------------------------------------------------------------------------------------- + +t_status_ok(_Config) -> + {ok, #{ + body := Resp, + status_code := StatusCode + }} = do_request(#{ + method => get, + path => ["status"], + headers => [], + body => no_body + }), + ?assertEqual(200, StatusCode), + ?assertMatch( + {match, _}, + re:run(Resp, <<"emqx is running$">>) + ), + ok. + +t_status_not_ok(_Config) -> + {ok, #{ + body := Resp, + status_code := StatusCode + }} = do_request(#{ + method => get, + path => ["status"], + headers => [], + body => no_body + }), + ?assertEqual(503, StatusCode), + ?assertMatch( + {match, _}, + re:run(Resp, <<"emqx is not_running$">>) + ), ok. diff --git a/changes/v5.0.10-en.md b/changes/v5.0.10-en.md index a822817d4..52400b5e6 100644 --- a/changes/v5.0.10-en.md +++ b/changes/v5.0.10-en.md @@ -9,3 +9,5 @@ ## Bug fixes - Fix error log message when `mechanism` is missing in authentication config [#8924](https://github.com/emqx/emqx/pull/8924). + +- Fixed the response status code for the `/status` endpoint. Before the fix, it always returned 200 even if the EMQX application was not running. Now it returns 503 in that case. diff --git a/changes/v5.0.10-zh.md b/changes/v5.0.10-zh.md index 0368a6f60..14bb94d1d 100644 --- a/changes/v5.0.10-zh.md +++ b/changes/v5.0.10-zh.md @@ -9,3 +9,5 @@ ## Bug fixes - 优化认认证配置中 `mechanism` 字段缺失情况下的错误日志 [#8924](https://github.com/emqx/emqx/pull/8924)。 + +- 修正了`/status`端点的响应状态代码。 在修复之前，它总是返回200，即使EMQX应用程序没有运行。 现在它在这种情况下返回503。 From 156b19b525e2bd57942dfd5ca3e43081cbaf52e0 Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Tue, 25 Oct 2022 17:24:36 +0200 Subject: [PATCH 2/4] docs: update v5.0.10-zh.md --- changes/v5.0.10-zh.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/changes/v5.0.10-zh.md b/changes/v5.0.10-zh.md index 14bb94d1d..3288efcda 100644 --- a/changes/v5.0.10-zh.md +++ b/changes/v5.0.10-zh.md @@ -10,4 +10,5 @@ - 优化认认证配置中 `mechanism` 字段缺失情况下的错误日志 [#8924](https://github.com/emqx/emqx/pull/8924)。 -- 修正了`/status`端点的响应状态代码。 在修复之前，它总是返回200，即使EMQX应用程序没有运行。 现在它在这种情况下返回503。 +- 修正了 `/status` 端点的响应状态代码 [#9211](https://github.com/emqx/emqx/pull/9211)。 + 在此修复前，它总是返回 HTTP 状态码 `200`，即使 EMQX 没有完成启动或正在重启。 现在它在这些情况下会返回状态码 `503`。 From 79f2d3e9c3e4578225ef93f27aa5752f853b9e9b Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Tue, 25 Oct 2022 17:26:05 +0200 Subject: [PATCH 3/4] docs: update v5.0.10-en.md --- changes/v5.0.10-en.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/changes/v5.0.10-en.md b/changes/v5.0.10-en.md index 52400b5e6..b63cd6e09 100644 --- a/changes/v5.0.10-en.md +++ b/changes/v5.0.10-en.md @@ -10,4 +10,5 @@ - Fix error log message when `mechanism` is missing in authentication config [#8924](https://github.com/emqx/emqx/pull/8924). -- Fixed the response status code for the `/status` endpoint. Before the fix, it always returned 200 even if the EMQX application was not running. Now it returns 503 in that case. +- Fixed the HTTP response status code for the `/status` endpoint [#9211](https://github.com/emqx/emqx/pull/9211). + Before the fix, it always returned `200` even if the EMQX application was not running. Now it returns `503` in that case. From 0eca531e64b22d7ad16ce9b38eb59d2d8ed93b07 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Tue, 25 Oct 2022 15:01:05 -0300 Subject: [PATCH 4/4] feat: add `retry-after` headers to unavailable response --- .../src/emqx_mgmt_api_status.erl | 6 +- .../test/emqx_mgmt_api_status_SUITE.erl | 60 ++++++++++++------- 2 files changed, 45 insertions(+), 21 deletions(-) diff --git a/apps/emqx_management/src/emqx_mgmt_api_status.erl b/apps/emqx_management/src/emqx_mgmt_api_status.erl index 37a8274f8..b313f4bcc 100644 --- a/apps/emqx_management/src/emqx_mgmt_api_status.erl +++ b/apps/emqx_management/src/emqx_mgmt_api_status.erl @@ -50,7 +50,11 @@ running_status() -> running -> 200; not_running -> 503 end, - {StatusCode, #{<<"content-type">> => <<"text/plain">>}, list_to_binary(Body)}; + Headers = #{ + <<"content-type">> => <<"text/plain">>, + <<"retry-after">> => <<"15">> + }, + {StatusCode, Headers, list_to_binary(Body)}; false -> {503, #{<<"retry-after">> => <<"15">>}, <<>>} end. diff --git a/apps/emqx_management/test/emqx_mgmt_api_status_SUITE.erl b/apps/emqx_management/test/emqx_mgmt_api_status_SUITE.erl index 4051bd62c..42d833bda 100644 --- a/apps/emqx_management/test/emqx_mgmt_api_status_SUITE.erl +++ b/apps/emqx_management/test/emqx_mgmt_api_status_SUITE.erl @@ -54,28 +54,43 @@ end_per_testcase(_TestCase, _Config) -> do_request(Opts) -> #{ - path := Path, + path := Path0, method := Method, headers := Headers, body := Body0 } = Opts, - URL = ?HOST ++ filename:join(Path), + URL = ?HOST ++ filename:join(Path0), + {ok, #{host := Host, port := Port, path := Path}} = emqx_http_lib:uri_parse(URL), + %% we must not use `httpc' here, because it keeps retrying when it + %% receives a 503 with `retry-after' header, and there's no option + %% to stop that behavior... + {ok, Gun} = gun:open(Host, Port, #{retry => 0}), + {ok, http} = gun:await_up(Gun), Request = - case Body0 of - no_body -> {URL, Headers}; - {Encoding, Body} -> {URL, Headers, Encoding, Body} + fun() -> + case Body0 of + no_body -> gun:Method(Gun, Path, Headers); + {_Encoding, Body} -> gun:Method(Gun, Path, Headers, Body) + end end, - ct:pal("Method: ~p, Request: ~p", [Method, Request]), - case httpc:request(Method, Request, [], []) of - {error, socket_closed_remotely} -> - {error, socket_closed_remotely}; - {ok, {{_, StatusCode, _}, Headers1, Body1}} -> - Body2 = - case emqx_json:safe_decode(Body1, [return_maps]) of - {ok, Json} -> Json; - {error, _} -> Body1 - end, - {ok, #{status_code => StatusCode, headers => Headers1, body => Body2}} + Ref = Request(), + receive + {gun_response, Gun, Ref, nofin, StatusCode, Headers1} -> + Data = data_loop(Gun, Ref, _Acc = <<>>), + #{status_code => StatusCode, headers => maps:from_list(Headers1), body => Data} + after 5_000 -> + error({timeout, Opts, process_info(self(), messages)}) + end. + +data_loop(Gun, Ref, Acc) -> + receive + {gun_data, Gun, Ref, nofin, Data} -> + data_loop(Gun, Ref, <>); + {gun_data, Gun, Ref, fin, Data} -> + gun:shutdown(Gun), + <> + after 5000 -> + error(timeout) end. %%--------------------------------------------------------------------------------------- @@ -83,10 +98,10 @@ do_request(Opts) -> %%--------------------------------------------------------------------------------------- t_status_ok(_Config) -> - {ok, #{ + #{ body := Resp, status_code := StatusCode - }} = do_request(#{ + } = do_request(#{ method => get, path => ["status"], headers => [], @@ -100,10 +115,11 @@ t_status_ok(_Config) -> ok. t_status_not_ok(_Config) -> - {ok, #{ + #{ body := Resp, + headers := Headers, status_code := StatusCode - }} = do_request(#{ + } = do_request(#{ method => get, path => ["status"], headers => [], @@ -114,4 +130,8 @@ t_status_not_ok(_Config) -> {match, _}, re:run(Resp, <<"emqx is not_running$">>) ), + ?assertMatch( + #{<<"retry-after">> := <<"15">>}, + Headers + ), ok.