From f8f0046fff711df947f5bfb16c8d8220641a1944 Mon Sep 17 00:00:00 2001 From: Michal Kuratczyk Date: Wed, 30 Apr 2025 09:38:15 +0200 Subject: [PATCH 1/7] Fix DQT in definition export (redundant property) The correct place for the `default_queue_type` property is inside the `metadata` block. However, right now we'd always export the value outside of `metadata` AND only export it inside `metadata`, if it was not `undefined`. This value outside of `metadata` was just misleading: if a user exported the definitins from a fresh node, changed `classic` to `quorum` and imported such modified values, the DQT would still be `classic`, because RMQ looks for the value inside `metadata`. Just to make it more confusing, if the DQT was changed successfully one way or another, the value outside of `metadata` would reflect that (it always shows the correct value, but is ignored on import). (cherry picked from commit 73da2a3fbbd7a1cc6b3930dabed3c2df644a9383) --- deps/rabbit/src/rabbit_definitions.erl | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/deps/rabbit/src/rabbit_definitions.erl b/deps/rabbit/src/rabbit_definitions.erl index 0f69b3ddf424..257f76232e10 100644 --- a/deps/rabbit/src/rabbit_definitions.erl +++ b/deps/rabbit/src/rabbit_definitions.erl @@ -1081,12 +1081,10 @@ list_vhosts() -> vhost_definition(VHost) -> Name = vhost:get_name(VHost), - DQT = rabbit_queue_type:short_alias_of(rabbit_vhost:default_queue_type(Name)), #{ <<"name">> => Name, <<"limits">> => vhost:get_limits(VHost), - <<"metadata">> => vhost:get_metadata(VHost), - <<"default_queue_type">> => DQT + <<"metadata">> => vhost:get_metadata(VHost) }. list_users() -> From 09b31f6e4c05f77bee3466168f137527dcdf89f9 Mon Sep 17 00:00:00 2001 From: Michal Kuratczyk Date: Wed, 30 Apr 2025 11:03:14 +0200 Subject: [PATCH 2/7] Remove vhost.default_queue_type from HTTP defs export (cherry picked from commit 5eb65f5f72875c655c9d97a052e85c93ef4e92f5) --- deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl b/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl index 6acdf9f7097c..4c6bf620b4c9 100644 --- a/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl +++ b/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl @@ -288,7 +288,7 @@ export_name(_Name) -> true. rw_state() -> [{users, [name, password_hash, hashing_algorithm, tags, limits]}, - {vhosts, [name, description, tags, default_queue_type, metadata]}, + {vhosts, [name, description, tags, metadata]}, {permissions, [user, vhost, configure, write, read]}, {topic_permissions, [user, vhost, exchange, write, read]}, {parameters, [vhost, component, name, value]}, From 4932d2e1d9b4e11eaf53d81555dfe8ab7ab5cf92 Mon Sep 17 00:00:00 2001 From: Michal Kuratczyk Date: Wed, 30 Apr 2025 14:30:00 +0200 Subject: [PATCH 3/7] vhost inherits DQT from node Rather than injecting node-level DQT when exporting definitions, inject it into vhost's metadata when a vhost is created. (cherry picked from commit 3c95bf32e7a7107e48033ccc1cb0ae90775787c3) --- deps/rabbit/src/rabbit_vhost.erl | 1 + deps/rabbit/src/vhost.erl | 3 ++- deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl | 6 +----- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/deps/rabbit/src/rabbit_vhost.erl b/deps/rabbit/src/rabbit_vhost.erl index bb616a684c77..7bea09935477 100644 --- a/deps/rabbit/src/rabbit_vhost.erl +++ b/deps/rabbit/src/rabbit_vhost.erl @@ -146,6 +146,7 @@ add(VHost, ActingUser) -> rabbit_types:ok_or_error(any()). add(Name, Description, Tags, ActingUser) -> add(Name, #{description => Description, + default_queue_type => rabbit_queue_type:default_alias(), tags => Tags}, ActingUser). -spec add(vhost:name(), vhost:metadata(), rabbit_types:username()) -> diff --git a/deps/rabbit/src/vhost.erl b/deps/rabbit/src/vhost.erl index a16116a3a99e..796f1224204d 100644 --- a/deps/rabbit/src/vhost.erl +++ b/deps/rabbit/src/vhost.erl @@ -215,7 +215,8 @@ disable_protection_from_deletion(VHost) -> -spec new_metadata(binary(), [atom()], rabbit_queue_type:queue_type() | 'undefined') -> metadata(). new_metadata(Description, Tags, undefined) -> #{description => Description, - tags => Tags}; + default_queue_type => rabbit_queue_type:default_alias(), + tags => Tags}; new_metadata(Description, Tags, DefaultQueueType) -> #{description => Description, tags => Tags, diff --git a/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl b/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl index 4c6bf620b4c9..343c46951d10 100644 --- a/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl +++ b/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl @@ -132,10 +132,7 @@ vhost_definitions(ReqData, VHostName, Context) -> ProductName = rabbit:product_name(), ProductVersion = rabbit:product_version(), - DQT = rabbit_queue_type:short_alias_of(rabbit_vhost:default_queue_type(VHostName)), - %% note: the type changes to a map - VHost1 = rabbit_queue_type:inject_dqt(VHost), - Metadata = maps:get(metadata, VHost1), + Metadata = vhost:get_metadata(VHost), TopLevelDefsAndMetadata = [ {rabbit_version, rabbit_data_coercion:to_binary(Vsn)}, @@ -147,7 +144,6 @@ vhost_definitions(ReqData, VHostName, Context) -> {explanation, rabbit_data_coercion:to_binary(io_lib:format("Definitions of virtual host '~ts'", [VHostName]))}, {metadata, Metadata}, {description, vhost:get_description(VHost)}, - {default_queue_type, DQT}, {limits, vhost:get_limits(VHost)} ], Result = TopLevelDefsAndMetadata ++ retain_whitelisted(Contents), From bededaa194c417d15744acfc0072aa681474d743 Mon Sep 17 00:00:00 2001 From: Michal Kuratczyk Date: Thu, 1 May 2025 09:16:40 +0200 Subject: [PATCH 4/7] Adjust tests to the new behaviour (cherry picked from commit 0e743b5fe73ff470b1d9dd5b9b94f45a4c3c58e1) --- deps/rabbit/test/vhost_SUITE.erl | 7 ++++--- deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl | 1 - 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/deps/rabbit/test/vhost_SUITE.erl b/deps/rabbit/test/vhost_SUITE.erl index 9a70a11de687..5b807f73b07b 100644 --- a/deps/rabbit/test/vhost_SUITE.erl +++ b/deps/rabbit/test/vhost_SUITE.erl @@ -307,13 +307,14 @@ vhost_update_default_queue_type_undefined(Config) -> VHost = <<"update-default_queue_type-with-undefined-test">>, Description = <<"rmqfpas-105 test vhost">>, Tags = [replicate, private], - DefaultQueueType = quorum, + VhostDefaultQueueType = quorum, + NodeDefaultQueueType = rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_queue_type, default_alias, []), Trace = false, ActingUser = <<"acting-user">>, try ?assertMatch(ok, rabbit_ct_broker_helpers:add_vhost(Config, VHost)), - PutVhostArgs0 = [VHost, Description, Tags, DefaultQueueType, Trace, ActingUser], + PutVhostArgs0 = [VHost, Description, Tags, VhostDefaultQueueType, Trace, ActingUser], ?assertMatch(ok, rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_vhost, put_vhost, PutVhostArgs0)), @@ -322,7 +323,7 @@ vhost_update_default_queue_type_undefined(Config) -> rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_vhost, put_vhost, PutVhostArgs1)), V = rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_vhost, lookup, [VHost]), - ?assertMatch(#{default_queue_type := DefaultQueueType}, vhost:get_metadata(V)) + ?assertMatch(#{default_queue_type := NodeDefaultQueueType}, vhost:get_metadata(V)) after rabbit_ct_broker_helpers:delete_vhost(Config, VHost) end. diff --git a/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl b/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl index 7cae1e5c484e..a44dd8962dd6 100644 --- a/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl +++ b/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl @@ -2126,7 +2126,6 @@ definitions_vhost_metadata_test(Config) -> ?assertEqual(#{ name => VHostName, description => Desc, - default_queue_type => DQT, tags => Tags, metadata => Metadata }, VH), From d072f43dcb839356a450cd9b58e0e83fd36d2f1f Mon Sep 17 00:00:00 2001 From: Michal Kuratczyk Date: Thu, 1 May 2025 15:53:17 +0200 Subject: [PATCH 5/7] Add DQT to vhost metadata on recovery Vhosts that currently don't have their own default queue type, now inherit it from the node configuration and store it in their metadata going forward. (cherry picked from commit 9d0f01b45bdd268635b76bcf3c88793918970fba) --- deps/rabbit/src/rabbit_vhost.erl | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/deps/rabbit/src/rabbit_vhost.erl b/deps/rabbit/src/rabbit_vhost.erl index 7bea09935477..b27a321daf6f 100644 --- a/deps/rabbit/src/rabbit_vhost.erl +++ b/deps/rabbit/src/rabbit_vhost.erl @@ -57,6 +57,38 @@ recover(VHost) -> ok = rabbit_file:ensure_dir(VHostStubFile), ok = file:write_file(VHostStubFile, VHost), ok = ensure_config_file(VHost), + + %% in the past, a vhost didn't necessarily have a default queue type + %% and queues declared in that vhost defaulted to the type configured + %% on the node level (in the config file). Now each vhost has its default + %% queue type in the metadata. For vhosts updated from older versions, + %% we need to add the default type to the metadata + case rabbit_db_vhost:get(VHost) of + undefined -> + rabbit_log:warning("Cannot check metadata for vhost '~ts' during recovery, record not found.", + [VHost]); + VHostRecord -> + Metadata = vhost:get_metadata(VHostRecord), + case maps:is_key(default_queue_type, Metadata) of + true -> + rabbit_log:debug("Default queue type for vhost '~ts' is ~p.", + [VHost, maps:get(default_queue_type, Metadata)]), + ok; + false -> + DefaultType = rabbit_queue_type:default_alias(), + rabbit_log:info("Setting missing default queue type to '~p' for vhost '~ts'.", + [DefaultType, VHost]), + case rabbit_db_vhost:merge_metadata(VHost, #{default_queue_type => DefaultType}) of + {ok, _UpdatedVHostRecord} -> + ok; + {error, Reason} -> + % Log the error but continue recovery + rabbit_log:warning("Failed to set the default queue type for vhost '~ts': ~p", + [VHost, Reason]) + end + end + end, + {Recovered, Failed} = rabbit_amqqueue:recover(VHost), AllQs = Recovered ++ Failed, QNames = [amqqueue:get_name(Q) || Q <- AllQs], From 01433f4017efd02548cb186cd13d947141b63bc1 Mon Sep 17 00:00:00 2001 From: Michal Kuratczyk Date: Fri, 2 May 2025 13:55:54 +0200 Subject: [PATCH 6/7] Set the DQT in rabbit_vhost:do_add (cherry picked from commit 9bd11b449fb603ebb18ad96e68f1db62ecbd3225) --- deps/rabbit/src/rabbit_vhost.erl | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/deps/rabbit/src/rabbit_vhost.erl b/deps/rabbit/src/rabbit_vhost.erl index b27a321daf6f..9a88d38ee43e 100644 --- a/deps/rabbit/src/rabbit_vhost.erl +++ b/deps/rabbit/src/rabbit_vhost.erl @@ -178,7 +178,6 @@ add(VHost, ActingUser) -> rabbit_types:ok_or_error(any()). add(Name, Description, Tags, ActingUser) -> add(Name, #{description => Description, - default_queue_type => rabbit_queue_type:default_alias(), tags => Tags}, ActingUser). -spec add(vhost:name(), vhost:metadata(), rabbit_types:username()) -> @@ -190,8 +189,16 @@ add(Name, Metadata, ActingUser) -> catch(do_add(Name, Metadata, ActingUser)) end. -do_add(Name, Metadata, ActingUser) -> +do_add(Name, Metadata0, ActingUser) -> ok = is_over_vhost_limit(Name), + + Metadata = case maps:is_key(default_queue_type, Metadata0) of + true -> + Metadata0; + false -> + Metadata0#{default_queue_type => rabbit_queue_type:default_alias()} + end, + Description = maps:get(description, Metadata, undefined), Tags = maps:get(tags, Metadata, []), From 6549254b45784347be41ed0a4b9d1a0a05c6aa0d Mon Sep 17 00:00:00 2001 From: Michal Kuratczyk Date: Fri, 2 May 2025 14:01:10 +0200 Subject: [PATCH 7/7] Add a test for DQT upon vhost creation (cherry picked from commit f61b9d9bf410bd6faa8e9a2ac4bf537a518a6ad0) --- deps/rabbit/test/vhost_SUITE.erl | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/deps/rabbit/test/vhost_SUITE.erl b/deps/rabbit/test/vhost_SUITE.erl index 5b807f73b07b..35a32a27d3c5 100644 --- a/deps/rabbit/test/vhost_SUITE.erl +++ b/deps/rabbit/test/vhost_SUITE.erl @@ -27,6 +27,7 @@ all() -> groups() -> ClusterSize1Tests = [ vhost_is_created_with_default_limits, + vhost_is_created_with_default_queue_type, vhost_is_created_with_operator_policies, vhost_is_created_with_default_user, single_node_vhost_deletion_forces_connection_closure, @@ -461,10 +462,37 @@ vhost_is_created_with_default_limits(Config) -> ?assertEqual(ok, rabbit_ct_broker_helpers:add_vhost(Config, VHost)), ?assertEqual(Limits, rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_vhost_limit, list, [VHost])) + after + rabbit_ct_broker_helpers:rpc( + Config, 0, + application, unset_env, [rabbit, default_limits]) + end. + +vhost_is_created_with_default_queue_type(Config) -> + VHost = atom_to_binary(?FUNCTION_NAME), + QName = atom_to_binary(?FUNCTION_NAME), + ?assertEqual(ok, rabbit_ct_broker_helpers:rpc(Config, 0, + application, set_env, [rabbit, default_queue_type, rabbit_quorum_queue])), + try + ?assertEqual(ok, rabbit_ct_broker_helpers:add_vhost(Config, VHost)), + rabbit_ct_broker_helpers:set_full_permissions(Config, <<"guest">>, VHost), + ?assertEqual(<<"quorum">>, rabbit_ct_broker_helpers:rpc(Config, 0, + rabbit_vhost, default_queue_type, [VHost])), + V = rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_db_vhost, get, [VHost]), + ct:pal("Vhost metadata: ~p", [V]), + ?assertEqual(<<"quorum">>, maps:get(default_queue_type, vhost:get_metadata(V))), + + Conn = rabbit_ct_client_helpers:open_unmanaged_connection(Config, 0, VHost), + {ok, Chan} = amqp_connection:open_channel(Conn), + amqp_channel:call(Chan, #'queue.declare'{queue = QName, durable = true}), + QNameRes = rabbit_misc:r(VHost, queue, QName), + {ok, Q} = rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_amqqueue, lookup, [QNameRes]), + ?assertMatch(rabbit_quorum_queue, amqqueue:get_type(Q)), + close_connections([Conn]) after rabbit_ct_broker_helpers:rpc( Config, 0, - application, unset_env, [rabbit, default_limits]) + application, unset_env, [rabbit, default_queue_type]) end. vhost_is_created_with_operator_policies(Config) ->