Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server: Reject SQL Pods with an invalid tenant id. #65683

Merged
merged 1 commit into from
Jun 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pkg/base/test_server_args.go
Original file line number Diff line number Diff line change
Expand Up @@ -268,4 +268,7 @@ type TestTenantArgs struct {
// automatically open a connection to the server. That's equivalent to running
// SET DATABASE=foo, which works even if the database doesn't (yet) exist.
UseDatabase string

// Skip check for tenant existence when running the test.
SkipTenantCheck bool
}
18 changes: 18 additions & 0 deletions pkg/ccl/serverccl/server_sql_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -205,3 +205,21 @@ func TestIdleExit(t *testing.T) {
t.Error("stop on idle didn't trigger")
}
}

func TestNonExistentTenant(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
ctx := context.Background()

tc := serverutils.StartNewTestCluster(t, 1, base.TestClusterArgs{})
defer tc.Stopper().Stop(ctx)

_, err := tc.Server(0).StartTenant(ctx,
base.TestTenantArgs{
TenantID: serverutils.TestTenantID(),
Existing: true,
SkipTenantCheck: true,
})
require.Error(t, err)
require.Equal(t, "system DB uninitialized, check if tenant is non existent", err.Error())
}
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 @@ -47,8 +47,10 @@ import (
"github.com/cockroachdb/cockroach/pkg/server/settingswatcher"
"github.com/cockroachdb/cockroach/pkg/server/status"
"github.com/cockroachdb/cockroach/pkg/sql"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/hydratedtables"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/lease"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema"
"github.com/cockroachdb/cockroach/pkg/sql/colexec"
"github.com/cockroachdb/cockroach/pkg/sql/contention"
"github.com/cockroachdb/cockroach/pkg/sql/distsql"
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 @@ -746,6 +747,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 the 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 @@ -764,6 +787,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
24 changes: 13 additions & 11 deletions pkg/server/testserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -716,18 +716,20 @@ func (ts *TestServer) StartTenant(
}
}

rowCount, err := ts.InternalExecutor().(*sql.InternalExecutor).Exec(
ctx, "testserver-check-tenant-active", nil,
"SELECT 1 FROM system.tenants WHERE id=$1 AND active=true",
params.TenantID.ToUint64(),
)
if err != nil {
return nil, err
}
if rowCount == 0 {
return nil, errors.New("not found")
}
if !params.SkipTenantCheck {
rowCount, err := ts.InternalExecutor().(*sql.InternalExecutor).Exec(
ctx, "testserver-check-tenant-active", nil,
"SELECT 1 FROM system.tenants WHERE id=$1 AND active=true",
params.TenantID.ToUint64(),
)

if err != nil {
return nil, err
}
if rowCount == 0 {
return nil, errors.New("not found")
}
}
st := params.Settings
if st == nil {
st = cluster.MakeTestingClusterSettings()
Expand Down