From 99869821fb39a989bac13e421749d1dd431d629b Mon Sep 17 00:00:00 2001 From: JimMoen Date: Thu, 23 May 2024 16:31:58 +0800 Subject: [PATCH 1/4] fix: ensure plugin config map after fresh install --- apps/emqx_plugins/src/emqx_plugins.erl | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/apps/emqx_plugins/src/emqx_plugins.erl b/apps/emqx_plugins/src/emqx_plugins.erl index ae97e7f5f..f8134d8ba 100644 --- a/apps/emqx_plugins/src/emqx_plugins.erl +++ b/apps/emqx_plugins/src/emqx_plugins.erl @@ -161,6 +161,7 @@ ensure_installed(NameVsn) -> ok = purge(NameVsn), case ensure_exists_and_installed(NameVsn) of ok -> + _ = maybe_ensure_plugin_config(NameVsn), maybe_post_op_after_installed(NameVsn); {error, _Reason} = Err -> Err @@ -1167,7 +1168,9 @@ ensure_plugin_config(NameVsn, Nodes) -> case get_plugin_config_from_any_node(Nodes, NameVsn, []) of {ok, ConfigMap} when is_map(ConfigMap) -> HoconBin = hocon_pp:do(ConfigMap, #{}), - ok = file:write_file(plugin_config_file(NameVsn), HoconBin), + Path = plugin_config_file(NameVsn), + ok = filelib:ensure_dir(Path), + ok = file:write_file(Path, HoconBin), ensure_config_map(NameVsn); _ -> ?SLOG(error, #{msg => "config_not_found_from_cluster", name_vsn => NameVsn}), @@ -1184,9 +1187,11 @@ cp_default_config_file(NameVsn) -> true ?= filelib:is_regular(Source), %% destination path not existed (not configured) true ?= (not filelib:is_regular(Destination)), + ok = filelib:ensure_dir(Destination), case file:copy(Source, Destination) of {ok, _} -> - ok; + ok, + ensure_config_map(NameVsn); {error, Reason} -> ?SLOG(warning, #{ msg => "failed_to_copy_plugin_default_hocon_config", From 25a9aa1797e0ee0ad74627ebaef9f0cfdec6ca65 Mon Sep 17 00:00:00 2001 From: JimMoen Date: Thu, 23 May 2024 17:07:55 +0800 Subject: [PATCH 2/4] fix: allow put plugin config without schema --- .../src/emqx_mgmt_api_plugins.erl | 21 ++++++-- .../proto/emqx_mgmt_api_plugins_proto_v3.erl | 7 +-- apps/emqx_plugins/include/emqx_plugins.hrl | 1 + apps/emqx_plugins/src/emqx_plugins.erl | 50 +++++++++++++------ 4 files changed, 57 insertions(+), 22 deletions(-) diff --git a/apps/emqx_management/src/emqx_mgmt_api_plugins.erl b/apps/emqx_management/src/emqx_mgmt_api_plugins.erl index ed5f016b3..9f3697268 100644 --- a/apps/emqx_management/src/emqx_mgmt_api_plugins.erl +++ b/apps/emqx_management/src/emqx_mgmt_api_plugins.erl @@ -20,6 +20,7 @@ -include_lib("typerefl/include/types.hrl"). -include_lib("emqx/include/logger.hrl"). -include_lib("emqx_plugins/include/emqx_plugins.hrl"). +-include_lib("erlavro/include/erlavro.hrl"). -dialyzer({no_match, [format_plugin_avsc_and_i18n/1]}). @@ -506,14 +507,20 @@ plugin_config(get, #{bindings := #{name := NameVsn}}) -> {404, plugin_not_found_msg()} end; plugin_config(put, #{bindings := #{name := NameVsn}, body := AvroJsonMap}) -> + Nodes = emqx:running_nodes(), case emqx_plugins:describe(NameVsn) of {ok, _} -> case emqx_plugins:decode_plugin_config_map(NameVsn, AvroJsonMap) of - {ok, AvroValueConfig} -> - Nodes = emqx:running_nodes(), + {ok, ?plugin_without_config_schema} -> + %% no plugin avro schema, just put the json map it as-is + _Res = emqx_mgmt_api_plugins_proto_v3:update_plugin_config( + Nodes, NameVsn, AvroJsonMap, ?plugin_without_config_schema + ), + {204}; + {ok, AvroValue} -> %% cluster call with config in map (binary key-value) _Res = emqx_mgmt_api_plugins_proto_v3:update_plugin_config( - Nodes, NameVsn, AvroJsonMap, AvroValueConfig + Nodes, NameVsn, AvroJsonMap, AvroValue ), {204}; {error, Reason} -> @@ -604,9 +611,13 @@ ensure_action(Name, restart) -> ok. %% for RPC plugin avro encoded config update -do_update_plugin_config(NameVsn, AvroJsonMap, PluginConfigMap) -> +-spec do_update_plugin_config( + name_vsn(), map(), avro_value() | ?plugin_without_config_schema +) -> + ok. +do_update_plugin_config(NameVsn, AvroJsonMap, AvroValue) -> %% TODO: maybe use `PluginConfigMap` to validate config - emqx_plugins:put_config(NameVsn, AvroJsonMap, PluginConfigMap). + emqx_plugins:put_config(NameVsn, AvroJsonMap, AvroValue). %%-------------------------------------------------------------------- %% Helper functions diff --git a/apps/emqx_management/src/proto/emqx_mgmt_api_plugins_proto_v3.erl b/apps/emqx_management/src/proto/emqx_mgmt_api_plugins_proto_v3.erl index 641d35f70..d221a5e2a 100644 --- a/apps/emqx_management/src/proto/emqx_mgmt_api_plugins_proto_v3.erl +++ b/apps/emqx_management/src/proto/emqx_mgmt_api_plugins_proto_v3.erl @@ -28,6 +28,7 @@ ]). -include_lib("emqx/include/bpapi.hrl"). +-include_lib("emqx_plugins/include/emqx_plugins.hrl"). introduced_in() -> "5.7.0". @@ -56,14 +57,14 @@ ensure_action(Name, Action) -> [node()], binary() | string(), binary(), - map() + map() | ?plugin_without_config_schema ) -> emqx_rpc:multicall_result(). -update_plugin_config(Nodes, NameVsn, AvroJsonMap, PluginConfig) -> +update_plugin_config(Nodes, NameVsn, AvroJsonMap, MaybeAvroValue) -> rpc:multicall( Nodes, emqx_mgmt_api_plugins, do_update_plugin_config, - [NameVsn, AvroJsonMap, PluginConfig], + [NameVsn, AvroJsonMap, MaybeAvroValue], 10000 ). diff --git a/apps/emqx_plugins/include/emqx_plugins.hrl b/apps/emqx_plugins/include/emqx_plugins.hrl index 66f23d4b0..3c7621ca7 100644 --- a/apps/emqx_plugins/include/emqx_plugins.hrl +++ b/apps/emqx_plugins/include/emqx_plugins.hrl @@ -25,6 +25,7 @@ -define(CONFIG_FORMAT_MAP, config_format_map). -define(plugin_conf_not_found, plugin_conf_not_found). +-define(plugin_without_config_schema, plugin_without_config_schema). -type schema_name() :: binary(). -type avsc_path() :: string(). diff --git a/apps/emqx_plugins/src/emqx_plugins.erl b/apps/emqx_plugins/src/emqx_plugins.erl index f8134d8ba..856748146 100644 --- a/apps/emqx_plugins/src/emqx_plugins.erl +++ b/apps/emqx_plugins/src/emqx_plugins.erl @@ -329,10 +329,11 @@ get_config_bin(NameVsn) -> %% @doc Update plugin's config. %% RPC call from Management API or CLI. -%% the plugin config Json Map and plugin config ALWAYS be valid before calling this function. -put_config(NameVsn, ConfigJsonMap, DecodedPluginConfig) when not is_binary(NameVsn) -> - put_config(bin(NameVsn), ConfigJsonMap, DecodedPluginConfig); -put_config(NameVsn, ConfigJsonMap, _DecodedPluginConfig) -> +%% The plugin config Json Map was valid by avro schema +%% Or: if no and plugin config ALWAYS be valid before calling this function. +put_config(NameVsn, ConfigJsonMap, AvroValue) when not is_binary(NameVsn) -> + put_config(bin(NameVsn), ConfigJsonMap, AvroValue); +put_config(NameVsn, ConfigJsonMap, _AvroValue) -> HoconBin = hocon_pp:do(ConfigJsonMap, #{}), ok = backup_and_write_hocon_bin(NameVsn, HoconBin), %% TODO: callback in plugin's on_config_changed (config update by mgmt API) @@ -370,10 +371,30 @@ list() -> %%-------------------------------------------------------------------- %% Package utils --spec decode_plugin_config_map(name_vsn(), map() | binary()) -> {ok, map()} | {error, any()}. -decode_plugin_config_map(NameVsn, AvroJsonMap) when is_map(AvroJsonMap) -> - decode_plugin_config_map(NameVsn, emqx_utils_json:encode(AvroJsonMap)); -decode_plugin_config_map(NameVsn, AvroJsonBin) -> +-spec decode_plugin_config_map(name_vsn(), map() | binary()) -> + {ok, map() | ?plugin_without_config_schema} + | {error, any()}. +decode_plugin_config_map(NameVsn, AvroJsonMap) -> + case with_plugin_avsc(NameVsn) of + true -> + case emqx_plugins_serde:lookup_serde(NameVsn) of + {error, not_found} -> + Reason = "plugin_config_schema_serde_not_found", + ?SLOG(error, #{ + msg => Reason, name_vsn => NameVsn, plugin_with_avro_schema => true + }), + {error, Reason}; + {ok, _Serde} -> + do_decode_plugin_config_map(NameVsn, AvroJsonMap) + end; + false -> + ?SLOG(debug, #{name_vsn => NameVsn, plugin_with_avro_schema => false}), + {ok, ?plugin_without_config_schema} + end. + +do_decode_plugin_config_map(NameVsn, AvroJsonMap) when is_map(AvroJsonMap) -> + do_decode_plugin_config_map(NameVsn, emqx_utils_json:encode(AvroJsonMap)); +do_decode_plugin_config_map(NameVsn, AvroJsonBin) -> case emqx_plugins_serde:decode(NameVsn, AvroJsonBin) of {ok, Config} -> {ok, Config}; {error, ReasonMap} -> {error, ReasonMap} @@ -1205,14 +1226,15 @@ cp_default_config_file(NameVsn) -> end. ensure_config_map(NameVsn) -> - with_plugin_avsc(NameVsn) andalso - do_ensure_config_map(NameVsn). - -do_ensure_config_map(NameVsn) -> case read_plugin_hocon(NameVsn, #{read_mode => ?JSON_MAP}) of {ok, ConfigJsonMap} -> - {ok, Config} = decode_plugin_config_map(NameVsn, ConfigJsonMap), - put_config(NameVsn, ConfigJsonMap, Config); + case with_plugin_avsc(NameVsn) of + true -> + {ok, AvroValue} = decode_plugin_config_map(NameVsn, ConfigJsonMap), + put_config(NameVsn, ConfigJsonMap, AvroValue); + false -> + put_config(NameVsn, ConfigJsonMap, ?plugin_without_config_schema) + end; _ -> ?SLOG(warning, #{msg => "failed_to_read_plugin_config_hocon", name_vsn => NameVsn}), ok From 5dc96a37e2e5b69be2604449bd1a1949bb9ea0e2 Mon Sep 17 00:00:00 2001 From: JimMoen Date: Thu, 23 May 2024 17:46:58 +0800 Subject: [PATCH 3/4] fix: ensure plugin avsc serde before config --- apps/emqx_plugins/src/emqx_plugins.erl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/emqx_plugins/src/emqx_plugins.erl b/apps/emqx_plugins/src/emqx_plugins.erl index 856748146..670475cfb 100644 --- a/apps/emqx_plugins/src/emqx_plugins.erl +++ b/apps/emqx_plugins/src/emqx_plugins.erl @@ -161,8 +161,8 @@ ensure_installed(NameVsn) -> ok = purge(NameVsn), case ensure_exists_and_installed(NameVsn) of ok -> - _ = maybe_ensure_plugin_config(NameVsn), - maybe_post_op_after_installed(NameVsn); + maybe_post_op_after_installed(NameVsn), + _ = maybe_ensure_plugin_config(NameVsn); {error, _Reason} = Err -> Err end From a40c7d646ab276d2dbb1c81e1d7ef95b6f42c777 Mon Sep 17 00:00:00 2001 From: JimMoen Date: Thu, 23 May 2024 20:28:14 +0800 Subject: [PATCH 4/4] fix: apply suggestions from code review, thanks @zmstone --- apps/emqx_management/src/emqx_mgmt_api_plugins.erl | 4 ++-- apps/emqx_plugins/src/emqx_plugins.erl | 9 +++++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/apps/emqx_management/src/emqx_mgmt_api_plugins.erl b/apps/emqx_management/src/emqx_mgmt_api_plugins.erl index 9f3697268..a60131cfc 100644 --- a/apps/emqx_management/src/emqx_mgmt_api_plugins.erl +++ b/apps/emqx_management/src/emqx_mgmt_api_plugins.erl @@ -512,7 +512,7 @@ plugin_config(put, #{bindings := #{name := NameVsn}, body := AvroJsonMap}) -> {ok, _} -> case emqx_plugins:decode_plugin_config_map(NameVsn, AvroJsonMap) of {ok, ?plugin_without_config_schema} -> - %% no plugin avro schema, just put the json map it as-is + %% no plugin avro schema, just put the json map as-is _Res = emqx_mgmt_api_plugins_proto_v3:update_plugin_config( Nodes, NameVsn, AvroJsonMap, ?plugin_without_config_schema ), @@ -616,7 +616,7 @@ ensure_action(Name, restart) -> ) -> ok. do_update_plugin_config(NameVsn, AvroJsonMap, AvroValue) -> - %% TODO: maybe use `PluginConfigMap` to validate config + %% TODO: maybe use `AvroValue` to validate config emqx_plugins:put_config(NameVsn, AvroJsonMap, AvroValue). %%-------------------------------------------------------------------- diff --git a/apps/emqx_plugins/src/emqx_plugins.erl b/apps/emqx_plugins/src/emqx_plugins.erl index 670475cfb..42372f2ec 100644 --- a/apps/emqx_plugins/src/emqx_plugins.erl +++ b/apps/emqx_plugins/src/emqx_plugins.erl @@ -162,7 +162,8 @@ ensure_installed(NameVsn) -> case ensure_exists_and_installed(NameVsn) of ok -> maybe_post_op_after_installed(NameVsn), - _ = maybe_ensure_plugin_config(NameVsn); + _ = maybe_ensure_plugin_config(NameVsn), + ok; {error, _Reason} = Err -> Err end @@ -1166,6 +1167,7 @@ do_create_config_dir(NameVsn) -> end end. +-spec maybe_ensure_plugin_config(name_vsn()) -> ok. maybe_ensure_plugin_config(NameVsn) -> maybe true ?= with_plugin_avsc(NameVsn), @@ -1174,10 +1176,13 @@ maybe_ensure_plugin_config(NameVsn) -> _ -> ok end. +-spec ensure_plugin_config(name_vsn()) -> ok. ensure_plugin_config(NameVsn) -> %% fetch plugin hocon config from cluster Nodes = [N || N <- mria:running_nodes(), N /= node()], ensure_plugin_config(NameVsn, Nodes). + +-spec ensure_plugin_config(name_vsn(), list()) -> ok. ensure_plugin_config(NameVsn, []) -> ?SLOG(debug, #{ msg => "default_plugin_config_used", @@ -1200,6 +1205,7 @@ ensure_plugin_config(NameVsn, Nodes) -> cp_default_config_file(NameVsn) end. +-spec cp_default_config_file(name_vsn()) -> ok. cp_default_config_file(NameVsn) -> %% always copy default hocon file into config dir when can not get config from other nodes Source = default_plugin_config_file(NameVsn), @@ -1211,7 +1217,6 @@ cp_default_config_file(NameVsn) -> ok = filelib:ensure_dir(Destination), case file:copy(Source, Destination) of {ok, _} -> - ok, ensure_config_map(NameVsn); {error, Reason} -> ?SLOG(warning, #{