From 5ffaf44bab4bb6f88a4acdc52aa2276eff69d760 Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Mon, 10 Aug 2020 16:03:29 -0400 Subject: [PATCH] sql: don't query BootstrapVersionKey on tenant SQL startup See https://github.com/cockroachdb/cockroach/pull/52094#pullrequestreview-461140089. We don't currently track the bootstrap version of each secondary tenant. For this to be meaningful, we'd need to record the binary version of the SQL gateway that processed the crdb_internal.create_tenant function which created the tenant, as this is what dictates the MetadataSchema that was in effect when the secondary tenant was constructed. This binary version very well may differ from the cluster-wide bootstrap version at which the system tenant was bootstrapped. Since we don't record this version anywhere, we do the next-best thing and pass a lower-bound on the bootstrap version. We know that no tenants could have been created before the start of the v20.2 dev cycle, so we pass VersionStart20_2. bootstrapVersion is only used to avoid performing superfluous but necessarily idempotent SQL migrations, so at worst, we're doing more work than strictly necessary during the first time that the migrations are run. Now that we don't query BootstrapVersionKey, we don't need to have it in the allowlists in the tenantAuth policy for Batch and RangeLookup RPCs. --- pkg/rpc/auth_tenant.go | 41 ++++++---------------------------------- pkg/server/server_sql.go | 29 ++++++++++++++++++++++++---- 2 files changed, 31 insertions(+), 39 deletions(-) diff --git a/pkg/rpc/auth_tenant.go b/pkg/rpc/auth_tenant.go index 6d47a0becd3b..30edfef3ef69 100644 --- a/pkg/rpc/auth_tenant.go +++ b/pkg/rpc/auth_tenant.go @@ -121,26 +121,10 @@ func (a tenantAuth) authBatch(tenID roachpb.TenantID, args *roachpb.BatchRequest return authError(err.Error()) } tenSpan := tenantPrefix(tenID) - if tenSpan.ContainsKeyRange(rSpan.Key, rSpan.EndKey) { - return nil - } - for _, allow := range batchSpanAllowlist { - if rSpan.Equal(allow) { - return nil - } + if !tenSpan.ContainsKeyRange(rSpan.Key, rSpan.EndKey) { + return authErrorf("requested key span %s not fully contained in tenant keyspace %s", rSpan, tenSpan) } - return authErrorf("requested key span %s not fully contained in tenant keyspace %s", rSpan, tenSpan) -} - -// batchSpanAllowlist contains spans outside of a tenant's keyspace that Batch -// RPC invocations are allowed to touch. -var batchSpanAllowlist = []roachpb.RSpan{ - // TODO(nvanbenschoten): Explore whether we can get rid of this by no longer - // reading this key in sqlServer.start. - { - Key: roachpb.RKey(keys.BootstrapVersionKey), - EndKey: roachpb.RKey(keys.BootstrapVersionKey.Next()), - }, + return nil } // authRangeLookup authorizes the provided tenant to invoke the RangeLookup RPC @@ -149,23 +133,10 @@ func (a tenantAuth) authRangeLookup( tenID roachpb.TenantID, args *roachpb.RangeLookupRequest, ) error { tenSpan := tenantPrefix(tenID) - if tenSpan.ContainsKey(args.Key) { - return nil - } - for _, allow := range rangeLookupKeyAllowlist { - if args.Key.Equal(allow) { - return nil - } + if !tenSpan.ContainsKey(args.Key) { + return authErrorf("requested key %s not fully contained in tenant keyspace %s", args.Key, tenSpan) } - return authErrorf("requested key %s not fully contained in tenant keyspace %s", args.Key, tenSpan) -} - -// rangeLookupKeyAllowlist contains keys outside of a tenant's keyspace that -// RangeLookup RPC invocations are allowed to touch. -var rangeLookupKeyAllowlist = []roachpb.Key{ - // TODO(nvanbenschoten): Explore whether we can get rid of this by no longer - // reading this key in sqlServer.start. - keys.BootstrapVersionKey, + return nil } // authRangeFeed authorizes the provided tenant to invoke the RangeFeed RPC with diff --git a/pkg/server/server_sql.go b/pkg/server/server_sql.go index 3ccd1f67e5a5..adb73cad55f2 100644 --- a/pkg/server/server_sql.go +++ b/pkg/server/server_sql.go @@ -21,6 +21,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/blobs" "github.com/cockroachdb/cockroach/pkg/blobs/blobspb" + "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/config" "github.com/cockroachdb/cockroach/pkg/gossip" "github.com/cockroachdb/cockroach/pkg/jobs" @@ -670,11 +671,31 @@ func (s *sqlServer) start( } var bootstrapVersion roachpb.Version - if err := s.execCfg.DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error { - return txn.GetProto(ctx, keys.BootstrapVersionKey, &bootstrapVersion) - }); err != nil { - return err + if s.execCfg.Codec.ForSystemTenant() { + if err := s.execCfg.DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error { + return txn.GetProto(ctx, keys.BootstrapVersionKey, &bootstrapVersion) + }); err != nil { + return err + } + } else { + // We don't currently track the bootstrap version of each secondary tenant. + // For this to be meaningful, we'd need to record the binary version of the + // SQL gateway that processed the crdb_internal.create_tenant function which + // created the tenant, as this is what dictates the MetadataSchema that was + // in effect when the secondary tenant was constructed. This binary version + // very well may differ from the cluster-wide bootstrap version at which the + // system tenant was bootstrapped. + // + // Since we don't record this version anywhere, we do the next-best thing + // and pass a lower-bound on the bootstrap version. We know that no tenants + // could have been created before the start of the v20.2 dev cycle, so we + // pass VersionStart20_2. bootstrapVersion is only used to avoid performing + // superfluous but necessarily idempotent SQL migrations, so at worst, we're + // doing more work than strictly necessary during the first time that the + // migrations are run. + bootstrapVersion = clusterversion.VersionByKey(clusterversion.VersionStart20_2) } + // Run startup migrations (note: these depend on jobs subsystem running). if err := migMgr.EnsureMigrations(ctx, bootstrapVersion); err != nil { return errors.Wrap(err, "ensuring SQL migrations")