Skip to content

Commit

Permalink
server: Reject SQL Pods with an invalid tenant id.
Browse files Browse the repository at this point in the history
This commit fixes an issue where attempting to start a SQL Pod
through cockroach mt start-sql command returns an error if the
tenant id has not been created before. Previously, attempting to
start a SQL Pod with a non existent tenant id would crash. Even
worse, it would poison the tenant id causing future
crdb_internal.create_sql_tenant calls to fail.
With this fix, the cockroach mt start-sql command returns an error
for non existent tenant ids and future calls to
crdb_internal.create_sql_tenant are successful.
Resolves: #64963
Release note (bug fix): The cockroach mt start-sql command with a
non existent tenant id returns an error. Previously, it would
crash and poison the tenant id for future usage.
  • Loading branch information
rimadeodhar committed May 26, 2021
1 parent c04887f commit 0ad0476
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 1 deletion.
1 change: 1 addition & 0 deletions pkg/server/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ go_library(
"//pkg/settings/cluster",
"//pkg/sql",
"//pkg/sql/catalog/bootstrap",
"//pkg/sql/catalog/catalogkeys",
"//pkg/sql/catalog/catconstants",
"//pkg/sql/catalog/colinfo",
"//pkg/sql/catalog/descpb",
Expand Down
32 changes: 31 additions & 1 deletion pkg/server/server_sql.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ package server

import (
"context"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema"
"math"
"net"
"os"
Expand Down Expand Up @@ -252,7 +254,6 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) {
codec = keys.MakeSQLCodec(override)
}
}

// Create blob service for inter-node file sharing.
blobService, err := blobs.NewBlobService(cfg.Settings.ExternalIODir)
if err != nil {
Expand Down Expand Up @@ -745,6 +746,28 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) {
}, nil
}

// Checks if tenant exists. This function does a very superficial check to see if the system db
// has been bootstrapped for the tenant. This is not a complete check and is only sufficient
// to be used in the dev environment.
func maybeCheckTenantExists(ctx context.Context, codec keys.SQLCodec, db *kv.DB) error {
if codec.ForSystemTenant() {
// Skip check for system tenant and return early
return nil
}
key := catalogkeys.NewDatabaseKey(systemschema.SystemDatabaseName).Key(codec)
result, err := db.Get(ctx, key)
if err != nil {
return err
}
if result.Value == nil || result.ValueInt() != keys.SystemDatabaseID {
return errors.New("System DB uninitialized, check if tenant is non existent")
}
// Tenant has been confirmed to be bootstrapped successfully
// as system database, which is a part of the bootstrap data for
// a tenant keyspace, exists in the namespace table.
return nil
}

func (s *SQLServer) preStart(
ctx context.Context,
stopper *stop.Stopper,
Expand All @@ -763,6 +786,13 @@ func (s *SQLServer) preStart(
return err
}
}
// Confirm tenant exists prior to initialization. This is a sanity
// check for the dev environment to ensure that a tenant has been
// successfully created before attempting to initialize a SQL
// server for it.
if err := maybeCheckTenantExists(ctx, s.execCfg.Codec, s.execCfg.DB); err != nil {
return err
}
s.connManager = connManager
s.pgL = pgL
s.execCfg.GCJobNotifier.Start(ctx)
Expand Down

0 comments on commit 0ad0476

Please sign in to comment.