From cf9a207743faaca639fe05ed3250d3e8ee5cf457 Mon Sep 17 00:00:00 2001 From: zhongwencool Date: Fri, 9 Jun 2023 15:28:28 +0800 Subject: [PATCH] feat: check all config schema before loading --- apps/emqx/src/emqx_config_handler.erl | 2 + apps/emqx_authn/src/emqx_authn.erl | 2 +- apps/emqx_authn/src/emqx_authn_api.erl | 2 + apps/emqx_conf/src/emqx_conf.erl | 34 ++++++++--- apps/emqx_conf/src/emqx_conf_cli.erl | 34 ++++++++++- apps/emqx_conf/test/emqx_conf_cli_SUITE.erl | 62 ++++++++++++++++++++- 6 files changed, 122 insertions(+), 14 deletions(-) diff --git a/apps/emqx/src/emqx_config_handler.erl b/apps/emqx/src/emqx_config_handler.erl index 1ca2e8b24..0b5cc695d 100644 --- a/apps/emqx/src/emqx_config_handler.erl +++ b/apps/emqx/src/emqx_config_handler.erl @@ -44,6 +44,8 @@ code_change/3 ]). +-export([schema/2]). + -define(MOD, {mod}). -define(WKEY, '?'). diff --git a/apps/emqx_authn/src/emqx_authn.erl b/apps/emqx_authn/src/emqx_authn.erl index e413a409a..e717550f1 100644 --- a/apps/emqx_authn/src/emqx_authn.erl +++ b/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}). diff --git a/apps/emqx_authn/src/emqx_authn_api.erl b/apps/emqx_authn/src/emqx_authn_api.erl index 65ce0cc32..b237108c2 100644 --- a/apps/emqx_authn/src/emqx_authn_api.erl +++ b/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() -> diff --git a/apps/emqx_conf/src/emqx_conf.erl b/apps/emqx_conf/src/emqx_conf.erl index b9e5e17fa..483241c37 100644 --- a/apps/emqx_conf/src/emqx_conf.erl +++ b/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 lists:filtermap(fun(Key) -> filter_changed(Key, CheckedConf) end, ?READONLY_KEYS) of - [] -> - {ok, maps:without([atom_to_binary(K) || K <- ?READONLY_KEYS], Raw)}; - Error -> + case check_config(SchemaMod, RawDefault) of + {ok, CheckedConf} -> + case + lists:filtermap(fun(Key) -> filter_changed(Key, CheckedConf) end, ?READONLY_KEYS) + 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", - read_only_keys => ?READONLY_KEYS, + msg => "failed_to_check_etc_config", 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 %%-------------------------------------------------------------------- diff --git a/apps/emqx_conf/src/emqx_conf_cli.erl b/apps/emqx_conf/src/emqx_conf_cli.erl index 84aef48f2..4aae4040b 100644 --- a/apps/emqx_conf/src/emqx_conf_cli.erl +++ b/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. diff --git a/apps/emqx_conf/test/emqx_conf_cli_SUITE.erl b/apps/emqx_conf/test/emqx_conf_cli_SUITE.erl index 3ca90646b..a485f4edd 100644 --- a/apps/emqx_conf/test/emqx_conf_cli_SUITE.erl +++ b/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">>}.