diff --git a/pkg/base/test_server_args.go b/pkg/base/test_server_args.go index 4ca768dfc617..010f55a7ce1b 100644 --- a/pkg/base/test_server_args.go +++ b/pkg/base/test_server_args.go @@ -218,4 +218,8 @@ type TestTenantArgs struct { // AllowSettingClusterSettings, if true, allows the tenant to set in-memory // cluster settings. AllowSettingClusterSettings bool + + // TenantIDCodecOverride overrides the tenant ID used to construct the SQL + // server's codec, but nothing else (e.g. its certs). Used for testing. + TenantIDCodecOverride roachpb.TenantID } diff --git a/pkg/ccl/kvccl/kvtenantccl/proxy.go b/pkg/ccl/kvccl/kvtenantccl/proxy.go index f847fe8e88a4..4e935d5ec462 100644 --- a/pkg/ccl/kvccl/kvtenantccl/proxy.go +++ b/pkg/ccl/kvccl/kvtenantccl/proxy.go @@ -25,14 +25,13 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/rpc" "github.com/cockroachdb/cockroach/pkg/util/contextutil" + "github.com/cockroachdb/cockroach/pkg/util/grpcutil" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/retry" "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/cockroachdb/cockroach/pkg/util/syncutil/singleflight" "github.com/cockroachdb/errors" "google.golang.org/grpc" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) func init() { @@ -297,7 +296,7 @@ func (p *Proxy) RangeLookup( }) if err != nil { log.Warningf(ctx, "error issuing RangeLookup RPC: %v", err) - if status.Code(err) == codes.Unauthenticated { + if grpcutil.IsAuthenticationError(err) { // Authentication error. Propagate. return nil, nil, err } diff --git a/pkg/ccl/serverccl/server_sql_test.go b/pkg/ccl/serverccl/server_sql_test.go index 4e07fc073016..b6bfda31aaa7 100644 --- a/pkg/ccl/serverccl/server_sql_test.go +++ b/pkg/ccl/serverccl/server_sql_test.go @@ -77,3 +77,20 @@ func TestTenantCannotSetClusterSetting(t *testing.T) { require.True(t, ok, "expected err to be a *pq.Error but is of type %T. error is: %v", err) require.Equal(t, pq.ErrorCode(pgcode.InsufficientPrivilege.String()), pqErr.Code, "err %v has unexpected code", err) } + +func TestTenantUnauthenticatedAccess(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + ctx := context.Background() + + tc := serverutils.StartTestCluster(t, 1, base.TestClusterArgs{}) + defer tc.Stopper().Stop(ctx) + + _, err := tc.Server(0).StartTenant(base.TestTenantArgs{ + TenantID: roachpb.MakeTenantID(security.EmbeddedTenantIDs()[0]), + // Configure the SQL server to access the wrong tenant keyspace. + TenantIDCodecOverride: roachpb.MakeTenantID(security.EmbeddedTenantIDs()[1]), + }) + require.Error(t, err) + require.Regexp(t, `Unauthenticated desc = requested key /Tenant/11/System/"system-version/" not fully contained in tenant keyspace /Tenant/1{0-1}`, err) +} diff --git a/pkg/kv/kvclient/kvcoord/dist_sender.go b/pkg/kv/kvclient/kvcoord/dist_sender.go index df9c0e729fa7..ca39073ea4f0 100644 --- a/pkg/kv/kvclient/kvcoord/dist_sender.go +++ b/pkg/kv/kvclient/kvcoord/dist_sender.go @@ -1007,7 +1007,7 @@ func (ds *DistSender) detectIntentMissingDueToIntentResolution( // We weren't able to determine whether the intent missing error is // due to intent resolution or not, so it is still ambiguous whether // the commit succeeded. - return false, roachpb.NewAmbiguousResultError(fmt.Sprintf("error=%s [intent missing]", pErr)) + return false, roachpb.NewAmbiguousResultErrorf("error=%s [intent missing]", pErr) } respTxn := &br.Responses[0].GetQueryTxn().QueriedTxn switch respTxn.Status { @@ -1032,7 +1032,7 @@ func (ds *DistSender) detectIntentMissingDueToIntentResolution( // to further isolates the ambiguity caused by the loss of information // during intent resolution. If this error becomes a problem, we can explore // this option. - return false, roachpb.NewAmbiguousResultError("intent missing and record aborted") + return false, roachpb.NewAmbiguousResultErrorf("intent missing and record aborted") default: // The transaction has not been finalized yet, so the missing intent // error must have been caused by a real missing intent. Propagate the @@ -1433,6 +1433,9 @@ func (ds *DistSender) sendPartialBatch( // We set pErr if we encountered an error getting the descriptor in // order to return the most recent error when we are out of retries. pErr = roachpb.NewError(err) + if !isRangeLookupErrorRetryable(err) { + return response{pErr: roachpb.NewError(err)} + } continue } @@ -1676,7 +1679,7 @@ func fillSkippedResponses( // the error that the last attempt to execute the request returned. func noMoreReplicasErr(ambiguousErr, lastAttemptErr error) error { if ambiguousErr != nil { - return roachpb.NewAmbiguousResultError(fmt.Sprintf("error=%s [exhausted]", ambiguousErr)) + return roachpb.NewAmbiguousResultErrorf("error=%s [exhausted]", ambiguousErr) } // TODO(bdarnell): The error from the last attempt is not necessarily the best @@ -1805,6 +1808,14 @@ func (ds *DistSender) sendToReplicas( br, err = transport.SendNext(ctx, ba) if err != nil { + if grpcutil.IsAuthenticationError(err) { + // Authentication error. Propagate. + if ambiguousError != nil { + return nil, roachpb.NewAmbiguousResultErrorf("error=%s [propagate]", ambiguousError) + } + return nil, err + } + // For most connection errors, we cannot tell whether or not the request // may have succeeded on the remote server (exceptions are captured in the // grpcutil.RequestDidNotStart function). We'll retry the request in order @@ -1940,7 +1951,7 @@ func (ds *DistSender) sendToReplicas( } default: if ambiguousError != nil { - return nil, roachpb.NewAmbiguousResultError(fmt.Sprintf("error=%s [propagate]", ambiguousError)) + return nil, roachpb.NewAmbiguousResultErrorf("error=%s [propagate]", ambiguousError) } // The error received is likely not specific to this @@ -1957,7 +1968,7 @@ func (ds *DistSender) sendToReplicas( reportedErr := errors.Wrap(ctx.Err(), "context done during DistSender.Send") log.Eventf(ctx, "%v", reportedErr) if ambiguousError != nil { - return nil, roachpb.NewAmbiguousResultError(reportedErr.Error()) + return nil, roachpb.NewAmbiguousResultErrorf(reportedErr.Error()) } // Don't consider this a sendError, because sendErrors indicate that we // were unable to reach a replica that could serve the request, and they diff --git a/pkg/kv/kvclient/kvcoord/dist_sender_rangefeed.go b/pkg/kv/kvclient/kvcoord/dist_sender_rangefeed.go index 0d74a56975ce..612c41bb67b4 100644 --- a/pkg/kv/kvclient/kvcoord/dist_sender_rangefeed.go +++ b/pkg/kv/kvclient/kvcoord/dist_sender_rangefeed.go @@ -19,6 +19,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/rpc" "github.com/cockroachdb/cockroach/pkg/util/ctxgroup" + "github.com/cockroachdb/cockroach/pkg/util/grpcutil" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/retry" @@ -137,6 +138,9 @@ func (ds *DistSender) partialRangeFeed( ri, err := ds.getRoutingInfo(ctx, rangeInfo.rs.Key, EvictionToken{}, false) if err != nil { log.VErrEventf(ctx, 1, "range descriptor re-lookup failed: %s", err) + if !isRangeLookupErrorRetryable(err) { + return err + } continue } rangeInfo.token = ri @@ -253,6 +257,10 @@ func (ds *DistSender) singleRangeFeed( stream, err := client.RangeFeed(clientCtx, &args) if err != nil { log.VErrEventf(ctx, 2, "RPC error: %s", err) + if grpcutil.IsAuthenticationError(err) { + // Authentication error. Propagate. + return args.Timestamp, err + } continue } for { diff --git a/pkg/kv/kvclient/kvcoord/range_cache.go b/pkg/kv/kvclient/kvcoord/range_cache.go index 4f2487b3882b..caa65dfbbc98 100644 --- a/pkg/kv/kvclient/kvcoord/range_cache.go +++ b/pkg/kv/kvclient/kvcoord/range_cache.go @@ -26,6 +26,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/cache" "github.com/cockroachdb/cockroach/pkg/util/contextutil" + "github.com/cockroachdb/cockroach/pkg/util/grpcutil" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/stop" "github.com/cockroachdb/cockroach/pkg/util/syncutil" @@ -1261,3 +1262,10 @@ func (e *rangeCacheEntry) updateLease(l *roachpb.Lease) (updated bool, newEntry lease: *l, } } + +// isRangeLookupErrorRetryable returns whether the provided range lookup error +// can be retried or whether it should be propagated immediately. +func isRangeLookupErrorRetryable(err error) bool { + // For now, all errors are retryable except authentication errors. + return !grpcutil.IsAuthenticationError(err) +} diff --git a/pkg/kv/kvclient/kvcoord/range_iter.go b/pkg/kv/kvclient/kvcoord/range_iter.go index 13387ccc3469..d45479251230 100644 --- a/pkg/kv/kvclient/kvcoord/range_iter.go +++ b/pkg/kv/kvclient/kvcoord/range_iter.go @@ -183,6 +183,9 @@ func (ri *RangeIterator) Seek(ctx context.Context, key roachpb.RKey, scanDir Sca // for before reaching this point. if err != nil { log.VEventf(ctx, 1, "range descriptor lookup failed: %s", err) + if !isRangeLookupErrorRetryable(err) { + break + } continue } if log.V(2) { diff --git a/pkg/roachpb/errors.go b/pkg/roachpb/errors.go index cb486968a7e1..fa57dcf5953c 100644 --- a/pkg/roachpb/errors.go +++ b/pkg/roachpb/errors.go @@ -461,6 +461,12 @@ func NewAmbiguousResultError(msg string) *AmbiguousResultError { return &AmbiguousResultError{Message: msg} } +// NewAmbiguousResultErrorf initializes a new AmbiguousResultError with +// an explanatory format and set of arguments. +func NewAmbiguousResultErrorf(format string, args ...interface{}) *AmbiguousResultError { + return NewAmbiguousResultError(fmt.Sprintf(format, args...)) +} + func (e *AmbiguousResultError) Error() string { return e.message(nil) } diff --git a/pkg/rpc/context.go b/pkg/rpc/context.go index ae287b2eaaa5..b941a950abd6 100644 --- a/pkg/rpc/context.go +++ b/pkg/rpc/context.go @@ -710,14 +710,13 @@ func (ctx *Context) grpcDialOptions( if ctx.Config.Insecure { dialOpts = append(dialOpts, grpc.WithInsecure()) } else { - var err error var tlsConfig *tls.Config + var err error if ctx.tenID == roachpb.SystemTenantID { tlsConfig, err = ctx.GetClientTLSConfig() } else { tlsConfig, err = ctx.GetTenantClientTLSConfig() } - if err != nil { return nil, err } diff --git a/pkg/server/config.go b/pkg/server/config.go index 547e423e85ec..a073ccccae90 100644 --- a/pkg/server/config.go +++ b/pkg/server/config.go @@ -332,6 +332,12 @@ type SQLConfig struct { // // Only applies when the SQL server is deployed individually. TenantKVAddrs []string + + // TenantIDCodecOverride overrides the tenant ID used to construct the SQL + // server's codec, but nothing else (e.g. its certs). Used for testing. + // + // Only applies when the SQL server is deployed individually. + TenantIDCodecOverride roachpb.TenantID } // MakeSQLConfig returns a SQLConfig with default values. diff --git a/pkg/server/server_sql.go b/pkg/server/server_sql.go index 3ccd1f67e5a5..5093de67a973 100644 --- a/pkg/server/server_sql.go +++ b/pkg/server/server_sql.go @@ -187,6 +187,9 @@ type sqlServerArgs struct { func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*sqlServer, error) { execCfg := &sql.ExecutorConfig{} codec := keys.MakeSQLCodec(cfg.SQLConfig.TenantID) + if override := cfg.SQLConfig.TenantIDCodecOverride; override != (roachpb.TenantID{}) { + codec = keys.MakeSQLCodec(override) + } // Create blob service for inter-node file sharing. blobService, err := blobs.NewBlobService(cfg.Settings.ExternalIODir) diff --git a/pkg/server/testserver.go b/pkg/server/testserver.go index 520e2f657908..0a82fba7f8e3 100644 --- a/pkg/server/testserver.go +++ b/pkg/server/testserver.go @@ -616,13 +616,14 @@ func (ts *TestServer) StartTenant(params base.TestTenantArgs) (pgAddr string, _ st := cluster.MakeTestingClusterSettings() sqlCfg := makeTestSQLConfig(st, params.TenantID) + sqlCfg.TenantKVAddrs = []string{ts.ServingTenantAddr()} + sqlCfg.TenantIDCodecOverride = params.TenantIDCodecOverride baseCfg := makeTestBaseConfig(st) if params.AllowSettingClusterSettings { baseCfg.TestingKnobs.TenantTestingKnobs = &sql.TenantTestingKnobs{ ClusterSettingsUpdater: st.MakeUpdater(), } } - sqlCfg.TenantKVAddrs = []string{ts.ServingTenantAddr()} return StartTenant( ctx, ts.Stopper(), diff --git a/pkg/util/grpcutil/grpc_util.go b/pkg/util/grpcutil/grpc_util.go index 9a63a20b2ed7..a20603b8c058 100644 --- a/pkg/util/grpcutil/grpc_util.go +++ b/pkg/util/grpcutil/grpc_util.go @@ -88,6 +88,15 @@ func IsClosedConnection(err error) bool { return netutil.IsClosedConnection(err) } +// IsAuthenticationError returns true if err's Cause is an error produced by +// gRPC due to invalid authentication credentials for the operation. +func IsAuthenticationError(err error) bool { + if s, ok := status.FromError(errors.UnwrapAll(err)); ok { + return s.Code() == codes.Unauthenticated + } + return false +} + // RequestDidNotStart returns true if the given error from gRPC // means that the request definitely could not have started on the // remote server.