From 45a39d97c6c9a09dcbf5a5fed3693c4753794046 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Tue, 31 Oct 2023 16:39:47 -0300 Subject: [PATCH] fix(bridges_v1): avoid create dangling connectors when updating bridges via api v1 Fixes https://emqx.atlassian.net/browse/EMQX-11291 --- apps/emqx_bridge/src/emqx_bridge_v2.erl | 71 ++++++++++--------- ...qx_bridge_v1_compatibility_layer_SUITE.erl | 33 ++++++--- .../emqx_bridge_gcp_pubsub_impl_producer.erl | 9 +-- .../src/schema/emqx_connector_schema.erl | 45 +++++++----- apps/emqx_utils/src/emqx_utils_maps.erl | 8 ++- 5 files changed, 99 insertions(+), 67 deletions(-) diff --git a/apps/emqx_bridge/src/emqx_bridge_v2.erl b/apps/emqx_bridge/src/emqx_bridge_v2.erl index 20e345475..3747a4671 100644 --- a/apps/emqx_bridge/src/emqx_bridge_v2.erl +++ b/apps/emqx_bridge/src/emqx_bridge_v2.erl @@ -1068,21 +1068,29 @@ split_bridge_v1_config_and_create(BridgeV1Type, BridgeName, RawConf) -> case lookup_conf(BridgeV2Type, BridgeName) of {error, _} -> %% If the bridge v2 does not exist, it is a valid bridge v1 - split_bridge_v1_config_and_create_helper(BridgeV1Type, BridgeName, RawConf); + PreviousRawConf = undefined, + split_bridge_v1_config_and_create_helper( + BridgeV1Type, BridgeName, RawConf, PreviousRawConf + ); _Conf -> case ?MODULE:is_valid_bridge_v1(BridgeV1Type, BridgeName) of true -> %% Using remove + create as update, hence do not delete deps. RemoveDeps = [], + PreviousRawConf = emqx:get_raw_config( + [?ROOT_KEY, BridgeV2Type, BridgeName], undefined + ), bridge_v1_check_deps_and_remove(BridgeV1Type, BridgeName, RemoveDeps), - split_bridge_v1_config_and_create_helper(BridgeV1Type, BridgeName, RawConf); + split_bridge_v1_config_and_create_helper( + BridgeV1Type, BridgeName, RawConf, PreviousRawConf + ); false -> %% If the bridge v2 exists, it is not a valid bridge v1 {error, non_compatible_bridge_v2_exists} end end. -split_bridge_v1_config_and_create_helper(BridgeV1Type, BridgeName, RawConf) -> +split_bridge_v1_config_and_create_helper(BridgeV1Type, BridgeName, RawConf, PreviousRawConf) -> #{ connector_type := ConnectorType, connector_name := NewConnectorName, @@ -1091,7 +1099,7 @@ split_bridge_v1_config_and_create_helper(BridgeV1Type, BridgeName, RawConf) -> bridge_v2_name := BridgeName, bridge_v2_conf := NewBridgeV2RawConf } = - split_and_validate_bridge_v1_config(BridgeV1Type, BridgeName, RawConf), + split_and_validate_bridge_v1_config(BridgeV1Type, BridgeName, RawConf, PreviousRawConf), case emqx_connector:create(ConnectorType, NewConnectorName, NewConnectorRawConf) of {ok, _} -> case create(BridgeType, BridgeName, NewBridgeV2RawConf) of @@ -1116,14 +1124,14 @@ split_bridge_v1_config_and_create_helper(BridgeV1Type, BridgeName, RawConf) -> Error end. -split_and_validate_bridge_v1_config(BridgeV1Type, BridgeName, RawConf) -> +split_and_validate_bridge_v1_config(BridgeV1Type, BridgeName, RawConf, PreviousRawConf) -> %% Create fake global config for the transformation and then call - %% emqx_connector_schema:transform_bridges_v1_to_connectors_and_bridges_v2/1 + %% `emqx_connector_schema:transform_bridges_v1_to_connectors_and_bridges_v2/1' BridgeV2Type = ?MODULE:bridge_v1_type_to_bridge_v2_type(BridgeV1Type), ConnectorType = connector_type(BridgeV2Type), - %% Needed so name confligts will ba avoided + %% Needed to avoid name conflicts CurrentConnectorsConfig = emqx:get_raw_config([connectors], #{}), - FakeGlobalConfig = #{ + FakeGlobalConfig0 = #{ <<"connectors">> => CurrentConnectorsConfig, <<"bridges">> => #{ bin(BridgeV1Type) => #{ @@ -1131,6 +1139,13 @@ split_and_validate_bridge_v1_config(BridgeV1Type, BridgeName, RawConf) -> } } }, + FakeGlobalConfig = + emqx_utils_maps:put_if( + FakeGlobalConfig0, + bin(?ROOT_KEY), + #{bin(BridgeV2Type) => #{bin(BridgeName) => PreviousRawConf}}, + PreviousRawConf =/= undefined + ), Output = emqx_connector_schema:transform_bridges_v1_to_connectors_and_bridges_v2( FakeGlobalConfig ), @@ -1143,34 +1158,21 @@ split_and_validate_bridge_v1_config(BridgeV1Type, BridgeName, RawConf) -> ], Output ), - ConnectorsBefore = - maps:keys( - emqx_utils_maps:deep_get( - [ - <<"connectors">>, - bin(ConnectorType) - ], - FakeGlobalConfig, - #{} - ) - ), - ConnectorsAfter = - maps:keys( - emqx_utils_maps:deep_get( - [ - <<"connectors">>, - bin(ConnectorType) - ], - Output - ) - ), - [NewConnectorName] = ConnectorsAfter -- ConnectorsBefore, + ConnectorName = emqx_utils_maps:deep_get( + [ + bin(?ROOT_KEY), + bin(BridgeV2Type), + bin(BridgeName), + <<"connector">> + ], + Output + ), NewConnectorRawConf = emqx_utils_maps:deep_get( [ <<"connectors">>, bin(ConnectorType), - bin(NewConnectorName) + bin(ConnectorName) ], Output ), @@ -1178,7 +1180,7 @@ split_and_validate_bridge_v1_config(BridgeV1Type, BridgeName, RawConf) -> NewFakeGlobalConfig = #{ <<"connectors">> => #{ bin(ConnectorType) => #{ - bin(NewConnectorName) => NewConnectorRawConf + bin(ConnectorName) => NewConnectorRawConf } }, <<"bridges_v2">> => #{ @@ -1197,7 +1199,7 @@ split_and_validate_bridge_v1_config(BridgeV1Type, BridgeName, RawConf) -> _ -> #{ connector_type => ConnectorType, - connector_name => NewConnectorName, + connector_name => ConnectorName, connector_conf => NewConnectorRawConf, bridge_v2_type => BridgeV2Type, bridge_v2_name => BridgeName, @@ -1212,6 +1214,7 @@ split_and_validate_bridge_v1_config(BridgeV1Type, BridgeName, RawConf) -> bridge_v1_create_dry_run(BridgeType, RawConfig0) -> RawConf = maps:without([<<"name">>], RawConfig0), TmpName = iolist_to_binary([?TEST_ID_PREFIX, emqx_utils:gen_id(8)]), + PreviousRawConf = undefined, #{ connector_type := _ConnectorType, connector_name := _NewConnectorName, @@ -1219,7 +1222,7 @@ bridge_v1_create_dry_run(BridgeType, RawConfig0) -> bridge_v2_type := BridgeV2Type, bridge_v2_name := _BridgeName, bridge_v2_conf := BridgeV2RawConf - } = split_and_validate_bridge_v1_config(BridgeType, TmpName, RawConf), + } = split_and_validate_bridge_v1_config(BridgeType, TmpName, RawConf, PreviousRawConf), create_dry_run_helper(BridgeV2Type, ConnectorRawConf, BridgeV2RawConf). bridge_v1_check_deps_and_remove(BridgeV1Type, BridgeName, RemoveDeps) -> diff --git a/apps/emqx_bridge/test/emqx_bridge_v1_compatibility_layer_SUITE.erl b/apps/emqx_bridge/test/emqx_bridge_v1_compatibility_layer_SUITE.erl index 5dd2a0280..bfc3eedc5 100644 --- a/apps/emqx_bridge/test/emqx_bridge_v1_compatibility_layer_SUITE.erl +++ b/apps/emqx_bridge/test/emqx_bridge_v1_compatibility_layer_SUITE.erl @@ -100,12 +100,6 @@ setup_mocks() -> IsBridgeV2TypeFun = fun(Type) -> BridgeV2Type = bridge_type(), BridgeV2TypeBin = bridge_type_bin(), - ct:pal("is_bridge_v2_type mock: ~p", [ - #{ - input_type => Type, - expected => [BridgeV2Type, BridgeV2TypeBin] - } - ]), case Type of BridgeV2Type -> true; BridgeV2TypeBin -> true; @@ -333,7 +327,8 @@ get_connector_http(Name) -> create_bridge_http_api_v1(Opts) -> Name = maps:get(name, Opts), Overrides = maps:get(overrides, Opts, #{}), - BridgeConfig = emqx_utils_maps:deep_merge(bridge_config(), Overrides), + BridgeConfig0 = emqx_utils_maps:deep_merge(bridge_config(), Overrides), + BridgeConfig = maps:without([<<"connector">>], BridgeConfig0), Params = BridgeConfig#{<<"type">> => bridge_type_bin(), <<"name">> => Name}, Path = emqx_mgmt_api_test_util:api_path(["bridges"]), ct:pal("creating bridge (http v1): ~p", [Params]), @@ -352,6 +347,19 @@ create_bridge_http_api_v2(Opts) -> ct:pal("bridge create (http v2) result:\n ~p", [Res]), Res. +update_bridge_http_api_v1(Opts) -> + Name = maps:get(name, Opts), + BridgeId = emqx_bridge_resource:bridge_id(bridge_type(), Name), + Overrides = maps:get(overrides, Opts, #{}), + BridgeConfig0 = emqx_utils_maps:deep_merge(bridge_config(), Overrides), + BridgeConfig = maps:without([<<"connector">>], BridgeConfig0), + Params = BridgeConfig, + Path = emqx_mgmt_api_test_util:api_path(["bridges", BridgeId]), + ct:pal("updating bridge (http v1): ~p", [Params]), + Res = request(put, Path, Params), + ct:pal("bridge update (http v1) result:\n ~p", [Res]), + Res. + delete_bridge_http_api_v1(Opts) -> Name = maps:get(name, Opts), BridgeId = emqx_bridge_resource:bridge_id(bridge_type(), Name), @@ -515,8 +523,17 @@ t_scenario_1(_Config) -> ), %% =================================================================================== - %% TODO: Update the bridge using v1 API. + %% Update the bridge using v1 API. %% =================================================================================== + ?assertMatch( + {ok, {{_, 200, _}, _, _}}, + update_bridge_http_api_v1(#{name => NameA}) + ), + ?assertMatch({ok, {{_, 200, _}, _, [#{<<"name">> := NameA}]}}, list_bridges_http_api_v1()), + ?assertMatch({ok, {{_, 200, _}, _, [#{<<"name">> := NameA}]}}, list_bridges_http_api_v2()), + ?assertMatch({ok, {{_, 200, _}, _, [#{}, #{}]}}, list_connectors_http()), + ?assertMatch({ok, {{_, 200, _}, _, #{<<"name">> := NameA}}}, get_bridge_http_api_v1(NameA)), + ?assertMatch({ok, {{_, 200, _}, _, #{<<"name">> := NameA}}}, get_bridge_http_api_v2(NameA)), %% =================================================================================== %% Now create a new bridge_v2 pointing to the same connector. It should no longer be diff --git a/apps/emqx_bridge_gcp_pubsub/src/emqx_bridge_gcp_pubsub_impl_producer.erl b/apps/emqx_bridge_gcp_pubsub/src/emqx_bridge_gcp_pubsub_impl_producer.erl index dc5eb01aa..cd7568001 100644 --- a/apps/emqx_bridge_gcp_pubsub/src/emqx_bridge_gcp_pubsub_impl_producer.erl +++ b/apps/emqx_bridge_gcp_pubsub/src/emqx_bridge_gcp_pubsub_impl_producer.erl @@ -222,13 +222,8 @@ encode_payload(State, Selected) -> OrderingKey = render_key(OrderingKeyTemplate, Selected), Attributes = proc_attributes(AttributesTemplate, Selected), Payload0 = #{data => base64:encode(Data)}, - Payload1 = put_if(Payload0, attributes, Attributes, map_size(Attributes) > 0), - put_if(Payload1, 'orderingKey', OrderingKey, OrderingKey =/= <<>>). - -put_if(Acc, K, V, true) -> - Acc#{K => V}; -put_if(Acc, _K, _V, false) -> - Acc. + Payload1 = emqx_utils_maps:put_if(Payload0, attributes, Attributes, map_size(Attributes) > 0), + emqx_utils_maps:put_if(Payload1, 'orderingKey', OrderingKey, OrderingKey =/= <<>>). -spec render_payload(emqx_placeholder:tmpl_token(), map()) -> binary(). render_payload([] = _Template, Selected) -> diff --git a/apps/emqx_connector/src/schema/emqx_connector_schema.erl b/apps/emqx_connector/src/schema/emqx_connector_schema.erl index f8447fe8d..4d803fd79 100644 --- a/apps/emqx_connector/src/schema/emqx_connector_schema.erl +++ b/apps/emqx_connector/src/schema/emqx_connector_schema.erl @@ -71,21 +71,31 @@ has_connector_field(BridgeConf, ConnectorFields) -> ConnectorFields ). -bridge_configs_to_transform(_BridgeType, [] = _BridgeNameBridgeConfList, _ConnectorFields) -> +bridge_configs_to_transform( + _BridgeType, [] = _BridgeNameBridgeConfList, _ConnectorFields, _RawConfig +) -> []; -bridge_configs_to_transform(BridgeType, [{BridgeName, BridgeConf} | Rest], ConnectorFields) -> +bridge_configs_to_transform( + BridgeType, [{BridgeName, BridgeConf} | Rest], ConnectorFields, RawConfig +) -> case has_connector_field(BridgeConf, ConnectorFields) of true -> + PreviousRawConfig = + emqx_utils_maps:deep_get( + [<<"bridges_v2">>, to_bin(BridgeType), to_bin(BridgeName)], + RawConfig, + undefined + ), [ - {BridgeType, BridgeName, BridgeConf, ConnectorFields} - | bridge_configs_to_transform(BridgeType, Rest, ConnectorFields) + {BridgeType, BridgeName, BridgeConf, ConnectorFields, PreviousRawConfig} + | bridge_configs_to_transform(BridgeType, Rest, ConnectorFields, RawConfig) ]; false -> - bridge_configs_to_transform(BridgeType, Rest, ConnectorFields) + bridge_configs_to_transform(BridgeType, Rest, ConnectorFields, RawConfig) end. split_bridge_to_connector_and_action( - {ConnectorsMap, {BridgeType, BridgeName, BridgeConf, ConnectorFields}} + {ConnectorsMap, {BridgeType, BridgeName, BridgeConf, ConnectorFields, PreviousRawConfig}} ) -> %% Get connector fields from bridge config ConnectorMap = lists:foldl( @@ -122,8 +132,12 @@ split_bridge_to_connector_and_action( BridgeConf, ConnectorFields ), - %% Generate a connector name - ConnectorName = generate_connector_name(ConnectorsMap, BridgeName, 0), + %% Generate a connector name, if needed. Avoid doing so if there was a previous config. + ConnectorName = + case PreviousRawConfig of + #{<<"connector">> := ConnectorName0} -> ConnectorName0; + _ -> generate_connector_name(ConnectorsMap, BridgeName, 0) + end, %% Add connector field to action map ActionMap = maps:put(<<"connector">>, ConnectorName, ActionMap0), {BridgeType, BridgeName, ActionMap, ConnectorName, ConnectorMap}. @@ -152,20 +166,17 @@ transform_old_style_bridges_to_connector_and_actions_of_type( BridgeTypes = ?MODULE:connector_type_to_bridge_types(ConnectorType), BridgesConfMap = maps:get(<<"bridges">>, RawConfig, #{}), ConnectorsConfMap = maps:get(<<"connectors">>, RawConfig, #{}), - BridgeConfigsToTransform1 = - lists:foldl( - fun(BridgeType, ToTranformSoFar) -> + BridgeConfigsToTransform = + lists:flatmap( + fun(BridgeType) -> BridgeNameToBridgeMap = maps:get(to_bin(BridgeType), BridgesConfMap, #{}), BridgeNameBridgeConfList = maps:to_list(BridgeNameToBridgeMap), - NewToTransform = bridge_configs_to_transform( - BridgeType, BridgeNameBridgeConfList, ConnectorFields - ), - [NewToTransform, ToTranformSoFar] + bridge_configs_to_transform( + BridgeType, BridgeNameBridgeConfList, ConnectorFields, RawConfig + ) end, - [], BridgeTypes ), - BridgeConfigsToTransform = lists:flatten(BridgeConfigsToTransform1), ConnectorsWithTypeMap = maps:get(to_bin(ConnectorType), ConnectorsConfMap, #{}), BridgeConfigsToTransformWithConnectorConf = lists:zip( lists:duplicate(length(BridgeConfigsToTransform), ConnectorsWithTypeMap), diff --git a/apps/emqx_utils/src/emqx_utils_maps.erl b/apps/emqx_utils/src/emqx_utils_maps.erl index d49c90e53..3945b7201 100644 --- a/apps/emqx_utils/src/emqx_utils_maps.erl +++ b/apps/emqx_utils/src/emqx_utils_maps.erl @@ -33,7 +33,8 @@ diff_maps/2, best_effort_recursive_sum/3, if_only_to_toggle_enable/2, - update_if_present/3 + update_if_present/3, + put_if/4 ]). -export_type([config_key/0, config_key_path/0]). @@ -303,3 +304,8 @@ update_if_present(Key, Fun, Map) -> _ -> Map end. + +put_if(Acc, K, V, true) -> + Acc#{K => V}; +put_if(Acc, _K, _V, false) -> + Acc.