From 570bf165af5c40c0a1e4d32cc8faefd42d5a11b4 Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Fri, 3 Mar 2023 16:43:09 +0100 Subject: [PATCH] fix: return human readable error message for most common cases --- apps/emqx_bridge/src/emqx_bridge_api.erl | 35 ++++++-- .../test/emqx_bridge_api_SUITE.erl | 79 ++++++++++++++++++- changes/ce/fix-10066.en.md | 1 + changes/ce/fix-10066.zh.md | 1 + 4 files changed, 105 insertions(+), 11 deletions(-) create mode 100644 changes/ce/fix-10066.en.md create mode 100644 changes/ce/fix-10066.zh.md diff --git a/apps/emqx_bridge/src/emqx_bridge_api.erl b/apps/emqx_bridge/src/emqx_bridge_api.erl index 871def0d6..dbc94c943 100644 --- a/apps/emqx_bridge/src/emqx_bridge_api.erl +++ b/apps/emqx_bridge/src/emqx_bridge_api.erl @@ -420,6 +420,9 @@ schema("/bridges/:id/:operation") -> ], responses => #{ 204 => <<"Operation success">>, + 400 => error_schema( + 'BAD_REQUEST', "Problem with configuration of external service" + ), 404 => error_schema('NOT_FOUND', "Bridge not found or invalid operation"), 501 => error_schema('NOT_IMPLEMENTED', "Not Implemented"), 503 => error_schema('SERVICE_UNAVAILABLE', "Service unavailable") @@ -440,7 +443,10 @@ schema("/nodes/:node/bridges/:id/:operation") -> ], responses => #{ 204 => <<"Operation success">>, - 400 => error_schema('BAD_REQUEST', "Forbidden operation, bridge not enabled"), + 400 => error_schema( + 'BAD_REQUEST', + "Problem with configuration of external service or bridge not enabled" + ), 404 => error_schema('NOT_FOUND', "Bridge not found or invalid operation"), 501 => error_schema('NOT_IMPLEMENTED', "Not Implemented"), 503 => error_schema('SERVICE_UNAVAILABLE', "Service unavailable") @@ -555,8 +561,8 @@ schema("/bridges_probe") -> case emqx_bridge_resource:create_dry_run(ConnType, maps:remove(<<"type">>, Params)) of ok -> {204}; - {error, Error} -> - {400, error_msg('TEST_FAILED', Error)} + {error, Reason} when not is_tuple(Reason); element(1, Reason) =/= 'exit' -> + {400, error_msg('TEST_FAILED', to_hr_reason(Reason))} end; BadRequest -> BadRequest @@ -577,8 +583,8 @@ do_lookup_from_all_nodes(BridgeType, BridgeName, SuccCode, FormatFun) -> {SuccCode, FormatFun([R || {ok, R} <- Results])}; {ok, [{error, not_found} | _]} -> ?BRIDGE_NOT_FOUND(BridgeType, BridgeName); - {error, ErrL} -> - {500, error_msg('INTERNAL_ERROR', ErrL)} + {error, Reason} -> + {500, error_msg('INTERNAL_ERROR', Reason)} end. lookup_from_local_node(BridgeType, BridgeName) -> @@ -885,7 +891,7 @@ is_ok(ResL) -> ) of [] -> {ok, [Res || {ok, Res} <- ResL]}; - ErrL -> {error, ErrL} + ErrL -> hd(ErrL) end. filter_out_request_body(Conf) -> @@ -934,8 +940,8 @@ call_operation(NodeOrAll, OperFunc, Args) -> ) ) )}; - {error, Reason} -> - {500, error_msg('INTERNAL_ERROR', Reason)} + {error, Reason} when not is_tuple(Reason); element(1, Reason) =/= 'exit' -> + {400, error_msg('BAD_REQUEST', to_hr_reason(Reason))} end. maybe_try_restart(all, start_bridges_to_all_nodes, Args) -> @@ -969,6 +975,19 @@ supported_versions(start_bridge_to_node) -> [2]; supported_versions(start_bridges_to_all_nodes) -> [2]; supported_versions(_Call) -> [1, 2]. +to_hr_reason(nxdomain) -> + <<"Host not found">>; +to_hr_reason(econnrefused) -> + <<"Connection refused">>; +to_hr_reason({unauthorized_client, _}) -> + <<"Unauthorized client">>; +to_hr_reason({not_authorized, _}) -> + <<"Not authorized">>; +to_hr_reason({malformed_username_or_password, _}) -> + <<"Malformed username or password">>; +to_hr_reason(Reason) -> + Reason. + redact(Term) -> emqx_misc:redact(Term). diff --git a/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl b/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl index 727764d73..02d0b7cd8 100644 --- a/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl +++ b/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl @@ -73,7 +73,7 @@ init_per_suite(Config) -> _ = application:stop(emqx_resource), _ = application:stop(emqx_connector), ok = emqx_mgmt_api_test_util:init_suite( - [emqx_rule_engine, emqx_bridge] + [emqx_rule_engine, emqx_bridge, emqx_authn] ), ok = emqx_common_test_helpers:load_config( emqx_rule_engine_schema, @@ -83,7 +83,8 @@ init_per_suite(Config) -> Config. end_per_suite(_Config) -> - emqx_mgmt_api_test_util:end_suite([emqx_rule_engine, emqx_bridge]), + emqx_mgmt_api_test_util:end_suite([emqx_rule_engine, emqx_bridge, emqx_authn]), + mria:clear_table(emqx_authn_mnesia), ok. init_per_testcase(t_broken_bpapi_vsn, Config) -> @@ -720,11 +721,83 @@ t_bridges_probe(Config) -> ?assertMatch( #{ <<"code">> := <<"TEST_FAILED">>, - <<"message">> := <<"econnrefused">> + <<"message">> := <<"Connection refused">> }, jsx:decode(ConnRefused) ), + {ok, 400, HostNotFound} = request( + post, + uri(["bridges_probe"]), + ?MQTT_BRIDGE(<<"nohost:2883">>) + ), + ?assertMatch( + #{ + <<"code">> := <<"TEST_FAILED">>, + <<"message">> := <<"Host not found">> + }, + jsx:decode(HostNotFound) + ), + + AuthnConfig = #{ + <<"mechanism">> => <<"password_based">>, + <<"backend">> => <<"built_in_database">>, + <<"user_id_type">> => <<"username">> + }, + Chain = 'mqtt:global', + emqx:update_config( + [authentication], + {create_authenticator, Chain, AuthnConfig} + ), + User = #{user_id => <<"u">>, password => <<"p">>}, + AuthenticatorID = <<"password_based:built_in_database">>, + {ok, _} = emqx_authentication:add_user( + Chain, + AuthenticatorID, + User + ), + + {ok, 400, Unauthorized} = request( + post, + uri(["bridges_probe"]), + ?MQTT_BRIDGE(<<"127.0.0.1:1883">>)#{<<"proto_ver">> => <<"v4">>} + ), + ?assertMatch( + #{ + <<"code">> := <<"TEST_FAILED">>, + <<"message">> := <<"Unauthorized client">> + }, + jsx:decode(Unauthorized) + ), + + {ok, 400, Malformed} = request( + post, + uri(["bridges_probe"]), + ?MQTT_BRIDGE(<<"127.0.0.1:1883">>)#{ + <<"proto_ver">> => <<"v4">>, <<"password">> => <<"mySecret">>, <<"username">> => <<"u">> + } + ), + ?assertMatch( + #{ + <<"code">> := <<"TEST_FAILED">>, + <<"message">> := <<"Malformed username or password">> + }, + jsx:decode(Malformed) + ), + + {ok, 400, NotAuthorized} = request( + post, + uri(["bridges_probe"]), + ?MQTT_BRIDGE(<<"127.0.0.1:1883">>) + ), + ?assertMatch( + #{ + <<"code">> := <<"TEST_FAILED">>, + <<"message">> := <<"Not authorized">> + }, + jsx:decode(NotAuthorized) + ), + {ok, 400, BadReq} = request( post, uri(["bridges_probe"]), diff --git a/changes/ce/fix-10066.en.md b/changes/ce/fix-10066.en.md new file mode 100644 index 000000000..2d23ad5b9 --- /dev/null +++ b/changes/ce/fix-10066.en.md @@ -0,0 +1 @@ +Return human readable error message for `/briges_probe` and `[/node/:node]/bridges/:id/:operation` API calls and set HTTP status code to `400` instead of `500`. diff --git a/changes/ce/fix-10066.zh.md b/changes/ce/fix-10066.zh.md new file mode 100644 index 000000000..c72f21ff1 --- /dev/null +++ b/changes/ce/fix-10066.zh.md @@ -0,0 +1 @@ +为 `/briges_probe` 和 `[/node/:node]/bridges/:id/:operation` 的 API 调用返回人类可读的错误信息，并将 HTTP 状态代码设置为 `400` 而不是 `500`。