From 16673df6b639e929f976d184e608b107df1380c8 Mon Sep 17 00:00:00 2001 From: Michael Klishin Date: Sat, 22 Jun 2024 02:16:22 -0400 Subject: [PATCH 1/2] Follow-up to #11457 The queue type argument won't always be a binary, for example, when a virtual host is created. As such, the validation code should accept at least atoms in addition to binaries. While at it, improve logging and error reporting when DQT validation fails, and while at it, make the definition import tests focussed on virtual host a bit more robust. (cherry picked from commit 1e577a82fc91a832366aa647ba9492467bc2559f) # Conflicts: # deps/rabbit/src/rabbit_vhost.erl --- deps/rabbit/src/rabbit_amqqueue.erl | 2 ++ deps/rabbit/src/rabbit_queue_type.erl | 2 ++ deps/rabbit/src/rabbit_vhost.erl | 10 +++++++--- deps/rabbit/test/definition_import_SUITE.erl | 12 +++++++++--- 4 files changed, 20 insertions(+), 6 deletions(-) diff --git a/deps/rabbit/src/rabbit_amqqueue.erl b/deps/rabbit/src/rabbit_amqqueue.erl index 4f39796cb7c..bb825684209 100644 --- a/deps/rabbit/src/rabbit_amqqueue.erl +++ b/deps/rabbit/src/rabbit_amqqueue.erl @@ -1121,6 +1121,8 @@ check_queue_type(Val, _Args) when is_binary(Val) -> true -> ok; false -> {error, rabbit_misc:format("unsupported queue type '~ts'", [Val])} end; +check_queue_type(Val, Args) when is_atom(Val) -> + check_queue_type(rabbit_data_coercion:to_binary(Val), Args); check_queue_type(_Val, _Args) -> {error, invalid_queue_type}. diff --git a/deps/rabbit/src/rabbit_queue_type.erl b/deps/rabbit/src/rabbit_queue_type.erl index d2e64738b2d..241815fa04a 100644 --- a/deps/rabbit/src/rabbit_queue_type.erl +++ b/deps/rabbit/src/rabbit_queue_type.erl @@ -246,6 +246,8 @@ discover(<<"classic">>) -> rabbit_classic_queue; discover(<<"stream">>) -> rabbit_stream_queue; +discover(Other) when is_atom(Other) -> + discover(rabbit_data_coercion:to_binary(Other)); discover(Other) when is_binary(Other) -> T = rabbit_registry:binary_to_type(Other), {ok, Mod} = rabbit_registry:lookup_module(queue, T), diff --git a/deps/rabbit/src/rabbit_vhost.erl b/deps/rabbit/src/rabbit_vhost.erl index bb2dd362086..fa84bbbe06b 100644 --- a/deps/rabbit/src/rabbit_vhost.erl +++ b/deps/rabbit/src/rabbit_vhost.erl @@ -171,6 +171,7 @@ do_add(Name, Metadata, ActingUser) -> case Metadata of #{default_queue_type := DQT} -> %% check that the queue type is known + rabbit_log:debug("Default queue type of virtual host '~ts' is ~tp", [Name, DQT]), try rabbit_queue_type:discover(DQT) of _ -> case rabbit_queue_type:feature_flag_name(DQT) of @@ -182,7 +183,7 @@ do_add(Name, Metadata, ActingUser) -> end end catch _:_ -> - throw({error, invalid_queue_type}) + throw({error, invalid_queue_type, DQT}) end; _ -> ok @@ -315,7 +316,7 @@ put_vhost(Name, Description, Tags0, Trace, Username) -> boolean(), rabbit_types:username()) -> 'ok' | {'error', any()} | {'EXIT', any()}. -put_vhost(Name, Description, Tags0, DefaultQueueType0, Trace, Username) -> +put_vhost(Name, Description, Tags0, DefaultQueueType, Trace, Username) -> Tags = case Tags0 of undefined -> <<"">>; null -> <<"">>; @@ -323,12 +324,15 @@ put_vhost(Name, Description, Tags0, DefaultQueueType0, Trace, Username) -> "null" -> <<"">>; Other -> Other end, +<<<<<<< HEAD DefaultQueueType = case DefaultQueueType0 of <<"undefined">> -> undefined; _ -> DefaultQueueType0 end, +======= +>>>>>>> 1e577a82fc (Follow-up to #11457) ParsedTags = parse_tags(Tags), - rabbit_log:debug("Parsed tags ~tp to ~tp", [Tags, ParsedTags]), + rabbit_log:debug("Parsed virtual host tags ~tp to ~tp", [Tags, ParsedTags]), Result = case exists(Name) of true -> update(Name, Description, ParsedTags, DefaultQueueType, Username); diff --git a/deps/rabbit/test/definition_import_SUITE.erl b/deps/rabbit/test/definition_import_SUITE.erl index e0c97654475..37d081d88d6 100644 --- a/deps/rabbit/test/definition_import_SUITE.erl +++ b/deps/rabbit/test/definition_import_SUITE.erl @@ -211,8 +211,9 @@ import_case11(Config) -> import_file_case(Config, "case11"). import_case12(Config) -> import_invalid_file_case(Config, "failing_case12"). import_case13(Config) -> - import_file_case(Config, "case13"), VHost = <<"/">>, + delete_vhost(Config, VHost), + import_file_case(Config, "case13"), QueueName = <<"definitions.import.case13.qq.1">>, QueueIsImported = fun () -> @@ -231,8 +232,9 @@ import_case13(Config) -> amqqueue:get_arguments(Q)). import_case13a(Config) -> - import_file_case(Config, "case13"), VHost = <<"/">>, + delete_vhost(Config, VHost), + import_file_case(Config, "case13"), QueueName = <<"definitions.import.case13.qq.1">>, QueueIsImported = fun () -> @@ -254,8 +256,9 @@ 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">>, + delete_vhost(Config, VHost), + import_file_case(Config, "case16"), VHostIsImported = fun () -> case vhost_lookup(Config, VHost) of @@ -516,3 +519,6 @@ vhost_lookup(Config, VHost) -> user_lookup(Config, User) -> rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_auth_backend_internal, lookup_user, [User]). + +delete_vhost(Config, VHost) -> + rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_vhost, delete, [VHost, <<"CT tests">>]). \ No newline at end of file From eeca81efdc90a4ea2b22bcd8b46a1ccb5feb284b Mon Sep 17 00:00:00 2001 From: Michael Klishin Date: Sat, 22 Jun 2024 05:22:31 -0400 Subject: [PATCH 2/2] Resolve a conflict #11528 #11532 --- deps/rabbit/src/rabbit_vhost.erl | 7 ------- 1 file changed, 7 deletions(-) diff --git a/deps/rabbit/src/rabbit_vhost.erl b/deps/rabbit/src/rabbit_vhost.erl index fa84bbbe06b..3f36a9317ff 100644 --- a/deps/rabbit/src/rabbit_vhost.erl +++ b/deps/rabbit/src/rabbit_vhost.erl @@ -324,13 +324,6 @@ put_vhost(Name, Description, Tags0, DefaultQueueType, Trace, Username) -> "null" -> <<"">>; Other -> Other end, -<<<<<<< HEAD - DefaultQueueType = case DefaultQueueType0 of - <<"undefined">> -> undefined; - _ -> DefaultQueueType0 - end, -======= ->>>>>>> 1e577a82fc (Follow-up to #11457) ParsedTags = parse_tags(Tags), rabbit_log:debug("Parsed virtual host tags ~tp to ~tp", [Tags, ParsedTags]), Result = case exists(Name) of