From cffe8b003efc9724ef3a67de35bfa5cbd6546fb7 Mon Sep 17 00:00:00 2001 From: Solon Gordon Date: Sun, 30 Aug 2020 20:23:13 -0400 Subject: [PATCH 01/11] sql: allow non-admins to perform some RESTOREs Release justification: Low risk, high reward change to existing functionality Release note (sql change): Non-admin users are now permitted to execute RESTORE statements as long as the restore does not depend on implicit credentials and the user has the appropriate privileges to create all of the resulting database objects. For database restores, this means the user must have the CREATEDB role privilege. For table restores, the user must have CREATE privileges on the parent database. Full cluster restores still require admin privileges. --- pkg/base/testing_knobs.go | 1 + pkg/ccl/backupccl/backup_test.go | 74 +++++++++++----- pkg/ccl/backupccl/restore_planning.go | 66 +++++++++++++- .../testdata/backup-restore/permissions | 88 +++++++++++++++++++ pkg/server/server_sql.go | 3 + pkg/sql/exec_util.go | 13 +++ 6 files changed, 217 insertions(+), 28 deletions(-) create mode 100644 pkg/ccl/backupccl/testdata/backup-restore/permissions diff --git a/pkg/base/testing_knobs.go b/pkg/base/testing_knobs.go index 202f13b681fa..ac1504606ccf 100644 --- a/pkg/base/testing_knobs.go +++ b/pkg/base/testing_knobs.go @@ -34,4 +34,5 @@ type TestingKnobs struct { Server ModuleTestingKnobs TenantTestingKnobs ModuleTestingKnobs JobsTestingKnobs ModuleTestingKnobs + BackupRestore ModuleTestingKnobs } diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go index 72f6c1c8b687..579495a5b7b8 100644 --- a/pkg/ccl/backupccl/backup_test.go +++ b/pkg/ccl/backupccl/backup_test.go @@ -83,14 +83,22 @@ func init() { cloud.RegisterKMSFromURIFactory(MakeTestKMS, "testkms") } +type sqlDBKey struct { + server string + user string +} + type datadrivenTestState struct { servers map[string]serverutils.TestServerInterface dataDirs map[string]string - sqlDBs map[string]*gosql.DB + sqlDBs map[sqlDBKey]*gosql.DB cleanupFns []func() } func (d *datadrivenTestState) cleanup(ctx context.Context) { + for _, db := range d.sqlDBs { + db.Close() + } for _, s := range d.servers { s.Stopper().Stop(ctx) } @@ -99,16 +107,23 @@ func (d *datadrivenTestState) cleanup(ctx context.Context) { } } -func (d *datadrivenTestState) addServer(t *testing.T, name, iodir string) { +func (d *datadrivenTestState) addServer( + t *testing.T, name, iodir string, allowImplicitAccess bool, +) { var tc serverutils.TestClusterInterface var cleanup func() + params := base.TestClusterArgs{} + if allowImplicitAccess { + params.ServerArgs.Knobs.BackupRestore = &sql.BackupRestoreTestingKnobs{ + AllowImplicitAccess: true, + } + } if iodir == "" { - _, tc, _, iodir, cleanup = BackupRestoreTestSetup(t, singleNode, 0, InitNone) + _, tc, _, iodir, cleanup = backupRestoreTestSetupWithParams(t, singleNode, 0, InitNone, params) } else { - _, tc, _, cleanup = backupRestoreTestSetupEmpty(t, singleNode, iodir, InitNone) + _, tc, _, cleanup = backupRestoreTestSetupEmptyWithParams(t, singleNode, iodir, InitNone, params) } d.servers[name] = tc.Server(0) - d.sqlDBs[name] = tc.ServerConn(0) d.dataDirs[name] = iodir d.cleanupFns = append(d.cleanupFns, cleanup) } @@ -121,11 +136,19 @@ func (d *datadrivenTestState) getIODir(t *testing.T, server string) string { return dir } -func (d *datadrivenTestState) getSQLDB(t *testing.T, server string) *gosql.DB { - db, ok := d.sqlDBs[server] - if !ok { - t.Fatalf("server %s does not exist", server) +func (d *datadrivenTestState) getSQLDB(t *testing.T, server string, user string) *gosql.DB { + key := sqlDBKey{server, user} + if db, ok := d.sqlDBs[key]; ok { + return db } + addr := d.servers[server].ServingSQLAddr() + pgURL, cleanup := sqlutils.PGUrl(t, addr, "TestBackupRestoreDataDriven", url.User(user)) + d.cleanupFns = append(d.cleanupFns, cleanup) + db, err := gosql.Open("postgres", pgURL.String()) + if err != nil { + t.Fatal(err) + } + d.sqlDBs[key] = db return db } @@ -133,7 +156,7 @@ func newDatadrivenTestState() datadrivenTestState { return datadrivenTestState{ servers: make(map[string]serverutils.TestServerInterface), dataDirs: make(map[string]string), - sqlDBs: make(map[string]*gosql.DB), + sqlDBs: make(map[sqlDBKey]*gosql.DB), } } @@ -169,6 +192,7 @@ func TestBackupRestoreDataDriven(t *testing.T) { return "" case "new-server": var name, shareDirWith, iodir string + var allowImplicitAccess bool d.ScanArgs(t, "name", &name) if d.HasArg("share-io-dir") { d.ScanArgs(t, "share-io-dir", &shareDirWith) @@ -176,29 +200,36 @@ func TestBackupRestoreDataDriven(t *testing.T) { if shareDirWith != "" { iodir = ds.getIODir(t, shareDirWith) } + if d.HasArg("allow-implicit-access") { + allowImplicitAccess = true + } lastCreatedServer = name - ds.addServer(t, name, iodir) + ds.addServer(t, name, iodir, allowImplicitAccess) return "" case "exec-sql": - var server string + server := lastCreatedServer + user := "root" if d.HasArg("server") { d.ScanArgs(t, "server", &server) - } else { - server = lastCreatedServer } - _, err := ds.getSQLDB(t, server).Exec(d.Input) + if d.HasArg("user") { + d.ScanArgs(t, "user", &user) + } + _, err := ds.getSQLDB(t, server, user).Exec(d.Input) if err == nil { return "" } return err.Error() case "query-sql": - var server string + server := lastCreatedServer + user := "root" if d.HasArg("server") { d.ScanArgs(t, "server", &server) - } else { - server = lastCreatedServer } - rows, err := ds.getSQLDB(t, server).Query(d.Input) + if d.HasArg("user") { + d.ScanArgs(t, "user", &user) + } + rows, err := ds.getSQLDB(t, server, user).Query(d.Input) if err != nil { return err.Error() } @@ -4462,11 +4493,6 @@ func TestBackupRestorePermissions(t *testing.T) { ) { t.Fatal(err) } - if _, err := testuser.Exec(`RESTORE blah FROM 'blah'`); !testutils.IsError( - err, "only users with the admin role are allowed to RESTORE", - ) { - t.Fatal(err) - } }) t.Run("privs-required", func(t *testing.T) { diff --git a/pkg/ccl/backupccl/restore_planning.go b/pkg/ccl/backupccl/restore_planning.go index d70a4c03823c..f2e0501b4a91 100644 --- a/pkg/ccl/backupccl/restore_planning.go +++ b/pkg/ccl/backupccl/restore_planning.go @@ -38,6 +38,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/privilege" + "github.com/cockroachdb/cockroach/pkg/sql/roleoption" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/sql/types" @@ -1150,10 +1151,6 @@ func restorePlanHook( ctx, span := tracing.ChildSpan(ctx, stmt.StatementTag()) defer tracing.FinishSpan(span) - if err := p.RequireAdminRole(ctx, "RESTORE"); err != nil { - return err - } - if !(p.ExtendedEvalContext().TxnImplicit || restoreStmt.Options.Detached) { return errors.Errorf("RESTORE cannot be used inside a transaction without DETACHED option") } @@ -1183,6 +1180,11 @@ func restorePlanHook( from[0][i] = parsed.String() } } + + if err := checkPrivilegesForRestore(ctx, restoreStmt, p, from); err != nil { + return err + } + var endTime hlc.Timestamp if restoreStmt.AsOf.Expr != nil { var err error @@ -1225,6 +1227,62 @@ func restorePlanHook( return fn, utilccl.BulkJobExecutionResultHeader, nil, false, nil } +func checkPrivilegesForRestore( + ctx context.Context, restoreStmt *tree.Restore, p sql.PlanHookState, from [][]string, +) error { + hasAdmin, err := p.HasAdminRole(ctx) + if err != nil { + return err + } + if hasAdmin { + return nil + } + // Do not allow full cluster restores. + if restoreStmt.DescriptorCoverage == tree.AllDescriptors { + return pgerror.Newf( + pgcode.InsufficientPrivilege, + "only users with the admin role are allowed to restore full cluster backups") + } + // Do not allow tenant restores. + if restoreStmt.Targets.Tenant != (roachpb.TenantID{}) { + return pgerror.Newf( + pgcode.InsufficientPrivilege, + "only users with the admin role can perform RESTORE TENANT") + } + // Database restores require the CREATEDB privileges. + if len(restoreStmt.Targets.Databases) > 0 { + hasCreateDB, err := p.HasRoleOption(ctx, roleoption.CREATEDB) + if err != nil { + return err + } + if !hasCreateDB { + return pgerror.Newf( + pgcode.InsufficientPrivilege, + "only users with the CREATEDB privilege can restore databases") + } + } + knobs := p.ExecCfg().BackupRestoreTestingKnobs + if knobs == nil || !knobs.AllowImplicitAccess { + // Check that none of the sources rely on implicit access. + for i := range from { + for j := range from[i] { + uri := from[i][j] + hasExplicitAuth, uriScheme, err := cloudimpl.AccessIsWithExplicitAuth(uri) + if err != nil { + return err + } + if !hasExplicitAuth { + return pgerror.Newf( + pgcode.InsufficientPrivilege, + "only users with the admin role are allowed to RESTORE from the specified %s URI", + uriScheme) + } + } + } + } + return nil +} + func doRestorePlan( ctx context.Context, restoreStmt *tree.Restore, diff --git a/pkg/ccl/backupccl/testdata/backup-restore/permissions b/pkg/ccl/backupccl/testdata/backup-restore/permissions new file mode 100644 index 000000000000..d44a7228ef6a --- /dev/null +++ b/pkg/ccl/backupccl/testdata/backup-restore/permissions @@ -0,0 +1,88 @@ +# Test permissions checks for non-admin users running RESTORE. +new-server name=s1 +---- + +exec-sql +CREATE DATABASE d; +CREATE TABLE d.t (x INT); +INSERT INTO d.t VALUES (1), (2), (3); +---- + +exec-sql +BACKUP TO 'nodelocal://0/test/' +---- + +# Start a new cluster with the same IO dir. +new-server name=s2 share-io-dir=s1 allow-implicit-access +---- + +exec-sql server=s2 +CREATE USER testuser +---- + +# Restore into the new cluster. +exec-sql server=s2 user=testuser +RESTORE FROM 'nodelocal://0/test/' +---- +pq: only users with the admin role are allowed to restore full cluster backups + +exec-sql server=s2 user=testuser +RESTORE DATABASE d FROM 'nodelocal://0/test/' +---- +pq: only users with the CREATEDB privilege can restore databases + +exec-sql server=s2 +CREATE DATABASE d +---- + +exec-sql server=s2 user=testuser +RESTORE TABLE d.t FROM 'nodelocal://0/test/' +---- +pq: user testuser does not have CREATE privilege on database d + +exec-sql server=s2 +GRANT CREATE ON DATABASE d TO testuser +---- + +exec-sql server=s2 user=testuser +RESTORE TABLE d.t FROM 'nodelocal://0/test/' +---- + +query-sql server=s2 +SELECT x FROM d.t ORDER BY x +---- +1 +2 +3 + +exec-sql server=s2 +DROP DATABASE d +---- + +exec-sql server=s2 +ALTER USER testuser CREATEDB +---- + +exec-sql server=s2 user=testuser +RESTORE DATABASE d FROM 'nodelocal://0/test/' +---- + +query-sql server=s2 +SELECT x FROM d.t ORDER BY x +---- +1 +2 +3 + +# Test that implicit access is disallowed when the testing knob isn't set. +new-server name=s3 share-io-dir=s1 +---- + +exec-sql server=s3 +CREATE USER testuser +---- + +exec-sql server=s3 user=testuser +RESTORE TABLE d.t FROM 'nodelocal://0/test/' +---- +pq: only users with the admin role are allowed to RESTORE from the specified nodelocal URI diff --git a/pkg/server/server_sql.go b/pkg/server/server_sql.go index 99de0c38763c..f431ffad945d 100644 --- a/pkg/server/server_sql.go +++ b/pkg/server/server_sql.go @@ -549,6 +549,9 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*sqlServer, error) { if tenantKnobs := cfg.TestingKnobs.TenantTestingKnobs; tenantKnobs != nil { execCfg.TenantTestingKnobs = tenantKnobs.(*sql.TenantTestingKnobs) } + if backupRestoreKnobs := cfg.TestingKnobs.BackupRestore; backupRestoreKnobs != nil { + execCfg.BackupRestoreTestingKnobs = backupRestoreKnobs.(*sql.BackupRestoreTestingKnobs) + } statsRefresher := stats.MakeRefresher( cfg.Settings, diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index e054da762c6d..8968680d6b0b 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -673,6 +673,7 @@ type ExecutorConfig struct { DistSQLRunTestingKnobs *execinfra.TestingKnobs EvalContextTestingKnobs tree.EvalContextTestingKnobs TenantTestingKnobs *TenantTestingKnobs + BackupRestoreTestingKnobs *BackupRestoreTestingKnobs // HistogramWindowInterval is (server.Config).HistogramWindowInterval. HistogramWindowInterval time.Duration @@ -845,6 +846,18 @@ func (k *TenantTestingKnobs) CanSetClusterSettings() bool { return k != nil && k.ClusterSettingsUpdater != nil } +// BackupRestoreTestingKnobs contains knobs for backup and restore behavior. +type BackupRestoreTestingKnobs struct { + // AllowImplicitAccess allows implicit access to data sources for non-admin + // users. This enables using nodelocal for testing RESTORE permissions. + AllowImplicitAccess bool +} + +var _ base.ModuleTestingKnobs = &BackupRestoreTestingKnobs{} + +// ModuleTestingKnobs implements the base.ModuleTestingKnobs interface. +func (*BackupRestoreTestingKnobs) ModuleTestingKnobs() {} + // databaseCacheHolder is a thread-safe container for a *Cache. // It also allows clients to block until the cache is updated to a desired // state. From a06c7c12845100c652644da17b6fc081261890b2 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Mon, 24 Aug 2020 11:41:19 +0200 Subject: [PATCH 02/11] pgwire: expose `root` and `node` client login names as non-redactable Release note: None Release justification: low risk, high benefit changes to existing functionality --- pkg/sql/pgwire/conn.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/sql/pgwire/conn.go b/pkg/sql/pgwire/conn.go index bf7819dabb08..3b29432d7d3d 100644 --- a/pkg/sql/pgwire/conn.go +++ b/pkg/sql/pgwire/conn.go @@ -213,7 +213,11 @@ func (c *conn) serveImpl( ) { defer func() { _ = c.conn.Close() }() - ctx = logtags.AddTag(ctx, "user", c.sessionArgs.User) + if c.sessionArgs.User == security.RootUser || c.sessionArgs.User == security.NodeUser { + ctx = logtags.AddTag(ctx, "user", log.Safe(c.sessionArgs.User)) + } else { + ctx = logtags.AddTag(ctx, "user", c.sessionArgs.User) + } inTestWithoutSQL := sqlServer == nil var authLogger *log.SecondaryLogger From 548beb56704e0784504554ca71e08ffed6775ddf Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Mon, 24 Aug 2020 11:43:52 +0200 Subject: [PATCH 03/11] kvserver: expose gossip functions as non-redactable Release note: None Release justification: low risk, high benefit changes to existing functionality --- pkg/kv/kvserver/store.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/kv/kvserver/store.go b/pkg/kv/kvserver/store.go index 55d4b3b4ba5e..371768dd0230 100644 --- a/pkg/kv/kvserver/store.go +++ b/pkg/kv/kvserver/store.go @@ -71,6 +71,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/uuid" "github.com/cockroachdb/errors" "github.com/cockroachdb/logtags" + "github.com/cockroachdb/redact" "github.com/google/btree" "go.etcd.io/etcd/raft" "golang.org/x/time/rate" @@ -1649,7 +1650,7 @@ func (s *Store) startGossip() { gossipFns := []struct { key roachpb.Key fn func(context.Context, *Replica) error - description string + description redact.SafeString interval time.Duration }{ { From 740dcd35da4ca90538fb4a66894c85b87567f77a Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Mon, 24 Aug 2020 11:54:29 +0200 Subject: [PATCH 04/11] roachpb,server: make store initialization messages non-redactable Release note: None Release justification: low risk, high benefit changes to existing functionality --- pkg/kv/kvserver/store.go | 7 ++++++- pkg/roachpb/metadata.go | 4 ++-- pkg/server/node.go | 7 ++++++- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/pkg/kv/kvserver/store.go b/pkg/kv/kvserver/store.go index 371768dd0230..cee494fa3bfa 100644 --- a/pkg/kv/kvserver/store.go +++ b/pkg/kv/kvserver/store.go @@ -1001,7 +1001,12 @@ func NewStore( // String formats a store for debug output. func (s *Store) String() string { - return fmt.Sprintf("[n%d,s%d]", s.Ident.NodeID, s.Ident.StoreID) + return redact.StringWithoutMarkers(s) +} + +// SafeFormat implements the redact.SafeFormatter interface. +func (s *Store) SafeFormat(w redact.SafePrinter, _ rune) { + w.Printf("[n%d,s%d]", s.Ident.NodeID, s.Ident.StoreID) } // ClusterSettings returns the node's ClusterSettings. diff --git a/pkg/roachpb/metadata.go b/pkg/roachpb/metadata.go index c0155782f66e..85de2a93f896 100644 --- a/pkg/roachpb/metadata.go +++ b/pkg/roachpb/metadata.go @@ -441,8 +441,8 @@ func (sc StoreCapacity) SafeFormat(w redact.SafePrinter, _ rune) { w.Printf("disk (capacity=%s, available=%s, used=%s, logicalBytes=%s), "+ "ranges=%d, leases=%d, queries=%.2f, writes=%.2f, "+ "bytesPerReplica={%s}, writesPerReplica={%s}", - humanizeutil.IBytes(sc.Capacity), humanizeutil.IBytes(sc.Available), - humanizeutil.IBytes(sc.Used), humanizeutil.IBytes(sc.LogicalBytes), + redact.Safe(humanizeutil.IBytes(sc.Capacity)), redact.Safe(humanizeutil.IBytes(sc.Available)), + redact.Safe(humanizeutil.IBytes(sc.Used)), redact.Safe(humanizeutil.IBytes(sc.LogicalBytes)), sc.RangeCount, sc.LeaseCount, sc.QueriesPerSecond, sc.WritesPerSecond, sc.BytesPerReplica, sc.WritesPerReplica) } diff --git a/pkg/server/node.go b/pkg/server/node.go index 9cd686710ba2..a1de9c0ece5b 100644 --- a/pkg/server/node.go +++ b/pkg/server/node.go @@ -48,6 +48,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/uuid" "github.com/cockroachdb/errors" "github.com/cockroachdb/logtags" + "github.com/cockroachdb/redact" opentracing "github.com/opentracing/opentracing-go" "google.golang.org/grpc/codes" grpcstatus "google.golang.org/grpc/status" @@ -476,7 +477,11 @@ func (n *Node) start( allEngines := append([]storage.Engine(nil), state.initializedEngines...) allEngines = append(allEngines, state.newEngines...) - log.Infof(ctx, "%s: started with %v engine(s) and attributes %v", n, allEngines, attrs.Attrs) + for _, e := range allEngines { + t := e.Type() + log.Infof(ctx, "started with engine type %v", t) + } + log.Infof(ctx, "started with attributes %v", attrs.Attrs) return nil } From cad8b3f23a8ec2d58b46ca9c10c00b091758c46c Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Mon, 24 Aug 2020 12:00:39 +0200 Subject: [PATCH 05/11] server: make more server start-up messages non-redactable Release note: None Release justification: low risk, high benefit changes to existing functionality --- pkg/cli/start.go | 72 +++++++++++++++++++++------------- pkg/server/server.go | 5 ++- pkg/storage/enginepb/engine.go | 22 +++++++---- 3 files changed, 63 insertions(+), 36 deletions(-) diff --git a/pkg/cli/start.go b/pkg/cli/start.go index b0a513b9fd6d..956a25eb51a1 100644 --- a/pkg/cli/start.go +++ b/pkg/cli/start.go @@ -53,6 +53,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/cockroach/pkg/util/tracing" "github.com/cockroachdb/errors" + "github.com/cockroachdb/redact" opentracing "github.com/opentracing/opentracing-go" "github.com/spf13/cobra" "google.golang.org/grpc" @@ -604,12 +605,12 @@ If problems persist, please see %s.` // Now inform the user that the server is running and tell the // user about its run-time derived parameters. - var buf bytes.Buffer + var buf redact.StringBuilder info := build.GetInfo() - tw := tabwriter.NewWriter(&buf, 2, 1, 2, ' ', 0) - fmt.Fprintf(tw, "CockroachDB node starting at %s (took %0.1fs)\n", timeutil.Now(), timeutil.Since(tBegin).Seconds()) - fmt.Fprintf(tw, "build:\t%s %s @ %s (%s)\n", info.Distribution, info.Tag, info.Time, info.GoVersion) - fmt.Fprintf(tw, "webui:\t%s\n", serverCfg.AdminURL()) + buf.Printf("CockroachDB node starting at %s (took %0.1fs)\n", timeutil.Now(), timeutil.Since(tBegin).Seconds()) + buf.Printf("build:\t%s %s @ %s (%s)\n", + redact.Safe(info.Distribution), redact.Safe(info.Tag), redact.Safe(info.Time), redact.Safe(info.GoVersion)) + buf.Printf("webui:\t%s\n", serverCfg.AdminURL()) // (Re-)compute the client connection URL. We cannot do this // earlier (e.g. above, in the runStart function) because @@ -620,63 +621,64 @@ If problems persist, please see %s.` log.Errorf(ctx, "failed computing the URL: %v", err) return err } - fmt.Fprintf(tw, "sql:\t%s\n", pgURL) + buf.Printf("sql:\t%s\n", pgURL) - fmt.Fprintf(tw, "RPC client flags:\t%s\n", clientFlagsRPC()) + buf.Printf("RPC client flags:\t%s\n", clientFlagsRPC()) if len(serverCfg.SocketFile) != 0 { - fmt.Fprintf(tw, "socket:\t%s\n", serverCfg.SocketFile) + buf.Printf("socket:\t%s\n", serverCfg.SocketFile) } - fmt.Fprintf(tw, "logs:\t%s\n", flag.Lookup("log-dir").Value) + buf.Printf("logs:\t%s\n", flag.Lookup("log-dir").Value) if serverCfg.AuditLogDirName.IsSet() { - fmt.Fprintf(tw, "SQL audit logs:\t%s\n", serverCfg.AuditLogDirName) + buf.Printf("SQL audit logs:\t%s\n", serverCfg.AuditLogDirName) } if serverCfg.Attrs != "" { - fmt.Fprintf(tw, "attrs:\t%s\n", serverCfg.Attrs) + buf.Printf("attrs:\t%s\n", serverCfg.Attrs) } if len(serverCfg.Locality.Tiers) > 0 { - fmt.Fprintf(tw, "locality:\t%s\n", serverCfg.Locality) + buf.Printf("locality:\t%s\n", serverCfg.Locality) } if s.TempDir() != "" { - fmt.Fprintf(tw, "temp dir:\t%s\n", s.TempDir()) + buf.Printf("temp dir:\t%s\n", s.TempDir()) } if ext := s.ClusterSettings().ExternalIODir; ext != "" { - fmt.Fprintf(tw, "external I/O path: \t%s\n", ext) + buf.Printf("external I/O path: \t%s\n", ext) } else { - fmt.Fprintf(tw, "external I/O path: \t\n") + buf.Printf("external I/O path: \t\n") } for i, spec := range serverCfg.Stores.Specs { - fmt.Fprintf(tw, "store[%d]:\t%s\n", i, spec) + buf.Printf("store[%d]:\t%s\n", i, spec) } - fmt.Fprintf(tw, "storage engine: \t%s\n", serverCfg.StorageEngine.String()) + buf.Printf("storage engine: \t%s\n", &serverCfg.StorageEngine) nodeID := s.NodeID() if initialStart { if nodeID == server.FirstNodeID { - fmt.Fprintf(tw, "status:\tinitialized new cluster\n") + buf.Printf("status:\tinitialized new cluster\n") } else { - fmt.Fprintf(tw, "status:\tinitialized new node, joined pre-existing cluster\n") + buf.Printf("status:\tinitialized new node, joined pre-existing cluster\n") } } else { - fmt.Fprintf(tw, "status:\trestarted pre-existing node\n") + buf.Printf("status:\trestarted pre-existing node\n") } if baseCfg.ClusterName != "" { - fmt.Fprintf(tw, "cluster name:\t%s\n", baseCfg.ClusterName) + buf.Printf("cluster name:\t%s\n", baseCfg.ClusterName) } // Remember the cluster ID for log file rotation. clusterID := s.ClusterID().String() log.SetClusterID(clusterID) - fmt.Fprintf(tw, "clusterID:\t%s\n", clusterID) - fmt.Fprintf(tw, "nodeID:\t%d\n", nodeID) + buf.Printf("clusterID:\t%s\n", clusterID) + buf.Printf("nodeID:\t%d\n", nodeID) // Collect the formatted string and show it to the user. - if err := tw.Flush(); err != nil { + msg, err := expandTabsInRedactableBytes(buf.RedactableBytes()) + if err != nil { return err } - msg := buf.String() - log.Infof(ctx, "node startup completed:\n%s", msg) + msgS := msg.ToString() + log.Infof(ctx, "node startup completed:\n%s", msgS) if !startCtx.inBackground && !log.LoggingToStderr(log.Severity_INFO) { - fmt.Print(msg) + fmt.Print(msgS.StripMarkers()) } return nil @@ -877,6 +879,22 @@ If problems persist, please see %s.` return returnErr } +// expandTabsInRedactableBytes expands tabs in the redactable byte +// slice, so that columns are aligned. The correctness of this +// function depends on the assumption that the `tabwriter` does not +// replace characters. +func expandTabsInRedactableBytes(s redact.RedactableBytes) (redact.RedactableBytes, error) { + var buf bytes.Buffer + tw := tabwriter.NewWriter(&buf, 2, 1, 2, ' ', 0) + if _, err := tw.Write([]byte(s)); err != nil { + return nil, err + } + if err := tw.Flush(); err != nil { + return nil, err + } + return redact.RedactableBytes(buf.Bytes()), nil +} + func hintServerCmdFlags(ctx context.Context, cmd *cobra.Command) { pf := flagSetForCmd(cmd) diff --git a/pkg/server/server.go b/pkg/server/server.go index 586bc9eaef8d..1ada30e96705 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -81,6 +81,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/uuid" "github.com/cockroachdb/errors" "github.com/cockroachdb/logtags" + "github.com/cockroachdb/redact" "github.com/cockroachdb/sentry-go" gwruntime "github.com/grpc-ecosystem/grpc-gateway/runtime" "github.com/opentracing/opentracing-go" @@ -1541,8 +1542,8 @@ func (s *Server) Start(ctx context.Context) error { s.grpc.setMode(modeOperational) log.Infof(ctx, "starting %s server at %s (use: %s)", - s.cfg.HTTPRequestScheme(), s.cfg.HTTPAddr, s.cfg.HTTPAdvertiseAddr) - rpcConnType := "grpc/postgres" + redact.Safe(s.cfg.HTTPRequestScheme()), s.cfg.HTTPAddr, s.cfg.HTTPAdvertiseAddr) + rpcConnType := redact.SafeString("grpc/postgres") if s.cfg.SplitListenSQL { rpcConnType = "grpc" log.Infof(ctx, "starting postgres server at %s (use: %s)", s.cfg.SQLAddr, s.cfg.SQLAdvertiseAddr) diff --git a/pkg/storage/enginepb/engine.go b/pkg/storage/enginepb/engine.go index b01f5e5307e8..da33c0832529 100644 --- a/pkg/storage/enginepb/engine.go +++ b/pkg/storage/enginepb/engine.go @@ -10,24 +10,32 @@ package enginepb -import "fmt" +import ( + "fmt" + + "github.com/cockroachdb/redact" +) // Type implements the pflag.Value interface. func (e *EngineType) Type() string { return "string" } // String implements the pflag.Value interface. -func (e *EngineType) String() string { +func (e *EngineType) String() string { return redact.StringWithoutMarkers(e) } + +// SafeFormat implements the refact.SafeFormatter interface. +func (e *EngineType) SafeFormat(p redact.SafePrinter, _ rune) { switch *e { case EngineTypeRocksDB: - return "rocksdb" + p.SafeString("rocksdb") case EngineTypeDefault: - return "default" + p.SafeString("default") case EngineTypePebble: - return "pebble" + p.SafeString("pebble") case EngineTypeTeePebbleRocksDB: - return "pebble+rocksdb" + p.SafeString("pebble+rocksdb") + default: + p.Printf("", int32(*e)) } - return "" } // Set implements the pflag.Value interface. From 2b28a31b69936384701ccbd259ec8838820592b3 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Tue, 25 Aug 2020 17:19:41 +0200 Subject: [PATCH 06/11] jobs: make a duration safe for reporting Release note: None Release justification: low risk, high benefit changes to existing functionality --- pkg/jobs/job_scheduler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/jobs/job_scheduler.go b/pkg/jobs/job_scheduler.go index d2605ed3c4da..9f90ce8f2983 100644 --- a/pkg/jobs/job_scheduler.go +++ b/pkg/jobs/job_scheduler.go @@ -333,7 +333,7 @@ func (s *jobScheduler) executeSchedules( func (s *jobScheduler) runDaemon(ctx context.Context, stopper *stop.Stopper) { stopper.RunWorker(ctx, func(ctx context.Context) { initialDelay := getInitialScanDelay(s.TestingKnobs) - log.Infof(ctx, "waiting %s before scheduled jobs daemon start", initialDelay.String()) + log.Infof(ctx, "waiting %v before scheduled jobs daemon start", initialDelay) for timer := time.NewTimer(initialDelay); ; timer.Reset( getWaitPeriod(&s.Settings.SV, s.TestingKnobs)) { From fe1116bdb0382bbb6363f1f5c5518efd463c298e Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Tue, 25 Aug 2020 17:25:52 +0200 Subject: [PATCH 07/11] sql: make schema IDs safe for reporting Release note: None Release justification: low risk, high benefit changes to existing functionality --- docs/generated/redact_safe.md | 3 +++ pkg/sql/catalog/descpb/structured.go | 3 +++ pkg/sql/sem/tree/table_ref.go | 6 ++++++ 3 files changed, 12 insertions(+) diff --git a/docs/generated/redact_safe.md b/docs/generated/redact_safe.md index 4e4fb8352ae5..ce17afc60428 100644 --- a/docs/generated/redact_safe.md +++ b/docs/generated/redact_safe.md @@ -10,6 +10,9 @@ pkg/roachpb/metadata.go | `RangeID` pkg/roachpb/metadata.go | `ReplicaID` pkg/roachpb/metadata.go | `RangeGeneration` pkg/roachpb/metadata.go | `ReplicaType` +pkg/sql/catalog/descpb/structured.go | `ID` +pkg/sql/sem/tree/table_ref.go | `ID` +pkg/sql/sem/tree/table_ref.go | `ColumnID` pkg/util/hlc/timestamp.go | `Timestamp` pkg/util/log/redact.go | `reflect.TypeOf(true)` pkg/util/log/redact.go | `reflect.TypeOf(123)` diff --git a/pkg/sql/catalog/descpb/structured.go b/pkg/sql/catalog/descpb/structured.go index e80e8af7fe50..b27d1f5b39b5 100644 --- a/pkg/sql/catalog/descpb/structured.go +++ b/pkg/sql/catalog/descpb/structured.go @@ -37,6 +37,9 @@ func (dir IndexDescriptor_Direction) ToEncodingDirection() (encoding.Direction, // ID is a custom type for {Database,Table}Descriptor IDs. type ID tree.ID +// SafeValue implements the redact.SafeValue interface. +func (ID) SafeValue() {} + // InvalidID is the uninitialised descriptor id. const InvalidID ID = 0 diff --git a/pkg/sql/sem/tree/table_ref.go b/pkg/sql/sem/tree/table_ref.go index 74a946640b78..db4ac7a49dc2 100644 --- a/pkg/sql/sem/tree/table_ref.go +++ b/pkg/sql/sem/tree/table_ref.go @@ -56,3 +56,9 @@ func (n *TableRef) String() string { return AsString(n) } // tableExpr implements the TableExpr interface. func (n *TableRef) tableExpr() {} + +// SafeValue implements the redact.SafeValue interface. +func (ID) SafeValue() {} + +// SafeValue implements the redact.SafeValue interface. +func (ColumnID) SafeValue() {} From c95d0cfd3e9d8440df8dce5e01c6a4b7c5dc0acb Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Tue, 25 Aug 2020 17:26:12 +0200 Subject: [PATCH 08/11] jobs: make job types safe for reporting Release note: None Release justification: low risk, high benefit changes to existing functionality --- docs/generated/redact_safe.md | 1 + pkg/jobs/jobspb/wrap.go | 3 +++ pkg/jobs/registry.go | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/generated/redact_safe.md b/docs/generated/redact_safe.md index ce17afc60428..2f7f606805e8 100644 --- a/docs/generated/redact_safe.md +++ b/docs/generated/redact_safe.md @@ -2,6 +2,7 @@ The following types are considered always safe for reporting: File | Type --|-- +pkg/jobs/jobspb/wrap.go | `Type` pkg/kv/kvserver/raft.go | `SnapshotRequest_Type` pkg/roachpb/data.go | `ReplicaChangeType` pkg/roachpb/metadata.go | `NodeID` diff --git a/pkg/jobs/jobspb/wrap.go b/pkg/jobs/jobspb/wrap.go index 70622df98de9..ed5994701245 100644 --- a/pkg/jobs/jobspb/wrap.go +++ b/pkg/jobs/jobspb/wrap.go @@ -214,3 +214,6 @@ const ( // Silence unused warning. _ = BaseFormatVersion ) + +// SafeValue implements the redact.SafeValue interface. +func (Type) SafeValue() {} diff --git a/pkg/jobs/registry.go b/pkg/jobs/registry.go index e9b3044d9350..54d231cf4c13 100644 --- a/pkg/jobs/registry.go +++ b/pkg/jobs/registry.go @@ -1047,7 +1047,7 @@ func (r *Registry) stepThroughStateMachine( jobErr error, ) error { payload := job.Payload() - jobType := payload.Type().String() + jobType := payload.Type() log.Infof(ctx, "%s job %d: stepping through state %s with error: %+v", jobType, *job.ID(), status, jobErr) switch status { case StatusRunning: From 4f2417d164de3fc6a6aa7212869e7dc9beeb79d5 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Tue, 25 Aug 2020 17:33:10 +0200 Subject: [PATCH 09/11] server: make drain details non-redactable Release note: None Release justification: low risk, high benefit changes to existing functionality --- pkg/kv/kvserver/node_liveness.go | 10 ++++++++-- pkg/kv/kvserver/store.go | 2 +- pkg/server/drain.go | 24 +++++++++++++----------- pkg/server/node.go | 2 +- pkg/sql/catalog/lease/lease.go | 3 ++- pkg/sql/distsql/server.go | 3 ++- pkg/sql/flowinfra/flow_registry.go | 5 ++++- pkg/sql/pgwire/server.go | 5 +++-- 8 files changed, 34 insertions(+), 20 deletions(-) diff --git a/pkg/kv/kvserver/node_liveness.go b/pkg/kv/kvserver/node_liveness.go index 5e3754dac853..da0e2bf81b4e 100644 --- a/pkg/kv/kvserver/node_liveness.go +++ b/pkg/kv/kvserver/node_liveness.go @@ -36,6 +36,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/cockroach/pkg/util/tracing" "github.com/cockroachdb/errors" + "github.com/cockroachdb/redact" ) var ( @@ -253,7 +254,9 @@ func (nl *NodeLiveness) sem(nodeID roachpb.NodeID) chan struct{} { // to report work that needed to be done and which may or may not have // been done by the time this call returns. See the explanation in // pkg/server/drain.go for details. -func (nl *NodeLiveness) SetDraining(ctx context.Context, drain bool, reporter func(int, string)) { +func (nl *NodeLiveness) SetDraining( + ctx context.Context, drain bool, reporter func(int, redact.SafeString), +) { ctx = nl.ambientCtx.AnnotateCtx(ctx) for r := retry.StartWithCtx(ctx, base.DefaultRetryOptions()); r.Next(); { oldLivenessRec, err := nl.SelfEx() @@ -359,7 +362,10 @@ func (nl *NodeLiveness) SetMembershipStatus( } func (nl *NodeLiveness) setDrainingInternal( - ctx context.Context, oldLivenessRec LivenessRecord, drain bool, reporter func(int, string), + ctx context.Context, + oldLivenessRec LivenessRecord, + drain bool, + reporter func(int, redact.SafeString), ) error { nodeID := nl.gossip.NodeID.Get() sem := nl.sem(nodeID) diff --git a/pkg/kv/kvserver/store.go b/pkg/kv/kvserver/store.go index cee494fa3bfa..aa60a2c13dde 100644 --- a/pkg/kv/kvserver/store.go +++ b/pkg/kv/kvserver/store.go @@ -1028,7 +1028,7 @@ func (s *Store) AnnotateCtx(ctx context.Context) context.Context { // to report work that needed to be done and which may or may not have // been done by the time this call returns. See the explanation in // pkg/server/drain.go for details. -func (s *Store) SetDraining(drain bool, reporter func(int, string)) { +func (s *Store) SetDraining(drain bool, reporter func(int, redact.SafeString)) { s.draining.Store(drain) if !drain { newStoreReplicaVisitor(s).Visit(func(r *Replica) bool { diff --git a/pkg/server/drain.go b/pkg/server/drain.go index 3f886010202b..971dc69daba5 100644 --- a/pkg/server/drain.go +++ b/pkg/server/drain.go @@ -12,7 +12,6 @@ package server import ( "context" - "fmt" "os" "reflect" "strings" @@ -23,6 +22,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/cockroachdb/errors" + "github.com/cockroachdb/redact" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -81,7 +81,7 @@ func (s *adminServer) Drain(req *serverpb.DrainRequest, stream serverpb.Admin_Dr return err } res.DrainRemainingIndicator = remaining - res.DrainRemainingDescription = info + res.DrainRemainingDescription = info.StripMarkers() } if s.server.isDraining() { res.DeprecatedDrainStatus = DeprecatedDrainParameter @@ -150,10 +150,12 @@ func (s *adminServer) Drain(req *serverpb.DrainRequest, stream serverpb.Admin_Dr // TODO(knz): This method is currently exported for use by the // shutdown code in cli/start.go; however, this is a mis-design. The // start code should use the Drain() RPC like quit does. -func (s *Server) Drain(ctx context.Context) (remaining uint64, info string, err error) { - reports := make(map[string]int) +func (s *Server) Drain( + ctx context.Context, +) (remaining uint64, info redact.RedactableString, err error) { + reports := make(map[redact.SafeString]int) var mu syncutil.Mutex - reporter := func(howMany int, what string) { + reporter := func(howMany int, what redact.SafeString) { if howMany > 0 { mu.Lock() reports[what] += howMany @@ -163,13 +165,13 @@ func (s *Server) Drain(ctx context.Context) (remaining uint64, info string, err defer func() { // Detail the counts based on the collected reports. var descBuf strings.Builder - comma := "" + comma := redact.SafeString("") for what, howMany := range reports { remaining += uint64(howMany) - fmt.Fprintf(&descBuf, "%s%s: %d", comma, what, howMany) + redact.Fprintf(&descBuf, "%s%s: %d", comma, what, howMany) comma = ", " } - info = descBuf.String() + info = redact.RedactableString(descBuf.String()) log.Infof(ctx, "drain remaining: %d", remaining) if info != "" { log.Infof(ctx, "drain details: %s", info) @@ -183,7 +185,7 @@ func (s *Server) Drain(ctx context.Context) (remaining uint64, info string, err return } -func (s *Server) doDrain(ctx context.Context, reporter func(int, string)) error { +func (s *Server) doDrain(ctx context.Context, reporter func(int, redact.SafeString)) error { // First drain all clients and SQL leases. if err := s.drainClients(ctx, reporter); err != nil { return err @@ -200,7 +202,7 @@ func (s *Server) isDraining() bool { } // drainClients starts draining the SQL layer. -func (s *Server) drainClients(ctx context.Context, reporter func(int, string)) error { +func (s *Server) drainClients(ctx context.Context, reporter func(int, redact.SafeString)) error { // Mark the server as draining in a way that probes to // /health?ready=1 will notice. s.grpc.setMode(modeDraining) @@ -226,7 +228,7 @@ func (s *Server) drainClients(ctx context.Context, reporter func(int, string)) e // drainNode initiates the draining mode for the node, which // starts draining range leases. -func (s *Server) drainNode(ctx context.Context, reporter func(int, string)) error { +func (s *Server) drainNode(ctx context.Context, reporter func(int, redact.SafeString)) error { s.nodeLiveness.SetDraining(ctx, true /* drain */, reporter) return s.node.SetDraining(true /* drain */, reporter) } diff --git a/pkg/server/node.go b/pkg/server/node.go index a1de9c0ece5b..4d0230e40501 100644 --- a/pkg/server/node.go +++ b/pkg/server/node.go @@ -503,7 +503,7 @@ func (n *Node) IsDraining() bool { // to report work that needed to be done and which may or may not have // been done by the time this call returns. See the explanation in // pkg/server/drain.go for details. -func (n *Node) SetDraining(drain bool, reporter func(int, string)) error { +func (n *Node) SetDraining(drain bool, reporter func(int, redact.SafeString)) error { return n.stores.VisitStores(func(s *kvserver.Store) error { s.SetDraining(drain, reporter) return nil diff --git a/pkg/sql/catalog/lease/lease.go b/pkg/sql/catalog/lease/lease.go index 6b4fd847b7d3..3850efc1f1a5 100644 --- a/pkg/sql/catalog/lease/lease.go +++ b/pkg/sql/catalog/lease/lease.go @@ -50,6 +50,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/tracing" "github.com/cockroachdb/errors" "github.com/cockroachdb/logtags" + "github.com/cockroachdb/redact" ) var errRenewLease = errors.New("renew lease on id") @@ -1780,7 +1781,7 @@ func (m *Manager) isDraining() bool { // to report work that needed to be done and which may or may not have // been done by the time this call returns. See the explanation in // pkg/server/drain.go for details. -func (m *Manager) SetDraining(drain bool, reporter func(int, string)) { +func (m *Manager) SetDraining(drain bool, reporter func(int, redact.SafeString)) { m.draining.Store(drain) if !drain { return diff --git a/pkg/sql/distsql/server.go b/pkg/sql/distsql/server.go index 35a3416cf38e..540a9777c328 100644 --- a/pkg/sql/distsql/server.go +++ b/pkg/sql/distsql/server.go @@ -41,6 +41,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/tracing" "github.com/cockroachdb/errors" "github.com/cockroachdb/logtags" + "github.com/cockroachdb/redact" "github.com/opentracing/opentracing-go" ) @@ -119,7 +120,7 @@ func (ds *ServerImpl) Start() { // Drain changes the node's draining state through gossip and drains the // server's flowRegistry. See flowRegistry.Drain for more details. func (ds *ServerImpl) Drain( - ctx context.Context, flowDrainWait time.Duration, reporter func(int, string), + ctx context.Context, flowDrainWait time.Duration, reporter func(int, redact.SafeString), ) { if err := ds.setDraining(true); err != nil { log.Warningf(ctx, "unable to gossip distsql draining state: %s", err) diff --git a/pkg/sql/flowinfra/flow_registry.go b/pkg/sql/flowinfra/flow_registry.go index 328681cef4f5..b626d40f828f 100644 --- a/pkg/sql/flowinfra/flow_registry.go +++ b/pkg/sql/flowinfra/flow_registry.go @@ -24,6 +24,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/errors" + "github.com/cockroachdb/redact" "github.com/opentracing/opentracing-go" ) @@ -362,7 +363,9 @@ func (fr *FlowRegistry) waitForFlowLocked( // been done by the time this call returns. See the explanation in // pkg/server/drain.go for details. func (fr *FlowRegistry) Drain( - flowDrainWait time.Duration, minFlowDrainWait time.Duration, reporter func(int, string), + flowDrainWait time.Duration, + minFlowDrainWait time.Duration, + reporter func(int, redact.SafeString), ) { allFlowsDone := make(chan struct{}, 1) start := timeutil.Now() diff --git a/pkg/sql/pgwire/server.go b/pkg/sql/pgwire/server.go index 7a5480dd1f7c..4539bf7fe57c 100644 --- a/pkg/sql/pgwire/server.go +++ b/pkg/sql/pgwire/server.go @@ -42,6 +42,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/errors" "github.com/cockroachdb/logtags" + "github.com/cockroachdb/redact" ) // ATTENTION: After changing this value in a unit test, you probably want to @@ -319,7 +320,7 @@ func (s *Server) Metrics() (res []interface{}) { // to report work that needed to be done and which may or may not have // been done by the time this call returns. See the explanation in // pkg/server/drain.go for details. -func (s *Server) Drain(drainWait time.Duration, reporter func(int, string)) error { +func (s *Server) Drain(drainWait time.Duration, reporter func(int, redact.SafeString)) error { return s.drainImpl(drainWait, cancelMaxWait, reporter) } @@ -354,7 +355,7 @@ func (s *Server) setDrainingLocked(drain bool) bool { // been done by the time this call returns. See the explanation in // pkg/server/drain.go for details. func (s *Server) drainImpl( - drainWait time.Duration, cancelWait time.Duration, reporter func(int, string), + drainWait time.Duration, cancelWait time.Duration, reporter func(int, redact.SafeString), ) error { // This anonymous function returns a copy of s.mu.connCancelMap if there are // any active connections to cancel. We will only attempt to cancel From a1325f38d23bcbfb56c34f2de2997f2ec1d2b31a Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Wed, 2 Sep 2020 12:58:18 +0200 Subject: [PATCH 10/11] descpb: opt more simple integer types into safe reporting Release justification: low risk, high benefit changes to existing functionality Release note: None --- docs/generated/redact_safe.md | 6 ++++++ pkg/sql/catalog/descpb/structured.go | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/docs/generated/redact_safe.md b/docs/generated/redact_safe.md index 2f7f606805e8..b00252c29283 100644 --- a/docs/generated/redact_safe.md +++ b/docs/generated/redact_safe.md @@ -12,6 +12,12 @@ pkg/roachpb/metadata.go | `ReplicaID` pkg/roachpb/metadata.go | `RangeGeneration` pkg/roachpb/metadata.go | `ReplicaType` pkg/sql/catalog/descpb/structured.go | `ID` +pkg/sql/catalog/descpb/structured.go | `FamilyID` +pkg/sql/catalog/descpb/structured.go | `IndexID` +pkg/sql/catalog/descpb/structured.go | `DescriptorVersion` +pkg/sql/catalog/descpb/structured.go | `IndexDescriptorVersion` +pkg/sql/catalog/descpb/structured.go | `ColumnID` +pkg/sql/catalog/descpb/structured.go | `MutationID` pkg/sql/sem/tree/table_ref.go | `ID` pkg/sql/sem/tree/table_ref.go | `ColumnID` pkg/util/hlc/timestamp.go | `Timestamp` diff --git a/pkg/sql/catalog/descpb/structured.go b/pkg/sql/catalog/descpb/structured.go index b27d1f5b39b5..e6dfe9805943 100644 --- a/pkg/sql/catalog/descpb/structured.go +++ b/pkg/sql/catalog/descpb/structured.go @@ -71,15 +71,27 @@ const ( // FamilyID is a custom type for ColumnFamilyDescriptor IDs. type FamilyID uint32 +// SafeValue implements the redact.SafeValue interface. +func (FamilyID) SafeValue() {} + // IndexID is a custom type for IndexDescriptor IDs. type IndexID tree.IndexID +// SafeValue implements the redact.SafeValue interface. +func (IndexID) SafeValue() {} + // DescriptorVersion is a custom type for TableDescriptor Versions. type DescriptorVersion uint32 +// SafeValue implements the redact.SafeValue interface. +func (DescriptorVersion) SafeValue() {} + // IndexDescriptorVersion is a custom type for IndexDescriptor Versions. type IndexDescriptorVersion uint32 +// SafeValue implements the redact.SafeValue interface. +func (IndexDescriptorVersion) SafeValue() {} + const ( // BaseIndexFormatVersion corresponds to the original encoding of secondary indexes that // don't respect table level column family definitions. We allow the 0 value of the type to @@ -93,6 +105,9 @@ const ( // ColumnID is a custom type for ColumnDescriptor IDs. type ColumnID tree.ColumnID +// SafeValue implements the redact.SafeValue interface. +func (ColumnID) SafeValue() {} + // ColumnIDs is a slice of ColumnDescriptor IDs. type ColumnIDs []ColumnID @@ -156,6 +171,9 @@ var _ = SecondaryIndexEncoding // MutationID is a custom type for TableDescriptor mutations. type MutationID uint32 +// SafeValue implements the redact.SafeValue interface. +func (MutationID) SafeValue() {} + // InvalidMutationID is the uninitialised mutation id. const InvalidMutationID MutationID = 0 From 6773e423d6d794c040c49e54a0a6c51b72933053 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Tue, 25 Aug 2020 17:19:25 +0200 Subject: [PATCH 11/11] roachpb: make version objects safe for reporting Release note: None Release justification: low risk, high benefit changes to existing functionality --- pkg/clusterversion/clusterversion.go | 8 ++++-- pkg/clusterversion/cockroach_versions_test.go | 26 +++++++++++++++++++ pkg/roachpb/version.go | 13 +++++++--- 3 files changed, 41 insertions(+), 6 deletions(-) diff --git a/pkg/clusterversion/clusterversion.go b/pkg/clusterversion/clusterversion.go index bf837b533ebf..76add51de4ef 100644 --- a/pkg/clusterversion/clusterversion.go +++ b/pkg/clusterversion/clusterversion.go @@ -45,6 +45,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/util/syncutil" + "github.com/cockroachdb/redact" ) // TODO(irfansharif): Should Initialize and SetBeforeChange be a part of the @@ -205,6 +206,9 @@ func (cv ClusterVersion) IsActive(versionKey VersionKey) bool { return cv.IsActiveVersion(v) } -func (cv ClusterVersion) String() string { - return cv.Version.String() +func (cv ClusterVersion) String() string { return redact.StringWithoutMarkers(cv) } + +// SafeFormat implements the redact.SafeFormatter interface. +func (cv ClusterVersion) SafeFormat(p redact.SafePrinter, _ rune) { + p.Print(cv.Version) } diff --git a/pkg/clusterversion/cockroach_versions_test.go b/pkg/clusterversion/cockroach_versions_test.go index 9ec4409cb53f..326e90fd1dc3 100644 --- a/pkg/clusterversion/cockroach_versions_test.go +++ b/pkg/clusterversion/cockroach_versions_test.go @@ -13,7 +13,9 @@ package clusterversion import ( "testing" + "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/cockroachdb/redact" "github.com/stretchr/testify/require" ) @@ -22,3 +24,27 @@ func TestVersionsAreValid(t *testing.T) { require.NoError(t, versionsSingleton.Validate()) } + +func TestVersionFormat(t *testing.T) { + defer leaktest.AfterTest(t)() + + v := ClusterVersion{ + Version: roachpb.Version{ + Major: 1, + Minor: 2, + Patch: 3, + }, + } + + if actual, expected := string(redact.Sprint(v.Version)), `1.2`; actual != expected { + t.Errorf("expected %q, got %q", expected, actual) + } + + if actual, expected := v.Version.String(), `1.2`; actual != expected { + t.Errorf("expected %q, got %q", expected, actual) + } + + if actual, expected := v.String(), `1.2`; actual != expected { + t.Errorf("expected %q, got %q", expected, actual) + } +} diff --git a/pkg/roachpb/version.go b/pkg/roachpb/version.go index 9435e93d3159..02ba7faa0a44 100644 --- a/pkg/roachpb/version.go +++ b/pkg/roachpb/version.go @@ -11,11 +11,11 @@ package roachpb import ( - "fmt" "strconv" "strings" "github.com/cockroachdb/errors" + "github.com/cockroachdb/redact" ) // Less compares two Versions. @@ -43,11 +43,16 @@ func (v Version) Less(otherV Version) bool { return false } -func (v Version) String() string { +// String implements the fmt.Stringer interface. +func (v Version) String() string { return redact.StringWithoutMarkers(v) } + +// SafeFormat implements the redact.SafeFormatter interface. +func (v Version) SafeFormat(p redact.SafePrinter, _ rune) { if v.Unstable == 0 { - return fmt.Sprintf("%d.%d", v.Major, v.Minor) + p.Printf("%d.%d", v.Major, v.Minor) + return } - return fmt.Sprintf("%d.%d-%d", v.Major, v.Minor, v.Unstable) + p.Printf("%d.%d-%d", v.Major, v.Minor, v.Unstable) } // ParseVersion parses a Version from a string of the form