From 10b2894b6e35072936274264a5bd4f55ec2ace31 Mon Sep 17 00:00:00 2001 From: Kjell Winblad Date: Wed, 1 Nov 2023 11:31:17 +0100 Subject: [PATCH 1/3] chore(bridge_v2_api): remove misplaced handling code --- apps/emqx_bridge/src/emqx_bridge_v2_api.erl | 5 ----- 1 file changed, 5 deletions(-) diff --git a/apps/emqx_bridge/src/emqx_bridge_v2_api.erl b/apps/emqx_bridge/src/emqx_bridge_v2_api.erl index da6c697a1..0e4e68c76 100644 --- a/apps/emqx_bridge/src/emqx_bridge_v2_api.erl +++ b/apps/emqx_bridge/src/emqx_bridge_v2_api.erl @@ -373,11 +373,6 @@ schema("/bridges_v2_probe") -> case emqx_bridge_v2:remove(BridgeType, BridgeName) of ok -> ?NO_CONTENT; - {error, {active_channels, Channels}} -> - ?BAD_REQUEST( - {<<"Cannot delete bridge while there are active channels defined for this bridge">>, - Channels} - ); {error, timeout} -> ?SERVICE_UNAVAILABLE(<<"request timeout">>); {error, Reason} -> From 1e935e9eb481da0820e1208cd0d59a0ed3bee2ba Mon Sep 17 00:00:00 2001 From: Kjell Winblad Date: Wed, 1 Nov 2023 15:01:08 +0100 Subject: [PATCH 2/3] fix(bridge_v2 API): optional cascading delete operation This commit makes the delete HTTP API operation for Bridge V2 behave in the same way as in the Bridge V1 API. Fixes: https://emqx.atlassian.net/browse/EMQX-11293 --- apps/emqx_bridge/src/emqx_bridge_v2.erl | 25 ++++++- apps/emqx_bridge/src/emqx_bridge_v2_api.erl | 31 +++++++- .../test/emqx_bridge_v2_api_SUITE.erl | 70 ++++++++++++++++++- 3 files changed, 121 insertions(+), 5 deletions(-) diff --git a/apps/emqx_bridge/src/emqx_bridge_v2.erl b/apps/emqx_bridge/src/emqx_bridge_v2.erl index 3747a4671..0660995ff 100644 --- a/apps/emqx_bridge/src/emqx_bridge_v2.erl +++ b/apps/emqx_bridge/src/emqx_bridge_v2.erl @@ -38,7 +38,11 @@ list/0, lookup/2, create/3, - remove/2 + remove/2, + %% The following is the remove function that is called by the HTTP API + %% It also checks for rule action dependencies and optionally removes + %% them + check_deps_and_remove/3 ]). %% Operations @@ -227,6 +231,25 @@ remove(BridgeType, BridgeName) -> {error, Reason} -> {error, Reason} end. +check_deps_and_remove(BridgeType, BridgeName, AlsoDeleteActions) -> + AlsoDelete = + case AlsoDeleteActions of + true -> [rule_actions]; + false -> [] + end, + case + emqx_bridge_lib:maybe_withdraw_rule_action( + BridgeType, + BridgeName, + AlsoDelete + ) + of + ok -> + remove(BridgeType, BridgeName); + {error, Reason} -> + {error, Reason} + end. + %%-------------------------------------------------------------------- %% Helpers for CRUD API %%-------------------------------------------------------------------- diff --git a/apps/emqx_bridge/src/emqx_bridge_v2_api.erl b/apps/emqx_bridge/src/emqx_bridge_v2_api.erl index 0e4e68c76..aa06b85f5 100644 --- a/apps/emqx_bridge/src/emqx_bridge_v2_api.erl +++ b/apps/emqx_bridge/src/emqx_bridge_v2_api.erl @@ -70,7 +70,11 @@ namespace() -> "bridge_v2". api_spec() -> - emqx_dashboard_swagger:spec(?MODULE, #{check_schema => true}). + %% TODO + %% The check_schema option needs to be set to false so we get the + %% query_string to the delete operation. We can change this once + %% we have fixed the schmea for the delete operation. + emqx_dashboard_swagger:spec(?MODULE, #{check_schema => false}). paths() -> [ @@ -365,14 +369,35 @@ schema("/bridges_v2_probe") -> ?BRIDGE_NOT_FOUND(BridgeType, BridgeName) end ); -'/bridges_v2/:id'(delete, #{bindings := #{id := Id}}) -> +'/bridges_v2/:id'(delete, #{bindings := #{id := Id}, query_string := Qs} = All) -> ?TRY_PARSE_ID( Id, case emqx_bridge_v2:lookup(BridgeType, BridgeName) of {ok, _} -> - case emqx_bridge_v2:remove(BridgeType, BridgeName) of + AlsoDeleteActions = + case maps:get(<<"also_delete_dep_actions">>, Qs, <<"false">>) of + <<"true">> -> true; + true -> true; + _ -> false + end, + case + emqx_bridge_v2:check_deps_and_remove(BridgeType, BridgeName, AlsoDeleteActions) + of ok -> ?NO_CONTENT; + {error, #{ + reason := rules_depending_on_this_bridge, + rule_ids := RuleIds + }} -> + RuleIdLists = [binary_to_list(iolist_to_binary(X)) || X <- RuleIds], + RulesStr = string:join(RuleIdLists, ", "), + Msg = io_lib:format( + "Cannot delete bridge while active rules are depending on it: ~s\n" + "Append ?also_delete_dep_actions=true to the request URL to delete " + "rule actions that depend on this bridge as well.", + [RulesStr] + ), + ?BAD_REQUEST(iolist_to_binary(Msg)); {error, timeout} -> ?SERVICE_UNAVAILABLE(<<"request timeout">>); {error, Reason} -> diff --git a/apps/emqx_bridge/test/emqx_bridge_v2_api_SUITE.erl b/apps/emqx_bridge/test/emqx_bridge_v2_api_SUITE.erl index 2fc17664f..484a1a325 100644 --- a/apps/emqx_bridge/test/emqx_bridge_v2_api_SUITE.erl +++ b/apps/emqx_bridge/test/emqx_bridge_v2_api_SUITE.erl @@ -147,7 +147,8 @@ emqx, emqx_auth, emqx_management, - {emqx_bridge, "bridges_v2 {}"} + {emqx_bridge, "bridges_v2 {}"}, + {emqx_rule_engine, "rule_engine { rules {} }"} ]). -define(APPSPEC_DASHBOARD, @@ -667,6 +668,73 @@ t_bridges_probe(Config) -> ), ok. +t_cascade_delete_actions(Config) -> + %% assert we there's no bridges at first + {ok, 200, []} = request_json(get, uri([?ROOT]), Config), + %% then we add a a bridge, using POST + %% POST /bridges_v2/ will create a bridge + BridgeID = emqx_bridge_resource:bridge_id(?BRIDGE_TYPE, ?BRIDGE_NAME), + {ok, 201, _} = request( + post, + uri([?ROOT]), + ?KAFKA_BRIDGE(?BRIDGE_NAME), + Config + ), + {ok, 201, #{<<"id">> := RuleId}} = request_json( + post, + uri(["rules"]), + #{ + <<"name">> => <<"t_http_crud_apis">>, + <<"enable">> => true, + <<"actions">> => [BridgeID], + <<"sql">> => <<"SELECT * from \"t\"">> + }, + Config + ), + %% delete the bridge will also delete the actions from the rules + {ok, 204, _} = request( + delete, + uri([?ROOT, BridgeID]) ++ "?also_delete_dep_actions=true", + Config + ), + {ok, 200, []} = request_json(get, uri([?ROOT]), Config), + ?assertMatch( + {ok, 200, #{<<"actions">> := []}}, + request_json(get, uri(["rules", RuleId]), Config) + ), + {ok, 204, <<>>} = request(delete, uri(["rules", RuleId]), Config), + + {ok, 201, _} = request( + post, + uri([?ROOT]), + ?KAFKA_BRIDGE(?BRIDGE_NAME), + Config + ), + {ok, 201, _} = request( + post, + uri(["rules"]), + #{ + <<"name">> => <<"t_http_crud_apis">>, + <<"enable">> => true, + <<"actions">> => [BridgeID], + <<"sql">> => <<"SELECT * from \"t\"">> + }, + Config + ), + {ok, 400, _} = request( + delete, + uri([?ROOT, BridgeID]), + Config + ), + {ok, 200, [_]} = request_json(get, uri([?ROOT]), Config), + %% Cleanup + {ok, 204, _} = request( + delete, + uri([?ROOT, BridgeID]) ++ "?also_delete_dep_actions=true", + Config + ), + {ok, 200, []} = request_json(get, uri([?ROOT]), Config). + %%% helpers listen_on_random_port() -> SockOpts = [binary, {active, false}, {packet, raw}, {reuseaddr, true}, {backlog, 1000}], From 4bea65bf9710c5e372ec6059df2fc72aba234806 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Wed, 1 Nov 2023 11:32:37 -0300 Subject: [PATCH 3/3] fix(bridge_v2_api): don't disable schema check --- apps/emqx_bridge/src/emqx_bridge_v2_api.erl | 22 ++++++++++++++------- rel/i18n/emqx_bridge_v2_api.hocon | 6 ++++++ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/apps/emqx_bridge/src/emqx_bridge_v2_api.erl b/apps/emqx_bridge/src/emqx_bridge_v2_api.erl index aa06b85f5..f495a770b 100644 --- a/apps/emqx_bridge/src/emqx_bridge_v2_api.erl +++ b/apps/emqx_bridge/src/emqx_bridge_v2_api.erl @@ -70,11 +70,7 @@ namespace() -> "bridge_v2". api_spec() -> - %% TODO - %% The check_schema option needs to be set to false so we get the - %% query_string to the delete operation. We can change this once - %% we have fixed the schmea for the delete operation. - emqx_dashboard_swagger:spec(?MODULE, #{check_schema => false}). + emqx_dashboard_swagger:spec(?MODULE, #{check_schema => true}). paths() -> [ @@ -127,6 +123,18 @@ param_path_id() -> } )}. +param_qs_delete_cascade() -> + {also_delete_dep_actions, + mk( + boolean(), + #{ + in => query, + required => false, + default => false, + desc => ?DESC("desc_qs_also_delete_dep_actions") + } + )}. + param_path_operation_cluster() -> {operation, mk( @@ -235,7 +243,7 @@ schema("/bridges_v2/:id") -> tags => [<<"bridges_v2">>], summary => <<"Delete bridge">>, description => ?DESC("desc_api5"), - parameters => [param_path_id()], + parameters => [param_path_id(), param_qs_delete_cascade()], responses => #{ 204 => <<"Bridge deleted">>, 400 => error_schema( @@ -369,7 +377,7 @@ schema("/bridges_v2_probe") -> ?BRIDGE_NOT_FOUND(BridgeType, BridgeName) end ); -'/bridges_v2/:id'(delete, #{bindings := #{id := Id}, query_string := Qs} = All) -> +'/bridges_v2/:id'(delete, #{bindings := #{id := Id}, query_string := Qs}) -> ?TRY_PARSE_ID( Id, case emqx_bridge_v2:lookup(BridgeType, BridgeName) of diff --git a/rel/i18n/emqx_bridge_v2_api.hocon b/rel/i18n/emqx_bridge_v2_api.hocon index 1ee4419f6..0f1340a67 100644 --- a/rel/i18n/emqx_bridge_v2_api.hocon +++ b/rel/i18n/emqx_bridge_v2_api.hocon @@ -79,6 +79,12 @@ desc_param_path_id.desc: desc_param_path_id.label: """Bridge ID""" +desc_qs_also_delete_dep_actions.desc: +"""Whether to cascade delete dependent actions.""" + +desc_qs_also_delete_dep_actions.label: +"""Cascade delete dependent actions?""" + desc_param_path_node.desc: """The node name, e.g. 'emqx@127.0.0.1'."""