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

logutil, *: implement log desensitization #3011

Merged
merged 15 commits into from
Sep 24, 2020
1 change: 1 addition & 0 deletions cmd/pd-server/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ func main() {
if err != nil {
log.Fatal("initialize logger error", errs.ZapError(err))
}
logutil.SetRedactLog(cfg.EnableRedactLog == int32(1))
Yisaer marked this conversation as resolved.
Show resolved Hide resolved

server.LogPDInfo()

Expand Down
37 changes: 37 additions & 0 deletions pkg/logutil/log.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"runtime"
"strings"
"sync"
"sync/atomic"

"github.com/coreos/pkg/capnslog"
zaplog "github.com/pingcap/log"
Expand Down Expand Up @@ -292,3 +293,39 @@ func LogPanic() {
zaplog.Fatal("panic", zap.Reflect("recover", e))
}
}

var (
enabledRedactLog atomic.Value
Yisaer marked this conversation as resolved.
Show resolved Hide resolved
)

func init() {
SetRedactLog(false)
}

// IsRedactLogEnabled indicates whether the log desensitization is enabled
func IsRedactLogEnabled() bool {
return enabledRedactLog.Load().(bool)
}

// SetRedactLog sets enabledRedactLog
func SetRedactLog(enabled bool) {
enabledRedactLog.Store(enabled)
}

// RedactArgIfNeeded will omit the argument if RedactLog is enabled
func RedactArgIfNeeded(arg interface{}) interface{} {
Yisaer marked this conversation as resolved.
Show resolved Hide resolved
if IsRedactLogEnabled() {
if arg == nil {
return nil
}
switch arg.(type) {
case []byte:
return []byte("?")
case string:
return "?"
default:
return "?"
}
}
return arg
}
56 changes: 56 additions & 0 deletions pkg/logutil/log_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,59 @@ func (s *testLogSuite) TestFileLog(c *C) {
c.Assert(InitFileLog(&zaplog.FileLogConfig{Filename: "/tmp"}), NotNil)
c.Assert(InitFileLog(&zaplog.FileLogConfig{Filename: "/tmp/test_file_log", MaxSize: 0}), IsNil)
}

func (s *testLogSuite) TestRedactLog(c *C) {
testcases := []struct {
name string
arg interface{}
enableRedactLog bool
expect interface{}
}{
{
name: "string arg, enable redact",
arg: "foo",
enableRedactLog: true,
expect: "?",
},
{
name: "string arg",
arg: "foo",
enableRedactLog: false,
expect: "foo",
},
{
name: "[]byte arg, enable redact",
arg: []byte("foo"),
enableRedactLog: true,
expect: []byte("?"),
},
{
name: "[]byte arg",
arg: []byte("foo"),
enableRedactLog: false,
expect: []byte("foo"),
},
{
name: "nil",
arg: nil,
enableRedactLog: true,
expect: nil,
},
}

for _, testcase := range testcases {
c.Log(testcase.name)
SetRedactLog(testcase.enableRedactLog)
r := RedactArgIfNeeded(testcase.arg)
if testcase.expect == nil {
c.Assert(r, IsNil)
continue
}
switch testcase.expect.(type) {
Yisaer marked this conversation as resolved.
Show resolved Hide resolved
case []byte:
c.Assert(bytes.Equal(testcase.expect.([]byte), r.([]byte)), Equals, true)
default:
c.Assert(r, Equals, testcase.expect)
}
}
}
2 changes: 1 addition & 1 deletion server/cluster/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,7 @@ func (c *RaftCluster) processRegionHeartbeat(region *core.RegionInfo) error {
if r.GetVersion() > o.GetVersion() {
log.Info("region Version changed",
zap.Uint64("region-id", region.GetID()),
zap.String("detail", core.DiffRegionKeyInfo(origin, region)),
zap.String("detail", logutil.RedactArgIfNeeded(core.DiffRegionKeyInfo(origin, region)).(string)),
Yisaer marked this conversation as resolved.
Show resolved Hide resolved
zap.Uint64("old-version", o.GetVersion()),
zap.Uint64("new-version", r.GetVersion()),
)
Expand Down
7 changes: 7 additions & 0 deletions server/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,8 @@ type Config struct {
Dashboard DashboardConfig `toml:"dashboard" json:"dashboard"`

ReplicationMode ReplicationModeConfig `toml:"replication-mode" json:"replication-mode"`
// EnableRedactLog indicates that whether redact log, 0 is disable. 1 is enable.
EnableRedactLog int32 `toml:"enable-redact-log" json:"enable-redact-log"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They use int32 because they need to do atomic operations but we don't. I think we don't have to follow them.

}

// NewConfig creates a new config.
Expand Down Expand Up @@ -221,6 +223,7 @@ const (
defaultDRWaitStoreTimeout = time.Minute
defaultDRWaitSyncTimeout = time.Minute
defaultDRWaitAsyncTimeout = 2 * time.Minute
defaultEnableRedactLog = int32(0)
)

var (
Expand Down Expand Up @@ -543,6 +546,10 @@ func (c *Config) Adjust(meta *toml.MetaData) error {

c.ReplicationMode.adjust(configMetaData.Child("replication-mode"))

if !configMetaData.IsDefined("enable-redact-log") {
c.EnableRedactLog = defaultEnableRedactLog
}

return nil
}

Expand Down
1 change: 1 addition & 0 deletions server/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ leader-schedule-limit = 0
c.Assert(cfg.PreVote, IsTrue)
c.Assert(cfg.Schedule.MaxMergeRegionKeys, Equals, uint64(defaultMaxMergeRegionKeys))
c.Assert(cfg.PDServerCfg.MetricStorage, Equals, "http://127.0.0.1:9090")
c.Assert(cfg.EnableRedactLog, Equals, defaultEnableRedactLog)

// Check undefined config fields
cfgData = `
Expand Down
6 changes: 4 additions & 2 deletions server/core/region_tree.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/pingcap/log"
"github.com/tikv/pd/pkg/btree"
"github.com/tikv/pd/pkg/errs"
"github.com/tikv/pd/pkg/logutil"
"go.uber.org/zap"
)

Expand Down Expand Up @@ -229,8 +230,9 @@ func (t *regionTree) RandomRegion(ranges []KeyRange) *RegionInfo {
if endIndex <= startIndex {
if len(endKey) > 0 && bytes.Compare(startKey, endKey) > 0 {
log.Error("wrong range keys",
zap.String("start-key", string(HexRegionKey(startKey))),
zap.String("end-key", string(HexRegionKey(endKey))), errs.ZapError(errs.ErrWrongRangeKeys))
zap.String("start-key", logutil.RedactArgIfNeeded(string(HexRegionKey(startKey))).(string)),
zap.String("end-key", logutil.RedactArgIfNeeded(string(HexRegionKey(endKey))).(string)),
errs.ZapError(errs.ErrWrongRangeKeys))
}
continue
}
Expand Down
7 changes: 5 additions & 2 deletions server/replication/replication_mode.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
pb "github.com/pingcap/kvproto/pkg/replication_modepb"
"github.com/pingcap/log"
"github.com/tikv/pd/pkg/errs"
"github.com/tikv/pd/pkg/logutil"
"github.com/tikv/pd/server/config"
"github.com/tikv/pd/server/core"
"github.com/tikv/pd/server/schedule/opt"
Expand Down Expand Up @@ -435,7 +436,7 @@ func (m *ModeManager) updateProgress() {
for len(m.drRecoverKey) > 0 || m.drRecoverCount == 0 {
regions := m.cluster.ScanRegions(m.drRecoverKey, nil, regionScanBatchSize)
if len(regions) == 0 {
log.Warn("scan empty regions", zap.ByteString("recover-key", m.drRecoverKey))
log.Warn("scan empty regions", zap.ByteString("recover-key", logutil.RedactArgIfNeeded(m.drRecoverKey).([]byte)))
return
}
for i, r := range regions {
Expand Down Expand Up @@ -486,7 +487,9 @@ func (m *ModeManager) estimateProgress() float32 {

func (m *ModeManager) checkRegionRecover(region *core.RegionInfo, startKey []byte) bool {
if !bytes.Equal(startKey, region.GetStartKey()) {
log.Warn("found region gap", zap.ByteString("key", startKey), zap.ByteString("region-start-key", region.GetStartKey()), zap.Uint64("region-id", region.GetID()))
log.Warn("found region gap", zap.ByteString("key", logutil.RedactArgIfNeeded(startKey).([]byte)),
zap.ByteString("region-start-key", logutil.RedactArgIfNeeded(region.GetStartKey()).([]byte)),
zap.Uint64("region-id", region.GetID()))
return false
}
return region.GetReplicationStatus().GetStateId() == m.drAutoSync.StateID &&
Expand Down
3 changes: 2 additions & 1 deletion tools/pd-simulator/simulator/event.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package simulator
import (
"github.com/pingcap/kvproto/pkg/metapb"
"github.com/pingcap/kvproto/pkg/pdpb"
"github.com/tikv/pd/pkg/logutil"
"github.com/tikv/pd/server/core"
"github.com/tikv/pd/tools/pd-simulator/simulator/cases"
"github.com/tikv/pd/tools/pd-simulator/simulator/simutil"
Expand Down Expand Up @@ -86,7 +87,7 @@ func (e *WriteFlowOnSpot) Run(raft *RaftEngine, tickCount int64) bool {
region := raft.SearchRegion([]byte(key))
simutil.Logger.Debug("search the region", zap.Reflect("region", region.GetMeta()))
if region == nil {
simutil.Logger.Error("region not found for key", zap.String("key", key))
simutil.Logger.Error("region not found for key", zap.String("key", logutil.RedactArgIfNeeded(key).(string)))
Yisaer marked this conversation as resolved.
Show resolved Hide resolved
continue
}
raft.updateRegionStore(region, size)
Expand Down