From 25b99aa03c5398eb88603f3814710e15e7025545 Mon Sep 17 00:00:00 2001
From: Raphael 'kena' Poss <knz@thaumogen.net>
Date: Sat, 22 Aug 2020 10:47:27 +0200
Subject: [PATCH] util/logs: request redactable logs by default

This changes the default value for the `--redactable-logs` flag to
`true`. This makes server produce redactable logs by default.

Note that `--redactable-logs` only triggers redactable logs if the
caller calls `SetupRedactionAndStderrRedirects()`. This is done by
e.g. the server code. This means that even with this patch,
*unit tests* continue to produce non-redactable logs.

This commit also adds telemetry for the enablement of redactable
markers, as `server.logging.redactable_logs.{enabled,disabled}`.

Release note (cli change): The `cockroach start` and `start-single-node`
commands now enable `--redactable-logs` by default. It is also
enabled by default in `cockroach demo` if `--log-dir` is passed.
This causes log files to become redactable, so that `cockroach debug
merge-log --redact` or `cockroach debug zip --redact` can remove
sensitive information out of log files. (Reminder: `cockroach debug
zip --redact` only affects *log files*; other items collected by the
command can still contain sensitive information)
---
 .../test_missing_log_output.tcl                 |  4 ++--
 pkg/cli/start.go                                | 17 +++++++++++++++++
 pkg/util/log/flags.go                           |  9 +++++++++
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/pkg/cli/interactive_tests/test_missing_log_output.tcl b/pkg/cli/interactive_tests/test_missing_log_output.tcl
index b1381b60cce0..00fc281ef8ba 100644
--- a/pkg/cli/interactive_tests/test_missing_log_output.tcl
+++ b/pkg/cli/interactive_tests/test_missing_log_output.tcl
@@ -34,7 +34,7 @@ eexpect ":/# "
 # is only reported on the logger which is writing first after stderr
 # is has been broken, and that may be the secondary logger.
 send "tail -F `find logs/db/logs -type l`\r"
-eexpect "error: write /dev/stderr: broken pipe"
+eexpect "error: write */dev/stderr*: broken pipe"
 interrupt
 eexpect ":/# "
 end_test
@@ -89,7 +89,7 @@ eexpect "CockroachDB node starting"
 system "($argv sql --insecure -e \"select crdb_internal.force_panic('helloworld')\" || true)&"
 # Check the panic is reported on the server's stderr
 eexpect "a SQL panic has occurred"
-eexpect "panic: helloworld"
+eexpect "panic: *helloworld"
 eexpect "stack trace"
 eexpect ":/# "
 # Check the panic is reported on the server log file
diff --git a/pkg/cli/start.go b/pkg/cli/start.go
index d2f54a28c736..da203bb51dea 100644
--- a/pkg/cli/start.go
+++ b/pkg/cli/start.go
@@ -35,6 +35,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/server/status"
+	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
@@ -1044,6 +1045,15 @@ func setupAndInitializeLoggingAndProfiling(
 			return nil, err
 		}
 
+		// Enable redactable logs if no other instruction given by the
+		// user. We do this only if we have a logging directory, because
+		// it's not safe to enable redaction markers on stderr.
+		if rl := fl.Lookup(logflags.RedactableLogsName); !rl.Changed {
+			if err := rl.Value.Set("true"); err != nil {
+				return nil, err
+			}
+		}
+
 		// Start the log file GC daemon to remove files that make the log
 		// directory too large.
 		log.StartGCDaemon(ctx)
@@ -1067,6 +1077,13 @@ func setupAndInitializeLoggingAndProfiling(
 		return nil, err
 	}
 
+	// Record redaction usage for telemetry.
+	if log.RedactableLogsEnabled() {
+		telemetry.Count("server.logging.redactable_logs.enabled")
+	} else {
+		telemetry.Count("server.logging.redactable_logs.disabled")
+	}
+
 	// We want to be careful to still produce useful debug dumps if the
 	// server configuration has disabled logging to files.
 	outputDirectory := "."
diff --git a/pkg/util/log/flags.go b/pkg/util/log/flags.go
index 9f9bec07981e..afcd72139e8f 100644
--- a/pkg/util/log/flags.go
+++ b/pkg/util/log/flags.go
@@ -183,3 +183,12 @@ func TestingResetActive() {
 	defer logging.mu.Unlock()
 	logging.mu.active = false
 }
+
+// RedactableLogsEnabled reports whether redaction markers were
+// actually enabled for the main logger. This is used for flag
+// telemetry; a better API would be needed for more fine grained
+// information, as each different logger may have a different
+// redaction setting.
+func RedactableLogsEnabled() bool {
+	return mainLog.redactableLogs.Get()
+}