multitenant: key out of tenant keyspace bounds - infinite retry loop

[ecwall]

While reviewing #97537 an existing problem was encountered:

Symptom
Passing in a key that is outside of a secondary tenant's keyspace into the new crdb_internal.ranges_in_span built-in results in an infinite retry loop:

SELECT * FROM crdb_internal.ranges_in_span('\x8b', '\x8c'); 

In the CLI this hangs until the query is canceled:

[email protected]:26257/movr> SELECT * FROM crdb_internal.ranges_in_span('\x8b', '\x8c');                                                                                                                                                                       
^C
attempting to cancel query...
ERROR: query execution canceled
SQLSTATE: 57014

---

RCA
This outer loop is the one that is retried infinitely:

cockroach/pkg/ccl/kvccl/kvtenantccl/connector.go

Line 511 in ce82ded

for ctx.Err() == nil {

stream.Recv() returns an error that is not io.EOF:

which results in this break:

cockroach/pkg/ccl/kvccl/kvtenantccl/connector.go

Line 543 in ce82ded

break

---

Possible fix
Categorize errors returned by stream.Recv() and return that error from Connector.NewIterator if it is a non-retryable client error like this.

---

Jira issue: CRDB-25537

Epic CRDB-23344

[knz]

Excellent issue description.

I am seeing the following code in the TokenBucket API handler just below:

      log.Warningf(⋄, "error issuing TokenBucket RPC: %v", err)
      if grpcutil.IsAuthError(err) {
        // Authentication or authorization error. Propagate.
        return nil, err
      }

could we solve our problem by doing the same in NewIterator?

And while we're at it, do it in the other for loops as well.

[blathers-crl]

Hi @knz, please add branch-* labels to identify which branch(es) this release-blocker affects.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[knz]

We should also consider extending the response type protobuf inside NewIterator (and the other API handlers that have a similar structure) to use an Error payload to stop the loop, the same way as done in RangeLookup.

[ecwall]

We should also consider extending the response type protobuf inside NewIterator (and the other API handlers that have a similar structure) to use an Error payload to stop the loop, the same way as done in RangeLookup.

Do you have insight into why RangeLookup is designed this way? It is not clear from this API why some errors are returned as nested errors inside the "success" response (resp.Error) and some errors are top level errors (err).

resp, err := client.RangeLookup(ctx, &kvpb.RangeLookupRequest{
	...
})
if err != nil {
	log.Warningf(ctx, "error issuing RangeLookup RPC: %v", err)
	if grpcutil.IsAuthError(err) {
		// Authentication or authorization error. Propagate.
		return nil, nil, err
	}
	// Soft RPC error. Drop client and retry.
	c.tryForgetClient(ctx, client)
	continue
}
if resp.Error != nil {
	// Hard logical error. Propagate.
	return nil, nil, resp.Error.GoError()
}

Also the response being "success" vs "error" seems to be enforced as a comment and not as a type union.

message RangeLookupResponse {
  repeated RangeDescriptor descriptors            = 1 [(gogoproto.nullable) = false];
  repeated RangeDescriptor prefetched_descriptors = 2 [(gogoproto.nullable) = false];
  // If non-nil, the other fields will be empty.
  kv.kvpb.Error error = 3;
}

If we are already forced to handle top-level errors and can use grpcutil.IsAuthError to distinguish between retriable and non-retriable, then what is the purpose of adding nested errors in the success response?

[knz]

The Error payload inside the protobuf is an applicaiton-level error, after authentication/authorization has succeeded. It can contain details about the application.

The "outer" gRPC error reflects errors happening in the communication between client and server, and can be reported before authentication/authorization and thus must not contain application details.

[ecwall]

It looks like application-level errors are supported in gRPC status codes - https://grpc.github.io/grpc/core/md_doc_statuscodes.html

I think the tenant trying to access keys outside of its keyspace would be OUT_OF_RANGE.

Is there a reason to not use a status code like this?

[knz]

Is there a reason to not use a status code like this?

Yes - grpc status codes/errors can't have a payload, so we don't get things like stack traces, redactable strings etc.

(We have a separate project to extend grpc error reporting to use our github.com/cockroachdb/errors protobuf to carry this data, but it's not done yet)

[knz]

Ok so I'm realizing that my previous comment was confusing.

What I meant in that comment is that we could consider having an Error protobuf payload to report KV-level errors in addition to reporting authentication/authz failures using gRPC statuses. We will need both eventually.

This means we will see something like this in the code for every tenant connector RPC in the future:

    if err != nil {
      if grpcutil.IsAuthError(err) {
        // gRPC-level Authentication or authorization error.  Propagate.
        return err
      }
      // Soft RPC error. Drop client and retry.
      c.tryForgetClient(ctx, client)
      continue
    }
    if resp.Error != nil {
      // Hard logical error. Propagate.
      return resp.Error.GoError()
    }

[ecwall]

I created a new issue for connector code improvement so that this issue can be closed as a ga-blocker: #99144