From 3b5e72913eb97cb851c8f640ce7b645c93c394cb Mon Sep 17 00:00:00 2001
From: Pierre Souchay <pierresouchay@users.noreply.github.com>
Date: Thu, 2 Apr 2020 09:22:17 +0200
Subject: [PATCH] config: validate system limits against
 limits.http_max_conns_per_client (#7434)

I spent some time today on my local Mac to figure out why Consul 1.6.3+
was not accepting limits.http_max_conns_per_client.

This adds an explicit check on number of file descriptors to be sure
it might work (this is no guarantee as if many clients are reaching
the agent, it might consume even more file descriptors)

Anyway, many users are fighting with RLIMIT_NOFILE, having a clear
message would allow them to figure out what to fix.

Example of message (reload or start):

```
2020-03-11T16:38:37.062+0100 [ERROR] agent: Error starting agent: error="system allows a max of 512 file descriptors, but limits.http_max_conns_per_client: 8192 needs at least 8212"
```
---
 agent/config/agent_limits.go      | 20 ++++++++++++++
 agent/config/agent_limits_test.go | 43 +++++++++++++++++++++++++++++++
 agent/config/builder.go           |  4 +++
 agent/config/limits.go            | 13 ++++++++++
 agent/config/limits_windows.go    |  9 +++++++
 5 files changed, 89 insertions(+)
 create mode 100644 agent/config/agent_limits.go
 create mode 100644 agent/config/agent_limits_test.go
 create mode 100644 agent/config/limits.go
 create mode 100644 agent/config/limits_windows.go

diff --git a/agent/config/agent_limits.go b/agent/config/agent_limits.go
new file mode 100644
index 000000000..71924f7e7
--- /dev/null
+++ b/agent/config/agent_limits.go
@@ -0,0 +1,20 @@
+package config
+
+import (
+	"fmt"
+)
+
+// checkLimitsFromMaxConnsPerClient check that value provided might be OK
+// return an error if values are not compatible
+func checkLimitsFromMaxConnsPerClient(maxConnsPerClient int) error {
+	maxFds, err := getrlimit()
+	if err == nil && maxConnsPerClient > 0 {
+		// We need the list port + a few at the minimum
+		// On Mac OS, 20 FDs are open by Consul without doing anything
+		requiredFds := uint64(maxConnsPerClient + 20)
+		if maxFds < requiredFds {
+			return fmt.Errorf("system allows a max of %d file descriptors, but limits.http_max_conns_per_client: %d needs at least %d", maxFds, maxConnsPerClient, requiredFds)
+		}
+	}
+	return err
+}
diff --git a/agent/config/agent_limits_test.go b/agent/config/agent_limits_test.go
new file mode 100644
index 000000000..ee87306b4
--- /dev/null
+++ b/agent/config/agent_limits_test.go
@@ -0,0 +1,43 @@
+package config
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestBuildAndValidate_HTTPMaxConnsPerClientExceedsRLimit(t *testing.T) {
+	t.Parallel()
+	hcl := `
+		limits{
+			# We put a very high value to be sure to fail
+			# This value is more than max on Windows as well
+			http_max_conns_per_client = 16777217
+		}`
+	b, err := NewBuilder(Flags{})
+	assert.NoError(t, err)
+	testsrc := Source{
+		Name:   "test",
+		Format: "hcl",
+		Data: `
+		    ae_interval = "1m"
+		    data_dir="/tmp/00000000001979"
+			bind_addr = "127.0.0.1"
+			advertise_addr = "127.0.0.1"
+			datacenter = "dc1"
+			bootstrap = true
+			server = true
+			node_id = "00000000001979"
+			node_name = "Node-00000000001979"
+		`,
+	}
+	b.Head = append(b.Head, testsrc)
+	b.Tail = append(b.Tail, DefaultConsulSource(), DevConsulSource())
+	b.Tail = append(b.Head, Source{Name: "hcl", Format: "hcl", Data: hcl})
+
+	_, validationError := b.BuildAndValidate()
+	if validationError == nil {
+		assert.Fail(t, "Config should not be valid")
+	}
+	assert.Contains(t, validationError.Error(), "but limits.http_max_conns_per_client: 16777217 needs at least 16777237")
+}
diff --git a/agent/config/builder.go b/agent/config/builder.go
index 04692ee27..48d18b272 100644
--- a/agent/config/builder.go
+++ b/agent/config/builder.go
@@ -1245,6 +1245,10 @@ func (b *Builder) Validate(rt RuntimeConfig) error {
 		}
 	}
 
+	if err := checkLimitsFromMaxConnsPerClient(rt.HTTPMaxConnsPerClient); err != nil {
+		return err
+	}
+
 	return nil
 }
 
diff --git a/agent/config/limits.go b/agent/config/limits.go
new file mode 100644
index 000000000..521052763
--- /dev/null
+++ b/agent/config/limits.go
@@ -0,0 +1,13 @@
+// +build !windows
+
+package config
+
+import "golang.org/x/sys/unix"
+
+// getrlimit return the max file descriptors allocated by system
+// return the number of file descriptors max
+func getrlimit() (uint64, error) {
+	var limit unix.Rlimit
+	err := unix.Getrlimit(unix.RLIMIT_NOFILE, &limit)
+	return uint64(limit.Cur), err
+}
diff --git a/agent/config/limits_windows.go b/agent/config/limits_windows.go
new file mode 100644
index 000000000..6cd8817b2
--- /dev/null
+++ b/agent/config/limits_windows.go
@@ -0,0 +1,9 @@
+// +build windows
+
+package config
+
+// getrlimit is no-op on Windows, as max fd/process is 2^24 on Wow64
+// Return (16 777 216, nil)
+func getrlimit() (uint64, error) {
+	return 16_777_216, nil
+}