Tighten up HA policy validation.bug25555

2 files changed, 48 insertions, 13 deletions

diff --git a/src/rabbit_mirror_queue_misc.erl b/src/rabbit_mirror_queue_misc.erl
index 8787e966..5607bfa9 100644
--- a/src/rabbit_mirror_queue_misc.erl
+++ b/src/rabbit_mirror_queue_misc.erl
@@ -316,24 +316,30 @@ update_mirrors0(OldQ = #amqqueue{name = QName},
 %%----------------------------------------------------------------------------
 
 validate_policy(KeyList) ->
-    Mode = proplists:get_value(<<"ha-mode">>, KeyList),
+    Mode = proplists:get_value(<<"ha-mode">>, KeyList, none),
     Params = proplists:get_value(<<"ha-params">>, KeyList, none),
-    case Mode of
-        undefined -> ok;
-        _         -> case module(Mode) of
-                         {ok, M} -> case M:validate_policy(Params) of
-                                        ok -> validate_sync_mode(KeyList);
-                                        E  -> E
-                                    end;
-                         _       -> {error,
-                                     "~p is not a valid ha-mode value", [Mode]}
-                     end
+    SyncMode = proplists:get_value(<<"ha-sync-mode">>, KeyList, none),
+    case {Mode, Params, SyncMode} of
+        {none, none, none} ->
+            ok;
+        {none, _, _} ->
+            {error, "ha-mode must be specified to specify ha-params or "
+             "ha-sync-mode", []};
+        _ ->
+            case module(Mode) of
+                {ok, M} -> case M:validate_policy(Params) of
+                               ok -> validate_sync_mode(SyncMode);
+                               E  -> E
+                           end;
+                _       -> {error, "~p is not a valid ha-mode value", [Mode]}
+            end
     end.
 
-validate_sync_mode(KeyList) ->
-    case proplists:get_value(<<"ha-sync-mode">>, KeyList, <<"manual">>) of
+validate_sync_mode(SyncMode) ->
+    case SyncMode of
         <<"automatic">> -> ok;
         <<"manual">>    -> ok;
+        none            -> ok;
         Mode            -> {error, "ha-sync-mode must be \"manual\" "
                             "or \"automatic\", got ~p", [Mode]}
     end.
diff --git a/src/rabbit_tests.erl b/src/rabbit_tests.erl
index 2e46304f..f32fe740 100644
--- a/src/rabbit_tests.erl
+++ b/src/rabbit_tests.erl
@@ -60,6 +60,7 @@ all_tests() ->
     passed = test_user_management(),
     passed = test_runtime_parameters(),
     passed = test_policy_validation(),
+    passed = test_ha_policy_validation(),
     passed = test_server_status(),
     passed = test_amqp_connection_refusal(),
     passed = test_confirms(),
@@ -1101,6 +1102,34 @@ test_policy_validation() ->
     rabbit_runtime_parameters_test:unregister_policy_validator(),
     passed.
 
+test_ha_policy_validation() ->
+    Set  = fun (JSON) -> control_action(set_policy, ["name", ".*", JSON]) end,
+    OK   = fun (JSON) -> ok = Set(JSON) end,
+    Fail = fun (JSON) -> {error_string, _} = Set(JSON) end,
+
+    OK  ("{\"ha-mode\":\"all\"}"),
+    Fail("{\"ha-mode\":\"made_up\"}"),
+
+    Fail("{\"ha-mode\":\"nodes\"}"),
+    Fail("{\"ha-mode\":\"nodes\",\"ha-params\":2}"),
+    Fail("{\"ha-mode\":\"nodes\",\"ha-params\":[\"a\",2]}"),
+    OK  ("{\"ha-mode\":\"nodes\",\"ha-params\":[\"a\",\"b\"]}"),
+    Fail("{\"ha-params\":[\"a\",\"b\"]}"),
+
+    Fail("{\"ha-mode\":\"exactly\"}"),
+    Fail("{\"ha-mode\":\"exactly\",\"ha-params\":[\"a\",\"b\"]}"),
+    OK  ("{\"ha-mode\":\"exactly\",\"ha-params\":2}"),
+    Fail("{\"ha-params\":2}"),
+
+    OK  ("{\"ha-mode\":\"all\",\"ha-sync-mode\":\"manual\"}"),
+    OK  ("{\"ha-mode\":\"all\",\"ha-sync-mode\":\"automatic\"}"),
+    Fail("{\"ha-mode\":\"all\",\"ha-sync-mode\":\"made_up\"}"),
+    Fail("{\"ha-sync-mode\":\"manual\"}"),
+    Fail("{\"ha-sync-mode\":\"automatic\"}"),
+
+    ok = control_action(clear_policy, ["name"]),
+    passed.
+
 test_server_status() ->
     %% create a few things so there is some useful information to list
     {_Writer, Limiter, Ch} = test_channel(),