From 5cedf8830edca034b2e9ba9395206c4c9af1d70d Mon Sep 17 00:00:00 2001 From: arulajmani Date: Mon, 7 Feb 2022 23:48:27 -0500 Subject: [PATCH 1/3] roachpb: modify SpanConfigEntry and span config RPCs to work on targets This patch modifies roachpb.SpanConfigEntry to tie a roachpb.SpanConfigTarget (instead of roachpb.Span) with a roachpb.SpanConfig. A roachpb.SpanConfigTarget is a union proto over roachpb.Span and roachpb.SystemSpanConfigTarget. Given a roachpb.SpanConfigEntry, we can use the roachpb.SpanConfigTarget to disambiguate between regular span configurations and system span configurations. We use the modified roachpb.SpanConfigEntry in all our RPCs. We also replace usages of roachpb.Spans with roachpb.SpanConfigTargets in the RPCs as well. Release note: None --- pkg/ccl/kvccl/kvtenantccl/connector.go | 18 +- pkg/roachpb/span_config.go | 10 - pkg/roachpb/span_config.proto | 94 ++++++--- pkg/rpc/BUILD.bazel | 1 + pkg/rpc/auth_tenant.go | 79 ++++++-- pkg/rpc/auth_test.go | 190 +++++++++++++++--- pkg/server/node.go | 21 +- .../spanconfigreconciler/reconciler.go | 2 +- .../spanconfigtestutils/recorder.go | 4 +- pkg/spanconfig/spanconfigtestutils/utils.go | 7 +- pkg/spanconfig/target.go | 70 ++++++- 11 files changed, 369 insertions(+), 127 deletions(-) diff --git a/pkg/ccl/kvccl/kvtenantccl/connector.go b/pkg/ccl/kvccl/kvtenantccl/connector.go index df22bb43df83..58133bf08a1d 100644 --- a/pkg/ccl/kvccl/kvtenantccl/connector.go +++ b/pkg/ccl/kvccl/kvtenantccl/connector.go @@ -461,12 +461,8 @@ func (c *Connector) GetSpanConfigRecords( ctx context.Context, targets []spanconfig.Target, ) (records []spanconfig.Record, _ error) { if err := c.withClient(ctx, func(ctx context.Context, c *client) error { - spans := make([]roachpb.Span, 0, len(targets)) - for _, target := range targets { - spans = append(spans, *target.GetSpan()) - } resp, err := c.GetSpanConfigs(ctx, &roachpb.GetSpanConfigsRequest{ - Spans: spans, + Targets: spanconfig.TargetsToTargetProtos(targets), }) if err != nil { return err @@ -485,18 +481,10 @@ func (c *Connector) GetSpanConfigRecords( func (c *Connector) UpdateSpanConfigRecords( ctx context.Context, toDelete []spanconfig.Target, toUpsert []spanconfig.Record, ) error { - spansToDelete := make([]roachpb.Span, 0, len(toDelete)) - for _, toDel := range toDelete { - spansToDelete = append(spansToDelete, roachpb.Span(toDel)) - } - - entriesToUpsert := spanconfig.RecordsToSpanConfigEntries(toUpsert) - return c.withClient(ctx, func(ctx context.Context, c *client) error { - _, err := c.UpdateSpanConfigs(ctx, &roachpb.UpdateSpanConfigsRequest{ - ToDelete: spansToDelete, - ToUpsert: entriesToUpsert, + ToDelete: spanconfig.TargetsToTargetProtos(toDelete), + ToUpsert: spanconfig.RecordsToSpanConfigEntries(toUpsert), }) return err }) diff --git a/pkg/roachpb/span_config.go b/pkg/roachpb/span_config.go index 89c9665b90be..ad3ebc198a27 100644 --- a/pkg/roachpb/span_config.go +++ b/pkg/roachpb/span_config.go @@ -96,16 +96,6 @@ func (c ConstraintsConjunction) String() string { return sb.String() } -// Equal compares two span config entries. -func (s *SpanConfigEntry) Equal(o SpanConfigEntry) bool { - return s.Span.Equal(o.Span) && s.Config.Equal(o.Config) -} - -// Empty returns true if the span config entry is empty. -func (s *SpanConfigEntry) Empty() bool { - return s.Equal(SpanConfigEntry{}) -} - // TestingDefaultSpanConfig exports the default span config for testing purposes. func TestingDefaultSpanConfig() SpanConfig { return SpanConfig{ diff --git a/pkg/roachpb/span_config.proto b/pkg/roachpb/span_config.proto index 857e87c757b0..b326da31cc2f 100644 --- a/pkg/roachpb/span_config.proto +++ b/pkg/roachpb/span_config.proto @@ -174,56 +174,98 @@ message SpanConfig { // Next ID: 12 } +// SystemSpanConfigTarget specifies the target of system span configurations. +message SystemSpanConfigTarget { + option (gogoproto.equal) = true; + + // SourceTenantID is the ID of the tenant that specified the system span + // configuration. + 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). + // + // 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"]; +} + +// SpanConfigTarget specifies the target of an associated span configuration. +message SpanConfigTarget { + oneof union { + // Span is a keyspan that a span config is said to apply over. + Span span = 1; + // SystemSpanConfigTarget specifies the target of a system span + // configuration. + SystemSpanConfigTarget system_span_config_target = 2; + } +} + // SpanConfigEntry ties a span to its corresponding config. message SpanConfigEntry { - // Span is the keyspan the config is said to apply over. - Span span = 1 [(gogoproto.nullable) = false]; + reserved 1; - // Config is the set of attributes that apply over the corresponding keyspan. + // Target specifies the target the config applies over. + SpanConfigTarget target = 3 [(gogoproto.nullable) = false]; + + // Config is the set of attributes that apply over the corresponding target. SpanConfig config = 2 [(gogoproto.nullable) = false]; }; -// GetSpanConfigsRequest is used to fetch the span configurations over the -// specified keyspans. +// GetSpanConfigsRequest is used to fetch the span configurations and system +// span configurations. message GetSpanConfigsRequest { - // Spans to request the configurations for. The spans listed here are not - // allowed to overlap with one another. - repeated Span spans = 1 [(gogoproto.nullable) = false]; + reserved 1; + + // Targets to request configurations for. The targets listed here are not + // allowed to be duplicated/overlap with one another. + repeated SpanConfigTarget targets = 2 [(gogoproto.nullable) = false]; }; -// GetSpanConfigsResponse lists out the span configurations that overlap with -// the requested spans. +// GetSpanConfigsResponse lists out the span configurations and system span +// configurations that have been requested. message GetSpanConfigsResponse { // SpanConfigEntries capture the span configurations over the requested spans. // The results for each Span in the matching GetSpanConfigsRequest are // flattened out into a single slice, and follow the same ordering. It's // possible for there to be no configurations for a given span; there'll // simply be no entries for it. + // + // Any system span configurations set by the tenant are also returned if + // requested. repeated SpanConfigEntry span_config_entries = 1 [(gogoproto.nullable) = false]; }; -// UpdateSpanConfigsRequest is used to update the span configurations over the -// given spans. +// UpdateSpanConfigsRequest is used to update the span configurations and system +// span configurations over the given targets. +// +// This is a "targeted" API: the targets being deleted are expected to have been +// present exactly as specified. The same is true for targets being upserted with +// new configs. If targets aren't present, an error is returned. // -// This is a "targeted" API: the spans being deleted are expected to have been -// present with the same bounds (same start/end key); the same is true for spans -// being upserted with new configs. If bounds are mismatched, an error is -// returned. If spans are being added, they're expected to not overlap with any +// Adding configurations that target a span are expected to not overlap with any // existing spans. When divvying up an existing span into multiple others, // callers are expected to delete the old and upsert the new ones. This can -// happen as part of the same request; we delete the spans marked for deletion -// before upserting whatever was requested. +// happen as part of the same request, as we delete targets marked for deletion +// before upserting what was requested. // -// Spans are not allowed to overlap with other spans in the same list but can -// across lists. This is necessary to support the delete+upsert semantics -// described above. +// Targets are not allowed to overlap with other targets in the same list. They +// are, however, allowed to overlap across lists. This is necessary to support +// the delete+upsert semantics described above. message UpdateSpanConfigsRequest { - // ToDelete captures the spans we want to delete configs for. - repeated Span to_delete = 1 [(gogoproto.nullable) = false]; + reserved 1, 2; + + // ToDelete captures the targets we want to delete configs for. + repeated SpanConfigTarget to_delete = 3 [(gogoproto.nullable) = false]; - // ToUpsert captures the spans we want to upsert and the configs we want to - // upsert with. - repeated SpanConfigEntry to_upsert = 2 [(gogoproto.nullable) = false]; + // ToUpsert captures the targets we want to upsert and the configs we want + // to upsert with. + repeated SpanConfigEntry to_upsert = 4 [(gogoproto.nullable) = false]; }; message UpdateSpanConfigsResponse { }; diff --git a/pkg/rpc/BUILD.bazel b/pkg/rpc/BUILD.bazel index d5bac30cd37a..d94490551a6e 100644 --- a/pkg/rpc/BUILD.bazel +++ b/pkg/rpc/BUILD.bazel @@ -98,6 +98,7 @@ go_test( "//pkg/security", "//pkg/security/securitytest", "//pkg/settings/cluster", + "//pkg/spanconfig", "//pkg/testutils", "//pkg/testutils/skip", "//pkg/util", diff --git a/pkg/rpc/auth_tenant.go b/pkg/rpc/auth_tenant.go index dea182f6a8e5..819f20841a3e 100644 --- a/pkg/rpc/auth_tenant.go +++ b/pkg/rpc/auth_tenant.go @@ -16,6 +16,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/errors" "github.com/cockroachdb/logtags" "google.golang.org/grpc" ) @@ -269,14 +270,9 @@ func (a tenantAuthorizer) authTenantSettings( func (a tenantAuthorizer) authGetSpanConfigs( tenID roachpb.TenantID, args *roachpb.GetSpanConfigsRequest, ) error { - tenSpan := tenantPrefix(tenID) - for _, sp := range args.Spans { - rSpan, err := keys.SpanAddr(sp) - if err != nil { - return authError(err.Error()) - } - if !tenSpan.ContainsKeyRange(rSpan.Key, rSpan.EndKey) { - return authErrorf("requested key span %s not fully contained in tenant keyspace %s", rSpan, tenSpan) + for _, target := range args.Targets { + if err := validateSpanConfigTarget(tenID, target); err != nil { + return err } } return nil @@ -287,8 +283,53 @@ func (a tenantAuthorizer) authGetSpanConfigs( func (a tenantAuthorizer) authUpdateSpanConfigs( tenID roachpb.TenantID, args *roachpb.UpdateSpanConfigsRequest, ) error { - tenSpan := tenantPrefix(tenID) - validate := func(sp roachpb.Span) error { + for _, entry := range args.ToUpsert { + if err := validateSpanConfigTarget(tenID, entry.Target); err != nil { + return err + } + } + for _, target := range args.ToDelete { + if err := validateSpanConfigTarget(tenID, target); err != nil { + return err + } + } + + return nil +} + +// validateSpanConfigTarget validates that the tenant is authorized to interact +// with the supplied span config target. In particular, span targets must be +// wholly contained within the tenant keyspace and system span config targets +// must be well-formed. +func validateSpanConfigTarget( + tenID roachpb.TenantID, spanConfigTarget roachpb.SpanConfigTarget, +) error { + validateSystemTarget := func(target roachpb.SystemSpanConfigTarget) error { + if target.SourceTenantID != tenID { + return authErrorf("malformed source tenant field") + } + + if tenID == roachpb.SystemTenantID { + // Nothing to validate, the system tenant is allowed to set system span + // configurations over secondary tenants, itself, and the entire cluster. + return nil + } + + if target.TargetTenantID == nil { + return authErrorf("secondary tenants must explicitly target themselves") + } + + if target.SourceTenantID != *target.TargetTenantID { + return authErrorf( + "secondary tenants cannot interact with system span configurations of other tenants", + ) + } + + return nil + } + + validateSpan := func(sp roachpb.Span) error { + tenSpan := tenantPrefix(tenID) rSpan, err := keys.SpanAddr(sp) if err != nil { return authError(err.Error()) @@ -299,18 +340,14 @@ func (a tenantAuthorizer) authUpdateSpanConfigs( return nil } - for _, entry := range args.ToUpsert { - if err := validate(entry.Span); err != nil { - return err - } - } - for _, span := range args.ToDelete { - if err := validate(span); err != nil { - return err - } + switch spanConfigTarget.Union.(type) { + case *roachpb.SpanConfigTarget_Span: + return validateSpan(*spanConfigTarget.GetSpan()) + case *roachpb.SpanConfigTarget_SystemSpanConfigTarget: + return validateSystemTarget(*spanConfigTarget.GetSystemSpanConfigTarget()) + default: + return errors.AssertionFailedf("unknown span config target type") } - - return nil } func contextWithTenant(ctx context.Context, tenID roachpb.TenantID) context.Context { diff --git a/pkg/rpc/auth_test.go b/pkg/rpc/auth_test.go index a7f0186ebe8a..14e188f7749e 100644 --- a/pkg/rpc/auth_test.go +++ b/pkg/rpc/auth_test.go @@ -21,6 +21,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security" + "github.com/cockroachdb/cockroach/pkg/spanconfig" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/stretchr/testify/require" "google.golang.org/grpc/codes" @@ -185,16 +186,34 @@ func TestTenantAuthRequest(t *testing.T) { } return ru } - makeGetSpanConfigsReq := func(key, endKey string) *roachpb.GetSpanConfigsRequest { - sp := makeSpan(key, endKey) - return &roachpb.GetSpanConfigsRequest{Spans: []roachpb.Span{sp}} + makeSystemSpanConfigTarget := func(source, target uint64) roachpb.SpanConfigTarget { + targetID := roachpb.MakeTenantID(target) + return roachpb.SpanConfigTarget{ + Union: &roachpb.SpanConfigTarget_SystemSpanConfigTarget{ + SystemSpanConfigTarget: &roachpb.SystemSpanConfigTarget{ + SourceTenantID: roachpb.MakeTenantID(source), + TargetTenantID: &targetID, + }, + }, + } + } + makeSpanTarget := func(sp roachpb.Span) roachpb.SpanConfigTarget { + return spanconfig.MakeSpanTarget(sp).SpanConfigTargetProto() + } + makeGetSpanConfigsReq := func( + target roachpb.SpanConfigTarget, + ) *roachpb.GetSpanConfigsRequest { + return &roachpb.GetSpanConfigsRequest{Targets: []roachpb.SpanConfigTarget{target}} } - makeUpdateSpanConfigsReq := func(key, endKey string, delete bool) *roachpb.UpdateSpanConfigsRequest { - sp := makeSpan(key, endKey) + makeUpdateSpanConfigsReq := func(target roachpb.SpanConfigTarget, delete bool) *roachpb.UpdateSpanConfigsRequest { if delete { - return &roachpb.UpdateSpanConfigsRequest{ToDelete: []roachpb.Span{sp}} + return &roachpb.UpdateSpanConfigsRequest{ToDelete: []roachpb.SpanConfigTarget{target}} } - return &roachpb.UpdateSpanConfigsRequest{ToUpsert: []roachpb.SpanConfigEntry{{Span: sp}}} + return &roachpb.UpdateSpanConfigsRequest{ToUpsert: []roachpb.SpanConfigEntry{ + { + Target: target, + }, + }} } const noError = "" @@ -465,29 +484,64 @@ func TestTenantAuthRequest(t *testing.T) { expErr: noError, }, { - req: makeGetSpanConfigsReq("a", "b"), + req: makeGetSpanConfigsReq(makeSpanTarget(makeSpan("a", "b"))), expErr: `requested key span {a-b} not fully contained in tenant keyspace /Tenant/1{0-1}`, }, { - req: makeGetSpanConfigsReq(prefix(5, "a"), prefix(5, "b")), + req: makeGetSpanConfigsReq( + makeSpanTarget(makeSpan(prefix(5, "a"), prefix(5, "b"))), + ), expErr: `requested key span /Tenant/5"{a"-b"} not fully contained in tenant keyspace /Tenant/1{0-1}`, }, { - req: makeGetSpanConfigsReq(prefix(10, "a"), prefix(10, "b")), + req: makeGetSpanConfigsReq( + makeSpanTarget(makeSpan(prefix(10, "a"), prefix(10, "b"))), + ), expErr: noError, }, { - req: makeGetSpanConfigsReq(prefix(50, "a"), prefix(50, "b")), + req: makeGetSpanConfigsReq( + makeSpanTarget(makeSpan(prefix(50, "a"), prefix(50, "b"))), + ), expErr: `requested key span /Tenant/50"{a"-b"} not fully contained in tenant keyspace /Tenant/1{0-1}`, }, { - req: makeGetSpanConfigsReq("a", prefix(10, "b")), + req: makeGetSpanConfigsReq( + makeSpanTarget(makeSpan("a", prefix(10, "b"))), + ), expErr: `requested key span {a-/Tenant/10"b"} not fully contained in tenant keyspace /Tenant/1{0-1}`, }, { - req: makeGetSpanConfigsReq(prefix(10, "a"), prefix(20, "b")), + req: makeGetSpanConfigsReq( + makeSpanTarget(makeSpan(prefix(10, "a"), prefix(20, "b"))), + ), expErr: `requested key span /Tenant/{10"a"-20"b"} not fully contained in tenant keyspace /Tenant/1{0-1}`, }, + { + req: makeGetSpanConfigsReq(makeSystemSpanConfigTarget(10, 10)), + expErr: noError, + }, + { + req: makeGetSpanConfigsReq(makeSystemSpanConfigTarget(10, 20)), + expErr: `secondary tenants cannot interact with system span configurations of other tenants`, + }, + { + // Ensure tenant 10 (the tenant we test all these with) can't pretend + // to be tenant 20 to get access to system span configurations. + req: makeGetSpanConfigsReq(makeSystemSpanConfigTarget(20, 20)), + expErr: `malformed source tenant field`, + }, + { + req: makeGetSpanConfigsReq(roachpb.SpanConfigTarget{ + Union: &roachpb.SpanConfigTarget_SystemSpanConfigTarget{ + SystemSpanConfigTarget: &roachpb.SystemSpanConfigTarget{ + SourceTenantID: roachpb.MakeTenantID(10), + TargetTenantID: nil, + }, + }, + }), + expErr: `secondary tenants must explicitly target themselves`, + }, }, "/cockroach.roachpb.Internal/UpdateSpanConfigs": { { @@ -495,53 +549,139 @@ func TestTenantAuthRequest(t *testing.T) { expErr: noError, }, { - req: makeUpdateSpanConfigsReq("a", "b", true), + req: makeUpdateSpanConfigsReq( + makeSpanTarget(makeSpan("a", "b")), + true, + ), expErr: `requested key span {a-b} not fully contained in tenant keyspace /Tenant/1{0-1}`, }, { - req: makeUpdateSpanConfigsReq(prefix(5, "a"), prefix(5, "b"), true), + req: makeUpdateSpanConfigsReq( + makeSpanTarget(makeSpan(prefix(5, "a"), prefix(5, "b"))), + true, + ), expErr: `requested key span /Tenant/5"{a"-b"} not fully contained in tenant keyspace /Tenant/1{0-1}`, }, { - req: makeUpdateSpanConfigsReq(prefix(10, "a"), prefix(10, "b"), true), + req: makeUpdateSpanConfigsReq( + makeSpanTarget(makeSpan(prefix(10, "a"), prefix(10, "b"))), + true, + ), expErr: noError, }, { - req: makeUpdateSpanConfigsReq(prefix(50, "a"), prefix(50, "b"), true), + req: makeUpdateSpanConfigsReq( + makeSpanTarget(makeSpan(prefix(50, "a"), prefix(50, "b"))), + true, + ), expErr: `requested key span /Tenant/50"{a"-b"} not fully contained in tenant keyspace /Tenant/1{0-1}`, }, { - req: makeUpdateSpanConfigsReq("a", prefix(10, "b"), true), + req: makeUpdateSpanConfigsReq( + makeSpanTarget(makeSpan("a", prefix(10, "b"))), + true, + ), expErr: `requested key span {a-/Tenant/10"b"} not fully contained in tenant keyspace /Tenant/1{0-1}`, }, { - req: makeUpdateSpanConfigsReq(prefix(10, "a"), prefix(20, "b"), true), + req: makeUpdateSpanConfigsReq( + makeSpanTarget(makeSpan(prefix(10, "a"), prefix(20, "b"))), + true, + ), expErr: `requested key span /Tenant/{10"a"-20"b"} not fully contained in tenant keyspace /Tenant/1{0-1}`, }, { - req: makeUpdateSpanConfigsReq("a", "b", false), + req: makeUpdateSpanConfigsReq( + makeSpanTarget(makeSpan("a", "b")), + false, + ), expErr: `requested key span {a-b} not fully contained in tenant keyspace /Tenant/1{0-1}`, }, { - req: makeUpdateSpanConfigsReq(prefix(5, "a"), prefix(5, "b"), false), + req: makeUpdateSpanConfigsReq( + makeSpanTarget(makeSpan(prefix(5, "a"), prefix(5, "b"))), + false, + ), expErr: `requested key span /Tenant/5"{a"-b"} not fully contained in tenant keyspace /Tenant/1{0-1}`, }, { - req: makeUpdateSpanConfigsReq(prefix(10, "a"), prefix(10, "b"), false), + req: makeUpdateSpanConfigsReq( + makeSpanTarget(makeSpan(prefix(10, "a"), prefix(10, "b"))), + false, + ), expErr: noError, }, { - req: makeUpdateSpanConfigsReq(prefix(50, "a"), prefix(50, "b"), false), + req: makeUpdateSpanConfigsReq( + makeSpanTarget(makeSpan(prefix(50, "a"), prefix(50, "b"))), + false, + ), expErr: `requested key span /Tenant/50"{a"-b"} not fully contained in tenant keyspace /Tenant/1{0-1}`, }, { - req: makeUpdateSpanConfigsReq("a", prefix(10, "b"), false), + req: makeUpdateSpanConfigsReq( + makeSpanTarget(makeSpan("a", prefix(10, "b"))), + false, + ), expErr: `requested key span {a-/Tenant/10"b"} not fully contained in tenant keyspace /Tenant/1{0-1}`, }, { - req: makeUpdateSpanConfigsReq(prefix(10, "a"), prefix(20, "b"), false), + req: makeUpdateSpanConfigsReq( + makeSpanTarget(makeSpan(prefix(10, "a"), prefix(20, "b"))), + false, + ), expErr: `requested key span /Tenant/{10"a"-20"b"} not fully contained in tenant keyspace /Tenant/1{0-1}`, }, + { + req: makeUpdateSpanConfigsReq(makeSystemSpanConfigTarget(10, 10), false), + expErr: noError, + }, + { + req: makeUpdateSpanConfigsReq(makeSystemSpanConfigTarget(10, 20), false), + expErr: `secondary tenants cannot interact with system span configurations of other tenants`, + }, + { + // Ensure tenant 10 (the tenant we test all these with) can't pretend + // to be tenant 20 to get access to system span configurations. + req: makeUpdateSpanConfigsReq(makeSystemSpanConfigTarget(20, 20), false), + expErr: `malformed source tenant field`, + }, + { + req: makeUpdateSpanConfigsReq(roachpb.SpanConfigTarget{ + Union: &roachpb.SpanConfigTarget_SystemSpanConfigTarget{ + SystemSpanConfigTarget: &roachpb.SystemSpanConfigTarget{ + SourceTenantID: roachpb.MakeTenantID(10), + TargetTenantID: nil, + }, + }, + }, false), + expErr: `secondary tenants must explicitly target themselves`, + }, + { + req: makeUpdateSpanConfigsReq(makeSystemSpanConfigTarget(10, 10), true), + expErr: noError, + }, + { + req: makeUpdateSpanConfigsReq(makeSystemSpanConfigTarget(10, 20), true), + expErr: `secondary tenants cannot interact with system span configurations of other tenants`, + }, + { + // Ensure tenant 10 (the tenant we test all these with) can't pretend + // to be tenant 20 to get access to system span configurations. + req: makeUpdateSpanConfigsReq(makeSystemSpanConfigTarget(20, 20), true), + expErr: `malformed source tenant field`, + }, + { + req: makeUpdateSpanConfigsReq(roachpb.SpanConfigTarget{ + Union: &roachpb.SpanConfigTarget_SystemSpanConfigTarget{ + SystemSpanConfigTarget: &roachpb.SystemSpanConfigTarget{ + SourceTenantID: roachpb.MakeTenantID(10), + TargetTenantID: nil, + }, + }, + }, true), + expErr: `secondary tenants must explicitly target themselves`, + }, }, "/cockroach.rpc.Heartbeat/Ping": { diff --git a/pkg/server/node.go b/pkg/server/node.go index 620629c06849..7adf6c6d2d4f 100644 --- a/pkg/server/node.go +++ b/pkg/server/node.go @@ -1549,12 +1549,9 @@ func (emptyMetricStruct) MetricStruct() {} func (n *Node) GetSpanConfigs( ctx context.Context, req *roachpb.GetSpanConfigsRequest, ) (*roachpb.GetSpanConfigsResponse, error) { - targets := make([]spanconfig.Target, 0, len(req.Spans)) - for _, span := range req.Spans { - targets = append(targets, spanconfig.MakeSpanTarget(span)) - } - - records, err := n.spanConfigAccessor.GetSpanConfigRecords(ctx, targets) + records, err := n.spanConfigAccessor.GetSpanConfigRecords( + ctx, spanconfig.TargetProtosToTargets(req.Targets), + ) if err != nil { return nil, err } @@ -1571,15 +1568,9 @@ func (n *Node) UpdateSpanConfigs( // TODO(irfansharif): We want to protect ourselves from tenants creating // outlandishly large string buffers here and OOM-ing the host cluster. Is // the maximum protobuf message size enough of a safeguard? - - toDelete := make([]spanconfig.Target, 0, len(req.ToDelete)) - for _, toDel := range req.ToDelete { - toDelete = append(toDelete, spanconfig.MakeSpanTarget(toDel)) - } - - toUpsert := spanconfig.EntriesToRecords(req.ToUpsert) - - err := n.spanConfigAccessor.UpdateSpanConfigRecords(ctx, toDelete, toUpsert) + err := n.spanConfigAccessor.UpdateSpanConfigRecords( + ctx, spanconfig.TargetProtosToTargets(req.ToDelete), spanconfig.EntriesToRecords(req.ToUpsert), + ) if err != nil { return nil, err } diff --git a/pkg/spanconfig/spanconfigreconciler/reconciler.go b/pkg/spanconfig/spanconfigreconciler/reconciler.go index b78718a96e52..0ada0176549f 100644 --- a/pkg/spanconfig/spanconfigreconciler/reconciler.go +++ b/pkg/spanconfig/spanconfigreconciler/reconciler.go @@ -283,7 +283,7 @@ func (f *fullReconciler) fetchExistingSpanConfigs( EndKey: keys.TableDataMax, }) if f.knobs.ConfigureScratchRange { - target.EndKey = keys.ScratchRangeMax + target.GetSpan().EndKey = keys.ScratchRangeMax } } else { // Secondary tenants govern everything prefixed by their tenant ID. diff --git a/pkg/spanconfig/spanconfigtestutils/recorder.go b/pkg/spanconfig/spanconfigtestutils/recorder.go index ec97b46b4390..f7c963e8325f 100644 --- a/pkg/spanconfig/spanconfigtestutils/recorder.go +++ b/pkg/spanconfig/spanconfigtestutils/recorder.go @@ -98,8 +98,8 @@ func (r *KVAccessorRecorder) Recording(clear bool) string { if mi.batchIdx != mj.batchIdx { // sort by batch/ts order return mi.batchIdx < mj.batchIdx } - if !mi.update.Target.Key.Equal(mj.update.Target.Key) { // sort by key order - return mi.update.Target.Key.Compare(mj.update.Target.Key) < 0 + if !mi.update.Target.Equal(mj.update.Target) { // sort by target order + return mi.update.Target.Less(mj.update.Target) } return mi.update.Deletion() // sort deletes before upserts diff --git a/pkg/spanconfig/spanconfigtestutils/utils.go b/pkg/spanconfig/spanconfigtestutils/utils.go index fa452e099182..912f741f5e90 100644 --- a/pkg/spanconfig/spanconfigtestutils/utils.go +++ b/pkg/spanconfig/spanconfigtestutils/utils.go @@ -56,7 +56,7 @@ func ParseSpan(t *testing.T, sp string) roachpb.Span { // TODO(arul): Once we have system targets, we'll want to parse them here too // instead of just calling ParseSpan here. func ParseTarget(t *testing.T, target string) spanconfig.Target { - return spanconfig.Target(ParseSpan(t, target)) + return spanconfig.MakeSpanTarget(ParseSpan(t, target)) } // ParseConfig is helper function that constructs a roachpb.SpanConfig that's @@ -194,7 +194,10 @@ func PrintSpan(sp roachpb.Span) string { // PrintTarget is a helper function that prints a spanconfig.Target. func PrintTarget(target spanconfig.Target) string { - return PrintSpan(roachpb.Span(target)) + if target.GetSpan() != nil { + return PrintSpan(*target.GetSpan()) + } + panic("targets other than span targets are unsupported") } // PrintSpanConfig is a helper function that transforms roachpb.SpanConfig into diff --git a/pkg/spanconfig/target.go b/pkg/spanconfig/target.go index 053f7d5573d5..61878807471a 100644 --- a/pkg/spanconfig/target.go +++ b/pkg/spanconfig/target.go @@ -15,29 +15,46 @@ import "github.com/cockroachdb/cockroach/pkg/roachpb" // Target specifies the target of an associated span configuration. // // TODO(arul): In the future, we will expand this to include system targets. -type Target roachpb.Span +type Target struct { + span roachpb.Span +} + +// MakeTarget returns a new Target. +func MakeTarget(t roachpb.SpanConfigTarget) Target { + switch t.Union.(type) { + case *roachpb.SpanConfigTarget_Span: + return MakeSpanTarget(*t.GetSpan()) + default: + panic("cannot handle target") + } +} // MakeSpanTarget constructs and returns a span target. func MakeSpanTarget(span roachpb.Span) Target { - return Target(span) + return Target{span: span} } // GetSpan returns the underlying roachpb.Span if the target is a span target // and nil otherwise. func (t *Target) GetSpan() *roachpb.Span { - sp := roachpb.Span(*t) - return &sp + if t.span.Equal(roachpb.Span{}) { + return nil + } + return &t.span } // Encode returns an encoded span suitable for persistence in // system.span_configurations. func (t Target) Encode() roachpb.Span { - return roachpb.Span(t) + if t.GetSpan() != nil { + return t.span + } + panic("cannot handle any other type of target") } // Less returns true if the receiver is less than the supplied target. func (t *Target) Less(o Target) bool { - return t.Key.Compare(o.Key) < 0 + return t.Encode().Key.Compare(o.Encode().Key) < 0 } // Equal returns true iff the receiver is equal to the supplied target. @@ -53,12 +70,25 @@ func (t Target) String() string { // IsEmpty returns true if the receiver is an empty target. func (t Target) IsEmpty() bool { return t.GetSpan().Equal(roachpb.Span{}) + +} + +func (t Target) SpanConfigTargetProto() roachpb.SpanConfigTarget { + if t.GetSpan() != nil { + return roachpb.SpanConfigTarget{ + Union: &roachpb.SpanConfigTarget_Span{ + Span: t.GetSpan(), + }, + } + } + + panic("cannot handle any other type of target") } // DecodeTarget takes a raw span and decodes it into a Target given its // encoding. It is the inverse of Encode. -func DecodeTarget(sp roachpb.Span) Target { - return Target(sp) +func DecodeTarget(span roachpb.Span) Target { + return Target{span: span} } // Targets is a slice of span config targets. @@ -81,7 +111,7 @@ func RecordsToSpanConfigEntries(records []Record) []roachpb.SpanConfigEntry { entries := make([]roachpb.SpanConfigEntry, 0, len(records)) for _, rec := range records { entries = append(entries, roachpb.SpanConfigEntry{ - Span: *rec.Target.GetSpan(), + Target: rec.Target.SpanConfigTargetProto(), Config: rec.Config, }) } @@ -94,9 +124,29 @@ func EntriesToRecords(entries []roachpb.SpanConfigEntry) []Record { records := make([]Record, 0, len(entries)) for _, entry := range entries { records = append(records, Record{ - Target: MakeSpanTarget(entry.Span), + Target: MakeTarget(entry.Target), Config: entry.Config, }) } return records } + +// TargetsToTargetProtos converts a list of targets to a list of +// roachpb.SpanConfigTarget protos suitable for sending over the wire. +func TargetsToTargetProtos(targets []Target) []roachpb.SpanConfigTarget { + targetProtos := make([]roachpb.SpanConfigTarget, 0, len(targets)) + for _, target := range targets { + targetProtos = append(targetProtos, target.SpanConfigTargetProto()) + } + return targetProtos +} + +// TargetProtosToTargets converts a list of roachpb.SpanConfigTargets +// (received over the wire) to a list of Targets. +func TargetProtosToTargets(protoTargtets []roachpb.SpanConfigTarget) []Target { + targets := make([]Target, 0, len(protoTargtets)) + for _, t := range protoTargtets { + targets = append(targets, MakeTarget(t)) + } + return targets +} From 13880c9e8d6abc5c7fbd624254f088432b500004 Mon Sep 17 00:00:00 2001 From: arulajmani Date: Tue, 8 Feb 2022 01:02:52 -0500 Subject: [PATCH 2/3] spanconfig: introduce SystemTarget and encoding/decoding methods This patch introduces a new spanconfig.SystemTarget type. When used in conjunction with a span configuration, tenants (and the system tenant) can use this type to target multiple keyspans with the associated span configuration. We use this type to expand the a spanconfig.Target to optionally embedd a SystemTarget (in additon to a span target like it did before). We also add special encoding/decoding methods for system targets, which we will make use of when persisting them in `system.span_configurations`. We do so by reserving a special SystemSpanConfigSpan at the end of the System range. This allows us to then assign special meaning to key prefixes carved out of this range and the corresponding spans stored in `system.span_configurations`. We must take special care that the host tenant doesn't install span configurations to these keyspans directly, and as such, we make exceptions in the translator and reconciler. Release note: None --- pkg/BUILD.bazel | 1 + pkg/ccl/kvccl/kvtenantccl/connector.go | 6 +- .../seed_tenant_span_configs_external_test.go | 14 +- .../datadriven_test.go | 2 +- .../spanconfigreconcilerccl/testdata/basic | 2 +- .../spanconfigreconcilerccl/testdata/indexes | 2 +- .../testdata/named_zones | 22 +- .../testdata/partitions | 8 +- .../datadriven_test.go | 4 +- .../testdata/full_translate | 2 +- .../full_translate_named_zones_deleted | 2 +- .../testdata/named_zones | 2 +- .../testdata/system_database | 2 +- pkg/keys/constants.go | 24 +++ pkg/keys/doc.go | 23 ++- pkg/keys/printer.go | 4 + pkg/keys/spans.go | 6 + pkg/kv/kvserver/client_spanconfigs_test.go | 2 +- .../migrations/migrate_span_configs_test.go | 10 +- .../migrations/seed_tenant_span_configs.go | 4 +- pkg/rpc/auth_test.go | 2 +- pkg/server/node.go | 6 +- pkg/spanconfig/BUILD.bazel | 17 +- pkg/spanconfig/spanconfig.go | 2 +- .../spanconfigkvaccessor/kvaccessor.go | 9 +- .../spanconfigkvaccessor/validation_test.go | 22 +- .../spanconfigkvsubscriber/kvsubscriber.go | 6 +- .../span_config_decoder_test.go | 4 +- .../spanconfigreconciler/reconciler.go | 30 ++- .../spanconfigsqltranslator/sqltranslator.go | 22 +- .../spanconfigstore/spanconfigstore.go | 22 +- .../spanconfigstore/spanconfigstore_test.go | 12 +- pkg/spanconfig/spanconfigstore/store.go | 8 +- pkg/spanconfig/spanconfigstore/store_test.go | 8 +- pkg/spanconfig/spanconfigtestutils/utils.go | 8 +- pkg/spanconfig/systemtarget.go | 189 ++++++++++++++++++ pkg/spanconfig/target.go | 159 +++++++++++---- pkg/spanconfig/target_test.go | 172 ++++++++++++++++ pkg/sql/tenant.go | 4 +- 39 files changed, 675 insertions(+), 169 deletions(-) create mode 100644 pkg/spanconfig/systemtarget.go create mode 100644 pkg/spanconfig/target_test.go diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index dca83b8b20b1..0afb3647b45d 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -201,6 +201,7 @@ ALL_TESTS = [ "//pkg/spanconfig/spanconfigsqlwatcher:spanconfigsqlwatcher_test", "//pkg/spanconfig/spanconfigstore:spanconfigstore_test", "//pkg/spanconfig/spanconfigtestutils:spanconfigtestutils_test", + "//pkg/spanconfig:spanconfig_test", "//pkg/sql/catalog/catalogkeys:catalogkeys_test", "//pkg/sql/catalog/catformat:catformat_test", "//pkg/sql/catalog/catpb:catpb_test", diff --git a/pkg/ccl/kvccl/kvtenantccl/connector.go b/pkg/ccl/kvccl/kvtenantccl/connector.go index 58133bf08a1d..82a28c164eb4 100644 --- a/pkg/ccl/kvccl/kvtenantccl/connector.go +++ b/pkg/ccl/kvccl/kvtenantccl/connector.go @@ -462,7 +462,7 @@ func (c *Connector) GetSpanConfigRecords( ) (records []spanconfig.Record, _ error) { if err := c.withClient(ctx, func(ctx context.Context, c *client) error { resp, err := c.GetSpanConfigs(ctx, &roachpb.GetSpanConfigsRequest{ - Targets: spanconfig.TargetsToTargetProtos(targets), + Targets: spanconfig.TargetsToProtos(targets), }) if err != nil { return err @@ -483,8 +483,8 @@ func (c *Connector) UpdateSpanConfigRecords( ) error { return c.withClient(ctx, func(ctx context.Context, c *client) error { _, err := c.UpdateSpanConfigs(ctx, &roachpb.UpdateSpanConfigsRequest{ - ToDelete: spanconfig.TargetsToTargetProtos(toDelete), - ToUpsert: spanconfig.RecordsToSpanConfigEntries(toUpsert), + ToDelete: spanconfig.TargetsToProtos(toDelete), + ToUpsert: spanconfig.RecordsToEntries(toUpsert), }) return err }) diff --git a/pkg/ccl/migrationccl/migrationsccl/seed_tenant_span_configs_external_test.go b/pkg/ccl/migrationccl/migrationsccl/seed_tenant_span_configs_external_test.go index 71e53b452831..42e477bb598f 100644 --- a/pkg/ccl/migrationccl/migrationsccl/seed_tenant_span_configs_external_test.go +++ b/pkg/ccl/migrationccl/migrationsccl/seed_tenant_span_configs_external_test.go @@ -71,11 +71,11 @@ func TestPreSeedSpanConfigsWrittenWhenActive(t *testing.T) { { records, err := scKVAccessor.GetSpanConfigRecords(ctx, []spanconfig.Target{ - spanconfig.MakeSpanTarget(tenantSpan), + spanconfig.MakeTargetFromSpan(tenantSpan), }) require.NoError(t, err) require.Len(t, records, 1) - require.Equal(t, *records[0].Target.GetSpan(), tenantSeedSpan) + require.Equal(t, records[0].Target.GetSpan(), tenantSeedSpan) } } @@ -106,7 +106,7 @@ func TestSeedTenantSpanConfigs(t *testing.T) { tenantID := roachpb.MakeTenantID(10) tenantPrefix := keys.MakeTenantPrefix(tenantID) - tenantTarget := spanconfig.MakeSpanTarget( + tenantTarget := spanconfig.MakeTargetFromSpan( roachpb.Span{Key: tenantPrefix, EndKey: tenantPrefix.PrefixEnd()}, ) tenantSeedSpan := roachpb.Span{Key: tenantPrefix, EndKey: tenantPrefix.Next()} @@ -144,7 +144,7 @@ func TestSeedTenantSpanConfigs(t *testing.T) { }) require.NoError(t, err) require.Len(t, records, 1) - require.Equal(t, *records[0].Target.GetSpan(), tenantSeedSpan) + require.Equal(t, records[0].Target.GetSpan(), tenantSeedSpan) } } @@ -175,7 +175,7 @@ func TestSeedTenantSpanConfigsWithExistingEntry(t *testing.T) { tenantID := roachpb.MakeTenantID(10) tenantPrefix := keys.MakeTenantPrefix(tenantID) - tenantTarget := spanconfig.MakeSpanTarget( + tenantTarget := spanconfig.MakeTargetFromSpan( roachpb.Span{Key: tenantPrefix, EndKey: tenantPrefix.PrefixEnd()}, ) tenantSeedSpan := roachpb.Span{Key: tenantPrefix, EndKey: tenantPrefix.Next()} @@ -200,7 +200,7 @@ func TestSeedTenantSpanConfigsWithExistingEntry(t *testing.T) { }) require.NoError(t, err) require.Len(t, records, 1) - require.Equal(t, *records[0].Target.GetSpan(), tenantSeedSpan) + require.Equal(t, records[0].Target.GetSpan(), tenantSeedSpan) } // Ensure the cluster version bump goes through successfully. @@ -215,6 +215,6 @@ func TestSeedTenantSpanConfigsWithExistingEntry(t *testing.T) { }) require.NoError(t, err) require.Len(t, records, 1) - require.Equal(t, *records[0].Target.GetSpan(), tenantSeedSpan) + require.Equal(t, records[0].Target.GetSpan(), tenantSeedSpan) } } diff --git a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go index 0e85dc64e406..48f211dc7519 100644 --- a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go +++ b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go @@ -192,7 +192,7 @@ func TestDataDriven(t *testing.T) { return nil }) records, err := kvAccessor.GetSpanConfigRecords( - ctx, []spanconfig.Target{spanconfig.MakeSpanTarget(keys.EverythingSpan)}, + ctx, []spanconfig.Target{spanconfig.MakeTargetFromSpan(keys.EverythingSpan)}, ) require.NoError(t, err) sort.Slice(records, func(i, j int) bool { diff --git a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/basic b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/basic index 7024b4e46f10..76625c86f346 100644 --- a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/basic +++ b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/basic @@ -11,7 +11,7 @@ upsert /{Min-System/NodeLiveness} ttl_seconds=3600 num_replicas=5 upsert /System/NodeLiveness{-Max} ttl_seconds=600 num_replicas=5 upsert /System/{NodeLivenessMax-tsd} range system upsert /System{/tsd-tse} range default -upsert /System{tse-/Max} range system +upsert /System{tse-/SystemSpanConfigKeys} range system upsert /Table/{SystemConfigSpan/Start-4} database system (host) upsert /Table/{4-5} database system (host) upsert /Table/{5-6} database system (host) diff --git a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/indexes b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/indexes index 0cd7925d2863..20642cb74d7b 100644 --- a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/indexes +++ b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/indexes @@ -35,8 +35,8 @@ ALTER INDEX db.t@idx CONFIGURE ZONE USING num_voters = 5; # - any future indexes that may be added to this table (table's config) mutations ---- -delete /Table/10{6-7} upsert /Table/106{-/2} num_replicas=7 +delete /Table/10{6-7} upsert /Table/106/{2-3} num_replicas=7 num_voters=5 upsert /Table/10{6/3-7} num_replicas=7 diff --git a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/named_zones b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/named_zones index bf7a0063c39f..7b379fe79757 100644 --- a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/named_zones +++ b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/named_zones @@ -13,7 +13,7 @@ state limit=5 /System/NodeLiveness{-Max} ttl_seconds=600 num_replicas=5 /System/{NodeLivenessMax-tsd} range system /System{/tsd-tse} range default -/System{tse-/Max} range system +/System{tse-/SystemSpanConfigKeys} range system ... # Adding an explicit zone configuration for the timeseries range should work @@ -48,8 +48,8 @@ mutations ---- delete /System/{NodeLivenessMax-tsd} upsert /System/{NodeLivenessMax-tsd} range default -delete /System{tse-/Max} -upsert /System{tse-/Max} range default +delete /System{tse-/SystemSpanConfigKeys} +upsert /System{tse-/SystemSpanConfigKeys} range default state limit=5 ---- @@ -57,7 +57,7 @@ state limit=5 /System/NodeLiveness{-Max} ttl_seconds=600 num_replicas=7 /System/{NodeLivenessMax-tsd} range default /System{/tsd-tse} ttl_seconds=42 -/System{tse-/Max} range default +/System{tse-/SystemSpanConfigKeys} range default ... # Ensure that discarding other named zones behave as expected (reparenting them @@ -80,7 +80,7 @@ state limit=5 /System/NodeLiveness{-Max} ttl_seconds=600 num_replicas=7 /System/{NodeLivenessMax-tsd} range default /System{/tsd-tse} range default -/System{tse-/Max} range default +/System{tse-/SystemSpanConfigKeys} range default ... @@ -106,8 +106,8 @@ delete /System/{NodeLivenessMax-tsd} upsert /System/{NodeLivenessMax-tsd} ttl_seconds=50 delete /System{/tsd-tse} upsert /System{/tsd-tse} ttl_seconds=50 -delete /System{tse-/Max} -upsert /System{tse-/Max} ttl_seconds=50 +delete /System{tse-/SystemSpanConfigKeys} +upsert /System{tse-/SystemSpanConfigKeys} ttl_seconds=50 delete /Table/10{6-7} upsert /Table/10{6-7} ttl_seconds=50 @@ -117,7 +117,7 @@ state limit=5 /System/NodeLiveness{-Max} ttl_seconds=600 num_replicas=7 /System/{NodeLivenessMax-tsd} ttl_seconds=50 /System{/tsd-tse} ttl_seconds=50 -/System{tse-/Max} ttl_seconds=50 +/System{tse-/SystemSpanConfigKeys} ttl_seconds=50 ... state offset=46 @@ -152,8 +152,8 @@ mutations ---- delete /System/{NodeLivenessMax-tsd} upsert /System/{NodeLivenessMax-tsd} ttl_seconds=100 -delete /System{tse-/Max} -upsert /System{tse-/Max} ttl_seconds=100 +delete /System{tse-/SystemSpanConfigKeys} +upsert /System{tse-/SystemSpanConfigKeys} ttl_seconds=100 state limit=5 ---- @@ -161,5 +161,5 @@ state limit=5 /System/NodeLiveness{-Max} ttl_seconds=600 num_replicas=7 /System/{NodeLivenessMax-tsd} ttl_seconds=100 /System{/tsd-tse} ttl_seconds=50 -/System{tse-/Max} ttl_seconds=100 +/System{tse-/SystemSpanConfigKeys} ttl_seconds=100 ... diff --git a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/partitions b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/partitions index 9ca109a832fc..90582be31c63 100644 --- a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/partitions +++ b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/partitions @@ -63,8 +63,8 @@ ALTER PARTITION one_two OF TABLE db.t CONFIGURE ZONE USING global_reads = true mutations ---- -delete /Table/10{6-7} upsert /Table/106{-/1/1} num_replicas=7 num_voters=5 +delete /Table/10{6-7} upsert /Table/106/1/{1-2} global_reads=true num_replicas=7 num_voters=5 upsert /Table/106/1/{2-3} global_reads=true num_replicas=7 num_voters=5 upsert /Table/10{6/1/3-7} num_replicas=7 num_voters=5 @@ -86,8 +86,8 @@ ALTER PARTITION three_four OF TABLE db.t CONFIGURE ZONE USING gc.ttlseconds = 5 mutations ---- -delete /Table/10{6/1/3-7} upsert /Table/106/1/{3-4} ttl_seconds=5 num_replicas=7 num_voters=5 +delete /Table/10{6/1/3-7} upsert /Table/106/1/{4-5} ttl_seconds=5 num_replicas=7 num_voters=5 upsert /Table/10{6/1/5-7} num_replicas=7 num_voters=5 @@ -120,11 +120,11 @@ ALTER PARTITION default OF TABLE db.t CONFIGURE ZONE USING num_voters = 6 mutations ---- -delete /Table/106{-/1/1} upsert /Table/106{-/1} num_replicas=7 num_voters=5 +delete /Table/106{-/1/1} upsert /Table/106/1{-/1} num_replicas=7 num_voters=6 -delete /Table/10{6/1/5-7} upsert /Table/106/{1/5-2} num_replicas=7 num_voters=6 +delete /Table/10{6/1/5-7} upsert /Table/10{6/2-7} num_replicas=7 num_voters=5 state offset=47 diff --git a/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go b/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go index d18d470cdf65..84d106f5ca13 100644 --- a/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go +++ b/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go @@ -167,7 +167,7 @@ func TestDataDriven(t *testing.T) { var output strings.Builder for _, record := range records { - output.WriteString(fmt.Sprintf("%-42s %s\n", *record.Target.GetSpan(), + output.WriteString(fmt.Sprintf("%-42s %s\n", record.Target.GetSpan(), spanconfigtestutils.PrintSpanConfigDiffedAgainstDefaults(record.Config))) } return output.String() @@ -182,7 +182,7 @@ func TestDataDriven(t *testing.T) { }) var output strings.Builder for _, record := range records { - output.WriteString(fmt.Sprintf("%-42s %s\n", *record.Target.GetSpan(), + output.WriteString(fmt.Sprintf("%-42s %s\n", record.Target.GetSpan(), spanconfigtestutils.PrintSpanConfigDiffedAgainstDefaults(record.Config))) } return output.String() diff --git a/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/full_translate b/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/full_translate index 5d9ba9080aea..5c37ec3919e0 100644 --- a/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/full_translate +++ b/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/full_translate @@ -24,7 +24,7 @@ full-translate /System/NodeLiveness{-Max} ttl_seconds=600 num_replicas=5 /System/{NodeLivenessMax-tsd} range system /System{/tsd-tse} range default -/System{tse-/Max} range system +/System{tse-/SystemSpanConfigKeys} range system /Table/{SystemConfigSpan/Start-4} database system (host) /Table/{4-5} database system (host) /Table/{5-6} database system (host) diff --git a/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/full_translate_named_zones_deleted b/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/full_translate_named_zones_deleted index a14690875db6..1fd076b7e38e 100644 --- a/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/full_translate_named_zones_deleted +++ b/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/full_translate_named_zones_deleted @@ -40,7 +40,7 @@ full-translate /System/NodeLiveness{-Max} range default /System/{NodeLivenessMax-tsd} range default /System{/tsd-tse} range default -/System{tse-/Max} range default +/System{tse-/SystemSpanConfigKeys} range default /Table/{SystemConfigSpan/Start-4} database system (host) /Table/{4-5} database system (host) /Table/{5-6} database system (host) diff --git a/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/named_zones b/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/named_zones index 6bd36c51c3a3..cd3ab2fb49f4 100644 --- a/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/named_zones +++ b/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/named_zones @@ -47,4 +47,4 @@ translate named-zone=liveness translate named-zone=system ---- /System/{NodeLivenessMax-tsd} range system -/System{tse-/Max} range system +/System{tse-/SystemSpanConfigKeys} range system diff --git a/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/system_database b/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/system_database index f0b296965327..711f0d116437 100644 --- a/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/system_database +++ b/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/system_database @@ -111,7 +111,7 @@ full-translate /System/NodeLiveness{-Max} ttl_seconds=600 num_replicas=5 /System/{NodeLivenessMax-tsd} range system /System{/tsd-tse} range default -/System{tse-/Max} range system +/System{tse-/SystemSpanConfigKeys} range system /Table/{SystemConfigSpan/Start-4} ignore_strict_gc=true num_replicas=7 rangefeed_enabled=true /Table/{4-5} ignore_strict_gc=true num_replicas=7 rangefeed_enabled=true /Table/{5-6} ignore_strict_gc=true num_replicas=7 rangefeed_enabled=true diff --git a/pkg/keys/constants.go b/pkg/keys/constants.go index fd582b1a7ff3..c26a935135b8 100644 --- a/pkg/keys/constants.go +++ b/pkg/keys/constants.go @@ -301,6 +301,30 @@ var ( TimeseriesPrefix = roachpb.Key(makeKey(SystemPrefix, roachpb.RKey("tsd"))) // TimeseriesKeyMax is the maximum value for any timeseries data. TimeseriesKeyMax = TimeseriesPrefix.PrefixEnd() + // + // SystemSpanConfigPrefix is the key prefix for all system span config data. + // + // We sort this at the end of the system keyspace to easily be able to exclude + // it from the span configuration that applies over the system keyspace. This + // is important because spans carved out from this range are used to store + // system span configurations in the `system.span_configurations` table, and + // as such, have special meaning associated with them; nothing is stored in + // the range itself. + SystemSpanConfigPrefix = roachpb.Key(makeKey(SystemPrefix, roachpb.RKey("\xffsys-scfg"))) + // SystemSpanConfigEntireKeyspace is the key prefix used to denote that the + // associated system span configuration applies over the entire keyspace + // (including all secondary tenants). + SystemSpanConfigEntireKeyspace = roachpb.Key(makeKey(SystemSpanConfigPrefix, roachpb.RKey("host/all"))) + // SystemSpanConfigHostOnTenantKeyspace is the key prefix used to denote that + // the associated system span configuration was applied by the host tenant + // over the keyspace of a secondary tenant. + SystemSpanConfigHostOnTenantKeyspace = roachpb.Key(makeKey(SystemSpanConfigPrefix, roachpb.RKey("host/ten/"))) + // SystemSpanConfigSecondaryTenantOnEntireKeyspace is the key prefix used to + // denote that the associated system span configuration was applied by a + // secondary tenant over its entire keyspace. + SystemSpanConfigSecondaryTenantOnEntireKeyspace = roachpb.Key(makeKey(SystemSpanConfigPrefix, roachpb.RKey("ten/"))) + // SystemSpanConfigKeyMax is the maximum value for any system span config key. + SystemSpanConfigKeyMax = SystemSpanConfigPrefix.PrefixEnd() // 3. System tenant SQL keys // diff --git a/pkg/keys/doc.go b/pkg/keys/doc.go index 47f5fc5dfc58..2bda3c4b54c9 100644 --- a/pkg/keys/doc.go +++ b/pkg/keys/doc.go @@ -246,17 +246,18 @@ var _ = [...]interface{}{ // 2. System keys: This is where we store global, system data which is // replicated across the cluster. SystemPrefix, - NodeLivenessPrefix, // "\x00liveness-" - BootstrapVersionKey, // "bootstrap-version" - descIDGenerator, // "desc-idgen" - NodeIDGenerator, // "node-idgen" - RangeIDGenerator, // "range-idgen" - StatusPrefix, // "status-" - StatusNodePrefix, // "status-node-" - StoreIDGenerator, // "store-idgen" - MigrationPrefix, // "system-version/" - MigrationLease, // "system-version/lease" - TimeseriesPrefix, // "tsd" + NodeLivenessPrefix, // "\x00liveness-" + BootstrapVersionKey, // "bootstrap-version" + descIDGenerator, // "desc-idgen" + NodeIDGenerator, // "node-idgen" + RangeIDGenerator, // "range-idgen" + StatusPrefix, // "status-" + StatusNodePrefix, // "status-node-" + StoreIDGenerator, // "store-idgen" + MigrationPrefix, // "system-version/" + MigrationLease, // "system-version/lease" + TimeseriesPrefix, // "tsd" + SystemSpanConfigPrefix, // "xffsys-scfg" SystemMax, // 3. System tenant SQL keys: This is where we store all system-tenant diff --git a/pkg/keys/printer.go b/pkg/keys/printer.go index 2125424da120..f2e589e2848a 100644 --- a/pkg/keys/printer.go +++ b/pkg/keys/printer.go @@ -128,6 +128,10 @@ var ( ppFunc: timeseriesKeyPrint, PSFunc: parseUnsupported, }, + {Name: "/SystemSpanConfigKeys", prefix: SystemSpanConfigPrefix, + ppFunc: decodeKeyPrint, + PSFunc: parseUnsupported, + }, }}, {Name: "/NamespaceTable", start: NamespaceTableMin, end: NamespaceTableMax, Entries: []DictEntry{ {Name: "", prefix: nil, ppFunc: decodeKeyPrint, PSFunc: parseUnsupported}, diff --git a/pkg/keys/spans.go b/pkg/keys/spans.go index 02e513ada545..01964926faee 100644 --- a/pkg/keys/spans.go +++ b/pkg/keys/spans.go @@ -37,6 +37,12 @@ var ( // TimeseriesSpan holds all the timeseries data in the cluster. TimeseriesSpan = roachpb.Span{Key: TimeseriesPrefix, EndKey: TimeseriesKeyMax} + // SystemSpanConfigSpan is part of the system keyspace that is used to carve + // out spans for system span configurations. No data is stored in these spans, + // instead, special meaning is assigned to them when stored in + // `system.span_configurations`. + SystemSpanConfigSpan = roachpb.Span{Key: SystemSpanConfigPrefix, EndKey: SystemSpanConfigKeyMax} + // SystemConfigSpan is the range of system objects which will be gossiped. SystemConfigSpan = roachpb.Span{Key: SystemConfigSplitKey, EndKey: SystemConfigTableDataMax} diff --git a/pkg/kv/kvserver/client_spanconfigs_test.go b/pkg/kv/kvserver/client_spanconfigs_test.go index e673df55d1ab..c9bfd59838fd 100644 --- a/pkg/kv/kvserver/client_spanconfigs_test.go +++ b/pkg/kv/kvserver/client_spanconfigs_test.go @@ -72,7 +72,7 @@ func TestSpanConfigUpdateAppliedToReplica(t *testing.T) { deleted, added := spanConfigStore.Apply( ctx, false, /* dryrun */ - spanconfig.Addition(spanconfig.MakeSpanTarget(span), conf), + spanconfig.Addition(spanconfig.MakeTargetFromSpan(span), conf), ) require.Empty(t, deleted) require.Len(t, added, 1) diff --git a/pkg/migration/migrations/migrate_span_configs_test.go b/pkg/migration/migrations/migrate_span_configs_test.go index 389bed931f57..9d1b4368d3a0 100644 --- a/pkg/migration/migrations/migrate_span_configs_test.go +++ b/pkg/migration/migrations/migrate_span_configs_test.go @@ -69,7 +69,7 @@ func TestEnsureSpanConfigReconciliation(t *testing.T) { { // Ensure that no span config entries are found. records, err := scKVAccessor.GetSpanConfigRecords(ctx, []spanconfig.Target{ - spanconfig.MakeSpanTarget(keys.EverythingSpan), + spanconfig.MakeTargetFromSpan(keys.EverythingSpan), }) require.NoError(t, err) require.Empty(t, records) @@ -91,7 +91,7 @@ func TestEnsureSpanConfigReconciliation(t *testing.T) { { // Ensure that the host tenant's span configs are installed. records, err := scKVAccessor.GetSpanConfigRecords(ctx, []spanconfig.Target{ - spanconfig.MakeSpanTarget(keys.EverythingSpan), + spanconfig.MakeTargetFromSpan(keys.EverythingSpan), }) require.NoError(t, err) require.NotEmpty(t, records) @@ -154,7 +154,7 @@ func TestEnsureSpanConfigReconciliationMultiNode(t *testing.T) { { // Ensure that no span config entries are to be found. records, err := scKVAccessor.GetSpanConfigRecords(ctx, []spanconfig.Target{ - spanconfig.MakeSpanTarget(keys.EverythingSpan), + spanconfig.MakeTargetFromSpan(keys.EverythingSpan), }) require.NoError(t, err) require.Empty(t, records) @@ -176,7 +176,7 @@ func TestEnsureSpanConfigReconciliationMultiNode(t *testing.T) { { // Ensure that the host tenant's span configs are installed. records, err := scKVAccessor.GetSpanConfigRecords(ctx, []spanconfig.Target{ - spanconfig.MakeSpanTarget(keys.EverythingSpan), + spanconfig.MakeTargetFromSpan(keys.EverythingSpan), }) require.NoError(t, err) require.NotEmpty(t, records) @@ -220,7 +220,7 @@ func TestEnsureSpanConfigSubscription(t *testing.T) { testutils.SucceedsSoon(t, func() error { records, err := scKVAccessor.GetSpanConfigRecords(ctx, []spanconfig.Target{ - spanconfig.MakeSpanTarget(keys.EverythingSpan), + spanconfig.MakeTargetFromSpan(keys.EverythingSpan), }) require.NoError(t, err) if len(records) == 0 { diff --git a/pkg/migration/migrations/seed_tenant_span_configs.go b/pkg/migration/migrations/seed_tenant_span_configs.go index c28601bf732a..c0b903c7cd9e 100644 --- a/pkg/migration/migrations/seed_tenant_span_configs.go +++ b/pkg/migration/migrations/seed_tenant_span_configs.go @@ -59,7 +59,7 @@ func seedTenantSpanConfigsMigration( // boundary. Look towards CreateTenantRecord for more details. tenantSpanConfig := d.SpanConfig.Default tenantPrefix := keys.MakeTenantPrefix(tenantID) - tenantTarget := spanconfig.MakeSpanTarget(roachpb.Span{ + tenantTarget := spanconfig.MakeTargetFromSpan(roachpb.Span{ Key: tenantPrefix, EndKey: tenantPrefix.PrefixEnd(), }) @@ -69,7 +69,7 @@ func seedTenantSpanConfigsMigration( } toUpsert := []spanconfig.Record{ { - Target: spanconfig.MakeSpanTarget(tenantSeedSpan), + Target: spanconfig.MakeTargetFromSpan(tenantSeedSpan), Config: tenantSpanConfig, }, } diff --git a/pkg/rpc/auth_test.go b/pkg/rpc/auth_test.go index 14e188f7749e..613e741d41b2 100644 --- a/pkg/rpc/auth_test.go +++ b/pkg/rpc/auth_test.go @@ -198,7 +198,7 @@ func TestTenantAuthRequest(t *testing.T) { } } makeSpanTarget := func(sp roachpb.Span) roachpb.SpanConfigTarget { - return spanconfig.MakeSpanTarget(sp).SpanConfigTargetProto() + return spanconfig.MakeTargetFromSpan(sp).ToProto() } makeGetSpanConfigsReq := func( target roachpb.SpanConfigTarget, diff --git a/pkg/server/node.go b/pkg/server/node.go index 7adf6c6d2d4f..8994f4ad24b4 100644 --- a/pkg/server/node.go +++ b/pkg/server/node.go @@ -1550,14 +1550,14 @@ func (n *Node) GetSpanConfigs( ctx context.Context, req *roachpb.GetSpanConfigsRequest, ) (*roachpb.GetSpanConfigsResponse, error) { records, err := n.spanConfigAccessor.GetSpanConfigRecords( - ctx, spanconfig.TargetProtosToTargets(req.Targets), + ctx, spanconfig.TargetsFromProtos(req.Targets), ) if err != nil { return nil, err } return &roachpb.GetSpanConfigsResponse{ - SpanConfigEntries: spanconfig.RecordsToSpanConfigEntries(records), + SpanConfigEntries: spanconfig.RecordsToEntries(records), }, nil } @@ -1569,7 +1569,7 @@ func (n *Node) UpdateSpanConfigs( // outlandishly large string buffers here and OOM-ing the host cluster. Is // the maximum protobuf message size enough of a safeguard? err := n.spanConfigAccessor.UpdateSpanConfigRecords( - ctx, spanconfig.TargetProtosToTargets(req.ToDelete), spanconfig.EntriesToRecords(req.ToUpsert), + ctx, spanconfig.TargetsFromProtos(req.ToDelete), spanconfig.EntriesToRecords(req.ToUpsert), ) if err != nil { return nil, err diff --git a/pkg/spanconfig/BUILD.bazel b/pkg/spanconfig/BUILD.bazel index 6373f91a57ff..2b1045833537 100644 --- a/pkg/spanconfig/BUILD.bazel +++ b/pkg/spanconfig/BUILD.bazel @@ -1,9 +1,10 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "spanconfig", srcs = [ "spanconfig.go", + "systemtarget.go", "target.go", "testing_knobs.go", ], @@ -16,6 +17,20 @@ go_library( "//pkg/roachpb", "//pkg/sql/catalog", "//pkg/sql/catalog/descpb", + "//pkg/util/encoding", "//pkg/util/hlc", + "@com_github_cockroachdb_errors//:errors", + ], +) + +go_test( + name = "spanconfig_test", + srcs = ["target_test.go"], + embed = [":spanconfig"], + deps = [ + "//pkg/keys", + "//pkg/roachpb", + "//pkg/testutils", + "@com_github_stretchr_testify//require", ], ) diff --git a/pkg/spanconfig/spanconfig.go b/pkg/spanconfig/spanconfig.go index c732da467353..6ed661b6e6a5 100644 --- a/pkg/spanconfig/spanconfig.go +++ b/pkg/spanconfig/spanconfig.go @@ -244,7 +244,7 @@ type Record struct { // IsEmpty returns true if the receiver is an empty Record. func (r *Record) IsEmpty() bool { - return r.Target.IsEmpty() && r.Config.IsEmpty() + return r.Target.isEmpty() && r.Config.IsEmpty() } // SQLUpdate captures either a descriptor or a protected timestamp update. diff --git a/pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go b/pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go index bb24b8722480..885387fc7c12 100644 --- a/pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go +++ b/pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go @@ -433,8 +433,8 @@ func validateUpdateArgs(toDelete []spanconfig.Target, toUpsert []spanconfig.Reco continue } - curSpan := *targets[i].GetSpan() - prevSpan := *targets[i-1].GetSpan() + curSpan := targets[i].GetSpan() + prevSpan := targets[i-1].GetSpan() if curSpan.Overlaps(prevSpan) { return errors.AssertionFailedf("overlapping spans %s and %s in same list", @@ -450,12 +450,11 @@ func validateUpdateArgs(toDelete []spanconfig.Target, toUpsert []spanconfig.Reco // are invalid or have an empty end key. func validateSpanTargets(targets []spanconfig.Target) error { for _, target := range targets { - sp := target.GetSpan() - if sp == nil { + if !target.IsSpanTarget() { // Nothing to do. continue } - if err := validateSpans(*sp); err != nil { + if err := validateSpans(target.GetSpan()); err != nil { return err } } diff --git a/pkg/spanconfig/spanconfigkvaccessor/validation_test.go b/pkg/spanconfig/spanconfigkvaccessor/validation_test.go index 5ce037c62739..8bdf9d26fd5f 100644 --- a/pkg/spanconfig/spanconfigkvaccessor/validation_test.go +++ b/pkg/spanconfig/spanconfigkvaccessor/validation_test.go @@ -36,7 +36,7 @@ func TestValidateUpdateArgs(t *testing.T) { }, { toDelete: []spanconfig.Target{ - spanconfig.MakeSpanTarget( + spanconfig.MakeTargetFromSpan( roachpb.Span{Key: roachpb.Key("a")}, // empty end key in delete list ), }, @@ -45,7 +45,7 @@ func TestValidateUpdateArgs(t *testing.T) { { toUpsert: []spanconfig.Record{ { - Target: spanconfig.MakeSpanTarget( + Target: spanconfig.MakeTargetFromSpan( roachpb.Span{Key: roachpb.Key("a")}, // empty end key in update list ), }, @@ -55,7 +55,7 @@ func TestValidateUpdateArgs(t *testing.T) { { toUpsert: []spanconfig.Record{ { - Target: spanconfig.MakeSpanTarget( + Target: spanconfig.MakeTargetFromSpan( roachpb.Span{Key: roachpb.Key("b"), EndKey: roachpb.Key("a")}, // invalid span; end < start ), }, @@ -64,7 +64,7 @@ func TestValidateUpdateArgs(t *testing.T) { }, { toDelete: []spanconfig.Target{ - spanconfig.MakeSpanTarget( + spanconfig.MakeTargetFromSpan( roachpb.Span{Key: roachpb.Key("b"), EndKey: roachpb.Key("a")}, // invalid span; end < start ), }, @@ -73,20 +73,20 @@ func TestValidateUpdateArgs(t *testing.T) { { toDelete: []spanconfig.Target{ // overlapping spans in the same list. - spanconfig.MakeSpanTarget(roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("c")}), - spanconfig.MakeSpanTarget(roachpb.Span{Key: roachpb.Key("b"), EndKey: roachpb.Key("c")}), + spanconfig.MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("c")}), + spanconfig.MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("b"), EndKey: roachpb.Key("c")}), }, expErr: "overlapping spans {a-c} and {b-c} in same list", }, { toUpsert: []spanconfig.Record{ // overlapping spans in the same list { - Target: spanconfig.MakeSpanTarget( + Target: spanconfig.MakeTargetFromSpan( roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("c")}, ), }, { - Target: spanconfig.MakeSpanTarget( + Target: spanconfig.MakeTargetFromSpan( roachpb.Span{Key: roachpb.Key("b"), EndKey: roachpb.Key("c")}, ), }, @@ -97,16 +97,16 @@ func TestValidateUpdateArgs(t *testing.T) { // Overlapping spans in different lists. toDelete: []spanconfig.Target{ // Overlapping spans in the same list. - spanconfig.MakeSpanTarget(roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("c")}), + spanconfig.MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("c")}), }, toUpsert: []spanconfig.Record{ { - Target: spanconfig.MakeSpanTarget( + Target: spanconfig.MakeTargetFromSpan( roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("b")}, ), }, { - Target: spanconfig.MakeSpanTarget( + Target: spanconfig.MakeTargetFromSpan( roachpb.Span{Key: roachpb.Key("b"), EndKey: roachpb.Key("c")}, ), }, diff --git a/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go b/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go index e8c5292e3bad..d08ba9c8638c 100644 --- a/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go +++ b/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go @@ -253,9 +253,9 @@ func (s *KVSubscriber) handlePartialUpdate( // TODO(arul): In the future, once we start reacting to system span // configurations, we'll want to invoke handlers with the correct span // here as well. - sp := ev.(*bufferEvent).Update.Target.GetSpan() - if sp != nil { - h.invoke(ctx, *sp) + target := ev.(*bufferEvent).Update.Target + if target.IsSpanTarget() { + h.invoke(ctx, target.GetSpan()) } } } diff --git a/pkg/spanconfig/spanconfigkvsubscriber/span_config_decoder_test.go b/pkg/spanconfig/spanconfigkvsubscriber/span_config_decoder_test.go index b8e4169ad080..8fd161968c9b 100644 --- a/pkg/spanconfig/spanconfigkvsubscriber/span_config_decoder_test.go +++ b/pkg/spanconfig/spanconfigkvsubscriber/span_config_decoder_test.go @@ -80,8 +80,8 @@ func TestSpanConfigDecoder(t *testing.T) { }, ) require.NoError(t, err) - require.Truef(t, span.Equal(*got.Target.GetSpan()), - "expected span=%s, got span=%s", span, *got.Target.GetSpan()) + require.Truef(t, span.Equal(got.Target.GetSpan()), + "expected span=%s, got span=%s", span, got.Target.GetSpan()) require.Truef(t, conf.Equal(got.Config), "expected config=%s, got config=%s", conf, got.Config) } diff --git a/pkg/spanconfig/spanconfigreconciler/reconciler.go b/pkg/spanconfig/spanconfigreconciler/reconciler.go index 0ada0176549f..ac53c2975924 100644 --- a/pkg/spanconfig/spanconfigreconciler/reconciler.go +++ b/pkg/spanconfig/spanconfigreconciler/reconciler.go @@ -270,7 +270,7 @@ func (f *fullReconciler) reconcile( func (f *fullReconciler) fetchExistingSpanConfigs( ctx context.Context, ) (*spanconfigstore.Store, error) { - var target spanconfig.Target + var targets []spanconfig.Target if f.codec.ForSystemTenant() { // The system tenant governs all system keys (meta, liveness, timeseries // ranges, etc.) and system tenant tables. @@ -278,28 +278,36 @@ func (f *fullReconciler) fetchExistingSpanConfigs( // TODO(irfansharif): Should we include the scratch range here? Some // tests make use of it; we may want to declare configs over it and have // it considered all the same. - target = spanconfig.MakeSpanTarget(roachpb.Span{ + // + // We don't want to request configs that are part of the + // SystemSpanConfigSpan, as the host tenant reserves that part of the + // keyspace to translate and persist SystemSpanConfigs. At the level of the + // reconciler we shouldn't be requesting these configs directly, instead, + // they should be targeted through SystemSpanConfigTargets instead. + targets = append(targets, spanconfig.MakeTargetFromSpan(roachpb.Span{ Key: keys.EverythingSpan.Key, + EndKey: keys.SystemSpanConfigSpan.Key, + })) + targets = append(targets, spanconfig.MakeTargetFromSpan(roachpb.Span{ + Key: keys.TableDataMin, EndKey: keys.TableDataMax, - }) + })) if f.knobs.ConfigureScratchRange { - target.GetSpan().EndKey = keys.ScratchRangeMax + sp := targets[1].GetSpan() + targets[1] = spanconfig.MakeTargetFromSpan(roachpb.Span{Key: sp.Key, EndKey: keys.ScratchRangeMax}) } } else { // Secondary tenants govern everything prefixed by their tenant ID. tenPrefix := keys.MakeTenantPrefix(f.tenID) - target = spanconfig.MakeSpanTarget(roachpb.Span{ + targets = append(targets, spanconfig.MakeTargetFromSpan(roachpb.Span{ Key: tenPrefix, EndKey: tenPrefix.PrefixEnd(), - }) + })) } - store := spanconfigstore.New(roachpb.SpanConfig{}) { // Fully populate the store with KVAccessor contents. - records, err := f.kvAccessor.GetSpanConfigRecords(ctx, []spanconfig.Target{ - target, - }) + records, err := f.kvAccessor.GetSpanConfigRecords(ctx, targets) if err != nil { return nil, err } @@ -398,7 +406,7 @@ func (r *incrementalReconciler) reconcile( Key: r.codec.TablePrefix(uint32(missingID)), EndKey: r.codec.TablePrefix(uint32(missingID)).PrefixEnd(), } - updates = append(updates, spanconfig.Deletion(spanconfig.MakeSpanTarget(tableSpan))) + updates = append(updates, spanconfig.Deletion(spanconfig.MakeTargetFromSpan(tableSpan))) } toDelete, toUpsert := r.storeWithKVContents.Apply(ctx, false /* dryrun */, updates...) diff --git a/pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go b/pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go index 399372d90cd9..2918547edb92 100644 --- a/pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go +++ b/pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go @@ -209,6 +209,10 @@ func (s *SQLTranslator) generateSpanConfigurationsForNamedZone( // Add spans for the system range without the timeseries and // liveness ranges, which are individually captured above. // + // We also don't apply configurations over the SystemSpanConfigSpan; spans + // carved from this range have no data, instead, associated configurations + // for these spans have special meaning in `system.span_configurations`. + // // Note that the NodeLivenessSpan sorts before the rest of the system // keyspace, so the first span here starts at the end of the // NodeLivenessSpan. @@ -218,7 +222,7 @@ func (s *SQLTranslator) generateSpanConfigurationsForNamedZone( }) spans = append(spans, roachpb.Span{ Key: keys.TimeseriesSpan.EndKey, - EndKey: keys.SystemMax, + EndKey: keys.SystemSpanConfigSpan.Key, }) case zonepb.TenantsZoneName: // nothing to do. default: @@ -233,7 +237,7 @@ func (s *SQLTranslator) generateSpanConfigurationsForNamedZone( var records []spanconfig.Record for _, span := range spans { records = append(records, spanconfig.Record{ - Target: spanconfig.MakeSpanTarget(span), + Target: spanconfig.MakeTargetFromSpan(span), Config: spanConfig, }) } @@ -294,7 +298,7 @@ func (s *SQLTranslator) generateSpanConfigurationsForTable( // but looking at range boundaries, it's slightly less confusing // this way. records = append(records, spanconfig.Record{ - Target: spanconfig.MakeSpanTarget(roachpb.Span{ + Target: spanconfig.MakeTargetFromSpan(roachpb.Span{ Key: s.codec.TenantPrefix(), EndKey: tableEndKey, }), @@ -311,7 +315,7 @@ func (s *SQLTranslator) generateSpanConfigurationsForTable( // (tiny) re-splitting costs when switching between the two // subsystems. records = append(records, spanconfig.Record{ - Target: spanconfig.MakeSpanTarget(roachpb.Span{ + Target: spanconfig.MakeTargetFromSpan(roachpb.Span{ Key: keys.SystemConfigSpan.Key, EndKey: tableEndKey, }), @@ -375,7 +379,7 @@ func (s *SQLTranslator) generateSpanConfigurationsForTable( if !prevEndKey.Equal(span.Key) { records = append(records, spanconfig.Record{ - Target: spanconfig.MakeSpanTarget(roachpb.Span{Key: prevEndKey, EndKey: span.Key}), + Target: spanconfig.MakeTargetFromSpan(roachpb.Span{Key: prevEndKey, EndKey: span.Key}), Config: tableSpanConfig, }, ) @@ -393,7 +397,7 @@ func (s *SQLTranslator) generateSpanConfigurationsForTable( } records = append(records, spanconfig.Record{ - Target: spanconfig.MakeSpanTarget(roachpb.Span{Key: span.Key, EndKey: span.EndKey}), + Target: spanconfig.MakeTargetFromSpan(roachpb.Span{Key: span.Key, EndKey: span.EndKey}), Config: subzoneSpanConfig, }, ) @@ -406,7 +410,7 @@ func (s *SQLTranslator) generateSpanConfigurationsForTable( if !prevEndKey.Equal(tableEndKey) { records = append(records, spanconfig.Record{ - Target: spanconfig.MakeSpanTarget(roachpb.Span{Key: prevEndKey, EndKey: tableEndKey}), + Target: spanconfig.MakeTargetFromSpan(roachpb.Span{Key: prevEndKey, EndKey: tableEndKey}), Config: tableSpanConfig, }, ) @@ -577,7 +581,7 @@ func (s *SQLTranslator) maybeGeneratePseudoTableRecords( tableStartKey := s.codec.TablePrefix(pseudoTableID) tableEndKey := tableStartKey.PrefixEnd() records = append(records, spanconfig.Record{ - Target: spanconfig.MakeSpanTarget(roachpb.Span{ + Target: spanconfig.MakeTargetFromSpan(roachpb.Span{ Key: tableStartKey, EndKey: tableEndKey, }), @@ -609,7 +613,7 @@ func (s *SQLTranslator) maybeGenerateScratchRangeRecord( } return spanconfig.Record{ - Target: spanconfig.MakeSpanTarget(roachpb.Span{ + Target: spanconfig.MakeTargetFromSpan(roachpb.Span{ Key: keys.ScratchRangeMin, EndKey: keys.ScratchRangeMax, }), diff --git a/pkg/spanconfig/spanconfigstore/spanconfigstore.go b/pkg/spanconfig/spanconfigstore/spanconfigstore.go index af65401cfb1e..2e3af504697f 100644 --- a/pkg/spanconfig/spanconfigstore/spanconfigstore.go +++ b/pkg/spanconfig/spanconfigstore/spanconfigstore.go @@ -44,7 +44,7 @@ func (s *spanConfigStore) copy(ctx context.Context) *spanConfigStore { clone := newSpanConfigStore() _ = s.forEachOverlapping(keys.EverythingSpan, func(entry spanConfigEntry) error { _, _, err := clone.apply(false /* dryrun */, spanconfig.Update{ - Target: spanconfig.MakeSpanTarget(entry.span), + Target: spanconfig.MakeTargetFromSpan(entry.span), Config: entry.config, }) if err != nil { @@ -298,15 +298,15 @@ func (s *spanConfigStore) accumulateOpsFor( } var ( - union = existing.span.Combine(*update.Target.GetSpan()) - inter = existing.span.Intersect(*update.Target.GetSpan()) + union = existing.span.Combine(update.Target.GetSpan()) + inter = existing.span.Intersect(update.Target.GetSpan()) pre = roachpb.Span{Key: union.Key, EndKey: inter.Key} post = roachpb.Span{Key: inter.EndKey, EndKey: union.EndKey} ) if update.Addition() { - if existing.span.Equal(*update.Target.GetSpan()) && existing.config.Equal(update.Config) { + if existing.span.Equal(update.Target.GetSpan()) && existing.config.Equal(update.Config) { skipAddingSelf = true break // no-op; peep-hole optimization } @@ -349,7 +349,7 @@ func (s *spanConfigStore) accumulateOpsFor( if update.Addition() && !skipAddingSelf { // Add the update itself. - toAdd = append(toAdd, s.makeEntry(*update.Target.GetSpan(), update.Config)) + toAdd = append(toAdd, s.makeEntry(update.Target.GetSpan(), update.Config)) // TODO(irfansharif): If we're adding an entry, we could inspect the // entries before and after and check whether either of them have @@ -385,11 +385,11 @@ func (s *spanConfigStore) makeEntry(sp roachpb.Span, conf roachpb.SpanConfig) sp // spans, those spans be valid, and non-overlapping. func validateApplyArgs(updates ...spanconfig.Update) error { for i := range updates { - sp := updates[i].Target.GetSpan() - - if sp == nil { + if !updates[i].Target.IsSpanTarget() { return errors.New("expected update to target a span") } + + sp := updates[i].Target.GetSpan() if !sp.Valid() || len(sp.EndKey) == 0 { return errors.New("invalid span") } @@ -406,11 +406,11 @@ func validateApplyArgs(updates ...spanconfig.Update) error { if i == 0 { continue } - if updates[i].Target.GetSpan().Overlaps(*updates[i-1].Target.GetSpan()) { + if updates[i].Target.GetSpan().Overlaps(updates[i-1].Target.GetSpan()) { return errors.Newf( "found overlapping updates %s and %s", - *updates[i-1].Target.GetSpan(), - *updates[i].Target.GetSpan(), + updates[i-1].Target.GetSpan(), + updates[i].Target.GetSpan(), ) } } diff --git a/pkg/spanconfig/spanconfigstore/spanconfigstore_test.go b/pkg/spanconfig/spanconfigstore/spanconfigstore_test.go index 1cc15adaf516..0c285477c60f 100644 --- a/pkg/spanconfig/spanconfigstore/spanconfigstore_test.go +++ b/pkg/spanconfig/spanconfigstore/spanconfigstore_test.go @@ -65,9 +65,9 @@ func TestRandomized(t *testing.T) { sp, conf, op := genRandomSpan(), getRandomConf(), getRandomOp() switch op { case "set": - return spanconfig.Addition(spanconfig.MakeSpanTarget(sp), conf) + return spanconfig.Addition(spanconfig.MakeTargetFromSpan(sp), conf) case "del": - return spanconfig.Deletion(spanconfig.MakeSpanTarget(sp)) + return spanconfig.Deletion(spanconfig.MakeTargetFromSpan(sp)) default: } t.Fatalf("unexpected op: %s", op) @@ -86,7 +86,7 @@ func TestRandomized(t *testing.T) { }) invalid := false for i := 1; i < numUpdates; i++ { - if updates[i].Target.GetSpan().Overlaps(*updates[i-1].Target.GetSpan()) { + if updates[i].Target.GetSpan().Overlaps(updates[i-1].Target.GetSpan()) { invalid = true } } @@ -113,7 +113,7 @@ func TestRandomized(t *testing.T) { _, _, err := store.apply(false /* dryrun */, updates...) require.NoError(t, err) for _, update := range updates { - if testSpan.Overlaps(*update.Target.GetSpan()) { + if testSpan.Overlaps(update.Target.GetSpan()) { if update.Addition() { expConfig, expFound = update.Config, true } else { @@ -128,7 +128,7 @@ func TestRandomized(t *testing.T) { func(entry spanConfigEntry) error { t.Fatalf("found unexpected entry: %s", spanconfigtestutils.PrintSpanConfigRecord(spanconfig.Record{ - Target: spanconfig.MakeSpanTarget(entry.span), + Target: spanconfig.MakeTargetFromSpan(entry.span), Config: entry.config, })) return nil @@ -141,7 +141,7 @@ func TestRandomized(t *testing.T) { if !foundEntry.isEmpty() { t.Fatalf("expected single overlapping entry, found second: %s", spanconfigtestutils.PrintSpanConfigRecord(spanconfig.Record{ - Target: spanconfig.MakeSpanTarget(entry.span), + Target: spanconfig.MakeTargetFromSpan(entry.span), Config: entry.config, })) } diff --git a/pkg/spanconfig/spanconfigstore/store.go b/pkg/spanconfig/spanconfigstore/store.go index c3e803bb2b46..41004fb5a0e7 100644 --- a/pkg/spanconfig/spanconfigstore/store.go +++ b/pkg/spanconfig/spanconfigstore/store.go @@ -129,7 +129,7 @@ func (s *Store) applyInternal( spanStoreUpdates := make([]spanconfig.Update, 0, len(updates)) for _, update := range updates { // TODO(arul): We'll hijack system span configurations here. - if update.Target.GetSpan() != nil { + if update.Target.IsSpanTarget() { spanStoreUpdates = append(spanStoreUpdates, update) } } @@ -139,12 +139,12 @@ func (s *Store) applyInternal( } for _, sp := range deletedSpans { - deleted = append(deleted, spanconfig.MakeSpanTarget(sp)) + deleted = append(deleted, spanconfig.MakeTargetFromSpan(sp)) } for _, entry := range addedEntries { added = append(added, spanconfig.Record{ - Target: spanconfig.MakeSpanTarget(entry.span), + Target: spanconfig.MakeTargetFromSpan(entry.span), Config: entry.config, }) } @@ -159,7 +159,7 @@ func (s *Store) Iterate(f func(spanconfig.Record) error) error { keys.EverythingSpan, func(s spanConfigEntry) error { return f(spanconfig.Record{ - Target: spanconfig.MakeSpanTarget(s.span), + Target: spanconfig.MakeTargetFromSpan(s.span), Config: s.config, }) }) diff --git a/pkg/spanconfig/spanconfigstore/store_test.go b/pkg/spanconfig/spanconfigstore/store_test.go index b083ba49823f..5b11e22d2629 100644 --- a/pkg/spanconfig/spanconfigstore/store_test.go +++ b/pkg/spanconfig/spanconfigstore/store_test.go @@ -134,7 +134,7 @@ func TestDataDriven(t *testing.T) { func(entry spanConfigEntry) error { results = append(results, spanconfigtestutils.PrintSpanConfigRecord(spanconfig.Record{ - Target: spanconfig.MakeSpanTarget(entry.span), + Target: spanconfig.MakeTargetFromSpan(entry.span), Config: entry.config, }), ) @@ -161,15 +161,15 @@ func TestStoreClone(t *testing.T) { updates := []spanconfig.Update{ spanconfig.Addition( - spanconfig.MakeSpanTarget(spanconfigtestutils.ParseSpan(t, "[a, b)")), + spanconfig.MakeTargetFromSpan(spanconfigtestutils.ParseSpan(t, "[a, b)")), spanconfigtestutils.ParseConfig(t, "A"), ), spanconfig.Addition( - spanconfig.MakeSpanTarget(spanconfigtestutils.ParseSpan(t, "[c, d)")), + spanconfig.MakeTargetFromSpan(spanconfigtestutils.ParseSpan(t, "[c, d)")), spanconfigtestutils.ParseConfig(t, "C"), ), spanconfig.Addition( - spanconfig.MakeSpanTarget(spanconfigtestutils.ParseSpan(t, "[e, f)")), + spanconfig.MakeTargetFromSpan(spanconfigtestutils.ParseSpan(t, "[e, f)")), spanconfigtestutils.ParseConfig(t, "E"), ), } diff --git a/pkg/spanconfig/spanconfigtestutils/utils.go b/pkg/spanconfig/spanconfigtestutils/utils.go index 912f741f5e90..be3445a3475c 100644 --- a/pkg/spanconfig/spanconfigtestutils/utils.go +++ b/pkg/spanconfig/spanconfigtestutils/utils.go @@ -56,7 +56,7 @@ func ParseSpan(t *testing.T, sp string) roachpb.Span { // TODO(arul): Once we have system targets, we'll want to parse them here too // instead of just calling ParseSpan here. func ParseTarget(t *testing.T, target string) spanconfig.Target { - return spanconfig.MakeSpanTarget(ParseSpan(t, target)) + return spanconfig.MakeTargetFromSpan(ParseSpan(t, target)) } // ParseConfig is helper function that constructs a roachpb.SpanConfig that's @@ -194,10 +194,10 @@ func PrintSpan(sp roachpb.Span) string { // PrintTarget is a helper function that prints a spanconfig.Target. func PrintTarget(target spanconfig.Target) string { - if target.GetSpan() != nil { - return PrintSpan(*target.GetSpan()) + if target.IsSpanTarget() { + return PrintSpan(target.GetSpan()) } - panic("targets other than span targets are unsupported") + panic("targets other than span targets are unsupported for now") } // PrintSpanConfig is a helper function that transforms roachpb.SpanConfig into diff --git a/pkg/spanconfig/systemtarget.go b/pkg/spanconfig/systemtarget.go new file mode 100644 index 000000000000..ce398af99944 --- /dev/null +++ b/pkg/spanconfig/systemtarget.go @@ -0,0 +1,189 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package spanconfig + +import ( + "bytes" + "fmt" + + "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/util/encoding" + "github.com/cockroachdb/errors" +) + +// SystemTarget specifies the target of a system span configuration. +type SystemTarget struct { + // 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 + + // 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). + // + // 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 +} + +// MakeSystemTarget constructs, validates, and returns a new SystemTarget. +func MakeSystemTarget( + sourceTenantID roachpb.TenantID, targetTenantID *roachpb.TenantID, +) (SystemTarget, error) { + t := SystemTarget{ + SourceTenantID: sourceTenantID, + TargetTenantID: targetTenantID, + } + return t, t.validate() +} + +// 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 +} + +// encode returns an encoded span associated with the receiver which is suitable +// for persistence in system.span_configurations. +func (st SystemTarget) encode() roachpb.Span { + var k roachpb.Key + + if st.SourceTenantID == roachpb.SystemTenantID && + st.TargetTenantID == nil { + k = keys.SystemSpanConfigEntireKeyspace + } else if st.SourceTenantID == roachpb.SystemTenantID { + k = encoding.EncodeUvarintAscending( + keys.SystemSpanConfigHostOnTenantKeyspace, st.TargetTenantID.InternalValue, + ) + } else { + k = encoding.EncodeUvarintAscending( + keys.SystemSpanConfigSecondaryTenantOnEntireKeyspace, st.SourceTenantID.InternalValue, + ) + } + 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, + ) + } + 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) +} + +// 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. +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 + } + + return st.TargetTenantID.InternalValue < ot.TargetTenantID.InternalValue + } + + if st.SourceTenantID == roachpb.SystemTenantID { + return true + } else if ot.SourceTenantID == roachpb.SystemTenantID { + return false + } + + return st.SourceTenantID.InternalValue < ot.SourceTenantID.InternalValue +} + +// equal returns true iff the receiver is equal to the supplied system target. +func (st SystemTarget) equal(ot SystemTarget) bool { + return st.SourceTenantID == ot.SourceTenantID && st.TargetTenantID == ot.TargetTenantID +} + +// String returns a pretty printed version of a system target. +func (st SystemTarget) String() string { + return fmt.Sprintf("{system target source: %s target: %s}", st.SourceTenantID, st.TargetTenantID) +} + +// 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. +func decodeSystemTarget(span roachpb.Span) (SystemTarget, error) { + // Validate the end key is well-formed. + if !span.EndKey.Equal(span.Key.PrefixEnd()) { + return SystemTarget{}, errors.AssertionFailedf("invalid end key in span %s", span) + } + switch { + case bytes.Equal(span.Key, keys.SystemSpanConfigEntireKeyspace): + return MakeSystemTarget(roachpb.SystemTenantID, nil /* targetTenantID */) + case bytes.HasPrefix(span.Key, keys.SystemSpanConfigHostOnTenantKeyspace): + // System span config was applied by the host tenant over a secondary + // tenant's entire keyspace. + tenIDBytes := span.Key[len(keys.SystemSpanConfigHostOnTenantKeyspace):] + _, tenIDRaw, err := encoding.DecodeUvarintAscending(tenIDBytes) + if err != nil { + return SystemTarget{}, err + } + tenID := roachpb.MakeTenantID(tenIDRaw) + return MakeSystemTarget(roachpb.SystemTenantID, &tenID) + case bytes.HasPrefix(span.Key, keys.SystemSpanConfigSecondaryTenantOnEntireKeyspace): + // System span config was applied by a secondary tenant over its entire + // keyspace. + tenIDBytes := span.Key[len(keys.SystemSpanConfigSecondaryTenantOnEntireKeyspace):] + _, tenIDRaw, err := encoding.DecodeUvarintAscending(tenIDBytes) + if err != nil { + return SystemTarget{}, err + } + tenID := roachpb.MakeTenantID(tenIDRaw) + return MakeSystemTarget(tenID, &tenID) + default: + return SystemTarget{}, + errors.AssertionFailedf("span %s did not conform to SystemTarget encoding", span) + } +} + +// spanStartKeyConformsToSystemTargetEncoding returns true if the given span's +// start key conforms to the key encoding of system span configurations. +func spanStartKeyConformsToSystemTargetEncoding(span roachpb.Span) bool { + return bytes.Equal(span.Key, keys.SystemSpanConfigEntireKeyspace) || + bytes.HasPrefix(span.Key, keys.SystemSpanConfigHostOnTenantKeyspace) || + bytes.HasPrefix(span.Key, keys.SystemSpanConfigSecondaryTenantOnEntireKeyspace) +} diff --git a/pkg/spanconfig/target.go b/pkg/spanconfig/target.go index 61878807471a..192fb9e14ebb 100644 --- a/pkg/spanconfig/target.go +++ b/pkg/spanconfig/target.go @@ -13,81 +13,162 @@ package spanconfig import "github.com/cockroachdb/cockroach/pkg/roachpb" // Target specifies the target of an associated span configuration. -// -// TODO(arul): In the future, we will expand this to include system targets. type Target struct { span roachpb.Span + + systemTarget SystemTarget } // MakeTarget returns a new Target. func MakeTarget(t roachpb.SpanConfigTarget) Target { switch t.Union.(type) { case *roachpb.SpanConfigTarget_Span: - return MakeSpanTarget(*t.GetSpan()) + return MakeTargetFromSpan(*t.GetSpan()) + // TODO(arul): Add a case here for SpanConfigTarget_SystemTarget once we've + // taught and tested the KVAccessor to work with system targets. default: panic("cannot handle target") } } -// MakeSpanTarget constructs and returns a span target. -func MakeSpanTarget(span roachpb.Span) Target { +// MakeTargetFromSpan constructs and returns a span target. +func MakeTargetFromSpan(span roachpb.Span) Target { return Target{span: span} } -// GetSpan returns the underlying roachpb.Span if the target is a span target -// and nil otherwise. -func (t *Target) GetSpan() *roachpb.Span { - if t.span.Equal(roachpb.Span{}) { - return nil +// MakeTargetFromSystemTarget returns a Target which wraps a system target. +func MakeTargetFromSystemTarget(systemTarget SystemTarget) Target { + return Target{systemTarget: systemTarget} +} + +// IsSpanTarget returns true if the target is a span target. +func (t Target) IsSpanTarget() bool { + return !t.span.Equal(roachpb.Span{}) +} + +// GetSpan returns the underlying roachpb.Span if the target is a span +// target; panics if that isn't he case. +func (t Target) GetSpan() roachpb.Span { + if !t.IsSpanTarget() { + panic("target is not a span target") + } + return t.span +} + +// IsSystemTarget returns true if the underlying target is a system target. +func (t Target) IsSystemTarget() bool { + return !t.systemTarget.isEmpty() +} + +// GetSystemTarget returns the underlying SystemTarget; it panics if that is not +// the case. +func (t Target) GetSystemTarget() SystemTarget { + if !t.IsSystemTarget() { + panic("target is not a system target") } - return &t.span + return t.systemTarget } // Encode returns an encoded span suitable for persistence in // system.span_configurations. func (t Target) Encode() roachpb.Span { - if t.GetSpan() != nil { + switch { + case t.IsSpanTarget(): return t.span + case t.IsSystemTarget(): + return t.systemTarget.encode() + default: + panic("cannot handle any other type of target") } - panic("cannot handle any other type of target") } -// Less returns true if the receiver is less than the supplied target. -func (t *Target) Less(o Target) bool { - return t.Encode().Key.Compare(o.Encode().Key) < 0 +// Less returns true if the receiver is considered less than the supplied +// target. +func (t Target) Less(o Target) bool { + // We consider system targets to be less than span targets. + + // If both targets are system targets delegate to the base type. + if t.IsSystemTarget() && o.IsSystemTarget() { + return t.GetSystemTarget().less(o.GetSystemTarget()) + } + + // Check if one of the targets is a system target and return accordingly. + if t.IsSystemTarget() { + return true + } else if o.IsSystemTarget() { + return false + } + + // We're dealing with 2 span targets; compare their start keys. + if !t.GetSpan().Key.Equal(o.GetSpan().Key) { + return t.GetSpan().Key.Compare(o.GetSpan().Key) < 0 + } + // If the start keys are equal, compare their end keys. + return t.GetSpan().EndKey.Compare(o.GetSpan().EndKey) < 0 } // Equal returns true iff the receiver is equal to the supplied target. -func (t *Target) Equal(o Target) bool { - return t.GetSpan().Equal(*o.GetSpan()) +func (t Target) Equal(o Target) bool { + if t.IsSpanTarget() && o.IsSpanTarget() { + return t.GetSpan().Equal(o.GetSpan()) + } + + if t.IsSystemTarget() && o.IsSystemTarget() { + return t.GetSystemTarget().equal(o.GetSystemTarget()) + } + + // We're dealing with one span target and one system target, so they're not + // equal. + return false } // String returns a formatted version of the traget suitable for printing. func (t Target) String() string { - return t.GetSpan().String() + if t.IsSpanTarget() { + return t.GetSpan().String() + } + return t.GetSystemTarget().String() } -// IsEmpty returns true if the receiver is an empty target. -func (t Target) IsEmpty() bool { - return t.GetSpan().Equal(roachpb.Span{}) - +// isEmpty returns true if the receiver is an empty target. +func (t Target) isEmpty() bool { + return t.systemTarget.isEmpty() && t.span.Equal(roachpb.Span{}) } -func (t Target) SpanConfigTargetProto() roachpb.SpanConfigTarget { - if t.GetSpan() != nil { +// ToProto returns a roachpb.SpanConfigTarget equivalent to the receiver. +func (t Target) ToProto() roachpb.SpanConfigTarget { + switch { + case t.IsSpanTarget(): + sp := t.GetSpan() return roachpb.SpanConfigTarget{ Union: &roachpb.SpanConfigTarget_Span{ - Span: t.GetSpan(), + Span: &sp, }, } + case t.IsSystemTarget(): + return roachpb.SpanConfigTarget{ + Union: &roachpb.SpanConfigTarget_SystemSpanConfigTarget{ + SystemSpanConfigTarget: &roachpb.SystemSpanConfigTarget{ + SourceTenantID: t.GetSystemTarget().SourceTenantID, + TargetTenantID: t.GetSystemTarget().TargetTenantID, + }, + }, + } + default: + panic("cannot handle any other type of target") } - - panic("cannot handle any other type of target") } // DecodeTarget takes a raw span and decodes it into a Target given its // encoding. It is the inverse of Encode. func DecodeTarget(span roachpb.Span) Target { + if spanStartKeyConformsToSystemTargetEncoding(span) { + systemTarget, err := decodeSystemTarget(span) + if err != nil { + panic(err) + } + return Target{systemTarget: systemTarget} + } return Target{span: span} } @@ -98,20 +179,22 @@ type Targets []Target func (t Targets) Len() int { return len(t) } // Swap implements sort.Interface. -func (t Targets) Swap(i, j int) { t[i], t[j] = t[j], t[i] } +func (t Targets) Swap(i, j int) { + t[i], t[j] = t[j], t[i] +} // Less implements Sort.Interface. func (t Targets) Less(i, j int) bool { return t[i].Less(t[j]) } -// RecordsToSpanConfigEntries converts a list of records to a list -// roachpb.SpanConfigEntry protos suitable for sending over the wire. -func RecordsToSpanConfigEntries(records []Record) []roachpb.SpanConfigEntry { +// RecordsToEntries converts a list of records to a list roachpb.SpanConfigEntry +// protos suitable for sending over the wire. +func RecordsToEntries(records []Record) []roachpb.SpanConfigEntry { entries := make([]roachpb.SpanConfigEntry, 0, len(records)) for _, rec := range records { entries = append(entries, roachpb.SpanConfigEntry{ - Target: rec.Target.SpanConfigTargetProto(), + Target: rec.Target.ToProto(), Config: rec.Config, }) } @@ -131,19 +214,19 @@ func EntriesToRecords(entries []roachpb.SpanConfigEntry) []Record { return records } -// TargetsToTargetProtos converts a list of targets to a list of +// TargetsToProtos converts a list of targets to a list of // roachpb.SpanConfigTarget protos suitable for sending over the wire. -func TargetsToTargetProtos(targets []Target) []roachpb.SpanConfigTarget { +func TargetsToProtos(targets []Target) []roachpb.SpanConfigTarget { targetProtos := make([]roachpb.SpanConfigTarget, 0, len(targets)) for _, target := range targets { - targetProtos = append(targetProtos, target.SpanConfigTargetProto()) + targetProtos = append(targetProtos, target.ToProto()) } return targetProtos } -// TargetProtosToTargets converts a list of roachpb.SpanConfigTargets +// TargetsFromProtos converts a list of roachpb.SpanConfigTargets // (received over the wire) to a list of Targets. -func TargetProtosToTargets(protoTargtets []roachpb.SpanConfigTarget) []Target { +func TargetsFromProtos(protoTargtets []roachpb.SpanConfigTarget) []Target { targets := make([]Target, 0, len(protoTargtets)) for _, t := range protoTargtets { targets = append(targets, MakeTarget(t)) diff --git a/pkg/spanconfig/target_test.go b/pkg/spanconfig/target_test.go new file mode 100644 index 000000000000..c2fd7f165ece --- /dev/null +++ b/pkg/spanconfig/target_test.go @@ -0,0 +1,172 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package spanconfig + +import ( + "math/rand" + "sort" + "testing" + + "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/testutils" + "github.com/stretchr/testify/require" +) + +// TestEncodeDecodeSystemTarget ensures that encoding/decoding a SystemTarget +// is roundtripable. +func TestEncodeDecodeSystemTarget(t *testing.T) { + tenant10 := roachpb.MakeTenantID(10) + for _, testTarget := range []SystemTarget{ + // Tenant targeting its logical cluster. + makeSystemTargetOrFatal(t, tenant10, &tenant10), + // System tenant targeting its logical cluster. + makeSystemTargetOrFatal(t, roachpb.SystemTenantID, &roachpb.SystemTenantID), + // System tenant targeting a secondary tenant. + makeSystemTargetOrFatal(t, roachpb.SystemTenantID, &tenant10), + // System tenant targeting the entire cluster. + makeSystemTargetOrFatal(t, roachpb.SystemTenantID, nil /* targetID */), + } { + systemTarget, err := decodeSystemTarget(testTarget.encode()) + require.NoError(t, err) + require.Equal(t, testTarget, systemTarget) + + // Next, we encode/decode a spanconfig.Target that wraps a SystemTarget. + target := MakeTargetFromSystemTarget(systemTarget) + decodedTarget := DecodeTarget(target.Encode()) + require.Equal(t, target, decodedTarget) + } +} + +// TestDecodeInvalidSpanAsSystemTarget ensures that decoding an invalid span +// as a system target fails. +func TestDecodeInvalidSpanAsSystemTarget(t *testing.T) { + for _, tc := range []struct { + span roachpb.Span + expectedErr string + }{ + { + span: roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("b")}, + expectedErr: "span .* did not conform to SystemTarget encoding", + }, + { + // No end key. + span: roachpb.Span{Key: keys.SystemSpanConfigEntireKeyspace}, + expectedErr: "invalid end key in span", + }, + { + // Invalid end key. + span: roachpb.Span{ + Key: keys.SystemSpanConfigEntireKeyspace, + EndKey: append(keys.SystemSpanConfigEntireKeyspace, byte('a')).PrefixEnd(), + }, + expectedErr: "invalid end key in span", + }, + { + // Sentinel key for SystemSpanConfigEntireKeyspace should not have a + // suffix. + span: roachpb.Span{ + Key: append(keys.SystemSpanConfigEntireKeyspace, byte('a')), + EndKey: append(keys.SystemSpanConfigEntireKeyspace, byte('a')).PrefixEnd(), + }, + expectedErr: "span .* did not conform to SystemTarget encoding", + }, + } { + _, err := decodeSystemTarget(tc.span) + require.Error(t, err) + require.True(t, testutils.IsError(err, tc.expectedErr)) + } +} + +// TestSystemTargetValidation ensures target.validate() works as expected. +func TestSystemTargetValidation(t *testing.T) { + for _, tc := range []struct { + sourceTenantID roachpb.TenantID + targetTenantID roachpb.TenantID + 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", + }, + { + // 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", + }, + // Test some valid targets. + { + // System tenant targeting secondary tenant is allowed. + sourceTenantID: roachpb.SystemTenantID, + targetTenantID: roachpb.MakeTenantID(20), + }, + { + // System tenant targeting itself is allowed. + sourceTenantID: roachpb.SystemTenantID, + targetTenantID: roachpb.SystemTenantID, + }, + { + // Secondary tenant targeting itself is allowed. + sourceTenantID: roachpb.MakeTenantID(10), + targetTenantID: roachpb.MakeTenantID(10), + }, + } { + target := SystemTarget{ + SourceTenantID: tc.sourceTenantID, + TargetTenantID: &tc.targetTenantID, + } + require.True(t, testutils.IsError(target.validate(), tc.expErr)) + } +} + +// TestTargetSortingRandomized ensures we sort targets correctly. +func TestTargetSortingRandomized(t *testing.T) { + tenant10 := roachpb.MakeTenantID(10) + tenant20 := roachpb.MakeTenantID(20) + tenant5 := roachpb.MakeTenantID(5) + + // Construct a set of sorted targets. + sortedTargets := Targets{ + MakeTargetFromSystemTarget(makeSystemTargetOrFatal(t, roachpb.SystemTenantID, nil /*targetID*/)), + MakeTargetFromSystemTarget(makeSystemTargetOrFatal(t, roachpb.SystemTenantID, &roachpb.SystemTenantID)), + MakeTargetFromSystemTarget(makeSystemTargetOrFatal(t, roachpb.SystemTenantID, &tenant10)), + MakeTargetFromSystemTarget(makeSystemTargetOrFatal(t, roachpb.SystemTenantID, &tenant20)), + MakeTargetFromSystemTarget(makeSystemTargetOrFatal(t, roachpb.MakeTenantID(5), &tenant5)), + MakeTargetFromSystemTarget(makeSystemTargetOrFatal(t, roachpb.MakeTenantID(10), &tenant10)), + MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("b")}), + MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("d")}), + MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("y"), EndKey: roachpb.Key("z")}), + } + + const numOps = 20 + for i := 0; i < numOps; i++ { + tc := make(Targets, len(sortedTargets)) + copy(tc, sortedTargets) + + rand.Shuffle(len(tc), func(i, j int) { + tc[i], tc[j] = tc[j], tc[i] + }) + + sort.Sort(tc) + require.Equal(t, sortedTargets, tc) + } +} + +func makeSystemTargetOrFatal( + t *testing.T, sourceID roachpb.TenantID, targetID *roachpb.TenantID, +) SystemTarget { + target, err := MakeSystemTarget(sourceID, targetID) + require.NoError(t, err) + return target +} diff --git a/pkg/sql/tenant.go b/pkg/sql/tenant.go index 50665f86f294..c3a10d330564 100644 --- a/pkg/sql/tenant.go +++ b/pkg/sql/tenant.go @@ -163,7 +163,7 @@ func CreateTenantRecord( tenantPrefix := keys.MakeTenantPrefix(roachpb.MakeTenantID(tenID)) toUpsert := []spanconfig.Record{ { - Target: spanconfig.MakeSpanTarget(roachpb.Span{ + Target: spanconfig.MakeTargetFromSpan(roachpb.Span{ Key: tenantPrefix, EndKey: tenantPrefix.Next(), }), @@ -461,7 +461,7 @@ func GCTenantSync(ctx context.Context, execCfg *ExecutorConfig, info *descpb.Ten scKVAccessor := execCfg.SpanConfigKVAccessor.WithTxn(ctx, txn) records, err := scKVAccessor.GetSpanConfigRecords( - ctx, []spanconfig.Target{spanconfig.MakeSpanTarget(tenantSpan)}, + ctx, []spanconfig.Target{spanconfig.MakeTargetFromSpan(tenantSpan)}, ) if err != nil { return err From c7b7c26acc08fb2de86d9bb49008a1d81f277e5d Mon Sep 17 00:00:00 2001 From: arulajmani Date: Mon, 14 Feb 2022 18:15:08 -0500 Subject: [PATCH 3/3] spanconfig: remove panic from code around targets Changed to return an error to the caller when dealing with proto to target conversions for protos received over the wire. There isn't likely to be an error in this codepath, but if there is, it makes sense to return it up the RPC chain to the tenant as opposed to letting it bring a KV node down. Release note: None --- pkg/ccl/kvccl/kvtenantccl/connector.go | 5 +++- pkg/server/node.go | 19 +++++++++----- pkg/spanconfig/target.go | 35 +++++++++++++++++--------- 3 files changed, 40 insertions(+), 19 deletions(-) diff --git a/pkg/ccl/kvccl/kvtenantccl/connector.go b/pkg/ccl/kvccl/kvtenantccl/connector.go index 82a28c164eb4..4dfb461f1d17 100644 --- a/pkg/ccl/kvccl/kvtenantccl/connector.go +++ b/pkg/ccl/kvccl/kvtenantccl/connector.go @@ -468,7 +468,10 @@ func (c *Connector) GetSpanConfigRecords( return err } - records = spanconfig.EntriesToRecords(resp.SpanConfigEntries) + records, err = spanconfig.EntriesToRecords(resp.SpanConfigEntries) + if err != nil { + return err + } return nil }); err != nil { return nil, err diff --git a/pkg/server/node.go b/pkg/server/node.go index 8994f4ad24b4..b4f838cbd676 100644 --- a/pkg/server/node.go +++ b/pkg/server/node.go @@ -1549,9 +1549,11 @@ func (emptyMetricStruct) MetricStruct() {} func (n *Node) GetSpanConfigs( ctx context.Context, req *roachpb.GetSpanConfigsRequest, ) (*roachpb.GetSpanConfigsResponse, error) { - records, err := n.spanConfigAccessor.GetSpanConfigRecords( - ctx, spanconfig.TargetsFromProtos(req.Targets), - ) + targets, err := spanconfig.TargetsFromProtos(req.Targets) + if err != nil { + return nil, err + } + records, err := n.spanConfigAccessor.GetSpanConfigRecords(ctx, targets) if err != nil { return nil, err } @@ -1568,11 +1570,16 @@ func (n *Node) UpdateSpanConfigs( // TODO(irfansharif): We want to protect ourselves from tenants creating // outlandishly large string buffers here and OOM-ing the host cluster. Is // the maximum protobuf message size enough of a safeguard? - err := n.spanConfigAccessor.UpdateSpanConfigRecords( - ctx, spanconfig.TargetsFromProtos(req.ToDelete), spanconfig.EntriesToRecords(req.ToUpsert), - ) + toUpsert, err := spanconfig.EntriesToRecords(req.ToUpsert) if err != nil { return nil, err } + toDelete, err := spanconfig.TargetsFromProtos(req.ToDelete) + if err != nil { + return nil, err + } + if err := n.spanConfigAccessor.UpdateSpanConfigRecords(ctx, toDelete, toUpsert); err != nil { + return nil, err + } return &roachpb.UpdateSpanConfigsResponse{}, nil } diff --git a/pkg/spanconfig/target.go b/pkg/spanconfig/target.go index 192fb9e14ebb..e94aa96fb1ec 100644 --- a/pkg/spanconfig/target.go +++ b/pkg/spanconfig/target.go @@ -10,7 +10,10 @@ package spanconfig -import "github.com/cockroachdb/cockroach/pkg/roachpb" +import ( + "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/errors" +) // Target specifies the target of an associated span configuration. type Target struct { @@ -20,14 +23,14 @@ type Target struct { } // MakeTarget returns a new Target. -func MakeTarget(t roachpb.SpanConfigTarget) Target { +func MakeTarget(t roachpb.SpanConfigTarget) (Target, error) { switch t.Union.(type) { case *roachpb.SpanConfigTarget_Span: - return MakeTargetFromSpan(*t.GetSpan()) + return MakeTargetFromSpan(*t.GetSpan()), nil // TODO(arul): Add a case here for SpanConfigTarget_SystemTarget once we've // taught and tested the KVAccessor to work with system targets. default: - panic("cannot handle target") + return Target{}, errors.AssertionFailedf("unknown type of system target %v", t) } } @@ -203,15 +206,19 @@ func RecordsToEntries(records []Record) []roachpb.SpanConfigEntry { // EntriesToRecords converts a list of roachpb.SpanConfigEntries // (received over the wire) to a list of Records. -func EntriesToRecords(entries []roachpb.SpanConfigEntry) []Record { +func EntriesToRecords(entries []roachpb.SpanConfigEntry) ([]Record, error) { records := make([]Record, 0, len(entries)) for _, entry := range entries { + target, err := MakeTarget(entry.Target) + if err != nil { + return nil, err + } records = append(records, Record{ - Target: MakeTarget(entry.Target), + Target: target, Config: entry.Config, }) } - return records + return records, nil } // TargetsToProtos converts a list of targets to a list of @@ -226,10 +233,14 @@ func TargetsToProtos(targets []Target) []roachpb.SpanConfigTarget { // TargetsFromProtos converts a list of roachpb.SpanConfigTargets // (received over the wire) to a list of Targets. -func TargetsFromProtos(protoTargtets []roachpb.SpanConfigTarget) []Target { - targets := make([]Target, 0, len(protoTargtets)) - for _, t := range protoTargtets { - targets = append(targets, MakeTarget(t)) +func TargetsFromProtos(protoTargets []roachpb.SpanConfigTarget) ([]Target, error) { + targets := make([]Target, 0, len(protoTargets)) + for _, t := range protoTargets { + target, err := MakeTarget(t) + if err != nil { + return nil, err + } + targets = append(targets, target) } - return targets + return targets, nil }