diff --git a/agent/acl_test.go b/agent/acl_test.go index e5d4df594f..7145fda341 100644 --- a/agent/acl_test.go +++ b/agent/acl_test.go @@ -162,7 +162,7 @@ func (a *TestACLAgent) Shutdown() error { func (a *TestACLAgent) Stats() map[string]map[string]string { return nil } -func (a *TestACLAgent) ReloadConfig(config *consul.Config) error { +func (a *TestACLAgent) ReloadConfig(_ consul.ReloadableConfig) error { return fmt.Errorf("Unimplemented") } diff --git a/agent/agent.go b/agent/agent.go index 1d48cd85de..e6b05a9cae 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -145,7 +145,7 @@ type delegate interface { SnapshotRPC(args *structs.SnapshotRequest, in io.Reader, out io.Writer, replyFn structs.SnapshotReplyFn) error Shutdown() error Stats() map[string]map[string]string - ReloadConfig(config *consul.Config) error + ReloadConfig(config consul.ReloadableConfig) error enterpriseDelegate } @@ -3517,11 +3517,6 @@ func (a *Agent) DisableNodeMaintenance() { a.logger.Info("Node left maintenance mode") } -func (a *Agent) loadLimits(conf *config.RuntimeConfig) { - a.config.RPCRateLimit = conf.RPCRateLimit - a.config.RPCMaxBurst = conf.RPCMaxBurst -} - // ReloadConfig will atomically reload all configuration, including // all services, checks, tokens, metadata, dnsServer configs, etc. // It will also reload all ongoing watches. @@ -3602,8 +3597,6 @@ func (a *Agent) reloadConfigInternal(newCfg *config.RuntimeConfig) error { return fmt.Errorf("Failed reloading watches: %v", err) } - a.loadLimits(newCfg) - a.httpConnLimiter.SetConfig(connlimit.Config{ MaxConnsPerClientIP: newCfg.HTTPMaxConnsPerClient, }) @@ -3614,24 +3607,18 @@ func (a *Agent) reloadConfigInternal(newCfg *config.RuntimeConfig) error { } } - // this only gets used by the consulConfig function and since - // that is only ever done during init and reload here then - // an in place modification is safe as reloads cannot be - // concurrent due to both gaining a full lock on the stateLock - a.config.ConfigEntryBootstrap = newCfg.ConfigEntryBootstrap - err := a.reloadEnterprise(newCfg) if err != nil { return err } - // create the config for the rpc server/client - consulCfg, err := newConsulConfig(a.config, a.logger) - if err != nil { - return err + cc := consul.ReloadableConfig{ + RPCRateLimit: newCfg.RPCRateLimit, + RPCMaxBurst: newCfg.RPCMaxBurst, + RPCMaxConnsPerClient: newCfg.RPCMaxConnsPerClient, + ConfigEntryBootstrap: newCfg.ConfigEntryBootstrap, } - - if err := a.delegate.ReloadConfig(consulCfg); err != nil { + if err := a.delegate.ReloadConfig(cc); err != nil { return err } diff --git a/agent/consul/client.go b/agent/consul/client.go index d2ae9a1edd..ee9147ddf8 100644 --- a/agent/consul/client.go +++ b/agent/consul/client.go @@ -403,7 +403,7 @@ func (c *Client) GetLANCoordinate() (lib.CoordinateSet, error) { // ReloadConfig is used to have the Client do an online reload of // relevant configuration information -func (c *Client) ReloadConfig(config *Config) error { - c.rpcLimiter.Store(rate.NewLimiter(config.RPCRate, config.RPCMaxBurst)) +func (c *Client) ReloadConfig(config ReloadableConfig) error { + c.rpcLimiter.Store(rate.NewLimiter(config.RPCRateLimit, config.RPCMaxBurst)) return nil } diff --git a/agent/consul/client_test.go b/agent/consul/client_test.go index a65c9f454e..01583daca2 100644 --- a/agent/consul/client_test.go +++ b/agent/consul/client_test.go @@ -9,6 +9,12 @@ import ( "testing" "time" + "github.com/hashicorp/go-hclog" + msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc" + "github.com/hashicorp/serf/serf" + "github.com/stretchr/testify/require" + "golang.org/x/time/rate" + "github.com/hashicorp/consul/agent/pool" "github.com/hashicorp/consul/agent/router" "github.com/hashicorp/consul/agent/structs" @@ -18,11 +24,6 @@ import ( "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/hashicorp/consul/testrpc" "github.com/hashicorp/consul/tlsutil" - "github.com/hashicorp/go-hclog" - msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc" - "github.com/hashicorp/serf/serf" - "github.com/stretchr/testify/require" - "golang.org/x/time/rate" ) func testClientConfig(t *testing.T) (string, *Config) { @@ -762,23 +763,25 @@ func TestClientServer_UserEvent(t *testing.T) { } } -func TestClient_Reload(t *testing.T) { - t.Parallel() - dir1, c := testClientWithConfig(t, func(c *Config) { - c.RPCRate = 500 - c.RPCMaxBurst = 5000 - }) - defer os.RemoveAll(dir1) - defer c.Shutdown() +func TestClient_ReloadConfig(t *testing.T) { + _, cfg := testClientConfig(t) + cfg.RPCRate = rate.Limit(500) + cfg.RPCMaxBurst = 5000 + deps := newDefaultDeps(t, &Config{NodeName: "node1", Datacenter: "dc1"}) + c, err := NewClient(cfg, deps) + require.NoError(t, err) limiter := c.rpcLimiter.Load().(*rate.Limiter) require.Equal(t, rate.Limit(500), limiter.Limit()) require.Equal(t, 5000, limiter.Burst()) - c.config.RPCRate = 1000 - c.config.RPCMaxBurst = 10000 + rc := ReloadableConfig{ + RPCRateLimit: 1000, + RPCMaxBurst: 10000, + RPCMaxConnsPerClient: 0, + } + require.NoError(t, c.ReloadConfig(rc)) - require.NoError(t, c.ReloadConfig(c.config)) limiter = c.rpcLimiter.Load().(*rate.Limiter) require.Equal(t, rate.Limit(1000), limiter.Limit()) require.Equal(t, 10000, limiter.Burst()) diff --git a/agent/consul/config.go b/agent/consul/config.go index 7b4cbb507e..dd6e7bca2f 100644 --- a/agent/consul/config.go +++ b/agent/consul/config.go @@ -655,3 +655,12 @@ func DefaultConfig() *Config { type RPCConfig struct { EnableStreaming bool } + +// ReloadableConfig is the configuration that is passed to ReloadConfig when +// application config is reloaded. +type ReloadableConfig struct { + RPCRateLimit rate.Limit + RPCMaxBurst int + RPCMaxConnsPerClient int + ConfigEntryBootstrap []structs.ConfigEntry +} diff --git a/agent/consul/rpc_test.go b/agent/consul/rpc_test.go index cd80ddfa73..64cba910e9 100644 --- a/agent/consul/rpc_test.go +++ b/agent/consul/rpc_test.go @@ -729,9 +729,12 @@ func TestRPC_RPCMaxConnsPerClient(t *testing.T) { defer conn4.Close() // Reload config with higher limit - newCfg := *s1.config - newCfg.RPCMaxConnsPerClient = 10 - require.NoError(t, s1.ReloadConfig(&newCfg)) + rc := ReloadableConfig{ + RPCRateLimit: s1.config.RPCRate, + RPCMaxBurst: s1.config.RPCMaxBurst, + RPCMaxConnsPerClient: 10, + } + require.NoError(t, s1.ReloadConfig(rc)) // Now another conn should be allowed conn5 := connectClient(t, s1, tc.magicByte, tc.tlsEnabled, true, "conn5") diff --git a/agent/consul/server.go b/agent/consul/server.go index 7b01bce674..15c47ec97a 100644 --- a/agent/consul/server.go +++ b/agent/consul/server.go @@ -1384,8 +1384,8 @@ func (s *Server) GetLANCoordinate() (lib.CoordinateSet, error) { // ReloadConfig is used to have the Server do an online reload of // relevant configuration information -func (s *Server) ReloadConfig(config *Config) error { - s.rpcLimiter.Store(rate.NewLimiter(config.RPCRate, config.RPCMaxBurst)) +func (s *Server) ReloadConfig(config ReloadableConfig) error { + s.rpcLimiter.Store(rate.NewLimiter(config.RPCRateLimit, config.RPCMaxBurst)) s.rpcConnLimiter.SetConfig(connlimit.Config{ MaxConnsPerClientIP: config.RPCMaxConnsPerClient, }) diff --git a/agent/consul/server_test.go b/agent/consul/server_test.go index 4b862b23e0..d31db5f3c5 100644 --- a/agent/consul/server_test.go +++ b/agent/consul/server_test.go @@ -16,9 +16,13 @@ import ( "time" "github.com/google/tcpproxy" + "github.com/hashicorp/memberlist" + "github.com/hashicorp/consul/agent/connect/ca" "github.com/hashicorp/consul/ipaddr" - "github.com/hashicorp/memberlist" + + "github.com/hashicorp/go-uuid" + "golang.org/x/time/rate" "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/agent/metadata" @@ -30,8 +34,6 @@ import ( "github.com/hashicorp/consul/testrpc" "github.com/hashicorp/consul/tlsutil" "github.com/hashicorp/consul/types" - "github.com/hashicorp/go-uuid" - "golang.org/x/time/rate" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -1458,14 +1460,13 @@ func TestServer_RevokeLeadershipIdempotent(t *testing.T) { s1.revokeLeadership() } -func TestServer_Reload(t *testing.T) { +func TestServer_ReloadConfig(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") } - t.Parallel() - global_entry_init := &structs.ProxyConfigEntry{ + entryInit := &structs.ProxyConfigEntry{ Kind: structs.ProxyDefaults, Name: structs.ProxyConfigGlobal, Config: map[string]interface{}{ @@ -1486,28 +1487,25 @@ func TestServer_Reload(t *testing.T) { testrpc.WaitForTestAgent(t, s.RPC, "dc1") - s.config.ConfigEntryBootstrap = []structs.ConfigEntry{ - global_entry_init, - } - limiter := s.rpcLimiter.Load().(*rate.Limiter) require.Equal(t, rate.Limit(500), limiter.Limit()) require.Equal(t, 5000, limiter.Burst()) - // Change rate limit - s.config.RPCRate = 1000 - s.config.RPCMaxBurst = 10000 - - s.ReloadConfig(s.config) + rc := ReloadableConfig{ + RPCRateLimit: 1000, + RPCMaxBurst: 10000, + ConfigEntryBootstrap: []structs.ConfigEntry{entryInit}, + } + require.NoError(t, s.ReloadConfig(rc)) _, entry, err := s.fsm.State().ConfigEntry(nil, structs.ProxyDefaults, structs.ProxyConfigGlobal, structs.DefaultEnterpriseMeta()) require.NoError(t, err) require.NotNil(t, entry) global, ok := entry.(*structs.ProxyConfigEntry) require.True(t, ok) - require.Equal(t, global_entry_init.Kind, global.Kind) - require.Equal(t, global_entry_init.Name, global.Name) - require.Equal(t, global_entry_init.Config, global.Config) + require.Equal(t, entryInit.Kind, global.Kind) + require.Equal(t, entryInit.Name, global.Name) + require.Equal(t, entryInit.Config, global.Config) // Check rate limiter got updated limiter = s.rpcLimiter.Load().(*rate.Limiter)