From c84e4a4187e543e91cda436ff44fa5de7ff43496 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Wed, 1 Nov 2023 14:11:43 -0300 Subject: [PATCH] fix(rule_engine): don't enable a rule that references non-existent bridge --- ...qx_bridge_v1_compatibility_layer_SUITE.erl | 103 ++++++++++++++++++ .../test/emqx_bridge_v2_testlib.erl | 4 +- .../emqx_rule_engine/src/emqx_rule_engine.erl | 57 +++++++++- .../test/emqx_rule_engine_SUITE.erl | 8 ++ .../test/emqx_telemetry_SUITE.erl | 10 +- 5 files changed, 174 insertions(+), 8 deletions(-) diff --git a/apps/emqx_bridge/test/emqx_bridge_v1_compatibility_layer_SUITE.erl b/apps/emqx_bridge/test/emqx_bridge_v1_compatibility_layer_SUITE.erl index bfc3eedc5..f3ae1232c 100644 --- a/apps/emqx_bridge/test/emqx_bridge_v1_compatibility_layer_SUITE.erl +++ b/apps/emqx_bridge/test/emqx_bridge_v1_compatibility_layer_SUITE.erl @@ -448,6 +448,21 @@ bridge_node_operation_http_api_v2(Name, Node0, Op0) -> ct:pal("bridge node op ~p (http v2) result:\n ~p", [{Node, Op}, Res]), Res. +is_rule_enabled(RuleId) -> + {ok, #{enable := Enable}} = emqx_rule_engine:get_rule(RuleId), + Enable. + +update_rule_http(RuleId, Params) -> + Path = emqx_mgmt_api_test_util:api_path(["rules", RuleId]), + ct:pal("update rule ~p:\n ~p", [RuleId, Params]), + Res = request(put, Path, Params), + ct:pal("update rule ~p result:\n ~p", [RuleId, Res]), + Res. + +enable_rule_http(RuleId) -> + Params = #{<<"enable">> => true}, + update_rule_http(RuleId, Params). + %%------------------------------------------------------------------------------ %% Test cases %%------------------------------------------------------------------------------ @@ -681,3 +696,91 @@ t_scenario_1(_Config) -> %% ?assertMatch({error, {{_, 404, _}, _, _}}, bridge_operation_http_api_v2(NameA, restart)), ok. + +t_scenario_2(Config) -> + %% =================================================================================== + %% Pre-conditions + %% =================================================================================== + ?assertMatch({ok, {{_, 200, _}, _, []}}, list_bridges_http_api_v1()), + ?assertMatch({ok, {{_, 200, _}, _, []}}, list_bridges_http_api_v2()), + %% created in the test case init + ?assertMatch({ok, {{_, 200, _}, _, [#{}]}}, list_connectors_http()), + {ok, {{_, 200, _}, _, [#{<<"name">> := _PreexistentConnectorName}]}} = list_connectors_http(), + + %% =================================================================================== + %% Try to create a rule referencing a non-existent bridge. It succeeds, but it's + %% implicitly disabled. Trying to update it later without creating the bridge should + %% keep it disabled. + %% =================================================================================== + BridgeName = <<"scenario2">>, + RuleTopic = <<"t/scenario2">>, + {ok, #{<<"id">> := RuleId0}} = + emqx_bridge_v2_testlib:create_rule_and_action_http( + bridge_type(), + RuleTopic, + [ + {bridge_name, BridgeName} + | Config + ], + #{overrides => #{enable => true}} + ), + ?assertNot(is_rule_enabled(RuleId0)), + ?assertMatch({ok, {{_, 200, _}, _, _}}, enable_rule_http(RuleId0)), + ?assertNot(is_rule_enabled(RuleId0)), + + %% =================================================================================== + %% Now we create the bridge, and attempt to create a new enabled rule. It should + %% start enabled. Also, updating the previous rule to enable it should work now. + %% =================================================================================== + ?assertMatch( + {ok, {{_, 201, _}, _, #{}}}, + create_bridge_http_api_v1(#{name => BridgeName}) + ), + {ok, #{<<"id">> := RuleId1}} = + emqx_bridge_v2_testlib:create_rule_and_action_http( + bridge_type(), + RuleTopic, + [ + {bridge_name, BridgeName} + | Config + ], + #{overrides => #{enable => true}} + ), + ?assertNot(is_rule_enabled(RuleId0)), + ?assert(is_rule_enabled(RuleId1)), + ?assertMatch({ok, {{_, 200, _}, _, _}}, enable_rule_http(RuleId0)), + ?assert(is_rule_enabled(RuleId0)), + + %% =================================================================================== + %% Creating a rule with mixed existent/non-existent bridges should deny enabling it. + %% =================================================================================== + NonExistentBridgeName = <<"scenario2_not_created">>, + {ok, #{<<"id">> := RuleId2}} = + emqx_bridge_v2_testlib:create_rule_and_action_http( + bridge_type(), + RuleTopic, + [ + {bridge_name, BridgeName} + | Config + ], + #{ + overrides => #{ + enable => true, + actions => [ + emqx_bridge_resource:bridge_id( + bridge_type(), + BridgeName + ), + emqx_bridge_resource:bridge_id( + bridge_type(), + NonExistentBridgeName + ) + ] + } + } + ), + ?assertNot(is_rule_enabled(RuleId2)), + ?assertMatch({ok, {{_, 200, _}, _, _}}, enable_rule_http(RuleId2)), + ?assertNot(is_rule_enabled(RuleId2)), + + ok. diff --git a/apps/emqx_bridge/test/emqx_bridge_v2_testlib.erl b/apps/emqx_bridge/test/emqx_bridge_v2_testlib.erl index 9ed9eb05e..10fb3a63b 100644 --- a/apps/emqx_bridge/test/emqx_bridge_v2_testlib.erl +++ b/apps/emqx_bridge/test/emqx_bridge_v2_testlib.erl @@ -260,11 +260,13 @@ create_rule_and_action_http(BridgeType, RuleTopic, Config, Opts) -> BridgeName = ?config(bridge_name, Config), BridgeId = emqx_bridge_resource:bridge_id(BridgeType, BridgeName), SQL = maps:get(sql, Opts, <<"SELECT * FROM \"", RuleTopic/binary, "\"">>), - Params = #{ + Params0 = #{ enable => true, sql => SQL, actions => [BridgeId] }, + Overrides = maps:get(overrides, Opts, #{}), + Params = emqx_utils_maps:deep_merge(Params0, Overrides), Path = emqx_mgmt_api_test_util:api_path(["rules"]), AuthHeader = emqx_mgmt_api_test_util:auth_header_(), ct:pal("rule action params: ~p", [Params]), diff --git a/apps/emqx_rule_engine/src/emqx_rule_engine.erl b/apps/emqx_rule_engine/src/emqx_rule_engine.erl index 22c476886..612cb5ff3 100644 --- a/apps/emqx_rule_engine/src/emqx_rule_engine.erl +++ b/apps/emqx_rule_engine/src/emqx_rule_engine.erl @@ -460,12 +460,11 @@ code_change(_OldVsn, State, _Extra) -> with_parsed_rule(Params = #{id := RuleId, sql := Sql, actions := Actions}, CreatedAt, Fun) -> case emqx_rule_sqlparser:parse(Sql) of {ok, Select} -> - Rule = #{ + Rule0 = #{ id => RuleId, name => maps:get(name, Params, <<"">>), created_at => CreatedAt, updated_at => now_ms(), - enable => maps:get(enable, Params, true), sql => Sql, actions => parse_actions(Actions), description => maps:get(description, Params, ""), @@ -478,6 +477,21 @@ with_parsed_rule(Params = #{id := RuleId, sql := Sql, actions := Actions}, Creat conditions => emqx_rule_sqlparser:select_where(Select) %% -- calculated fields end }, + InputEnable = maps:get(enable, Params, true), + Enable = + case validate_bridge_existence_in_actions(Rule0) of + ok -> + InputEnable; + {error, NonExistentBridgeIDs} -> + ?SLOG(error, #{ + msg => "action_references_nonexistent_bridges", + rule_id => RuleId, + nonexistent_bridge_ids => NonExistentBridgeIDs, + hint => "this rule will be disabled" + }), + false + end, + Rule = Rule0#{enable => Enable}, ok = Fun(Rule), {ok, Rule}; {error, Reason} -> @@ -593,3 +607,42 @@ extra_functions_module() -> set_extra_functions_module(Mod) -> persistent_term:put({?MODULE, extra_functions}, Mod), ok. + +%% Checks whether the referenced bridges in actions all exist. If there are non-existent +%% ones, the rule shouldn't be allowed to be enabled. +%% The actions here are already parsed. +validate_bridge_existence_in_actions(#{actions := Actions, from := Froms} = _Rule) -> + BridgeIDs0 = + lists:map( + fun(BridgeID) -> + emqx_bridge_resource:parse_bridge_id(BridgeID, #{atom_name => false}) + end, + get_referenced_hookpoints(Froms) + ), + BridgeIDs1 = + lists:filtermap( + fun + ({bridge_v2, Type, Name}) -> {true, {Type, Name}}; + ({bridge, Type, Name, _ResId}) -> {true, {Type, Name}}; + (_) -> false + end, + Actions + ), + NonExistentBridgeIDs = + lists:filter( + fun({Type, Name}) -> + try + case emqx_bridge:lookup(Type, Name) of + {ok, _} -> false; + {error, _} -> true + end + catch + _:_ -> true + end + end, + BridgeIDs0 ++ BridgeIDs1 + ), + case NonExistentBridgeIDs of + [] -> ok; + _ -> {error, #{nonexistent_bridge_ids => NonExistentBridgeIDs}} + end. diff --git a/apps/emqx_rule_engine/test/emqx_rule_engine_SUITE.erl b/apps/emqx_rule_engine/test/emqx_rule_engine_SUITE.erl index 31b0e83fa..2c2af9c9a 100644 --- a/apps/emqx_rule_engine/test/emqx_rule_engine_SUITE.erl +++ b/apps/emqx_rule_engine/test/emqx_rule_engine_SUITE.erl @@ -251,6 +251,10 @@ init_per_testcase(t_events, Config) -> ), ?assertMatch(#{id := <<"rule:t_events">>}, Rule), [{hook_points_rules, Rule} | Config]; +init_per_testcase(t_get_basic_usage_info_1, Config) -> + meck:new(emqx_bridge, [passthrough, no_link, no_history]), + meck:expect(emqx_bridge, lookup, fun(_Type, _Name) -> {ok, #{mocked => true}} end), + Config; init_per_testcase(_TestCase, Config) -> Config. @@ -259,6 +263,10 @@ end_per_testcase(t_events, Config) -> ok = delete_rule(?config(hook_points_rules, Config)), emqx_common_test_helpers:call_janitor(), ok; +end_per_testcase(t_get_basic_usage_info_1, _Config) -> + meck:unload(), + emqx_common_test_helpers:call_janitor(), + ok; end_per_testcase(_TestCase, _Config) -> emqx_common_test_helpers:call_janitor(), ok. diff --git a/apps/emqx_telemetry/test/emqx_telemetry_SUITE.erl b/apps/emqx_telemetry/test/emqx_telemetry_SUITE.erl index 92839d06a..fa0dd07df 100644 --- a/apps/emqx_telemetry/test/emqx_telemetry_SUITE.erl +++ b/apps/emqx_telemetry/test/emqx_telemetry_SUITE.erl @@ -781,8 +781,8 @@ setup_fake_rule_engine_data() -> [ #{function => <<"erlang:hibernate">>, args => #{}}, #{function => console}, - <<"webhook:my_webhook">>, - <<"webhook:my_webhook">> + <<"webhook:basic_usage_info_webhook">>, + <<"webhook:basic_usage_info_webhook_disabled">> ] } ), @@ -793,8 +793,8 @@ setup_fake_rule_engine_data() -> sql => <<"select 1 from topic">>, actions => [ - <<"mqtt:my_mqtt_bridge">>, - <<"webhook:my_webhook">> + <<"mqtt:basic_usage_info_mqtt">>, + <<"webhook:basic_usage_info_webhook">> ] } ), @@ -802,7 +802,7 @@ setup_fake_rule_engine_data() -> emqx_rule_engine:create_rule( #{ id => <<"rule:t_get_basic_usage_info:3">>, - sql => <<"select 1 from \"$bridges/mqtt:mqtt_in\"">>, + sql => <<"select 1 from \"$bridges/mqtt:basic_usage_info_mqtt\"">>, actions => [ #{function => console}