erikgrinaker commented 3 years ago

{En,De}codeError() currently does not propagate gRPC status.Error, instead masking it as opaqueLeaf.

This is further complicated by gRPC errors being vanilla Protobuf messages while EncodedError uses GoGo Protobufs. The structured error is encoded as an Any field, but since vanilla Protobuf messages are not registered with the GoGo type registry they cannot be used in Any fields since the GoGo unmarshaler does not know about the type to decode into. See also https://github.com/cockroachdb/cockroach/issues/56718#issuecomment-787496529.

14 added some tooling for gRPC errors, including a GoGo message type that can be used as a wrapper. Care must be taken with error details, since these may also be a mix of vanilla and GoGo types, and we may not be able to preserve vanilla types for the same reasons as outlined above.

erikgrinaker commented 3 years ago

Preserving details turned out to be even more challenging: calling Status.WithDetails() will immediately marshal the detail as an Any type, which fails to decode during Details() for GoGo types since they're not registered with the standard Protobuf type registry used by gRPC.

In the following example, we add a GoGo type as a status detail, and then try to immediately read it back, which fails. Note that cockroachdb/errors is not involved here at all.

func TestGRPCStatusDetails(t *testing.T) {
  protoDetail := grpcstatus.New(codes.InvalidArgument, "status").Proto()  // standard Protobuf
  gogoDetail := &errorspb.StringsPayload{Details: []string{"foo", "bar"}} // GoGo Protobuf
  status, err := grpcstatus.New(codes.NotFound, "message").WithDetails(protoDetail, gogoDetail)
  require.NoError(t, err)

  // This fails, because details[1] contains prefixError{s:"not found"}. This
  // happens because the standard Protobuf decoder does not know about
  // gogoDetail's type.
  require.Equal(t, []interface{}{protoDetail, gogoDetail}, status.Details())
}

As far as I can tell, it's not possible to use GoGo type details with standard gRPC statuses at all, and this appears to be confirmed by this blog post: https://jbrandhorst.com/post/gogoproto/

@tbg To use gRPC error details I think we'll have to migrate CockroachDB to use https://github.com/gogo/status. This has the same API as gRPC status but uses GoGo types instead.

tbg commented 3 years ago

Yikes! Thanks for the investigation. I think, then, that the right thing to do is to just lint against WithDetails for now, as this has clearly never worked and the proto situation is unclear enough to avoid buying into gogoproto more. You can follow this example to add the lint:

https://github.com/cockroachdb/cockroach/blob/765260333cb0eb3467c5ad6856bd042587307a95/pkg/testutils/lint/passes/forbiddenmethod/analyzers.go#L17-L35

Let me know if you don't think that is a reasonable way forward.

cockroachdb / errors

Handle gRPC status.Error in {En,De}codeError() #63

14 added some tooling for gRPC errors, including a GoGo message type that can be used as a wrapper. Care must be taken with error details, since these may also be a mix of vanilla and GoGo types, and we may not be able to preserve vanilla types for the same reasons as outlined above.