From 7eea6934222df565ac1cc99583680c3c89f2146a Mon Sep 17 00:00:00 2001 From: Zhongwen Deng Date: Thu, 1 Jun 2023 17:24:09 +0800 Subject: [PATCH] fix: reorder authn when updating --- apps/emqx/src/emqx_authentication.erl | 33 ++++- apps/emqx/src/emqx_authentication_config.erl | 36 +++--- apps/emqx/test/emqx_authentication_SUITE.erl | 13 +- apps/emqx_authn/test/emqx_authn_SUITE.erl | 121 +++++++++++++++++++ apps/emqx_conf/src/emqx_conf_cli.erl | 18 +-- apps/emqx_ctl/src/emqx_ctl.erl | 6 +- apps/emqx_ctl/test/emqx_ctl_SUITE.erl | 4 +- 7 files changed, 201 insertions(+), 30 deletions(-) diff --git a/apps/emqx/src/emqx_authentication.erl b/apps/emqx/src/emqx_authentication.erl index 29d637fa5..cce789f24 100644 --- a/apps/emqx/src/emqx_authentication.erl +++ b/apps/emqx/src/emqx_authentication.erl @@ -60,7 +60,8 @@ update_authenticator/3, lookup_authenticator/2, list_authenticators/1, - move_authenticator/3 + move_authenticator/3, + reorder_authenticator/2 ]). %% APIs for observer built_in_database @@ -401,6 +402,12 @@ list_authenticators(ChainName) -> move_authenticator(ChainName, AuthenticatorID, Position) -> call({move_authenticator, ChainName, AuthenticatorID, Position}). +-spec reorder_authenticator(chain_name(), [authenticator_id()]) -> ok. +reorder_authenticator(_ChainName, []) -> + ok; +reorder_authenticator(ChainName, AuthenticatorIDs) -> + call({reorder_authenticator, ChainName, AuthenticatorIDs}). + -spec import_users(chain_name(), authenticator_id(), {binary(), binary()}) -> ok | {error, term()}. import_users(ChainName, AuthenticatorID, Filename) -> @@ -493,6 +500,12 @@ handle_call({move_authenticator, ChainName, AuthenticatorID, Position}, _From, S end, Reply = with_chain(ChainName, UpdateFun), reply(Reply, State); +handle_call({reorder_authenticator, ChainName, AuthenticatorIDs}, _From, State) -> + UpdateFun = fun(Chain) -> + handle_reorder_authenticator(Chain, AuthenticatorIDs) + end, + Reply = with_chain(ChainName, UpdateFun), + reply(Reply, State); handle_call({import_users, ChainName, AuthenticatorID, Filename}, _From, State) -> Reply = call_authenticator(ChainName, AuthenticatorID, import_users, [Filename]), reply(Reply, State); @@ -598,6 +611,24 @@ handle_move_authenticator(Chain, AuthenticatorID, Position) -> {error, Reason} end. +handle_reorder_authenticator(Chain, AuthenticatorIDs) -> + #chain{authenticators = Authenticators} = Chain, + NAuthenticators = + lists:filtermap( + fun(ID) -> + case lists:keyfind(ID, #authenticator.id, Authenticators) of + false -> + ?SLOG(error, #{msg => "authenticator_not_found", id => ID}), + false; + Authenticator -> + {true, Authenticator} + end + end, + AuthenticatorIDs + ), + NewChain = Chain#chain{authenticators = NAuthenticators}, + {ok, ok, NewChain}. + handle_create_authenticator(Chain, Config, Providers) -> #chain{name = Name, authenticators = Authenticators} = Chain, AuthenticatorID = authenticator_id(Config), diff --git a/apps/emqx/src/emqx_authentication_config.erl b/apps/emqx/src/emqx_authentication_config.erl index f9246a5a3..a1b55ea43 100644 --- a/apps/emqx/src/emqx_authentication_config.erl +++ b/apps/emqx/src/emqx_authentication_config.erl @@ -202,7 +202,28 @@ do_post_config_update(Paths, _UpdateReq, NewConfig0, OldConfig0, _AppEnvs) -> NewConfig = to_list(NewConfig0), OldIds = lists:map(fun authenticator_id/1, OldConfig), NewIds = lists:map(fun authenticator_id/1, NewConfig), - %% delete authenticators that are not in the new config + ok = delete_authenticators(NewIds, ChainName, OldConfig), + ok = create_or_update_authenticators(OldIds, ChainName, NewConfig), + ok = emqx_authentication:reorder_authenticator(ChainName, NewIds), + ok. + +%% create new authenticators and update existing ones +create_or_update_authenticators(OldIds, ChainName, NewConfig) -> + lists:foreach( + fun(Conf) -> + Id = authenticator_id(Conf), + case lists:member(Id, OldIds) of + true -> + emqx_authentication:update_authenticator(ChainName, Id, Conf); + false -> + emqx_authentication:create_authenticator(ChainName, Conf) + end + end, + NewConfig + ). + +%% delete authenticators that are not in the new config +delete_authenticators(NewIds, ChainName, OldConfig) -> lists:foreach( fun(Conf) -> Id = authenticator_id(Conf), @@ -216,19 +237,6 @@ do_post_config_update(Paths, _UpdateReq, NewConfig0, OldConfig0, _AppEnvs) -> end end, OldConfig - ), - %% create new authenticators and update existing ones - lists:foreach( - fun(Conf) -> - Id = authenticator_id(Conf), - case lists:member(Id, OldIds) of - true -> - emqx_authentication:update_authenticator(ChainName, Id, Conf); - false -> - emqx_authentication:create_authenticator(ChainName, Conf) - end - end, - NewConfig ). to_list(undefined) -> []; diff --git a/apps/emqx/test/emqx_authentication_SUITE.erl b/apps/emqx/test/emqx_authentication_SUITE.erl index 5f3e28bf3..71ef44d49 100644 --- a/apps/emqx/test/emqx_authentication_SUITE.erl +++ b/apps/emqx/test/emqx_authentication_SUITE.erl @@ -174,7 +174,7 @@ t_authenticator(Config) when is_list(Config) -> register_provider(AuthNType1, ?MODULE), ID1 = <<"password_based:built_in_database">>, - % CRUD of authencaticator + % CRUD of authenticator ?assertMatch( {ok, #{id := ID1, state := #{mark := 1}}}, ?AUTHN:create_authenticator(ChainName, AuthenticatorConfig1) @@ -355,6 +355,10 @@ t_update_config(Config) when is_list(Config) -> ?assertMatch({ok, [#{id := ID2}, #{id := ID1}]}, ?AUTHN:list_authenticators(Global)), + [Raw2, Raw1] = emqx:get_raw_config([?CONF_ROOT]), + ?assertMatch({ok, _}, update_config([?CONF_ROOT], [Raw1, Raw2])), + ?assertMatch({ok, [#{id := ID1}, #{id := ID2}]}, ?AUTHN:list_authenticators(Global)), + ?assertMatch({ok, _}, update_config([?CONF_ROOT], {delete_authenticator, Global, ID1})), ?assertEqual( {error, {not_found, {authenticator, ID1}}}, @@ -417,11 +421,16 @@ t_update_config(Config) when is_list(Config) -> {ok, _}, update_config(ConfKeyPath, {move_authenticator, ListenerID, ID2, ?CMD_MOVE_FRONT}) ), - ?assertMatch( {ok, [#{id := ID2}, #{id := ID1}]}, ?AUTHN:list_authenticators(ListenerID) ), + [LRaw2, LRaw1] = emqx:get_raw_config(ConfKeyPath), + ?assertMatch({ok, _}, update_config(ConfKeyPath, [LRaw1, LRaw2])), + ?assertMatch( + {ok, [#{id := ID1}, #{id := ID2}]}, + ?AUTHN:list_authenticators(ListenerID) + ), ?assertMatch( {ok, _}, diff --git a/apps/emqx_authn/test/emqx_authn_SUITE.erl b/apps/emqx_authn/test/emqx_authn_SUITE.erl index 4f96ca2dd..d5df4add3 100644 --- a/apps/emqx_authn/test/emqx_authn_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_SUITE.erl @@ -200,6 +200,127 @@ t_union_selector_errors(Config) when is_list(Config) -> ), ok. +t_update_conf({init, Config}) -> + emqx_common_test_helpers:start_apps([emqx_conf, emqx_authn]), + {ok, _} = emqx:update_config([authentication], []), + Config; +t_update_conf({'end', _Config}) -> + {ok, _} = emqx:update_config([authentication], []), + emqx_common_test_helpers:stop_apps([emqx_authn, emqx_conf]), + ok; +t_update_conf(Config) when is_list(Config) -> + Authn1 = #{ + <<"mechanism">> => <<"password_based">>, + <<"backend">> => <<"built_in_database">>, + <<"user_id_type">> => <<"clientid">>, + <<"enable">> => true + }, + Authn2 = #{ + <<"mechanism">> => <<"password_based">>, + <<"backend">> => <<"http">>, + <<"method">> => <<"post">>, + <<"url">> => <<"http://127.0.0.1:18083">>, + <<"headers">> => #{ + <<"content-type">> => <<"application/json">> + }, + <<"enable">> => true + }, + Authn3 = #{ + <<"mechanism">> => <<"jwt">>, + <<"use_jwks">> => false, + <<"algorithm">> => <<"hmac-based">>, + <<"secret">> => <<"mysecret">>, + <<"secret_base64_encoded">> => false, + <<"verify_claims">> => #{<<"username">> => <<"${username}">>}, + <<"enable">> => true + }, + Chain = 'mqtt:global', + {ok, _} = emqx:update_config([authentication], [Authn1]), + ?assertMatch( + {ok, #{ + authenticators := [ + #{ + enable := true, + id := <<"password_based:built_in_database">>, + provider := emqx_authn_mnesia + } + ] + }}, + emqx_authentication:lookup_chain(Chain) + ), + + {ok, _} = emqx:update_config([authentication], [Authn1, Authn2, Authn3]), + ?assertMatch( + {ok, #{ + authenticators := [ + #{ + enable := true, + id := <<"password_based:built_in_database">>, + provider := emqx_authn_mnesia + }, + #{ + enable := true, + id := <<"password_based:http">>, + provider := emqx_authn_http + }, + #{ + enable := true, + id := <<"jwt">>, + provider := emqx_authn_jwt + } + ] + }}, + emqx_authentication:lookup_chain(Chain) + ), + {ok, _} = emqx:update_config([authentication], [Authn2, Authn1]), + ?assertMatch( + {ok, #{ + authenticators := [ + #{ + enable := true, + id := <<"password_based:http">>, + provider := emqx_authn_http + }, + #{ + enable := true, + id := <<"password_based:built_in_database">>, + provider := emqx_authn_mnesia + } + ] + }}, + emqx_authentication:lookup_chain(Chain) + ), + + {ok, _} = emqx:update_config([authentication], [Authn3, Authn2, Authn1]), + ?assertMatch( + {ok, #{ + authenticators := [ + #{ + enable := true, + id := <<"jwt">>, + provider := emqx_authn_jwt + }, + #{ + enable := true, + id := <<"password_based:http">>, + provider := emqx_authn_http + }, + #{ + enable := true, + id := <<"password_based:built_in_database">>, + provider := emqx_authn_mnesia + } + ] + }}, + emqx_authentication:lookup_chain(Chain) + ), + {ok, _} = emqx:update_config([authentication], []), + ?assertMatch( + {error, {not_found, {chain, Chain}}}, + emqx_authentication:lookup_chain(Chain) + ), + ok. + parse(Bytes) -> {ok, Frame, <<>>, {none, _}} = emqx_frame:parse(Bytes), Frame. diff --git a/apps/emqx_conf/src/emqx_conf_cli.erl b/apps/emqx_conf/src/emqx_conf_cli.erl index 7c391d2d1..9240d2116 100644 --- a/apps/emqx_conf/src/emqx_conf_cli.erl +++ b/apps/emqx_conf/src/emqx_conf_cli.erl @@ -33,21 +33,23 @@ unload() -> emqx_ctl:unregister_command(?CLUSTER_CALL), emqx_ctl:unregister_command(?CONF). -conf(["print", "--only-keys"]) -> +conf(["show", "--keys-only"]) -> print(emqx_config:get_root_names()); -conf(["print"]) -> +conf(["show"]) -> print_hocon(get_config()); -conf(["print", Key]) -> +conf(["show", Key]) -> print_hocon(get_config(Key)); conf(["load", Path]) -> load_config(Path); conf(_) -> emqx_ctl:usage( [ + %% TODO add reload %{"conf reload", "reload etc/emqx.conf on local node"}, - {"conf print --only-keys", "print all keys"}, - {"conf print", "print all running configures"}, - {"conf print ", "print a specific configuration"} + {"conf show --keys-only", "print all keys"}, + {"conf show", "print all running configures"}, + {"conf show ", "print a specific configuration"}, + {"conf load ", "load a hocon file to all nodes"} ] ). @@ -137,6 +139,6 @@ load_config(Path) -> Conf ); {error, Reason} -> - emqx_ctl:print("load ~ts failed: ~p~n", [Path, Reason]), - {error, Reason} + emqx_ctl:print("load ~ts failed~n~p~n", [Path, Reason]), + {error, bad_hocon_file} end. diff --git a/apps/emqx_ctl/src/emqx_ctl.erl b/apps/emqx_ctl/src/emqx_ctl.erl index b27b6a468..d2ced7268 100644 --- a/apps/emqx_ctl/src/emqx_ctl.erl +++ b/apps/emqx_ctl/src/emqx_ctl.erl @@ -135,7 +135,7 @@ run_command(Cmd, Args) when is_atom(Cmd) -> -spec lookup_command(cmd()) -> [{module(), atom()}]. lookup_command(Cmd) when is_atom(Cmd) -> - case is_init() of + case is_initialized() of true -> case ets:match(?CMD_TAB, {{'_', Cmd}, '$1', '_'}) of [El] -> El; @@ -150,7 +150,7 @@ get_commands() -> [{Cmd, M, F} || {{_Seq, Cmd}, {M, F}, _Opts} <- ets:tab2list(?CMD_TAB)]. help() -> - case is_init() of + case is_initialized() of true -> case ets:tab2list(?CMD_TAB) of [] -> @@ -290,5 +290,5 @@ safe_to_existing_atom(Str) -> undefined end. -is_init() -> +is_initialized() -> ets:info(?CMD_TAB) =/= undefined. diff --git a/apps/emqx_ctl/test/emqx_ctl_SUITE.erl b/apps/emqx_ctl/test/emqx_ctl_SUITE.erl index 46d9008e8..c11a1d5cb 100644 --- a/apps/emqx_ctl/test/emqx_ctl_SUITE.erl +++ b/apps/emqx_ctl/test/emqx_ctl_SUITE.erl @@ -49,8 +49,8 @@ t_reg_unreg_command(_) -> emqx_ctl:unregister_command(cmd1), emqx_ctl:unregister_command(cmd2), ct:sleep(100), - ?assertEqual([], emqx_ctl:lookup_command(cmd1)), - ?assertEqual([], emqx_ctl:lookup_command(cmd2)), + ?assertEqual({error, cmd_not_found}, emqx_ctl:lookup_command(cmd1)), + ?assertEqual({error, cmd_not_found}, emqx_ctl:lookup_command(cmd2)), ?assertEqual([], emqx_ctl:get_commands()) end ).