Add deprecation flag for master-locator

mkuratczyk · mkuratczyk · commit f0537bb22b1e · 2024-07-12T09:57:34.000+02:00
diff --git a/deps/rabbit/src/rabbit_classic_queue.erl b/deps/rabbit/src/rabbit_classic_queue.erl
@@ -79,9 +79,14 @@
 
 validate_policy(Args) ->
     Strategy = proplists:get_value(<<"queue-master-locator">>, Args, unknown),
-    case lists:member(Strategy, rabbit_queue_location:queue_leader_locators()) of
-        true -> ok;
-        false -> {error, "~tp is not a valid master locator", [Strategy]}
+    case rabbit_queue_location:master_locator_permitted() of
+        true ->
+            case lists:member(Strategy, rabbit_queue_location:queue_leader_locators()) of
+                true -> ok;
+                false -> {error, "~tp is not a valid master locator", [Strategy]}
+            end;
+        false ->
+            {error, "use of deprecated queue-master-locator argument is not permitted", []}
     end.
 
 -spec is_enabled() -> boolean().
@@ -91,7 +96,29 @@ is_enabled() -> true.
 is_compatible(_, _, _) ->
     true.
 
+validate_arguments(Args) ->
+    case lists:keyfind(<<"x-queue-master-locator">>, 1, Args) of
+        false ->
+            ok;
+        _ ->
+            case rabbit_queue_location:master_locator_permitted() of
+                true ->
+                    ok;
+                false ->
+                    error
+            end
+    end.
+
 declare(Q, Node) when ?amqqueue_is_classic(Q) ->
+    case validate_arguments(amqqueue:get_arguments(Q)) of
+        ok -> do_declare(Q, Node);
+        error ->
+            Warning = rabbit_deprecated_features:get_warning(
+                        queue_master_locator),
+            {protocol_error, internal_error, "~ts", [Warning]}
+    end.
+
+do_declare(Q, Node) when ?amqqueue_is_classic(Q) ->
     QName = amqqueue:get_name(Q),
     VHost = amqqueue:get_vhost(Q),
     Node1 = case {Node, rabbit_amqqueue:is_exclusive(Q)} of
@@ -536,7 +563,10 @@ capabilities() ->
                           <<"x-max-length-bytes">>, <<"x-max-priority">>,
                           <<"x-overflow">>, <<"x-queue-mode">>, <<"x-queue-version">>,
                           <<"x-single-active-consumer">>, <<"x-queue-type">>,
-                          <<"x-queue-master-locator">>, <<"x-queue-leader-locator">>],
+                          <<"x-queue-leader-locator">>] ++ case rabbit_queue_location:master_locator_permitted() of
+                                                               true -> [<<"x-queue-master-locator">>];
+                                                               false -> []
+                                                           end,
       consumer_arguments => [<<"x-priority">>, <<"x-credit">>],
       server_named => true}.
 
diff --git a/deps/rabbit/src/rabbit_queue_location.erl b/deps/rabbit/src/rabbit_queue_location.erl
@@ -10,7 +10,8 @@
 -include("amqqueue.hrl").
 
 -export([queue_leader_locators/0,
-         select_leader_and_followers/2]).
+         select_leader_and_followers/2,
+         master_locator_permitted/0]).
 
 %% these are needed because of they are called with ?MODULE:
 %% to allow mecking them in tests
@@ -21,6 +22,15 @@
 -export([select_members/7, leader_node/6, leader_locator/1]).
 -endif.
 
+-rabbit_deprecated_feature(
+   {queue_master_locator,
+    #{deprecation_phase => permitted_by_default,
+      messages =>
+      #{when_permitted =>
+        "queue-master-locator is deprecated. "
+        "queue-leader-locator should be used instead. Allowed values are 'client-local' and 'balanced'."}}
+   }).
+
 -define(QUEUE_LEADER_LOCATORS_DEPRECATED, [<<"random">>, <<"least-leaders">>, <<"min-masters">>]).
 -define(QUEUE_LEADER_LOCATORS, [<<"client-local">>, <<"balanced">>] ++ ?QUEUE_LEADER_LOCATORS_DEPRECATED).
 -define(QUEUE_COUNT_START_RANDOM_SELECTION, 1_000).
@@ -215,3 +225,6 @@ queues_per_node(Nodes, GetQueues) ->
 %% for unit testing
 -spec node() -> node().
 node() -> erlang:node().
+
+master_locator_permitted() ->
+    rabbit_deprecated_features:is_permitted(queue_master_locator).
diff --git a/deps/rabbit/test/rabbitmq_4_0_deprecations_SUITE.erl b/deps/rabbit/test/rabbitmq_4_0_deprecations_SUITE.erl
@@ -32,7 +32,10 @@
          set_policy_when_cmq_is_not_permitted_from_conf/1,
 
          when_transient_nonexcl_is_permitted_by_default/1,
-         when_transient_nonexcl_is_not_permitted_from_conf/1
+         when_transient_nonexcl_is_not_permitted_from_conf/1,
+
+         when_queue_master_locator_is_permitted_by_default/1,
+         when_queue_master_locator_is_not_permitted_from_conf/1
         ]).
 
 suite() ->
@@ -57,7 +60,10 @@ groups() ->
                 set_policy_when_cmq_is_not_permitted_from_conf]},
               {transient_nonexcl_queues, [],
                [when_transient_nonexcl_is_permitted_by_default,
-                when_transient_nonexcl_is_not_permitted_from_conf]}
+                when_transient_nonexcl_is_not_permitted_from_conf]},
+              {queue_master_locator, [],
+               [when_queue_master_locator_is_permitted_by_default,
+                when_queue_master_locator_is_not_permitted_from_conf]}
              ],
     [{mnesia_store, [], Groups},
      {khepri_store, [], Groups}].
@@ -89,6 +95,8 @@ init_per_group(classic_queue_mirroring, Config) ->
     rabbit_ct_helpers:set_config(Config, {rmq_nodes_count, 1});
 init_per_group(transient_nonexcl_queues, Config) ->
     rabbit_ct_helpers:set_config(Config, {rmq_nodes_count, 1});
+init_per_group(queue_master_locator, Config) ->
+    rabbit_ct_helpers:set_config(Config, {rmq_nodes_count, 1});
 init_per_group(_Group, Config) ->
     Config.
 
@@ -125,6 +133,14 @@ init_per_testcase(
                  [{permit_deprecated_features,
                    #{transient_nonexcl_queues => false}}]}),
     init_per_testcase1(Testcase, Config1);
+init_per_testcase(
+    when_queue_master_locator_is_not_permitted_from_conf = Testcase, Config) ->
+    Config1 = rabbit_ct_helpers:merge_app_env(
+                Config,
+                {rabbit,
+                 [{permit_deprecated_features,
+                   #{queue_master_locator => false}}]}),
+    init_per_testcase1(Testcase, Config1);
 init_per_testcase(Testcase, Config) ->
     init_per_testcase1(Testcase, Config).
 
@@ -454,6 +470,64 @@ when_transient_nonexcl_is_not_permitted_from_conf(Config) ->
          ["Deprecated features: `transient_nonexcl_queues`: Feature `transient_nonexcl_queues` is deprecated",
           "Its use is not permitted per the configuration"])).
 
+%% -------------------------------------------------------------------
+%% (x-)queue-master-locator
+%% -------------------------------------------------------------------
+
+when_queue_master_locator_is_permitted_by_default(Config) ->
+    [NodeA] = rabbit_ct_broker_helpers:get_node_configs(Config, nodename),
+
+    Ch = rabbit_ct_client_helpers:open_channel(Config, NodeA),
+
+    QName = list_to_binary(atom_to_list(?FUNCTION_NAME)),
+    ?assertEqual(
+       {'queue.declare_ok', QName, 0, 0},
+       amqp_channel:call(
+         Ch,
+         #'queue.declare'{queue = QName,
+                          arguments = [{<<"x-queue-master-locator">>, longstr, <<"client-local">>}]})),
+
+    ?assertEqual(
+       ok,
+       rabbit_ct_broker_helpers:set_policy(
+         Config, 0, <<"client-local">>, <<".*">>, <<"queues">>, [{<<"queue-master-locator">>, <<"client-local">>}])),
+
+    ?assert(
+       log_file_contains_message(
+         Config, NodeA,
+         ["Deprecated features: `queue_master_locator`: queue-master-locator is deprecated. ",
+          "queue-leader-locator should be used instead."])).
+
+when_queue_master_locator_is_not_permitted_from_conf(Config) ->
+    [NodeA] = rabbit_ct_broker_helpers:get_node_configs(Config, nodename),
+
+    Ch = rabbit_ct_client_helpers:open_channel(Config, NodeA),
+
+    QName = list_to_binary(atom_to_list(?FUNCTION_NAME)),
+    ?assertExit(
+       {{shutdown,
+         {connection_closing,
+          {server_initiated_close, 541,
+           <<"INTERNAL_ERROR - Feature `queue_master_locator` is "
+             "deprecated.", _/binary>>}}}, _},
+       amqp_channel:call(
+         Ch,
+         #'queue.declare'{queue = QName,
+                          arguments = [{<<"x-queue-master-locator">>, longstr, <<"client-local">>}]})),
+
+    ?assertError(
+       {badmatch,
+        {error_string,
+         "Validation failed\n\nuse of deprecated queue-master-locator argument is not permitted\n"}},
+       rabbit_ct_broker_helpers:set_policy(
+         Config, 0, <<"client-local">>, <<".*">>, <<"queues">>, [{<<"queue-master-locator">>, <<"client-local">>}])),
+
+    ?assert(
+       log_file_contains_message(
+         Config, NodeA,
+         ["Deprecated features: `queue_master_locator`: Feature `queue_master_locator` is deprecated",
+          "Its use is not permitted per the configuration"])).
+
 %% -------------------------------------------------------------------
 %% Helpers.
 %% -------------------------------------------------------------------
