fix(gw): enhance the authn resources managing logic

apps/emqx_gateway/src/emqx_gateway.app.src

@@ -4,7 +4,7 @@
     {vsn, "0.1.0"},
     {registered, []},
     {mod, {emqx_gateway_app, []}},
-    {applications, [kernel, stdlib, grpc, emqx]},
+    {applications, [kernel, stdlib, grpc, emqx, emqx_authn]},
     {env, []},
     {modules, []},
     {licenses, ["Apache 2.0"]},

apps/emqx_gateway/src/emqx_gateway_ctx.erl

@@ -29,6 +29,7 @@
     #{
         %% Gateway Name
         gwname := gateway_name(),
+        %% FIXME: use process name instead of pid()
         %% The ConnectionManager PID
         cm := pid()
     }.

apps/emqx_gateway/src/emqx_gateway_insta_sup.erl

@@ -45,7 +45,6 @@
     name :: gateway_name(),
     config :: emqx_config:config(),
     ctx :: emqx_gateway_ctx:context(),
-    authns :: [{emqx_authentication:chain_name(), map()}],
     status :: stopped | running,
     child_pids :: [pid()],
     gw_state :: emqx_gateway_impl:state() | undefined,
@@ -101,13 +100,14 @@ init([Gateway, Ctx, _GwDscrptr]) ->
     State = #state{
         ctx = Ctx,
         name = GwName,
-        authns = [],
         config = Config,
         child_pids = [],
         status = stopped,
         created_at = erlang:system_time(millisecond)
     },
-    case maps:get(enable, Config, true) of
+    Enable = maps:get(enable, Config, true),
+    ok = ensure_authn_running(State, Enable),
+    case Enable of
         false ->
             ?SLOG(info, #{
                 msg => "skip_to_start_gateway_due_to_disabled",
@@ -115,11 +115,11 @@ init([Gateway, Ctx, _GwDscrptr]) ->
             }),
             {ok, State};
         true ->
-            case cb_gateway_load(ensure_authn_created(State)) of
+            case cb_gateway_load(State) of
                 {error, Reason} ->
                     {stop, Reason};
-                {ok, NState1} ->
+                {ok, NState} ->
-                    {ok, NState1}
+                    {ok, NState}
             end
     end.
 
@@ -130,7 +130,8 @@ handle_call(disable, _From, State = #state{status = Status}) ->
         running ->
             case cb_gateway_unload(State) of
                 {ok, NState} ->
-                    {reply, ok, disable_authns(NState)};
+                    ok = disable_authns(State),
+                    {reply, ok, NState};
                 {error, Reason} ->
                     {reply, {error, Reason}, State}
             end;
@@ -140,7 +141,8 @@ handle_call(disable, _From, State = #state{status = Status}) ->
 handle_call(enable, _From, State = #state{status = Status}) ->
     case Status of
         stopped ->
-            case cb_gateway_load(ensure_authn_running(State)) of
+            ok = ensure_authn_running(State),
+            case cb_gateway_load(State) of
                 {error, Reason} ->
                     {reply, {error, Reason}, State};
                 {ok, NState1} ->
@@ -210,7 +212,7 @@ handle_info(Info, State) ->
 
 terminate(_Reason, State = #state{child_pids = Pids}) ->
     Pids /= [] andalso (_ = cb_gateway_unload(State)),
-    _ = do_deinit_authn(State#state.authns),
+    _ = remove_all_authns(State),
     ok.
 
 code_change(_OldVsn, State, _Extra) ->
@@ -236,65 +238,61 @@ detailed_gateway_info(State) ->
 %%--------------------------------------------------------------------
 %% Authn resources managing funcs
 
-%% ensure authentication chain, authenticator created and keep its status
+%% ensure authentication chain, authenticator created and keep its configured
-%% as expected
+%% status
-ensure_authn_created(State = #state{ctx = Ctx, name = GwName, config = Config}) ->
+ensure_authn_running(#state{name = GwName, config = Config}) ->
-    Authns = init_authn(GwName, Config),
-    AuthnNames = lists:map(fun({ChainName, _}) -> ChainName end, Authns),
-    State#state{authns = Authns, ctx = maps:put(auth, AuthnNames, Ctx)}.
-
-%% temporarily disable authenticators after gateway disabled
-disable_authns(State = #state{ctx = Ctx, authns = Authns}) ->
     lists:foreach(
         fun({ChainName, AuthConf}) ->
-            TempConf = maps:put(enable, false, AuthConf),
+            ok = ensure_authenticator_created(ChainName, AuthConf)
-            do_update_authenticator(ChainName, TempConf)
         end,
-        Authns
+        authns(GwName, Config)
-    ),
+    ).
-    State#state{ctx = maps:remove(auth, Ctx)}.
 
-%% keep authenticators running as expected
+%% ensure authentication chain, authenticator created and keep its status
-ensure_authn_running(State = #state{ctx = Ctx, authns = Authns}) ->
+%% as given
-    AuthnNames = lists:map(
+ensure_authn_running(#state{name = GwName, config = Config}, Enable) ->
+    lists:foreach(
         fun({ChainName, AuthConf}) ->
-            ok = do_update_authenticator(ChainName, AuthConf),
+            ok = ensure_authenticator_created(ChainName, AuthConf#{enable => Enable})
-            ChainName
         end,
-        Authns
+        authns(GwName, Config)
-    ),
+    ).
-    State#state{ctx = maps:put(auth, AuthnNames, Ctx)}.
 
-do_update_authenticator({ChainName, Confs}) ->
+%% temporarily disable authenticators after gateway disabled
-    do_update_authenticator(ChainName, Confs).
+disable_authns(State) ->
+    ensure_authn_running(State, false).
 
-do_update_authenticator(ChainName, Confs) ->
+%% remove all authns if gateway unloaded
-    {ok, [#{id := AuthenticatorId}]} = emqx_authentication:list_authenticators(ChainName),
+remove_all_authns(#state{name = GwName, config = Config}) ->
-    {ok, _} = emqx_authentication:update_authenticator(ChainName, AuthenticatorId, Confs),
+    lists:foreach(
-    ok.
+        fun({ChainName, _}) ->
+            case emqx_authentication:delete_chain(ChainName) of
+                ok ->
+                    ok;
+                {error, {not_found, _}} ->
+                    ok;
+                {error, Reason} ->
+                    ?SLOG(error, #{
+                        msg => "failed_to_clean_authn_chain",
+                        chain_name => ChainName,
+                        reason => Reason
+                    })
+            end
+        end,
+        authns(GwName, Config)
+    ).
 
-%% There are two layer authentication configs
+ensure_authenticator_created(ChainName, Confs) ->
-%%       stomp.authn
+    case emqx_authentication:list_authenticators(ChainName) of
-%%           /                   \
+        {ok, [#{id := AuthenticatorId}]} ->
-%%   listeners.tcp.default.authn  *.ssl.default.authn
+            {ok, _} = emqx_authentication:update_authenticator(ChainName, AuthenticatorId, Confs),
-%%
+            ok;
-init_authn(GwName, Config) ->
+        {ok, []} ->
-    Authns = authns(GwName, Config),
+            ok = do_create_authenticator(ChainName, Confs);
-    try
+        {error, {not_found, {chain, _}}} ->
-        ok = do_init_authn(Authns),
+            ok = do_create_authenticator(ChainName, Confs)
-        Authns
-    catch
-        throw:Reason = {badauth, _} ->
-            do_deinit_authn(Authns),
-            throw(Reason)
     end.
 
-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).
-
 authns(GwName, Config) ->
     Listeners = maps:to_list(maps:get(listeners, Config, #{})),
     Authns0 =
@@ -319,7 +317,7 @@ authns(GwName, Config) ->
 authn_conf(Conf) ->
     maps:get(authentication, Conf, undefined).
 
-do_create_authn_chain(ChainName, AuthConf) ->
+do_create_authenticator(ChainName, AuthConf) ->
     case emqx_authentication:create_authenticator(ChainName, AuthConf) of
         {ok, _} ->
             ok;
@@ -333,25 +331,6 @@ do_create_authn_chain(ChainName, AuthConf) ->
             throw({badauth, Reason})
     end.
 
-do_deinit_authn(Authns) ->
-    lists:foreach(
-        fun({ChainName, _}) ->
-            case emqx_authentication:delete_chain(ChainName) of
-                ok ->
-                    ok;
-                {error, {not_found, _}} ->
-                    ok;
-                {error, Reason} ->
-                    ?SLOG(error, #{
-                        msg => "failed_to_clean_authn_chain",
-                        chain_name => ChainName,
-                        reason => Reason
-                    })
-            end
-        end,
-        Authns
-    ).
-
 do_update_one_by_one(
     NCfg,
     State = #state{
@@ -365,53 +344,41 @@ do_update_one_by_one(
     OAuthns = authns(GwName, OCfg),
     NAuthns = authns(GwName, NCfg),
 
+    ok = remove_deleted_authns(NAuthns, OAuthns),
+
     case {Status, NEnable} of
         {stopped, true} ->
-            NState = State#state{config = NCfg},
+            ok = ensure_authn_running(State#state{config = NCfg}),
-            cb_gateway_load(ensure_authn_running(NState));
+            cb_gateway_load(State#state{config = NCfg});
         {stopped, false} ->
+            ok = disable_authns(State#state{config = NCfg}),
             {ok, State#state{config = NCfg}};
         {running, true} ->
-            {Added, Updated, Deleted} = diff_auths(NAuthns, OAuthns),
+            %% FIXME: minimum impact update
-            _ = do_deinit_authn(Deleted),
+            ok = ensure_authn_running(State#state{config = NCfg}),
-            _ = do_init_authn(Added),
+            cb_gateway_update(NCfg, State);
-            _ = lists:foreach(fun do_update_authenticator/1, Updated),
-            NState = State#state{authns = NAuthns},
-            %% TODO: minimum impact update ???
-            cb_gateway_update(NCfg, NState);
         {running, false} ->
             case cb_gateway_unload(State) of
-                {ok, NState} -> {ok, disable_authns(NState#state{config = NCfg})};
+                {ok, NState} ->
-                {error, Reason} -> {error, Reason}
+                    ok = disable_authns(State#state{config = NCfg}),
+                    {ok, NState#state{config = NCfg}};
+                {error, Reason} ->
+                    {error, Reason}
             end;
         _ ->
             throw(nomatch)
     end.
 
-diff_auths(NAuthns, OAuthns) ->
+remove_deleted_authns(NAuthns, OAuthns) ->
     NNames = proplists:get_keys(NAuthns),
     ONames = proplists:get_keys(OAuthns),
-    AddedNames = NNames -- ONames,
     DeletedNames = ONames -- NNames,
-    BothNames = NNames -- AddedNames,
+    lists:foreach(
-    UpdatedNames = lists:foldl(
+        fun(ChainName) ->
-        fun(Name, Acc) ->
+            _ = emqx_authentication:delete_chain(ChainName)
-            case
-                proplists:get_value(Name, NAuthns) ==
-                    proplists:get_value(Name, OAuthns)
-            of
-                true -> Acc;
-                false -> [Name | Acc]
-            end
         end,
-        [],
+        DeletedNames
-        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{
@@ -461,7 +428,6 @@ cb_gateway_load(
             {ok, ChildPidOrSpecs, GwState} ->
                 ChildPids = start_child_process(ChildPidOrSpecs),
                 {ok, State#state{
-                    ctx = Ctx,
                     status = running,
                     child_pids = ChildPids,
                     gw_state = GwState,
@@ -475,7 +441,6 @@ cb_gateway_load(
                 msg => "load_gateway_crashed",
                 gateway_name => GwName,
                 gateway => Gateway,
-                ctx => Ctx,
                 reason => {Class, Reason1},
                 stacktrace => Stk
             }),

apps/emqx_gateway/test/emqx_gateway_authz_SUITE.erl

@@ -69,7 +69,7 @@ init_per_suite(Config) ->
     init_gateway_conf(),
     meck:new(emqx_authz_file, [non_strict, passthrough, no_history, no_link]),
     meck:expect(emqx_authz_file, create, fun(S) -> S end),
-    emqx_mgmt_api_test_util:init_suite([emqx_conf, emqx_authz, emqx_gateway]),
+    emqx_mgmt_api_test_util:init_suite([emqx_conf, emqx_authz, emqx_authn, emqx_gateway]),
     application:ensure_all_started(cowboy),
     emqx_gateway_auth_ct:start(),
     Config.
@@ -79,7 +79,7 @@ end_per_suite(Config) ->
     emqx_gateway_auth_ct:stop(),
     ok = emqx_authz_test_lib:restore_authorizers(),
     emqx_config:erase(gateway),
-    emqx_mgmt_api_test_util:end_suite([cowboy, emqx_authz, emqx_gateway]),
+    emqx_mgmt_api_test_util:end_suite([cowboy, emqx_authz, emqx_authn, emqx_gateway]),
     Config.
 
 init_per_testcase(_Case, Config) ->

apps/emqx_gateway/test/emqx_gateway_registry_SUITE.erl

@@ -38,11 +38,11 @@ all() -> emqx_common_test_helpers:all(?MODULE).
 
 init_per_suite(Cfg) ->
     ok = 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]),
     Cfg.
 
 end_per_suite(_Cfg) ->
-    emqx_common_test_helpers:stop_apps([emqx_gateway]),
+    emqx_common_test_helpers:stop_apps([emqx_gateway, emqx_authn]),
     ok.
 
 %%--------------------------------------------------------------------

apps/emqx_gateway/test/emqx_lwm2m_api_SUITE.erl

@@ -107,6 +107,7 @@ end_per_testcase(_AllTestCase, Config) ->
     gen_udp:close(?config(sock, Config)),
     emqtt:disconnect(?config(emqx_c, Config)),
     ok = application:stop(emqx_gateway),
+    ok = application:stop(emqx_authn),
     timer:sleep(300).
 
 %%--------------------------------------------------------------------