Merge pull request #12940 from zmstone/0427-catch-hocon-syntax-error

feat(mgmt): add ignore_readonly to  configs API

apps/emqx/rebar.config

@@ -30,7 +30,7 @@
     {esockd, {git, "https://github.com/emqx/esockd", {tag, "5.11.2"}}},
     {ekka, {git, "https://github.com/emqx/ekka", {tag, "0.19.3"}}},
     {gen_rpc, {git, "https://github.com/emqx/gen_rpc", {tag, "3.3.1"}}},
-    {hocon, {git, "https://github.com/emqx/hocon.git", {tag, "0.42.1"}}},
+    {hocon, {git, "https://github.com/emqx/hocon.git", {tag, "0.42.2"}}},
     {emqx_http_lib, {git, "https://github.com/emqx/emqx_http_lib.git", {tag, "0.5.3"}}},
     {pbkdf2, {git, "https://github.com/emqx/erlang-pbkdf2.git", {tag, "2.0.4"}}},
     {recon, {git, "https://github.com/ferd/recon", {tag, "2.5.1"}}},

apps/emqx/src/emqx_logger_jsonfmt.erl

@@ -32,7 +32,7 @@
 -export([format/2]).
 
 %% For CLI HTTP API outputs
--export([best_effort_json/1, best_effort_json/2]).
+-export([best_effort_json/1, best_effort_json/2, best_effort_json_obj/1]).
 
 -ifdef(TEST).
 -include_lib("proper/include/proper.hrl").
@@ -65,10 +65,13 @@
 best_effort_json(Input) ->
     best_effort_json(Input, [pretty, force_utf8]).
 best_effort_json(Input, Opts) ->
-    Config = #{depth => unlimited, single_line => true, chars_limit => unlimited},
-    JsonReady = best_effort_json_obj(Input, Config),
+    JsonReady = best_effort_json_obj(Input),
     emqx_utils_json:encode(JsonReady, Opts).
 
+best_effort_json_obj(Input) ->
+    Config = #{depth => unlimited, single_line => true, chars_limit => unlimited},
+    best_effort_json_obj(Input, Config).
+
 -spec format(logger:log_event(), config()) -> iodata().
 format(#{level := Level, msg := Msg, meta := Meta}, Config0) when is_map(Config0) ->
     Config = add_default_config(Config0),

apps/emqx_conf/src/emqx_conf_cli.erl

@@ -242,7 +242,7 @@ load_config(Bin, Opts) when is_binary(Bin) ->
 load_config_from_raw(RawConf0, Opts) ->
     SchemaMod = emqx_conf:schema_module(),
     RawConf1 = emqx_config:upgrade_raw_conf(SchemaMod, RawConf0),
-    case check_config(RawConf1) of
+    case check_config(RawConf1, Opts) of
         {ok, RawConf} ->
             %% It has been ensured that the connector is always the first configuration to be updated.
             %% However, when deleting the connector, we need to clean up the dependent actions/sources first;
@@ -395,24 +395,28 @@ suggest_msg(#{kind := validation_error, reason := unknown_fields}, Mode) ->
 suggest_msg(_, _) ->
     <<"">>.
 
-check_config(Conf) ->
-    case check_keys_is_not_readonly(Conf) of
-        ok ->
-            Conf1 = emqx_config:fill_defaults(Conf),
-            case check_config_schema(Conf1) of
-                ok -> {ok, Conf1};
+check_config(Conf0, Opts) ->
+    case check_keys_is_not_readonly(Conf0, Opts) of
+        {ok, Conf1} ->
+            Conf = emqx_config:fill_defaults(Conf1),
+            case check_config_schema(Conf) of
+                ok -> {ok, Conf};
                 {error, Reason} -> {error, Reason}
             end;
         Error ->
             Error
     end.
 
-check_keys_is_not_readonly(Conf) ->
+check_keys_is_not_readonly(Conf, Opts) ->
+    IgnoreReadonly = maps:get(ignore_readonly, Opts, false),
     Keys = maps:keys(Conf),
     ReadOnlyKeys = [atom_to_binary(K) || K <- ?READONLY_KEYS],
     case lists:filter(fun(K) -> lists:member(K, Keys) end, ReadOnlyKeys) of
         [] ->
-            ok;
+            {ok, Conf};
+        BadKeys when IgnoreReadonly ->
+            ?SLOG(info, #{msg => "readonly_root_keys_ignored", keys => BadKeys}),
+            {ok, maps:without(BadKeys, Conf)};
         BadKeys ->
             BadKeysStr = lists:join(<<",">>, BadKeys),
             {error, ?UPDATE_READONLY_KEYS_PROHIBITED, BadKeysStr}

apps/emqx_management/src/emqx_mgmt_api_configs.erl

@@ -147,7 +147,9 @@ schema("/configs") ->
                     hoconsc:mk(
                         hoconsc:enum([replace, merge]),
                         #{in => query, default => merge, required => false}
-                    )}
+                    )},
+                {ignore_readonly,
+                    hoconsc:mk(boolean(), #{in => query, default => false, required => false})}
             ],
             'requestBody' => #{
                 content =>
@@ -361,16 +363,18 @@ configs(get, #{query_string := QueryStr, headers := Headers}, _Req) ->
         {ok, <<"text/plain">>} -> get_configs_v2(QueryStr);
         {error, _} = Error -> {400, #{code => 'INVALID_ACCEPT', message => ?ERR_MSG(Error)}}
     end;
-configs(put, #{body := Conf, query_string := #{<<"mode">> := Mode}}, _Req) ->
-    case emqx_conf_cli:load_config(Conf, #{mode => Mode, log => none}) of
+configs(put, #{body := Conf, query_string := #{<<"mode">> := Mode} = QS}, _Req) ->
+    IngnoreReadonly = maps:get(<<"ignore_readonly">>, QS, false),
+    case
+        emqx_conf_cli:load_config(Conf, #{
+            mode => Mode, log => none, ignore_readonly => IngnoreReadonly
+        })
+    of
         ok ->
             {200};
         %% bad hocon format
-        {error, MsgList = [{_, _} | _]} ->
-            JsonFun = fun(K, V) -> {K, emqx_utils_maps:binary_string(V)} end,
-            JsonMap = emqx_utils_maps:jsonable_map(maps:from_list(MsgList), JsonFun),
-            {400, #{<<"content-type">> => <<"text/plain">>}, JsonMap};
-        {error, Msg} ->
+        {error, Errors} ->
+            Msg = emqx_logger_jsonfmt:best_effort_json_obj(#{errors => Errors}),
             {400, #{<<"content-type">> => <<"text/plain">>}, Msg}
     end.
 

apps/emqx_management/test/emqx_mgmt_api_configs_SUITE.erl

@@ -331,18 +331,20 @@ t_configs_key(_Config) ->
         Log
     ),
     Log1 = emqx_utils_maps:deep_put([<<"log">>, <<"console">>, <<"level">>], Log, <<"error">>),
-    ?assertEqual(<<>>, update_configs_with_binary(iolist_to_binary(hocon_pp:do(Log1, #{})))),
+    ?assertEqual({ok, <<>>}, update_configs_with_binary(iolist_to_binary(hocon_pp:do(Log1, #{})))),
     ?assertEqual(<<"error">>, read_conf([<<"log">>, <<"console">>, <<"level">>])),
     BadLog = emqx_utils_maps:deep_put([<<"log">>, <<"console">>, <<"level">>], Log, <<"erro1r">>),
     {error, Error} = update_configs_with_binary(iolist_to_binary(hocon_pp:do(BadLog, #{}))),
     ExpectError = #{
-        <<"log">> =>
-            #{
-                <<"kind">> => <<"validation_error">>,
-                <<"path">> => <<"log.console.level">>,
-                <<"reason">> => <<"unable_to_convert_to_enum_symbol">>,
-                <<"value">> => <<"erro1r">>
-            }
+        <<"errors">> => #{
+            <<"log">> =>
+                #{
+                    <<"kind">> => <<"validation_error">>,
+                    <<"path">> => <<"log.console.level">>,
+                    <<"reason">> => <<"unable_to_convert_to_enum_symbol">>,
+                    <<"value">> => <<"erro1r">>
+                }
+        }
     },
     ?assertEqual(ExpectError, emqx_utils_json:decode(Error, [return_maps])),
     ReadOnlyConf = #{
@@ -355,7 +357,8 @@ t_configs_key(_Config) ->
     },
     ReadOnlyBin = iolist_to_binary(hocon_pp:do(ReadOnlyConf, #{})),
     {error, ReadOnlyError} = update_configs_with_binary(ReadOnlyBin),
-    ?assertEqual(<<"Cannot update read-only key 'cluster'.">>, ReadOnlyError),
+    ?assertEqual(<<"{\"errors\":\"Cannot update read-only key 'cluster'.\"}">>, ReadOnlyError),
+    ?assertMatch({ok, <<>>}, update_configs_with_binary(ReadOnlyBin, _InogreReadonly = true)),
     ok.
 
 t_get_configs_in_different_accept(_Config) ->
@@ -405,7 +408,7 @@ t_create_webhook_v1_bridges_api(Config) ->
     WebHookFile = filename:join(?config(data_dir, Config), "webhook_v1.conf"),
     ?assertMatch({ok, _}, hocon:files([WebHookFile])),
     {ok, WebHookBin} = file:read_file(WebHookFile),
-    ?assertEqual(<<>>, update_configs_with_binary(WebHookBin)),
+    ?assertEqual({ok, <<>>}, update_configs_with_binary(WebHookBin)),
     Actions =
         #{
             <<"http">> =>
@@ -487,6 +490,22 @@ t_create_webhook_v1_bridges_api(Config) ->
     ?assertEqual(#{<<"webhook">> => #{}}, emqx_conf:get_raw([<<"bridges">>])),
     ok.
 
+t_config_update_parse_error(_Config) ->
+    ?assertMatch(
+        {error, <<"{\"errors\":\"{parse_error,", _/binary>>},
+        update_configs_with_binary(<<"not an object">>)
+    ),
+    ?assertMatch(
+        {error, <<"{\"errors\":\"{parse_error,", _/binary>>},
+        update_configs_with_binary(<<"a = \"tlsv1\"\"\"3e-01">>)
+    ).
+
+t_config_update_unknown_root(_Config) ->
+    ?assertMatch(
+        {error, <<"{\"errors\":{\"a\":\"{root_key_not_found,", _/binary>>},
+        update_configs_with_binary(<<"a = \"tlsv1.3\"">>)
+    ).
+
 %% Helpers
 
 get_config(Name) ->
@@ -539,18 +558,29 @@ get_configs_with_binary(Key, Node) ->
     end.
 
 update_configs_with_binary(Bin) ->
-    Path = emqx_mgmt_api_test_util:api_path(["configs"]),
+    update_configs_with_binary(Bin, _InogreReadonly = undefined).
+
+update_configs_with_binary(Bin, IgnoreReadonly) ->
+    Path =
+        case IgnoreReadonly of
+            undefined ->
+                emqx_mgmt_api_test_util:api_path(["configs"]);
+            Boolean ->
+                emqx_mgmt_api_test_util:api_path([
+                    "configs?ignore_readonly=" ++ atom_to_list(Boolean)
+                ])
+        end,
     Auth = emqx_mgmt_api_test_util:auth_header_(),
     Headers = [{"accept", "text/plain"}, Auth],
     case httpc:request(put, {Path, Headers, "text/plain", Bin}, [], [{body_format, binary}]) of
         {ok, {{"HTTP/1.1", Code, _}, _Headers, Body}} when
             Code >= 200 andalso Code =< 299
         ->
-            Body;
-        {ok, {{"HTTP/1.1", _Code, _}, _Headers, Body}} ->
+            {ok, Body};
+        {ok, {{"HTTP/1.1", 400, _}, _Headers, Body}} ->
             {error, Body};
         Error ->
-            Error
+            error({unexpected, Error})
     end.
 
 update_config(Name, Change) ->

changes/ce/feat-12940.en.md

@@ -0,0 +1,12 @@
+Add `ignore_readonly` argument to `PUT /configs` API.
+
+Prior to this change, EMQX would retrun 400 (BAD_REQUEST) if the raw config
+included readonly root keys (`cluster`, `rpc`, and `node`).
+
+After this enhancement it can be called as `PUT /configs?ignore_readonly=true`,
+EMQX will in this case ignore readonly root config keys, and apply the rest.
+
+For observability purposes, an info level message is logged if any readonly keys are dropped.
+
+Also fixed an exception when config has bad HOCON syntax (returns 500).
+Now bad syntax will cause the API to return 400 (BAD_REQUEST).

mix.exs

@@ -72,7 +72,7 @@ defmodule EMQXUmbrella.MixProject do
       # in conflict by emqtt and hocon
       {:getopt, "1.0.2", override: true},
       {:snabbkaffe, github: "kafka4beam/snabbkaffe", tag: "1.0.8", override: true},
-      {:hocon, github: "emqx/hocon", tag: "0.42.1", override: true},
+      {:hocon, github: "emqx/hocon", tag: "0.42.2", override: true},
       {:emqx_http_lib, github: "emqx/emqx_http_lib", tag: "0.5.3", override: true},
       {:esasl, github: "emqx/esasl", tag: "0.2.1"},
       {:jose, github: "potatosalad/erlang-jose", tag: "1.11.2"},

rebar.config

@@ -97,7 +97,7 @@
     {system_monitor, {git, "https://github.com/ieQu1/system_monitor", {tag, "3.0.3"}}},
     {getopt, "1.0.2"},
     {snabbkaffe, {git, "https://github.com/kafka4beam/snabbkaffe.git", {tag, "1.0.8"}}},
-    {hocon, {git, "https://github.com/emqx/hocon.git", {tag, "0.42.1"}}},
+    {hocon, {git, "https://github.com/emqx/hocon.git", {tag, "0.42.2"}}},
     {emqx_http_lib, {git, "https://github.com/emqx/emqx_http_lib.git", {tag, "0.5.3"}}},
     {esasl, {git, "https://github.com/emqx/esasl", {tag, "0.2.1"}}},
     {jose, {git, "https://github.com/potatosalad/erlang-jose", {tag, "1.11.2"}}},