-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
132586: roachprod: redirect CRDB logs utility r=DarrylWong a=herkolategan 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 Co-authored-by: Herko Lategan <[email protected]>
- Loading branch information
Showing
6 changed files
with
184 additions
and
22 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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", | ||
| ], | ||
| ) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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)) | ||
| } |