From 1217995bc62669dbbbc2ac9c538c9b3b25bb4698 Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Fri, 25 Oct 2024 14:25:31 -0400 Subject: [PATCH] raft: implement SafeFormatter on Progress This commit implements the `redact.SafeFormatter` interface on `tracker.Progress` so that it will never be redacted when logged. Epic: None Release note: None --- pkg/base/config.go | 4 ++-- pkg/raft/tracker/BUILD.bazel | 2 ++ pkg/raft/tracker/progress.go | 23 +++++++++++-------- pkg/raft/tracker/progress_test.go | 8 ++++++- pkg/raft/tracker/state.go | 4 ++++ pkg/storage/enginepb/engine.go | 2 +- .../lint/passes/redactcheck/redactcheck.go | 3 +++ 7 files changed, 33 insertions(+), 13 deletions(-) diff --git a/pkg/base/config.go b/pkg/base/config.go index 75e3d0521c30..765f4712f2d8 100644 --- a/pkg/base/config.go +++ b/pkg/base/config.go @@ -852,7 +852,7 @@ func (m *WALFailoverMode) String() string { return redact.StringWithoutMarkers(m) } -// SafeFormat implements the refact.SafeFormatter interface. +// SafeFormat implements the redact.SafeFormatter interface. func (m *WALFailoverMode) SafeFormat(p redact.SafePrinter, _ rune) { switch *m { case WALFailoverDefault: @@ -905,7 +905,7 @@ func (c *WALFailoverConfig) String() string { return redact.StringWithoutMarkers(c) } -// SafeFormat implements the refact.SafeFormatter interface. +// SafeFormat implements the redact.SafeFormatter interface. func (c *WALFailoverConfig) SafeFormat(p redact.SafePrinter, _ rune) { switch c.Mode { case WALFailoverDefault: diff --git a/pkg/raft/tracker/BUILD.bazel b/pkg/raft/tracker/BUILD.bazel index d9754f7846d7..d7c8e6bbd70d 100644 --- a/pkg/raft/tracker/BUILD.bazel +++ b/pkg/raft/tracker/BUILD.bazel @@ -19,6 +19,7 @@ go_library( "//pkg/raft/raftstoreliveness", "//pkg/util/hlc", "//pkg/util/syncutil", + "@com_github_cockroachdb_redact//:redact", "@org_golang_x_exp//maps", ], ) @@ -39,6 +40,7 @@ go_test( "//pkg/util/hlc", "//pkg/util/leaktest", "//pkg/util/log", + "@com_github_cockroachdb_redact//:redact", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", ], diff --git a/pkg/raft/tracker/progress.go b/pkg/raft/tracker/progress.go index 8b4376cdd07b..944f7effa2aa 100644 --- a/pkg/raft/tracker/progress.go +++ b/pkg/raft/tracker/progress.go @@ -23,6 +23,7 @@ import ( "strings" pb "github.com/cockroachdb/cockroach/pkg/raft/raftpb" + "github.com/cockroachdb/redact" "golang.org/x/exp/maps" ) @@ -390,29 +391,33 @@ func (pr *Progress) ShouldSendProbe(last, commit uint64, advanceCommit bool) boo } } +// String implements the fmt.Stringer interface. func (pr *Progress) String() string { - var buf strings.Builder - fmt.Fprintf(&buf, "%s match=%d next=%d sentCommit=%d matchCommit=%d", pr.State, pr.Match, + return redact.StringWithoutMarkers(pr) +} + +// SafeFormat implements the redact.SafeFormatter interface. +func (pr *Progress) SafeFormat(p redact.SafePrinter, _ rune) { + p.Printf("%s match=%d next=%d sentCommit=%d matchCommit=%d", pr.State, pr.Match, pr.Next, pr.SentCommit, pr.MatchCommit) if pr.IsLearner { - fmt.Fprint(&buf, " learner") + p.SafeString(" learner") } if pr.IsPaused() { - fmt.Fprint(&buf, " paused") + p.SafeString(" paused") } if pr.PendingSnapshot > 0 { - fmt.Fprintf(&buf, " pendingSnap=%d", pr.PendingSnapshot) + p.Printf(" pendingSnap=%d", pr.PendingSnapshot) } if !pr.RecentActive { - fmt.Fprint(&buf, " inactive") + p.SafeString(" inactive") } if n := pr.Inflights.Count(); n > 0 { - fmt.Fprintf(&buf, " inflight=%d", n) + p.Printf(" inflight=%d", n) if pr.Inflights.Full() { - fmt.Fprint(&buf, "[full]") + p.SafeString("[full]") } } - return buf.String() } // ProgressMap is a map of *Progress. diff --git a/pkg/raft/tracker/progress_test.go b/pkg/raft/tracker/progress_test.go index 303cbf5fad52..f6caa7a432b6 100644 --- a/pkg/raft/tracker/progress_test.go +++ b/pkg/raft/tracker/progress_test.go @@ -20,10 +20,11 @@ package tracker import ( "testing" + "github.com/cockroachdb/redact" "github.com/stretchr/testify/assert" ) -func TestProgressString(t *testing.T) { +func TestProgressStringAndSafeFormat(t *testing.T) { ins := NewInflights(1, 0) ins.Add(123, 1) pr := &Progress{ @@ -40,7 +41,12 @@ func TestProgressString(t *testing.T) { } const exp = "StateSnapshot match=3 next=4 sentCommit=2 matchCommit=1 learner paused " + "pendingSnap=123 inactive inflight=1[full]" + // String. assert.Equal(t, exp, pr.String()) + // Redactable string. + assert.EqualValues(t, exp, redact.Sprint(pr)) + // Redacted string. + assert.EqualValues(t, exp, redact.Sprint(pr).Redact()) } func TestProgressIsPaused(t *testing.T) { diff --git a/pkg/raft/tracker/state.go b/pkg/raft/tracker/state.go index 7dbdd63fa666..c2ffc89d2d66 100644 --- a/pkg/raft/tracker/state.go +++ b/pkg/raft/tracker/state.go @@ -1,3 +1,6 @@ +// This code has been modified from its original form by The Cockroach Authors. +// All modifications are Copyright 2024 The Cockroach Authors. +// // Copyright 2019 The etcd Authors // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -40,3 +43,4 @@ var prstmap = [...]string{ } func (st StateType) String() string { return prstmap[st] } +func (st StateType) SafeValue() {} diff --git a/pkg/storage/enginepb/engine.go b/pkg/storage/enginepb/engine.go index a48467b8e907..d092067f8d92 100644 --- a/pkg/storage/enginepb/engine.go +++ b/pkg/storage/enginepb/engine.go @@ -17,7 +17,7 @@ func (e *EngineType) Type() string { return "string" } // String implements the pflag.Value interface. func (e *EngineType) String() string { return redact.StringWithoutMarkers(e) } -// SafeFormat implements the refact.SafeFormatter interface. +// SafeFormat implements the redact.SafeFormatter interface. func (e *EngineType) SafeFormat(p redact.SafePrinter, _ rune) { switch *e { case EngineTypeDefault: diff --git a/pkg/testutils/lint/passes/redactcheck/redactcheck.go b/pkg/testutils/lint/passes/redactcheck/redactcheck.go index 00e764fdeb6e..26f97ce8dec3 100644 --- a/pkg/testutils/lint/passes/redactcheck/redactcheck.go +++ b/pkg/testutils/lint/passes/redactcheck/redactcheck.go @@ -145,6 +145,9 @@ func runAnalyzer(pass *analysis.Pass) (interface{}, error) { "ConfChangeType": {}, "ConfChangeTransition": {}, }, + "github.com/cockroachdb/cockroach/pkg/raft/tracker": { + "StateType": {}, + }, "github.com/cockroachdb/cockroach/pkg/repstream/streampb": { "StreamID": {}, },