refactor limits and throttling

irc/client.go

@@ -542,9 +542,7 @@ func (client *Client) destroy() {
 	ipaddr := client.IP()
 	// this check shouldn't be required but eh
 	if ipaddr != nil {
-		client.server.connectionLimitsMutex.Lock()
 		client.server.connectionLimits.RemoveClient(ipaddr)
-		client.server.connectionLimitsMutex.Unlock()
 	}
 
 	// alert monitors

irc/connection_limits.go

@@ -7,6 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"net"
+	"sync"
 )
 
 var (
@@ -15,6 +16,8 @@ var (
 
 // ConnectionLimits manages the automated client connection limits.
 type ConnectionLimits struct {
+	sync.Mutex
+
 	enabled  bool
 	ipv4Mask net.IPMask
 	ipv6Mask net.IPMask
@@ -45,6 +48,9 @@ func (cl *ConnectionLimits) maskAddr(addr net.IP) net.IP {
 // AddClient adds a client to our population if possible. If we can't, throws an error instead.
 // 'force' is used to add already-existing clients (i.e. ones that are already on the network).
 func (cl *ConnectionLimits) AddClient(addr net.IP, force bool) error {
+	cl.Lock()
+	defer cl.Unlock()
+
 	if !cl.enabled {
 		return nil
 	}
@@ -75,6 +81,9 @@ func (cl *ConnectionLimits) AddClient(addr net.IP, force bool) error {
 
 // RemoveClient removes the given address from our population
 func (cl *ConnectionLimits) RemoveClient(addr net.IP) {
+	cl.Lock()
+	defer cl.Unlock()
+
 	if !cl.enabled {
 		return
 	}
@@ -89,34 +98,47 @@ func (cl *ConnectionLimits) RemoveClient(addr net.IP) {
 }
 
 // NewConnectionLimits returns a new connection limit handler.
-func NewConnectionLimits(config ConnectionLimitsConfig) (*ConnectionLimits, error) {
+// The handler is functional, but disabled; it can be enabled via `ApplyConfig`.
+func NewConnectionLimits() *ConnectionLimits {
 	var cl ConnectionLimits
-	cl.enabled = config.Enabled
 
+	// initialize empty population; all other state is configurable
 	cl.population = make(map[string]int)
-	cl.exemptedIPs = make(map[string]bool)
 
-	cl.ipv4Mask = net.CIDRMask(config.CidrLenIPv4, 32)
-	cl.ipv6Mask = net.CIDRMask(config.CidrLenIPv6, 128)
-	// subnetLimit is explicitly NOT capped at a minimum of one.
-	// this is so that CL config can be used to allow ONLY clients from exempted IPs/nets
-	cl.subnetLimit = config.IPsPerCidr
+	return &cl
+}
 
+// ApplyConfig atomically applies a config update to a connection limit handler
+func (cl *ConnectionLimits) ApplyConfig(config ConnectionLimitsConfig) error {
 	// assemble exempted nets
+	exemptedIPs := make(map[string]bool)
+	var exemptedNets []net.IPNet
 	for _, cidr := range config.Exempted {
 		ipaddr := net.ParseIP(cidr)
 		_, netaddr, err := net.ParseCIDR(cidr)
 
 		if ipaddr == nil && err != nil {
-			return nil, fmt.Errorf("Could not parse exempted IP/network [%s]", cidr)
+			return fmt.Errorf("Could not parse exempted IP/network [%s]", cidr)
 		}
 
 		if ipaddr != nil {
-			cl.exemptedIPs[ipaddr.String()] = true
+			exemptedIPs[ipaddr.String()] = true
 		} else {
-			cl.exemptedNets = append(cl.exemptedNets, *netaddr)
+			exemptedNets = append(exemptedNets, *netaddr)
 		}
 	}
 
-	return &cl, nil
+	cl.Lock()
+	defer cl.Unlock()
+
+	cl.enabled = config.Enabled
+	cl.ipv4Mask = net.CIDRMask(config.CidrLenIPv4, 32)
+	cl.ipv6Mask = net.CIDRMask(config.CidrLenIPv6, 128)
+	// subnetLimit is explicitly NOT capped at a minimum of one.
+	// this is so that CL config can be used to allow ONLY clients from exempted IPs/nets
+	cl.subnetLimit = config.IPsPerCidr
+	cl.exemptedIPs = exemptedIPs
+	cl.exemptedNets = exemptedNets
+
+	return nil
 }

irc/connection_throttling.go

@@ -6,6 +6,7 @@ package irc
 import (
 	"fmt"
 	"net"
+	"sync"
 	"time"
 )
 
@@ -17,6 +18,8 @@ type ThrottleDetails struct {
 
 // ConnectionThrottle manages automated client connection throttling.
 type ConnectionThrottle struct {
+	sync.RWMutex
+
 	enabled     bool
 	ipv4Mask    net.IPMask
 	ipv6Mask    net.IPMask
@@ -25,9 +28,8 @@ type ConnectionThrottle struct {
 	population  map[string]ThrottleDetails
 
 	// used by the server to ban clients that go over this limit
-	BanDuration     time.Duration
-	BanMessage      string
-	BanMessageBytes []byte
+	banDuration time.Duration
+	banMessage  string
 
 	// exemptedIPs holds IPs that are exempt from limits
 	exemptedIPs map[string]bool
@@ -50,6 +52,9 @@ func (ct *ConnectionThrottle) maskAddr(addr net.IP) net.IP {
 
 // ResetFor removes any existing count for the given address.
 func (ct *ConnectionThrottle) ResetFor(addr net.IP) {
+	ct.Lock()
+	defer ct.Unlock()
+
 	if !ct.enabled {
 		return
 	}
@@ -62,6 +67,9 @@ func (ct *ConnectionThrottle) ResetFor(addr net.IP) {
 
 // AddClient introduces a new client connection if possible. If we can't, throws an error instead.
 func (ct *ConnectionThrottle) AddClient(addr net.IP) error {
+	ct.Lock()
+	defer ct.Unlock()
+
 	if !ct.enabled {
 		return nil
 	}
@@ -97,38 +105,63 @@ func (ct *ConnectionThrottle) AddClient(addr net.IP) error {
 	return nil
 }
 
+func (ct *ConnectionThrottle) BanDuration() time.Duration {
+	ct.RLock()
+	defer ct.RUnlock()
+
+	return ct.banDuration
+}
+
+func (ct *ConnectionThrottle) BanMessage() string {
+	ct.RLock()
+	defer ct.RUnlock()
+
+	return ct.banMessage
+}
+
 // NewConnectionThrottle returns a new client connection throttler.
-func NewConnectionThrottle(config ConnectionThrottleConfig) (*ConnectionThrottle, error) {
+// The throttler is functional, but disabled; it can be enabled via `ApplyConfig`.
+func NewConnectionThrottle() *ConnectionThrottle {
 	var ct ConnectionThrottle
-	ct.enabled = config.Enabled
 
+	// initialize empty population; all other state is configurable
 	ct.population = make(map[string]ThrottleDetails)
-	ct.exemptedIPs = make(map[string]bool)
 
-	ct.ipv4Mask = net.CIDRMask(config.CidrLenIPv4, 32)
-	ct.ipv6Mask = net.CIDRMask(config.CidrLenIPv6, 128)
-	ct.subnetLimit = config.ConnectionsPerCidr
-
-	ct.duration = config.Duration
-
-	ct.BanDuration = config.BanDuration
-	ct.BanMessage = config.BanMessage
+	return &ct
+}
 
+// ApplyConfig atomically applies a config update to a throttler
+func (ct *ConnectionThrottle) ApplyConfig(config ConnectionThrottleConfig) error {
 	// assemble exempted nets
+	exemptedIPs := make(map[string]bool)
+	var exemptedNets []net.IPNet
 	for _, cidr := range config.Exempted {
 		ipaddr := net.ParseIP(cidr)
 		_, netaddr, err := net.ParseCIDR(cidr)
 
 		if ipaddr == nil && err != nil {
-			return nil, fmt.Errorf("Could not parse exempted IP/network [%s]", cidr)
+			return fmt.Errorf("Could not parse exempted IP/network [%s]", cidr)
 		}
 
 		if ipaddr != nil {
-			ct.exemptedIPs[ipaddr.String()] = true
+			exemptedIPs[ipaddr.String()] = true
 		} else {
-			ct.exemptedNets = append(ct.exemptedNets, *netaddr)
+			exemptedNets = append(exemptedNets, *netaddr)
 		}
 	}
 
-	return &ct, nil
+	ct.Lock()
+	defer ct.Unlock()
+
+	ct.enabled = config.Enabled
+	ct.ipv4Mask = net.CIDRMask(config.CidrLenIPv4, 32)
+	ct.ipv6Mask = net.CIDRMask(config.CidrLenIPv6, 128)
+	ct.subnetLimit = config.ConnectionsPerCidr
+	ct.duration = config.Duration
+	ct.banDuration = config.BanDuration
+	ct.banMessage = config.BanMessage
+	ct.exemptedIPs = exemptedIPs
+	ct.exemptedNets = exemptedNets
+
+	return nil
 }

irc/server.go

@@ -87,9 +87,7 @@ type Server struct {
 	configFilename               string
 	configurableStateMutex       sync.RWMutex // generic protection for server state modified by rehash()
 	connectionLimits             *ConnectionLimits
-	connectionLimitsMutex        sync.Mutex // used when affecting the connection limiter, to make sure rehashing doesn't make things go out-of-whack
 	connectionThrottle           *ConnectionThrottle
-	connectionThrottleMutex      sync.Mutex // used when affecting the connection limiter, to make sure rehashing doesn't make things go out-of-whack
 	ctime                        time.Time
 	defaultChannelModes          Modes
 	dlines                       *DLineManager
@@ -149,6 +147,8 @@ func NewServer(config *Config, logger *logger.Manager) (*Server, error) {
 		channels:           *NewChannelNameMap(),
 		clients:            NewClientLookupSet(),
 		commands:           make(chan Command),
+		connectionLimits:   connection_limiting.NewConnectionLimits(),
+		connectionThrottle: connection_limiting.NewConnectionThrottle(),
 		listeners:          make(map[string]*ListenerWrapper),
 		logger:             logger,
 		monitorManager:     NewMonitorManager(),
@@ -301,32 +301,29 @@ func (server *Server) checkBans(ipaddr net.IP) (banned bool, message string) {
 	}
 
 	// check connection limits
-	server.connectionLimitsMutex.Lock()
 	err := server.connectionLimits.AddClient(ipaddr, false)
-	server.connectionLimitsMutex.Unlock()
 	if err != nil {
 		// too many connections from one client, tell the client and close the connection
 		return true, "Too many clients from your network"
 	}
 
 	// check connection throttle
-	server.connectionThrottleMutex.Lock()
 	err = server.connectionThrottle.AddClient(ipaddr)
-	server.connectionThrottleMutex.Unlock()
 	if err != nil {
 		// too many connections too quickly from client, tell them and close the connection
+		duration := server.connectionThrottle.BanDuration()
 		length := &IPRestrictTime{
-			Duration: server.connectionThrottle.BanDuration,
-			Expires:  time.Now().Add(server.connectionThrottle.BanDuration),
+			Duration: duration,
+			Expires:  time.Now().Add(duration),
 		}
-		server.dlines.AddIP(ipaddr, length, server.connectionThrottle.BanMessage, "Exceeded automated connection throttle")
+		server.dlines.AddIP(ipaddr, length, server.connectionThrottle.BanMessage(), "Exceeded automated connection throttle")
 
 		// they're DLINE'd for 15 minutes or whatever, so we can reset the connection throttle now,
 		// and once their temporary DLINE is finished they can fill up the throttler again
 		server.connectionThrottle.ResetFor(ipaddr)
 
 		// this might not show up properly on some clients, but our objective here is just to close it out before it has a load impact on us
-		return true, server.connectionThrottle.BanMessage
+		return true, server.connectionThrottle.BanMessage()
 	}
 
 	return false, ""
@@ -1229,18 +1226,6 @@ func (server *Server) applyConfig(config *Config, initial bool) error {
 		return fmt.Errorf("Server name isn't valid [%s]: %s", config.Server.Name, err.Error())
 	}
 
-	// confirm connectionLimits are fine
-	connectionLimits, err := NewConnectionLimits(config.Server.ConnectionLimits)
-	if err != nil {
-		return fmt.Errorf("Error rehashing config file connection-limits: %s", err.Error())
-	}
-
-	// confirm connectionThrottler is fine
-	connectionThrottle, err := NewConnectionThrottle(config.Server.ConnectionThrottle)
-	if err != nil {
-		return fmt.Errorf("Error rehashing config file connection-throttle: %s", err.Error())
-	}
-
 	// confirm operator stuff all exists and is fine
 	operclasses, err := config.OperatorClasses()
 	if err != nil {
@@ -1272,22 +1257,15 @@ func (server *Server) applyConfig(config *Config, initial bool) error {
 	// apply new PROXY command restrictions
 	server.proxyAllowedFrom = config.Server.ProxyAllowedFrom
 
-	// apply new connectionlimits
-	server.connectionLimitsMutex.Lock()
-	server.connectionLimits = connectionLimits
-	server.connectionThrottleMutex.Lock()
-	server.connectionThrottle = connectionThrottle
+	err = server.connectionLimits.ApplyConfig(config.Server.ConnectionLimits)
+	if err != nil {
+		return err
+	}
 
-	server.clients.ByNickMutex.RLock()
-	for _, client := range server.clients.ByNick {
-		ipaddr := client.IP()
-		if ipaddr != nil {
-			server.connectionLimits.AddClient(ipaddr, true)
+	err = server.connectionThrottle.ApplyConfig(config.Server.ConnectionThrottle)
+	if err != nil {
+		return err
 	}
-	}
-	server.clients.ByNickMutex.RUnlock()
-	server.connectionThrottleMutex.Unlock()
-	server.connectionLimitsMutex.Unlock()
 
 	// setup new and removed caps
 	addedCaps := caps.NewSet()