From dcff9a219532b14119de93658a93c553b90cd3f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-S=C3=A9bastien=20P=C3=A9dron?= Date: Wed, 3 Aug 2022 17:38:11 +0200 Subject: [PATCH] Revert "Merge pull request #5409 from rabbitmq/mergify/bp/v3.11.x/pr-5236" Two reasons: 1. It was too early to backport. 2. There are at least two pull requests to backport first to avoid conflicts. This reverts commit 871f1ff6c77b45580cda5b54439bc3e2c8a91d2e, reversing changes made to adaf46b028cb7e8d1e13a17400b6b43755183e85. --- deps/rabbit/include/vhost.hrl | 4 +- deps/rabbit/include/vhost_v1.hrl | 4 + deps/rabbit/src/vhost.erl | 121 +++++++++++++----- deps/rabbit/src/vhost_v1.erl | 106 +++++++++++++++ deps/rabbit/test/cluster_SUITE.erl | 46 ++++--- deps/rabbit/test/definition_import_SUITE.erl | 41 +++--- deps/rabbit/test/quorum_queue_SUITE.erl | 4 + .../test/rabbit_mgmt_http_SUITE.erl | 18 ++- .../test/rabbit_stream_SUITE.erl | 1 + 9 files changed, 275 insertions(+), 70 deletions(-) create mode 100644 deps/rabbit/include/vhost_v1.hrl create mode 100644 deps/rabbit/src/vhost_v1.erl diff --git a/deps/rabbit/include/vhost.hrl b/deps/rabbit/include/vhost.hrl index 4f9b4aeae7f9..d3abc0dd2a5d 100644 --- a/deps/rabbit/include/vhost.hrl +++ b/deps/rabbit/include/vhost.hrl @@ -1,4 +1,6 @@ +-include("vhost_v1.hrl"). -include("vhost_v2.hrl"). -define(is_vhost(V), - (?is_vhost_v2(V))). + (?is_vhost_v2(V) orelse + ?is_vhost_v1(V))). diff --git a/deps/rabbit/include/vhost_v1.hrl b/deps/rabbit/include/vhost_v1.hrl new file mode 100644 index 000000000000..185739c6be0f --- /dev/null +++ b/deps/rabbit/include/vhost_v1.hrl @@ -0,0 +1,4 @@ +-define(is_vhost_v1(V), is_record(V, vhost, 3)). + +-define(vhost_v1_field_name(V), element(2, V)). +-define(vhost_v1_field_limits(V), element(3, V)). diff --git a/deps/rabbit/src/vhost.erl b/deps/rabbit/src/vhost.erl index cdd3ba3ce582..d6d7e8aac7a1 100644 --- a/deps/rabbit/src/vhost.erl +++ b/deps/rabbit/src/vhost.erl @@ -42,7 +42,7 @@ tags => [atom()], metadata_key() => any()} | undefined). --type vhost() :: vhost_v2(). +-type vhost() :: vhost_v1:vhost_v1() | vhost_v2(). -record(vhost, { %% name as a binary @@ -58,7 +58,8 @@ metadata :: metadata() }. --type vhost_pattern() :: vhost_v2_pattern(). +-type vhost_pattern() :: vhost_v1:vhost_v1_pattern() | + vhost_v2_pattern(). -type vhost_v2_pattern() :: #vhost{ virtual_host :: name() | '_', limits :: '_', @@ -75,63 +76,104 @@ -spec new(name(), list()) -> vhost(). new(Name, Limits) -> - #vhost{virtual_host = Name, limits = Limits}. + case record_version_to_use() of + ?record_version -> + #vhost{virtual_host = Name, limits = Limits}; + _ -> + vhost_v1:new(Name, Limits) + end. -spec new(name(), list(), map()) -> vhost(). new(Name, Limits, Metadata) -> - #vhost{virtual_host = Name, limits = Limits, metadata = Metadata}. + case record_version_to_use() of + ?record_version -> + #vhost{virtual_host = Name, limits = Limits, metadata = Metadata}; + _ -> + vhost_v1:new(Name, Limits) + end. --spec record_version_to_use() -> vhost_v2. +-spec record_version_to_use() -> vhost_v1 | vhost_v2. record_version_to_use() -> - ?record_version. + case rabbit_feature_flags:is_enabled(virtual_host_metadata) of + true -> ?record_version; + false -> vhost_v1:record_version_to_use() + end. -spec upgrade(vhost()) -> vhost(). -upgrade(#vhost{} = VHost) -> VHost. +upgrade(#vhost{} = VHost) -> VHost; +upgrade(OldVHost) -> upgrade_to(record_version_to_use(), OldVHost). --spec upgrade_to(vhost_v2, vhost()) -> vhost_v2(). +-spec upgrade_to +(vhost_v2, vhost()) -> vhost_v2(); +(vhost_v1, vhost_v1:vhost_v1()) -> vhost_v1:vhost_v1(). upgrade_to(?record_version, #vhost{} = VHost) -> - VHost. + VHost; +upgrade_to(?record_version, OldVHost) -> + Fields = erlang:tuple_to_list(OldVHost) ++ [#{description => <<"">>, tags => []}], + #vhost{} = erlang:list_to_tuple(Fields); +upgrade_to(Version, OldVHost) -> + vhost_v1:upgrade_to(Version, OldVHost). + fields() -> - fields(?record_version). + case record_version_to_use() of + ?record_version -> fields(?record_version); + _ -> vhost_v1:fields() + end. -fields(?record_version) -> record_info(fields, vhost). +fields(?record_version) -> record_info(fields, vhost); +fields(Version) -> vhost_v1:fields(Version). info_keys() -> - %% note: this reports description and tags separately even though - %% they are stored in the metadata map. MK. - [name, - description, - tags, - default_queue_type, - metadata, - tracing, - cluster_state]. + case record_version_to_use() of + %% note: this reports description and tags separately even though + %% they are stored in the metadata map. MK. + ?record_version -> + [name, + description, + tags, + default_queue_type, + metadata, + tracing, + cluster_state]; + _ -> + vhost_v1:info_keys() + end. -spec pattern_match_all() -> vhost_pattern(). pattern_match_all() -> - #vhost{_ = '_'}. + case record_version_to_use() of + ?record_version -> #vhost{_ = '_'}; + _ -> vhost_v1:pattern_match_all() + end. -spec get_name(vhost()) -> name(). -get_name(#vhost{virtual_host = Value}) -> Value. +get_name(#vhost{virtual_host = Value}) -> Value; +get_name(VHost) -> vhost_v1:get_name(VHost). -spec get_limits(vhost()) -> list(). -get_limits(#vhost{limits = Value}) -> Value. +get_limits(#vhost{limits = Value}) -> Value; +get_limits(VHost) -> vhost_v1:get_limits(VHost). -spec get_metadata(vhost()) -> metadata(). -get_metadata(#vhost{metadata = Value}) -> Value. +get_metadata(#vhost{metadata = Value}) -> Value; +get_metadata(VHost) -> vhost_v1:get_metadata(VHost). -spec get_description(vhost()) -> binary(). get_description(#vhost{} = VHost) -> - maps:get(description, get_metadata(VHost), undefined). + maps:get(description, get_metadata(VHost), undefined); +get_description(VHost) -> + vhost_v1:get_description(VHost). -spec get_tags(vhost()) -> [atom()]. get_tags(#vhost{} = VHost) -> - maps:get(tags, get_metadata(VHost), undefined). + maps:get(tags, get_metadata(VHost), undefined); +get_tags(VHost) -> + vhost_v1:get_tags(VHost). -spec get_default_queue_type(vhost()) -> binary() | undefined. get_default_queue_type(#vhost{} = VHost) -> @@ -140,17 +182,34 @@ get_default_queue_type(_VHost) -> undefined. set_limits(VHost, Value) -> - VHost#vhost{limits = Value}. + case record_version_to_use() of + ?record_version -> + VHost#vhost{limits = Value}; + _ -> + vhost_v1:set_limits(VHost, Value) + end. -spec set_metadata(vhost(), metadata()) -> vhost(). set_metadata(VHost, Value) -> - VHost#vhost{metadata = Value}. + case record_version_to_use() of + ?record_version -> + VHost#vhost{metadata = Value}; + _ -> + %% the field is not available, so this is a no-op + VHost + end. -spec merge_metadata(vhost(), metadata()) -> vhost(). merge_metadata(VHost, Value) -> - Meta0 = get_metadata(VHost), - NewMeta = maps:merge(Meta0, Value), - VHost#vhost{metadata = NewMeta}. + case record_version_to_use() of + ?record_version -> + Meta0 = get_metadata(VHost), + NewMeta = maps:merge(Meta0, Value), + VHost#vhost{metadata = NewMeta}; + _ -> + %% the field is not available, so this is a no-op + VHost + end. -spec is_tagged_with(vhost:vhost(), atom()) -> boolean(). is_tagged_with(VHost, Tag) -> diff --git a/deps/rabbit/src/vhost_v1.erl b/deps/rabbit/src/vhost_v1.erl new file mode 100644 index 000000000000..3d94b0cb4b8f --- /dev/null +++ b/deps/rabbit/src/vhost_v1.erl @@ -0,0 +1,106 @@ +%% This Source Code Form is subject to the terms of the Mozilla Public +%% License, v. 2.0. If a copy of the MPL was not distributed with this +%% file, You can obtain one at https://mozilla.org/MPL/2.0/. +%% +%% Copyright (c) 2018-2022 VMware, Inc. or its affiliates. All rights reserved. +%% + +-module(vhost_v1). + +-include("vhost.hrl"). + +-export([new/2, + new/3, + upgrade/1, + upgrade_to/2, + fields/0, + fields/1, + info_keys/0, + field_name/0, + record_version_to_use/0, + pattern_match_all/0, + get_name/1, + get_limits/1, + get_metadata/1, + get_description/1, + get_tags/1, + set_limits/2 +]). + +-define(record_version, ?MODULE). + +%% Represents a vhost. +%% +%% Historically this record had 2 arguments although the 2nd +%% was never used (`dummy`, always undefined). This is because +%% single field records were/are illegal in OTP. +%% +%% As of 3.6.x, the second argument is vhost limits, +%% which is actually used and has the same default. +%% Nonetheless, this required a migration, see rabbit_upgrade_functions. + +-record(vhost, { + %% name as a binary + virtual_host :: vhost:name() | '_', + %% proplist of limits configured, if any + limits :: list() | '_'}). + +-type vhost() :: vhost_v1(). +-type vhost_v1() :: #vhost{ + virtual_host :: vhost:name(), + limits :: list() + }. + +-export_type([vhost/0, + vhost_v1/0, + vhost_pattern/0, + vhost_v1_pattern/0]). + + +-spec new(vhost:name(), list()) -> vhost(). +new(Name, Limits) -> + #vhost{virtual_host = Name, limits = Limits}. + +-spec new(vhost:name(), list(), map()) -> vhost(). +new(Name, Limits, _Metadata) -> + #vhost{virtual_host = Name, limits = Limits}. + + +-spec record_version_to_use() -> vhost_v1. +record_version_to_use() -> + ?record_version. + +-spec upgrade(vhost()) -> vhost(). +upgrade(#vhost{} = VHost) -> VHost. + +-spec upgrade_to(vhost_v1, vhost()) -> vhost(). +upgrade_to(?record_version, #vhost{} = VHost) -> + VHost. + +fields() -> fields(?record_version). + +fields(?record_version) -> record_info(fields, vhost). + +field_name() -> #vhost.virtual_host. + +info_keys() -> [name, tracing, cluster_state]. + +-type vhost_pattern() :: vhost_v1_pattern(). +-type vhost_v1_pattern() :: #vhost{ + virtual_host :: vhost:name() | '_', + limits :: '_' + }. + +-spec pattern_match_all() -> vhost_pattern(). + +pattern_match_all() -> #vhost{_ = '_'}. + +get_name(#vhost{virtual_host = Value}) -> Value. +get_limits(#vhost{limits = Value}) -> Value. + +get_metadata(_VHost) -> undefined. +get_description(_VHost) -> undefined. +get_tags(_VHost) -> undefined. + +set_limits(VHost, Value) -> + VHost#vhost{limits = Value}. diff --git a/deps/rabbit/test/cluster_SUITE.erl b/deps/rabbit/test/cluster_SUITE.erl index 55303500245d..97373e767202 100644 --- a/deps/rabbit/test/cluster_SUITE.erl +++ b/deps/rabbit/test/cluster_SUITE.erl @@ -313,24 +313,34 @@ queue_name(Name) -> rabbit_misc:r(<<"/">>, queue, Name). credentials_obfuscation(Config) -> - Value = <<"amqp://something">>, - Obfuscated0 = obfuscate_secret(Config, 0, Value), - Obfuscated1 = obfuscate_secret(Config, 1, Value), - - ok = rabbit_ct_broker_helpers:restart_broker(Config, 1), - - ?assertEqual(Value, deobfuscate_secret(Config, 0, Obfuscated0)), - ?assertEqual(Value, deobfuscate_secret(Config, 1, Obfuscated1)), - ?assertEqual(Value, deobfuscate_secret(Config, 0, Obfuscated1)), - ?assertEqual(Value, deobfuscate_secret(Config, 1, Obfuscated1)), - - Obfuscated2 = obfuscate_secret(Config, 1, Value), - - ok = rabbit_ct_broker_helpers:restart_broker(Config, 0), - - ?assertEqual(Value, deobfuscate_secret(Config, 0, Obfuscated2)), - ok. - + case rabbit_ct_helpers:is_mixed_versions() of + false -> + case rabbit_ct_broker_helpers:enable_feature_flag(Config, virtual_host_metadata) of + ok -> + Value = <<"amqp://something">>, + Obfuscated0 = obfuscate_secret(Config, 0, Value), + Obfuscated1 = obfuscate_secret(Config, 1, Value), + + ok = rabbit_ct_broker_helpers:restart_broker(Config, 1), + + ?assertEqual(Value, deobfuscate_secret(Config, 0, Obfuscated0)), + ?assertEqual(Value, deobfuscate_secret(Config, 1, Obfuscated1)), + ?assertEqual(Value, deobfuscate_secret(Config, 0, Obfuscated1)), + ?assertEqual(Value, deobfuscate_secret(Config, 1, Obfuscated1)), + + Obfuscated2 = obfuscate_secret(Config, 1, Value), + + ok = rabbit_ct_broker_helpers:restart_broker(Config, 0), + + ?assertEqual(Value, deobfuscate_secret(Config, 0, Obfuscated2)), + ok; + Skip -> + Skip + end; + _ -> + %% skip the test in mixed version mode + {skip, "Should not run in mixed version environments"} + end. obfuscate_secret(Config, Node, Value) -> {encrypted, _} = Result = rabbit_ct_broker_helpers:rpc(Config, Node, credentials_obfuscation, encrypt, [Value]), diff --git a/deps/rabbit/test/definition_import_SUITE.erl b/deps/rabbit/test/definition_import_SUITE.erl index 5cf00719312c..35e5adff45a4 100644 --- a/deps/rabbit/test/definition_import_SUITE.erl +++ b/deps/rabbit/test/definition_import_SUITE.erl @@ -261,22 +261,33 @@ import_case14(Config) -> import_file_case(Config, "case14"). import_case15(Config) -> import_file_case(Config, "case15"). %% contains a virtual host with tags import_case16(Config) -> - import_file_case(Config, "case16"), - VHost = <<"tagged">>, - VHostIsImported = - fun () -> - case vhost_lookup(Config, VHost) of - {error, {no_such_vhosts, _}} -> false; - _ -> true - end - end, - rabbit_ct_helpers:await_condition(VHostIsImported, 20000), - VHostRec = vhost_lookup(Config, VHost), - ?assertEqual(<<"A case16 description">>, vhost:get_description(VHostRec)), - ?assertEqual(<<"quorum">>, vhost:get_default_queue_type(VHostRec)), - ?assertEqual([multi_dc_replication,ab,cde], vhost:get_tags(VHostRec)), + case rabbit_ct_helpers:is_mixed_versions() of + false -> + case rabbit_ct_broker_helpers:enable_feature_flag(Config, virtual_host_metadata) of + ok -> + import_file_case(Config, "case16"), + VHost = <<"tagged">>, + VHostIsImported = + fun () -> + case vhost_lookup(Config, VHost) of + {error, {no_such_vhosts, _}} -> false; + _ -> true + end + end, + rabbit_ct_helpers:await_condition(VHostIsImported, 20000), + VHostRec = vhost_lookup(Config, VHost), + ?assertEqual(<<"A case16 description">>, vhost:get_description(VHostRec)), + ?assertEqual(<<"quorum">>, vhost:get_default_queue_type(VHostRec)), + ?assertEqual([multi_dc_replication,ab,cde], vhost:get_tags(VHostRec)), - ok. + ok; + Skip -> + Skip + end; + _ -> + %% skip the test in mixed version mode + {skip, "Should not run in mixed version environments"} + end. import_case17(Config) -> import_invalid_file_case(Config, "failing_case17"). diff --git a/deps/rabbit/test/quorum_queue_SUITE.erl b/deps/rabbit/test/quorum_queue_SUITE.erl index d22aa735c768..3121ad1ff520 100644 --- a/deps/rabbit/test/quorum_queue_SUITE.erl +++ b/deps/rabbit/test/quorum_queue_SUITE.erl @@ -222,6 +222,10 @@ init_per_group(Group, Config) -> %% more time after clustering before running the %% tests. timer:sleep(ClusterSize * 1000), + ok = rabbit_ct_broker_helpers:enable_feature_flag( + Config2, maintenance_mode_status), + ok = rabbit_ct_broker_helpers:enable_feature_flag( + Config2, virtual_host_metadata), Config2; Skip -> end_per_group(Group, Config2), diff --git a/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl b/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl index 7bfc8aef2473..6e0c70602174 100644 --- a/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl +++ b/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl @@ -467,13 +467,21 @@ vhosts_test(Config) -> passed. vhosts_description_test(Config) -> + Ret = rabbit_ct_broker_helpers:enable_feature_flag( + Config, virtual_host_metadata), + http_put(Config, "/vhosts/myvhost", [{description, <<"vhost description">>}, {tags, <<"tag1,tag2">>}], {group, '2xx'}), - Expected = #{name => <<"myvhost">>, - metadata => #{ - description => <<"vhost description">>, - tags => [<<"tag1">>, <<"tag2">>] - }}, + Expected = case Ret of + {skip, _} -> + #{name => <<"myvhost">>}; + _ -> + #{name => <<"myvhost">>, + metadata => #{ + description => <<"vhost description">>, + tags => [<<"tag1">>, <<"tag2">>] + }} + end, assert_item(Expected, http_get(Config, "/vhosts/myvhost")), %% Delete it diff --git a/deps/rabbitmq_stream/test/rabbit_stream_SUITE.erl b/deps/rabbitmq_stream/test/rabbit_stream_SUITE.erl index b70130bf02f8..4a7ee9801e27 100644 --- a/deps/rabbitmq_stream/test/rabbit_stream_SUITE.erl +++ b/deps/rabbitmq_stream/test/rabbit_stream_SUITE.erl @@ -82,6 +82,7 @@ init_per_group(Group, Config) {rabbit, [{forced_feature_flags_on_init, [classic_mirrored_queue_version, + virtual_host_metadata, quorum_queue, stream_queue]}]}) end];