Skip to content

Commit

Permalink
multitenant: prevent tenant from overriding next tenant's span config
Browse files Browse the repository at this point in the history
Fixes #95882

We want to ensure we have a split at the non-inclusive end of the tenant's
keyspace, which also happens to be the inclusive start of the next
tenant's (with ID=ours+1). If we didn't do anything here, and we were the
tenant with the highest ID thus far, our last range would extend to /Max.
If a tenant with a higher ID was created, when installing its initial span
config record, it would carve itself off (and our last range would only
extend to that next tenant's boundary), but this is a bit awkward for code
that wants to rely on the invariant that ranges for a tenant only extend
to the tenant's well-defined end key.

So how do we ensure this split at this tenant's non-inclusive end key?
Hard splits are controlled by the start keys on span config records[^1],
so we'll try insert one accordingly. But we cannot do this blindly. We
cannot assume that tenants are created in ID order, so it's possible that
the tenant with the next ID is already present + running. If so, it may
already have span config records that start at the key at which we want
to write a span config record for. Over-writing it blindly would be a
mistake -- there's no telling what config that next tenant has associated
for that span. So we'll do something simple -- we'll check transactionally
whether there's anything written already, and if so, do nothing. We
already have the split we need.

[^1]: See ComputeSplitKey in spanconfig.StoreReader.

Release note: None
  • Loading branch information
ecwall committed Feb 7, 2023
1 parent cc99062 commit 9d4e27a
Show file tree
Hide file tree
Showing 4 changed files with 187 additions and 9 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
# Test that tenant creation does not overwrite span config state for another
# tenant.

reconcile
----

mutations discard
----

# Initialize a new tenant, tenant=11, that DOES NOT have a pre-existing tenant,
# tenant=12, next to it.
initialize tenant=11
----

# A record IS written for a key that logically belongs to the next tenant,
# tenant=12, because tenant=12 DOES NOT exist.
state offset=57 limit=3
----
...
/Tenant/11{-\x00} database system (tenant)
/Tenant/12{-\x00} database system (tenant)

# Start the reconciliation loop for the tenant=11. It'll first clear out its own
# first-key record and install span configs for its SQL state. It won't touch
# the record we created at in the tenant=12 keyspace because it's not taught to
# look there, and neither should it. The keyspace it's responsible for is its
# own.
reconcile tenant=11
----

# Peek near the start of the span_configurations table where tenant=11's records
# are stored. The first one is from the start of its keyspace to start of
# table with ID=4: /Tenant/11{-/Table/4}.
state offset=56 limit=3
----
...
/Table/5{7-8} ttl_seconds=3600 ignore_strict_gc=true num_replicas=5 rangefeed_enabled=true
/Tenant/11{-/Table/4} database system (tenant)
/Tenant/11/Table/{4-5} database system (tenant)
...

# Peek near the end of the span_configurations table where tenant=11's records
# are stored. The last one is for its last system table:
# /Tenant/11/Table/5{7-8}. Right now the split is at /Tenant/12. Which is fine.
state offset=99 limit=3
----
...
/Tenant/11/Table/5{7-8} database system (tenant)
/Tenant/12{-\x00} database system (tenant)

# Just another view of what the tenant's reconciler actually did. It got rid of
# the original, single-key /Tenant/11{-\x00} record, and replaced it with
# /Tenant/11{-/Table/4}, just like the comment above said. The remaining upserts
# are for its SQL state.
mutations tenant=11 limit=2
----
delete /Tenant/11{-\x00}
upsert /Tenant/11{-/Table/4} database system (tenant)
upsert /Tenant/11/Table/{4-5} database system (tenant)
upsert /Tenant/11/Table/{5-6} database system (tenant)
upsert /Tenant/11/Table/{6-7} database system (tenant)
upsert /Tenant/11/Table/{7-8} database system (tenant)
upsert /Tenant/11/Table/1{1-2} database system (tenant)
upsert /Tenant/11/Table/1{2-3} database system (tenant)
upsert /Tenant/11/Table/1{3-4} database system (tenant)
upsert /Tenant/11/Table/1{4-5} database system (tenant)
upsert /Tenant/11/Table/1{5-6} database system (tenant)
upsert /Tenant/11/Table/{19-20} database system (tenant)
upsert /Tenant/11/Table/2{0-1} database system (tenant)
upsert /Tenant/11/Table/2{1-2} database system (tenant)
upsert /Tenant/11/Table/2{3-4} database system (tenant)
upsert /Tenant/11/Table/2{4-5} database system (tenant)
upsert /Tenant/11/Table/2{5-6} database system (tenant)
upsert /Tenant/11/Table/2{6-7} database system (tenant)
upsert /Tenant/11/Table/2{7-8} database system (tenant)
upsert /Tenant/11/Table/2{8-9} database system (tenant)
upsert /Tenant/11/NamespaceTable/{30-Max} database system (tenant)
upsert /Tenant/11/{NamespaceTable/Max-Table/32} database system (tenant)
upsert /Tenant/11/Table/3{2-3} database system (tenant)
upsert /Tenant/11/Table/3{3-4} database system (tenant)
upsert /Tenant/11/Table/3{4-5} database system (tenant)
upsert /Tenant/11/Table/3{5-6} database system (tenant)
upsert /Tenant/11/Table/3{6-7} database system (tenant)
upsert /Tenant/11/Table/3{7-8} database system (tenant)
upsert /Tenant/11/Table/{39-40} database system (tenant)
upsert /Tenant/11/Table/4{0-1} database system (tenant)
upsert /Tenant/11/Table/4{1-2} database system (tenant)
upsert /Tenant/11/Table/4{2-3} database system (tenant)
upsert /Tenant/11/Table/4{3-4} database system (tenant)
upsert /Tenant/11/Table/4{4-5} database system (tenant)
upsert /Tenant/11/Table/4{6-7} database system (tenant)
upsert /Tenant/11/Table/4{8-9} database system (tenant)
upsert /Tenant/11/Table/5{0-1} database system (tenant)
upsert /Tenant/11/Table/5{1-2} database system (tenant)
upsert /Tenant/11/Table/5{2-3} database system (tenant)
upsert /Tenant/11/Table/5{3-4} database system (tenant)
upsert /Tenant/11/Table/5{4-5} database system (tenant)
upsert /Tenant/11/Table/5{5-6} database system (tenant)
upsert /Tenant/11/Table/5{6-7} database system (tenant)
upsert /Tenant/11/Table/5{7-8} database system (tenant)

# Initialize a new tenant, tenant=10, that DOES have a pre-existing tenant,
# tenant=11, next to it.
initialize tenant=10
----

# A record IS NOT written for a key that logically belongs to the next tenant,
# tenant=11, because tenant=11 DOES exist.
state offset=57 limit=3
----
...
/Tenant/10{-\x00} database system (tenant)
/Tenant/11{-/Table/4} database system (tenant)
/Tenant/11/Table/{4-5} database system (tenant)
...
1 change: 1 addition & 0 deletions pkg/spanconfig/spanconfigreconciler/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ go_library(
"//pkg/sql/isql",
"//pkg/sql/sqlliveness",
"//pkg/util/hlc",
"//pkg/util/iterutil",
"//pkg/util/log",
"//pkg/util/retry",
"//pkg/util/syncutil",
Expand Down
24 changes: 24 additions & 0 deletions pkg/spanconfig/spanconfigreconciler/reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/isql"
"github.com/cockroachdb/cockroach/pkg/sql/sqlliveness"
"github.com/cockroachdb/cockroach/pkg/util/hlc"
"github.com/cockroachdb/cockroach/pkg/util/iterutil"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/retry"
"github.com/cockroachdb/cockroach/pkg/util/syncutil"
Expand Down Expand Up @@ -298,6 +299,29 @@ func (f *fullReconciler) reconcile(
storeWithLatestSpanConfigs.Apply(ctx, false /* dryrun */, del)
}

if !f.codec.ForSystemTenant() {
tenantPrefixKey := f.codec.TenantPrefix()
// We want to ensure tenant ranges do not straddle tenant boundaries. As
// such, a full reconciliation should always include a SpanConfig where the
// start key is keys.MakeTenantPrefix(tenantID). This ensures there is a
// split point right at the start of the tenant's keyspace, so that the
// last range of the previous tenant doesn't straddle across into this
// tenant. Also, sql.CreateTenantRecord relies on such a SpanConfigs
// existence to ensure the same thing for newly created tenants.
if err := storeWithLatestSpanConfigs.Iterate(func(record spanconfig.Record) error {
spanConfigStartKey := record.GetTarget().GetSpan().Key
if tenantPrefixKey.Compare(spanConfigStartKey) != 0 {
return errors.AssertionFailedf(
"tenant prefix key %q does not match first span config start key %q",
tenantPrefixKey, spanConfigStartKey,
)
}
return iterutil.StopIteration()
}); err != nil {
return nil, hlc.Timestamp{}, err
}
}

return storeWithLatestSpanConfigs, readTimestamp, nil
}

Expand Down
56 changes: 47 additions & 9 deletions pkg/sql/tenant_creation.go
Original file line number Diff line number Diff line change
Expand Up @@ -396,27 +396,65 @@ func CreateTenantRecord(

// This adds a split at the start of the tenant keyspace.
tenantPrefix := keys.MakeTenantPrefix(tenantID)
startRecord, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(roachpb.Span{
startRecordTarget := spanconfig.MakeTargetFromSpan(roachpb.Span{
Key: tenantPrefix,
EndKey: tenantPrefix.Next(),
}), tenantSpanConfig)
})
startRecord, err := spanconfig.MakeRecord(startRecordTarget, tenantSpanConfig)
if err != nil {
return roachpb.TenantID{}, err
}

// 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.
toUpsert := []spanconfig.Record{startRecord}

// We want to ensure we have a split at the non-inclusive end of the tenant's
// keyspace, which also happens to be the inclusive start of the next
// tenant's (with ID=ours+1). If we didn't do anything here, and we were the
// tenant with the highest ID thus far, our last range would extend to /Max.
// If a tenant with a higher ID was created, when installing its initial span
// config record, it would carve itself off (and our last range would only
// extend to that next tenant's boundary), but this is a bit awkward for code
// that wants to rely on the invariant that ranges for a tenant only extend
// to the tenant's well-defined end key.
//
// So how do we ensure this split at this tenant's non-inclusive end key?
// Hard splits are controlled by the start keys on span config records[^1],
// so we'll try insert one accordingly. But we cannot do this blindly. We
// cannot assume that tenants are created in ID order, so it's possible that
// the tenant with the next ID is already present + running. If so, it may
// already have span config records that start at the key at which we want
// to write a span config record for. Over-writing it blindly would be a
// mistake -- there's no telling what config that next tenant has associated
// for that span. So we'll do something simple -- we'll check transactionally
// whether there's anything written already, and if so, do nothing. We
// already have the split we need.
//
// [^1]: See ComputeSplitKey in spanconfig.StoreReader.
tenantPrefixEnd := tenantPrefix.PrefixEnd()
endRecord, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(roachpb.Span{
endRecordTarget := spanconfig.MakeTargetFromSpan(roachpb.Span{
Key: tenantPrefixEnd,
EndKey: tenantPrefixEnd.Next(),
}), tenantSpanConfig)
})

// Check if a record exists for the next tenant's startKey from when the next
// tenant was created. The current tenant's endRecordTarget is the same as
// the next tenant's startRecordTarget.
records, err := spanConfigs.GetSpanConfigRecords(ctx, []spanconfig.Target{endRecordTarget})
if err != nil {
return roachpb.TenantID{}, err
}

toUpsert := []spanconfig.Record{startRecord, endRecord}
// If the next tenant's startKey record exists then do not split at the
// current tenant's endKey. Doing will incorrectly overwrite the next
// tenant's first span config.
// See: https://github.com/cockroachdb/cockroach/issues/95882
if len(records) == 0 {
endRecord, err := spanconfig.MakeRecord(endRecordTarget, tenantSpanConfig)
if err != nil {
return roachpb.TenantID{}, err
}
toUpsert = append(toUpsert, endRecord)
}

return tenantID, spanConfigs.UpdateSpanConfigRecords(
ctx, nil, toUpsert, hlc.MinTimestamp, hlc.MaxTimestamp,
)
Expand Down

0 comments on commit 9d4e27a

Please sign in to comment.