diff --git a/apps/emqx/include/emqx_api_lib.hrl b/apps/emqx/include/emqx_api_lib.hrl new file mode 100644 index 000000000..549b0f94c --- /dev/null +++ b/apps/emqx/include/emqx_api_lib.hrl @@ -0,0 +1,36 @@ +%%-------------------------------------------------------------------- +%% Copyright (c) 2021-2023 EMQ Technologies Co., Ltd. All Rights Reserved. +%% +%% Licensed under the Apache License, Version 2.0 (the "License"); +%% you may not use this file except in compliance with the License. +%% You may obtain a copy of the License at +%% +%% http://www.apache.org/licenses/LICENSE-2.0 +%% +%% Unless required by applicable law or agreed to in writing, software +%% distributed under the License is distributed on an "AS IS" BASIS, +%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +%% See the License for the specific language governing permissions and +%% limitations under the License. +%%-------------------------------------------------------------------- + +-ifndef(EMQX_API_LIB_HRL). +-define(EMQX_API_LIB_HRL, true). + +-define(ERROR_MSG(CODE, REASON), #{code => CODE, message => emqx_misc:readable_error_msg(REASON)}). + +-define(OK(CONTENT), {200, CONTENT}). + +-define(NO_CONTENT, 204). + +-define(BAD_REQUEST(CODE, REASON), {400, ?ERROR_MSG(CODE, REASON)}). +-define(BAD_REQUEST(REASON), ?BAD_REQUEST('BAD_REQUEST', REASON)). + +-define(NOT_FOUND(REASON), {404, ?ERROR_MSG('NOT_FOUND', REASON)}). + +-define(INTERNAL_ERROR(REASON), {500, ?ERROR_MSG('INTERNAL_ERROR', REASON)}). + +-define(NOT_IMPLEMENTED, 501). + +-define(SERVICE_UNAVAILABLE(REASON), {503, ?ERROR_MSG('SERVICE_UNAVAILABLE', REASON)}). +-endif. diff --git a/apps/emqx/src/emqx_api_lib.erl b/apps/emqx/src/emqx_api_lib.erl new file mode 100644 index 000000000..8c49c57c3 --- /dev/null +++ b/apps/emqx/src/emqx_api_lib.erl @@ -0,0 +1,69 @@ +%%-------------------------------------------------------------------- +%% Copyright (c) 2020-2023 EMQ Technologies Co., Ltd. All Rights Reserved. +%% +%% Licensed under the Apache License, Version 2.0 (the "License"); +%% you may not use this file except in compliance with the License. +%% You may obtain a copy of the License at +%% +%% http://www.apache.org/licenses/LICENSE-2.0 +%% +%% Unless required by applicable law or agreed to in writing, software +%% distributed under the License is distributed on an "AS IS" BASIS, +%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +%% See the License for the specific language governing permissions and +%% limitations under the License. +%%-------------------------------------------------------------------- + +-module(emqx_api_lib). + +-export([ + with_node/2, + with_node_or_cluster/2 +]). + +-include("emqx_api_lib.hrl"). + +-define(NODE_NOT_FOUND(NODE), ?NOT_FOUND(<<"Node not found: ", NODE/binary>>)). + +%%-------------------------------------------------------------------- +%% exported API +%%-------------------------------------------------------------------- +-spec with_node(binary(), fun((atom()) -> {ok, term()} | {error, term()})) -> + ?OK(term()) | ?NOT_FOUND(binary()) | ?BAD_REQUEST(term()). +with_node(BinNode, Fun) -> + case lookup_node(BinNode) of + {ok, Node} -> + handle_result(Fun(Node)); + not_found -> + ?NODE_NOT_FOUND(BinNode) + end. + +-spec with_node_or_cluster(binary(), fun((atom()) -> {ok, term()} | {error, term()})) -> + ?OK(term()) | ?NOT_FOUND(iolist()) | ?BAD_REQUEST(term()). +with_node_or_cluster(<<"all">>, Fun) -> + handle_result(Fun(all)); +with_node_or_cluster(Node, Fun) -> + with_node(Node, Fun). + +%%-------------------------------------------------------------------- +%% Internal +%%-------------------------------------------------------------------- + +-spec lookup_node(binary()) -> {ok, atom()} | not_found. +lookup_node(BinNode) -> + case emqx_misc:safe_to_existing_atom(BinNode, utf8) of + {ok, Node} -> + case lists:member(Node, mria:running_nodes()) of + true -> + {ok, Node}; + false -> + not_found + end; + _Error -> + not_found + end. + +handle_result({ok, Result}) -> + ?OK(Result); +handle_result({error, Reason}) -> + ?BAD_REQUEST(Reason). diff --git a/apps/emqx/test/emqx_api_lib_SUITE.erl b/apps/emqx/test/emqx_api_lib_SUITE.erl new file mode 100644 index 000000000..29f5c6095 --- /dev/null +++ b/apps/emqx/test/emqx_api_lib_SUITE.erl @@ -0,0 +1,101 @@ +%%-------------------------------------------------------------------- +%% Copyright (c) 2020-2023 EMQ Technologies Co., Ltd. All Rights Reserved. +%% +%% Licensed under the Apache License, Version 2.0 (the "License"); +%% you may not use this file except in compliance with the License. +%% You may obtain a copy of the License at +%% +%% http://www.apache.org/licenses/LICENSE-2.0 +%% +%% Unless required by applicable law or agreed to in writing, software +%% distributed under the License is distributed on an "AS IS" BASIS, +%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +%% See the License for the specific language governing permissions and +%% limitations under the License. +%%-------------------------------------------------------------------- + +-module(emqx_api_lib_SUITE). + +-compile(export_all). +-compile(nowarn_export_all). + +-include("emqx_api_lib.hrl"). +-include_lib("eunit/include/eunit.hrl"). + +-define(DUMMY, dummy_module). + +all() -> emqx_common_test_helpers:all(?MODULE). + +init_per_suite(Config) -> + emqx_common_test_helpers:boot_modules(all), + emqx_common_test_helpers:start_apps([]), + Config. + +end_per_suite(_Config) -> + emqx_common_test_helpers:stop_apps([]). + +init_per_testcase(_Case, Config) -> + meck:new(?DUMMY, [non_strict]), + meck:expect(?DUMMY, expect_not_called, 1, fun(Node) -> throw({blow_this_up, Node}) end), + meck:expect(?DUMMY, expect_success, 1, {ok, success}), + meck:expect(?DUMMY, expect_error, 1, {error, error}), + Config. + +end_per_testcase(_Case, _Config) -> + meck:unload(?DUMMY). + +t_with_node(_) -> + test_with(fun emqx_api_lib:with_node/2, [<<"all">>]). + +t_with_node_or_cluster(_) -> + test_with(fun emqx_api_lib:with_node_or_cluster/2, []), + meck:reset(?DUMMY), + ?assertEqual( + ?OK(success), + emqx_api_lib:with_node_or_cluster( + <<"all">>, + fun ?DUMMY:expect_success/1 + ) + ), + ?assertMatch([{_, {?DUMMY, expect_success, [all]}, {ok, success}}], meck:history(?DUMMY)). + +%% helpers +test_with(TestFun, ExtraBadNodes) -> + % make sure this is an atom + 'unknownnode@unknownnohost', + BadNodes = + [ + <<"undefined">>, + <<"this_should_not_be_an_atom">>, + <<"unknownnode@unknownnohost">> + ] ++ ExtraBadNodes, + [ensure_not_found(TestFun(N, fun ?DUMMY:expect_not_called/1)) || N <- BadNodes], + ensure_not_called(?DUMMY, expect_not_called), + ensure_not_existing_atom(<<"this_should_not_be_an_atom">>), + + GoodNode = node(), + + ?assertEqual( + ?OK(success), + TestFun(GoodNode, fun ?DUMMY:expect_success/1) + ), + + ?assertEqual( + ?BAD_REQUEST(error), + TestFun(GoodNode, fun ?DUMMY:expect_error/1) + ), + ok. + +ensure_not_found(Result) -> + ?assertMatch({404, _}, Result). + +ensure_not_called(Mod, Fun) -> + ?assert(not meck:called(Mod, Fun, '_')). + +ensure_not_existing_atom(Bin) -> + try binary_to_existing_atom(Bin) of + _ -> throw(is_atom) + catch + error:badarg -> + ok + end. diff --git a/apps/emqx_bridge/src/emqx_bridge_api.erl b/apps/emqx_bridge/src/emqx_bridge_api.erl index b9a6d4c06..ff93ac584 100644 --- a/apps/emqx_bridge/src/emqx_bridge_api.erl +++ b/apps/emqx_bridge/src/emqx_bridge_api.erl @@ -20,6 +20,7 @@ -include_lib("typerefl/include/types.hrl"). -include_lib("hocon/include/hoconsc.hrl"). -include_lib("emqx/include/logger.hrl"). +-include_lib("emqx/include/emqx_api_lib.hrl"). -include_lib("emqx_bridge/include/emqx_bridge.hrl"). -import(hoconsc, [mk/2, array/1, enum/1]). @@ -46,14 +47,10 @@ -export([lookup_from_local_node/2]). --define(BAD_REQUEST(Reason), {400, error_msg('BAD_REQUEST', Reason)}). - -define(BRIDGE_NOT_ENABLED, ?BAD_REQUEST(<<"Forbidden operation, bridge not enabled">>) ). --define(NOT_FOUND(Reason), {404, error_msg('NOT_FOUND', Reason)}). - -define(BRIDGE_NOT_FOUND(BridgeType, BridgeName), ?NOT_FOUND( <<"Bridge lookup failed: bridge named '", (BridgeName)/binary, "' of type ", diff --git a/apps/emqx_dashboard/src/emqx_dashboard.app.src b/apps/emqx_dashboard/src/emqx_dashboard.app.src index 3970d76e4..8a4764c84 100644 --- a/apps/emqx_dashboard/src/emqx_dashboard.app.src +++ b/apps/emqx_dashboard/src/emqx_dashboard.app.src @@ -2,7 +2,7 @@ {application, emqx_dashboard, [ {description, "EMQX Web Dashboard"}, % strict semver, bump manually! - {vsn, "5.0.15"}, + {vsn, "5.0.16"}, {modules, []}, {registered, [emqx_dashboard_sup]}, {applications, [kernel, stdlib, mnesia, minirest, emqx, emqx_ctl]}, diff --git a/apps/emqx_dashboard/src/emqx_dashboard_monitor_api.erl b/apps/emqx_dashboard/src/emqx_dashboard_monitor_api.erl index 69f5bf34e..aaee92b8c 100644 --- a/apps/emqx_dashboard/src/emqx_dashboard_monitor_api.erl +++ b/apps/emqx_dashboard/src/emqx_dashboard_monitor_api.erl @@ -121,32 +121,27 @@ fields(sampler_current) -> monitor(get, #{query_string := QS, bindings := Bindings}) -> Latest = maps:get(<<"latest">>, QS, infinity), - RawNode = maps:get(node, Bindings, all), - with_node(RawNode, dashboard_samplers_fun(Latest)). + RawNode = maps:get(node, Bindings, <<"all">>), + emqx_api_lib:with_node_or_cluster(RawNode, dashboard_samplers_fun(Latest)). dashboard_samplers_fun(Latest) -> fun(NodeOrCluster) -> case emqx_dashboard_monitor:samplers(NodeOrCluster, Latest) of - {badrpc, _} = Error -> Error; + {badrpc, _} = Error -> {error, Error}; Samplers -> {ok, Samplers} end end. monitor_current(get, #{bindings := Bindings}) -> - RawNode = maps:get(node, Bindings, all), - with_node(RawNode, fun emqx_dashboard_monitor:current_rate/1). + RawNode = maps:get(node, Bindings, <<"all">>), + emqx_api_lib:with_node_or_cluster(RawNode, fun current_rate/1). -with_node(RawNode, Fun) -> - case emqx_misc:safe_to_existing_atom(RawNode, utf8) of - {ok, NodeOrCluster} -> - case Fun(NodeOrCluster) of - {badrpc, {Node, Reason}} -> - {404, 'NOT_FOUND', io_lib:format("Node not found: ~p (~p)", [Node, Reason])}; - {ok, Result} -> - {200, Result} - end; - _Error -> - {404, 'NOT_FOUND', io_lib:format("Node not found: ~p", [RawNode])} +current_rate(Node) -> + case emqx_dashboard_monitor:current_rate(Node) of + {badrpc, _} = BadRpc -> + {error, BadRpc}; + {ok, _} = OkResult -> + OkResult end. %% ------------------------------------------------------------------------------------------------- diff --git a/apps/emqx_management/src/emqx_management.app.src b/apps/emqx_management/src/emqx_management.app.src index 966358f47..9863f5cf6 100644 --- a/apps/emqx_management/src/emqx_management.app.src +++ b/apps/emqx_management/src/emqx_management.app.src @@ -2,7 +2,7 @@ {application, emqx_management, [ {description, "EMQX Management API and CLI"}, % strict semver, bump manually! - {vsn, "5.0.16"}, + {vsn, "5.0.17"}, {modules, []}, {registered, [emqx_management_sup]}, {applications, [kernel, stdlib, emqx_plugins, minirest, emqx, emqx_ctl]}, diff --git a/apps/emqx_management/src/emqx_mgmt_api_nodes.erl b/apps/emqx_management/src/emqx_mgmt_api_nodes.erl index 21d905331..a4173f5b0 100644 --- a/apps/emqx_management/src/emqx_mgmt_api_nodes.erl +++ b/apps/emqx_management/src/emqx_mgmt_api_nodes.erl @@ -17,7 +17,6 @@ -behaviour(minirest_api). --include_lib("emqx/include/emqx.hrl"). -include_lib("typerefl/include/types.hrl"). -import(hoconsc, [mk/2, ref/1, ref/2, enum/1, array/1]). @@ -25,8 +24,6 @@ -define(NODE_METRICS_MODULE, emqx_mgmt_api_metrics). -define(NODE_STATS_MODULE, emqx_mgmt_api_stats). --define(SOURCE_ERROR, 'SOURCE_ERROR'). - %% Swagger specs from hocon schema -export([ api_spec/0, @@ -88,7 +85,7 @@ schema("/nodes/:node") -> ref(node_info), #{desc => <<"Get node info successfully">>} ), - 400 => node_error() + 404 => not_found() } } }; @@ -106,7 +103,7 @@ schema("/nodes/:node/metrics") -> ref(?NODE_METRICS_MODULE, node_metrics), #{desc => <<"Get node metrics successfully">>} ), - 400 => node_error() + 404 => not_found() } } }; @@ -124,7 +121,7 @@ schema("/nodes/:node/stats") -> ref(?NODE_STATS_MODULE, node_stats_data), #{desc => <<"Get node stats successfully">>} ), - 400 => node_error() + 404 => not_found() } } }. @@ -136,7 +133,7 @@ fields(node_name) -> [ {node, mk( - atom(), + binary(), #{ in => path, description => <<"Node name">>, @@ -250,55 +247,46 @@ nodes(get, _Params) -> list_nodes(#{}). node(get, #{bindings := #{node := NodeName}}) -> - get_node(NodeName). + emqx_api_lib:with_node(NodeName, to_ok_result_fun(fun get_node/1)). node_metrics(get, #{bindings := #{node := NodeName}}) -> - get_metrics(NodeName). + emqx_api_lib:with_node(NodeName, to_ok_result_fun(fun emqx_mgmt:get_metrics/1)). node_stats(get, #{bindings := #{node := NodeName}}) -> - get_stats(NodeName). + emqx_api_lib:with_node(NodeName, to_ok_result_fun(fun emqx_mgmt:get_stats/1)). %%-------------------------------------------------------------------- %% api apply list_nodes(#{}) -> - NodesInfo = [format(Node, NodeInfo) || {Node, NodeInfo} <- emqx_mgmt:list_nodes()], + NodesInfo = [format(NodeInfo) || {_Node, NodeInfo} <- emqx_mgmt:list_nodes()], {200, NodesInfo}. get_node(Node) -> - case emqx_mgmt:lookup_node(Node) of - {error, _} -> - {400, #{code => 'SOURCE_ERROR', message => <<"rpc_failed">>}}; - NodeInfo -> - {200, format(Node, NodeInfo)} - end. - -get_metrics(Node) -> - case emqx_mgmt:get_metrics(Node) of - {error, _} -> - {400, #{code => 'SOURCE_ERROR', message => <<"rpc_failed">>}}; - Metrics -> - {200, Metrics} - end. - -get_stats(Node) -> - case emqx_mgmt:get_stats(Node) of - {error, _} -> - {400, #{code => 'SOURCE_ERROR', message => <<"rpc_failed">>}}; - Stats -> - {200, Stats} - end. + format(emqx_mgmt:lookup_node(Node)). %%-------------------------------------------------------------------- %% internal function -format(_Node, Info = #{memory_total := Total, memory_used := Used}) -> +format(Info = #{memory_total := Total, memory_used := Used}) -> Info#{ memory_total := emqx_mgmt_util:kmg(Total), memory_used := emqx_mgmt_util:kmg(Used) }; -format(_Node, Info) when is_map(Info) -> +format(Info) when is_map(Info) -> Info. -node_error() -> - emqx_dashboard_swagger:error_codes([?SOURCE_ERROR], <<"Node error">>). +to_ok_result({error, _} = Error) -> + Error; +to_ok_result({ok, _} = Ok) -> + Ok; +to_ok_result(Result) -> + {ok, Result}. + +to_ok_result_fun(Fun) when is_function(Fun) -> + fun(Arg) -> + to_ok_result(Fun(Arg)) + end. + +not_found() -> + emqx_dashboard_swagger:error_codes(['NOT_FOUND'], <<"Node not found">>). diff --git a/apps/emqx_management/test/emqx_mgmt_api_nodes_SUITE.erl b/apps/emqx_management/test/emqx_mgmt_api_nodes_SUITE.erl index 03b0ea2d9..30313e555 100644 --- a/apps/emqx_management/test/emqx_mgmt_api_nodes_SUITE.erl +++ b/apps/emqx_management/test/emqx_mgmt_api_nodes_SUITE.erl @@ -68,7 +68,7 @@ t_nodes_api(_) -> BadNodePath = emqx_mgmt_api_test_util:api_path(["nodes", "badnode"]), ?assertMatch( - {error, {_, 400, _}}, + {error, {_, 404, _}}, emqx_mgmt_api_test_util:request_api(get, BadNodePath) ). @@ -94,7 +94,7 @@ t_node_stats_api(_) -> BadNodePath = emqx_mgmt_api_test_util:api_path(["nodes", "badnode", "stats"]), ?assertMatch( - {error, {_, 400, _}}, + {error, {_, 404, _}}, emqx_mgmt_api_test_util:request_api(get, BadNodePath) ). @@ -112,7 +112,7 @@ t_node_metrics_api(_) -> BadNodePath = emqx_mgmt_api_test_util:api_path(["nodes", "badnode", "metrics"]), ?assertMatch( - {error, {_, 400, _}}, + {error, {_, 404, _}}, emqx_mgmt_api_test_util:request_api(get, BadNodePath) ). diff --git a/changes/ce/fix-10237.en.md b/changes/ce/fix-10237.en.md new file mode 100644 index 000000000..cf3fc707b --- /dev/null +++ b/changes/ce/fix-10237.en.md @@ -0,0 +1 @@ +Ensure we return `404` status code for unknown node names in `/nodes/:node[/metrics|/stats]` API.