Use negative values to disable per-vhost limits, 0 to refuse all connections

Per recommendation from @dumbbell.

4 files changed, 115 insertions, 29 deletions

diff --git a/src/rabbit_connection_tracking.erl b/src/rabbit_connection_tracking.erl
index 5e8c4415d3..2d23e7c888 100644
--- a/src/rabbit_connection_tracking.erl
+++ b/src/rabbit_connection_tracking.erl
@@ -195,16 +195,23 @@ list_on_node(Node) ->
 -spec is_over_connection_limit(rabbit_types:vhost()) -> {true, non_neg_integer()} | false.
 
 is_over_connection_limit(VirtualHost) ->
-    ConnectionCount = count_connections_in(VirtualHost),
     case rabbit_vhost_limit:connection_limit(VirtualHost) of
-        undefined   -> false;
-        {ok, Limit} -> case {ConnectionCount, ConnectionCount >= Limit} of
-                           %% 0 = no limit
-                           {0, _}     -> false;
-                           %% the limit hasn't been reached
-                           {_, false} -> false;
-                           {_N, true} -> {true, Limit}
-                       end
+        %% no limit configured
+        undefined                                            -> false;
+        %% with limit = 0, no connections are allowed
+        {ok, 0}                                              -> {true, 0};
+        {ok, Limit} when is_integer(Limit) andalso Limit > 0 ->
+          ConnectionCount = count_connections_in(VirtualHost),
+          case ConnectionCount >= Limit of
+            false -> false;
+            true  -> {true, Limit}
+          end;
+        %% any negative value means "no limit". Note that parameter validation
+        %% will replace negative integers with 'undefined', so this is to be
+        %% explicit and extra defensive
+        {ok, Limit} when is_integer(Limit) andalso Limit < 0 -> false;
+        %% ignore non-integer limits
+        {ok, _Limit}                                         -> false
     end.
 
 
diff --git a/src/rabbit_parameter_validation.erl b/src/rabbit_parameter_validation.erl
index 90ab1d5286..00e83d2757 100644
--- a/src/rabbit_parameter_validation.erl
+++ b/src/rabbit_parameter_validation.erl
@@ -16,7 +16,7 @@
 
 -module(rabbit_parameter_validation).
 
--export([number/2, binary/2, boolean/2, list/2, regex/2, proplist/3, enum/1]).
+-export([number/2, integer/2, binary/2, boolean/2, list/2, regex/2, proplist/3, enum/1]).
 
 number(_Name, Term) when is_number(Term) ->
     ok;
@@ -24,6 +24,12 @@ number(_Name, Term) when is_number(Term) ->
 number(Name, Term) ->
     {error, "~s should be number, actually was ~p", [Name, Term]}.
 
+integer(_Name, Term) when is_integer(Term) ->
+    ok;
+
+integer(Name, Term) ->
+    {error, "~s should be number, actually was ~p", [Name, Term]}.
+
 binary(_Name, Term) when is_binary(Term) ->
     ok;
 
diff --git a/src/rabbit_vhost_limit.erl b/src/rabbit_vhost_limit.erl
index dd7ce51089..2d9a2f075e 100644
--- a/src/rabbit_vhost_limit.erl
+++ b/src/rabbit_vhost_limit.erl
@@ -72,7 +72,7 @@ clear(VHost) ->
                                         <<"limits">>).
 
 vhost_limit_validation() ->
-    [{<<"max-connections">>, fun rabbit_parameter_validation:number/2, mandatory}].
+    [{<<"max-connections">>, fun rabbit_parameter_validation:integer/2, mandatory}].
 
 update_vhost(VHostName, Limits) ->
     rabbit_misc:execute_mnesia_transaction(
@@ -91,8 +91,9 @@ get_limit(VirtualHost, Limit) ->
                        undefined -> undefined;
                        Val       -> case pget(Limit, Val) of
                                         undefined     -> undefined;
-                                        N when N =< 0 -> undefined;
-                                        N when N > 0  -> {ok, N}
+                                        %% no limit
+                                        N when N < 0  -> undefined;
+                                        N when N >= 0 -> {ok, N}
                                     end
                    end
     end.
diff --git a/test/per_vhost_connection_limit_SUITE.erl b/test/per_vhost_connection_limit_SUITE.erl
index 033bd0abdd..f327e82cb7 100644
--- a/test/per_vhost_connection_limit_SUITE.erl
+++ b/test/per_vhost_connection_limit_SUITE.erl
@@ -40,7 +40,9 @@ groups() ->
           single_node_multiple_vhost_connection_count,
           single_node_list_in_vhost,
           single_node_single_vhost_limit,
+          single_node_single_vhost_zero_limit,
           single_node_multiple_vhost_limit,
+          single_node_multiple_vhost_zero_limit,
           single_node_vhost_deletion_forces_connection_closure
         ]},
       {cluster_size_2, [], [
@@ -398,6 +400,10 @@ cluster_node_list_on_node(Config) ->
     rabbit_ct_broker_helpers:start_broker(Config, 1).
 
 single_node_single_vhost_limit(Config) ->
+    single_node_single_vhost_limit_with(Config, 5),
+    single_node_single_vhost_limit_with(Config, -1).
+
+single_node_single_vhost_limit_with(Config, WatermarkLimit) ->
     VHost = <<"/">>,
     set_vhost_connection_limit(Config, VHost, 3),
 
@@ -408,11 +414,11 @@ single_node_single_vhost_limit(Config) ->
     Conn3 = open_unmanaged_connection(Config, 0),
 
     %% we've crossed the limit
-    {error, not_allowed} = open_unmanaged_connection(Config, 0),
-    {error, not_allowed} = open_unmanaged_connection(Config, 0),
-    {error, not_allowed} = open_unmanaged_connection(Config, 0),
+    expect_that_client_connection_is_rejected(Config, 0),
+    expect_that_client_connection_is_rejected(Config, 0),
+    expect_that_client_connection_is_rejected(Config, 0),
 
-    set_vhost_connection_limit(Config, VHost, 5),
+    set_vhost_connection_limit(Config, VHost, WatermarkLimit),
     Conn4 = open_unmanaged_connection(Config, 0),
     Conn5 = open_unmanaged_connection(Config, 0),
 
@@ -421,7 +427,29 @@ single_node_single_vhost_limit(Config) ->
                   end, [Conn1, Conn2, Conn3, Conn4, Conn5]),
 
     ?assertEqual(0, count_connections_in(Config, VHost)),
-    set_vhost_connection_limit(Config, VHost, 0).
+    set_vhost_connection_limit(Config, VHost,  -1).
+
+single_node_single_vhost_zero_limit(Config) ->
+    VHost = <<"/">>,
+    set_vhost_connection_limit(Config, VHost, 0),
+
+    ?assertEqual(0, count_connections_in(Config, VHost)),
+
+    %% with limit = 0 no connections are allowed
+    expect_that_client_connection_is_rejected(Config),
+    expect_that_client_connection_is_rejected(Config),
+    expect_that_client_connection_is_rejected(Config),
+
+    set_vhost_connection_limit(Config, VHost, -1),
+    Conn1 = open_unmanaged_connection(Config, 0),
+    Conn2 = open_unmanaged_connection(Config, 0),
+
+    lists:foreach(fun (C) ->
+                          rabbit_ct_client_helpers:close_connection(C)
+                  end, [Conn1, Conn2]),
+
+    ?assertEqual(0, count_connections_in(Config, VHost)).
+
 
 single_node_multiple_vhost_limit(Config) ->
     VHost1 = <<"vhost1">>,
@@ -442,13 +470,13 @@ single_node_multiple_vhost_limit(Config) ->
     Conn4 = open_unmanaged_connection(Config, 0, VHost2),
 
     %% we've crossed the limit
-    {error, not_allowed} = open_unmanaged_connection(Config, 0, VHost1),
-    {error, not_allowed} = open_unmanaged_connection(Config, 0, VHost2),
+    expect_that_client_connection_is_rejected(Config, 0, VHost1),
+    expect_that_client_connection_is_rejected(Config, 0, VHost2),
 
     Conn5 = open_unmanaged_connection(Config, 0),
 
     set_vhost_connection_limit(Config, VHost1, 5),
-    set_vhost_connection_limit(Config, VHost2, 5),
+    set_vhost_connection_limit(Config, VHost2, -10),
 
     Conn6 = open_unmanaged_connection(Config, 0, VHost1),
     Conn7 = open_unmanaged_connection(Config, 0, VHost1),
@@ -464,12 +492,47 @@ single_node_multiple_vhost_limit(Config) ->
     ?assertEqual(0, count_connections_in(Config, VHost1)),
     ?assertEqual(0, count_connections_in(Config, VHost2)),
 
-    set_vhost_connection_limit(Config, VHost1, 100000000),
-    set_vhost_connection_limit(Config, VHost2, 100000000),
+    set_vhost_connection_limit(Config, VHost1, -1),
+    set_vhost_connection_limit(Config, VHost2, -1),
 
     rabbit_ct_broker_helpers:delete_vhost(Config, VHost1),
     rabbit_ct_broker_helpers:delete_vhost(Config, VHost2).
 
+
+single_node_multiple_vhost_zero_limit(Config) ->
+    VHost1 = <<"vhost1">>,
+    VHost2 = <<"vhost2">>,
+
+    set_up_vhost(Config, VHost1),
+    set_up_vhost(Config, VHost2),
+
+    set_vhost_connection_limit(Config, VHost1, 0),
+    set_vhost_connection_limit(Config, VHost2, 0),
+
+    ?assertEqual(0, count_connections_in(Config, VHost1)),
+    ?assertEqual(0, count_connections_in(Config, VHost2)),
+
+    %% with limit = 0 no connections are allowed
+    expect_that_client_connection_is_rejected(Config, 0, VHost1),
+    expect_that_client_connection_is_rejected(Config, 0, VHost2),
+    expect_that_client_connection_is_rejected(Config, 0, VHost1),
+
+    set_vhost_connection_limit(Config, VHost1, -1),
+    Conn1 = open_unmanaged_connection(Config, 0, VHost1),
+    Conn2 = open_unmanaged_connection(Config, 0, VHost1),
+
+    lists:foreach(fun (C) ->
+                          rabbit_ct_client_helpers:close_connection(C)
+                  end, [Conn1, Conn2]),
+
+    ?assertEqual(0, count_connections_in(Config, VHost1)),
+    ?assertEqual(0, count_connections_in(Config, VHost2)),
+
+    set_vhost_connection_limit(Config, VHost1, -1),
+    set_vhost_connection_limit(Config, VHost2, -1).
+
+
+
 cluster_single_vhost_limit(Config) ->
     VHost = <<"/">>,
     set_vhost_connection_limit(Config, VHost, 2),
@@ -483,8 +546,8 @@ cluster_single_vhost_limit(Config) ->
     timer:sleep(200),
 
     %% we've crossed the limit
-    {error, not_allowed} = open_unmanaged_connection(Config, 0, VHost),
-    {error, not_allowed} = open_unmanaged_connection(Config, 1, VHost),
+    expect_that_client_connection_is_rejected(Config, 0, VHost),
+    expect_that_client_connection_is_rejected(Config, 1, VHost),
 
     set_vhost_connection_limit(Config, VHost, 5),
 
@@ -497,7 +560,7 @@ cluster_single_vhost_limit(Config) ->
 
     ?assertEqual(0, count_connections_in(Config, VHost)),
 
-    set_vhost_connection_limit(Config, VHost, 0).
+    set_vhost_connection_limit(Config, VHost,  -1).
 
 cluster_single_vhost_limit2(Config) ->
     VHost = <<"/">>,
@@ -510,10 +573,10 @@ cluster_single_vhost_limit2(Config) ->
     Conn2 = open_unmanaged_connection(Config, 0, VHost),
 
     %% we've crossed the limit
-    {error, not_allowed} = open_unmanaged_connection(Config, 0, VHost),
+    expect_that_client_connection_is_rejected(Config, 0, VHost),
 
     timer:sleep(200),
-    {error, not_allowed} = open_unmanaged_connection(Config, 1, VHost),
+    expect_that_client_connection_is_rejected(Config, 1, VHost),
 
     set_vhost_connection_limit(Config, VHost, 5),
 
@@ -528,7 +591,7 @@ cluster_single_vhost_limit2(Config) ->
 
     ?assertEqual(0, count_connections_in(Config, VHost)),
 
-    set_vhost_connection_limit(Config, VHost, 0).
+    set_vhost_connection_limit(Config, VHost,  -1).
 
 single_node_vhost_deletion_forces_connection_closure(Config) ->
     VHost1 = <<"vhost1">>,
@@ -634,3 +697,12 @@ set_vhost_connection_limit(Config, NodeIndex, VHost, Count) ->
 
 await_running_node_refresh(_Config, _NodeIndex) ->
     timer:sleep(250).
+
+expect_that_client_connection_is_rejected(Config) ->
+    expect_that_client_connection_is_rejected(Config, 0).
+
+expect_that_client_connection_is_rejected(Config, NodeIndex) ->
+    {error, not_allowed} = open_unmanaged_connection(Config, NodeIndex).
+
+expect_that_client_connection_is_rejected(Config, NodeIndex, VHost) ->
+    {error, not_allowed} = open_unmanaged_connection(Config, NodeIndex, VHost).