From c1dc957bf918066560eed2c45edf12e4bc812f67 Mon Sep 17 00:00:00 2001 From: Herko Lategan Date: Mon, 30 Sep 2024 10:54:10 +0100 Subject: [PATCH] roachprod: redirect CRDB logs utility Currently, CRDB log is configured, by roachtest, to log to a file to catch any logs written to it during a roachtest run. This is usually from a shared test util that uses the CRDB log. The file sink on the CRDB logger will log program arguments by default, but this can leak sensitive information. This PR introduces a log redirect that uses the CRDB log interceptor functionality instead of using a file sink. This way we can avoid logging the program arguments. Epic: None Release note: None --- pkg/BUILD.bazel | 2 + pkg/roachprod/logger/BUILD.bazel | 19 ++++- pkg/roachprod/logger/log_redirect.go | 93 +++++++++++++++++++++++ pkg/roachprod/logger/log_redirect_test.go | 59 ++++++++++++++ 4 files changed, 171 insertions(+), 2 deletions(-) create mode 100644 pkg/roachprod/logger/log_redirect.go create mode 100644 pkg/roachprod/logger/log_redirect_test.go diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index 3d4d9a8bf4af..d45845c8507f 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -306,6 +306,7 @@ ALL_TESTS = [ "//pkg/roachprod/cloud:cloud_test", "//pkg/roachprod/config:config_test", "//pkg/roachprod/install:install_test", + "//pkg/roachprod/logger:logger_test", "//pkg/roachprod/opentelemetry:opentelemetry_test", "//pkg/roachprod/prometheus:prometheus_test", "//pkg/roachprod/promhelperclient:promhelperclient_test", @@ -1611,6 +1612,7 @@ GO_TARGETS = [ "//pkg/roachprod/install:install_test", "//pkg/roachprod/lock:lock", "//pkg/roachprod/logger:logger", + "//pkg/roachprod/logger:logger_test", "//pkg/roachprod/opentelemetry:opentelemetry", "//pkg/roachprod/opentelemetry:opentelemetry_test", "//pkg/roachprod/prometheus:prometheus", diff --git a/pkg/roachprod/logger/BUILD.bazel b/pkg/roachprod/logger/BUILD.bazel index f9d02e7c92b2..bb46bcd067f6 100644 --- a/pkg/roachprod/logger/BUILD.bazel +++ b/pkg/roachprod/logger/BUILD.bazel @@ -1,13 +1,28 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "logger", - srcs = ["log.go"], + srcs = [ + "log.go", + "log_redirect.go", + ], importpath = "github.com/cockroachdb/cockroach/pkg/roachprod/logger", visibility = ["//visibility:public"], deps = [ "//pkg/cli/exit", "//pkg/util/log", + "//pkg/util/log/logconfig", + "//pkg/util/log/logpb", "//pkg/util/syncutil", ], ) + +go_test( + name = "logger_test", + srcs = ["log_redirect_test.go"], + embed = [":logger"], + deps = [ + "//pkg/util/log", + "@com_github_stretchr_testify//require", + ], +) diff --git a/pkg/roachprod/logger/log_redirect.go b/pkg/roachprod/logger/log_redirect.go new file mode 100644 index 000000000000..8cd894c7a787 --- /dev/null +++ b/pkg/roachprod/logger/log_redirect.go @@ -0,0 +1,93 @@ +// Copyright 2024 The Cockroach Authors. +// +// Use of this software is governed by the CockroachDB Software License +// included in the /LICENSE file. + +package logger + +import ( + "context" + "encoding/json" + "fmt" + "io" + + "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/cockroach/pkg/util/log/logconfig" + "github.com/cockroachdb/cockroach/pkg/util/log/logpb" + "github.com/cockroachdb/cockroach/pkg/util/syncutil" +) + +type logRedirect struct { + syncutil.Mutex + logger *Logger + cancelIntercept func() + configured bool +} + +var logRedirectInst = &logRedirect{} + +// InitCRDBLogConfig sets up an interceptor for the CockroachDB log in order to +// redirect logs to a roachprod logger. This is necessary as the CockroachDB log +// is used in some test utilities shared between roachtest and CockroachDB. +// Generally, CockroachDB logs should not be used explicitly in roachtest. +func InitCRDBLogConfig(logger *Logger) { + logRedirectInst.Lock() + defer logRedirectInst.Unlock() + if logRedirectInst.configured { + panic("internal error: CRDB log interceptor already configured") + } + if logger == nil { + panic("internal error: specified logger is nil") + } + logConf := logconfig.DefaultStderrConfig() + logConf.Sinks.Stderr.Filter = logpb.Severity_FATAL + // Disable logging to a file. File sinks write the application arguments to + // the log by default (see: log_entry.go), and it is best to avoid logging + // the roachtest arguments as it may contain sensitive information. + if err := logConf.Validate(nil); err != nil { + panic(fmt.Errorf("internal error: could not validate CRDB log config: %w", err)) + } + if _, err := log.ApplyConfig(logConf, nil /* fileSinkMetricsForDir */, nil /* fatalOnLogStall */); err != nil { + panic(fmt.Errorf("internal error: could not apply CRDB log config: %w", err)) + } + logRedirectInst.logger = logger + logRedirectInst.cancelIntercept = log.InterceptWith(context.Background(), logRedirectInst) + logRedirectInst.configured = true +} + +// TestingCRDBLogConfig is meant to be used in unit tests to reset the CRDB log, +// it's interceptor, and the redirect log config. This is necessary to avoid +// leaking state between tests, that test the logging. +func TestingCRDBLogConfig(logger *Logger) { + logRedirectInst.Lock() + defer logRedirectInst.Unlock() + if logRedirectInst.cancelIntercept != nil { + logRedirectInst.cancelIntercept() + } + log.TestingResetActive() + logRedirectInst = &logRedirect{} + InitCRDBLogConfig(logger) +} + +// Intercept intercepts CockroachDB log entries and redirects it to the +// appropriate roachtest test logger or stderr. +func (i *logRedirect) Intercept(logData []byte) { + var entry logpb.Entry + if err := json.Unmarshal(logData, &entry); err != nil { + i.logger.Errorf("failed to unmarshal intercepted log entry: %v", err) + } + l := i.logger + if l != nil && !l.Closed() { + if entry.Severity == logpb.Severity_ERROR || entry.Severity == logpb.Severity_FATAL { + i.writeLog(l.Stderr, entry) + return + } + i.writeLog(l.Stdout, entry) + } +} + +func (i *logRedirect) writeLog(dst io.Writer, entry logpb.Entry) { + if err := log.FormatLegacyEntry(entry, dst); err != nil { + i.logger.Errorf("could not format and write CRDB log entry: %v", err) + } +} diff --git a/pkg/roachprod/logger/log_redirect_test.go b/pkg/roachprod/logger/log_redirect_test.go new file mode 100644 index 000000000000..8d8dcf88a850 --- /dev/null +++ b/pkg/roachprod/logger/log_redirect_test.go @@ -0,0 +1,59 @@ +// Copyright 2024 The Cockroach Authors. +// +// Use of this software is governed by the CockroachDB Software License +// included in the /LICENSE file. + +package logger + +import ( + "context" + "strings" + "testing" + + "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/stretchr/testify/require" +) + +type mockLogger struct { + logger *Logger + writer *mockWriter +} + +type mockWriter struct { + lines []string +} + +func (w *mockWriter) Write(p []byte) (n int, err error) { + w.lines = append(w.lines, string(p)) + return len(p), nil +} + +func newMockLogger(t *testing.T) *mockLogger { + writer := &mockWriter{} + logConf := Config{Stdout: writer, Stderr: writer} + l, err := logConf.NewLogger("" /* path */) + require.NoError(t, err) + return &mockLogger{logger: l, writer: writer} +} + +func requireLine(t *testing.T, l *mockLogger, line string) { + t.Helper() + found := false + for _, logLine := range l.writer.lines { + if strings.Contains(logLine, line) { + found = true + break + } + } + require.True(t, found, "expected line not found: %s", line) +} + +func TestLogRedirect(t *testing.T) { + l := newMockLogger(t) + TestingCRDBLogConfig(l.logger) + ctx := context.Background() + + log.Infof(ctx, "[simple test]") + requireLine(t, l, "[simple test]") + require.Equal(t, 1, len(l.writer.lines)) +}