Merge pull request #8851 from zhongwencool/denny-update-local-override-conf

feat: deny hot updates to the configuration in local-override.conf

CHANGES-5.0.md

@@ -18,6 +18,7 @@
 
 * Print a warning message when boot with the default (insecure) Erlang cookie. [#8905](https://github.com/emqx/emqx/pull/8905)
 * 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)
 * Remove `node.etc_dir` from emqx.conf, because it is never used.
   Also allow user to customize the logging directory [#8892](https://github.com/emqx/emqx/pull/8892)
 * Added a new API `POST /listeners` for creating listener. [#8876](https://github.com/emqx/emqx/pull/8876)

apps/emqx/src/emqx_config.erl

@@ -476,7 +476,7 @@ read_override_conf(#{} = Opts) ->
 
 override_conf_file(Opts) when is_map(Opts) ->
     Key =
-        case maps:get(override_to, Opts, local) of
+        case maps:get(override_to, Opts, cluster) of
             local -> local_override_conf_file;
             cluster -> cluster_override_conf_file
         end,

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.

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}.
 

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} ->