From 9e3499cf58fb44c0094013e26805dd78d54274b6 Mon Sep 17 00:00:00 2001
From: Michel Laterman <82832767+michel-laterman@users.noreply.github.com>
Date: Mon, 26 Sep 2022 15:22:48 -0700
Subject: [PATCH] LoadLimits does not override existing values (#1912)

Fleet-server will use any specified cache or server limit values over
whatever is returned by the default/agent number loader. For example, if
A max body size is specifically set to a value such as 5MB, and the
default returned by the LoadLimits is 1MB, the 5MB value is used.

(cherry picked from commit 1b2834cf7bb03f785df54d59eae1f352abc31f5c)
---
 CHANGELOG.next.asciidoc            |  1 +
 internal/pkg/config/cache.go       | 29 +++++++++++----
 internal/pkg/config/config_test.go | 32 ++++++++++++++++
 internal/pkg/config/limits.go      | 59 ++++++++++++++++--------------
 4 files changed, 86 insertions(+), 35 deletions(-)

diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc
index 63e4e3bc0..c65b70b5a 100644
--- a/CHANGELOG.next.asciidoc
+++ b/CHANGELOG.next.asciidoc
@@ -7,6 +7,7 @@
 - Improve authc debug logging. {pull}1870[1870]
 - Add error detail to catch-all HTTP error response. {pull}1854[1854]
 - Update apikey.cache_hit log field name to match convention. {pull}1900[1900]
+- LoadServerLimits will not overwrite specified limits when loading default/agent number specified values. {issue}1841[1841] {pull}1912[1912]
 
 ==== New Features
 
diff --git a/internal/pkg/config/cache.go b/internal/pkg/config/cache.go
index f6e3f84e7..c3bc5ab39 100644
--- a/internal/pkg/config/cache.go
+++ b/internal/pkg/config/cache.go
@@ -30,14 +30,29 @@ func (c *Cache) InitDefaults() {
 	c.LoadLimits(loadLimits(0))
 }
 
+// LoadLimits loads envLimits for any attribute that is not defined in Cache
 func (c *Cache) LoadLimits(limits *envLimits) {
 	l := limits.Cache
 
-	c.NumCounters = l.NumCounters
-	c.MaxCost = l.MaxCost
-	c.ActionTTL = defaultActionTTL
-	c.EnrollKeyTTL = defaultEnrollKeyTTL
-	c.ArtifactTTL = defaultArtifactTTL
-	c.APIKeyTTL = defaultAPIKeyTTL
-	c.APIKeyJitter = defaultAPIKeyJitter
+	if c.NumCounters == 0 {
+		c.NumCounters = l.NumCounters
+	}
+	if c.MaxCost == 0 {
+		c.MaxCost = l.MaxCost
+	}
+	if c.ActionTTL == 0 {
+		c.ActionTTL = defaultActionTTL
+	}
+	if c.EnrollKeyTTL == 0 {
+		c.EnrollKeyTTL = defaultEnrollKeyTTL
+	}
+	if c.ArtifactTTL == 0 {
+		c.ArtifactTTL = defaultArtifactTTL
+	}
+	if c.APIKeyTTL == 0 {
+		c.APIKeyTTL = defaultAPIKeyTTL
+	}
+	if c.APIKeyJitter == 0 {
+		c.APIKeyJitter = defaultAPIKeyJitter
+	}
 }
diff --git a/internal/pkg/config/config_test.go b/internal/pkg/config/config_test.go
index 4e3d7ea61..00381e665 100644
--- a/internal/pkg/config/config_test.go
+++ b/internal/pkg/config/config_test.go
@@ -181,6 +181,38 @@ func TestConfig(t *testing.T) {
 	}
 }
 
+func TestLoadServerLimits(t *testing.T) {
+	t.Run("empty loads limits", func(t *testing.T) {
+		c := &Config{Inputs: []Input{{}}}
+		err := c.LoadServerLimits()
+		assert.NoError(t, err)
+		assert.Equal(t, int64(defaultCheckinMaxBody), c.Inputs[0].Server.Limits.CheckinLimit.MaxBody)
+		assert.Equal(t, defaultActionTTL, c.Inputs[0].Cache.ActionTTL)
+	})
+	t.Run("existing values are not overridden", func(t *testing.T) {
+		c := &Config{
+			Inputs: []Input{{
+				Server: Server{
+					Limits: ServerLimits{
+						CheckinLimit: Limit{
+							MaxBody: 5 * defaultCheckinMaxBody,
+						},
+					},
+				},
+				Cache: Cache{
+					ActionTTL: time.Minute,
+				},
+			}},
+		}
+		err := c.LoadServerLimits()
+		assert.NoError(t, err)
+		assert.Equal(t, int64(5*defaultCheckinMaxBody), c.Inputs[0].Server.Limits.CheckinLimit.MaxBody)
+		assert.Equal(t, defaultCheckinBurst, c.Inputs[0].Server.Limits.CheckinLimit.Burst)
+		assert.Equal(t, time.Minute, c.Inputs[0].Cache.ActionTTL)
+	})
+
+}
+
 // Stub out the defaults so that the above is easier to maintain
 
 func defaultCache() Cache {
diff --git a/internal/pkg/config/limits.go b/internal/pkg/config/limits.go
index 1ee1ebe46..0a9b4cb18 100644
--- a/internal/pkg/config/limits.go
+++ b/internal/pkg/config/limits.go
@@ -36,38 +36,41 @@ func (c *ServerLimits) InitDefaults() {
 func (c *ServerLimits) LoadLimits(limits *envLimits) {
 	l := limits.Server
 
-	c.MaxHeaderByteSize = 8192 // 8k
-	c.MaxConnections = l.MaxConnections
-	c.PolicyThrottle = l.PolicyThrottle
+	if c.MaxHeaderByteSize == 0 {
+		c.MaxHeaderByteSize = 8192 // 8k
+	}
+	if c.MaxConnections == 0 {
+		c.MaxConnections = l.MaxConnections
+	}
+	if c.PolicyThrottle == 0 {
+		c.PolicyThrottle = l.PolicyThrottle
+	}
+
+	c.CheckinLimit = mergeEnvLimit(c.CheckinLimit, l.CheckinLimit)
+	c.ArtifactLimit = mergeEnvLimit(c.ArtifactLimit, l.ArtifactLimit)
+	c.EnrollLimit = mergeEnvLimit(c.EnrollLimit, l.EnrollLimit)
+	c.AckLimit = mergeEnvLimit(c.AckLimit, l.AckLimit)
+	c.StatusLimit = mergeEnvLimit(c.StatusLimit, l.StatusLimit)
+}
 
-	c.CheckinLimit = Limit{
-		Interval: l.CheckinLimit.Interval,
-		Burst:    l.CheckinLimit.Burst,
-		Max:      l.CheckinLimit.Max,
-		MaxBody:  l.CheckinLimit.MaxBody,
+func mergeEnvLimit(L Limit, l limit) Limit {
+	result := Limit{
+		Interval: L.Interval,
+		Burst:    L.Burst,
+		Max:      L.Max,
+		MaxBody:  L.MaxBody,
 	}
-	c.ArtifactLimit = Limit{
-		Interval: l.ArtifactLimit.Interval,
-		Burst:    l.ArtifactLimit.Burst,
-		Max:      l.ArtifactLimit.Max,
-		MaxBody:  l.ArtifactLimit.MaxBody,
+	if result.Interval == 0 {
+		result.Interval = l.Interval
 	}
-	c.EnrollLimit = Limit{
-		Interval: l.EnrollLimit.Interval,
-		Burst:    l.EnrollLimit.Burst,
-		Max:      l.EnrollLimit.Max,
-		MaxBody:  l.EnrollLimit.MaxBody,
+	if result.Burst == 0 {
+		result.Burst = l.Burst
 	}
-	c.AckLimit = Limit{
-		Interval: l.AckLimit.Interval,
-		Burst:    l.AckLimit.Burst,
-		Max:      l.AckLimit.Max,
-		MaxBody:  l.AckLimit.MaxBody,
+	if result.Max == 0 {
+		result.Max = l.Max
 	}
-	c.StatusLimit = Limit{
-		Interval: l.StatusLimit.Interval,
-		Burst:    l.StatusLimit.Burst,
-		Max:      l.StatusLimit.Max,
-		MaxBody:  l.StatusLimit.MaxBody,
+	if result.MaxBody == 0 {
+		result.MaxBody = l.MaxBody
 	}
+	return result
 }