feat: check all config schema before loading

apps/emqx/src/emqx_config_handler.erl

@@ -44,6 +44,8 @@
     code_change/3
 ]).
 
+-export([schema/2]).
+
 -define(MOD, {mod}).
 -define(WKEY, '?').
 

apps/emqx_authn/src/emqx_authn.erl

@@ -161,4 +161,4 @@ authn_list(Authn) when is_map(Authn) ->
     [Authn].
 
 merge_config(AuthNs) ->
-    emqx_authn_api:update_config(?CONF_NS_BINARY, {merge_authenticators, AuthNs}).
+    emqx_authn_api:update_config([?CONF_NS_ATOM], {merge_authenticators, AuthNs}).

apps/emqx_authn/src/emqx_authn_api.erl

@@ -89,6 +89,8 @@
     param_listener_id/0
 ]).
 
+-export([update_config/2]).
+
 -elvis([{elvis_style, god_modules, disable}]).
 
 api_spec() ->

apps/emqx_conf/src/emqx_conf.erl

@@ -32,6 +32,7 @@
 -export([schema_module/0]).
 -export([gen_example_conf/2]).
 -export([reload_etc_conf_on_local_node/0]).
+-export([check_config/2]).
 
 %% TODO: move to emqx_dashboard when we stop building api schema at build time
 -export([
@@ -280,19 +281,38 @@ load_etc_config_file() ->
 check_readonly_config(Raw) ->
     SchemaMod = schema_module(),
     RawDefault = emqx_config:fill_defaults(Raw),
-    {_AppEnvs, CheckedConf} = emqx_config:check_config(SchemaMod, RawDefault),
+    case check_config(SchemaMod, RawDefault) of
-    case lists:filtermap(fun(Key) -> filter_changed(Key, CheckedConf) end, ?READONLY_KEYS) of
+        {ok, CheckedConf} ->
-        [] ->
+            case
-            {ok, maps:without([atom_to_binary(K) || K <- ?READONLY_KEYS], Raw)};
+                lists:filtermap(fun(Key) -> filter_changed(Key, CheckedConf) end, ?READONLY_KEYS)
-        Error ->
+            of
+                [] ->
+                    {ok, maps:without([atom_to_binary(K) || K <- ?READONLY_KEYS], Raw)};
+                Error ->
+                    ?SLOG(error, #{
+                        msg => "failed_to_change_read_only_key_in_etc_config",
+                        read_only_keys => ?READONLY_KEYS,
+                        error => Error
+                    }),
+                    {error, Error}
+            end;
+        {error, Error} ->
             ?SLOG(error, #{
-                msg => "failed_to_change_read_only_key_in_etc_config",
+                msg => "failed_to_check_etc_config",
-                read_only_keys => ?READONLY_KEYS,
                 error => Error
             }),
             {error, Error}
     end.
 
+check_config(Mod, Raw) ->
+    try
+        {_AppEnvs, CheckedConf} = emqx_config:check_config(Mod, Raw),
+        {ok, CheckedConf}
+    catch
+        throw:Error ->
+            {error, Error}
+    end.
+
 %%--------------------------------------------------------------------
 %% Internal functions
 %%--------------------------------------------------------------------

apps/emqx_conf/src/emqx_conf_cli.erl

@@ -188,14 +188,17 @@ load_config(Path, AuthChain) ->
             emqx_ctl:warning("load ~ts is empty~n", [Path]),
             {error, empty_hocon_file};
         {ok, RawConf} ->
-            case check_config_keys(RawConf) of
+            case check_config(RawConf) of
                 ok ->
                     maps:foreach(fun(K, V) -> update_config(K, V, AuthChain) end, RawConf);
-                {error, Reason} ->
+                {error, Reason} when is_list(Reason) ->
                     emqx_ctl:warning("load ~ts failed~n~ts~n", [Path, Reason]),
                     emqx_ctl:warning(
                         "Maybe try `emqx_ctl conf reload` to reload etc/emqx.conf on local node~n"
                     ),
+                    {error, Reason};
+                {error, Reason} when is_map(Reason) ->
+                    emqx_ctl:warning("load ~ts schema check failed~n~p~n", [Path, Reason]),
                     {error, Reason}
             end;
         {error, Reason} ->
@@ -216,10 +219,35 @@ update_config(Key, Value, _) ->
 check_res(Key, {ok, _}) -> emqx_ctl:print("load ~ts in cluster ok~n", [Key]);
 check_res(Key, {error, Reason}) -> emqx_ctl:warning("load ~ts failed~n~p~n", [Key, Reason]).
 
-check_config_keys(Conf) ->
+check_config(Conf) ->
+    case check_keys_is_not_readonly(Conf) of
+        ok -> check_config_schema(Conf);
+        Error -> Error
+    end.
+
+check_keys_is_not_readonly(Conf) ->
     Keys = maps:keys(Conf),
     ReadOnlyKeys = [atom_to_binary(K) || K <- ?READONLY_KEYS],
     case ReadOnlyKeys -- Keys of
         ReadOnlyKeys -> ok;
         _ -> {error, "update_readonly_keys_prohibited"}
     end.
+
+check_config_schema(Conf) ->
+    SchemaMod = emqx_conf:schema_module(),
+    Res =
+        maps:fold(
+            fun(Key, Value, Acc) ->
+                Schema = emqx_config_handler:schema(SchemaMod, [Key]),
+                case emqx_conf:check_config(Schema, #{Key => Value}) of
+                    {ok, _} -> Acc;
+                    {error, Reason} -> #{Key => Reason}
+                end
+            end,
+            #{},
+            Conf
+        ),
+    case Res =:= #{} of
+        true -> ok;
+        false -> {error, Res}
+    end.

apps/emqx_conf/test/emqx_conf_cli_SUITE.erl

@@ -56,17 +56,73 @@ t_load_config(Config) ->
     ok.
 
 t_load_readonly(Config) ->
-    Base = #{<<"mqtt">> => emqx_conf:get_raw([mqtt])},
+    Base0 = base_conf(),
+    Base1 = Base0#{<<"mqtt">> => emqx_conf:get_raw([mqtt])},
     lists:foreach(
         fun(Key) ->
+            KeyBin = atom_to_binary(Key),
             Conf = emqx_conf:get_raw([Key]),
-            ConfBin0 = hocon_pp:do(Base#{Key => Conf}, #{}),
+            ConfBin0 = hocon_pp:do(Base1#{KeyBin => Conf}, #{}),
             ConfFile0 = prepare_conf_file(?FUNCTION_NAME, ConfBin0, Config),
             ?assertEqual(
                 {error, "update_readonly_keys_prohibited"},
                 emqx_conf_cli:conf(["load", ConfFile0])
-            )
+            ),
+            %% reload etc/emqx.conf changed readonly keys
+            ConfBin1 = hocon_pp:do(Base1#{KeyBin => changed(Key)}, #{}),
+            ConfFile1 = prepare_conf_file(?FUNCTION_NAME, ConfBin1, Config),
+            application:set_env(emqx, config_files, [ConfFile1]),
+            ?assertMatch({error, [{Key, #{changed := _}}]}, emqx_conf_cli:conf(["reload"]))
         end,
         ?READONLY_KEYS
     ),
     ok.
+
+t_error_schema_check(Config) ->
+    Base = #{
+        %% bad multiplier
+        <<"mqtt">> => #{<<"keepalive_multiplier">> => -1},
+        <<"zones">> => #{<<"my-zone">> => #{<<"mqtt">> => #{<<"keepalive_multiplier">> => 10}}}
+    },
+    ConfBin0 = hocon_pp:do(Base, #{}),
+    ConfFile0 = prepare_conf_file(?FUNCTION_NAME, ConfBin0, Config),
+    ?assertMatch({error, _}, emqx_conf_cli:conf(["load", ConfFile0])),
+    %% zones is not updated because of error
+    ?assertEqual(#{}, emqx_config:get_raw([zones])),
+    ok.
+
+t_reload_etc_emqx_conf_not_persistent(Config) ->
+    Mqtt = emqx_conf:get_raw([mqtt]),
+    Base = base_conf(),
+    Conf = Base#{<<"mqtt">> => Mqtt#{<<"keepalive_multiplier">> => 3}},
+    ConfBin = hocon_pp:do(Conf, #{}),
+    ConfFile = prepare_conf_file(?FUNCTION_NAME, ConfBin, Config),
+    application:set_env(emqx, config_files, [ConfFile]),
+    ok = emqx_conf_cli:conf(["reload"]),
+    ?assertEqual(3, emqx:get_config([mqtt, keepalive_multiplier])),
+    ?assertNotEqual(
+        3,
+        emqx_utils_maps:deep_get(
+            [<<"mqtt">>, <<"keepalive_multiplier">>],
+            emqx_config:read_override_conf(#{}),
+            undefined
+        )
+    ),
+    ok.
+
+base_conf() ->
+    #{
+        <<"cluster">> => emqx_conf:get_raw([cluster]),
+        <<"node">> => emqx_conf:get_raw([node])
+    }.
+
+changed(cluster) ->
+    #{<<"name">> => <<"emqx-test">>};
+changed(node) ->
+    #{
+        <<"name">> => <<"emqx-test@127.0.0.1">>,
+        <<"cookie">> => <<"gokdfkdkf1122">>,
+        <<"data_dir">> => <<"data">>
+    };
+changed(rpc) ->
+    #{<<"mode">> => <<"sync">>}.