diff --git a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/BUILD.bazel b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/BUILD.bazel index 59a04956b03c..9923ad6f5f49 100644 --- a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/BUILD.bazel +++ b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/BUILD.bazel @@ -13,7 +13,6 @@ go_test( "//pkg/ccl/partitionccl", "//pkg/ccl/utilccl", "//pkg/jobs", - "//pkg/keys", "//pkg/roachpb", "//pkg/security", "//pkg/security/securitytest", diff --git a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go index 48f211dc7519..b728bc31b5cc 100644 --- a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go +++ b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go @@ -19,7 +19,6 @@ import ( _ "github.com/cockroachdb/cockroach/pkg/ccl/kvccl/kvtenantccl" _ "github.com/cockroachdb/cockroach/pkg/ccl/partitionccl" "github.com/cockroachdb/cockroach/pkg/jobs" - "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/spanconfig" "github.com/cockroachdb/cockroach/pkg/spanconfig/spanconfigtestutils" @@ -192,7 +191,7 @@ func TestDataDriven(t *testing.T) { return nil }) records, err := kvAccessor.GetSpanConfigRecords( - ctx, []spanconfig.Target{spanconfig.MakeTargetFromSpan(keys.EverythingSpan)}, + ctx, spanconfig.TestingEntireSpanConfigurationStateTargets(), ) require.NoError(t, err) sort.Slice(records, func(i, j int) bool { diff --git a/pkg/migration/migrations/migrate_span_configs_test.go b/pkg/migration/migrations/migrate_span_configs_test.go index a62a5c8a1a71..956052920fa7 100644 --- a/pkg/migration/migrations/migrate_span_configs_test.go +++ b/pkg/migration/migrations/migrate_span_configs_test.go @@ -17,7 +17,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/clusterversion" - "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv/kvclient/rangefeed/rangefeedcache" "github.com/cockroachdb/cockroach/pkg/server" "github.com/cockroachdb/cockroach/pkg/spanconfig" @@ -67,10 +66,11 @@ func TestEnsureSpanConfigReconciliation(t *testing.T) { tdb.Exec(t, `SET CLUSTER SETTING spanconfig.reconciliation_job.enabled = true`) tdb.Exec(t, `SET CLUSTER SETTING spanconfig.reconciliation_job.checkpoint_interval = '100ms'`) - { // Ensure that no span config entries are found. - records, err := scKVAccessor.GetSpanConfigRecords(ctx, []spanconfig.Target{ - spanconfig.MakeTargetFromSpan(keys.EverythingSpan), - }) + { // Ensure that no span config records are found. + records, err := scKVAccessor.GetSpanConfigRecords( + ctx, + spanconfig.TestingEntireSpanConfigurationStateTargets(), + ) require.NoError(t, err) require.Empty(t, records) } @@ -90,9 +90,10 @@ func TestEnsureSpanConfigReconciliation(t *testing.T) { require.False(t, scReconciler.Checkpoint().IsEmpty()) { // Ensure that the host tenant's span configs are installed. - records, err := scKVAccessor.GetSpanConfigRecords(ctx, []spanconfig.Target{ - spanconfig.MakeTargetFromSpan(keys.EverythingSpan), - }) + records, err := scKVAccessor.GetSpanConfigRecords( + ctx, + spanconfig.TestingEntireSpanConfigurationStateTargets(), + ) require.NoError(t, err) require.NotEmpty(t, records) } @@ -152,10 +153,11 @@ func TestEnsureSpanConfigReconciliationMultiNode(t *testing.T) { tdb.Exec(t, `SET CLUSTER SETTING spanconfig.reconciliation_job.enabled = true`) tdb.Exec(t, `SET CLUSTER SETTING spanconfig.reconciliation_job.checkpoint_interval = '100ms'`) - { // Ensure that no span config entries are to be found. - records, err := scKVAccessor.GetSpanConfigRecords(ctx, []spanconfig.Target{ - spanconfig.MakeTargetFromSpan(keys.EverythingSpan), - }) + { // Ensure that no span config records are to be found. + records, err := scKVAccessor.GetSpanConfigRecords( + ctx, + spanconfig.TestingEntireSpanConfigurationStateTargets(), + ) require.NoError(t, err) require.Empty(t, records) } @@ -175,9 +177,10 @@ func TestEnsureSpanConfigReconciliationMultiNode(t *testing.T) { require.False(t, scReconciler.Checkpoint().IsEmpty()) { // Ensure that the host tenant's span configs are installed. - records, err := scKVAccessor.GetSpanConfigRecords(ctx, []spanconfig.Target{ - spanconfig.MakeTargetFromSpan(keys.EverythingSpan), - }) + records, err := scKVAccessor.GetSpanConfigRecords( + ctx, + spanconfig.TestingEntireSpanConfigurationStateTargets(), + ) require.NoError(t, err) require.NotEmpty(t, records) } @@ -219,9 +222,10 @@ func TestEnsureSpanConfigSubscription(t *testing.T) { tdb.Exec(t, `SET CLUSTER SETTING spanconfig.reconciliation_job.enabled = true`) testutils.SucceedsSoon(t, func() error { - records, err := scKVAccessor.GetSpanConfigRecords(ctx, []spanconfig.Target{ - spanconfig.MakeTargetFromSpan(keys.EverythingSpan), - }) + records, err := scKVAccessor.GetSpanConfigRecords( + ctx, + spanconfig.TestingEntireSpanConfigurationStateTargets(), + ) require.NoError(t, err) if len(records) == 0 { return fmt.Errorf("empty global span configuration state") diff --git a/pkg/migration/migrations/seed_tenant_span_configs.go b/pkg/migration/migrations/seed_tenant_span_configs.go index c0b903c7cd9e..3010ac30bacf 100644 --- a/pkg/migration/migrations/seed_tenant_span_configs.go +++ b/pkg/migration/migrations/seed_tenant_span_configs.go @@ -78,7 +78,7 @@ func seedTenantSpanConfigsMigration( return err } if len(scRecords) != 0 { - // This tenant already has span config entries. It was either + // This tenant already has span config records. It was either // already migrated (migrations need to be idempotent) or it was // created after PreSeedTenantSpanConfigs was activated. There's // nothing left to do here. diff --git a/pkg/roachpb/span_config.proto b/pkg/roachpb/span_config.proto index b326da31cc2f..84570af44d3a 100644 --- a/pkg/roachpb/span_config.proto +++ b/pkg/roachpb/span_config.proto @@ -183,16 +183,38 @@ message SystemSpanConfigTarget { TenantID source_tenant_id = 1 [(gogoproto.nullable) = false, (gogoproto.customname) = "SourceTenantID"]; // TargetTenantID is the ID of the tenant that the associated system span - // configuration applies to. - // - // If the host tenant is the source and the target is unspecified then the - // associated system span configuration applies over all ranges in the system - // (including those belonging to secondary tenants). + // configuration applies to. This field can only be set in conjunction with + // the type targeting a SpecificTenant; it must be left unset for all other + // system target types. // // Secondary tenants are only allowed to target themselves and must fill in // this field. The host tenant may use this field to target a specific // secondary tenant. TenantID target_tenant_id = 2 [(gogoproto.customname) = "TargetTenantID"]; + + + // SystemTargetType indicates the type of the SystemTarget. + enum SystemTargetType { + // Unset indicates that the type hasn't been set; should never be the case + // for well formed SystemTargets. + Unset = 0; + // SpecificTenant indicates that the system target is targeting a specific + // tenant. + SpecificTenant = 1; + // EntireCluster indicates that the system target is targeting the entire + // cluster, i.e, all ranges in the system. Only the system tenant is allowed + // to target the entire cluster. + EntireCluster = 2; + // EverythingTargetingTenants indicates that the system target is + // specifying all system span configurations installed by the source tenant + // that target specific tenants. This field is only meaningful in the + // context of fetching system span configurations, not installing them; it + // should not be used as a target in UpdateSystemSpanConfigsRequest. + EverythingTargetingTenants = 3; + } + + // SystemTargetType represents the type of the system target. + SystemTargetType system_target_type = 3; } // SpanConfigTarget specifies the target of an associated span configuration. diff --git a/pkg/rpc/auth_tenant.go b/pkg/rpc/auth_tenant.go index 819f20841a3e..ca88bff123a4 100644 --- a/pkg/rpc/auth_tenant.go +++ b/pkg/rpc/auth_tenant.go @@ -315,11 +315,12 @@ func validateSpanConfigTarget( return nil } - if target.TargetTenantID == nil { - return authErrorf("secondary tenants must explicitly target themselves") + if target.SystemTargetType == roachpb.SystemSpanConfigTarget_EntireCluster { + return authErrorf("secondary tenants cannot target the entire cluster") } - if target.SourceTenantID != *target.TargetTenantID { + if target.SystemTargetType == roachpb.SystemSpanConfigTarget_SpecificTenant && + target.SourceTenantID != *target.TargetTenantID { return authErrorf( "secondary tenants cannot interact with system span configurations of other tenants", ) diff --git a/pkg/rpc/auth_test.go b/pkg/rpc/auth_test.go index 613e741d41b2..9e0d60a404a7 100644 --- a/pkg/rpc/auth_test.go +++ b/pkg/rpc/auth_test.go @@ -191,8 +191,9 @@ func TestTenantAuthRequest(t *testing.T) { return roachpb.SpanConfigTarget{ Union: &roachpb.SpanConfigTarget_SystemSpanConfigTarget{ SystemSpanConfigTarget: &roachpb.SystemSpanConfigTarget{ - SourceTenantID: roachpb.MakeTenantID(source), - TargetTenantID: &targetID, + SourceTenantID: roachpb.MakeTenantID(source), + TargetTenantID: &targetID, + SystemTargetType: roachpb.SystemSpanConfigTarget_SpecificTenant, }, }, } @@ -535,12 +536,25 @@ func TestTenantAuthRequest(t *testing.T) { req: makeGetSpanConfigsReq(roachpb.SpanConfigTarget{ Union: &roachpb.SpanConfigTarget_SystemSpanConfigTarget{ SystemSpanConfigTarget: &roachpb.SystemSpanConfigTarget{ - SourceTenantID: roachpb.MakeTenantID(10), - TargetTenantID: nil, + SourceTenantID: roachpb.MakeTenantID(10), + TargetTenantID: nil, + SystemTargetType: roachpb.SystemSpanConfigTarget_EntireCluster, }, }, }), - expErr: `secondary tenants must explicitly target themselves`, + expErr: `secondary tenants cannot target the entire cluster`, + }, + { + req: makeGetSpanConfigsReq(roachpb.SpanConfigTarget{ + Union: &roachpb.SpanConfigTarget_SystemSpanConfigTarget{ + SystemSpanConfigTarget: &roachpb.SystemSpanConfigTarget{ + SourceTenantID: roachpb.MakeTenantID(20), + TargetTenantID: nil, + SystemTargetType: roachpb.SystemSpanConfigTarget_EverythingTargetingTenants, + }, + }, + }), + expErr: `malformed source tenant field`, }, }, "/cockroach.roachpb.Internal/UpdateSpanConfigs": { @@ -650,12 +664,13 @@ func TestTenantAuthRequest(t *testing.T) { req: makeUpdateSpanConfigsReq(roachpb.SpanConfigTarget{ Union: &roachpb.SpanConfigTarget_SystemSpanConfigTarget{ SystemSpanConfigTarget: &roachpb.SystemSpanConfigTarget{ - SourceTenantID: roachpb.MakeTenantID(10), - TargetTenantID: nil, + SourceTenantID: roachpb.MakeTenantID(10), + TargetTenantID: nil, + SystemTargetType: roachpb.SystemSpanConfigTarget_EntireCluster, }, }, }, false), - expErr: `secondary tenants must explicitly target themselves`, + expErr: `secondary tenants cannot target the entire cluster`, }, { req: makeUpdateSpanConfigsReq(makeSystemSpanConfigTarget(10, 10), true), @@ -675,12 +690,13 @@ func TestTenantAuthRequest(t *testing.T) { req: makeUpdateSpanConfigsReq(roachpb.SpanConfigTarget{ Union: &roachpb.SpanConfigTarget_SystemSpanConfigTarget{ SystemSpanConfigTarget: &roachpb.SystemSpanConfigTarget{ - SourceTenantID: roachpb.MakeTenantID(10), - TargetTenantID: nil, + SourceTenantID: roachpb.MakeTenantID(10), + TargetTenantID: nil, + SystemTargetType: roachpb.SystemSpanConfigTarget_EntireCluster, }, }, }, true), - expErr: `secondary tenants must explicitly target themselves`, + expErr: `secondary tenants cannot target the entire cluster`, }, }, diff --git a/pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go b/pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go index 581499c9f823..a1f725bc8bb2 100644 --- a/pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go +++ b/pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go @@ -309,7 +309,7 @@ func (k *KVAccessor) constructDeleteStmtAndArgs( } // constructUpsertStmtAndArgs constructs the statement and query arguments -// needed to upsert the given span config entries. +// needed to upsert the given span config records. func (k *KVAccessor) constructUpsertStmtAndArgs( toUpsert []spanconfig.Record, ) (string, []interface{}, error) { @@ -429,6 +429,12 @@ func validateUpdateArgs(toDelete []spanconfig.Target, toUpsert []spanconfig.Reco copy(targets, list) sort.Sort(spanconfig.Targets(targets)) for i := range targets { + if targets[i].IsSystemTarget() && targets[i].GetSystemTarget().IsReadOnly() { + return errors.AssertionFailedf( + "cannot use read only system target %s as an update argument", targets[i], + ) + } + if i == 0 { continue } diff --git a/pkg/spanconfig/spanconfigkvaccessor/kvaccessor_test.go b/pkg/spanconfig/spanconfigkvaccessor/kvaccessor_test.go index 55d8dc610ad4..6d6dfe3f77b0 100644 --- a/pkg/spanconfig/spanconfigkvaccessor/kvaccessor_test.go +++ b/pkg/spanconfig/spanconfigkvaccessor/kvaccessor_test.go @@ -34,9 +34,11 @@ import ( // span [a,e) // span [a,b) // span [b,c) +// system-target {cluster} // system-target {source=1,target=20} // system-target {source=1,target=1} // system-target {source=20,target=20} +// system-target {source=1,everything-installed-on-tenants} // ---- // // kvaccessor-update @@ -45,13 +47,14 @@ import ( // upsert [d,e):D // delete {source=1,target=1} // upsert {source=1,target=1}:A +// upsert {cluster}:F // ---- // // They tie into GetSpanConfigRecords and UpdateSpanConfigRecords // respectively. For kvaccessor-get, each listed target is added to the set of // targets being read. For kvaccessor-update, the lines prefixed with "delete" // count towards the targets being deleted, and for "upsert" they correspond to -// the span config entries being upserted. See +// the span config records being upserted. See // spanconfigtestutils.Parse{Span,Config,SpanConfigRecord} for more details. func TestDataDriven(t *testing.T) { defer leaktest.AfterTest(t)() diff --git a/pkg/spanconfig/spanconfigkvaccessor/testdata/system_span_configs b/pkg/spanconfig/spanconfigkvaccessor/testdata/system_span_configs index 42a6e592e75f..9834d420a92b 100644 --- a/pkg/spanconfig/spanconfigkvaccessor/testdata/system_span_configs +++ b/pkg/spanconfig/spanconfigkvaccessor/testdata/system_span_configs @@ -54,6 +54,13 @@ system-target {source=20,target=20} {source=1,target=10}:H {source=20,target=20}:I +kvaccessor-get +system-target {source=1,everything-installed-on-tenants} +---- +{source=1,target=1}:G +{source=1,target=10}:H + + # Delete all the system span configurations that we just added and ensure # they take effect. kvaccessor-update @@ -71,6 +78,10 @@ system-target {source=1,target=10} system-target {source=20,target=20} ---- +kvaccessor-get +system-target {source=1,everything-installed-on-tenants} +---- + # Lastly, try adding multiple system targets set by the host tenant that apply # to distinct secondary tenants. We also add a system span configuration set by # one of these secondary tenant's on itself for kicks. @@ -95,3 +106,15 @@ system-target {source=10,target=10} {source=1,target=20}:B {source=1,target=30}:C {source=10,target=10}:G + +kvaccessor-get +system-target {source=1,everything-installed-on-tenants} +---- +{source=1,target=10}:A +{source=1,target=20}:B +{source=1,target=30}:C + +kvaccessor-get +system-target {source=10,everything-installed-on-tenants} +---- +{source=10,target=10}:G diff --git a/pkg/spanconfig/spanconfigkvaccessor/validation_test.go b/pkg/spanconfig/spanconfigkvaccessor/validation_test.go index a8215bbdda53..cbd7aba15d80 100644 --- a/pkg/spanconfig/spanconfigkvaccessor/validation_test.go +++ b/pkg/spanconfig/spanconfigkvaccessor/validation_test.go @@ -25,10 +25,7 @@ import ( func TestValidateUpdateArgs(t *testing.T) { defer leaktest.AfterTest(t)() - clusterTarget := spanconfig.MakeTargetFromSystemTarget(spanconfig.SystemTarget{ - SourceTenantID: roachpb.SystemTenantID, - TargetTenantID: nil, - }) + clusterTarget := spanconfig.MakeTargetFromSystemTarget(spanconfig.MakeClusterTarget()) makeTenantTarget := func(id uint64) spanconfig.Target { target, err := spanconfig.MakeTenantTarget(roachpb.MakeTenantID(id), roachpb.MakeTenantID(id)) @@ -183,7 +180,38 @@ func TestValidateUpdateArgs(t *testing.T) { }, expErr: "", }, + { + // Read only targets are not valid delete args. + toDelete: []spanconfig.Target{ + spanconfig.MakeTargetFromSystemTarget( + spanconfig.MakeEverythingTargetingTenantsTarget(roachpb.SystemTenantID), + ), + }, + expErr: "cannot use read only system target .* as an update argument", + }, + { + // Read only targets are not valid upsert args. + toUpsert: []spanconfig.Record{ + { + Target: spanconfig.MakeTargetFromSystemTarget( + spanconfig.MakeEverythingTargetingTenantsTarget(roachpb.SystemTenantID), + ), + }, + }, + expErr: "cannot use read only system target .* as an update argument", + }, + { + // Read only target validation also applies when the source is a secondary + // tenant. + toDelete: []spanconfig.Target{ + spanconfig.MakeTargetFromSystemTarget( + spanconfig.MakeEverythingTargetingTenantsTarget(roachpb.MakeTenantID(10)), + ), + }, + expErr: "cannot use read only system target .* as an update argument", + }, } { - require.True(t, testutils.IsError(validateUpdateArgs(tc.toDelete, tc.toUpsert), tc.expErr)) + err := validateUpdateArgs(tc.toDelete, tc.toUpsert) + require.True(t, testutils.IsError(err, tc.expErr), "exp %s; got %s", tc.expErr, err) } } diff --git a/pkg/spanconfig/spanconfigtestutils/utils.go b/pkg/spanconfig/spanconfigtestutils/utils.go index 5ae42e7a398e..d56ddf0fe073 100644 --- a/pkg/spanconfig/spanconfigtestutils/utils.go +++ b/pkg/spanconfig/spanconfigtestutils/utils.go @@ -33,8 +33,10 @@ import ( var spanRe = regexp.MustCompile(`^\[(\w+),\s??(\w+)\)$`) // systemTargetRe matches strings of the form -// "{source=(|system),target=(|system)}". -var systemTargetRe = regexp.MustCompile(`^{(cluster)|(source=(\d*),\s??target=(\d*))}$`) +// "{cluster|source=,(target=|everything-installed-on-tenants)}". +var systemTargetRe = regexp.MustCompile( + `^{(cluster)|(source=(\d*),\s??((target=(\d*))|everything-installed-on-tenants))}$`, +) // configRe matches a single word. It's a shorthand for declaring a unique // config. @@ -69,7 +71,10 @@ func parseSystemTarget(t *testing.T, systemTarget string) spanconfig.SystemTarge sourceID, err := strconv.Atoi(matches[3]) require.NoError(t, err) - targetID, err := strconv.Atoi(matches[4]) + if matches[4] == "everything-installed-on-tenants" { + return spanconfig.MakeEverythingTargetingTenantsTarget(roachpb.MakeTenantID(uint64(sourceID))) + } + targetID, err := strconv.Atoi(matches[6]) require.NoError(t, err) target, err := spanconfig.MakeTenantTarget( roachpb.MakeTenantID(uint64(sourceID)), roachpb.MakeTenantID(uint64(targetID)), diff --git a/pkg/spanconfig/systemtarget.go b/pkg/spanconfig/systemtarget.go index be77cfcd808e..0ecf7efcdf89 100644 --- a/pkg/spanconfig/systemtarget.go +++ b/pkg/spanconfig/systemtarget.go @@ -22,33 +22,51 @@ import ( // SystemTarget specifies the target of a system span configuration. type SystemTarget struct { - // SourceTenantID is the ID of the tenant that specified the system span + // sourceTenantID is the ID of the tenant that specified the system span // configuration. - // SourceTenantID is the ID of the tenant that specified the system span - // configuration. - SourceTenantID roachpb.TenantID + sourceTenantID roachpb.TenantID - // TargetTenantID is the ID of the tenant that the associated system span - // configuration applies to. - // - // If the host tenant is the source and the TargetTenantID is unspecified then - // the associated system span configuration applies over all ranges in the - // system (including those belonging to secondary tenants). + // targetTenantID is the ID of the tenant that the associated system span + // configuration applies. This field can only be set in conjunction with the + // type being SystemTargetTypeSpecificTenant; it must be left unset for all + // other system target types. // // Secondary tenants are only allowed to target themselves. The host tenant - // may use this field to target a specific secondary tenant. We validate this - // when constructing new system targets. - TargetTenantID *roachpb.TenantID + // may use this field to target a specific secondary tenant. + targetTenantID *roachpb.TenantID + + // systemTargetType indicates the type of the system target. targetTenantID + // can only be set if the system target is specific. + systemTargetType systemTargetType } +// systemTargetType indicates the type of SystemTarget. +type systemTargetType int + +const ( + _ systemTargetType = iota + // SystemTargetTypeSpecificTenant indicates that the system target is + // targeting a specific tenant. + SystemTargetTypeSpecificTenant + // SystemTargetTypeEntireCluster indicates that the system target is targeting + // the entire cluster, i.e, all ranges in the system. Only the host tenant is + // allowed to do so. + SystemTargetTypeEntireCluster + // SystemTargetTypeEverythingTargetingTenants represents all system span + // configurations installed by a source tenant that target specific tenants. + // This is a read-only system target. + SystemTargetTypeEverythingTargetingTenants +) + // MakeTenantTarget constructs, validates, and returns a new SystemTarget that // targets the physical keyspace of the targetTenantID. func MakeTenantTarget( sourceTenantID roachpb.TenantID, targetTenantID roachpb.TenantID, ) (SystemTarget, error) { t := SystemTarget{ - SourceTenantID: sourceTenantID, - TargetTenantID: &targetTenantID, + sourceTenantID: sourceTenantID, + targetTenantID: &targetTenantID, + systemTargetType: SystemTargetTypeSpecificTenant, } return t, t.validate() } @@ -56,125 +74,186 @@ func MakeTenantTarget( // MakeSystemTargetFromProto constructs a SystemTarget from a // roachpb.SystemSpanConfigTarget and validates it. func MakeSystemTargetFromProto(proto *roachpb.SystemSpanConfigTarget) (SystemTarget, error) { - if proto.SourceTenantID == roachpb.SystemTenantID && proto.TargetTenantID == nil { - return MakeClusterTarget(), nil - } else if proto.TargetTenantID == nil { - return SystemTarget{}, - errors.Newf("secondary tenant %s cannot target the entire cluster", proto.SourceTenantID) + var t SystemTarget + switch proto.SystemTargetType { + case roachpb.SystemSpanConfigTarget_SpecificTenant: + t = SystemTarget{ + sourceTenantID: proto.SourceTenantID, + targetTenantID: proto.TargetTenantID, + systemTargetType: SystemTargetTypeSpecificTenant, + } + case roachpb.SystemSpanConfigTarget_EntireCluster: + t = SystemTarget{ + sourceTenantID: proto.SourceTenantID, + targetTenantID: proto.TargetTenantID, + systemTargetType: SystemTargetTypeEntireCluster, + } + case roachpb.SystemSpanConfigTarget_EverythingTargetingTenants: + t = SystemTarget{ + sourceTenantID: proto.SourceTenantID, + targetTenantID: proto.TargetTenantID, + systemTargetType: SystemTargetTypeEverythingTargetingTenants, + } + case roachpb.SystemSpanConfigTarget_Unset: + return SystemTarget{}, errors.AssertionFailedf("system target type unset on proto") } - - return MakeTenantTarget(proto.SourceTenantID, *proto.TargetTenantID) + return t, t.validate() } // MakeClusterTarget returns a new system target that targets the entire cluster. // Only the host tenant is allowed to target the entire cluster. func MakeClusterTarget() SystemTarget { return SystemTarget{ - SourceTenantID: roachpb.SystemTenantID, + sourceTenantID: roachpb.SystemTenantID, + systemTargetType: SystemTargetTypeEntireCluster, + } +} + +// MakeEverythingTargetingTenantsTarget returns a new SystemTarget that +// represents all system span configurations installed by the given tenant ID +// on specific tenants (including both itself and other tenants). +func MakeEverythingTargetingTenantsTarget(sourceID roachpb.TenantID) SystemTarget { + return SystemTarget{ + sourceTenantID: sourceID, + systemTargetType: SystemTargetTypeEverythingTargetingTenants, } } // targetsEntireCluster returns true if the target applies to all ranges in the // system (including those belonging to secondary tenants). func (st SystemTarget) targetsEntireCluster() bool { - return st.SourceTenantID == roachpb.SystemTenantID && st.TargetTenantID == nil + return st.systemTargetType == SystemTargetTypeEntireCluster +} + +// IsReadOnly returns true if the system target is read-only. Read only targets +// should not be persisted. +func (st SystemTarget) IsReadOnly() bool { + return st.systemTargetType == SystemTargetTypeEverythingTargetingTenants } // encode returns an encoded span associated with the receiver which is suitable -// for persistence in system.span_configurations. +// for interaction with system.span_configurations table. func (st SystemTarget) encode() roachpb.Span { var k roachpb.Key - if st.SourceTenantID == roachpb.SystemTenantID && - st.TargetTenantID == nil { + switch st.systemTargetType { + case SystemTargetTypeEntireCluster: k = keys.SystemSpanConfigEntireKeyspace - } else if st.SourceTenantID == roachpb.SystemTenantID { - k = encoding.EncodeUvarintAscending( - keys.SystemSpanConfigHostOnTenantKeyspace, st.TargetTenantID.ToUint64(), - ) - } else { - k = encoding.EncodeUvarintAscending( - keys.SystemSpanConfigSecondaryTenantOnEntireKeyspace, st.SourceTenantID.ToUint64(), - ) + case SystemTargetTypeSpecificTenant: + if st.sourceTenantID == roachpb.SystemTenantID { + k = encoding.EncodeUvarintAscending( + keys.SystemSpanConfigHostOnTenantKeyspace, st.targetTenantID.ToUint64(), + ) + } else { + k = encoding.EncodeUvarintAscending( + keys.SystemSpanConfigSecondaryTenantOnEntireKeyspace, st.sourceTenantID.ToUint64(), + ) + } + case SystemTargetTypeEverythingTargetingTenants: + if st.sourceTenantID == roachpb.SystemTenantID { + k = keys.SystemSpanConfigHostOnTenantKeyspace + } else { + k = encoding.EncodeUvarintAscending( + keys.SystemSpanConfigSecondaryTenantOnEntireKeyspace, st.sourceTenantID.ToUint64(), + ) + } } return roachpb.Span{Key: k, EndKey: k.PrefixEnd()} } // validate ensures that the receiver is well-formed. func (st SystemTarget) validate() error { - if st.SourceTenantID == roachpb.SystemTenantID { - // The system tenant can target itself, other secondary tenants, and the - // entire cluster (including secondary tenant ranges). - return nil - } - if st.TargetTenantID == nil { - return errors.AssertionFailedf( - "secondary tenant %s cannot have unspecified target tenant ID", st.SourceTenantID, - ) - } - if st.SourceTenantID != *st.TargetTenantID { - return errors.AssertionFailedf( - "secondary tenant %s cannot target another tenant with ID %s", - st.SourceTenantID, - st.TargetTenantID, - ) + switch st.systemTargetType { + case SystemTargetTypeEverythingTargetingTenants: + if st.targetTenantID != nil { + return errors.AssertionFailedf( + "targetTenantID must be unset when targeting everything installed on tenants", + ) + } + case SystemTargetTypeEntireCluster: + if st.sourceTenantID != roachpb.SystemTenantID { + return errors.AssertionFailedf("only the host tenant is allowed to target the entire cluster") + } + if st.targetTenantID != nil { + return errors.AssertionFailedf("malformed system target for entire cluster; targetTenantID set") + } + case SystemTargetTypeSpecificTenant: + if st.targetTenantID == nil { + return errors.AssertionFailedf("malformed system target for specific tenant; targetTenantID unset") + } + if st.sourceTenantID != roachpb.SystemTenantID && st.sourceTenantID != *st.targetTenantID { + return errors.AssertionFailedf( + "secondary tenant %s cannot target another tenant with ID %s", + st.sourceTenantID, + st.targetTenantID, + ) + } + default: + return errors.AssertionFailedf("invalid system target type") } return nil } // isEmpty returns true if the receiver is empty. func (st SystemTarget) isEmpty() bool { - return st.SourceTenantID.Equal(roachpb.TenantID{}) && st.TargetTenantID.Equal(nil) + return st.sourceTenantID.Equal(roachpb.TenantID{}) && st.targetTenantID.Equal(nil) } // less returns true if the receiver is considered less than the supplied // target. The semantics are defined as follows: -// - host installed targets that target the entire cluster come first. -// - host installed targets that target a tenant come next (ordered by target -// tenant ID). -// - secondary tenant installed targets come next, ordered by secondary tenant -// ID. +// - read only targets come first, ordered by tenant ID. +// - targets that target the entire cluster come next. +// - targets that target specific tenant come last, sorted by source tenant ID; +// target tenant ID is used as a tiebreaker if the source's are the same. func (st SystemTarget) less(ot SystemTarget) bool { - if st.SourceTenantID == roachpb.SystemTenantID && - ot.SourceTenantID == roachpb.SystemTenantID { - if st.targetsEntireCluster() { - return true - } else if ot.targetsEntireCluster() { - return false - } + if st.IsReadOnly() && ot.IsReadOnly() { + return st.sourceTenantID.ToUint64() < ot.sourceTenantID.ToUint64() + } - return st.TargetTenantID.ToUint64() < ot.TargetTenantID.ToUint64() + if st.IsReadOnly() { + return true + } else if ot.IsReadOnly() { + return false } - if st.SourceTenantID == roachpb.SystemTenantID { + if st.targetsEntireCluster() { return true - } else if ot.SourceTenantID == roachpb.SystemTenantID { + } else if ot.targetsEntireCluster() { return false } - return st.SourceTenantID.ToUint64() < ot.SourceTenantID.ToUint64() + if st.sourceTenantID.ToUint64() == ot.sourceTenantID.ToUint64() { + return st.targetTenantID.ToUint64() < ot.targetTenantID.ToUint64() + } + + return st.sourceTenantID.ToUint64() < ot.sourceTenantID.ToUint64() } // equal returns true iff the receiver is equal to the supplied system target. func (st SystemTarget) equal(ot SystemTarget) bool { - return st.SourceTenantID.Equal(ot.SourceTenantID) && st.TargetTenantID.Equal(ot.TargetTenantID) + return st.sourceTenantID.Equal(ot.sourceTenantID) && st.targetTenantID.Equal(ot.targetTenantID) } // String returns a pretty printed version of a system target. func (st SystemTarget) String() string { - if st.targetsEntireCluster() { + switch st.systemTargetType { + case SystemTargetTypeEntireCluster: return "{cluster}" + case SystemTargetTypeEverythingTargetingTenants: + return fmt.Sprintf("{source=%d, everything-installed-on-tenants}", st.sourceTenantID) + case SystemTargetTypeSpecificTenant: + return fmt.Sprintf( + "{source=%d,target=%d}", + st.sourceTenantID.ToUint64(), + st.targetTenantID.ToUint64(), + ) + default: + panic("unreachable") } - return fmt.Sprintf( - "{source=%d,target=%d}", - st.SourceTenantID.ToUint64(), - st.TargetTenantID.ToUint64(), - ) } // decodeSystemTarget converts the given span into a SystemTarget. An error is -// returned if the supplied span does not conform to a system target's -// encoding. +// returned if the supplied span does not conform to a system target's encoding. func decodeSystemTarget(span roachpb.Span) (SystemTarget, error) { // Validate the end key is well-formed. if !span.EndKey.Equal(span.Key.PrefixEnd()) { diff --git a/pkg/spanconfig/target.go b/pkg/spanconfig/target.go index cd69dd604f6d..43f084191f3e 100644 --- a/pkg/spanconfig/target.go +++ b/pkg/spanconfig/target.go @@ -11,6 +11,7 @@ package spanconfig import ( + "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/errors" ) @@ -26,7 +27,7 @@ type Target struct { func MakeTarget(t roachpb.SpanConfigTarget) (Target, error) { switch t.Union.(type) { case *roachpb.SpanConfigTarget_Span: - return MakeTargetFromSpan(*t.GetSpan()), nil + return MakeSpanTargetFromProto(t) case *roachpb.SpanConfigTarget_SystemSpanConfigTarget: systemTarget, err := MakeSystemTargetFromProto(t.GetSystemSpanConfigTarget()) if err != nil { @@ -38,8 +39,28 @@ func MakeTarget(t roachpb.SpanConfigTarget) (Target, error) { } } -// MakeTargetFromSpan constructs and returns a span target. +// MakeSpanTargetFromProto returns a new Target backed by an underlying span. +// An error is returned if the proto does not contain a span or if the span +// overlaps with the reserved system span config keyspace. +func MakeSpanTargetFromProto(spanTarget roachpb.SpanConfigTarget) (Target, error) { + if spanTarget.GetSpan() == nil { + return Target{}, errors.AssertionFailedf("span config target did not contain a span") + } + if keys.SystemSpanConfigSpan.Overlaps(*spanTarget.GetSpan()) { + return Target{}, errors.AssertionFailedf( + "cannot target spans in reserved system span config keyspace", + ) + } + return MakeTargetFromSpan(*spanTarget.GetSpan()), nil +} + +// MakeTargetFromSpan constructs and returns a span target. Callers are not +// allowed to target the reserved system span config keyspace (or part of it) +// directly; system targets should be used instead. func MakeTargetFromSpan(span roachpb.Span) Target { + if keys.SystemSpanConfigSpan.Overlaps(span) { + panic("cannot target spans in reserved system span config keyspace") + } return Target{span: span} } @@ -76,8 +97,8 @@ func (t Target) GetSystemTarget() SystemTarget { return t.systemTarget } -// Encode returns an encoded span suitable for persistence in -// system.span_configurations. +// Encode returns an encoded span suitable for interaction with the +// system.span_configurations table. func (t Target) Encode() roachpb.Span { switch { case t.IsSpanTarget(): @@ -153,11 +174,20 @@ func (t Target) ToProto() roachpb.SpanConfigTarget { }, } case t.IsSystemTarget(): + systemTargetType := roachpb.SystemSpanConfigTarget_Unset + if t.GetSystemTarget().systemTargetType == SystemTargetTypeEverythingTargetingTenants { + systemTargetType = roachpb.SystemSpanConfigTarget_EverythingTargetingTenants + } else if t.GetSystemTarget().systemTargetType == SystemTargetTypeSpecificTenant { + systemTargetType = roachpb.SystemSpanConfigTarget_SpecificTenant + } else if t.GetSystemTarget().systemTargetType == SystemTargetTypeEntireCluster { + systemTargetType = roachpb.SystemSpanConfigTarget_EntireCluster + } return roachpb.SpanConfigTarget{ Union: &roachpb.SpanConfigTarget_SystemSpanConfigTarget{ SystemSpanConfigTarget: &roachpb.SystemSpanConfigTarget{ - SourceTenantID: t.GetSystemTarget().SourceTenantID, - TargetTenantID: t.GetSystemTarget().TargetTenantID, + SourceTenantID: t.GetSystemTarget().sourceTenantID, + TargetTenantID: t.GetSystemTarget().targetTenantID, + SystemTargetType: systemTargetType, }, }, } @@ -248,3 +278,15 @@ func TargetsFromProtos(protoTargets []roachpb.SpanConfigTarget) ([]Target, error } return targets, nil } + +// TestingEntireSpanConfigurationStateTargets returns a list of targets which +// can be used to read the entire span configuration state. This includes all +// span configurations installed by all tenants and all system span +// configurations, including those installed by secondary tenants. +func TestingEntireSpanConfigurationStateTargets() []Target { + return Targets{ + Target{ + span: keys.EverythingSpan, + }, + } +} diff --git a/pkg/spanconfig/target_test.go b/pkg/spanconfig/target_test.go index c31e8c25ff00..79e2f9209b19 100644 --- a/pkg/spanconfig/target_test.go +++ b/pkg/spanconfig/target_test.go @@ -87,45 +87,127 @@ func TestDecodeInvalidSpanAsSystemTarget(t *testing.T) { // TestSystemTargetValidation ensures target.validate() works as expected. func TestSystemTargetValidation(t *testing.T) { + tenant10 := roachpb.MakeTenantID(10) + tenant20 := roachpb.MakeTenantID(20) for _, tc := range []struct { sourceTenantID roachpb.TenantID - targetTenantID roachpb.TenantID + targetTenantID *roachpb.TenantID + targetType systemTargetType expErr string }{ { // Secondary tenants cannot target the system tenant. - sourceTenantID: roachpb.MakeTenantID(10), - targetTenantID: roachpb.SystemTenantID, - expErr: "secondary tenant 10 cannot target another tenant with ID", + sourceTenantID: tenant10, + targetTenantID: &roachpb.SystemTenantID, + targetType: SystemTargetTypeSpecificTenant, + expErr: "secondary tenant 10 cannot target another tenant with ID system", }, { // Secondary tenants cannot target other secondary tenants. - sourceTenantID: roachpb.MakeTenantID(10), - targetTenantID: roachpb.MakeTenantID(20), - expErr: "secondary tenant 10 cannot target another tenant with ID", + sourceTenantID: tenant10, + targetTenantID: &tenant20, + targetType: SystemTargetTypeSpecificTenant, + expErr: "secondary tenant 10 cannot target another tenant with ID 20", + }, + { + // Secondary tenants cannot target the entire cluster. + sourceTenantID: tenant10, + targetTenantID: nil, + targetType: SystemTargetTypeEntireCluster, + expErr: "only the host tenant is allowed to target the entire cluster", + }, + { + // Ensure secondary tenants can't target the entire cluster even if they + // set targetTenantID to themselves. + sourceTenantID: tenant10, + targetTenantID: &tenant10, + targetType: SystemTargetTypeEntireCluster, + expErr: "only the host tenant is allowed to target the entire cluster", + }, + { + // System tenant can't set both targetTenantID and target everything + // installed on tenants. + sourceTenantID: roachpb.SystemTenantID, + targetTenantID: &tenant10, + targetType: SystemTargetTypeEverythingTargetingTenants, + expErr: "targetTenantID must be unset when targeting everything installed", + }, + { + // System tenant must fill in a targetTenantID when targeting a specific + // tenant. + sourceTenantID: roachpb.SystemTenantID, + targetTenantID: nil, + targetType: SystemTargetTypeSpecificTenant, + expErr: "malformed system target for specific tenant; targetTenantID unset", + }, + { + // System tenant can't set both targetTenantID and target the entire + // cluster. + sourceTenantID: roachpb.SystemTenantID, + targetTenantID: &tenant10, + targetType: SystemTargetTypeEntireCluster, + expErr: "malformed system target for entire cluster; targetTenantID set", + }, + { + // secondary tenant can't set both targetTenantID and target everything + // installed on tenants. + sourceTenantID: tenant10, + targetTenantID: &tenant10, + targetType: SystemTargetTypeEverythingTargetingTenants, + expErr: "targetTenantID must be unset when targeting everything installed", }, // Test some valid targets. { // System tenant targeting secondary tenant is allowed. sourceTenantID: roachpb.SystemTenantID, - targetTenantID: roachpb.MakeTenantID(20), + targetTenantID: &tenant20, + targetType: SystemTargetTypeSpecificTenant, + }, + { + // System tenant targeting the entire cluster is allowed. + sourceTenantID: roachpb.SystemTenantID, + targetTenantID: nil, + targetType: SystemTargetTypeEntireCluster, }, { // System tenant targeting itself is allowed. sourceTenantID: roachpb.SystemTenantID, - targetTenantID: roachpb.SystemTenantID, + targetTenantID: &roachpb.SystemTenantID, + targetType: SystemTargetTypeSpecificTenant, }, { // Secondary tenant targeting itself is allowed. - sourceTenantID: roachpb.MakeTenantID(10), - targetTenantID: roachpb.MakeTenantID(10), + sourceTenantID: tenant10, + targetTenantID: &tenant10, + targetType: SystemTargetTypeSpecificTenant, + }, + { + // Secondary tenant targeting everything installed on tenants by it is + // allowed. + sourceTenantID: tenant10, + targetTenantID: nil, + targetType: SystemTargetTypeEverythingTargetingTenants, + }, + { + // System tenant targeting everything installed on tenants by it is + // allowed. + sourceTenantID: roachpb.SystemTenantID, + targetTenantID: nil, + targetType: SystemTargetTypeEverythingTargetingTenants, }, } { target := SystemTarget{ - SourceTenantID: tc.sourceTenantID, - TargetTenantID: &tc.targetTenantID, + sourceTenantID: tc.sourceTenantID, + targetTenantID: tc.targetTenantID, + systemTargetType: tc.targetType, } - require.True(t, testutils.IsError(target.validate(), tc.expErr)) + require.True( + t, + testutils.IsError(target.validate(), tc.expErr), + "expected: %s got: %s ", + tc.expErr, + target.validate(), + ) } } @@ -133,6 +215,8 @@ func TestSystemTargetValidation(t *testing.T) { func TestTargetSortingRandomized(t *testing.T) { // Construct a set of sorted targets. sortedTargets := Targets{ + MakeTargetFromSystemTarget(MakeEverythingTargetingTenantsTarget(roachpb.SystemTenantID)), + MakeTargetFromSystemTarget(MakeEverythingTargetingTenantsTarget(roachpb.MakeTenantID(10))), MakeTargetFromSystemTarget(MakeClusterTarget()), MakeTargetFromSystemTarget(makeTenantTargetOrFatal(t, roachpb.SystemTenantID, roachpb.SystemTenantID)), MakeTargetFromSystemTarget(makeTenantTargetOrFatal(t, roachpb.SystemTenantID, roachpb.MakeTenantID(10))), @@ -158,6 +242,34 @@ func TestTargetSortingRandomized(t *testing.T) { } } +// TestSpanTargetsConstructedInSystemSpanConfigKeyspace ensures that +// constructing span targets +func TestSpanTargetsConstructedInSystemSpanConfigKeyspace(t *testing.T) { + for _, tc := range []roachpb.Span{ + MakeClusterTarget().encode(), + makeTenantTargetOrFatal(t, roachpb.MakeTenantID(10), roachpb.MakeTenantID(10)).encode(), + makeTenantTargetOrFatal(t, roachpb.SystemTenantID, roachpb.SystemTenantID).encode(), + makeTenantTargetOrFatal(t, roachpb.SystemTenantID, roachpb.MakeTenantID(10)).encode(), + { + // Extends into from the left + Key: keys.TimeseriesKeyMax, + EndKey: keys.SystemSpanConfigPrefix.Next(), // End Key isn't inclusive. + }, + { + // Entirely contained. + Key: keys.SystemSpanConfigPrefix.Next(), + EndKey: keys.SystemSpanConfigPrefix.Next().PrefixEnd(), + }, + { + // Extends beyond on the right. + Key: keys.SystemSpanConfigPrefix.Next().PrefixEnd(), + EndKey: keys.SystemSpanConfigKeyMax.Next().Next(), + }, + } { + require.Panics(t, func() { MakeTargetFromSpan(tc) }) + } +} + func makeTenantTargetOrFatal( t *testing.T, sourceID roachpb.TenantID, targetID roachpb.TenantID, ) SystemTarget {