From e4d29682aff3ca40297c9c57a6873427b464a477 Mon Sep 17 00:00:00 2001 From: Hillium Date: Fri, 15 May 2020 14:45:06 +0800 Subject: [PATCH 1/7] backup: don't log secret of s3. --- pkg/backup/client.go | 7 ++++++- tests/br_s3/run.sh | 22 ++++++++++++++++++++-- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/pkg/backup/client.go b/pkg/backup/client.go index 709e3842f..7c00265ec 100644 --- a/pkg/backup/client.go +++ b/pkg/backup/client.go @@ -760,7 +760,12 @@ func SendBackup( req kvproto.BackupRequest, respFn func(*kvproto.BackupResponse) error, ) error { - log.Info("try backup", zap.Any("backup request", req)) + log.Info("try backup", + zap.Binary("StartKey", req.StartKey), + zap.Binary("EndKey", req.EndKey), + zap.Uint64("RateLimit", req.RateLimit), + zap.Uint32("Concurrency", req.Concurrency), + ) ctx, cancel := context.WithCancel(ctx) defer cancel() bcli, err := client.Backup(ctx, &req) diff --git a/tests/br_s3/run.sh b/tests/br_s3/run.sh index 394ddcf0e..4d5385821 100755 --- a/tests/br_s3/run.sh +++ b/tests/br_s3/run.sh @@ -58,7 +58,16 @@ done # backup full echo "backup start..." -run_br --pd $PD_ADDR backup full -s "s3://mybucket/$DB?endpoint=http://$S3_ENDPOINT" +BACKUP_LOG="backup.log" +rm -f $BACKUP_LOG +unset BR_LOG_TO_TERM +run_br --pd $PD_ADDR backup full -s "s3://mybucket/$DB?endpoint=http://$S3_ENDPOINT" --log-file $BACKUP_LOG || \ + (cat $BACKUP_LOG && exit 1) + +if grep -i $MINIO_SECRET_KEY $BACKUP_LOG; then + echo "Secret key logged in log. Please remove them." + exit 1 +fi for i in $(seq $DB_COUNT); do run_sql "DROP DATABASE $DB${i};" @@ -66,7 +75,16 @@ done # restore full echo "restore start..." -run_br restore full -s "s3://mybucket/$DB" --pd $PD_ADDR --s3.endpoint="http://$S3_ENDPOINT" +RESTORE_LOG="restore.log" +rm -f $RESTORE_LOG +run_br restore full -s "s3://mybucket/$DB" --pd $PD_ADDR --s3.endpoint="http://$S3_ENDPOINT" --log-file $RESTORE_LOG || \ + (cat $RESTORE_LOG && exit 1) +BR_LOG_TO_TERM=1 + +if grep -i $MINIO_SECRET_KEY $RESTORE_LOG; then + echo "Secret key logged in log. Please remove them." + exit 1 +fi for i in $(seq $DB_COUNT); do row_count_new[${i}]=$(run_sql "SELECT COUNT(*) FROM $DB${i}.$TABLE;" | awk '/COUNT/{print $2}') From d7eca47bd30879507727d6f8f7d31e5efdc4793c Mon Sep 17 00:00:00 2001 From: Hillium Date: Fri, 15 May 2020 15:16:46 +0800 Subject: [PATCH 2/7] br_s3: always print log. --- tests/br_s3/run.sh | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tests/br_s3/run.sh b/tests/br_s3/run.sh index 4d5385821..0eac3fb3e 100755 --- a/tests/br_s3/run.sh +++ b/tests/br_s3/run.sh @@ -61,8 +61,11 @@ echo "backup start..." BACKUP_LOG="backup.log" rm -f $BACKUP_LOG unset BR_LOG_TO_TERM -run_br --pd $PD_ADDR backup full -s "s3://mybucket/$DB?endpoint=http://$S3_ENDPOINT" --log-file $BACKUP_LOG || \ - (cat $BACKUP_LOG && exit 1) +run_br --pd $PD_ADDR backup full -s "s3://mybucket/$DB?endpoint=http://$S3_ENDPOINT" \ + --log-file $BACKUP_LOG || \ + ( ( cat $BACKUP_LOG || BR_LOG_TO_TERM=1) && exit 1 ) +cat $BACKUP_LOG +BR_LOG_TO_TERM=1 if grep -i $MINIO_SECRET_KEY $BACKUP_LOG; then echo "Secret key logged in log. Please remove them." @@ -77,8 +80,11 @@ done echo "restore start..." RESTORE_LOG="restore.log" rm -f $RESTORE_LOG -run_br restore full -s "s3://mybucket/$DB" --pd $PD_ADDR --s3.endpoint="http://$S3_ENDPOINT" --log-file $RESTORE_LOG || \ - (cat $RESTORE_LOG && exit 1) +unset BR_LOG_TO_TERM +run_br restore full -s "s3://mybucket/$DB" --pd $PD_ADDR --s3.endpoint="http://$S3_ENDPOINT" \ + --log-file $RESTORE_LOG || \ + ( ( cat $RESTORE_LOG || BR_LOG_TO_TERM=1) && exit 1 ) +cat $RESTORE_LOG BR_LOG_TO_TERM=1 if grep -i $MINIO_SECRET_KEY $RESTORE_LOG; then From 530dbdf1d36e1efd8a5cb10521e1f301930aef53 Mon Sep 17 00:00:00 2001 From: Hillium Date: Fri, 15 May 2020 16:20:18 +0800 Subject: [PATCH 3/7] *: remove query string on logging arguments. --- cmd/backup.go | 2 +- cmd/restore.go | 2 +- cmd/validate.go | 2 +- pkg/task/common.go | 28 ++++++++++++++++++++++++++++ pkg/task/common_test.go | 37 +++++++++++++++++++++++++++++++++++++ pkg/utils/version.go | 11 ----------- 6 files changed, 68 insertions(+), 14 deletions(-) create mode 100644 pkg/task/common_test.go diff --git a/cmd/backup.go b/cmd/backup.go index d37229e0a..0958e92db 100644 --- a/cmd/backup.go +++ b/cmd/backup.go @@ -42,7 +42,7 @@ func NewBackupCommand() *cobra.Command { return err } utils.LogBRInfo() - utils.LogArguments(c) + task.LogArguments(c) // Do not run ddl worker in BR. ddl.RunWorker = false diff --git a/cmd/restore.go b/cmd/restore.go index 547501d95..d4f2e8ec6 100644 --- a/cmd/restore.go +++ b/cmd/restore.go @@ -53,7 +53,7 @@ func NewRestoreCommand() *cobra.Command { return err } utils.LogBRInfo() - utils.LogArguments(c) + task.LogArguments(c) // Do not run stat worker in BR. session.DisableStats4Test() diff --git a/cmd/validate.go b/cmd/validate.go index 386a7bb47..5968db2d1 100644 --- a/cmd/validate.go +++ b/cmd/validate.go @@ -37,7 +37,7 @@ func NewValidateCommand() *cobra.Command { return err } utils.LogBRInfo() - utils.LogArguments(c) + task.LogArguments(c) return nil }, } diff --git a/pkg/task/common.go b/pkg/task/common.go index 94e91495b..9e45451e4 100644 --- a/pkg/task/common.go +++ b/pkg/task/common.go @@ -6,9 +6,13 @@ import ( "context" "crypto/tls" "fmt" + "net/url" "regexp" "strings" + "github.com/pingcap/log" + "go.uber.org/zap" + "github.com/gogo/protobuf/proto" "github.com/pingcap/errors" "github.com/pingcap/kvproto/pkg/backup" @@ -286,3 +290,27 @@ func escapeFilterName(name string) string { } return "~^" + regexp.QuoteMeta(name) + "$" } + +// flagToZapField checks whether this flag can be logged, +// if need to log, return its zap field. Or return a field with hidden value. +func flagToZapField(f *pflag.Flag) zap.Field { + if f.Name == flagStorage { + hiddenQuery, err := url.Parse(f.Value.String()) + if err != nil { + return zap.String(f.Name, "") + } + // hide all query here. + hiddenQuery.RawQuery = "" + return zap.Stringer(f.Name, hiddenQuery) + } + return zap.Stringer(f.Name, f.Value) +} + +// LogArguments prints origin command arguments. +func LogArguments(cmd *cobra.Command) { + var fields []zap.Field + cmd.Flags().VisitAll(func(f *pflag.Flag) { + fields = append(fields, flagToZapField(f)) + }) + log.Info("arguments", fields...) +} diff --git a/pkg/task/common_test.go b/pkg/task/common_test.go new file mode 100644 index 000000000..ef1db4994 --- /dev/null +++ b/pkg/task/common_test.go @@ -0,0 +1,37 @@ +package task + +import ( + "fmt" + + . "github.com/pingcap/check" + "github.com/spf13/pflag" +) + +var _ = Suite(&testCommonSuite{}) + +type testCommonSuite struct{} + +type fakeValue string + +func (f fakeValue) String() string { + return string(f) +} + +func (f fakeValue) Set(string) error { + panic("implement me") +} + +func (f fakeValue) Type() string { + panic("implement me") +} + +func (*testCommonSuite) TestUrlNoQuery(c *C) { + flag := &pflag.Flag{ + Name: flagStorage, + Value: fakeValue("s3://some/what?secret=a123456789&key=987654321"), + } + + field := flagToZapField(flag) + c.Assert(field.Key, Equals, flagStorage) + c.Assert(field.Interface.(fmt.Stringer).String(), Equals, "s3://some/what") +} diff --git a/pkg/utils/version.go b/pkg/utils/version.go index b8248f969..e9c81b68e 100644 --- a/pkg/utils/version.go +++ b/pkg/utils/version.go @@ -9,8 +9,6 @@ import ( "github.com/pingcap/log" "github.com/pingcap/tidb/util/israce" - "github.com/spf13/cobra" - "github.com/spf13/pflag" "go.uber.org/zap" ) @@ -45,12 +43,3 @@ func BRInfo() string { fmt.Fprintf(&buf, "Race Enabled: %t", israce.RaceEnabled) return buf.String() } - -// LogArguments prints origin command arguments. -func LogArguments(cmd *cobra.Command) { - var fields []zap.Field - cmd.Flags().VisitAll(func(f *pflag.Flag) { - fields = append(fields, zap.Stringer(f.Name, f.Value)) - }) - log.Info("arguments", fields...) -} From f621c78237210f67cca16e05c6a9ae097605d8eb Mon Sep 17 00:00:00 2001 From: Hillium Date: Fri, 15 May 2020 16:24:24 +0800 Subject: [PATCH 4/7] br_s3: reset BR_LOG_TO_TERM on fail. --- tests/br_s3/run.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/br_s3/run.sh b/tests/br_s3/run.sh index 0eac3fb3e..8b2cdace0 100755 --- a/tests/br_s3/run.sh +++ b/tests/br_s3/run.sh @@ -63,7 +63,7 @@ rm -f $BACKUP_LOG unset BR_LOG_TO_TERM run_br --pd $PD_ADDR backup full -s "s3://mybucket/$DB?endpoint=http://$S3_ENDPOINT" \ --log-file $BACKUP_LOG || \ - ( ( cat $BACKUP_LOG || BR_LOG_TO_TERM=1) && exit 1 ) + ( cat $BACKUP_LOG && BR_LOG_TO_TERM=1 && exit 1 ) cat $BACKUP_LOG BR_LOG_TO_TERM=1 @@ -83,7 +83,7 @@ rm -f $RESTORE_LOG unset BR_LOG_TO_TERM run_br restore full -s "s3://mybucket/$DB" --pd $PD_ADDR --s3.endpoint="http://$S3_ENDPOINT" \ --log-file $RESTORE_LOG || \ - ( ( cat $RESTORE_LOG || BR_LOG_TO_TERM=1) && exit 1 ) + ( cat $RESTORE_LOG && BR_LOG_TO_TERM=1 && exit 1 ) cat $RESTORE_LOG BR_LOG_TO_TERM=1 From 9bd3d407389e89714f7dd8abd9789b313ee85bed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=B1=B1=E5=B2=9A?= <36239017+YuJuncen@users.noreply.github.com> Date: Fri, 15 May 2020 16:26:34 +0800 Subject: [PATCH 5/7] Apply suggestions from code review Co-authored-by: kennytm --- pkg/backup/client.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/backup/client.go b/pkg/backup/client.go index 7c00265ec..080c7e53a 100644 --- a/pkg/backup/client.go +++ b/pkg/backup/client.go @@ -763,8 +763,7 @@ func SendBackup( log.Info("try backup", zap.Binary("StartKey", req.StartKey), zap.Binary("EndKey", req.EndKey), - zap.Uint64("RateLimit", req.RateLimit), - zap.Uint32("Concurrency", req.Concurrency), + zap.Uint64("storeID", storeID), ) ctx, cancel := context.WithCancel(ctx) defer cancel() From 5311a0f40870c3d2f4ac2516af2a94eb5020506e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=B1=B1=E5=B2=9A?= <36239017+YuJuncen@users.noreply.github.com> Date: Fri, 15 May 2020 16:35:59 +0800 Subject: [PATCH 6/7] Apply suggestions from code review Co-authored-by: kennytm --- pkg/task/common.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/task/common.go b/pkg/task/common.go index 9e45451e4..540751717 100644 --- a/pkg/task/common.go +++ b/pkg/task/common.go @@ -310,7 +310,9 @@ func flagToZapField(f *pflag.Flag) zap.Field { func LogArguments(cmd *cobra.Command) { var fields []zap.Field cmd.Flags().VisitAll(func(f *pflag.Flag) { - fields = append(fields, flagToZapField(f)) + if f.Changed { + fields = append(fields, flagToZapField(f)) + } }) log.Info("arguments", fields...) } From 6e90efc41b68975791abd6bce96d4e2fab709372 Mon Sep 17 00:00:00 2001 From: Hillium Date: Fri, 15 May 2020 16:44:43 +0800 Subject: [PATCH 7/7] *: remove query string on logging arguments. --- pkg/task/common.go | 5 ++--- pkg/task/common_test.go | 2 ++ 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/task/common.go b/pkg/task/common.go index 540751717..c7962a94a 100644 --- a/pkg/task/common.go +++ b/pkg/task/common.go @@ -10,18 +10,17 @@ import ( "regexp" "strings" - "github.com/pingcap/log" - "go.uber.org/zap" - "github.com/gogo/protobuf/proto" "github.com/pingcap/errors" "github.com/pingcap/kvproto/pkg/backup" + "github.com/pingcap/log" pd "github.com/pingcap/pd/v4/client" "github.com/pingcap/tidb-tools/pkg/filter" "github.com/pingcap/tidb/store/tikv" "github.com/spf13/cobra" "github.com/spf13/pflag" "go.etcd.io/etcd/pkg/transport" + "go.uber.org/zap" "github.com/pingcap/br/pkg/conn" "github.com/pingcap/br/pkg/glue" diff --git a/pkg/task/common_test.go b/pkg/task/common_test.go index ef1db4994..6c4869809 100644 --- a/pkg/task/common_test.go +++ b/pkg/task/common_test.go @@ -1,3 +1,5 @@ +// Copyright 2020 PingCAP, Inc. Licensed under Apache-2.0. + package task import (