Skip to content

Commit

Permalink
spanconfig: remove panic from code around targets
Browse files Browse the repository at this point in the history
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
  • Loading branch information
arulajmani authored and RajivTS committed Mar 6, 2022
1 parent dcf0ba1 commit edf6e7a
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 19 deletions.
5 changes: 4 additions & 1 deletion pkg/ccl/kvccl/kvtenantccl/connector.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 13 additions & 6 deletions pkg/server/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
}
35 changes: 23 additions & 12 deletions pkg/spanconfig/target.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
}
}

Expand Down Expand Up @@ -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
Expand All @@ -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
}

0 comments on commit edf6e7a

Please sign in to comment.