From 77d4faa632be899265eefe3f098b1db137cdb291 Mon Sep 17 00:00:00 2001
From: JianBo He <heeejianbo@gmail.com>
Date: Tue, 14 Jun 2022 16:40:34 +0800
Subject: [PATCH] fix(gw): not updating the authenticator using re-creation

---
 .../src/emqx_gateway_insta_sup.erl            | 72 ++++++++++++-------
 apps/emqx_gateway/test/emqx_exproto_SUITE.erl |  1 -
 apps/emqx_gateway/test/emqx_gateway_SUITE.erl |  4 +-
 .../test/emqx_sn_protocol_SUITE.erl           |  4 --
 4 files changed, 48 insertions(+), 33 deletions(-)

diff --git a/apps/emqx_gateway/src/emqx_gateway_insta_sup.erl b/apps/emqx_gateway/src/emqx_gateway_insta_sup.erl
index 27c9bdfca..a8e2d2f40 100644
--- a/apps/emqx_gateway/src/emqx_gateway_insta_sup.erl
+++ b/apps/emqx_gateway/src/emqx_gateway_insta_sup.erl
@@ -265,8 +265,11 @@ ensure_authn_running(State = #state{ctx = Ctx, authns = Authns}) ->
     ),
     State#state{ctx = maps:put(auth, AuthnNames, Ctx)}.
 
+do_update_authenticator({ChainName, Confs}) ->
+    do_update_authenticator(ChainName, Confs).
+
 do_update_authenticator(ChainName, Confs) ->
-    [#{id := AuthenticatorId}] = emqx_authentication:list_authenticators(ChainName),
+    {ok, [#{id := AuthenticatorId}]} = emqx_authentication:list_authenticators(ChainName),
     {ok, _} = emqx_authentication:update_authenticator(ChainName, AuthenticatorId, Confs),
     ok.
 
@@ -278,21 +281,19 @@ do_update_authenticator(ChainName, Confs) ->
 init_authn(GwName, Config) ->
     Authns = authns(GwName, Config),
     try
-        _ = application:ensure_all_started(emqx_authn),
-        do_init_authn(Authns, [])
+        ok = do_init_authn(Authns),
+        Authns
     catch
         throw:Reason = {badauth, _} ->
-            do_deinit_authn(proplists:get_keys(Authns)),
+            do_deinit_authn(Authns),
             throw(Reason)
     end.
 
-do_init_authn([], Authns) ->
-    lists:reverse(Authns);
-do_init_authn([{ChainName, AuthConf} | More], Authns) when is_map(AuthConf) ->
+do_init_authn([]) ->
+    ok;
+do_init_authn([{ChainName, AuthConf} | More]) when is_map(AuthConf) ->
     ok = do_create_authn_chain(ChainName, AuthConf),
-    do_init_authn(More, [{ChainName, AuthConf} | Authns]);
-do_init_authn([_BadConf | More], Authns) ->
-    do_init_authn(More, Authns).
+    do_init_authn(More).
 
 authns(GwName, Config) ->
     Listeners = maps:to_list(maps:get(listeners, Config, #{})),
@@ -332,9 +333,9 @@ do_create_authn_chain(ChainName, AuthConf) ->
             throw({badauth, Reason})
     end.
 
-do_deinit_authn(Names) ->
+do_deinit_authn(Authns) ->
     lists:foreach(
-        fun(ChainName) ->
+        fun({ChainName, _}) ->
             case emqx_authentication:delete_chain(ChainName) of
                 ok ->
                     ok;
@@ -348,7 +349,7 @@ do_deinit_authn(Names) ->
                     })
             end
         end,
-        Names
+        Authns
     ).
 
 do_update_one_by_one(
@@ -361,8 +362,8 @@ do_update_one_by_one(
 ) ->
     NEnable = maps:get(enable, NCfg, true),
 
-    OAuths = authns(GwName, OCfg),
-    NAuths = authns(GwName, NCfg),
+    OAuthns = authns(GwName, OCfg),
+    NAuthns = authns(GwName, NCfg),
 
     case {Status, NEnable} of
         {stopped, true} ->
@@ -371,16 +372,11 @@ do_update_one_by_one(
         {stopped, false} ->
             {ok, State#state{config = NCfg}};
         {running, true} ->
-            NState =
-                case NAuths == OAuths of
-                    true ->
-                        State;
-                    false ->
-                        %% Reset Authentication first
-                        _ = do_deinit_authn(State#state.authns),
-                        Authns = init_authn(State#state.name, NCfg),
-                        State#state{authns = Authns}
-                end,
+            {Added, Updated, Deleted} = diff_auths(NAuthns, OAuthns),
+            _ = do_deinit_authn(Deleted),
+            _ = do_init_authn(Added),
+            _ = lists:foreach(fun do_update_authenticator/1, Updated),
+            NState = State#state{authns = NAuthns},
             %% TODO: minimum impact update ???
             cb_gateway_update(NCfg, NState);
         {running, false} ->
@@ -392,6 +388,31 @@ do_update_one_by_one(
             throw(nomatch)
     end.
 
+diff_auths(NAuthns, OAuthns) ->
+    NNames = proplists:get_keys(NAuthns),
+    ONames = proplists:get_keys(OAuthns),
+    AddedNames = NNames -- ONames,
+    DeletedNames = ONames -- NNames,
+    BothNames = NNames -- AddedNames,
+    UpdatedNames = lists:foldl(
+        fun(Name, Acc) ->
+            case
+                proplists:get_value(Name, NAuthns) ==
+                    proplists:get_value(Name, OAuthns)
+            of
+                true -> Acc;
+                false -> [Name | Acc]
+            end
+        end,
+        [],
+        BothNames
+    ),
+    {
+        lists:filter(fun({Name, _}) -> lists:member(Name, AddedNames) end, NAuthns),
+        lists:filter(fun({Name, _}) -> lists:member(Name, UpdatedNames) end, NAuthns),
+        lists:filter(fun({Name, _}) -> lists:member(Name, DeletedNames) end, OAuthns)
+    }.
+
 cb_gateway_unload(
     State = #state{
         name = GwName,
@@ -404,7 +425,6 @@ cb_gateway_unload(
         CbMod:on_gateway_unload(Gateway, GwState),
         {ok, State#state{
             child_pids = [],
-            authns = [],
             status = stopped,
             gw_state = undefined,
             started_at = undefined,
diff --git a/apps/emqx_gateway/test/emqx_exproto_SUITE.erl b/apps/emqx_gateway/test/emqx_exproto_SUITE.erl
index 9e1c3e6e9..244fadd2b 100644
--- a/apps/emqx_gateway/test/emqx_exproto_SUITE.erl
+++ b/apps/emqx_gateway/test/emqx_exproto_SUITE.erl
@@ -72,7 +72,6 @@ init_per_group(GrpName, Cfg) ->
     put(grpname, GrpName),
     Svrs = emqx_exproto_echo_svr:start(),
     emqx_common_test_helpers:start_apps([emqx_gateway], fun set_special_cfg/1),
-    emqx_logger:set_log_level(debug),
     [{servers, Svrs}, {listener_type, GrpName} | Cfg].
 
 end_per_group(_, Cfg) ->
diff --git a/apps/emqx_gateway/test/emqx_gateway_SUITE.erl b/apps/emqx_gateway/test/emqx_gateway_SUITE.erl
index 99bb05983..0bed52a76 100644
--- a/apps/emqx_gateway/test/emqx_gateway_SUITE.erl
+++ b/apps/emqx_gateway/test/emqx_gateway_SUITE.erl
@@ -34,7 +34,7 @@ all() -> emqx_common_test_helpers:all(?MODULE).
 init_per_suite(Conf) ->
     emqx_config:erase(gateway),
     emqx_common_test_helpers:load_config(emqx_gateway_schema, ?CONF_DEFAULT),
-    emqx_common_test_helpers:start_apps([emqx_gateway]),
+    emqx_common_test_helpers:start_apps([emqx_authn, emqx_gateway]),
     Conf.
 
 end_per_suite(_Conf) ->
@@ -44,7 +44,7 @@ end_per_suite(_Conf) ->
 
 init_per_testcase(t_get_basic_usage_info_2, Config) ->
     DataDir = ?config(data_dir, Config),
-    emqx_common_test_helpers:stop_apps([emqx_gateway]),
+    application:stop(emqx_gateway),
     ok = setup_fake_usage_data(DataDir),
     Config;
 init_per_testcase(_TestCase, Config) ->
diff --git a/apps/emqx_gateway/test/emqx_sn_protocol_SUITE.erl b/apps/emqx_gateway/test/emqx_sn_protocol_SUITE.erl
index 41e007770..22b87f772 100644
--- a/apps/emqx_gateway/test/emqx_sn_protocol_SUITE.erl
+++ b/apps/emqx_gateway/test/emqx_sn_protocol_SUITE.erl
@@ -1914,8 +1914,6 @@ t_register_subs_resume_on(_) ->
     _ = emqx:publish(emqx_message:make(test, ?QOS_1, <<"topic-b">>, <<"m2">>)),
     _ = emqx:publish(emqx_message:make(test, ?QOS_2, <<"topic-b">>, <<"m3">>)),
 
-    emqx_logger:set_log_level(debug),
-
     {ok, NSocket} = gen_udp:open(0, [binary]),
     send_connect_msg(NSocket, <<"test">>, 0),
     ?assertMatch(
@@ -2088,8 +2086,6 @@ t_register_skip_failure_topic_name_and_reach_max_retry_times(_) ->
     ?assertMatch(<<2, ?SN_DISCONNECT>>, receive_response(Socket)),
     gen_udp:close(Socket),
 
-    emqx_logger:set_log_level(debug),
-
     {ok, NSocket} = gen_udp:open(0, [binary]),
     send_connect_msg(NSocket, <<"test">>, 0),
     ?assertMatch(