From 39b06b3b51229c86cf0ad19e101990d68b76834e Mon Sep 17 00:00:00 2001 From: Pierre Souchay Date: Wed, 11 Mar 2020 16:32:03 +0100 Subject: [PATCH 1/9] Validate RLIMIT_NOFILE against limits.http_max_conns_per_client 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/agent.go | 22 ++++++++++++++++++++++ agent/limits.go | 13 +++++++++++++ agent/limits_windows.go | 9 +++++++++ 3 files changed, 44 insertions(+) create mode 100644 agent/limits.go create mode 100644 agent/limits_windows.go diff --git a/agent/agent.go b/agent/agent.go index 6940064e1c84..bda7af1e7f4c 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -537,6 +537,9 @@ func (a *Agent) Start() error { a.httpConnLimiter.SetConfig(connlimit.Config{ MaxConnsPerClientIP: a.config.HTTPMaxConnsPerClient, }) + if err := a.CheckLimitsFromMaxConnsPerClient(a.config.HTTPMaxConnsPerClient); err != nil { + return err + } // Create listeners and unstarted servers; see comment on listenHTTP why // we are doing this. @@ -4015,6 +4018,21 @@ func (a *Agent) loadLimits(conf *config.RuntimeConfig) { a.config.RPCMaxBurst = conf.RPCMaxBurst } +// CheckLimitsFromMaxConnsPerClient check that value provided might be OK +// return an error if values are not compatible +func (a *Agent) 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 +} + func (a *Agent) ReloadConfig(newCfg *config.RuntimeConfig) error { // Bulk update the services and checks a.PauseSync() @@ -4068,6 +4086,10 @@ func (a *Agent) ReloadConfig(newCfg *config.RuntimeConfig) error { MaxConnsPerClientIP: newCfg.HTTPMaxConnsPerClient, }) + if err := a.CheckLimitsFromMaxConnsPerClient(newCfg.HTTPMaxConnsPerClient); err != nil { + return err + } + for _, s := range a.dnsServers { if err := s.ReloadConfig(newCfg); err != nil { return fmt.Errorf("Failed reloading dns config : %v", err) diff --git a/agent/limits.go b/agent/limits.go new file mode 100644 index 000000000000..f45ac73dbe95 --- /dev/null +++ b/agent/limits.go @@ -0,0 +1,13 @@ +// +build !windows + +package agent + +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/limits_windows.go b/agent/limits_windows.go new file mode 100644 index 000000000000..2ed29ffecfea --- /dev/null +++ b/agent/limits_windows.go @@ -0,0 +1,9 @@ +// +build windows + +package agent + +// 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 +} From d1b33a575060b5399d103bc92f0e8db83a3edbea Mon Sep 17 00:00:00 2001 From: Pierre Souchay Date: Wed, 11 Mar 2020 17:31:08 +0100 Subject: [PATCH 2/9] Added unit test for `GetRLimits()` --- agent/agent_test.go | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/agent/agent_test.go b/agent/agent_test.go index 89b6d5a38726..0810cced06b0 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -3418,6 +3418,48 @@ func TestAgent_ReloadConfigOutgoingRPCConfig(t *testing.T) { require.Len(t, tlsConf.ClientCAs.Subjects(), 2) } +func TestAgent_GetRLimits(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 + }` + a := TestAgent{Name: t.Name(), HCL: hcl} + a.LogOutput = testutil.TestWriter(t) + if err := a.Start(); err == nil { + defer a.Shutdown() + assert.Fail(t, "a.Start() should have failed as limits.http_max_conns_per_client is too big") + } +} + +func TestAgent_GetRLimitsReload(t *testing.T) { + t.Parallel() + dataDir := testutil.TempDir(t, "agent") // we manage the data dir + defer os.RemoveAll(dataDir) + hcl := ` + data_dir = "` + dataDir + `" + limits{ + # Should be fine + http_max_conns_per_client = 22 + }` + a := NewTestAgent(t, t.Name(), hcl) + defer a.Shutdown() + + hclFailing := ` + data_dir = "` + dataDir + `" + 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 + }` + c := TestConfig(testutil.Logger(t), config.Source{Name: t.Name(), Format: "hcl", Data: hclFailing}) + if err := a.ReloadConfig(c); err == nil { + assert.Fail(t, "a.ReloadConfig() should have failed as limits.http_max_conns_per_client is too big") + } +} + func TestAgent_ReloadConfigAndKeepChecksStatus(t *testing.T) { t.Parallel() dataDir := testutil.TempDir(t, "agent") // we manage the data dir From 8593ca73dcfbccd9b5d806a1ef6991e5071d4308 Mon Sep 17 00:00:00 2001 From: Pierre Souchay Date: Wed, 25 Mar 2020 18:01:54 +0100 Subject: [PATCH 3/9] Now enforce at validation time to allow consul validate to fail --- agent/agent.go | 19 ++----------------- agent/config/builder.go | 4 ++++ lib/agent_limits.go | 18 ++++++++++++++++++ {agent => lib}/limits.go | 2 +- {agent => lib}/limits_windows.go | 2 +- 5 files changed, 26 insertions(+), 19 deletions(-) create mode 100644 lib/agent_limits.go rename {agent => lib}/limits.go (95%) rename {agent => lib}/limits_windows.go (92%) diff --git a/agent/agent.go b/agent/agent.go index bda7af1e7f4c..305ae5ddb9fd 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -537,7 +537,7 @@ func (a *Agent) Start() error { a.httpConnLimiter.SetConfig(connlimit.Config{ MaxConnsPerClientIP: a.config.HTTPMaxConnsPerClient, }) - if err := a.CheckLimitsFromMaxConnsPerClient(a.config.HTTPMaxConnsPerClient); err != nil { + if err := lib.CheckLimitsFromMaxConnsPerClient(a.config.HTTPMaxConnsPerClient); err != nil { return err } @@ -4018,21 +4018,6 @@ func (a *Agent) loadLimits(conf *config.RuntimeConfig) { a.config.RPCMaxBurst = conf.RPCMaxBurst } -// CheckLimitsFromMaxConnsPerClient check that value provided might be OK -// return an error if values are not compatible -func (a *Agent) 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 -} - func (a *Agent) ReloadConfig(newCfg *config.RuntimeConfig) error { // Bulk update the services and checks a.PauseSync() @@ -4086,7 +4071,7 @@ func (a *Agent) ReloadConfig(newCfg *config.RuntimeConfig) error { MaxConnsPerClientIP: newCfg.HTTPMaxConnsPerClient, }) - if err := a.CheckLimitsFromMaxConnsPerClient(newCfg.HTTPMaxConnsPerClient); err != nil { + if err := lib.CheckLimitsFromMaxConnsPerClient(newCfg.HTTPMaxConnsPerClient); err != nil { return err } diff --git a/agent/config/builder.go b/agent/config/builder.go index 04692ee2777e..58fa72340573 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -1001,6 +1001,10 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) { b.warn(`BootstrapExpect is set to 1; this is the same as Bootstrap mode.`) } + if err := lib.CheckLimitsFromMaxConnsPerClient(rt.HTTPMaxConnsPerClient); err != nil { + return rt, err + } + return rt, nil } diff --git a/lib/agent_limits.go b/lib/agent_limits.go new file mode 100644 index 000000000000..e6bf7a963d77 --- /dev/null +++ b/lib/agent_limits.go @@ -0,0 +1,18 @@ +package lib + +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/limits.go b/lib/limits.go similarity index 95% rename from agent/limits.go rename to lib/limits.go index f45ac73dbe95..41890a7d099e 100644 --- a/agent/limits.go +++ b/lib/limits.go @@ -1,6 +1,6 @@ // +build !windows -package agent +package lib import "golang.org/x/sys/unix" diff --git a/agent/limits_windows.go b/lib/limits_windows.go similarity index 92% rename from agent/limits_windows.go rename to lib/limits_windows.go index 2ed29ffecfea..a4ea707517bf 100644 --- a/agent/limits_windows.go +++ b/lib/limits_windows.go @@ -1,6 +1,6 @@ // +build windows -package agent +package lib // Getrlimit is no-op on Windows, as max fd/process is 2^24 on Wow64 // Return (16 777 216, nil) From 1902584a900f27bf839971f0c48ff013f29c2796 Mon Sep 17 00:00:00 2001 From: Pierre Souchay Date: Wed, 25 Mar 2020 19:27:05 +0100 Subject: [PATCH 4/9] Updated unit test to take into account validation failing early --- agent/agent_test.go | 11 ++++++++++- agent/config/builder.go | 8 ++++---- agent/testagent.go | 26 +++++++++++++++++--------- 3 files changed, 31 insertions(+), 14 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index 0810cced06b0..af88380f33d4 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -3454,7 +3454,16 @@ func TestAgent_GetRLimitsReload(t *testing.T) { # This value is more than max on Windows as well http_max_conns_per_client = 16777217 }` - c := TestConfig(testutil.Logger(t), config.Source{Name: t.Name(), Format: "hcl", Data: hclFailing}) + c, _, validationError := TestConfigWithErr(testutil.Logger(t), config.Source{Name: t.Name(), Format: "hcl", Data: hclFailing}) + if validationError == nil { + assert.Fail(t, "Config should not be valid") + } + // Value has not been taken into account because validation did fail + assert.Equal(t, 0, c.HTTPMaxConnsPerClient) + // Force updating entry + c.HTTPMaxConnsPerClient = 16777217 + assert.Equal(t, 16777217, c.HTTPMaxConnsPerClient) + // Still try to load config even if not valid if err := a.ReloadConfig(c); err == nil { assert.Fail(t, "a.ReloadConfig() should have failed as limits.http_max_conns_per_client is too big") } diff --git a/agent/config/builder.go b/agent/config/builder.go index 58fa72340573..b6a04325bb6c 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -1001,10 +1001,6 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) { b.warn(`BootstrapExpect is set to 1; this is the same as Bootstrap mode.`) } - if err := lib.CheckLimitsFromMaxConnsPerClient(rt.HTTPMaxConnsPerClient); err != nil { - return rt, err - } - return rt, nil } @@ -1249,6 +1245,10 @@ func (b *Builder) Validate(rt RuntimeConfig) error { } } + if err := lib.CheckLimitsFromMaxConnsPerClient(rt.HTTPMaxConnsPerClient); err != nil { + return err + } + return nil } diff --git a/agent/testagent.go b/agent/testagent.go index 28cac61ca553..48a0a730eae4 100644 --- a/agent/testagent.go +++ b/agent/testagent.go @@ -409,9 +409,8 @@ func NodeID() string { return id } -// TestConfig returns a unique default configuration for testing an -// agent. -func TestConfig(logger hclog.Logger, sources ...config.Source) *config.RuntimeConfig { +// TestConfigWithErr returns a unique default configuration for testing an agent. +func TestConfigWithErr(logger hclog.Logger, sources ...config.Source) (*config.RuntimeConfig, *config.Builder, error) { nodeID := NodeID() testsrc := config.Source{ Name: "test", @@ -446,11 +445,7 @@ func TestConfig(logger hclog.Logger, sources ...config.Source) *config.RuntimeCo cfg, err := b.BuildAndValidate() if err != nil { - panic("Error building config: " + err.Error()) - } - - for _, w := range b.Warnings { - logger.Warn(w) + return &cfg, b, err } // Effectively disables the delay after root rotation before requesting CSRs @@ -458,7 +453,20 @@ func TestConfig(logger hclog.Logger, sources ...config.Source) *config.RuntimeCo // tiny delay is effectively thre same. cfg.ConnectTestCALeafRootChangeSpread = 1 * time.Nanosecond - return &cfg + return &cfg, b, nil +} + +// TestConfig returns a unique default configuration for testing an +// agent. +func TestConfig(logger hclog.Logger, sources ...config.Source) *config.RuntimeConfig { + cfg, b, err := TestConfigWithErr(logger, sources...) + if err != nil { + panic("Error building config: " + err.Error()) + } + for _, w := range b.Warnings { + logger.Warn(w) + } + return cfg } // TestACLConfig returns a default configuration for testing an agent From 995f7b951587b6f6321c7f2602c1b34ab907bf79 Mon Sep 17 00:00:00 2001 From: Pierre Souchay Date: Thu, 26 Mar 2020 21:42:18 +0100 Subject: [PATCH 5/9] Since validation fails now, fix unit test `TestAgent_GetRLimits()`. --- agent/agent_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index af88380f33d4..6e10ef0fa355 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -3426,11 +3426,9 @@ func TestAgent_GetRLimits(t *testing.T) { # This value is more than max on Windows as well http_max_conns_per_client = 16777217 }` - a := TestAgent{Name: t.Name(), HCL: hcl} - a.LogOutput = testutil.TestWriter(t) - if err := a.Start(); err == nil { - defer a.Shutdown() - assert.Fail(t, "a.Start() should have failed as limits.http_max_conns_per_client is too big") + _, _, validationError := TestConfigWithErr(testutil.Logger(t), config.Source{Name: t.Name(), Format: "hcl", Data: hcl}) + if validationError == nil { + assert.Fail(t, "Config should not be valid") } } From d609c06518814c1f01e5a745e01eaf51983b81a1 Mon Sep 17 00:00:00 2001 From: Pierre Souchay Date: Wed, 1 Apr 2020 17:43:35 +0200 Subject: [PATCH 6/9] Removed duplicate test + only validate RLimit() while parsing configuration --- agent/agent.go | 7 ------ agent/agent_test.go | 35 --------------------------- {lib => agent/config}/agent_limits.go | 10 +++++--- agent/config/builder.go | 2 +- 4 files changed, 8 insertions(+), 46 deletions(-) rename {lib => agent/config}/agent_limits.go (85%) diff --git a/agent/agent.go b/agent/agent.go index 305ae5ddb9fd..6940064e1c84 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -537,9 +537,6 @@ func (a *Agent) Start() error { a.httpConnLimiter.SetConfig(connlimit.Config{ MaxConnsPerClientIP: a.config.HTTPMaxConnsPerClient, }) - if err := lib.CheckLimitsFromMaxConnsPerClient(a.config.HTTPMaxConnsPerClient); err != nil { - return err - } // Create listeners and unstarted servers; see comment on listenHTTP why // we are doing this. @@ -4071,10 +4068,6 @@ func (a *Agent) ReloadConfig(newCfg *config.RuntimeConfig) error { MaxConnsPerClientIP: newCfg.HTTPMaxConnsPerClient, }) - if err := lib.CheckLimitsFromMaxConnsPerClient(newCfg.HTTPMaxConnsPerClient); err != nil { - return err - } - for _, s := range a.dnsServers { if err := s.ReloadConfig(newCfg); err != nil { return fmt.Errorf("Failed reloading dns config : %v", err) diff --git a/agent/agent_test.go b/agent/agent_test.go index 6e10ef0fa355..29666a2eb450 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -3432,41 +3432,6 @@ func TestAgent_GetRLimits(t *testing.T) { } } -func TestAgent_GetRLimitsReload(t *testing.T) { - t.Parallel() - dataDir := testutil.TempDir(t, "agent") // we manage the data dir - defer os.RemoveAll(dataDir) - hcl := ` - data_dir = "` + dataDir + `" - limits{ - # Should be fine - http_max_conns_per_client = 22 - }` - a := NewTestAgent(t, t.Name(), hcl) - defer a.Shutdown() - - hclFailing := ` - data_dir = "` + dataDir + `" - 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 - }` - c, _, validationError := TestConfigWithErr(testutil.Logger(t), config.Source{Name: t.Name(), Format: "hcl", Data: hclFailing}) - if validationError == nil { - assert.Fail(t, "Config should not be valid") - } - // Value has not been taken into account because validation did fail - assert.Equal(t, 0, c.HTTPMaxConnsPerClient) - // Force updating entry - c.HTTPMaxConnsPerClient = 16777217 - assert.Equal(t, 16777217, c.HTTPMaxConnsPerClient) - // Still try to load config even if not valid - if err := a.ReloadConfig(c); err == nil { - assert.Fail(t, "a.ReloadConfig() should have failed as limits.http_max_conns_per_client is too big") - } -} - func TestAgent_ReloadConfigAndKeepChecksStatus(t *testing.T) { t.Parallel() dataDir := testutil.TempDir(t, "agent") // we manage the data dir diff --git a/lib/agent_limits.go b/agent/config/agent_limits.go similarity index 85% rename from lib/agent_limits.go rename to agent/config/agent_limits.go index e6bf7a963d77..3fd817e9a5ff 100644 --- a/lib/agent_limits.go +++ b/agent/config/agent_limits.go @@ -1,11 +1,15 @@ -package lib +package config -import "fmt" +import ( + "fmt" + + "github.com/hashicorp/consul/lib" +) // CheckLimitsFromMaxConnsPerClient check that value provided might be OK // return an error if values are not compatible func CheckLimitsFromMaxConnsPerClient(maxConnsPerClient int) error { - maxFds, err := Getrlimit() + maxFds, err := lib.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 diff --git a/agent/config/builder.go b/agent/config/builder.go index b6a04325bb6c..0fd83d025040 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -1245,7 +1245,7 @@ func (b *Builder) Validate(rt RuntimeConfig) error { } } - if err := lib.CheckLimitsFromMaxConnsPerClient(rt.HTTPMaxConnsPerClient); err != nil { + if err := CheckLimitsFromMaxConnsPerClient(rt.HTTPMaxConnsPerClient); err != nil { return err } From f6b83a881561ff8f8b61606839c3490a4828b5f6 Mon Sep 17 00:00:00 2001 From: Pierre Souchay Date: Wed, 1 Apr 2020 19:31:21 +0200 Subject: [PATCH 7/9] Refactored and moved everything into config package --- agent/agent_test.go | 14 ---------- agent/config/agent_limits.go | 4 +-- agent/config/agent_limits_test.go | 45 +++++++++++++++++++++++++++++++ agent/config/builder.go | 2 +- agent/testagent.go | 26 +++++++----------- 5 files changed, 57 insertions(+), 34 deletions(-) create mode 100644 agent/config/agent_limits_test.go diff --git a/agent/agent_test.go b/agent/agent_test.go index 29666a2eb450..89b6d5a38726 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -3418,20 +3418,6 @@ func TestAgent_ReloadConfigOutgoingRPCConfig(t *testing.T) { require.Len(t, tlsConf.ClientCAs.Subjects(), 2) } -func TestAgent_GetRLimits(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 - }` - _, _, validationError := TestConfigWithErr(testutil.Logger(t), config.Source{Name: t.Name(), Format: "hcl", Data: hcl}) - if validationError == nil { - assert.Fail(t, "Config should not be valid") - } -} - func TestAgent_ReloadConfigAndKeepChecksStatus(t *testing.T) { t.Parallel() dataDir := testutil.TempDir(t, "agent") // we manage the data dir diff --git a/agent/config/agent_limits.go b/agent/config/agent_limits.go index 3fd817e9a5ff..a6f3f642967f 100644 --- a/agent/config/agent_limits.go +++ b/agent/config/agent_limits.go @@ -6,9 +6,9 @@ import ( "github.com/hashicorp/consul/lib" ) -// CheckLimitsFromMaxConnsPerClient check that value provided might be OK +// checkLimitsFromMaxConnsPerClient check that value provided might be OK // return an error if values are not compatible -func CheckLimitsFromMaxConnsPerClient(maxConnsPerClient int) error { +func checkLimitsFromMaxConnsPerClient(maxConnsPerClient int) error { maxFds, err := lib.Getrlimit() if err == nil && maxConnsPerClient > 0 { // We need the list port + a few at the minimum diff --git a/agent/config/agent_limits_test.go b/agent/config/agent_limits_test.go new file mode 100644 index 000000000000..fdef81eafbf6 --- /dev/null +++ b/agent/config/agent_limits_test.go @@ -0,0 +1,45 @@ +// +build !consulent + +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 0fd83d025040..48d18b2724c4 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -1245,7 +1245,7 @@ func (b *Builder) Validate(rt RuntimeConfig) error { } } - if err := CheckLimitsFromMaxConnsPerClient(rt.HTTPMaxConnsPerClient); err != nil { + if err := checkLimitsFromMaxConnsPerClient(rt.HTTPMaxConnsPerClient); err != nil { return err } diff --git a/agent/testagent.go b/agent/testagent.go index 48a0a730eae4..28cac61ca553 100644 --- a/agent/testagent.go +++ b/agent/testagent.go @@ -409,8 +409,9 @@ func NodeID() string { return id } -// TestConfigWithErr returns a unique default configuration for testing an agent. -func TestConfigWithErr(logger hclog.Logger, sources ...config.Source) (*config.RuntimeConfig, *config.Builder, error) { +// TestConfig returns a unique default configuration for testing an +// agent. +func TestConfig(logger hclog.Logger, sources ...config.Source) *config.RuntimeConfig { nodeID := NodeID() testsrc := config.Source{ Name: "test", @@ -445,7 +446,11 @@ func TestConfigWithErr(logger hclog.Logger, sources ...config.Source) (*config.R cfg, err := b.BuildAndValidate() if err != nil { - return &cfg, b, err + panic("Error building config: " + err.Error()) + } + + for _, w := range b.Warnings { + logger.Warn(w) } // Effectively disables the delay after root rotation before requesting CSRs @@ -453,20 +458,7 @@ func TestConfigWithErr(logger hclog.Logger, sources ...config.Source) (*config.R // tiny delay is effectively thre same. cfg.ConnectTestCALeafRootChangeSpread = 1 * time.Nanosecond - return &cfg, b, nil -} - -// TestConfig returns a unique default configuration for testing an -// agent. -func TestConfig(logger hclog.Logger, sources ...config.Source) *config.RuntimeConfig { - cfg, b, err := TestConfigWithErr(logger, sources...) - if err != nil { - panic("Error building config: " + err.Error()) - } - for _, w := range b.Warnings { - logger.Warn(w) - } - return cfg + return &cfg } // TestACLConfig returns a default configuration for testing an agent From 765fd8a89813a356c0068f119db1e6d14ffe857d Mon Sep 17 00:00:00 2001 From: Pierre Souchay Date: Wed, 1 Apr 2020 22:18:01 +0200 Subject: [PATCH 8/9] Moved everything in package config --- agent/config/agent_limits.go | 4 +--- {lib => agent/config}/limits.go | 6 +++--- agent/config/limits_windows.go | 9 +++++++++ lib/limits_windows.go | 9 --------- 4 files changed, 13 insertions(+), 15 deletions(-) rename {lib => agent/config}/limits.go (64%) create mode 100644 agent/config/limits_windows.go delete mode 100644 lib/limits_windows.go diff --git a/agent/config/agent_limits.go b/agent/config/agent_limits.go index a6f3f642967f..71924f7e7b75 100644 --- a/agent/config/agent_limits.go +++ b/agent/config/agent_limits.go @@ -2,14 +2,12 @@ package config import ( "fmt" - - "github.com/hashicorp/consul/lib" ) // checkLimitsFromMaxConnsPerClient check that value provided might be OK // return an error if values are not compatible func checkLimitsFromMaxConnsPerClient(maxConnsPerClient int) error { - maxFds, err := lib.Getrlimit() + 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 diff --git a/lib/limits.go b/agent/config/limits.go similarity index 64% rename from lib/limits.go rename to agent/config/limits.go index 41890a7d099e..5210527632b5 100644 --- a/lib/limits.go +++ b/agent/config/limits.go @@ -1,12 +1,12 @@ // +build !windows -package lib +package config import "golang.org/x/sys/unix" -// Getrlimit return the max file descriptors allocated by system +// getrlimit return the max file descriptors allocated by system // return the number of file descriptors max -func Getrlimit() (uint64, error) { +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 000000000000..6cd8817b2b5d --- /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 +} diff --git a/lib/limits_windows.go b/lib/limits_windows.go deleted file mode 100644 index a4ea707517bf..000000000000 --- a/lib/limits_windows.go +++ /dev/null @@ -1,9 +0,0 @@ -// +build windows - -package lib - -// 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 -} From 1bd0553f604fdd718b4d6fc3ef7b3751f040bbaf Mon Sep 17 00:00:00 2001 From: Pierre Souchay Date: Wed, 1 Apr 2020 23:08:29 +0200 Subject: [PATCH 9/9] Removed wrong build directive in test. --- agent/config/agent_limits_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/agent/config/agent_limits_test.go b/agent/config/agent_limits_test.go index fdef81eafbf6..ee87306b4180 100644 --- a/agent/config/agent_limits_test.go +++ b/agent/config/agent_limits_test.go @@ -1,5 +1,3 @@ -// +build !consulent - package config import (