Merge pull request #12993 from zmstone/0508-fix-update-listener-zone-from-dashboard

0508 fix update listener zone from dashboard

apps/emqx/src/config/emqx_config_zones.erl

@@ -20,6 +20,7 @@
 %% API
 -export([add_handler/0, remove_handler/0, pre_config_update/3]).
 -export([is_olp_enabled/0]).
+-export([assert_zone_exists/1]).
 
 -define(ZONES, [zones]).
 
@@ -44,3 +45,26 @@ is_olp_enabled() ->
         false,
         emqx_config:get([zones], #{})
     ).
+
+-spec assert_zone_exists(binary() | atom()) -> ok.
+assert_zone_exists(Name0) when is_binary(Name0) ->
+    %% an existing zone must have already an atom-name
+    Name =
+        try
+            binary_to_existing_atom(Name0)
+        catch
+            _:_ ->
+                throw({unknown_zone, Name0})
+        end,
+    assert_zone_exists(Name);
+assert_zone_exists(default) ->
+    %% there is always a 'default' zone
+    ok;
+assert_zone_exists(Name) when is_atom(Name) ->
+    try
+        _ = emqx_config:get([zones, Name]),
+        ok
+    catch
+        error:{config_not_found, _} ->
+            throw({unknown_zone, Name})
+    end.

apps/emqx/src/emqx_listeners.erl

@@ -124,7 +124,7 @@ format_raw_listeners({Type0, Conf}) ->
                 Bind = parse_bind(LConf0),
                 MaxConn = maps:get(<<"max_connections">>, LConf0, default_max_conn()),
                 Running = is_running(Type, listener_id(Type, LName), LConf0#{bind => Bind}),
-                LConf1 = maps:without([<<"authentication">>, <<"zone">>], LConf0),
+                LConf1 = maps:without([<<"authentication">>], LConf0),
                 LConf2 = maps:put(<<"running">>, Running, LConf1),
                 CurrConn =
                     case Running of
@@ -526,6 +526,7 @@ pre_config_update([?ROOT_KEY, _Type, _Name], {update, _Request}, undefined) ->
 pre_config_update([?ROOT_KEY, Type, Name], {update, Request}, RawConf) ->
     RawConf1 = emqx_utils_maps:deep_merge(RawConf, Request),
     RawConf2 = ensure_override_limiter_conf(RawConf1, Request),
+    ok = assert_zone_exists(RawConf2),
     {ok, convert_certs(Type, Name, RawConf2)};
 pre_config_update([?ROOT_KEY, _Type, _Name], {action, _Action, Updated}, RawConf) ->
     {ok, emqx_utils_maps:deep_merge(RawConf, Updated)};
@@ -884,6 +885,11 @@ convert_certs(Type, Name, Conf) ->
 filter_stacktrace({Reason, _Stacktrace}) -> Reason;
 filter_stacktrace(Reason) -> Reason.
 
+assert_zone_exists(#{<<"zone">> := Zone}) ->
+    emqx_config_zones:assert_zone_exists(Zone);
+assert_zone_exists(_) ->
+    ok.
+
 %% limiter config should override, not merge
 ensure_override_limiter_conf(Conf, #{<<"limiter">> := Limiter}) ->
     Conf#{<<"limiter">> => Limiter};

apps/emqx_management/src/emqx_mgmt_api_listeners.erl

@@ -810,6 +810,7 @@ listener_id_status_example() ->
 
 tcp_schema_example() ->
     #{
+        type => tcp,
         acceptors => 16,
         access_rules => ["allow all"],
         bind => <<"0.0.0.0:1884">>,
@@ -820,6 +821,7 @@ tcp_schema_example() ->
         proxy_protocol => false,
         proxy_protocol_timeout => <<"3s">>,
         running => true,
+        zone => default,
         tcp_options => #{
             active_n => 100,
             backlog => 1024,
@@ -829,8 +831,7 @@ tcp_schema_example() ->
             reuseaddr => true,
             send_timeout => <<"15s">>,
             send_timeout_close => true
-        },
-        type => tcp
+        }
     }.
 
 create_listener(From, Body) ->

apps/emqx_management/test/emqx_mgmt_api_listeners_SUITE.erl

@@ -36,9 +36,12 @@ groups() ->
     MaxConnTests = [
         t_max_connection_default
     ],
+    ZoneTests = [
+        t_update_listener_zone
+    ],
     [
         {with_defaults_in_file, AllTests -- MaxConnTests},
-        {without_defaults_in_file, AllTests -- MaxConnTests},
+        {without_defaults_in_file, AllTests -- (MaxConnTests ++ ZoneTests)},
         {max_connections, MaxConnTests}
     ].
 
@@ -403,6 +406,21 @@ crud_listeners_by_id(ListenerId, NewListenerId, MinListenerId, BadId, Type, Port
     ?assertMatch({error, {"HTTP/1.1", 404, _}}, request(delete, NewPath, [], [])),
     ok.
 
+t_update_listener_zone({init, Config}) ->
+    %% fake a zone
+    Config;
+t_update_listener_zone({'end', _Config}) ->
+    ok;
+t_update_listener_zone(_Config) ->
+    ListenerId = <<"tcp:default">>,
+    Path = emqx_mgmt_api_test_util:api_path(["listeners", ListenerId]),
+    Conf = request(get, Path, [], []),
+    %% update
+    AddConf1 = Conf#{<<"zone">> => <<"unknownzone">>},
+    AddConf2 = Conf#{<<"zone">> => <<"zone1">>},
+    ?assertMatch({error, {_, 400, _}}, request(put, Path, [], AddConf1)),
+    ?assertMatch(#{<<"zone">> := <<"zone1">>}, request(put, Path, [], AddConf2)).
+
 t_delete_nonexistent_listener(Config) when is_list(Config) ->
     NonExist = emqx_mgmt_api_test_util:api_path(["listeners", "tcp:nonexistent"]),
     ?assertMatch(
@@ -518,5 +536,9 @@ cert_file(Name) ->
 default_listeners_hocon_text() ->
     Sc = #{roots => emqx_schema:listeners()},
     Listeners = hocon_tconf:make_serializable(Sc, #{}, #{}),
-    Config = #{<<"listeners">> => Listeners},
+    Zones = #{<<"zone1">> => #{<<"mqtt">> => #{<<"max_inflight">> => 2}}},
+    Config = #{
+        <<"listeners">> => Listeners,
+        <<"zones">> => Zones
+    },
     hocon_pp:do(Config, #{}).

changes/ce/fix-12993.en.md

@@ -0,0 +1,5 @@
+Fix listener config update API when handling an unknown zone.
+
+Prior to this fix, when a listener config is updated with an unknown zone, for example `{"zone": "unknown"}`,
+the change would be accepted, causing all clients to crash when connect.
+After this fix, updating listener with an unknown zone name will get a "Bad request" response.