From 7d7c069257a3f4a2ee9d74308edc75ad7dde2615 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9F=90=E6=96=87?= Date: Tue, 16 May 2023 15:32:43 +0800 Subject: [PATCH 1/3] feat: update wehbook's request_timeout into resource_opts --- apps/emqx_bridge/src/emqx_bridge.app.src | 2 +- apps/emqx_bridge/src/emqx_bridge_resource.erl | 26 +++++++--- .../src/schema/emqx_bridge_schema.erl | 49 +------------------ .../src/schema/emqx_bridge_webhook_schema.erl | 28 ++++++++--- 4 files changed, 42 insertions(+), 63 deletions(-) diff --git a/apps/emqx_bridge/src/emqx_bridge.app.src b/apps/emqx_bridge/src/emqx_bridge.app.src index e408250be..d2bf0f0c2 100644 --- a/apps/emqx_bridge/src/emqx_bridge.app.src +++ b/apps/emqx_bridge/src/emqx_bridge.app.src @@ -1,7 +1,7 @@ %% -*- mode: erlang -*- {application, emqx_bridge, [ {description, "EMQX bridges"}, - {vsn, "0.1.18"}, + {vsn, "0.1.19"}, {registered, [emqx_bridge_sup]}, {mod, {emqx_bridge_app, []}}, {applications, [ diff --git a/apps/emqx_bridge/src/emqx_bridge_resource.erl b/apps/emqx_bridge/src/emqx_bridge_resource.erl index 0d2feef83..a756f535e 100644 --- a/apps/emqx_bridge/src/emqx_bridge_resource.erl +++ b/apps/emqx_bridge/src/emqx_bridge_resource.erl @@ -165,20 +165,20 @@ create(BridgeId, Conf) -> create(Type, Name, Conf) -> create(Type, Name, Conf, #{}). -create(Type, Name, Conf, Opts0) -> +create(Type, Name, Conf, Opts) -> ?SLOG(info, #{ msg => "create bridge", type => Type, name => Name, config => emqx_utils:redact(Conf) }), - Opts = override_start_after_created(Conf, Opts0), + TypeBin = bin(Type), {ok, _Data} = emqx_resource:create_local( resource_id(Type, Name), <<"emqx_bridge">>, bridge_to_resource_type(Type), - parse_confs(bin(Type), Name, Conf), - Opts + parse_confs(TypeBin, Name, Conf), + parse_opts(TypeBin, Conf, Opts) ), ok. @@ -189,7 +189,7 @@ update(BridgeId, {OldConf, Conf}) -> update(Type, Name, {OldConf, Conf}) -> update(Type, Name, {OldConf, Conf}, #{}). -update(Type, Name, {OldConf, Conf}, Opts0) -> +update(Type, Name, {OldConf, Conf}, Opts) -> %% TODO: sometimes its not necessary to restart the bridge connection. %% %% - if the connection related configs like `servers` is updated, we should restart/start @@ -198,7 +198,6 @@ update(Type, Name, {OldConf, Conf}, Opts0) -> %% the `method` or `headers` of a WebHook is changed, then the bridge can be updated %% without restarting the bridge. %% - Opts = override_start_after_created(Conf, Opts0), case emqx_utils_maps:if_only_to_toggle_enable(OldConf, Conf) of false -> ?SLOG(info, #{ @@ -241,11 +240,12 @@ recreate(Type, Name, Conf) -> recreate(Type, Name, Conf, #{}). recreate(Type, Name, Conf, Opts) -> + TypeBin = bin(Type), emqx_resource:recreate_local( resource_id(Type, Name), bridge_to_resource_type(Type), - parse_confs(bin(Type), Name, Conf), - Opts + parse_confs(TypeBin, Name, Conf), + parse_opts(TypeBin, Conf, Opts) ). create_dry_run(Type, Conf0) -> @@ -402,6 +402,16 @@ bin(Bin) when is_binary(Bin) -> Bin; bin(Str) when is_list(Str) -> list_to_binary(Str); bin(Atom) when is_atom(Atom) -> atom_to_binary(Atom, utf8). +parse_opts(Type, Conf, Opts0) -> + Opts1 = override_start_after_created(Conf, Opts0), + override_resource_request_timeout(Type, Conf, Opts1). + +%% Put webhook's http request_timeout into the resource options +override_resource_request_timeout(<<"webhook">>, #{request_timeout := ReqTimeout}, Opts) -> + Opts#{request_timeout => ReqTimeout}; +override_resource_request_timeout(_Type, _Conf, Opts) -> + Opts. + override_start_after_created(Config, Opts) -> Enabled = maps:get(enable, Config, true), StartAfterCreated = Enabled andalso maps:get(start_after_created, Opts, Enabled), diff --git a/apps/emqx_bridge/src/schema/emqx_bridge_schema.erl b/apps/emqx_bridge/src/schema/emqx_bridge_schema.erl index f58805b6b..d1755bf73 100644 --- a/apps/emqx_bridge/src/schema/emqx_bridge_schema.erl +++ b/apps/emqx_bridge/src/schema/emqx_bridge_schema.erl @@ -223,51 +223,6 @@ node_name() -> {"node", mk(binary(), #{desc => ?DESC("desc_node_name"), example => "emqx@127.0.0.1"})}. webhook_bridge_converter(Conf0, _HoconOpts) -> - Conf1 = emqx_bridge_compatible_config:upgrade_pre_ee( + emqx_bridge_compatible_config:upgrade_pre_ee( Conf0, fun emqx_bridge_compatible_config:webhook_maybe_upgrade/1 - ), - case Conf1 of - undefined -> - undefined; - _ -> - maps:map( - fun(_Name, Conf) -> - do_convert_webhook_config(Conf) - end, - Conf1 - ) - end. - -do_convert_webhook_config( - #{<<"request_timeout">> := ReqT, <<"resource_opts">> := #{<<"request_timeout">> := ReqT}} = Conf -) -> - %% ok: same values - Conf; -do_convert_webhook_config( - #{ - <<"request_timeout">> := ReqTRootRaw, - <<"resource_opts">> := #{<<"request_timeout">> := ReqTResourceRaw} - } = Conf0 -) -> - %% different values; we set them to the same, if they are valid - %% durations - MReqTRoot = emqx_schema:to_duration_ms(ReqTRootRaw), - MReqTResource = emqx_schema:to_duration_ms(ReqTResourceRaw), - case {MReqTRoot, MReqTResource} of - {{ok, ReqTRoot}, {ok, ReqTResource}} -> - {_Parsed, ReqTRaw} = max({ReqTRoot, ReqTRootRaw}, {ReqTResource, ReqTResourceRaw}), - Conf1 = emqx_utils_maps:deep_merge( - Conf0, - #{ - <<"request_timeout">> => ReqTRaw, - <<"resource_opts">> => #{<<"request_timeout">> => ReqTRaw} - } - ), - Conf1; - _ -> - %% invalid values; let the type checker complain about - %% that. - Conf0 - end; -do_convert_webhook_config(Conf) -> - Conf. + ). diff --git a/apps/emqx_bridge/src/schema/emqx_bridge_webhook_schema.erl b/apps/emqx_bridge/src/schema/emqx_bridge_webhook_schema.erl index 1540f77bf..83a3dba9b 100644 --- a/apps/emqx_bridge/src/schema/emqx_bridge_webhook_schema.erl +++ b/apps/emqx_bridge/src/schema/emqx_bridge_webhook_schema.erl @@ -40,12 +40,15 @@ fields("put") -> fields("get") -> emqx_bridge_schema:status_fields() ++ fields("post"); fields("creation_opts") -> - lists:filter( - fun({K, _V}) -> - not lists:member(K, unsupported_opts()) - end, - emqx_resource_schema:fields("creation_opts") - ). + [ + deprecated_request_timeout() + | lists:filter( + fun({K, _V}) -> + not lists:member(K, unsupported_opts()) + end, + emqx_resource_schema:fields("creation_opts") + ) + ]. desc("config") -> ?DESC("desc_config"); @@ -163,7 +166,8 @@ unsupported_opts() -> [ enable_batch, batch_size, - batch_time + batch_time, + request_timeout ]. %%====================================================================================== @@ -190,3 +194,13 @@ name_field() -> method() -> enum([post, put, get, delete]). + +deprecated_request_timeout() -> + {request_timeout, + mk( + hoconsc:union([infinity, emqx_schema:duration_ms()]), + #{ + default => <<"15s">>, + deprecated => {since, "5.0.26"} + } + )}. From a2aa6b46667630a99a3b2b07a17d85f6f473820f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9F=90=E6=96=87?= Date: Tue, 16 May 2023 20:57:57 +0800 Subject: [PATCH 2/3] chore: make ci happy again --- .../test/emqx_bridge_api_SUITE.erl | 14 ++++---- .../emqx_bridge_compatible_config_tests.erl | 34 ++++++++----------- changes/ce/feat-10713.en.md | 3 ++ 3 files changed, 23 insertions(+), 28 deletions(-) create mode 100644 changes/ce/feat-10713.en.md diff --git a/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl b/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl index d55b92138..1b5e51a11 100644 --- a/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl +++ b/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl @@ -1284,19 +1284,17 @@ t_inconsistent_webhook_request_timeouts(Config) -> <<"resource_opts">> => #{<<"request_timeout">> => <<"2s">>} } ), - ?assertMatch( - {ok, 201, #{ - %% note: same value on both fields - <<"request_timeout">> := <<"2s">>, - <<"resource_opts">> := #{<<"request_timeout">> := <<"2s">>} - }}, + {ok, 201, #{ + <<"request_timeout">> := <<"1s">>, + <<"resource_opts">> := ResourceOpts + }} = request_json( post, uri(["bridges"]), BadBridgeParams, Config - ) - ), + ), + ?assertNot(maps:is_key(<<"request_timeout">>, ResourceOpts)), ok. %% diff --git a/apps/emqx_bridge/test/emqx_bridge_compatible_config_tests.erl b/apps/emqx_bridge/test/emqx_bridge_compatible_config_tests.erl index acafb84ca..3481ac30c 100644 --- a/apps/emqx_bridge/test/emqx_bridge_compatible_config_tests.erl +++ b/apps/emqx_bridge/test/emqx_bridge_compatible_config_tests.erl @@ -59,27 +59,21 @@ webhook_config_test() -> }, check(Conf2) ), - - %% the converter should pick the greater of the two - %% request_timeouts and place them in the root and inside - %% resource_opts. - ?assertMatch( - #{ - <<"bridges">> := #{ - <<"webhook">> := #{ - <<"the_name">> := - #{ - <<"method">> := get, - <<"request_timeout">> := 60_000, - <<"resource_opts">> := #{<<"request_timeout">> := 60_000}, - <<"body">> := <<"${payload}">> - } - } + #{ + <<"bridges">> := #{ + <<"webhook">> := #{ + <<"the_name">> := + #{ + <<"method">> := get, + <<"request_timeout">> := RequestTime, + <<"resource_opts">> := ResourceOpts, + <<"body">> := <<"${payload}">> + } } - }, - check(Conf3) - ), - + } + } = check(Conf3), + ?assertEqual(60_000, RequestTime), + ?assertNot(maps:is_key(<<"requst_timeout">>, ResourceOpts)), ok. up(#{<<"bridges">> := Bridges0} = Conf0) -> diff --git a/changes/ce/feat-10713.en.md b/changes/ce/feat-10713.en.md new file mode 100644 index 000000000..0e28a1a12 --- /dev/null +++ b/changes/ce/feat-10713.en.md @@ -0,0 +1,3 @@ +We deprecated the request_timeout in resource_option of the webhook to keep it consistent with the http request_timeout of the webhook. +From now on, when configuring a webhook through API or configuration files, +it is no longer necessary to configure the request_timeout of the resource. Only configuring the http request_timeout is sufficient, and the request_timeout in the resource will automatically be consistent with the http request_timeout. From 2b99a9b988c336345a1a19bfe98de583a8c01666 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9F=90=E6=96=87?= Date: Wed, 17 May 2023 13:31:45 +0800 Subject: [PATCH 3/3] feat: hide resource_opts's request_timeout --- apps/emqx_bridge/src/emqx_bridge_api.erl | 13 +++++++--- apps/emqx_bridge/src/emqx_bridge_resource.erl | 15 ++++-------- .../src/schema/emqx_bridge_schema.erl | 23 ++++++++++++++++-- .../src/schema/emqx_bridge_webhook_schema.erl | 8 +++---- .../test/emqx_bridge_api_SUITE.erl | 24 +++++++++++++++++++ .../emqx_bridge_compatible_config_tests.erl | 2 +- changes/ce/feat-10713.en.md | 2 +- 7 files changed, 65 insertions(+), 22 deletions(-) diff --git a/apps/emqx_bridge/src/emqx_bridge_api.erl b/apps/emqx_bridge/src/emqx_bridge_api.erl index c7e48990b..9802d5fe8 100644 --- a/apps/emqx_bridge/src/emqx_bridge_api.erl +++ b/apps/emqx_bridge/src/emqx_bridge_api.erl @@ -892,11 +892,18 @@ fill_defaults(Type, RawConf) -> pack_bridge_conf(Type, RawConf) -> #{<<"bridges">> => #{bin(Type) => #{<<"foo">> => RawConf}}}. -unpack_bridge_conf(Type, PackedConf) -> - #{<<"bridges">> := Bridges} = PackedConf, - #{<<"foo">> := RawConf} = maps:get(bin(Type), Bridges), +%% Hide webhook's resource_opts.request_timeout from user. +filter_raw_conf(<<"webhook">>, RawConf0) -> + emqx_utils_maps:deep_remove([<<"resource_opts">>, <<"request_timeout">>], RawConf0); +filter_raw_conf(_TypeBin, RawConf) -> RawConf. +unpack_bridge_conf(Type, PackedConf) -> + TypeBin = bin(Type), + #{<<"bridges">> := Bridges} = PackedConf, + #{<<"foo">> := RawConf} = maps:get(TypeBin, Bridges), + filter_raw_conf(TypeBin, RawConf). + is_ok(ok) -> ok; is_ok(OkResult = {ok, _}) -> diff --git a/apps/emqx_bridge/src/emqx_bridge_resource.erl b/apps/emqx_bridge/src/emqx_bridge_resource.erl index a756f535e..60ee242d1 100644 --- a/apps/emqx_bridge/src/emqx_bridge_resource.erl +++ b/apps/emqx_bridge/src/emqx_bridge_resource.erl @@ -178,7 +178,7 @@ create(Type, Name, Conf, Opts) -> <<"emqx_bridge">>, bridge_to_resource_type(Type), parse_confs(TypeBin, Name, Conf), - parse_opts(TypeBin, Conf, Opts) + parse_opts(Conf, Opts) ), ok. @@ -245,7 +245,7 @@ recreate(Type, Name, Conf, Opts) -> resource_id(Type, Name), bridge_to_resource_type(Type), parse_confs(TypeBin, Name, Conf), - parse_opts(TypeBin, Conf, Opts) + parse_opts(Conf, Opts) ). create_dry_run(Type, Conf0) -> @@ -402,15 +402,8 @@ bin(Bin) when is_binary(Bin) -> Bin; bin(Str) when is_list(Str) -> list_to_binary(Str); bin(Atom) when is_atom(Atom) -> atom_to_binary(Atom, utf8). -parse_opts(Type, Conf, Opts0) -> - Opts1 = override_start_after_created(Conf, Opts0), - override_resource_request_timeout(Type, Conf, Opts1). - -%% Put webhook's http request_timeout into the resource options -override_resource_request_timeout(<<"webhook">>, #{request_timeout := ReqTimeout}, Opts) -> - Opts#{request_timeout => ReqTimeout}; -override_resource_request_timeout(_Type, _Conf, Opts) -> - Opts. +parse_opts(Conf, Opts0) -> + override_start_after_created(Conf, Opts0). override_start_after_created(Config, Opts) -> Enabled = maps:get(enable, Config, true), diff --git a/apps/emqx_bridge/src/schema/emqx_bridge_schema.erl b/apps/emqx_bridge/src/schema/emqx_bridge_schema.erl index d1755bf73..b590f0cd4 100644 --- a/apps/emqx_bridge/src/schema/emqx_bridge_schema.erl +++ b/apps/emqx_bridge/src/schema/emqx_bridge_schema.erl @@ -223,6 +223,25 @@ node_name() -> {"node", mk(binary(), #{desc => ?DESC("desc_node_name"), example => "emqx@127.0.0.1"})}. webhook_bridge_converter(Conf0, _HoconOpts) -> - emqx_bridge_compatible_config:upgrade_pre_ee( + Conf1 = emqx_bridge_compatible_config:upgrade_pre_ee( Conf0, fun emqx_bridge_compatible_config:webhook_maybe_upgrade/1 - ). + ), + case Conf1 of + undefined -> + undefined; + _ -> + maps:map( + fun(_Name, Conf) -> + do_convert_webhook_config(Conf) + end, + Conf1 + ) + end. + +%% We hide resource_opts.request_timeout from user. +do_convert_webhook_config( + #{<<"request_timeout">> := ReqT, <<"resource_opts">> := ResOpts} = Conf +) -> + Conf#{<<"resource_opts">> => ResOpts#{<<"request_timeout">> => ReqT}}; +do_convert_webhook_config(Conf) -> + Conf. diff --git a/apps/emqx_bridge/src/schema/emqx_bridge_webhook_schema.erl b/apps/emqx_bridge/src/schema/emqx_bridge_webhook_schema.erl index 83a3dba9b..de1e09abb 100644 --- a/apps/emqx_bridge/src/schema/emqx_bridge_webhook_schema.erl +++ b/apps/emqx_bridge/src/schema/emqx_bridge_webhook_schema.erl @@ -41,7 +41,7 @@ fields("get") -> emqx_bridge_schema:status_fields() ++ fields("post"); fields("creation_opts") -> [ - deprecated_request_timeout() + hidden_request_timeout() | lists:filter( fun({K, _V}) -> not lists:member(K, unsupported_opts()) @@ -195,12 +195,12 @@ name_field() -> method() -> enum([post, put, get, delete]). -deprecated_request_timeout() -> +hidden_request_timeout() -> {request_timeout, mk( hoconsc:union([infinity, emqx_schema:duration_ms()]), #{ - default => <<"15s">>, - deprecated => {since, "5.0.26"} + required => false, + importance => ?IMPORTANCE_HIDDEN } )}. diff --git a/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl b/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl index 1b5e51a11..288b1da29 100644 --- a/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl +++ b/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl @@ -1295,8 +1295,32 @@ t_inconsistent_webhook_request_timeouts(Config) -> Config ), ?assertNot(maps:is_key(<<"request_timeout">>, ResourceOpts)), + validate_resource_request_timeout(proplists:get_value(group, Config), 1000, Name), ok. +validate_resource_request_timeout(single, Timeout, Name) -> + SentData = #{payload => <<"Hello EMQX">>, timestamp => 1668602148000}, + BridgeID = emqx_bridge_resource:bridge_id(?BRIDGE_TYPE_HTTP, Name), + ResId = emqx_bridge_resource:resource_id(<<"webhook">>, Name), + ?check_trace( + begin + {ok, Res} = + ?wait_async_action( + emqx_bridge:send_message(BridgeID, SentData), + #{?snk_kind := async_query}, + 1000 + ), + ?assertMatch({ok, #{id := ResId, query_opts := #{timeout := Timeout}}}, Res) + end, + fun(Trace0) -> + Trace = ?of_kind(async_query, Trace0), + ?assertMatch([#{query_opts := #{timeout := Timeout}}], Trace), + ok + end + ); +validate_resource_request_timeout(_Cluster, _Timeout, _Name) -> + ignore. + %% request(Method, URL, Config) -> diff --git a/apps/emqx_bridge/test/emqx_bridge_compatible_config_tests.erl b/apps/emqx_bridge/test/emqx_bridge_compatible_config_tests.erl index 3481ac30c..080ca9107 100644 --- a/apps/emqx_bridge/test/emqx_bridge_compatible_config_tests.erl +++ b/apps/emqx_bridge/test/emqx_bridge_compatible_config_tests.erl @@ -73,7 +73,7 @@ webhook_config_test() -> } } = check(Conf3), ?assertEqual(60_000, RequestTime), - ?assertNot(maps:is_key(<<"requst_timeout">>, ResourceOpts)), + ?assertMatch(#{<<"request_timeout">> := 60_000}, ResourceOpts), ok. up(#{<<"bridges">> := Bridges0} = Conf0) -> diff --git a/changes/ce/feat-10713.en.md b/changes/ce/feat-10713.en.md index 0e28a1a12..6de542be6 100644 --- a/changes/ce/feat-10713.en.md +++ b/changes/ce/feat-10713.en.md @@ -1,3 +1,3 @@ -We deprecated the request_timeout in resource_option of the webhook to keep it consistent with the http request_timeout of the webhook. +We hide the request_timeout in resource_option of the webhook to keep it consistent with the http request_timeout of the webhook. From now on, when configuring a webhook through API or configuration files, it is no longer necessary to configure the request_timeout of the resource. Only configuring the http request_timeout is sufficient, and the request_timeout in the resource will automatically be consistent with the http request_timeout.