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

2022-08-31 14:30:03 +08:00 · 2022-08-31 14:30:03 +08:00 · 5718b28eeb
parent 7041378c80
commit 5718b28eeb
4 changed files with 190 additions and 22 deletions
--- 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

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

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