diff --git a/CHANGES-5.0.md b/CHANGES-5.0.md index 742883e27..e6ff9e36f 100644 --- a/CHANGES-5.0.md +++ b/CHANGES-5.0.md @@ -2,7 +2,8 @@ ## Enhancements -* change the `/gateway` API path to plural form. [#8823](https://github.com/emqx/emqx/pull/8823) +* Change the `/gateway` API path to plural form. [#8823](https://github.com/emqx/emqx/pull/8823) +* Don't allow updating config items when they already exist in `local-override.conf`. [#8851](https://github.com/emqx/emqx/pull/8851) # 5.0.7 diff --git a/apps/emqx/src/emqx_config_handler.erl b/apps/emqx/src/emqx_config_handler.erl index ad46c1ee3..0311418a9 100644 --- a/apps/emqx/src/emqx_config_handler.erl +++ b/apps/emqx/src/emqx_config_handler.erl @@ -43,6 +43,7 @@ terminate/2, code_change/3 ]). +-export([is_mutable/3]). -define(MOD, {mod}). -define(WKEY, '?'). @@ -229,15 +230,26 @@ process_update_request([_], _Handlers, {remove, _Opts}) -> process_update_request(ConfKeyPath, _Handlers, {remove, Opts}) -> OldRawConf = emqx_config:get_root_raw(ConfKeyPath), BinKeyPath = bin_path(ConfKeyPath), - NewRawConf = emqx_map_lib:deep_remove(BinKeyPath, OldRawConf), - OverrideConf = remove_from_override_config(BinKeyPath, Opts), - {ok, NewRawConf, OverrideConf, Opts}; + case check_permissions(remove, BinKeyPath, OldRawConf, Opts) of + allow -> + NewRawConf = emqx_map_lib:deep_remove(BinKeyPath, OldRawConf), + OverrideConf = remove_from_override_config(BinKeyPath, Opts), + {ok, NewRawConf, OverrideConf, Opts}; + {deny, Reason} -> + {error, {permission_denied, Reason}} + end; process_update_request(ConfKeyPath, Handlers, {{update, UpdateReq}, Opts}) -> OldRawConf = emqx_config:get_root_raw(ConfKeyPath), case do_update_config(ConfKeyPath, Handlers, OldRawConf, UpdateReq) of {ok, NewRawConf} -> - OverrideConf = update_override_config(NewRawConf, Opts), - {ok, NewRawConf, OverrideConf, Opts}; + BinKeyPath = bin_path(ConfKeyPath), + case check_permissions(update, BinKeyPath, NewRawConf, Opts) of + allow -> + OverrideConf = update_override_config(NewRawConf, Opts), + {ok, NewRawConf, OverrideConf, Opts}; + {deny, Reason} -> + {error, {permission_denied, Reason}} + end; Error -> Error end. @@ -272,12 +284,11 @@ check_and_save_configs( UpdateArgs, Opts ) -> - OldConf = emqx_config:get_root(ConfKeyPath), Schema = schema(SchemaModule, ConfKeyPath), {AppEnvs, NewConf} = emqx_config:check_config(Schema, NewRawConf), + OldConf = emqx_config:get_root(ConfKeyPath), case do_post_config_update(ConfKeyPath, Handlers, OldConf, NewConf, AppEnvs, UpdateArgs, #{}) of {ok, Result0} -> - remove_from_local_if_cluster_change(ConfKeyPath, Opts), ok = emqx_config:save_configs(AppEnvs, NewConf, NewRawConf, OverrideConf, Opts), Result1 = return_change_result(ConfKeyPath, UpdateArgs), {ok, Result1#{post_config_update => Result0}}; @@ -430,16 +441,6 @@ merge_to_old_config(UpdateReq, RawConf) when is_map(UpdateReq), is_map(RawConf) merge_to_old_config(UpdateReq, _RawConf) -> {ok, UpdateReq}. -%% local-override.conf priority is higher than cluster-override.conf -%% If we want cluster to take effect, we must remove the local. -remove_from_local_if_cluster_change(BinKeyPath, #{override_to := cluster} = Opts) -> - Opts1 = Opts#{override_to => local}, - Local = remove_from_override_config(BinKeyPath, Opts1), - _ = emqx_config:save_to_override_conf(Local, Opts1), - ok; -remove_from_local_if_cluster_change(_BinKeyPath, _Opts) -> - ok. - remove_from_override_config(_BinKeyPath, #{persistent := false}) -> undefined; remove_from_override_config(BinKeyPath, Opts) -> @@ -544,3 +545,98 @@ load_prev_handlers() -> save_handlers(Handlers) -> application:set_env(emqx, ?MODULE, Handlers). + +check_permissions(_Action, _ConfKeyPath, _NewRawConf, #{override_to := local}) -> + allow; +check_permissions(Action, ConfKeyPath, NewRawConf, _Opts) -> + case emqx_map_lib:deep_find(ConfKeyPath, NewRawConf) of + {ok, NewRaw} -> + LocalOverride = emqx_config:read_override_conf(#{override_to => local}), + case emqx_map_lib:deep_find(ConfKeyPath, LocalOverride) of + {ok, LocalRaw} -> + case is_mutable(Action, NewRaw, LocalRaw) of + ok -> + allow; + {error, Error} -> + ?SLOG(error, #{ + msg => "prevent_remove_local_override_conf", + config_key_path => ConfKeyPath, + error => Error + }), + {deny, "Disable changed from local-override.conf"} + end; + {not_found, _, _} -> + allow + end; + {not_found, _, _} -> + allow + end. + +is_mutable(Action, NewRaw, LocalRaw) -> + try + KeyPath = [], + is_mutable(KeyPath, Action, NewRaw, LocalRaw) + catch + throw:Error -> Error + end. + +-define(REMOVE_FAILED, "remove_failed"). +-define(UPDATE_FAILED, "update_failed"). + +is_mutable(KeyPath, Action, New = #{}, Local = #{}) -> + maps:foreach( + fun(Key, SubLocal) -> + case maps:find(Key, New) of + error -> ok; + {ok, SubNew} -> is_mutable(KeyPath ++ [Key], Action, SubNew, SubLocal) + end + end, + Local + ); +is_mutable(KeyPath, remove, Update, Origin) -> + throw({error, {?REMOVE_FAILED, KeyPath, Update, Origin}}); +is_mutable(_KeyPath, update, Val, Val) -> + ok; +is_mutable(KeyPath, update, Update, Origin) -> + throw({error, {?UPDATE_FAILED, KeyPath, Update, Origin}}). + +-ifdef(TEST). +-include_lib("eunit/include/eunit.hrl"). + +is_mutable_update_test() -> + Action = update, + ?assertEqual(ok, is_mutable(Action, #{}, #{})), + ?assertEqual(ok, is_mutable(Action, #{a => #{b => #{c => #{}}}}, #{a => #{b => #{c => #{}}}})), + ?assertEqual(ok, is_mutable(Action, #{a => #{b => #{c => 1}}}, #{a => #{b => #{c => 1}}})), + ?assertEqual( + {error, {?UPDATE_FAILED, [a, b, c], 1, 2}}, + is_mutable(Action, #{a => #{b => #{c => 1}}}, #{a => #{b => #{c => 2}}}) + ), + ?assertEqual( + {error, {?UPDATE_FAILED, [a, b, d], 2, 3}}, + is_mutable(Action, #{a => #{b => #{c => 1, d => 2}}}, #{a => #{b => #{c => 1, d => 3}}}) + ), + ok. + +is_mutable_remove_test() -> + Action = remove, + ?assertEqual(ok, is_mutable(Action, #{}, #{})), + ?assertEqual(ok, is_mutable(Action, #{a => #{b => #{c => #{}}}}, #{a1 => #{b => #{c => #{}}}})), + ?assertEqual(ok, is_mutable(Action, #{a => #{b => #{c => 1}}}, #{a => #{b1 => #{c => 1}}})), + ?assertEqual(ok, is_mutable(Action, #{a => #{b => #{c => 1}}}, #{a => #{b => #{c1 => 1}}})), + + ?assertEqual( + {error, {?REMOVE_FAILED, [a, b, c], 1, 1}}, + is_mutable(Action, #{a => #{b => #{c => 1}}}, #{a => #{b => #{c => 1}}}) + ), + ?assertEqual( + {error, {?REMOVE_FAILED, [a, b, c], 1, 2}}, + is_mutable(Action, #{a => #{b => #{c => 1}}}, #{a => #{b => #{c => 2}}}) + ), + ?assertEqual( + {error, {?REMOVE_FAILED, [a, b, c], 1, 1}}, + is_mutable(Action, #{a => #{b => #{c => 1, d => 2}}}, #{a => #{b => #{c => 1, d => 3}}}) + ), + ok. + +-endif. diff --git a/apps/emqx/test/emqx_config_handler_SUITE.erl b/apps/emqx/test/emqx_config_handler_SUITE.erl index ae34bee7a..1fe79f74c 100644 --- a/apps/emqx/test/emqx_config_handler_SUITE.erl +++ b/apps/emqx/test/emqx_config_handler_SUITE.erl @@ -21,6 +21,8 @@ -define(MOD, {mod}). -define(WKEY, '?'). +-define(LOCAL_CONF, "/tmp/local-override.conf"). +-define(CLUSTER_CONF, "/tmp/cluster-override.conf"). -include_lib("eunit/include/eunit.hrl"). -include_lib("common_test/include/ct.hrl"). @@ -36,6 +38,8 @@ end_per_suite(_Config) -> emqx_common_test_helpers:stop_apps([]). init_per_testcase(_Case, Config) -> + _ = file:delete(?LOCAL_CONF), + _ = file:delete(?CLUSTER_CONF), Config. end_per_testcase(_Case, _Config) -> @@ -196,6 +200,62 @@ t_sub_key_update_remove(_Config) -> ok = emqx_config_handler:remove_handler(KeyPath2), ok. +t_local_override_update_remove(_Config) -> + application:set_env(emqx, local_override_conf_file, ?LOCAL_CONF), + application:set_env(emqx, cluster_override_conf_file, ?CLUSTER_CONF), + KeyPath = [sysmon, os, cpu_high_watermark], + ok = emqx_config_handler:add_handler(KeyPath, ?MODULE), + LocalOpts = #{override_to => local}, + {ok, Res} = emqx:update_config(KeyPath, <<"70%">>, LocalOpts), + ?assertMatch( + #{ + config := 0.7, + post_config_update := #{}, + raw_config := <<"70%">> + }, + Res + ), + ClusterOpts = #{override_to => cluster}, + ?assertMatch( + {error, {permission_denied, _}}, emqx:update_config(KeyPath, <<"71%">>, ClusterOpts) + ), + ?assertMatch(0.7, emqx:get_config(KeyPath)), + + KeyPath2 = [sysmon, os, cpu_low_watermark], + ok = emqx_config_handler:add_handler(KeyPath2, ?MODULE), + ?assertMatch( + {error, {permission_denied, _}}, emqx:update_config(KeyPath2, <<"40%">>, ClusterOpts) + ), + + %% remove + ?assertMatch({error, {permission_denied, _}}, emqx:remove_config(KeyPath)), + ?assertEqual( + {ok, #{post_config_update => #{}}}, + emqx:remove_config(KeyPath, #{override_to => local}) + ), + ?assertEqual( + {ok, #{post_config_update => #{}}}, + emqx:remove_config(KeyPath) + ), + ?assertError({config_not_found, KeyPath}, emqx:get_raw_config(KeyPath)), + OSKey = maps:keys(emqx:get_raw_config([sysmon, os])), + ?assertEqual(false, lists:member(<<"cpu_high_watermark">>, OSKey)), + ?assert(length(OSKey) > 0), + + ?assertEqual( + {ok, #{config => 0.8, post_config_update => #{}, raw_config => <<"80%">>}}, + emqx:reset_config(KeyPath, ClusterOpts) + ), + OSKey1 = maps:keys(emqx:get_raw_config([sysmon, os])), + ?assertEqual(true, lists:member(<<"cpu_high_watermark">>, OSKey1)), + ?assert(length(OSKey1) > 1), + + ok = emqx_config_handler:remove_handler(KeyPath), + ok = emqx_config_handler:remove_handler(KeyPath2), + application:unset_env(emqx, local_override_conf_file), + application:unset_env(emqx, cluster_override_conf_file), + ok. + t_check_failed(_Config) -> KeyPath = [sysmon, os, cpu_check_interval], Opts = #{rawconf_with_defaults => true}, @@ -219,7 +279,7 @@ t_stop(_Config) -> ok. t_callback_crash(_Config) -> - CrashPath = [sysmon, os, cpu_high_watermark], + CrashPath = [sysmon, os, procmem_high_watermark], Opts = #{rawconf_with_defaults => true}, ok = emqx_config_handler:add_handler(CrashPath, ?MODULE), Old = emqx:get_raw_config(CrashPath), @@ -334,6 +394,8 @@ pre_config_update([sysmon, os, cpu_check_interval], UpdateReq, _RawConf) -> {ok, UpdateReq}; pre_config_update([sysmon, os, cpu_low_watermark], UpdateReq, _RawConf) -> {ok, UpdateReq}; +pre_config_update([sysmon, os, cpu_high_watermark], UpdateReq, _RawConf) -> + {ok, UpdateReq}; pre_config_update([sysmon, os, sysmem_high_watermark], UpdateReq, _RawConf) -> {ok, UpdateReq}; pre_config_update([sysmon, os, mem_check_interval], _UpdateReq, _RawConf) -> @@ -347,6 +409,8 @@ post_config_update([sysmon, os, cpu_check_interval], _UpdateReq, _NewConf, _OldC {ok, ok}; post_config_update([sysmon, os, cpu_low_watermark], _UpdateReq, _NewConf, _OldConf, _AppEnvs) -> ok; +post_config_update([sysmon, os, cpu_high_watermark], _UpdateReq, _NewConf, _OldConf, _AppEnvs) -> + ok; post_config_update([sysmon, os, sysmem_high_watermark], _UpdateReq, _NewConf, _OldConf, _AppEnvs) -> {error, post_config_update_error}. diff --git a/apps/emqx_management/src/emqx_mgmt_api_configs.erl b/apps/emqx_management/src/emqx_mgmt_api_configs.erl index 7435e5e0d..8eb801952 100644 --- a/apps/emqx_management/src/emqx_mgmt_api_configs.erl +++ b/apps/emqx_management/src/emqx_mgmt_api_configs.erl @@ -141,7 +141,8 @@ schema("/configs_reset/:rootname") -> ], responses => #{ 200 => <<"Rest config successfully">>, - 400 => emqx_dashboard_swagger:error_codes(['NO_DEFAULT_VALUE', 'REST_FAILED']) + 400 => emqx_dashboard_swagger:error_codes(['NO_DEFAULT_VALUE', 'REST_FAILED']), + 403 => emqx_dashboard_swagger:error_codes(['REST_FAILED']) } } }; @@ -160,7 +161,8 @@ schema("/configs/global_zone") -> 'requestBody' => Schema, responses => #{ 200 => Schema, - 400 => emqx_dashboard_swagger:error_codes(['UPDATE_FAILED']) + 400 => emqx_dashboard_swagger:error_codes(['UPDATE_FAILED']), + 403 => emqx_dashboard_swagger:error_codes(['UPDATE_FAILED']) } } }; @@ -226,7 +228,8 @@ schema(Path) -> 'requestBody' => Schema, responses => #{ 200 => Schema, - 400 => emqx_dashboard_swagger:error_codes(['UPDATE_FAILED']) + 400 => emqx_dashboard_swagger:error_codes(['UPDATE_FAILED']), + 403 => emqx_dashboard_swagger:error_codes(['UPDATE_FAILED']) } } }. @@ -254,6 +257,8 @@ config(put, #{body := Body}, Req) -> case emqx_conf:update(Path, Body, ?OPTS) of {ok, #{raw_config := RawConf}} -> {200, RawConf}; + {error, {permission_denied, Reason}} -> + {403, #{code => 'UPDATE_FAILED', message => Reason}}; {error, Reason} -> {400, #{code => 'UPDATE_FAILED', message => ?ERR_MSG(Reason)}} end. @@ -297,6 +302,8 @@ config_reset(post, _Params, Req) -> case emqx_conf:reset(Path, ?OPTS) of {ok, _} -> {200}; + {error, {permission_denied, Reason}} -> + {403, #{code => 'REST_FAILED', message => Reason}}; {error, no_default_value} -> {400, #{code => 'NO_DEFAULT_VALUE', message => <<"No Default Value.">>}}; {error, Reason} ->