From 0420d91cdd48306ba633cb8b83887dfe19baa905 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 29 Jul 2020 16:18:54 -0400 Subject: [PATCH] Remove LogOutput from Agent Now that it is no longer used, we can remove this unnecessary field. This is a pre-step in cleanup up RuntimeConfig->Consul.Config, which is a pre-step to adding a gRPCHandler component to Server for streaming. Removing this field also allows us to remove one of the return values from logging.Setup. --- agent/acl_test.go | 8 +------- agent/agent.go | 14 +------------- command/connect/proxy/proxy.go | 2 +- logging/logger.go | 15 ++++++--------- logging/logger_test.go | 19 +++++++++---------- 5 files changed, 18 insertions(+), 40 deletions(-) diff --git a/agent/acl_test.go b/agent/acl_test.go index f8bfdb3d17..3d25b965a8 100644 --- a/agent/acl_test.go +++ b/agent/acl_test.go @@ -37,10 +37,6 @@ type TestACLAgent struct { // when Shutdown() is called. Config *config.RuntimeConfig - // LogOutput is the sink for the logs. If nil, logs are written - // to os.Stderr. - LogOutput io.Writer - // DataDir is the data directory which is used when Config.DataDir // is not set. It is created automatically and removed when // Shutdown() is called. @@ -59,11 +55,10 @@ func NewTestACLAgent(t *testing.T, name string, hcl string, resolveAuthz authzRe a := &TestACLAgent{Name: name, HCL: hcl, resolveAuthzFn: resolveAuthz, resolveIdentFn: resolveIdent} dataDir := `data_dir = "acl-agent"` - logOutput := testutil.NewLogBuffer(t) logger := hclog.NewInterceptLogger(&hclog.LoggerOptions{ Name: a.Name, Level: hclog.Debug, - Output: logOutput, + Output: testutil.NewLogBuffer(t), }) opts := []AgentOption{ @@ -82,7 +77,6 @@ func NewTestACLAgent(t *testing.T, name string, hcl string, resolveAuthz authzRe a.Config = agent.GetConfig() a.Agent = agent - agent.LogOutput = logOutput agent.logger = logger agent.MemSink = metrics.NewInmemSink(1*time.Second, time.Minute) diff --git a/agent/agent.go b/agent/agent.go index f724ae1bc6..021b20292c 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -174,15 +174,6 @@ type Agent struct { // Used for writing our logs logger hclog.InterceptLogger - // LogOutput is a Writer which is used when creating dependencies that - // require logging. Note that this LogOutput is not used by the agent logger, - // so setting this field does not result in the agent logs being written to - // LogOutput. - // FIXME: refactor so that: dependencies accept an hclog.Logger, - // or LogOutput is part of RuntimeConfig, or change Agent.logger to be - // a new type with an Out() io.Writer method which returns this value. - LogOutput io.Writer - // In-memory sink used for collecting metrics MemSink *metrics.InmemSink @@ -477,14 +468,11 @@ func New(options ...AgentOption) (*Agent, error) { LogRotateMaxFiles: config.LogRotateMaxFiles, } - logger, logOutput, err := logging.Setup(logConf, flat.writers) + a.logger, err = logging.Setup(logConf, flat.writers) if err != nil { return nil, err } - a.logger = logger - a.LogOutput = logOutput - grpclog.SetLoggerV2(logging.NewGRPCLogger(logConf, a.logger)) } diff --git a/command/connect/proxy/proxy.go b/command/connect/proxy/proxy.go index 658c820172..8bc5bbb888 100644 --- a/command/connect/proxy/proxy.go +++ b/command/connect/proxy/proxy.go @@ -140,7 +140,7 @@ func (c *cmd) Run(args []string) int { logGate := logging.GatedWriter{Writer: &cli.UiWriter{Ui: c.UI}} - logger, _, err := logging.Setup(logConfig, []io.Writer{&logGate}) + logger, err := logging.Setup(logConfig, []io.Writer{&logGate}) if err != nil { c.UI.Error(err.Error()) return 1 diff --git a/logging/logger.go b/logging/logger.go index ec1dcc613d..bd530581cc 100644 --- a/logging/logger.go +++ b/logging/logger.go @@ -63,9 +63,9 @@ type LogSetupErrorFn func(string) // The provided ui object will get any log messages related to setting up // logging itself, and will also be hooked up to the gated logger. The final bool // parameter indicates if logging was set up successfully. -func Setup(config *Config, writers []io.Writer) (hclog.InterceptLogger, io.Writer, error) { +func Setup(config *Config, writers []io.Writer) (hclog.InterceptLogger, error) { if !ValidateLogLevel(config.LogLevel) { - return nil, nil, fmt.Errorf("Invalid log level: %s. Valid log levels are: %v", + return nil, fmt.Errorf("Invalid log level: %s. Valid log levels are: %v", config.LogLevel, allowedLogLevels) } @@ -84,7 +84,7 @@ func Setup(config *Config, writers []io.Writer) (hclog.InterceptLogger, io.Write if i == retries { timeout := time.Duration(retries) * delay - return nil, nil, fmt.Errorf("Syslog setup did not succeed within timeout (%s).", timeout.String()) + return nil, fmt.Errorf("Syslog setup did not succeed within timeout (%s).", timeout.String()) } time.Sleep(delay) @@ -121,19 +121,16 @@ func Setup(config *Config, writers []io.Writer) (hclog.InterceptLogger, io.Write MaxFiles: config.LogRotateMaxFiles, } if err := logFile.openNew(); err != nil { - return nil, nil, fmt.Errorf("Failed to setup logging: %w", err) + return nil, fmt.Errorf("Failed to setup logging: %w", err) } writers = append(writers, logFile) } - logOutput := io.MultiWriter(writers...) - logger := hclog.NewInterceptLogger(&hclog.LoggerOptions{ Level: LevelFromString(config.LogLevel), Name: config.Name, - Output: logOutput, + Output: io.MultiWriter(writers...), JSONFormat: config.LogJSON, }) - - return logger, logOutput, nil + return logger, nil } diff --git a/logging/logger_test.go b/logging/logger_test.go index eed4029e46..d7eb9b04dd 100644 --- a/logging/logger_test.go +++ b/logging/logger_test.go @@ -19,9 +19,8 @@ func TestLogger_SetupBasic(t *testing.T) { LogLevel: "INFO", } - logger, writer, err := Setup(cfg, nil) + logger, err := Setup(cfg, nil) require.NoError(err) - require.NotNil(writer) require.NotNil(logger) } @@ -29,7 +28,7 @@ func TestLogger_SetupInvalidLogLevel(t *testing.T) { t.Parallel() cfg := &Config{} - _, _, err := Setup(cfg, nil) + _, err := Setup(cfg, nil) testutil.RequireErrorContains(t, err, "Invalid log level") } @@ -62,7 +61,7 @@ func TestLogger_SetupLoggerErrorLevel(t *testing.T) { require := require.New(t) var buf bytes.Buffer - logger, _, err := Setup(&cfg, []io.Writer{&buf}) + logger, err := Setup(&cfg, []io.Writer{&buf}) require.NoError(err) require.NotNil(logger) @@ -85,7 +84,7 @@ func TestLogger_SetupLoggerDebugLevel(t *testing.T) { } var buf bytes.Buffer - logger, _, err := Setup(cfg, []io.Writer{&buf}) + logger, err := Setup(cfg, []io.Writer{&buf}) require.NoError(err) require.NotNil(logger) @@ -107,7 +106,7 @@ func TestLogger_SetupLoggerWithName(t *testing.T) { } var buf bytes.Buffer - logger, _, err := Setup(cfg, []io.Writer{&buf}) + logger, err := Setup(cfg, []io.Writer{&buf}) require.NoError(err) require.NotNil(logger) @@ -126,7 +125,7 @@ func TestLogger_SetupLoggerWithJSON(t *testing.T) { } var buf bytes.Buffer - logger, _, err := Setup(cfg, []io.Writer{&buf}) + logger, err := Setup(cfg, []io.Writer{&buf}) require.NoError(err) require.NotNil(logger) @@ -154,7 +153,7 @@ func TestLogger_SetupLoggerWithValidLogPath(t *testing.T) { } var buf bytes.Buffer - logger, _, err := Setup(cfg, []io.Writer{&buf}) + logger, err := Setup(cfg, []io.Writer{&buf}) require.NoError(err) require.NotNil(logger) } @@ -169,7 +168,7 @@ func TestLogger_SetupLoggerWithInValidLogPath(t *testing.T) { } var buf bytes.Buffer - logger, _, err := Setup(cfg, []io.Writer{&buf}) + logger, err := Setup(cfg, []io.Writer{&buf}) require.Error(err) require.True(errors.Is(err, os.ErrNotExist)) require.Nil(logger) @@ -190,7 +189,7 @@ func TestLogger_SetupLoggerWithInValidLogPathPermission(t *testing.T) { } var buf bytes.Buffer - logger, _, err := Setup(cfg, []io.Writer{&buf}) + logger, err := Setup(cfg, []io.Writer{&buf}) require.Error(err) require.True(errors.Is(err, os.ErrPermission)) require.Nil(logger)