From 7f72151bcc291a28c7eda9dbb06d944528a7c140 Mon Sep 17 00:00:00 2001
From: piersy <pierspowlesland@gmail.com>
Date: Mon, 25 Sep 2023 14:09:46 +0100
Subject: [PATCH] config: warn if connmgr limits conflict with rcmgr (#2527)

Co-authored-by: Sukun <sukunrt@gmail.com>
---
 config/config.go                    |  9 +++++++++
 core/connmgr/manager.go             | 10 ++++++++++
 core/connmgr/null.go                |  1 +
 p2p/host/resource-manager/extapi.go |  4 ++++
 p2p/net/connmgr/connmgr.go          | 12 ++++++++++++
 p2p/net/connmgr/connmgr_test.go     | 21 +++++++++++++++++++++
 6 files changed, 57 insertions(+)

diff --git a/config/config.go b/config/config.go
index bb3a121356..640584250c 100644
--- a/config/config.go
+++ b/config/config.go
@@ -295,6 +295,15 @@ func (cfg *Config) addTransports(h host.Host) error {
 //
 // This function consumes the config. Do not reuse it (really!).
 func (cfg *Config) NewNode() (host.Host, error) {
+	// If possible check that the resource manager conn limit is higher than the
+	// limit set in the conn manager.
+	if l, ok := cfg.ResourceManager.(connmgr.GetConnLimiter); ok {
+		err := cfg.ConnManager.CheckLimit(l)
+		if err != nil {
+			log.Warn(fmt.Sprintf("rcmgr limit conflicts with connmgr limit: %v", err))
+		}
+	}
+
 	eventBus := eventbus.NewBus(eventbus.WithMetricsTracer(eventbus.NewMetricsTracer(eventbus.WithRegisterer(cfg.PrometheusRegisterer))))
 	swrm, err := cfg.makeSwarm(eventBus, !cfg.DisableMetrics)
 	if err != nil {
diff --git a/core/connmgr/manager.go b/core/connmgr/manager.go
index e0d6c52012..d756e4399f 100644
--- a/core/connmgr/manager.go
+++ b/core/connmgr/manager.go
@@ -74,6 +74,10 @@ type ConnManager interface {
 	// then it will return true if the peer is protected for any tag
 	IsProtected(id peer.ID, tag string) (protected bool)
 
+	// CheckLimit will return an error if the connection manager's internal
+	// connection limit exceeds the provided system limit.
+	CheckLimit(l GetConnLimiter) error
+
 	// Close closes the connection manager and stops background processes.
 	Close() error
 }
@@ -89,3 +93,9 @@ type TagInfo struct {
 	// Conns maps connection ids (such as remote multiaddr) to their creation time.
 	Conns map[string]time.Time
 }
+
+// GetConnLimiter provides access to a component's total connection limit.
+type GetConnLimiter interface {
+	// GetConnLimit returns the total connection limit of the implementing component.
+	GetConnLimit() int
+}
diff --git a/core/connmgr/null.go b/core/connmgr/null.go
index 25743f4ec7..735411712a 100644
--- a/core/connmgr/null.go
+++ b/core/connmgr/null.go
@@ -21,4 +21,5 @@ func (NullConnMgr) Notifee() network.Notifiee                { return network.Gl
 func (NullConnMgr) Protect(peer.ID, string)                  {}
 func (NullConnMgr) Unprotect(peer.ID, string) bool           { return false }
 func (NullConnMgr) IsProtected(peer.ID, string) bool         { return false }
+func (NullConnMgr) CheckLimit(l GetConnLimiter) error        { return nil }
 func (NullConnMgr) Close() error                             { return nil }
diff --git a/p2p/host/resource-manager/extapi.go b/p2p/host/resource-manager/extapi.go
index 03edcd79ea..b3214f814a 100644
--- a/p2p/host/resource-manager/extapi.go
+++ b/p2p/host/resource-manager/extapi.go
@@ -145,3 +145,7 @@ func (r *resourceManager) Stat() (result ResourceManagerStat) {
 
 	return result
 }
+
+func (r *resourceManager) GetConnLimit() int {
+	return r.limits.GetConnLimits().GetConnTotalLimit()
+}
diff --git a/p2p/net/connmgr/connmgr.go b/p2p/net/connmgr/connmgr.go
index b42a122fac..6caa3dc1a6 100644
--- a/p2p/net/connmgr/connmgr.go
+++ b/p2p/net/connmgr/connmgr.go
@@ -2,6 +2,7 @@ package connmgr
 
 import (
 	"context"
+	"fmt"
 	"sort"
 	"sync"
 	"sync/atomic"
@@ -239,6 +240,17 @@ func (cm *BasicConnMgr) IsProtected(id peer.ID, tag string) (protected bool) {
 	return protected
 }
 
+func (cm *BasicConnMgr) CheckLimit(systemLimit connmgr.GetConnLimiter) error {
+	if cm.cfg.highWater > systemLimit.GetConnLimit() {
+		return fmt.Errorf(
+			"conn manager high watermark limit: %d, exceeds the system connection limit of: %d",
+			cm.cfg.highWater,
+			systemLimit.GetConnLimit(),
+		)
+	}
+	return nil
+}
+
 // peerInfo stores metadata for a given peer.
 type peerInfo struct {
 	id       peer.ID
diff --git a/p2p/net/connmgr/connmgr_test.go b/p2p/net/connmgr/connmgr_test.go
index 8b5bbad440..3c2813e5b6 100644
--- a/p2p/net/connmgr/connmgr_test.go
+++ b/p2p/net/connmgr/connmgr_test.go
@@ -966,3 +966,24 @@ func TestSafeConcurrency(t *testing.T) {
 		wg.Wait()
 	})
 }
+
+func TestCheckLimit(t *testing.T) {
+	low, hi := 1, 2
+	cm, err := NewConnManager(low, hi)
+	require.NoError(t, err)
+
+	err = cm.CheckLimit(testLimitGetter{hi + 1})
+	require.NoError(t, err)
+	err = cm.CheckLimit(testLimitGetter{hi})
+	require.NoError(t, err)
+	err = cm.CheckLimit(testLimitGetter{hi - 1})
+	require.Error(t, err)
+}
+
+type testLimitGetter struct {
+	limit int
+}
+
+func (g testLimitGetter) GetConnLimit() int {
+	return g.limit
+}