[FIXED] Honor max_connections setting #387
Conversation
The max_connections config parameter was accepted but the server would not check this limit when processing client connections. Resolves #386
|
@derekcollison Did not see that you assigned #386 to yourself. Feel free to ignore this PR. Not even sure if this is the best way of enforcing the limit. |
| @@ -447,6 +447,12 @@ func (c *client) processConnect(arg []byte) error { | |||
| srv.mu.Unlock() | |||
| } | |||
|
|
|||
| // Check for max connections | |||
There was a problem hiding this comment.
The client does not technically have to send a connect (think telnet to demo server). So we should probably check on accept and simply send an error there.
There was a problem hiding this comment.
This is what I did first (hence the fix in clearConnection because when calling closeConnection before client is initialized would crash since c.bw was nil).
Keep in mind that the client expects an INFO before any possible error message. So I need to do the send after the client is initialized (buffer created so that sendErr actually works) and after sending the server info, all that in createClient(). The problem with this approach is that the client will get the error only, in my tests, 10% of the cases. Otherwise, it either gets EOF or connection reset by peer. With the original approach, it gets the error 100% of the times. That being said, I understand the argument of telnet, etc.. so I guess it is worth having the client not always know why it failed to connect but maximizing the protection of the server, right?
| @@ -490,6 +496,18 @@ func (c *client) authViolation() { | |||
| c.closeConnection() | |||
| } | |||
|
|
|||
| func (c *client) maxConnLimit() { | |||
| if c.srv != nil && c.srv.opts.Users != nil { | |||
There was a problem hiding this comment.
Since its global scope I think we can just use the non-user generic one below, don't need the user specific one IMO.
|
|
||
| // ErrTooManyConnections signals a client that the maximum number of connections supported by the | ||
| // server has been reached. | ||
| ErrTooManyConnections = errors.New("Too many connections") |
There was a problem hiding this comment.
Maybe "Maximum Connections Exceeded"
| @@ -731,6 +731,17 @@ func (s *Server) checkAuth(c *client) bool { | |||
| } | |||
| } | |||
|
|
|||
| // Check that number of clients is below Max connection setting. | |||
| func (s *Server) checkMaxConn(c *client) bool { | |||
| if c.typ == CLIENT { | |||
There was a problem hiding this comment.
If we have added to s.clients, we could just do the following since if its a route will be ok I think.
s.mu.Lock()
defer s.mu.Unlock()
return len(s.clients) <+ s.opts.MaxConn
There was a problem hiding this comment.
And there is a huge defect here that I should check that s.opts.MaxConn is > 0 first!!!
| nc2, err := nats.Connect(addr) | ||
| if err == nil { | ||
| if nc2 != nil { | ||
| nc2.Close() |
There was a problem hiding this comment.
We can call Close() on nil no?
There was a problem hiding this comment.
No we can't, but I should not check for nc2 != nil since if err == nil, it means that nc2 is not nil!
|
Thanks for the PR @kozlovic, no worries. |
This will protect the server from non NATS clients (telnet, etc), or misbehaving clients that would create the tcp connection but block before sending the CONNECT. The drawback is that the client may or may not receive the error message (in my tests, it was getting only between 10%-20% of times).
|
@derekcollison I just realized that probably this check is no longer relevant since we close the client connection before starting the client's |
|
Feel free to remove in new PR. |
The max_connections config parameter was accepted but the server
would not check this limit when processing client connections.
Resolves #386