Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix non-JSON log messages when using -log-format JSON #24252

Merged
merged 12 commits into from
Nov 29, 2023
32 changes: 19 additions & 13 deletions command/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import (

systemd "github.com/coreos/go-systemd/daemon"
ctconfig "github.com/hashicorp/consul-template/config"
log "github.com/hashicorp/go-hclog"
hclog "github.com/hashicorp/go-hclog"
"github.com/hashicorp/go-multierror"
"github.com/hashicorp/go-secure-stdlib/gatedwriter"
"github.com/hashicorp/go-secure-stdlib/parseutil"
Expand Down Expand Up @@ -78,7 +78,7 @@ type AgentCommand struct {

logWriter io.Writer
logGate *gatedwriter.Writer
logger log.Logger
logger hclog.Logger

// Telemetry object
metricsHelper *metricsutil.MetricsHelper
Expand Down Expand Up @@ -210,7 +210,13 @@ func (c *AgentCommand) Run(args []string) int {
c.outputErrors(err)
return 1
}

// Update the logger and then base the log writer on that logger.
// Log writer is supplied to consul-template runners for templates and execs.
// We want to ensure that consul-template will honor the settings, for example
// if the -log-format is JSON we want JSON, not a mix of JSON and non-JSON messages.
c.logger = l
c.logWriter = l.StandardWriter(&hclog.StandardLoggerOptions{InferLevels: true})

infoKeys := make([]string, 0, 10)
info := make(map[string]string)
Expand Down Expand Up @@ -1093,31 +1099,31 @@ func (c *AgentCommand) handleQuit(enabled bool) http.Handler {
}

// newLogger creates a logger based on parsed config field on the Agent Command struct.
func (c *AgentCommand) newLogger() (log.InterceptLogger, error) {
func (c *AgentCommand) newLogger() (hclog.InterceptLogger, error) {
if c.config == nil {
return nil, fmt.Errorf("cannot create logger, no config")
}

var errors error
var errs *multierror.Error

// Parse all the log related config
logLevel, err := logging.ParseLogLevel(c.config.LogLevel)
if err != nil {
errors = multierror.Append(errors, err)
errs = multierror.Append(errs, err)
}

logFormat, err := logging.ParseLogFormat(c.config.LogFormat)
if err != nil {
errors = multierror.Append(errors, err)
errs = multierror.Append(errs, err)
}

logRotateDuration, err := parseutil.ParseDurationSecond(c.config.LogRotateDuration)
if err != nil {
errors = multierror.Append(errors, err)
errs = multierror.Append(errs, err)
}

if errors != nil {
return nil, errors
if errs != nil {
return nil, errs
}

logCfg := &logging.LogConfig{
Expand All @@ -1140,20 +1146,20 @@ func (c *AgentCommand) newLogger() (log.InterceptLogger, error) {

// loadConfig attempts to generate an Agent config from the file(s) specified.
func (c *AgentCommand) loadConfig(paths []string) (*agentConfig.Config, error) {
var errors error
var errs *multierror.Error
cfg := agentConfig.NewConfig()

for _, configPath := range paths {
configFromPath, err := agentConfig.LoadConfig(configPath)
if err != nil {
errors = multierror.Append(errors, fmt.Errorf("error loading configuration from %s: %w", configPath, err))
errs = multierror.Append(errs, fmt.Errorf("error loading configuration from %s: %w", configPath, err))
} else {
cfg = cfg.Merge(configFromPath)
}
}

if errors != nil {
return nil, errors
if errs != nil {
return nil, errs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the error is of type multierror.Error, you may want to use errs.ErrorOrNil() on the return.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hghaf099, do you mean like this?

if errs.ErrorOrNil() != nil {
		return nil, errs.ErrorOrNil()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my bad. It seems that we only need to return if the error is non-nil. Then what you already had is sufficient. Sorry for the confusion.

}

if err := cfg.ValidateConfig(); err != nil {
Expand Down
1 change: 0 additions & 1 deletion command/agent/internal/ctmanager/runner_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
ctconfig "github.com/hashicorp/consul-template/config"
ctlogging "github.com/hashicorp/consul-template/logging"
"github.com/hashicorp/go-hclog"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nobody likes the extra line.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"github.com/hashicorp/vault/command/agent/config"
"github.com/hashicorp/vault/sdk/helper/pointerutil"
)
Expand Down