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
47 changes: 47 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,49 @@ 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)
}

// RedactBytesArgIfNeeded receives []byte argument and return omitted information if redact log enabled
func RedactBytesArgIfNeeded(arg []byte) []byte {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks verbose. How about just RedactBytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

return redactArgIfNeeded(arg).([]byte)
}

// RedactStringArgIfNeeded receives string argument and return omitted information if redact log enabled
func RedactStringArgIfNeeded(arg string) string {
return redactArgIfNeeded(arg).(string)
}

// redactArgIfNeeded will omit the argument if RedactLog is enabled
func redactArgIfNeeded(arg interface{}) interface{} {
if IsRedactLogEnabled() {
if arg == nil {
return nil
}
switch arg.(type) {
case []byte:
return []byte("?")
case string:
return "?"
default:
return "?"
}
}
return arg
}
51 changes: 51 additions & 0 deletions pkg/logutil/log_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,54 @@ 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
}
c.Assert(r, DeepEquals, 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.RedactStringArgIfNeeded(core.DiffRegionKeyInfo(origin, region))),
zap.Uint64("old-version", o.GetVersion()),
zap.Uint64("new-version", r.GetVersion()),
)
Expand Down
9 changes: 9 additions & 0 deletions server/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"encoding/json"
"flag"
"fmt"
"github.com/tikv/pd/pkg/logutil"
"net/url"
"os"
"path/filepath"
Expand Down Expand Up @@ -144,6 +145,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 +224,7 @@ const (
defaultDRWaitStoreTimeout = time.Minute
defaultDRWaitSyncTimeout = time.Minute
defaultDRWaitAsyncTimeout = 2 * time.Minute
defaultEnableRedactLog = int32(0)
)

var (
Expand Down Expand Up @@ -543,6 +547,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 Expand Up @@ -1143,6 +1151,7 @@ func (c *Config) SetupLogger() error {
}
c.logger = lg
c.logProps = p
logutil.SetRedactLog(c.EnableRedactLog == int32(1))
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.RedactStringArgIfNeeded(string(HexRegionKey(startKey)))),
zap.String("end-key", logutil.RedactStringArgIfNeeded(string(HexRegionKey(endKey)))),
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.RedactBytesArgIfNeeded(m.drRecoverKey)))
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.RedactBytesArgIfNeeded(startKey)),
zap.ByteString("region-start-key", logutil.RedactBytesArgIfNeeded(region.GetStartKey())),
zap.Uint64("region-id", region.GetID()))
return false
}
return region.GetReplicationStatus().GetStateId() == m.drAutoSync.StateID &&
Expand Down