[jira] [Commented] (QPID-8484) [Broker-J] Reimplementation of the limit number of connections per user

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/QPID-8484?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17313227#comment-17313227 ] 

ASF GitHub Bot commented on QPID-8484:
--------------------------------------

mklaca edited a comment on pull request #76:
URL: https://github.com/apache/qpid-broker-j/pull/76#issuecomment-800155282


   Hi @alex-rufous,
   
   > I think that creation of design document (we can use qpid confluence) for this new feature (before jumping into implementation) would benefit us both and could save us a lot of time. it is still not to late to do it.
   
   I don't have rights to create a new or edit any qpid confluence page.
   
   > IMHO functionality to check limits should live on ConnectionLimitProviders including registering and de-registering connections. On connection registration/deregistration the VH simply needs to iterate through its own and broker ConnectionLimitProviders and call corresponding methods (register/deregister) to verify limits. The VH should not call ConnectionLimiter and keep a reference to it. Instead, VH should operate with ConnectionLimitProviders.
   
   If the borker ConnectionLimitProvider kept the ConnectionLimiter then the connection counters inside the limiter would be shared amoung multiple VirtualHosts. The VirtualHosts would effect each other, but it is stated in the documentation - [Java Broker concepts](http://qpid.apache.org/releases/qpid-broker-j-8.0.3/book/Java-Broker-Concepts-Virtualhosts.html) that:
   _Virtualhosts are independent; the messaging that goes on within one virtualhost is independent of any messaging that goes on in another virtualhost._
   Hence, I proposed the ConnectionLimitProvider as a factory that builds a new independent limiter, which is provided to a VirtualHost.
   
   VirtualHost can not simple iterate through the limit providers or limiters because the consistency of the connection counters is needed. Let's suppose that the VirtualHost has multiple limit providers. If any of the providers/limiters fails to register connection then all have to fail. The connection has to be registered everywhere or nowhere and so the co-ordination is needed. Hence, the virtual host needs a master limiter that co-ordinates the update of the connection counters.
   
   There are concurrency issues with simple iteration. The working thread needs an exclusive access to all connection counters of the user to check and update all counters atomically.
   
   I see three approaches:
   1. One connection limiter.
   2. Master limiter that co-ordinates all limiters.
   3. Chain of responsibility, when the limiter itself call the next limiter.
   
   I have to point out that the functionality of the multiple limiters has not been requested yet.
   
   > IMHO a special runtime exception can be thrown when any of the limits is breached this type of exception can be caught in the protocol IO layers and connection can be closed accordingly.
   
   I have introduced a new ConnectionLimitException but it looks like a checked exception to me.
   
   > I would prefer to use json/yml over bespoke format. That should reduce implementation code.
   
   I want the file based configuration that is backward compatible with existing ACL rule file. Therefore, I had introduced a ACL like bespoke format and so it is not required to generate a new configuration file in different format. The file based configuration can point to the former ACL file.
   
   > The limit rules can be simplified by removing "Blocked". The user simply can create a rule for ALL where connection limit (frequency limit) is 0.
   
   Yes, it can be but 
   
   - Declaring an user as blocked is more friendly and understandable.
   - Setting "count_limit=0" could be confusing because the value 0 has meaning "no limit" in many other situations. I don't like the idea of the "magic" constant.
   - The blocked user is handled in different way as unblocked user, a connection opened by blocked user can be rejected immediately and any counter is not needed.
   
   > A frequency value can be set as a number of connections per unit of time in the rules. For example, 1/s, 2/m,100/h, 1000/d. I think that would make the rules simpler for the end user.
   
   Yes it is transparent for end user, I have implemented it but the feature complicates evaluation of the rules a lot.
   
   Let suppose that user is a part of the groups gA, gB, gC. Each group has frequency limit:
   gA: frequency limit = 10
   gB: frequency limit = 5
   gC: frequency limit = 15
   I can easily pick up the most restrictive limit 5 because all limits have the same time period and I can easily merge all limits into single effective limit.
   
   The frequency limit 1/s is not the same as 60/m, they looks similar but 60/m allows a short peak of new connections, 1/s does not. If multiple rules could have different time period then I can not pick up the most restrictive limit and the user can have multiple frequency limits with different frequency periods.
   
   > The reject reason can be logged as part of connection close operational logs. Thus, new message resources are not really required. We can delete those and all corresponding logging functionality.
   
   We have received complains and our support team queries the logging of the connection limits. Rejecting a new connection because of the breaking the limits is not a regular close operation.
   
   We need:
   - Log the limit when a new connection is rejected.
   - Log current value of the counter when a new connection is accepted. This should be optional.
   - Connection limit messages should be easy to distinguish and filter out.
   
   > A DeleteTask for every identity should be registered on the connection to free the slot.
   
   I can not use DeleteTask because it does not ensure release of the connection slot when any of the delete tasks fails.
   Suppose that the connection has 3 delete task, if first task fails then the second and third task are not executed. Hence, the connection counter remains none zero for ever.
   The de-registering the connection from VirtualHost is not delete task because of the same reason.
   
   Regards,
   Marek


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


> [Broker-J] Reimplementation of the limit number of connections per user
> -----------------------------------------------------------------------
>
>                 Key: QPID-8484
>                 URL: https://issues.apache.org/jira/browse/QPID-8484
>             Project: Qpid
>          Issue Type: Improvement
>          Components: Broker-J
>    Affects Versions: qpid-java-broker-8.0.1, qpid-java-broker-8.0.2
>            Reporter: Marek Laca
>            Priority: Major
>              Labels: connection, limit, user
>
> If some user creates too much connections, he can prevent other users from connecting to amqp ports. [QPID-8369|https://issues.apache.org/jira/projects/QPID/issues/QPID-8369] added an ability to limit maximum open connections per user.
>  The user connection limit was implemented as ACL dynamic rule and it is part of the access control logic.
> But I have queries about the implementation:
>  * The connection count of the user is not checked properly.
>  For example 2 connections should be rejected when an user has limit 5 and tries to open 7 parallel connections. But what happens:
>  ## Every connection increments the counter then the counter will be 7.
>  ## ACL logic compares the actual counter value with the limit for every connection (the counter value at the moment of the acl rule check) and 2,3 … or all 7 connections can be denied. The ACL logic does not know which connection broke the limit.
>  ## The counter is decremented when connection is closed.
>  * ACL rules were static and so the result of the check did not vary in time and the Broker could cache the result ALLOWED or DENIED. From my point of view a dynamic check should not be part of the ACL logic because it makes ACL logic time dependent.
>  * The user connection limit should be checked as soon as possible.
> I suggest to introduce a new plugin (similar to the access control provider plugin) that will hold the user's counters of open connections.
>  It will provide following functionality:
>  * It registers new connections for given user and the connection will be rejected if the user breaks the limit. The registration and update of user's counters will be an atomic operation.
>  * It de-registers the connections for given user.
> If user breaks the limit then the connection will be closed with proper response "amqp:resource-limit-exceeded" instead of "amqp:not-allowed".



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org