diff --git a/pkg/ccl/backupccl/backup_planning.go b/pkg/ccl/backupccl/backup_planning.go index 937503942e1c..58ea0793324e 100644 --- a/pkg/ccl/backupccl/backup_planning.go +++ b/pkg/ccl/backupccl/backup_planning.go @@ -425,8 +425,13 @@ func checkPrivilegesForBackup( for _, desc := range targetDescs { switch desc := desc.(type) { case catalog.DatabaseDescriptor: - hasRequiredBackupPrivileges = hasRequiredBackupPrivileges && - p.CheckPrivilegeForUser(ctx, desc, privilege.BACKUP, p.User()) == nil + if hasRequiredBackupPrivileges { + if ok, err := p.HasPrivilege(ctx, desc, privilege.BACKUP, p.User()); err != nil { + return err + } else { + hasRequiredBackupPrivileges = ok + } + } } } } else if backupStmt.Targets.Tables.TablePatterns != nil { @@ -439,8 +444,13 @@ func checkPrivilegesForBackup( for _, desc := range targetDescs { switch desc := desc.(type) { case catalog.TableDescriptor: - hasRequiredBackupPrivileges = hasRequiredBackupPrivileges && - p.CheckPrivilegeForUser(ctx, desc, privilege.BACKUP, p.User()) == nil + if hasRequiredBackupPrivileges { + if ok, err := p.HasPrivilege(ctx, desc, privilege.BACKUP, p.User()); err != nil { + return err + } else { + hasRequiredBackupPrivileges = ok + } + } } } } else { diff --git a/pkg/ccl/backupccl/restore_planning.go b/pkg/ccl/backupccl/restore_planning.go index 2a51efa522ef..973ce518782a 100644 --- a/pkg/ccl/backupccl/restore_planning.go +++ b/pkg/ccl/backupccl/restore_planning.go @@ -1215,7 +1215,9 @@ func checkRestoreDestinationPrivileges( func checkRestorePrivilegesOnDatabase( ctx context.Context, p sql.PlanHookState, parentDB catalog.DatabaseDescriptor, ) (shouldBufferNotice bool, err error) { - if err := p.CheckPrivilege(ctx, parentDB, privilege.RESTORE); err == nil { + if ok, err := p.HasPrivilege(ctx, parentDB, privilege.RESTORE, p.User()); err != nil { + return false, err + } else if ok { return false, nil } @@ -1275,7 +1277,12 @@ func checkPrivilegesForRestore( // error. In 22.2 we continue to check for old style privileges and role // options. if len(restoreStmt.Targets.Databases) > 0 { - hasRestoreSystemPrivilege := p.CheckPrivilegeForUser(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.RESTORE, p.User()) == nil + var hasRestoreSystemPrivilege bool + if ok, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.RESTORE, p.User()); err != nil { + return err + } else { + hasRestoreSystemPrivilege = ok + } if hasRestoreSystemPrivilege { return checkRestoreDestinationPrivileges(ctx, p, from) } diff --git a/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant b/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant index 350f24ba99a7..9ceec5e0c720 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant +++ b/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant @@ -322,12 +322,12 @@ txn_id txn_fingerprint_id query implicit_txn session_id start_time end_tim query ITTI SELECT range_id, start_pretty, end_pretty, lease_holder FROM crdb_internal.ranges ---- -55 /Tenant/10 /Max 1 +55 /Tenant/10 /Tenant/11 1 query ITT SELECT range_id, start_pretty, end_pretty FROM crdb_internal.ranges_no_leases ---- -55 /Tenant/10 /Max +55 /Tenant/10 /Tenant/11 query IT SELECT zone_id, target FROM crdb_internal.zones ORDER BY 1 diff --git a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go index 50abb799c34b..6d8f99d14179 100644 --- a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go +++ b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go @@ -1750,6 +1750,13 @@ func TestTenantLogic_span_builtins( runLogicTest(t, "span_builtins") } +func TestTenantLogic_sql_keys( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "sql_keys") +} + func TestTenantLogic_sqllite( t *testing.T, ) { diff --git a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/multitenant/basic b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/multitenant/basic index ff4c9c48cc87..ca23c0e416f1 100644 --- a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/multitenant/basic +++ b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/multitenant/basic @@ -30,6 +30,7 @@ state offset=47 /Table/5{3-4} database system (host) /Tenant/10{-\x00} database system (tenant) /Tenant/11{-\x00} database system (tenant) +/Tenant/12{-\x00} database system (tenant) # Start the reconciliation loop for the secondary tenant. reconcile tenant=10 @@ -127,6 +128,7 @@ state offset=47 /Tenant/10/Table/5{2-3} database system (tenant) /Tenant/10/Table/5{3-4} database system (tenant) /Tenant/11{-\x00} database system (tenant) +/Tenant/12{-\x00} database system (tenant) exec-sql tenant=10 CREATE DATABASE db; @@ -165,3 +167,4 @@ state offset=81 /Tenant/10/Table/11{2-3} range default /Tenant/10/Table/11{3-4} range default /Tenant/11{-\x00} database system (tenant) +/Tenant/12{-\x00} database system (tenant) diff --git a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/multitenant/protectedts b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/multitenant/protectedts index ecfd857eb500..fb8d8f883b29 100644 --- a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/multitenant/protectedts +++ b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/multitenant/protectedts @@ -26,6 +26,7 @@ state offset=47 /Table/5{2-3} database system (host) /Table/5{3-4} database system (host) /Tenant/10{-\x00} database system (tenant) +/Tenant/11{-\x00} database system (tenant) # Write a protected timestamp record on the system tenant cluster. protect record-id=1 ts=1 diff --git a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/multitenant/range_tenants b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/multitenant/range_tenants index aae4e63d8b66..3aeae4a48ccc 100644 --- a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/multitenant/range_tenants +++ b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/multitenant/range_tenants @@ -46,6 +46,7 @@ state offset=47 /Table/5{3-4} database system (host) /Tenant/10{-\x00} database system (tenant) /Tenant/11{-\x00} ttl_seconds=18000 ignore_strict_gc=true rangefeed_enabled=true +/Tenant/12{-\x00} ttl_seconds=18000 ignore_strict_gc=true rangefeed_enabled=true # Start the reconciliation loop for the tenant=10. It should have the vanilla # RANGE DEFAULT. Check against the underlying KV state, the SQL view of the @@ -105,6 +106,7 @@ state offset=47 /Tenant/10/Table/5{2-3} database system (tenant) /Tenant/10/Table/5{3-4} database system (tenant) /Tenant/11{-\x00} ttl_seconds=18000 ignore_strict_gc=true rangefeed_enabled=true +/Tenant/12{-\x00} ttl_seconds=18000 ignore_strict_gc=true rangefeed_enabled=true query-sql tenant=10 SHOW ZONE CONFIGURATION FOR RANGE DEFAULT @@ -192,6 +194,7 @@ state offset=81 /Tenant/11/Table/5{1-2} ttl_seconds=18000 ignore_strict_gc=true rangefeed_enabled=true /Tenant/11/Table/5{2-3} ttl_seconds=18000 ignore_strict_gc=true rangefeed_enabled=true /Tenant/11/Table/5{3-4} ttl_seconds=18000 ignore_strict_gc=true rangefeed_enabled=true +/Tenant/12{-\x00} ttl_seconds=18000 ignore_strict_gc=true rangefeed_enabled=true query-sql tenant=11 SHOW ZONE CONFIGURATION FOR RANGE DEFAULT diff --git a/pkg/cloud/cloudprivilege/privileges.go b/pkg/cloud/cloudprivilege/privileges.go index 074788b06b2e..b5014fc1c820 100644 --- a/pkg/cloud/cloudprivilege/privileges.go +++ b/pkg/cloud/cloudprivilege/privileges.go @@ -44,8 +44,11 @@ func CheckDestinationPrivileges(ctx context.Context, p sql.PlanHookState, to []s // Check if the destination requires the user to be an admin or have the // `EXTERNALIOIMPLICITACCESS` privilege. requiresImplicitAccess := !conf.AccessIsWithExplicitAuth() - hasImplicitAccessPrivilege := - p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.EXTERNALIOIMPLICITACCESS) == nil + hasImplicitAccessPrivilege, privErr := + p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.EXTERNALIOIMPLICITACCESS, p.User()) + if privErr != nil { + return privErr + } if requiresImplicitAccess && !(p.ExecCfg().ExternalIODirConfig.EnableNonAdminImplicitAndArbitraryOutbound || hasImplicitAccessPrivilege) { return pgerror.Newf( pgcode.InsufficientPrivilege, diff --git a/pkg/cmd/roachtest/tests/cluster_to_cluster.go b/pkg/cmd/roachtest/tests/cluster_to_cluster.go index 3d1a022213f1..65513046e30c 100644 --- a/pkg/cmd/roachtest/tests/cluster_to_cluster.go +++ b/pkg/cmd/roachtest/tests/cluster_to_cluster.go @@ -26,6 +26,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test" "github.com/cockroachdb/cockroach/pkg/jobs" "github.com/cockroachdb/cockroach/pkg/jobs/jobspb" + "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/roachprod/install" "github.com/cockroachdb/cockroach/pkg/roachprod/logger" "github.com/cockroachdb/cockroach/pkg/roachprod/vm/local" @@ -159,6 +160,10 @@ type c2cMetrics struct { cutoverStart sizeTime cutoverEnd sizeTime + + fingerprintingStart time.Time + + fingerprintingEnd time.Time } func (m c2cMetrics) export() map[string]exportedMetric { @@ -427,7 +432,7 @@ func registerClusterToCluster(r registry.Registry) { }) cutoverTime := chooseCutover(t, setup.dst.sql, workloadDuration, sp.cutover) - t.Status("cutover time chosen: %s", cutoverTime.String()) + t.Status(fmt.Sprintf("cutover time chosen: %s", cutoverTime.String())) // TODO(ssd): The job doesn't record the initial // statement time, so we can't correctly measure the @@ -446,23 +451,19 @@ func registerClusterToCluster(r registry.Registry) { m.Wait() t.Status("waiting for replication stream to cutover") + retainedTime := getReplicationRetainedTime(t, setup.dst.sql, roachpb.TenantName(setup.dst.name)) setup.metrics.cutoverStart = newSizeTime(ctx, du, setup.dst.kvNodes) stopReplicationStream(t, setup.dst.sql, ingestionJobID, cutoverTime) setup.metrics.cutoverEnd = newSizeTime(ctx, du, setup.dst.kvNodes) t.Status("comparing fingerprints") - // Currently, it takes about 15 minutes to generate a fingerprint for - // about 30 GB of data. Once the fingerprinting job is used instead, - // this should only take about 5 seconds for the same amount of data. At - // that point, we should increase the number of warehouses in this test. - // - // The new fingerprinting job currently OOMs this test. Once it becomes - // more efficient, it will be used. compareTenantFingerprintsAtTimestamp( t, m, setup, - hlc.Timestamp{WallTime: cutoverTime.UnixNano()}) + retainedTime, + cutoverTime, + ) lv.assertValid(t) // TODO(msbutler): export metrics to roachperf or prom/grafana @@ -492,16 +493,21 @@ func chooseCutover( } func compareTenantFingerprintsAtTimestamp( - t test.Test, m cluster.Monitor, setup *c2cSetup, ts hlc.Timestamp, + t test.Test, m cluster.Monitor, setup *c2cSetup, startTime, endTime time.Time, ) { + t.Status(fmt.Sprintf("comparing tenant fingerprints between start time %s and end time %s", startTime.UTC(), endTime.UTC())) + + // TODO(adityamaru,lidorcarmel): Once we agree on the format and precision we + // display all user facing timestamps with, we should revisit how we format + // the start time to ensure we are fingerprinting from the most accurate lower + // bound. + microSecondRFC3339Format := "2006-01-02 15:04:05.999999" + startTimeStr := startTime.Format(microSecondRFC3339Format) + aost := hlc.Timestamp{WallTime: endTime.UnixNano()}.AsOfSystemTime() fingerprintQuery := fmt.Sprintf(` -SELECT - xor_agg( - fnv64(crdb_internal.trim_tenant_prefix(key), - substring(value from 5)) - ) AS fingerprint -FROM crdb_internal.scan(crdb_internal.tenant_span($1::INT)) -AS OF SYSTEM TIME '%s'`, ts.AsOfSystemTime()) +SELECT * +FROM crdb_internal.fingerprint(crdb_internal.tenant_span($1::INT), '%s'::TIMESTAMPTZ, true) +AS OF SYSTEM TIME '%s'`, startTimeStr, aost) var srcFingerprint int64 m.Go(func(ctx context.Context) error { @@ -509,7 +515,15 @@ AS OF SYSTEM TIME '%s'`, ts.AsOfSystemTime()) return nil }) var destFingerprint int64 - setup.dst.sql.QueryRow(t, fingerprintQuery, setup.dst.ID).Scan(&destFingerprint) + m.Go(func(ctx context.Context) error { + // TODO(adityamaru): Measure and record fingerprinting throughput. + setup.metrics.fingerprintingStart = timeutil.Now() + setup.dst.sql.QueryRow(t, fingerprintQuery, setup.dst.ID).Scan(&destFingerprint) + setup.metrics.fingerprintingEnd = timeutil.Now() + fingerprintingDuration := setup.metrics.fingerprintingEnd.Sub(setup.metrics.fingerprintingStart).String() + t.L().Printf("fingerprinting the destination tenant took %s", fingerprintingDuration) + return nil + }) // If the goroutine gets cancelled or fataled, return before comparing fingerprints. require.NoError(t, m.WaitE()) @@ -573,6 +587,17 @@ func waitForHighWatermark(t test.Test, db *gosql.DB, ingestionJobID int, wait ti }, wait) } +// getReplicationRetainedTime returns the `retained_time` of the replication +// job. +func getReplicationRetainedTime( + t test.Test, destSQL *sqlutils.SQLRunner, destTenantName roachpb.TenantName, +) time.Time { + var retainedTime time.Time + destSQL.QueryRow(t, `SELECT retained_time FROM [SHOW TENANT $1 WITH REPLICATION STATUS]`, + destTenantName).Scan(&retainedTime) + return retainedTime +} + func stopReplicationStream( t test.Test, destSQL *sqlutils.SQLRunner, ingestionJob int, cutoverTime time.Time, ) { diff --git a/pkg/kv/kvclient/scan_meta.go b/pkg/kv/kvclient/scan_meta.go index 7995511d0d8d..facf10e0f625 100644 --- a/pkg/kv/kvclient/scan_meta.go +++ b/pkg/kv/kvclient/scan_meta.go @@ -38,41 +38,3 @@ func ScanMetaKVs(ctx context.Context, txn *kv.Txn, span roachpb.Span) ([]kv.KeyV } return kvs, nil } - -// GetRangeWithID returns the RangeDescriptor with the requested id, or nil if -// no such range exists. Note that it performs a scan over the meta. -func GetRangeWithID( - ctx context.Context, txn *kv.Txn, id roachpb.RangeID, -) (*roachpb.RangeDescriptor, error) { - // Scan the range meta K/V's to find the target range. We do this in a - // chunk-wise fashion to avoid loading all ranges into memory. - var ranges []kv.KeyValue - var err error - var rangeDesc roachpb.RangeDescriptor - const chunkSize = 100 - metaStart := keys.RangeMetaKey(keys.MustAddr(keys.MinKey).Next()) - metaEnd := keys.MustAddr(keys.Meta2Prefix.PrefixEnd()) - for { - // Scan a batch of ranges. - ranges, err = txn.Scan(ctx, metaStart, metaEnd, chunkSize) - if err != nil { - return nil, err - } - // If no results were returned, then exit. - if len(ranges) == 0 { - break - } - for _, r := range ranges { - if err := r.ValueProto(&rangeDesc); err != nil { - return nil, err - } - // Look for a range that matches the target range ID. - if rangeDesc.RangeID == id { - return &rangeDesc, nil - } - } - // Set the next starting point to after the last key in this batch. - metaStart = keys.MustAddr(ranges[len(ranges)-1].Key.Next()) - } - return nil, nil -} diff --git a/pkg/kv/kvserver/batcheval/cmd_export.go b/pkg/kv/kvserver/batcheval/cmd_export.go index 624d303fb54e..7f1a9f706e12 100644 --- a/pkg/kv/kvserver/batcheval/cmd_export.go +++ b/pkg/kv/kvserver/batcheval/cmd_export.go @@ -160,6 +160,14 @@ func evalExport( resumeKeyTS = args.ResumeKeyTS } + maybeAnnotateExceedMaxSizeError := func(err error) error { + if errors.HasType(err, (*storage.ExceedMaxSizeError)(nil)) { + return errors.WithHintf(err, + "consider increasing cluster setting %q", MaxExportOverageSetting) + } + return err + } + var curSizeOfExportedSSTs int64 for start := args.Key; start != nil; { destFile := &storage.MemFile{} @@ -186,16 +194,26 @@ func evalExport( StripTenantPrefix: true, StripValueChecksum: true, } - summary, resume, fingerprint, err = storage.MVCCExportFingerprint(ctx, cArgs.EvalCtx.ClusterSettings(), reader, opts, destFile) + var hasRangeKeys bool + summary, resume, fingerprint, hasRangeKeys, err = storage.MVCCExportFingerprint(ctx, + cArgs.EvalCtx.ClusterSettings(), reader, opts, destFile) + if err != nil { + return result.Result{}, maybeAnnotateExceedMaxSizeError(err) + } + + // If no range keys were encountered during fingerprinting then we zero + // out the underlying SST file as there is no use in sending an empty file + // part of the ExportResponse. This frees up the memory used by the empty + // SST file. + if !hasRangeKeys { + destFile = &storage.MemFile{} + } } else { - summary, resume, err = storage.MVCCExportToSST(ctx, cArgs.EvalCtx.ClusterSettings(), reader, opts, destFile) - } - if err != nil { - if errors.HasType(err, (*storage.ExceedMaxSizeError)(nil)) { - err = errors.WithHintf(err, - "consider increasing cluster setting %q", MaxExportOverageSetting) + summary, resume, err = storage.MVCCExportToSST(ctx, cArgs.EvalCtx.ClusterSettings(), reader, + opts, destFile) + if err != nil { + return result.Result{}, maybeAnnotateExceedMaxSizeError(err) } - return result.Result{}, err } data := destFile.Data() @@ -222,12 +240,26 @@ func evalExport( } else { span.EndKey = args.EndKey } - exported := roachpb.ExportResponse_File{ - Span: span, - EndKeyTS: resume.Timestamp, - Exported: summary, - SST: data, - Fingerprint: fingerprint, + + var exported roachpb.ExportResponse_File + if args.ExportFingerprint { + // A fingerprinting ExportRequest does not need to return the + // BulkOpSummary or the exported Span. This is because we do not expect + // the sender of a fingerprint ExportRequest to use anything but the + // `Fingerprint` for point-keys and the SST file that contains the + // rangekeys we encountered during ExportRequest evaluation. + exported = roachpb.ExportResponse_File{ + EndKeyTS: resume.Timestamp, + SST: data, + Fingerprint: fingerprint, + } + } else { + exported = roachpb.ExportResponse_File{ + Span: span, + EndKeyTS: resume.Timestamp, + Exported: summary, + SST: data, + } } reply.Files = append(reply.Files, exported) start = resume.Key diff --git a/pkg/rpc/auth_tenant.go b/pkg/rpc/auth_tenant.go index 5d7a472a09d9..f577c3e14719 100644 --- a/pkg/rpc/auth_tenant.go +++ b/pkg/rpc/auth_tenant.go @@ -147,6 +147,13 @@ func (a tenantAuthorizer) authorize( } } +func checkSpanBounds(rSpan, tenSpan roachpb.RSpan) error { + if !tenSpan.ContainsKeyRange(rSpan.Key, rSpan.EndKey) { + return authErrorf("requested key span %s not fully contained in tenant keyspace %s", rSpan, tenSpan) + } + return nil +} + // authBatch authorizes the provided tenant to invoke the Batch RPC with the // provided args. func (a tenantAuthorizer) authBatch(tenID roachpb.TenantID, args *roachpb.BatchRequest) error { @@ -195,10 +202,7 @@ func (a tenantAuthorizer) authBatch(tenID roachpb.TenantID, args *roachpb.BatchR return authError(err.Error()) } tenSpan := tenantPrefix(tenID) - if !tenSpan.ContainsKeyRange(rSpan.Key, rSpan.EndKey) { - return authErrorf("requested key span %s not fully contained in tenant keyspace %s", rSpan, tenSpan) - } - return nil + return checkSpanBounds(rSpan, tenSpan) } func (a tenantAuthorizer) authGetRangeDescriptors( @@ -229,10 +233,7 @@ func (a tenantAuthorizer) authRangeFeed( return authError(err.Error()) } tenSpan := tenantPrefix(tenID) - if !tenSpan.ContainsKeyRange(rSpan.Key, rSpan.EndKey) { - return authErrorf("requested key span %s not fully contained in tenant keyspace %s", rSpan, tenSpan) - } - return nil + return checkSpanBounds(rSpan, tenSpan) } // authGossipSubscription authorizes the provided tenant to invoke the @@ -440,10 +441,7 @@ func validateSpan(tenID roachpb.TenantID, sp roachpb.Span) error { if err != nil { return authError(err.Error()) } - if !tenSpan.ContainsKeyRange(rSpan.Key, rSpan.EndKey) { - return authErrorf("requested key span %s not fully contained in tenant keyspace %s", rSpan, tenSpan) - } - return nil + return checkSpanBounds(rSpan, tenSpan) } // contextWithClientTenant inserts a tenant identifier in the context, diff --git a/pkg/server/BUILD.bazel b/pkg/server/BUILD.bazel index fdcb12c48a9f..2e783110064c 100644 --- a/pkg/server/BUILD.bazel +++ b/pkg/server/BUILD.bazel @@ -230,6 +230,7 @@ go_library( "//pkg/storage", "//pkg/storage/enginepb", "//pkg/storage/fs", + "//pkg/testutils", "//pkg/testutils/serverutils", "//pkg/testutils/skip", "//pkg/ts", diff --git a/pkg/server/admin.go b/pkg/server/admin.go index 5722bce3dfb1..d4ea2d96f224 100644 --- a/pkg/server/admin.go +++ b/pkg/server/admin.go @@ -1948,7 +1948,7 @@ func (s *adminServer) Settings( keys = settings.Keys(settings.ForSystemTenant) } - user, isAdmin, err := s.getUserAndRole(ctx) + _, isAdmin, err := s.getUserAndRole(ctx) if err != nil { return nil, serverError(ctx, err) } @@ -1965,24 +1965,8 @@ func (s *adminServer) Settings( } else { // Non-root access cannot see the values in any case. lookupPurpose = settings.LookupForReporting - - hasView := s.checkHasGlobalPrivilege(ctx, user, privilege.VIEWCLUSTERSETTING) - hasModify := s.checkHasGlobalPrivilege(ctx, user, privilege.MODIFYCLUSTERSETTING) - if !hasModify && !hasView { - hasView, err := s.hasRoleOption(ctx, user, roleoption.VIEWCLUSTERSETTING) - if err != nil { - return nil, err - } - - hasModify, err := s.hasRoleOption(ctx, user, roleoption.MODIFYCLUSTERSETTING) - if err != nil { - return nil, err - } - if !hasModify && !hasView { - return nil, grpcstatus.Errorf( - codes.PermissionDenied, "this operation requires either %s or %s system privileges", - privilege.VIEWCLUSTERSETTING, privilege.MODIFYCLUSTERSETTING) - } + if err := s.adminPrivilegeChecker.requireViewClusterSettingOrModifyClusterSettingPermission(ctx); err != nil { + return nil, err } } @@ -3712,21 +3696,22 @@ func (c *adminPrivilegeChecker) requireViewActivityPermission(ctx context.Contex if err != nil { return serverError(ctx, err) } - if !isAdmin { - hasView := c.checkHasGlobalPrivilege(ctx, userName, privilege.VIEWACTIVITY) - if !hasView { - hasView, err := c.hasRoleOption(ctx, userName, roleoption.VIEWACTIVITY) - if err != nil { - return serverError(ctx, err) - } - if !hasView { - return grpcstatus.Errorf( - codes.PermissionDenied, "this operation requires the %s system privilege", - roleoption.VIEWACTIVITY) - } - } + if isAdmin { + return nil } - return nil + if hasView, err := c.hasGlobalPrivilege(ctx, userName, privilege.VIEWACTIVITY); err != nil { + return serverError(ctx, err) + } else if hasView { + return nil + } + if hasView, err := c.hasRoleOption(ctx, userName, roleoption.VIEWACTIVITY); err != nil { + return serverError(ctx, err) + } else if hasView { + return nil + } + return grpcstatus.Errorf( + codes.PermissionDenied, "this operation requires the %s system privilege", + roleoption.VIEWACTIVITY) } // requireViewActivityOrViewActivityRedactedPermission's error return is a gRPC error. @@ -3737,26 +3722,68 @@ func (c *adminPrivilegeChecker) requireViewActivityOrViewActivityRedactedPermiss if err != nil { return serverError(ctx, err) } - if !isAdmin { - hasView := c.checkHasGlobalPrivilege(ctx, userName, privilege.VIEWACTIVITY) - hasViewRedacted := c.checkHasGlobalPrivilege(ctx, userName, privilege.VIEWACTIVITYREDACTED) - if !hasView && !hasViewRedacted { - hasView, err := c.hasRoleOption(ctx, userName, roleoption.VIEWACTIVITY) - if err != nil { - return serverError(ctx, err) - } - hasViewRedacted, err := c.hasRoleOption(ctx, userName, roleoption.VIEWACTIVITYREDACTED) - if err != nil { - return serverError(ctx, err) - } - if !hasView && !hasViewRedacted { - return grpcstatus.Errorf( - codes.PermissionDenied, "this operation requires the %s or %s system privileges", - roleoption.VIEWACTIVITY, roleoption.VIEWACTIVITYREDACTED) - } - } + if isAdmin { + return nil } - return nil + if hasView, err := c.hasGlobalPrivilege(ctx, userName, privilege.VIEWACTIVITY); err != nil { + return serverError(ctx, err) + } else if hasView { + return nil + } + if hasViewRedacted, err := c.hasGlobalPrivilege(ctx, userName, privilege.VIEWACTIVITYREDACTED); err != nil { + return serverError(ctx, err) + } else if hasViewRedacted { + return nil + } + if hasView, err := c.hasRoleOption(ctx, userName, roleoption.VIEWACTIVITY); err != nil { + return serverError(ctx, err) + } else if hasView { + return nil + } + if hasViewRedacted, err := c.hasRoleOption(ctx, userName, roleoption.VIEWACTIVITYREDACTED); err != nil { + return serverError(ctx, err) + } else if hasViewRedacted { + return nil + } + return grpcstatus.Errorf( + codes.PermissionDenied, "this operation requires the %s or %s system privileges", + roleoption.VIEWACTIVITY, roleoption.VIEWACTIVITYREDACTED) +} + +// requireViewClusterSettingOrModifyClusterSettingPermission's error return is a gRPC error. +func (c *adminPrivilegeChecker) requireViewClusterSettingOrModifyClusterSettingPermission( + ctx context.Context, +) (err error) { + userName, isAdmin, err := c.getUserAndRole(ctx) + if err != nil { + return serverError(ctx, err) + } + if isAdmin { + return nil + } + if hasView, err := c.hasGlobalPrivilege(ctx, userName, privilege.VIEWCLUSTERSETTING); err != nil { + return serverError(ctx, err) + } else if hasView { + return nil + } + if hasModify, err := c.hasGlobalPrivilege(ctx, userName, privilege.MODIFYCLUSTERSETTING); err != nil { + return serverError(ctx, err) + } else if hasModify { + return nil + } + if hasView, err := c.hasRoleOption(ctx, userName, roleoption.VIEWCLUSTERSETTING); err != nil { + return serverError(ctx, err) + } else if hasView { + return nil + } + if hasModify, err := c.hasRoleOption(ctx, userName, roleoption.MODIFYCLUSTERSETTING); err != nil { + return serverError(ctx, err) + } else if hasModify { + return nil + } + return grpcstatus.Errorf( + codes.PermissionDenied, "this operation requires the %s or %s system privileges", + privilege.VIEWCLUSTERSETTING, privilege.MODIFYCLUSTERSETTING) } // This function requires that the user have the VIEWACTIVITY role, but does not @@ -3771,7 +3798,10 @@ func (c *adminPrivilegeChecker) requireViewActivityAndNoViewActivityRedactedPerm } if !isAdmin { - hasViewRedacted := c.checkHasGlobalPrivilege(ctx, userName, privilege.VIEWACTIVITYREDACTED) + hasViewRedacted, err := c.hasGlobalPrivilege(ctx, userName, privilege.VIEWACTIVITYREDACTED) + if err != nil { + return serverError(ctx, err) + } if !hasViewRedacted { hasViewRedacted, err := c.hasRoleOption(ctx, userName, roleoption.VIEWACTIVITYREDACTED) if err != nil { @@ -3801,14 +3831,17 @@ func (c *adminPrivilegeChecker) requireViewClusterMetadataPermission( if err != nil { return serverError(ctx, err) } - if !isAdmin { - if hasViewClusterMetadata := c.checkHasGlobalPrivilege(ctx, userName, privilege.VIEWCLUSTERMETADATA); !hasViewClusterMetadata { - return grpcstatus.Errorf( - codes.PermissionDenied, "this operation requires the %s system privilege", - privilege.VIEWCLUSTERMETADATA) - } + if isAdmin { + return nil } - return nil + if hasViewClusterMetadata, err := c.hasGlobalPrivilege(ctx, userName, privilege.VIEWCLUSTERMETADATA); err != nil { + return serverError(ctx, err) + } else if hasViewClusterMetadata { + return nil + } + return grpcstatus.Errorf( + codes.PermissionDenied, "this operation requires the %s system privilege", + privilege.VIEWCLUSTERMETADATA) } // requireViewDebugPermission requires the user have admin or the VIEWDEBUG system privilege @@ -3818,14 +3851,17 @@ func (c *adminPrivilegeChecker) requireViewDebugPermission(ctx context.Context) if err != nil { return serverError(ctx, err) } - if !isAdmin { - if hasViewDebug := c.checkHasGlobalPrivilege(ctx, userName, privilege.VIEWDEBUG); !hasViewDebug { - return grpcstatus.Errorf( - codes.PermissionDenied, "this operation requires the %s system privilege", - privilege.VIEWDEBUG) - } + if isAdmin { + return nil } - return nil + if hasViewDebug, err := c.hasGlobalPrivilege(ctx, userName, privilege.VIEWDEBUG); err != nil { + return serverError(ctx, err) + } else if hasViewDebug { + return nil + } + return grpcstatus.Errorf( + codes.PermissionDenied, "this operation requires the %s system privilege", + privilege.VIEWDEBUG) } // Note that the function returns plain errors, and it is the caller's @@ -3899,17 +3935,16 @@ func (c *adminPrivilegeChecker) hasRoleOption( return bool(dbDatum), nil } -// checkHasGlobalPrivilege is a helper function which calls +// hasGlobalPrivilege is a helper function which calls // CheckPrivilege and returns a true/false based on the returned // result. -func (c *adminPrivilegeChecker) checkHasGlobalPrivilege( +func (c *adminPrivilegeChecker) hasGlobalPrivilege( ctx context.Context, user username.SQLUsername, privilege privilege.Kind, -) bool { +) (bool, error) { planner, cleanup := c.makePlanner("check-system-privilege") defer cleanup() aa := planner.(sql.AuthorizationAccessor) - err := aa.CheckPrivilegeForUser(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege, user) - return err == nil + return aa.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege, user) } var errRequiresAdmin = grpcstatus.Error(codes.PermissionDenied, "this operation requires admin privilege") diff --git a/pkg/server/status.go b/pkg/server/status.go index 0f6e5f3579d3..ded6e118d53b 100644 --- a/pkg/server/status.go +++ b/pkg/server/status.go @@ -335,7 +335,10 @@ func (b *baseStatusServer) checkCancelPrivilege( if sessionUser != reqUser { // Must have CANCELQUERY privilege to cancel other users' // sessions/queries. - hasCancelQuery := b.privilegeChecker.checkHasGlobalPrivilege(ctx, reqUser, privilege.CANCELQUERY) + hasCancelQuery, err := b.privilegeChecker.hasGlobalPrivilege(ctx, reqUser, privilege.CANCELQUERY) + if err != nil { + return serverError(ctx, err) + } if !hasCancelQuery { ok, err := b.privilegeChecker.hasRoleOption(ctx, reqUser, roleoption.CANCELQUERY) if err != nil { diff --git a/pkg/server/testserver.go b/pkg/server/testserver.go index 2a25fcdca88a..b61105797a11 100644 --- a/pkg/server/testserver.go +++ b/pkg/server/testserver.go @@ -49,6 +49,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/storage" + "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/ts" @@ -60,6 +61,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/log/severity" "github.com/cockroachdb/cockroach/pkg/util/metric" addrutil "github.com/cockroachdb/cockroach/pkg/util/netutil/addr" + "github.com/cockroachdb/cockroach/pkg/util/rangedesc" "github.com/cockroachdb/cockroach/pkg/util/retry" "github.com/cockroachdb/cockroach/pkg/util/stop" "github.com/cockroachdb/cockroach/pkg/util/tracing" @@ -792,6 +794,31 @@ func (t *TestTenant) Tracer() *tracing.Tracer { return t.SQLServer.ambientCtx.Tracer } +// WaitForTenantEndKeySplit is part of the TestTenantInterface. +func (t *TestTenant) WaitForTenantEndKeySplit(ctx context.Context) error { + // Wait until the tenant end key split happens. + return testutils.SucceedsWithinError(func() error { + factory := t.RangeDescIteratorFactory().(rangedesc.IteratorFactory) + + iterator, err := factory.NewIterator(ctx, t.Codec().TenantSpan()) + if err != nil { + return err + } + if !iterator.Valid() { + return errors.New("range iterator has no ranges") + } + + for iterator.Valid() { + rangeDesc := iterator.CurRangeDescriptor() + if rangeDesc.EndKey.Compare(roachpb.RKeyMax) == 0 { + return errors.Newf("range ID %d end key not split", rangeDesc.RangeID) + } + iterator.Next() + } + return nil + }, 10*time.Second) +} + // StartSharedProcessTenant is part of TestServerInterface. func (ts *TestServer) StartSharedProcessTenant( ctx context.Context, args base.TestSharedProcessTenantArgs, @@ -1577,6 +1604,12 @@ func (ts *TestServer) Tracer() *tracing.Tracer { return ts.node.storeCfg.AmbientCtx.Tracer } +// WaitForTenantEndKeySplit is part of the TestTenantInterface. +func (ts *TestServer) WaitForTenantEndKeySplit(context.Context) error { + // Does not apply to system tenant. + return nil +} + // ForceTableGC is part of TestServerInterface. func (ts *TestServer) ForceTableGC( ctx context.Context, database, table string, timestamp hlc.Timestamp, diff --git a/pkg/server/user.go b/pkg/server/user.go index cee6e175835f..13943c243232 100644 --- a/pkg/server/user.go +++ b/pkg/server/user.go @@ -27,14 +27,17 @@ func (s *baseStatusServer) UserSQLRoles( username, isAdmin, err := s.privilegeChecker.getUserAndRole(ctx) if err != nil { - return nil, err + return nil, serverError(ctx, err) } var resp serverpb.UserSQLRolesResponse if !isAdmin { for _, privKind := range privilege.GlobalPrivileges { privName := privKind.String() - hasPriv := s.privilegeChecker.checkHasGlobalPrivilege(ctx, username, privKind) + hasPriv, err := s.privilegeChecker.hasGlobalPrivilege(ctx, username, privKind) + if err != nil { + return nil, serverError(ctx, err) + } if hasPriv { resp.Roles = append(resp.Roles, privName) continue @@ -45,7 +48,7 @@ func (s *baseStatusServer) UserSQLRoles( } hasRole, err := s.privilegeChecker.hasRoleOption(ctx, username, roleOpt) if err != nil { - return nil, err + return nil, serverError(ctx, err) } if hasRole { resp.Roles = append(resp.Roles, privName) diff --git a/pkg/sql/alter_database.go b/pkg/sql/alter_database.go index 82b6f0c30d22..16e87f0908bc 100644 --- a/pkg/sql/alter_database.go +++ b/pkg/sql/alter_database.go @@ -473,10 +473,13 @@ func (p *planner) checkPrivilegesForMultiRegionOp( // TODO(arul): It's worth noting CREATE isn't a thing on tables in postgres, // so this will require some changes when (if) we move our privilege system // to be more in line with postgres. - err := p.CheckPrivilege(ctx, desc, privilege.CREATE) + hasPriv, err := p.HasPrivilege(ctx, desc, privilege.CREATE, p.User()) + if err != nil { + return err + } // Wrap an insufficient privileges error a bit better to reflect the lack // of ownership as well. - if pgerror.GetPGCode(err) == pgcode.InsufficientPrivilege { + if !hasPriv { return pgerror.Newf(pgcode.InsufficientPrivilege, "user %s must be owner of %s or have %s privilege on %s %s", p.SessionData().User(), @@ -486,7 +489,6 @@ func (p *planner) checkPrivilegesForMultiRegionOp( desc.GetName(), ) } - return err } return nil } diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index aa1493cf5fa6..d680a9e5be44 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -98,7 +98,7 @@ func (p *planner) AlterTable(ctx context.Context, n *tree.AlterTable) (planNode, // This check for CREATE privilege is kept for backwards compatibility. if err := p.CheckPrivilege(ctx, tableDesc, privilege.CREATE); err != nil { - return nil, pgerror.Newf(pgcode.InsufficientPrivilege, + return nil, pgerror.Wrapf(err, pgcode.InsufficientPrivilege, "must be owner of table %s or have CREATE privilege on table %s", tree.Name(tableDesc.GetName()), tree.Name(tableDesc.GetName())) } @@ -860,7 +860,10 @@ func (p *planner) setAuditMode( } if !hasAdmin { // Check for system privilege first, otherwise fall back to role options. - hasModify := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYCLUSTERSETTING) == nil + hasModify, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYCLUSTERSETTING, p.User()) + if err != nil { + return false, err + } if !hasModify { hasModify, err = p.HasRoleOption(ctx, roleoption.MODIFYCLUSTERSETTING) if err != nil { diff --git a/pkg/sql/authorization.go b/pkg/sql/authorization.go index 6be9510f149e..14c2a86bc5dd 100644 --- a/pkg/sql/authorization.go +++ b/pkg/sql/authorization.go @@ -79,6 +79,12 @@ type AuthorizationAccessor interface { ctx context.Context, tableID descpb.ID, privilege privilege.Kind, ) error + // HasPrivilege checks if the user has `privilege` on `descriptor`. + HasPrivilege(ctx context.Context, privilegeObject privilege.Object, privilege privilege.Kind, user username.SQLUsername) (bool, error) + + // HasAnyPrivilege returns true if user has any privileges at all. + HasAnyPrivilege(ctx context.Context, privilegeObject privilege.Object) (bool, error) + // CheckPrivilege verifies that the user has `privilege` on `descriptor`. CheckPrivilegeForUser( ctx context.Context, privilegeObject privilege.Object, privilege privilege.Kind, user username.SQLUsername, @@ -121,19 +127,18 @@ type AuthorizationAccessor interface { var _ AuthorizationAccessor = &planner{} -// CheckPrivilegeForUser implements the AuthorizationAccessor interface. -// Requires a valid transaction to be open. -func (p *planner) CheckPrivilegeForUser( +// HasPrivilege is part of the AuthorizationAccessor interface. +func (p *planner) HasPrivilege( ctx context.Context, privilegeObject privilege.Object, privilegeKind privilege.Kind, user username.SQLUsername, -) error { +) (bool, error) { // Verify that the txn is valid in any case, so that // we don't get the risk to say "OK" to root requests // with an invalid API usage. if p.txn == nil { - return errors.AssertionFailedf("cannot use CheckPrivilege without a txn") + return false, errors.AssertionFailedf("cannot use CheckPrivilege without a txn") } // root, admin and node user should always have privileges. @@ -148,9 +153,9 @@ func (p *planner) CheckPrivilegeForUser( if privilege.GetValidPrivilegesForObject( privilegeObject.GetObjectType(), ).Contains(privilegeKind) { - return nil + return true, nil } - return insufficientPrivilegeError(user, privilegeKind, privilegeObject) + return false, nil } // Test whether the object is being audited, and if so, record an @@ -162,12 +167,12 @@ func (p *planner) CheckPrivilegeForUser( privs, err := p.getPrivilegeDescriptor(ctx, privilegeObject) if err != nil { - return err + return false, err } // Check if the 'public' pseudo-role has privileges. if privs.CheckPrivilege(username.PublicRoleName(), privilegeKind) { - return nil + return true, nil } hasPriv, err := p.checkRolePredicate(ctx, user, func(role username.SQLUsername) (bool, error) { @@ -175,12 +180,78 @@ func (p *planner) CheckPrivilegeForUser( return isOwner || privs.CheckPrivilege(role, privilegeKind), err }) if err != nil { - return err + return false, err } if hasPriv { - return nil + return true, nil } - return insufficientPrivilegeError(user, privilegeKind, privilegeObject) + return false, nil +} + +// HasAnyPrivilege is part of the AuthorizationAccessor interface. +func (p *planner) HasAnyPrivilege( + ctx context.Context, privilegeObject privilege.Object, +) (bool, error) { + // Verify that the txn is valid in any case, so that + // we don't get the risk to say "OK" to root requests + // with an invalid API usage. + if p.txn == nil { + return false, errors.AssertionFailedf("cannot use CheckAnyPrivilege without a txn") + } + + user := p.SessionData().User() + if user.IsNodeUser() { + // User "node" has all privileges. + return true, nil + } + + privs, err := p.getPrivilegeDescriptor(ctx, privilegeObject) + if err != nil { + return false, err + } + + // Check if 'user' itself has privileges. + if privs.AnyPrivilege(user) { + return true, nil + } + + // Check if 'public' has privileges. + if privs.AnyPrivilege(username.PublicRoleName()) { + return true, nil + } + + // Expand role memberships. + memberOf, err := p.MemberOfWithAdminOption(ctx, user) + if err != nil { + return false, err + } + + // Iterate over the roles that 'user' is a member of. We don't care about the admin option. + for role := range memberOf { + if privs.AnyPrivilege(role) { + return true, nil + } + } + + return false, nil +} + +// CheckPrivilegeForUser implements the AuthorizationAccessor interface. +// Requires a valid transaction to be open. +func (p *planner) CheckPrivilegeForUser( + ctx context.Context, + privilegeObject privilege.Object, + privilegeKind privilege.Kind, + user username.SQLUsername, +) error { + ok, err := p.HasPrivilege(ctx, privilegeObject, privilegeKind, user) + if err != nil { + return err + } + if !ok { + return insufficientPrivilegeError(user, privilegeKind, privilegeObject) + } + return nil } // CheckPrivilege implements the AuthorizationAccessor interface. @@ -335,49 +406,15 @@ func (p *planner) checkRolePredicate( // CheckAnyPrivilege implements the AuthorizationAccessor interface. // Requires a valid transaction to be open. func (p *planner) CheckAnyPrivilege(ctx context.Context, privilegeObject privilege.Object) error { - // Verify that the txn is valid in any case, so that - // we don't get the risk to say "OK" to root requests - // with an invalid API usage. - if p.txn == nil { - return errors.AssertionFailedf("cannot use CheckAnyPrivilege without a txn") - } - user := p.SessionData().User() - - if user.IsNodeUser() { - // User "node" has all privileges. - return nil - } - - privs, err := p.getPrivilegeDescriptor(ctx, privilegeObject) - if err != nil { - return err - } - - // Check if 'user' itself has privileges. - if privs.AnyPrivilege(user) { - return nil - } - - // Check if 'public' has privileges. - if privs.AnyPrivilege(username.PublicRoleName()) { - return nil - } - - // Expand role memberships. - memberOf, err := p.MemberOfWithAdminOption(ctx, user) + ok, err := p.HasAnyPrivilege(ctx, privilegeObject) if err != nil { return err } - - // Iterate over the roles that 'user' is a member of. We don't care about the admin option. - for role := range memberOf { - if privs.AnyPrivilege(role) { - return nil - } + if !ok { + return insufficientPrivilegeError(user, 0 /* kind */, privilegeObject) } - - return insufficientPrivilegeError(user, 0 /* kind */, privilegeObject) + return nil } // UserHasAdminRole implements the AuthorizationAccessor interface. @@ -871,29 +908,32 @@ func (p *planner) HasOwnershipOnSchema( } func (p *planner) HasViewActivityOrViewActivityRedactedRole(ctx context.Context) (bool, error) { - hasAdmin, err := p.HasAdminRole(ctx) - if err != nil { + if hasAdmin, err := p.HasAdminRole(ctx); err != nil { return hasAdmin, err + } else if hasAdmin { + return true, nil } - if !hasAdmin { - hasView := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWACTIVITY) == nil - hasViewRedacted := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWACTIVITYREDACTED) == nil - if !hasView && !hasViewRedacted { - hasView, err := p.HasRoleOption(ctx, roleoption.VIEWACTIVITY) - if err != nil { - return hasView, err - } - hasViewRedacted, err := p.HasRoleOption(ctx, roleoption.VIEWACTIVITYREDACTED) - if err != nil { - return hasViewRedacted, err - } - if !hasView && !hasViewRedacted { - return false, nil - } - } + if hasView, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWACTIVITY, p.User()); err != nil { + return false, err + } else if hasView { + return true, nil } - - return true, nil + if hasViewRedacted, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWACTIVITYREDACTED, p.User()); err != nil { + return false, err + } else if hasViewRedacted { + return true, nil + } + if hasView, err := p.HasRoleOption(ctx, roleoption.VIEWACTIVITY); err != nil { + return false, err + } else if hasView { + return true, nil + } + if hasViewRedacted, err := p.HasRoleOption(ctx, roleoption.VIEWACTIVITYREDACTED); err != nil { + return false, err + } else if hasViewRedacted { + return true, nil + } + return false, nil } func insufficientPrivilegeError( diff --git a/pkg/sql/catalog/randgen/randgen.go b/pkg/sql/catalog/randgen/randgen.go index e16e9d2d0aae..acd8692546f7 100644 --- a/pkg/sql/catalog/randgen/randgen.go +++ b/pkg/sql/catalog/randgen/randgen.go @@ -221,6 +221,7 @@ type genError struct { type Catalog interface { HasAdminRole(context.Context) (bool, error) CheckAnyPrivilege(context.Context, privilege.Object) error + HasAnyPrivilege(context.Context, privilege.Object) (bool, error) CanCreateDatabase(context.Context) error CheckPrivilegeForUser(context.Context, privilege.Object, privilege.Kind, username.SQLUsername) error ExpandTableGlob(context.Context, tree.TablePattern) (tree.TableNames, descpb.IDs, error) diff --git a/pkg/sql/catalog/randgen/templates.go b/pkg/sql/catalog/randgen/templates.go index 3410d6cfc41f..f6b61064b121 100644 --- a/pkg/sql/catalog/randgen/templates.go +++ b/pkg/sql/catalog/randgen/templates.go @@ -97,11 +97,19 @@ outer: // Extract the templates. for _, desc := range descs { // Can this user even see this table? - if err := g.ext.cat.CheckAnyPrivilege(ctx, desc); err != nil { + if ok, err := g.ext.cat.HasAnyPrivilege(ctx, desc); err != nil { + panic(genError{err}) + } else if !ok { if len(descs) == 1 { // The pattern was specific to just one table, so let's be // helpful to the user about why it can't be used. - panic(genError{err}) + panic(genError{ + pgerror.Newf( + pgcode.InsufficientPrivilege, + "user has no privileges on %s", + desc.GetName(), + ), + }) } else { // The expansion resulted in multiple objects; we simply // ignore objects the user can't use. diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index 1ba2e4001b8f..4df2bb26762c 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -546,8 +546,13 @@ CREATE TABLE crdb_internal.tables ( // Note: we do not use forEachTableDesc() here because we want to // include added and dropped descriptors. for _, desc := range descs { - table, ok := desc.(catalog.TableDescriptor) - if !ok || p.CheckAnyPrivilege(ctx, table) != nil { + table, isTable := desc.(catalog.TableDescriptor) + if !isTable { + continue + } + if ok, err := p.HasAnyPrivilege(ctx, table); err != nil { + return err + } else if !ok { continue } dbName := dbNames[table.GetParentID()] @@ -618,8 +623,13 @@ func crdbInternalTablesDatabaseLookupFunc( if desc.GetParentID() != dbID { return nil } - table, ok := desc.(catalog.TableDescriptor) - if !ok || p.CheckAnyPrivilege(ctx, table) != nil { + table, isTable := desc.(catalog.TableDescriptor) + if !isTable { + return nil + } + if ok, err := p.HasAnyPrivilege(ctx, table); err != nil { + return err + } else if !ok { return nil } seenAny = true @@ -769,8 +779,13 @@ CREATE TABLE crdb_internal.schema_changes ( // Note: we do not use forEachTableDesc() here because we want to // include added and dropped descriptors. for _, desc := range descs { - table, ok := desc.(catalog.TableDescriptor) - if !ok || p.CheckAnyPrivilege(ctx, table) != nil { + table, isTable := desc.(catalog.TableDescriptor) + if !isTable { + continue + } + if ok, err := p.HasAnyPrivilege(ctx, table); err != nil { + return err + } else if !ok { continue } tableID := tree.NewDInt(tree.DInt(int64(table.GetID()))) @@ -825,25 +840,31 @@ CREATE TABLE crdb_internal.leases ( )`, populate: func( ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error, - ) (err error) { + ) error { nodeID, _ := p.execCfg.NodeInfo.NodeID.OptionalNodeID() // zero if not available + var iterErr error p.LeaseMgr().VisitLeases(func(desc catalog.Descriptor, takenOffline bool, _ int, expiration tree.DTimestamp) (wantMore bool) { - if p.CheckAnyPrivilege(ctx, desc) != nil { - // TODO(ajwerner): inspect what type of error got returned. + if ok, err := p.HasAnyPrivilege(ctx, desc); err != nil { + iterErr = err + return false + } else if !ok { return true } - err = addRow( + if err := addRow( tree.NewDInt(tree.DInt(nodeID)), tree.NewDInt(tree.DInt(int64(desc.GetID()))), tree.NewDString(desc.GetName()), tree.NewDInt(tree.DInt(int64(desc.GetParentID()))), &expiration, tree.MakeDBool(tree.DBool(takenOffline)), - ) - return err == nil + ); err != nil { + iterErr = err + return false + } + return true }) - return err + return iterErr }, } @@ -1816,29 +1837,39 @@ CREATE TABLE crdb_internal.cluster_settings ( description STRING NOT NULL )`, populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error { - hasAdmin, err := p.HasAdminRole(ctx) - if err != nil { - return err - } - if !hasAdmin { - hasModify := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYCLUSTERSETTING) == nil - hasView := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERSETTING) == nil - - if !hasModify && !hasView { - hasModify, err := p.HasRoleOption(ctx, roleoption.MODIFYCLUSTERSETTING) - if err != nil { - return err - } - hasView, err := p.HasRoleOption(ctx, roleoption.VIEWCLUSTERSETTING) - if err != nil { - return err - } - if !hasModify && !hasView { - return pgerror.Newf(pgcode.InsufficientPrivilege, - "only users with either %s or %s system privileges are allowed to read "+ - "crdb_internal.cluster_settings", privilege.MODIFYCLUSTERSETTING, privilege.VIEWCLUSTERSETTING) - } + if hasPriv, err := func() (bool, error) { + if hasAdmin, err := p.HasAdminRole(ctx); err != nil { + return false, err + } else if hasAdmin { + return true, nil + } + if hasModify, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYCLUSTERSETTING, p.User()); err != nil { + return false, err + } else if hasModify { + return true, nil + } + if hasView, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERSETTING, p.User()); err != nil { + return false, err + } else if hasView { + return true, nil } + if hasModify, err := p.HasRoleOption(ctx, roleoption.MODIFYCLUSTERSETTING); err != nil { + return false, err + } else if hasModify { + return true, nil + } + if hasView, err := p.HasRoleOption(ctx, roleoption.VIEWCLUSTERSETTING); err != nil { + return false, err + } else if hasView { + return true, nil + } + return false, nil + }(); err != nil { + return err + } else if !hasPriv { + return pgerror.Newf(pgcode.InsufficientPrivilege, + "only users with either %s or %s system privileges are allowed to read "+ + "crdb_internal.cluster_settings", privilege.MODIFYCLUSTERSETTING, privilege.VIEWCLUSTERSETTING) } for _, k := range settings.Keys(p.ExecCfg().Codec.ForSystemTenant()) { setting, _ := settings.Lookup( @@ -3668,7 +3699,7 @@ CREATE VIEW crdb_internal.ranges AS SELECT ` + // table). It also returns maps of table descriptor IDs to the parent schema ID // and the parent (database) descriptor ID, to aid in necessary lookups. func descriptorsByType( - descs []catalog.Descriptor, privCheckerFunc func(desc catalog.Descriptor) bool, + descs []catalog.Descriptor, privCheckerFunc func(desc catalog.Descriptor) (bool, error), ) ( hasPermission bool, dbNames map[uint32]string, @@ -3677,6 +3708,7 @@ func descriptorsByType( indexNames map[uint32]map[uint32]string, schemaParents map[uint32]uint32, parents map[uint32]uint32, + retErr error, ) { // TODO(knz): maybe this could use internalLookupCtx. dbNames = make(map[uint32]string) @@ -3688,7 +3720,10 @@ func descriptorsByType( hasPermission = false for _, desc := range descs { id := uint32(desc.GetID()) - if !privCheckerFunc(desc) { + if ok, err := privCheckerFunc(desc); err != nil { + retErr = err + return + } else if !ok { continue } hasPermission = true @@ -3708,7 +3743,7 @@ func descriptorsByType( } } - return hasPermission, dbNames, tableNames, schemaNames, indexNames, schemaParents, parents + return hasPermission, dbNames, tableNames, schemaNames, indexNames, schemaParents, parents, nil } // lookupNamesByKey is a utility function that, given a key, utilizes the maps @@ -3784,17 +3819,18 @@ CREATE TABLE crdb_internal.ranges_no_leases ( } descs := all.OrderedDescriptors() - privCheckerFunc := func(desc catalog.Descriptor) bool { + privCheckerFunc := func(desc catalog.Descriptor) (bool, error) { if hasAdmin { - return true + return true, nil } - - return p.CheckPrivilege(ctx, desc, privilege.ZONECONFIG) == nil + return p.HasPrivilege(ctx, desc, privilege.ZONECONFIG, p.User()) } hasPermission := false for _, desc := range descs { - if privCheckerFunc(desc) { + if ok, err := privCheckerFunc(desc); err != nil { + return nil, nil, err + } else if ok { hasPermission = true break } @@ -4024,7 +4060,9 @@ CREATE TABLE crdb_internal.zones ( if err != nil { return err } - if p.CheckAnyPrivilege(ctx, database) != nil { + if ok, err := p.HasAnyPrivilege(ctx, database); err != nil { + return err + } else if !ok { continue } } else if zoneSpecifier.TableOrIndex.Table.ObjectName != "" { @@ -4032,7 +4070,9 @@ CREATE TABLE crdb_internal.zones ( if err != nil { return err } - if p.CheckAnyPrivilege(ctx, tableEntry) != nil { + if ok, err := p.HasAnyPrivilege(ctx, tableEntry); err != nil { + return err + } else if !ok { continue } table = tableEntry @@ -4997,7 +5037,9 @@ CREATE TABLE crdb_internal.kv_catalog_descriptor ( // Delegate privilege check to system table. { sysTable := all.LookupDescriptor(systemschema.DescriptorTable.GetID()) - if p.CheckPrivilege(ctx, sysTable, privilege.SELECT) != nil { + if ok, err := p.HasPrivilege(ctx, sysTable, privilege.SELECT, p.User()); err != nil { + return err + } else if !ok { return nil } } @@ -5031,7 +5073,9 @@ CREATE TABLE crdb_internal.kv_catalog_zones ( // Delegate privilege check to system table. { sysTable := all.LookupDescriptor(systemschema.ZonesTable.GetID()) - if p.CheckPrivilege(ctx, sysTable, privilege.SELECT) != nil { + if ok, err := p.HasPrivilege(ctx, sysTable, privilege.SELECT, p.User()); err != nil { + return err + } else if !ok { return nil } } @@ -5068,7 +5112,9 @@ CREATE TABLE crdb_internal.kv_catalog_namespace ( // Delegate privilege check to system table. { sysTable := all.LookupDescriptor(systemschema.NamespaceTable.GetID()) - if p.CheckPrivilege(ctx, sysTable, privilege.SELECT) != nil { + if ok, err := p.HasPrivilege(ctx, sysTable, privilege.SELECT, p.User()); err != nil { + return err + } else if !ok { return nil } } @@ -5103,7 +5149,9 @@ CREATE TABLE crdb_internal.kv_catalog_comments ( // Delegate privilege check to system table. { sysTable := all.LookupDescriptor(systemschema.CommentsTable.GetID()) - if p.CheckPrivilege(ctx, sysTable, privilege.SELECT) != nil { + if ok, err := p.HasPrivilege(ctx, sysTable, privilege.SELECT, p.User()); err != nil { + return err + } else if !ok { return nil } } @@ -6626,19 +6674,24 @@ func genClusterLocksGenerator( } descs := all.OrderedDescriptors() - privCheckerFunc := func(desc catalog.Descriptor) bool { + privCheckerFunc := func(desc catalog.Descriptor) (bool, error) { if hasAdmin { - return true + return true, nil } - return p.CheckAnyPrivilege(ctx, desc) == nil + return p.HasAnyPrivilege(ctx, desc) } - _, dbNames, tableNames, schemaNames, indexNames, schemaParents, parents := + _, dbNames, tableNames, schemaNames, indexNames, schemaParents, parents, err := descriptorsByType(descs, privCheckerFunc) + if err != nil { + return nil, nil, err + } var spansToQuery roachpb.Spans for _, desc := range descs { - if !privCheckerFunc(desc) { + if ok, err := privCheckerFunc(desc); err != nil { + return nil, nil, err + } else if !ok { continue } switch desc := desc.(type) { diff --git a/pkg/sql/distsql_running.go b/pkg/sql/distsql_running.go index 5620ce2474af..bfb1a582594b 100644 --- a/pkg/sql/distsql_running.go +++ b/pkg/sql/distsql_running.go @@ -43,6 +43,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/cockroach/pkg/sql/types" + "github.com/cockroachdb/cockroach/pkg/util/buildutil" "github.com/cockroachdb/cockroach/pkg/util/contextutil" "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented" "github.com/cockroachdb/cockroach/pkg/util/hlc" @@ -463,9 +464,12 @@ func (dsp *DistSQLPlanner) setupFlows( // Start all the remote flows. // - // usedWorker indicates whether we used at least one DistSQL worker - // goroutine to issue the SetupFlow RPC. - var usedWorker bool + // numAsyncRequests tracks the number of the SetupFlow RPCs that were + // delegated to the DistSQL runner goroutines. + var numAsyncRequests int + // numSerialRequests tracks the number of the SetupFlow RPCs that were + // issued by the current goroutine on its own. + var numSerialRequests int if sp := tracing.SpanFromContext(origCtx); sp != nil && !sp.IsNoop() { setupReq.TraceInfo = sp.Meta().ToProto() } @@ -490,6 +494,7 @@ func (dsp *DistSQLPlanner) setupFlows( var runnerSpan *tracing.Span // This span is necessary because it can outlive its parent. runnerCtx, runnerSpan = tracing.ChildSpan(runnerCtx, "setup-flow-async" /* opName */) + // runnerCleanup can only be executed _after_ all issued RPCs are complete. runnerCleanup := func() { cancelRunnerCtx() runnerSpan.Finish() @@ -499,6 +504,23 @@ func (dsp *DistSQLPlanner) setupFlows( var listenerGoroutineWillCleanup bool defer func() { if !listenerGoroutineWillCleanup { + // Make sure to receive from the result channel as many times as + // there were total SetupFlow RPCs issued, regardless of whether + // they were executed concurrently by a DistSQL worker or serially + // in the current goroutine. This is needed in order to block + // finishing the runner span (in runnerCleanup) until all concurrent + // requests are done since the runner span is used as the parent for + // the RPC span, and, thus, the runner span can only be finished + // when we know that all SetupFlow RPCs have already been completed. + // + // Note that even in case of an error in runnerRequest.run we still + // send on the result channel. + for i := 0; i < numAsyncRequests+numSerialRequests; i++ { + <-resultChan + } + // At this point, we know that all concurrent requests (if there + // were any) are complete, so we can safely perform the runner + // cleanup. runnerCleanup() } }() @@ -521,8 +543,9 @@ func (dsp *DistSQLPlanner) setupFlows( // directly. select { case dsp.runnerCoordinator.runnerChan <- runReq: - usedWorker = true + numAsyncRequests++ default: + numSerialRequests++ // Use the context of the local flow since we're executing this // SetupFlow RPC synchronously. runReq.ctx = ctx @@ -531,8 +554,16 @@ func (dsp *DistSQLPlanner) setupFlows( } } } + if buildutil.CrdbTestBuild { + if numAsyncRequests+numSerialRequests != len(flows)-1 { + return ctx, flow, errors.AssertionFailedf( + "expected %d requests, found only %d async and %d serial", + len(flows)-1, numAsyncRequests, numSerialRequests, + ) + } + } - if !usedWorker { + if numAsyncRequests == 0 { // We executed all SetupFlow RPCs in the current goroutine, and all RPCs // succeeded. return ctx, flow, nil @@ -565,6 +596,10 @@ func (dsp *DistSQLPlanner) setupFlows( cancelRunnerCtx() }) err = dsp.stopper.RunAsyncTask(origCtx, "distsql-remote-flows-setup-listener", func(ctx context.Context) { + // Note that in the loop below we always receive from the result channel + // as many times as there were SetupFlow RPCs issued, thus, by the time + // this defer is executed, we are certain that all RPCs were complete, + // and runnerCleanup() is safe to be executed. defer runnerCleanup() var seenError bool for i := 0; i < len(flows)-1; i++ { diff --git a/pkg/sql/faketreeeval/evalctx.go b/pkg/sql/faketreeeval/evalctx.go index 682bbda78eca..29c1ca3a830a 100644 --- a/pkg/sql/faketreeeval/evalctx.go +++ b/pkg/sql/faketreeeval/evalctx.go @@ -65,8 +65,8 @@ func (so *DummySequenceOperators) SchemaExists( return false, errors.WithStack(errSequenceOperators) } -// HasAnyPrivilege is part of the eval.DatabaseCatalog interface. -func (so *DummySequenceOperators) HasAnyPrivilege( +// HasAnyPrivilegeForSpecifier is part of the eval.DatabaseCatalog interface. +func (so *DummySequenceOperators) HasAnyPrivilegeForSpecifier( ctx context.Context, specifier eval.HasPrivilegeSpecifier, user username.SQLUsername, @@ -355,8 +355,8 @@ func (ep *DummyEvalPlanner) SchemaExists(ctx context.Context, dbName, scName str return false, errors.WithStack(errEvalPlanner) } -// HasAnyPrivilege is part of the eval.DatabaseCatalog interface. -func (ep *DummyEvalPlanner) HasAnyPrivilege( +// HasAnyPrivilegeForSpecifier is part of the eval.DatabaseCatalog interface. +func (ep *DummyEvalPlanner) HasAnyPrivilegeForSpecifier( ctx context.Context, specifier eval.HasPrivilegeSpecifier, user username.SQLUsername, @@ -454,6 +454,13 @@ func (ep *DummyEvalPlanner) IsANSIDML() bool { return false } +// GetRangeDescByID is part of the eval.Planner interface. +func (ep *DummyEvalPlanner) GetRangeDescByID( + context.Context, roachpb.RangeID, +) (rangeDesc roachpb.RangeDescriptor, err error) { + return +} + // DummyPrivilegedAccessor implements the tree.PrivilegedAccessor interface by returning errors. type DummyPrivilegedAccessor struct{} diff --git a/pkg/sql/importer/import_table_creation.go b/pkg/sql/importer/import_table_creation.go index 69f522d0cd46..2fe3fe9d17a1 100644 --- a/pkg/sql/importer/import_table_creation.go +++ b/pkg/sql/importer/import_table_creation.go @@ -303,8 +303,8 @@ func (so *importSequenceOperators) SchemaExists( return false, errSequenceOperators } -// HasAnyPrivilege is part of the eval.DatabaseCatalog interface. -func (so *importSequenceOperators) HasAnyPrivilege( +// HasAnyPrivilegeForSpecifier is part of the eval.DatabaseCatalog interface. +func (so *importSequenceOperators) HasAnyPrivilegeForSpecifier( ctx context.Context, specifier eval.HasPrivilegeSpecifier, user username.SQLUsername, diff --git a/pkg/sql/information_schema.go b/pkg/sql/information_schema.go index 6ee3711e7518..b6cdaaceaf3b 100644 --- a/pkg/sql/information_schema.go +++ b/pkg/sql/information_schema.go @@ -2796,10 +2796,21 @@ func userCanSeeDescriptor( // TODO(richardjcai): We may possibly want to remove the ability to view // the descriptor if they have any privilege on the descriptor and only // allow the descriptor to be viewed if they have CONNECT on the DB. #59827. - canSeeDescriptor := p.CheckAnyPrivilege(ctx, desc) == nil + canSeeDescriptor := false + if ok, err := p.HasAnyPrivilege(ctx, desc); err != nil { + return false, err + } else { + canSeeDescriptor = ok + } // Users can see objects in the database if they have connect privilege. if parentDBDesc != nil { - canSeeDescriptor = canSeeDescriptor || p.CheckPrivilege(ctx, parentDBDesc, privilege.CONNECT) == nil + if !canSeeDescriptor { + if ok, err := p.HasPrivilege(ctx, parentDBDesc, privilege.CONNECT, p.User()); err != nil { + return false, err + } else { + canSeeDescriptor = ok + } + } } return canSeeDescriptor, nil } diff --git a/pkg/sql/logictest/logic.go b/pkg/sql/logictest/logic.go index 0c279e0c848e..4bf6bdf89845 100644 --- a/pkg/sql/logictest/logic.go +++ b/pkg/sql/logictest/logic.go @@ -1503,6 +1503,9 @@ func (t *logicTest) newCluster( if err != nil { t.rootT.Fatalf("%+v", err) } + if err := tenant.WaitForTenantEndKeySplit(context.Background()); err != nil { + t.rootT.Fatalf("%+v", err) + } t.tenantAddrs[i] = tenant.SQLAddr() } diff --git a/pkg/sql/logictest/testdata/logic_test/gen_test_objects b/pkg/sql/logictest/testdata/logic_test/gen_test_objects index eac7b278ec95..350e07b5b320 100644 --- a/pkg/sql/logictest/testdata/logic_test/gen_test_objects +++ b/pkg/sql/logictest/testdata/logic_test/gen_test_objects @@ -507,7 +507,7 @@ user testuser query error permission denied to create database SELECT crdb_internal.generate_test_objects('foo._._', ARRAY[1,0,0]) -query error user testuser has no privileges on relation foo +query error user has no privileges on foo SELECT crdb_internal.generate_test_objects('{"table_templates":["rootonly.foo"]}'::jsonb) query error template name expansion did not find any usable tables diff --git a/pkg/sql/logictest/testdata/logic_test/pg_catalog b/pkg/sql/logictest/testdata/logic_test/pg_catalog index e99656dd1f2c..844bd39f01f8 100644 --- a/pkg/sql/logictest/testdata/logic_test/pg_catalog +++ b/pkg/sql/logictest/testdata/logic_test/pg_catalog @@ -1631,9 +1631,9 @@ oid typname typnamespace typowner typlen typbyval typty 21 int2 4294967127 NULL 2 true b 22 int2vector 4294967127 NULL -1 false b 23 int4 4294967127 NULL 4 true b -24 regproc 4294967127 NULL 8 true b +24 regproc 4294967127 NULL 4 true b 25 text 4294967127 NULL -1 false b -26 oid 4294967127 NULL 8 true b +26 oid 4294967127 NULL 4 true b 30 oidvector 4294967127 NULL -1 false b 700 float4 4294967127 NULL 4 true b 701 float8 4294967127 NULL 8 true b @@ -1676,9 +1676,9 @@ oid typname typnamespace typowner typlen typbyval typty 1562 varbit 4294967127 NULL -1 false b 1563 _varbit 4294967127 NULL -1 false b 1700 numeric 4294967127 NULL -1 false b -2202 regprocedure 4294967127 NULL 8 true b -2205 regclass 4294967127 NULL 8 true b -2206 regtype 4294967127 NULL 8 true b +2202 regprocedure 4294967127 NULL 4 true b +2205 regclass 4294967127 NULL 4 true b +2206 regtype 4294967127 NULL 4 true b 2207 _regprocedure 4294967127 NULL -1 false b 2210 _regclass 4294967127 NULL -1 false b 2211 _regtype 4294967127 NULL -1 false b @@ -1695,9 +1695,9 @@ oid typname typnamespace typowner typlen typbyval typty 3645 _tsquery 4294967127 NULL -1 false b 3802 jsonb 4294967127 NULL -1 false b 3807 _jsonb 4294967127 NULL -1 false b -4089 regnamespace 4294967127 NULL 8 true b +4089 regnamespace 4294967127 NULL 4 true b 4090 _regnamespace 4294967127 NULL -1 false b -4096 regrole 4294967127 NULL 8 true b +4096 regrole 4294967127 NULL 4 true b 4097 _regrole 4294967127 NULL -1 false b 90000 geometry 4294967127 NULL -1 false b 90001 _geometry 4294967127 NULL -1 false b diff --git a/pkg/sql/logictest/testdata/logic_test/sql_keys b/pkg/sql/logictest/testdata/logic_test/sql_keys index ba153b214dc3..c4c8d1dd8c8a 100644 --- a/pkg/sql/logictest/testdata/logic_test/sql_keys +++ b/pkg/sql/logictest/testdata/logic_test/sql_keys @@ -1,4 +1,5 @@ -# LogicTest: local +# LogicTest: local 3node-tenant +# tenant-cluster-setting-override-opt: allow-split-at-for-secondary-tenants # This test depends on table ID's being stable, so add new tests at the bottom # of the file. @@ -15,6 +16,14 @@ SELECT range_id FROM [SHOW RANGES FROM TABLE t] OFFSET 1 LIMIT 1 let $tableid SELECT id FROM system.namespace WHERE name = 't' +# wait until split happens on secondary tenant's end key or list_sql_keys_in_range will fail +onlyif config 3node-tenant +query TT +SELECT start_key, end_key FROM [SHOW RANGES] +---- +/Tenant/10 /Tenant/10/Table/106/1/0 +/Tenant/10/Table/106/1/0 /Tenant/11 + # Without any data in the table, there shouldn't be any keys in the range. query T SELECT key FROM crdb_internal.list_sql_keys_in_range($rangeid) @@ -26,20 +35,36 @@ INSERT INTO t VALUES (1, 1), (2, 2) # List out all of the keys in this range. The values themselves are # different on each run of the test due to metadata stored in the value. +onlyif config local query T SELECT key FROM crdb_internal.list_sql_keys_in_range($rangeid) ---- /Table/106/1/1/0 /Table/106/1/2/0 +onlyif config 3node-tenant +query T +SELECT key FROM crdb_internal.list_sql_keys_in_range($rangeid) +---- +/Tenant/10/Table/106/1/1/0 +/Tenant/10/Table/106/1/2/0 + # List out all of the keys in this range. The values themselves are # different on each run of the test due to metadata stored in the value. +onlyif config local query T SELECT crdb_internal.pretty_key(key, 0) FROM crdb_internal.scan(crdb_internal.table_span($tableid)) ---- /106/1/1/0 /106/1/2/0 +onlyif config 3node-tenant +query T +SELECT crdb_internal.pretty_key(key, 0) FROM crdb_internal.scan(crdb_internal.table_span($tableid)) +---- +/10/Table/106/1/1/0 +/10/Table/106/1/2/0 + # An error should be returned when an invalid range ID is specified. statement error pq: range with ID 1000000 not found SELECT key FROM crdb_internal.list_sql_keys_in_range(1000000) @@ -69,5 +94,10 @@ SELECT count(key), count(DISTINCT key) FROM crdb_internal.scan(crdb_internal.tab # Regression test for not closing the generator builtin if it encounters an # error in Start() (#87248). +onlyif config local statement error failed to verify keys for Scan SELECT crdb_internal.scan('\xff':::BYTES, '\x3f5918':::BYTES); + +onlyif config 3node-tenant +statement error not fully contained in tenant keyspace +SELECT crdb_internal.scan('\xff':::BYTES, '\x3f5918':::BYTES); diff --git a/pkg/sql/multitenant_admin_function_test.go b/pkg/sql/multitenant_admin_function_test.go index 104e94a51f0e..1fbe541168dc 100644 --- a/pkg/sql/multitenant_admin_function_test.go +++ b/pkg/sql/multitenant_admin_function_test.go @@ -264,6 +264,7 @@ func (tc testCase) runTest( testServer.Stopper(), ) + var secondaryTenants []serverutils.TestTenantInterface createSecondaryDB := func(tenantID roachpb.TenantID, skipSQLSystemTentantCheck bool, clusterSettings ...*settings.BoolSetting) *gosql.DB { testingClusterSettings := cluster.MakeTestingClusterSettings() for _, clusterSetting := range clusterSettings { @@ -271,7 +272,7 @@ func (tc testCase) runTest( clusterSetting.Override(ctx, &testingClusterSettings.SV, true) } } - _, db := serverutils.StartTenant( + tenant, db := serverutils.StartTenant( t, testServer, base.TestTenantArgs{ Settings: testingClusterSettings, TestingKnobs: base.TestingKnobs{ @@ -282,6 +283,7 @@ func (tc testCase) runTest( TenantID: tenantID, }, ) + secondaryTenants = append(secondaryTenants, tenant) return db } @@ -306,6 +308,12 @@ func (tc testCase) runTest( cfg.queryClusterSetting, ) + // Wait for splits after starting all tenants to make test start up faster. + for _, tenant := range secondaryTenants { + err := tenant.WaitForTenantEndKeySplit(ctx) + require.NoError(t, err) + } + execQueries(testCluster, systemDB, "system", tc.system) execQueries(testCluster, secondaryDB, "secondary", tc.secondary) if tc.secondaryWithoutClusterSetting.isSet() { @@ -558,6 +566,12 @@ func TestTruncateTable(t *testing.T) { {"…/104/2/1", ""}, }, }, + secondaryWithoutCapability: tenantExpected{ + result: [][]string{ + {"", "…/104/2/1"}, + {"…/104/2/1", ""}, + }, + }, setupClusterSetting: sql.SecondaryTenantSplitAtEnabled, } tc.runTest( diff --git a/pkg/sql/opt/BUILD.bazel b/pkg/sql/opt/BUILD.bazel index 828bb67fd8bd..2dfc60c133f3 100644 --- a/pkg/sql/opt/BUILD.bazel +++ b/pkg/sql/opt/BUILD.bazel @@ -61,6 +61,7 @@ go_test( args = ["-test.timeout=55s"], embed = [":opt"], deps = [ + "//pkg/roachpb", "//pkg/settings/cluster", "//pkg/sql/catalog/descpb", "//pkg/sql/opt/cat", diff --git a/pkg/sql/opt/metadata_test.go b/pkg/sql/opt/metadata_test.go index fdf25e2afd21..70c0e6f8e2fc 100644 --- a/pkg/sql/opt/metadata_test.go +++ b/pkg/sql/opt/metadata_test.go @@ -16,6 +16,7 @@ import ( "reflect" "testing" + "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/opt" @@ -461,3 +462,10 @@ func (f *fakeGetMultiregionConfigPlanner) GetMultiregionConfig( func (f *fakeGetMultiregionConfigPlanner) IsANSIDML() bool { return false } + +// GetRangeDescByID is part of the eval.Planner interface. +func (ep *fakeGetMultiregionConfigPlanner) GetRangeDescByID( + context.Context, roachpb.RangeID, +) (rangeDesc roachpb.RangeDescriptor, err error) { + return +} diff --git a/pkg/sql/pg_extension.go b/pkg/sql/pg_extension.go index 71fac142d360..bc8d8e61b0e8 100644 --- a/pkg/sql/pg_extension.go +++ b/pkg/sql/pg_extension.go @@ -50,7 +50,9 @@ func postgisColumnsTablePopulator( if !table.IsPhysicalTable() { return nil } - if p.CheckAnyPrivilege(ctx, table) != nil { + if ok, err := p.HasAnyPrivilege(ctx, table); err != nil { + return err + } else if !ok { return nil } for _, col := range table.PublicColumns() { diff --git a/pkg/sql/pgwire/pgwirebase/encoding.go b/pkg/sql/pgwire/pgwirebase/encoding.go index fe5dd7b51e7b..96db062d0fb0 100644 --- a/pkg/sql/pgwire/pgwirebase/encoding.go +++ b/pkg/sql/pgwire/pgwirebase/encoding.go @@ -556,12 +556,6 @@ func DecodeDatum( } i := int64(binary.BigEndian.Uint64(b)) return tree.NewDInt(tree.DInt(i)), nil - case oid.T_oid: - if len(b) < 4 { - return nil, pgerror.Newf(pgcode.Syntax, "oid requires 4 bytes for binary format") - } - u := binary.BigEndian.Uint32(b) - return tree.NewDOid(oid.Oid(u)), nil case oid.T_float4: if len(b) < 4 { return nil, pgerror.Newf(pgcode.Syntax, "float4 requires 4 bytes for binary format") @@ -833,6 +827,17 @@ func DecodeDatum( if typ.Family() == types.TupleFamily { return decodeBinaryTuple(ctx, evalCtx, b) } + if typ.Family() == types.OidFamily { + if len(b) < 4 { + return nil, pgerror.Newf(pgcode.ProtocolViolation, "oid requires 4 bytes for binary format") + } + u := binary.BigEndian.Uint32(b) + oidTyp := types.Oid + if t, ok := types.OidToType[id]; ok { + oidTyp = t + } + return tree.NewDOidWithType(oid.Oid(u), oidTyp), nil + } } default: return nil, errors.AssertionFailedf( diff --git a/pkg/sql/pgwire/testdata/pgtest/oid b/pkg/sql/pgwire/testdata/pgtest/oid new file mode 100644 index 000000000000..d094a89657f8 --- /dev/null +++ b/pkg/sql/pgwire/testdata/pgtest/oid @@ -0,0 +1,35 @@ +# [2205, 4089, 24, 2206, 26] are the OIDs for regclass, regnamespace, regproc, regtype, and oid. +send +Parse {"Query": "SELECT $1, $2, $3, $4, $5", "ParameterOIDs": [2205,4089,24,2206,26]} +Describe {"ObjectType": "S"} +Bind {"ParameterFormatCodes": [1,1,1,1,1], "Parameters": [{"binary":"01000029"},{"binary":"0100002a"},{"binary":"0100002b"},{"binary":"0100002c"},{"binary":"ffffffff"}]} +Execute +Sync +---- + +until +ReadyForQuery +---- +{"Type":"ParseComplete"} +{"Type":"ParameterDescription","ParameterOIDs":[2205,4089,24,2206,26]} +{"Type":"RowDescription","Fields":[{"Name":"?column?","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":2205,"DataTypeSize":4,"TypeModifier":-1,"Format":0},{"Name":"?column?","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":4089,"DataTypeSize":4,"TypeModifier":-1,"Format":0},{"Name":"?column?","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":24,"DataTypeSize":4,"TypeModifier":-1,"Format":0},{"Name":"?column?","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":2206,"DataTypeSize":4,"TypeModifier":-1,"Format":0},{"Name":"?column?","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":26,"DataTypeSize":4,"TypeModifier":-1,"Format":0}]} +{"Type":"BindComplete"} +{"Type":"DataRow","Values":[{"text":"16777257"},{"text":"16777258"},{"text":"16777259"},{"text":"16777260"},{"text":"4294967295"}]} +{"Type":"CommandComplete","CommandTag":"SELECT 1"} +{"Type":"ReadyForQuery","TxStatus":"I"} + +# [2205] is the OID for regclass. +send +Parse {"Query": "SELECT $1", "ParameterOIDs": [2205]} +Bind {"ParameterFormatCodes": [1], "Parameters": [{"binary":"0029"}]} +Execute +Sync +---- + +until +ErrorResponse +ReadyForQuery +---- +{"Type":"ParseComplete"} +{"Type":"ErrorResponse","Code":"08P01"} +{"Type":"ReadyForQuery","TxStatus":"I"} diff --git a/pkg/sql/privileged_accessor.go b/pkg/sql/privileged_accessor.go index 113a9b847e81..8f8dc9648942 100644 --- a/pkg/sql/privileged_accessor.go +++ b/pkg/sql/privileged_accessor.go @@ -89,7 +89,7 @@ func (p *planner) checkDescriptorPermissions(ctx context.Context, id descpb.ID) return err } if err := p.CheckAnyPrivilege(ctx, desc); err != nil { - return pgerror.New(pgcode.InsufficientPrivilege, "insufficient privilege") + return pgerror.Wrapf(err, pgcode.InsufficientPrivilege, "insufficient privilege") } return nil } diff --git a/pkg/sql/region_util.go b/pkg/sql/region_util.go index e700a0996beb..408db2a38f3d 100644 --- a/pkg/sql/region_util.go +++ b/pkg/sql/region_util.go @@ -2397,6 +2397,29 @@ func (p *planner) IsANSIDML() bool { return p.stmt.IsANSIDML() } +// GetRangeDescByID is part of the eval.Planner interface. +func (p *planner) GetRangeDescByID( + ctx context.Context, rangeID roachpb.RangeID, +) (rangeDesc roachpb.RangeDescriptor, _ error) { + execCfg := p.execCfg + tenantSpan := execCfg.Codec.TenantSpan() + rangeDescIterator, err := execCfg.RangeDescIteratorFactory.NewIterator(ctx, tenantSpan) + if err != nil { + return rangeDesc, err + } + for rangeDescIterator.Valid() { + rangeDesc = rangeDescIterator.CurRangeDescriptor() + if rangeDesc.RangeID == rangeID { + break + } + rangeDescIterator.Next() + } + if !rangeDescIterator.Valid() { + return rangeDesc, errors.Newf("range with ID %d not found", rangeID) + } + return rangeDesc, nil +} + // OptimizeSystemDatabase is part of the eval.RegionOperator interface. func (p *planner) OptimizeSystemDatabase(ctx context.Context) error { globalTables := []string{ diff --git a/pkg/sql/resolver.go b/pkg/sql/resolver.go index d05d8490302d..735ce50670b2 100644 --- a/pkg/sql/resolver.go +++ b/pkg/sql/resolver.go @@ -134,8 +134,8 @@ func (p *planner) SchemaExists(ctx context.Context, dbName, scName string) (foun return found, err } -// HasAnyPrivilege is part of the eval.DatabaseCatalog interface. -func (p *planner) HasAnyPrivilege( +// HasAnyPrivilegeForSpecifier is part of the eval.DatabaseCatalog interface. +func (p *planner) HasAnyPrivilegeForSpecifier( ctx context.Context, specifier eval.HasPrivilegeSpecifier, user username.SQLUsername, @@ -161,11 +161,10 @@ func (p *planner) HasAnyPrivilege( continue } - if err := p.CheckPrivilegeForUser(ctx, desc, priv.Kind, user); err != nil { - if pgerror.GetPGCode(err) == pgcode.InsufficientPrivilege { - continue - } + if ok, err := p.HasPrivilege(ctx, desc, priv.Kind, user); err != nil { return eval.HasNoPrivilege, err + } else if !ok { + continue } if priv.GrantOption { diff --git a/pkg/sql/schemachanger/scbuild/dependencies.go b/pkg/sql/schemachanger/scbuild/dependencies.go index 9de072dc1857..92652c670467 100644 --- a/pkg/sql/schemachanger/scbuild/dependencies.go +++ b/pkg/sql/schemachanger/scbuild/dependencies.go @@ -174,6 +174,12 @@ type AuthorizationAccessor interface { // MemberOfWithAdminOption looks up all the roles 'member' belongs to (direct // and indirect) and returns a map of "role" -> "isAdmin". MemberOfWithAdminOption(ctx context.Context, member username.SQLUsername) (map[username.SQLUsername]bool, error) + + // HasPrivilege checks if the user has `privilege` on `descriptor`. + HasPrivilege(ctx context.Context, privilegeObject privilege.Object, privilege privilege.Kind, user username.SQLUsername) (bool, error) + + // HasAnyPrivilege returns true if user has any privileges at all. + HasAnyPrivilege(ctx context.Context, privilegeObject privilege.Object) (bool, error) } // AstFormatter provides interfaces for formatting AST nodes. diff --git a/pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go b/pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go index 254f45eaa060..52c40968c58a 100644 --- a/pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go +++ b/pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go @@ -160,6 +160,23 @@ func (s *TestState) HasOwnership( return true, nil } +// HasPrivilege implements the scbuild.AuthorizationAccessor interface. +func (s *TestState) HasPrivilege( + ctx context.Context, + privilegeObject privilege.Object, + privilege privilege.Kind, + user username.SQLUsername, +) (bool, error) { + return true, nil +} + +// HasAnyPrivilege implements the scbuild.AuthorizationAccessor interface. +func (s *TestState) HasAnyPrivilege( + ctx context.Context, privilegeObject privilege.Object, +) (bool, error) { + return true, nil +} + // CheckPrivilegeForUser implements the scbuild.AuthorizationAccessor interface. func (s *TestState) CheckPrivilegeForUser( ctx context.Context, diff --git a/pkg/sql/sem/builtins/generator_builtins.go b/pkg/sql/sem/builtins/generator_builtins.go index b40d073c7d16..53923a9ad800 100644 --- a/pkg/sql/sem/builtins/generator_builtins.go +++ b/pkg/sql/sem/builtins/generator_builtins.go @@ -2230,6 +2230,7 @@ type rangeKeyIterator struct { // by the constructor of the rangeKeyIterator. rangeID roachpb.RangeID spanKeyIterator + planner eval.Planner } var _ eval.ValueGenerator = &rangeKeyIterator{} @@ -2246,12 +2247,14 @@ func makeRangeKeyIterator( if !isAdmin { return nil, pgerror.Newf(pgcode.InsufficientPrivilege, "user needs the admin role to view range data") } + planner := evalCtx.Planner rangeID := roachpb.RangeID(tree.MustBeDInt(args[0])) return &rangeKeyIterator{ spanKeyIterator: spanKeyIterator{ - acc: evalCtx.Planner.Mon().MakeBoundAccount(), + acc: planner.Mon().MakeBoundAccount(), }, rangeID: rangeID, + planner: planner, }, nil } @@ -2261,17 +2264,14 @@ func (rk *rangeKeyIterator) ResolvedType() *types.T { } // Start implements the tree.ValueGenerator interface. -func (rk *rangeKeyIterator) Start(ctx context.Context, txn *kv.Txn) error { +func (rk *rangeKeyIterator) Start(ctx context.Context, txn *kv.Txn) (err error) { // Scan the range meta K/V's to find the target range. We do this in a // chunk-wise fashion to avoid loading all ranges into memory. - rangeDesc, err := kvclient.GetRangeWithID(ctx, txn, rk.rangeID) + rangeDesc, err := rk.planner.GetRangeDescByID(ctx, rk.rangeID) if err != nil { return err } - if rangeDesc == nil { - return errors.Newf("range with ID %d not found", rk.rangeID) - } - rk.spanKeyIterator.span = roachpb.Span{Key: rangeDesc.StartKey.AsRawKey(), EndKey: rangeDesc.EndKey.AsRawKey()} + rk.span = rangeDesc.KeySpan().AsRawSpanWithNoLocals() return rk.spanKeyIterator.Start(ctx, txn) } diff --git a/pkg/sql/sem/builtins/pg_builtins.go b/pkg/sql/sem/builtins/pg_builtins.go index 37be57c5f51c..3c9e87f6b9d8 100644 --- a/pkg/sql/sem/builtins/pg_builtins.go +++ b/pkg/sql/sem/builtins/pg_builtins.go @@ -1250,7 +1250,7 @@ var pgBuiltins = map[string]builtinDefinition{ if err != nil { return eval.HasNoPrivilege, err } - return evalCtx.Planner.HasAnyPrivilege(ctx, specifier, user, privs) + return evalCtx.Planner.HasAnyPrivilegeForSpecifier(ctx, specifier, user, privs) }, ), @@ -1278,7 +1278,7 @@ var pgBuiltins = map[string]builtinDefinition{ if err != nil { return eval.HasNoPrivilege, err } - return evalCtx.Planner.HasAnyPrivilege(ctx, specifier, user, privs) + return evalCtx.Planner.HasAnyPrivilegeForSpecifier(ctx, specifier, user, privs) }, ), @@ -1307,7 +1307,7 @@ var pgBuiltins = map[string]builtinDefinition{ return eval.HasNoPrivilege, err } - return evalCtx.Planner.HasAnyPrivilege(ctx, specifier, user, privs) + return evalCtx.Planner.HasAnyPrivilegeForSpecifier(ctx, specifier, user, privs) }, ), @@ -1388,7 +1388,7 @@ var pgBuiltins = map[string]builtinDefinition{ // For user-defined function, utilize the descriptor based way. if catid.IsOIDUserDefined(oid.(*tree.DOid).Oid) { - return evalCtx.Planner.HasAnyPrivilege(ctx, specifier, evalCtx.SessionData().User(), privs) + return evalCtx.Planner.HasAnyPrivilegeForSpecifier(ctx, specifier, evalCtx.SessionData().User(), privs) } // For builtin functions, all users should have `EXECUTE` privilege, but @@ -1465,7 +1465,7 @@ var pgBuiltins = map[string]builtinDefinition{ return eval.ObjectNotFound, nil } - return evalCtx.Planner.HasAnyPrivilege(ctx, specifier, user, privs) + return evalCtx.Planner.HasAnyPrivilegeForSpecifier(ctx, specifier, user, privs) }, ), @@ -1491,7 +1491,7 @@ var pgBuiltins = map[string]builtinDefinition{ if err != nil { return eval.HasPrivilege, err } - return evalCtx.Planner.HasAnyPrivilege(ctx, specifier, user, privs) + return evalCtx.Planner.HasAnyPrivilegeForSpecifier(ctx, specifier, user, privs) }, ), @@ -1564,7 +1564,7 @@ var pgBuiltins = map[string]builtinDefinition{ if err != nil { return eval.HasNoPrivilege, err } - return evalCtx.Planner.HasAnyPrivilege(ctx, specifier, user, privs) + return evalCtx.Planner.HasAnyPrivilegeForSpecifier(ctx, specifier, user, privs) }, ), diff --git a/pkg/sql/sem/eval/deps.go b/pkg/sql/sem/eval/deps.go index 4e1b72d1de48..1ec74f89b33c 100644 --- a/pkg/sql/sem/eval/deps.go +++ b/pkg/sql/sem/eval/deps.go @@ -65,9 +65,9 @@ type DatabaseCatalog interface { // whether it exists. SchemaExists(ctx context.Context, dbName, scName string) (found bool, err error) - // HasAnyPrivilege returns whether the current user has privilege to access - // the given object. - HasAnyPrivilege(ctx context.Context, specifier HasPrivilegeSpecifier, user username.SQLUsername, privs []privilege.Privilege) (HasAnyPrivilegeResult, error) + // HasAnyPrivilegeForSpecifier returns whether the current user has privilege + // to access the given object. + HasAnyPrivilegeForSpecifier(ctx context.Context, specifier HasPrivilegeSpecifier, user username.SQLUsername, privs []privilege.Privilege) (HasAnyPrivilegeResult, error) } // CastFunc is a function which cases a datum to a given type. @@ -357,6 +357,9 @@ type Planner interface { // statements, SELECT, UPDATE, INSERT, DELETE, or an EXPLAIN of one of these // statements. IsANSIDML() bool + + // GetRangeDescByID gets the RangeDescriptor by the specified RangeID. + GetRangeDescByID(context.Context, roachpb.RangeID) (roachpb.RangeDescriptor, error) } // InternalRows is an iterator interface that's exposed by the internal diff --git a/pkg/sql/sem/tree/datum.go b/pkg/sql/sem/tree/datum.go index fbad6bb1d982..116c2dc1b177 100644 --- a/pkg/sql/sem/tree/datum.go +++ b/pkg/sql/sem/tree/datum.go @@ -5766,7 +5766,7 @@ var baseDatumTypeSizes = map[types.Family]struct { types.JsonFamily: {unsafe.Sizeof(DJSON{}), variableSize}, types.UuidFamily: {unsafe.Sizeof(DUuid{}), fixedSize}, types.INetFamily: {unsafe.Sizeof(DIPAddr{}), fixedSize}, - types.OidFamily: {unsafe.Sizeof(DInt(0)), fixedSize}, + types.OidFamily: {unsafe.Sizeof(DOid{}.Oid), fixedSize}, types.EnumFamily: {unsafe.Sizeof(DEnum{}), variableSize}, types.VoidFamily: {sz: unsafe.Sizeof(DVoid{}), variable: fixedSize}, diff --git a/pkg/sql/tenant_creation.go b/pkg/sql/tenant_creation.go index b42e5ecd6549..137f0e3e16f4 100644 --- a/pkg/sql/tenant_creation.go +++ b/pkg/sql/tenant_creation.go @@ -314,16 +314,32 @@ func CreateTenantRecord( // Make it behave like usual system database ranges, for good measure. tenantSpanConfig.GCPolicy.IgnoreStrictEnforcement = true - tenantPrefix := keys.MakeTenantPrefix(roachpb.MustMakeTenantID(tenID)) - record, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(roachpb.Span{ + tenantID := roachpb.MustMakeTenantID(tenID) + + // This adds a split at the start of the tenant keyspace. + tenantPrefix := keys.MakeTenantPrefix(tenantID) + startRecord, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(roachpb.Span{ Key: tenantPrefix, EndKey: tenantPrefix.Next(), }), tenantSpanConfig) if err != nil { return roachpb.TenantID{}, err } - toUpsert := []spanconfig.Record{record} - return roachpb.MustMakeTenantID(tenID), spanConfigs.UpdateSpanConfigRecords( + + // This adds a split at the end of the tenant keyspace. This split would + // eventually be created when the next tenant is created, but until then + // this tenant's EndKey will be /Max which is outside of it's keyspace. + tenantPrefixEnd := tenantPrefix.PrefixEnd() + endRecord, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(roachpb.Span{ + Key: tenantPrefixEnd, + EndKey: tenantPrefixEnd.Next(), + }), tenantSpanConfig) + if err != nil { + return roachpb.TenantID{}, err + } + + toUpsert := []spanconfig.Record{startRecord, endRecord} + return tenantID, spanConfigs.UpdateSpanConfigRecords( ctx, nil, toUpsert, hlc.MinTimestamp, hlc.MaxTimestamp, ) } diff --git a/pkg/sql/user.go b/pkg/sql/user.go index c07972aa11e0..0ff58cbe52ea 100644 --- a/pkg/sql/user.go +++ b/pkg/sql/user.go @@ -307,7 +307,9 @@ func retrieveAuthInfo( return aInfo, err } if !hasAdmin { - if noSQLLogin := aa.CheckPrivilegeForUser(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.NOSQLLOGIN, user) == nil; noSQLLogin { + if ok, err = aa.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.NOSQLLOGIN, user); err != nil { + return aInfo, err + } else if ok { aInfo.CanLoginSQL = false } } diff --git a/pkg/storage/fingerprint_writer.go b/pkg/storage/fingerprint_writer.go index c3be092a2fb8..9f4357a4c513 100644 --- a/pkg/storage/fingerprint_writer.go +++ b/pkg/storage/fingerprint_writer.go @@ -191,7 +191,6 @@ func FingerprintRangekeys( ) (uint64, error) { ctx, sp := tracing.ChildSpan(ctx, "storage.FingerprintRangekeys") defer sp.Finish() - _ = ctx // ctx is currently unused, but this new ctx should be used below in the future. if len(ssts) == 0 { return 0, nil diff --git a/pkg/storage/mvcc.go b/pkg/storage/mvcc.go index 51e5c8d2be70..6491aa6a28dd 100644 --- a/pkg/storage/mvcc.go +++ b/pkg/storage/mvcc.go @@ -6151,10 +6151,13 @@ func MVCCIsSpanEmpty( // Range keys are not fingerprinted but instead written to a pebble SST that is // returned to the caller. This is because range keys do not have a stable, // discrete identity and so it is up to the caller to define a deterministic -// fingerprinting scheme across all returned range keys. +// fingerprinting scheme across all returned range keys. The returned boolean +// indicates whether any rangekeys were encountered during the export, this bool +// is used by the caller to throw away the empty SST file and avoid unnecessary +// allocations. func MVCCExportFingerprint( ctx context.Context, cs *cluster.Settings, reader Reader, opts MVCCExportOptions, dest io.Writer, -) (roachpb.BulkOpSummary, MVCCKey, uint64, error) { +) (roachpb.BulkOpSummary, MVCCKey, uint64, bool, error) { ctx, span := tracing.ChildSpan(ctx, "storage.MVCCExportFingerprint") defer span.Finish() @@ -6164,11 +6167,16 @@ func MVCCExportFingerprint( summary, resumeKey, err := mvccExportToWriter(ctx, reader, opts, &fingerprintWriter) if err != nil { - return roachpb.BulkOpSummary{}, MVCCKey{}, 0, err + return roachpb.BulkOpSummary{}, MVCCKey{}, 0, false, err } fingerprint, err := fingerprintWriter.Finish() - return summary, resumeKey, fingerprint, err + if err != nil { + return roachpb.BulkOpSummary{}, MVCCKey{}, 0, false, err + } + + hasRangeKeys := fingerprintWriter.sstWriter.DataSize != 0 + return summary, resumeKey, fingerprint, hasRangeKeys, err } // MVCCExportToSST exports changes to the keyrange [StartKey, EndKey) over the @@ -6191,9 +6199,9 @@ func MVCCExportToSST( // If no records were added to the sstable, skip // completing it and return an empty summary. // - // We still propogate the resumeKey because our + // We still propagate the resumeKey because our // iteration may have been halted because of resource - // limitiations before any keys were added to the + // limitations before any keys were added to the // returned SST. return roachpb.BulkOpSummary{}, resumeKey, nil } diff --git a/pkg/storage/mvcc_history_test.go b/pkg/storage/mvcc_history_test.go index efb49de7cf17..49e384928748 100644 --- a/pkg/storage/mvcc_history_test.go +++ b/pkg/storage/mvcc_history_test.go @@ -1388,12 +1388,17 @@ func cmdExport(e *evalCtx) error { var summary roachpb.BulkOpSummary var resume storage.MVCCKey var fingerprint uint64 + var hasRangeKeys bool var err error if shouldFingerprint { - summary, resume, fingerprint, err = storage.MVCCExportFingerprint(e.ctx, e.st, r, opts, sstFile) + summary, resume, fingerprint, hasRangeKeys, err = storage.MVCCExportFingerprint(e.ctx, e.st, r, + opts, sstFile) if err != nil { return err } + if !hasRangeKeys { + sstFile = &storage.MemFile{} + } e.results.buf.Printf("export: %s", &summary) e.results.buf.Print(" fingerprint=true") } else { @@ -1410,9 +1415,12 @@ func cmdExport(e *evalCtx) error { e.results.buf.Printf("\n") if shouldFingerprint { + var ssts [][]byte + if sstFile.Len() != 0 { + ssts = append(ssts, sstFile.Bytes()) + } // Fingerprint the rangekeys returned as a pebble SST. - rangekeyFingerprint, err := storage.FingerprintRangekeys(e.ctx, e.st, opts.FingerprintOptions, - [][]byte{sstFile.Bytes()}) + rangekeyFingerprint, err := storage.FingerprintRangekeys(e.ctx, e.st, opts.FingerprintOptions, ssts) if err != nil { return err } diff --git a/pkg/storage/mvcc_test.go b/pkg/storage/mvcc_test.go index 92b158301143..604abc7084e7 100644 --- a/pkg/storage/mvcc_test.go +++ b/pkg/storage/mvcc_test.go @@ -6441,9 +6441,12 @@ func TestMVCCExportFingerprint(t *testing.T) { fingerprint := func(opts MVCCExportOptions, engine Engine) (uint64, []byte, roachpb.BulkOpSummary, MVCCKey) { dest := &MemFile{} var err error - res, resumeKey, fingerprint, err := MVCCExportFingerprint( + res, resumeKey, fingerprint, hasRangeKeys, err := MVCCExportFingerprint( ctx, st, engine, opts, dest) require.NoError(t, err) + if !hasRangeKeys { + dest = &MemFile{} + } return fingerprint, dest.Data(), res, resumeKey } @@ -6751,6 +6754,10 @@ func (f *fingerprintOracle) fingerprintPointKeys(t *testing.T, dataSST []byte) u func getRangeKeys(t *testing.T, dataSST []byte) []MVCCRangeKeyStack { t.Helper() + if len(dataSST) == 0 { + return []MVCCRangeKeyStack{} + } + iterOpts := IterOptions{ KeyTypes: IterKeyTypeRangesOnly, LowerBound: keys.LocalMax, diff --git a/pkg/storage/sst_writer.go b/pkg/storage/sst_writer.go index d9773a7df32c..7ca4f910c1f6 100644 --- a/pkg/storage/sst_writer.go +++ b/pkg/storage/sst_writer.go @@ -26,7 +26,6 @@ import ( // SSTWriter writes SSTables. type SSTWriter struct { fw *sstable.Writer - f io.Writer // DataSize tracks the total key and value bytes added so far. DataSize int64 scratch []byte @@ -101,7 +100,6 @@ func MakeBackupSSTWriter(ctx context.Context, cs *cluster.Settings, f io.Writer) opts.MergerName = "nullptr" return SSTWriter{ fw: sstable.NewWriter(noopSyncCloser{f}, opts), - f: f, supportsRangeKeys: opts.TableFormat >= sstable.TableFormatPebblev2, } } @@ -115,7 +113,6 @@ func MakeIngestionSSTWriter( opts := MakeIngestionWriterOptions(ctx, cs) return SSTWriter{ fw: sstable.NewWriter(f, opts), - f: f, supportsRangeKeys: opts.TableFormat >= sstable.TableFormatPebblev2, } } diff --git a/pkg/testutils/serverutils/test_tenant_shim.go b/pkg/testutils/serverutils/test_tenant_shim.go index 3e5ca8a703ea..8db2ad2af07b 100644 --- a/pkg/testutils/serverutils/test_tenant_shim.go +++ b/pkg/testutils/serverutils/test_tenant_shim.go @@ -170,9 +170,17 @@ type TestTenantInterface interface { // as an interface{}. RangeDescIteratorFactory() interface{} - //Tracer returns a reference to the tenant's Tracer. + // Tracer returns a reference to the tenant's Tracer. Tracer() *tracing.Tracer + // WaitForTenantEndKeySplit blocks until the tenant's initial range is split + // at the end key. For example, this will wait until tenant 10 has a split at + // /Tenant/11. + // + // Tests that use crdb_internal.ranges, crdb_internal.ranges_no_leases, or + // SHOW RANGES from a secondary tenant should call this to avoid races. + WaitForTenantEndKeySplit(ctx context.Context) error + // TODO(irfansharif): We'd benefit from an API to construct a *gosql.DB, or // better yet, a *sqlutils.SQLRunner. We use it all the time, constructing // it by hand each time.