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

release-24.2: roachprod: redirect CRDB logs utility #133958

Merged
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
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
  • Loading branch information
herkolategan committed Oct 31, 2024
commit d89f0df513b3a9e2d63e16dce10edc2495dc1617
2 changes: 2 additions & 0 deletions pkg/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -296,6 +296,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",
@@ -1567,6 +1568,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",
19 changes: 17 additions & 2 deletions pkg/roachprod/logger/BUILD.bazel
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",
],
)
93 changes: 93 additions & 0 deletions pkg/roachprod/logger/log_redirect.go
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)
}
}
59 changes: 59 additions & 0 deletions pkg/roachprod/logger/log_redirect_test.go
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))
}