Merge pull request #11864 from thalesmg/test-enable-rule-check-deps-r53-20231101

fix(rule_engine): don't enable a rule that references non-existent bridge

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
 %%------------------------------------------------------------------------------
@@ -703,3 +718,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.

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]),

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.

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.

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:basic_usage_info_webhook">>,
-                        <<"webhook:my_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">>,
+                        <<"mqtt:basic_usage_info_mqtt">>,
-                        <<"webhook:my_webhook">>
+                        <<"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}