diff --git a/docs/generated/redact_safe.md b/docs/generated/redact_safe.md index 4e4fb8352ae5..b00252c29283 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` @@ -10,6 +11,15 @@ 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/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` pkg/util/log/redact.go | `reflect.TypeOf(true)` pkg/util/log/redact.go | `reflect.TypeOf(123)` 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/cli/start.go b/pkg/cli/start.go index 5f43e64394a8..4a2992ad375c 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/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/jobs/job_scheduler.go b/pkg/jobs/job_scheduler.go index e1953f7d549b..121f070900c8 100644 --- a/pkg/jobs/job_scheduler.go +++ b/pkg/jobs/job_scheduler.go @@ -347,7 +347,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)) { 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: 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 55d4b3b4ba5e..aa60a2c13dde 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" @@ -1000,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. @@ -1022,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 { @@ -1649,7 +1655,7 @@ func (s *Store) startGossip() { gossipFns := []struct { key roachpb.Key fn func(context.Context, *Replica) error - description string + description redact.SafeString interval time.Duration }{ { 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/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 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 9cd686710ba2..4d0230e40501 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 } @@ -498,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/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/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/catalog/descpb/structured.go b/pkg/sql/catalog/descpb/structured.go index e80e8af7fe50..e6dfe9805943 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 @@ -68,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 @@ -90,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 @@ -153,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 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/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. 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/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 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 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() {} 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.