From 380a2f6c18d7c30ef167f979a3e7332b0f5e4b4d Mon Sep 17 00:00:00 2001 From: Alex Barganier Date: Tue, 31 Oct 2023 20:19:10 +0000 Subject: [PATCH] pkg/util/log: selectively apply file-defaults.format-options to stderr Fixes: https://github.com/cockroachdb/cockroach/issues/113321 The stderr sink in the log config is only allowed to use the `crdb-v2-tty` format, and therefore, only `format-options` supported by the `crdb-v2-tty` format should be applied to the stderr sink. Unfortunately, a bug in the log config parse & validation code didn't follow this rule. The configuration that's part of the `file-defaults` is propagated to the stderr sink, and this included the `format-options`, even if they were only relevant to another log format type (e.g. `json`). This caused an error when trying to apply the options to the stderr log sink on startup, e.g.: ``` ERROR: unknown format option: "datetime-format" ``` To solve this problem, we should only propagate the `format-options` used in `file-defaults` to the stderr sink's config IFF the `file-defaults` format is of a `crdb-v2` variety. Since the stderr sink also uses the `crdb-v2-tty` format, we can only be sure that the `format-options` used in `file-defaults` is supported by the stderr sink if the `format` used in `file-defaults` is also part of `crdb-v2`. However, if `format-options` is explicitly defined within the `sinks.stderr` config, we need to be careful not to overwrite them with those defined in `file-defaults`. This patch accomplishes fixes for all these issues, and adds new tests to cover all these scenarios. Release note: none --- pkg/cli/interactive_tests/test_log_flags.tcl | 1 - pkg/util/log/logconfig/config.go | 6 +- pkg/util/log/logconfig/testdata/validate | 87 ++++++++++++++++++++ pkg/util/log/logconfig/validate.go | 17 ++++ 4 files changed, 108 insertions(+), 3 deletions(-) diff --git a/pkg/cli/interactive_tests/test_log_flags.tcl b/pkg/cli/interactive_tests/test_log_flags.tcl index 49fcac53342e..cfb9c44e20ab 100644 --- a/pkg/cli/interactive_tests/test_log_flags.tcl +++ b/pkg/cli/interactive_tests/test_log_flags.tcl @@ -77,7 +77,6 @@ end_test start_test "Check that the log flag is properly recognized for non-server commands" send "$argv debug reset-quorum 123 --log='sinks: {stderr: {format: json }}'\r" -eexpect "\"severity\":\"ERROR\"" eexpect "connection to server failed" eexpect ":/# " end_test diff --git a/pkg/util/log/logconfig/config.go b/pkg/util/log/logconfig/config.go index 0a2a6fbd242d..874f4e9a6906 100644 --- a/pkg/util/log/logconfig/config.go +++ b/pkg/util/log/logconfig/config.go @@ -29,8 +29,10 @@ import ( // specified in a configuration. const DefaultFileFormat = `crdb-v2` -// DefaultStderrFormat is the entry format for stderr sinks -// when not specified in a configuration. +// DefaultStderrFormat is the entry format for stderr sinks. +// NB: The format for stderr is always set to `crdb-v2-tty`, +// and cannot be changed. We enforce this in the validation step. +// See: https://www.cockroachlabs.com/docs/stable/configure-logs#output-to-stderr const DefaultStderrFormat = `crdb-v2-tty` // DefaultFluentFormat is the entry format for fluent sinks diff --git a/pkg/util/log/logconfig/testdata/validate b/pkg/util/log/logconfig/testdata/validate index b03266962ed5..c5b8c9c8f68b 100644 --- a/pkg/util/log/logconfig/testdata/validate +++ b/pkg/util/log/logconfig/testdata/validate @@ -254,6 +254,93 @@ capture-stray-errors: dir: /default-dir max-group-size: 100MiB +# Check that file-defaults format options are transferred to stderr if using a crdb-v2 format. +yaml +file-defaults: + format: crdb-v2 + format-options: {timezone: america/new_york} +---- +sinks: + file-groups: + default: + channels: {INFO: all} + filter: INFO + format-options: + timezone: america/new_york + stderr: + filter: NONE + format-options: + timezone: america/new_york +capture-stray-errors: + enable: true + dir: /default-dir + max-group-size: 100MiB + +# Check that file-defaults format options are NOT transferred to stderr if NOT using a crdb-v2 format. +yaml +file-defaults: + format: json + format-options: {datetime-format: rfc3339, datetime-timezone: America/New_York} +---- +sinks: + file-groups: + default: + channels: {INFO: all} + filter: INFO + format: json + format-options: + datetime-format: rfc3339 + datetime-timezone: America/New_York + stderr: + filter: NONE +capture-stray-errors: + enable: true + dir: /default-dir + max-group-size: 100MiB + +# Check that file-defaults format options do NOT overwrite format-options if explicitly defined in stderr. +yaml +file-defaults: + format: crdb-v2 + format-options: {timezone: america/new_york} +sinks: + stderr: + format-options: {timezone: america/chicago} +---- +sinks: + file-groups: + default: + channels: {INFO: all} + filter: INFO + format-options: + timezone: america/new_york + stderr: + filter: NONE + format-options: + timezone: america/chicago +capture-stray-errors: + enable: true + dir: /default-dir + max-group-size: 100MiB + +# Check that stderr always uses crdb-v2-tty format, even if we try to set it to an invalid format +yaml +sinks: + stderr: + format: crdb-v1-tty +---- +sinks: + file-groups: + default: + channels: {INFO: all} + filter: INFO + stderr: + filter: NONE +capture-stray-errors: + enable: true + dir: /default-dir + max-group-size: 100MiB + # Check that NONE filter elides files. yaml file-defaults: {filter: NONE} diff --git a/pkg/util/log/logconfig/validate.go b/pkg/util/log/logconfig/validate.go index 2213f7cdf30d..68bf016c884f 100644 --- a/pkg/util/log/logconfig/validate.go +++ b/pkg/util/log/logconfig/validate.go @@ -170,11 +170,28 @@ func (c *Config) Validate(defaultLogDir *string) (resErr error) { if c.Sinks.Stderr.Filter == logpb.Severity_UNKNOWN { c.Sinks.Stderr.Filter = logpb.Severity_NONE } + // We need to know if format-options were specifically defined on the stderr sink later on, + // since this information is lost once propagateCommonDefaults is called. + stdErrFormatOptionsOriginallySet := len(c.Sinks.Stderr.FormatOptions) > 0 propagateCommonDefaults(&c.Sinks.Stderr.CommonSinkConfig, c.FileDefaults.CommonSinkConfig) if c.Sinks.Stderr.Auditable != nil && *c.Sinks.Stderr.Auditable { c.Sinks.Stderr.Criticality = &bt } c.Sinks.Stderr.Auditable = nil + // The format parameter for stderr is set to `crdb-v2-tty` and cannot be changed. + // See docs: https://www.cockroachlabs.com/docs/stable/configure-logs#output-to-stderr + if *c.Sinks.Stderr.Format != DefaultStderrFormat { + f := DefaultStderrFormat + c.Sinks.Stderr.Format = &f + } + // FormatOptions are format-specific. We should only copy them over to StdErr from + // FileDefaults if FileDefaults is also making use of a crdb-v2 format. Otherwise, + // we are likely to error when trying to apply an unsupported format option. + if c.FileDefaults.CommonSinkConfig.Format != nil && + !strings.Contains(*c.FileDefaults.CommonSinkConfig.Format, "v2") && + !stdErrFormatOptionsOriginallySet { + c.Sinks.Stderr.CommonSinkConfig.FormatOptions = map[string]string{} + } if err := c.ValidateCommonSinkConfig(c.Sinks.Stderr.CommonSinkConfig); err != nil { fmt.Fprintf(&errBuf, "stderr sink: %v\n", err) }