From 5acf01ceeb88d83d67b3939d664e99731202a406 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 29 Jul 2020 16:05:51 -0400 Subject: [PATCH] Remove LogOutput from Server --- agent/agent.go | 3 --- agent/consul/client_test.go | 6 ++--- agent/consul/config.go | 5 ---- agent/consul/leader_test.go | 50 +++++++++++++++++++++++++------------ agent/consul/server.go | 37 ++------------------------- agent/consul/server_test.go | 33 ++++++++++++------------ 6 files changed, 56 insertions(+), 78 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 0553628985..f724ae1bc6 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1517,9 +1517,6 @@ func (a *Agent) consulConfig() (*consul.Config, error) { } } - // Setup the loggers - base.LogOutput = a.LogOutput - // This will set up the LAN keyring, as well as the WAN and any segments // for servers. if err := a.setupKeyrings(base); err != nil { diff --git a/agent/consul/client_test.go b/agent/consul/client_test.go index 3328c3404a..4ec005ecd3 100644 --- a/agent/consul/client_test.go +++ b/agent/consul/client_test.go @@ -438,7 +438,7 @@ func TestClient_RPC_TLS(t *testing.T) { conf1.VerifyIncoming = true conf1.VerifyOutgoing = true configureTLS(conf1) - s1, err := NewServer(conf1) + s1, err := newServer(t, conf1) if err != nil { t.Fatalf("err: %v", err) } @@ -494,7 +494,7 @@ func newClient(t *testing.T, config *Config) (*Client, error) { func TestClient_RPC_RateLimit(t *testing.T) { t.Parallel() dir1, conf1 := testServerConfig(t) - s1, err := NewServer(conf1) + s1, err := newServer(t, conf1) if err != nil { t.Fatalf("err: %v", err) } @@ -606,7 +606,7 @@ func TestClient_SnapshotRPC_TLS(t *testing.T) { conf1.VerifyIncoming = true conf1.VerifyOutgoing = true configureTLS(conf1) - s1, err := NewServer(conf1) + s1, err := newServer(t, conf1) if err != nil { t.Fatalf("err: %v", err) } diff --git a/agent/consul/config.go b/agent/consul/config.go index 65820822db..ebfca98249 100644 --- a/agent/consul/config.go +++ b/agent/consul/config.go @@ -2,7 +2,6 @@ package consul import ( "fmt" - "io" "net" "os" "time" @@ -161,10 +160,6 @@ type Config struct { // leader election. ReconcileInterval time.Duration - // LogOutput is the location to write logs to. If this is not set, - // logs will go to stderr. - LogOutput io.Writer - // ProtocolVersion is the protocol version to speak. This must be between // ProtocolVersionMin and ProtocolVersionMax. ProtocolVersion uint8 diff --git a/agent/consul/leader_test.go b/agent/consul/leader_test.go index 365e50ec25..9c632a5eab 100644 --- a/agent/consul/leader_test.go +++ b/agent/consul/leader_test.go @@ -10,10 +10,13 @@ import ( "time" "github.com/hashicorp/consul/agent/structs" + "github.com/hashicorp/consul/agent/token" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/sdk/testutil" "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" @@ -1283,24 +1286,39 @@ func TestLeader_ConfigEntryBootstrap_Fail(t *testing.T) { } }() - dir1, s1 := testServerWithConfig(t, func(c *Config) { - c.LogOutput = io.MultiWriter(pw, testutil.NewLogBuffer(t)) - c.Build = "1.6.0" - c.ConfigEntryBootstrap = []structs.ConfigEntry{ - &structs.ServiceSplitterConfigEntry{ - Kind: structs.ServiceSplitter, - Name: "web", - Splits: []structs.ServiceSplit{ - {Weight: 100, Service: "web"}, - }, + dir, config := testServerConfig(t) + defer os.RemoveAll(dir) + config.Build = "1.6.0" + config.ConfigEntryBootstrap = []structs.ConfigEntry{ + &structs.ServiceSplitterConfigEntry{ + Kind: structs.ServiceSplitter, + Name: "web", + Splits: []structs.ServiceSplit{ + {Weight: 100, Service: "web"}, }, - } - }) - defer os.RemoveAll(dir1) - defer s1.Shutdown() + }, + } - result := <-ch - require.Empty(t, result) + logger := hclog.NewInterceptLogger(&hclog.LoggerOptions{ + Name: config.NodeName, + Level: hclog.Debug, + Output: io.MultiWriter(pw, testutil.NewLogBuffer(t)), + }) + tlsConf, err := tlsutil.NewConfigurator(config.ToTLSUtilConfig(), logger) + require.NoError(t, err) + srv, err := NewServerWithOptions(config, + WithLogger(logger), + WithTokenStore(new(token.Store)), + WithTLSConfigurator(tlsConf)) + require.NoError(t, err) + defer srv.Shutdown() + + select { + case result := <-ch: + require.Empty(t, result) + case <-time.After(time.Second): + t.Fatal("timeout waiting for a result from tailing logs") + } } func TestLeader_ACLLegacyReplication(t *testing.T) { diff --git a/agent/consul/server.go b/agent/consul/server.go index cc472e21c5..f878cf7692 100644 --- a/agent/consul/server.go +++ b/agent/consul/server.go @@ -313,25 +313,6 @@ type Server struct { EnterpriseServer } -// NewServer is only used to help setting up a server for testing. Normal code -// exercises NewServerLogger. -func NewServer(config *Config) (*Server, error) { - c, err := tlsutil.NewConfigurator(config.ToTLSUtilConfig(), nil) - if err != nil { - return nil, err - } - return NewServerLogger(config, nil, new(token.Store), c) -} - -// NewServerLogger is used to construct a new Consul server from the -// configuration, potentially returning an error -func NewServerLogger(config *Config, logger hclog.InterceptLogger, tokens *token.Store, tlsConfigurator *tlsutil.Configurator) (*Server, error) { - return NewServerWithOptions(config, - WithLogger(logger), - WithTokenStore(tokens), - WithTLSConfigurator(tlsConfigurator)) -} - // NewServerWithOptions is used to construct a new Consul server from the configuration // and extra options, potentially returning an error func NewServerWithOptions(config *Config, options ...ConsulOption) (*Server, error) { @@ -342,31 +323,17 @@ func NewServerWithOptions(config *Config, options ...ConsulOption) (*Server, err tlsConfigurator := flat.tlsConfigurator connPool := flat.connPool - // Check the protocol version. if err := config.CheckProtocolVersion(); err != nil { return nil, err } - - // Check for a data directory. if config.DataDir == "" && !config.DevMode { return nil, fmt.Errorf("Config must provide a DataDir") } - - // Sanity check the ACLs. if err := config.CheckACL(); err != nil { return nil, err } - - // Ensure we have a log output and create a logger. - if config.LogOutput == nil { - config.LogOutput = os.Stderr - } - if logger == nil { - logger = hclog.NewInterceptLogger(&hclog.LoggerOptions{ - Level: hclog.Debug, - Output: config.LogOutput, - }) + return nil, fmt.Errorf("logger is required") } // Check if TLS is enabled @@ -735,7 +702,7 @@ func (s *Server) setupRaft() error { log = cacheStore // Create the snapshot store. - snapshots, err := raft.NewFileSnapshotStore(path, snapshotsRetained, s.config.LogOutput) + snapshots, err := raft.NewFileSnapshotStoreWithLogger(path, snapshotsRetained, s.logger.Named("snapshot")) if err != nil { return err } diff --git a/agent/consul/server_test.go b/agent/consul/server_test.go index 99632a499c..18d61618f0 100644 --- a/agent/consul/server_test.go +++ b/agent/consul/server_test.go @@ -146,7 +146,6 @@ func testServerConfig(t *testing.T) (string, *Config) { config.Bootstrap = true config.Datacenter = "dc1" config.DataDir = dir - config.LogOutput = testutil.NewLogBuffer(t) // bind the rpc server to a random port. config.RPCAdvertise will be // set to the listen address unless it was set in the configuration. @@ -258,18 +257,18 @@ func testServerDCExpectNonVoter(t *testing.T, dc string, expect int) (string, *S func testServerWithConfig(t *testing.T, cb func(*Config)) (string, *Server) { var dir string - var config *Config var srv *Server - var err error // Retry added to avoid cases where bind addr is already in use retry.RunWith(retry.ThreeTimes(), t, func(r *retry.R) { + var config *Config dir, config = testServerConfig(t) if cb != nil { cb(config) } - srv, err = newServer(config) + var err error + srv, err = newServer(t, config) if err != nil { config.NotifyShutdown() os.RemoveAll(dir) @@ -295,7 +294,7 @@ func testACLServerWithConfig(t *testing.T, cb func(*Config), initReplicationToke return dir, srv, codec } -func newServer(c *Config) (*Server, error) { +func newServer(t *testing.T, c *Config) (*Server, error) { // chain server up notification oldNotify := c.NotifyListen up := make(chan struct{}) @@ -306,21 +305,19 @@ func newServer(c *Config) (*Server, error) { } } - // start server - w := c.LogOutput - if w == nil { - w = os.Stderr - } logger := hclog.NewInterceptLogger(&hclog.LoggerOptions{ Name: c.NodeName, Level: hclog.Debug, - Output: w, + Output: testutil.NewLogBuffer(t), }) tlsConf, err := tlsutil.NewConfigurator(c.ToTLSUtilConfig(), logger) if err != nil { return nil, err } - srv, err := NewServerLogger(c, logger, new(token.Store), tlsConf) + srv, err := NewServer(c, + WithLogger(logger), + WithTokenStore(new(token.Store)), + WithTLSConfigurator(tlsConf)) if err != nil { return nil, err } @@ -1088,7 +1085,7 @@ func TestServer_JoinLAN_TLS(t *testing.T) { conf1.VerifyIncoming = true conf1.VerifyOutgoing = true configureTLS(conf1) - s1, err := newServer(conf1) + s1, err := newServer(t, conf1) if err != nil { t.Fatalf("err: %v", err) } @@ -1101,7 +1098,7 @@ func TestServer_JoinLAN_TLS(t *testing.T) { conf2.VerifyIncoming = true conf2.VerifyOutgoing = true configureTLS(conf2) - s2, err := newServer(conf2) + s2, err := newServer(t, conf2) if err != nil { t.Fatalf("err: %v", err) } @@ -1486,7 +1483,7 @@ func TestServer_RPC_RateLimit(t *testing.T) { dir1, conf1 := testServerConfig(t) conf1.RPCRate = 2 conf1.RPCMaxBurst = 2 - s1, err := NewServer(conf1) + s1, err := newServer(t, conf1) if err != nil { t.Fatalf("err: %v", err) } @@ -1512,7 +1509,11 @@ func TestServer_CALogging(t *testing.T) { c, err := tlsutil.NewConfigurator(conf1.ToTLSUtilConfig(), logger) require.NoError(t, err) - s1, err := NewServerLogger(conf1, logger, new(token.Store), c) + + s1, err := NewServer(conf1, + WithLogger(logger), + WithTokenStore(new(token.Store)), + WithTLSConfigurator(c)) if err != nil { t.Fatalf("err: %v", err) }