extgrpc: add support for automatic en/decoding of gRPC Status

erikgrinaker · erikgrinaker · commit c7e33ef3898f · 2021-03-17T13:20:55.000+01:00
This patch adds support for automatic en/decoding of gRPC `Status` errors, enabled by importing the `extgrpc` package. It supports statuses generated both by the standard `google.golang.org/grpc/status` and the gogoproto-based `github.com/gogo/status` packages. Encoded statuses are always represented as `gogo/status`-based `Status` Protobufs, since the `EncodedError` type is a gogoproto type using an `Any` field for structured errors, and only gogoproto types can be placed here since standard Protobufs are not registered in the gogoproto type registry. We lock the `gogo/googleapis` package to 1.2.0 to use gogoproto 1.2-compatible Protobufs required by CRDB, these have been verified to not be vulnerable to the "skippy pb" bug. Note that to preserve error details, gogo-based `Status` objects with gogo-based details must be used, otherwise details may not be preserved. This is for similar reasons as outlined above, and is a limitation in the gRPC package -- see code comments for further details. A CRDB linter rule forbidding use of `Status.WithDetails()` was submitted in cockroachdb/cockroach#61617.
diff --git a/extgrpc/ext_grpc.go b/extgrpc/ext_grpc.go
@@ -12,6 +12,31 @@
 // implied. See the License for the specific language governing
 // permissions and limitations under the License.
 
+// This package provides support for gRPC error handling. It has
+// two main features:
+//
+// 1. Automatic en/decoding of gRPC Status errors via EncodeError and
+//    DecodeError, enabled by importing the package. Supports both the
+//    standard google.golang.org/grpc/status and the gogoproto-compatible
+//    github.com/gogo/status packages.
+//
+// 2. Wrapping arbitrary errors with a gRPC status code via WrapWithGrpcCode()
+//    and GetGrpcCode(). There is also a gRPC middleware in middleware/grpc
+//    that will automatically do this (un)wrapping.
+//
+// Note that it is not possible to mix standard Protobuf and gogoproto
+// status details via Status.WithDetails(). All details must be standard
+// Protobufs with the standard grpc/status package, and all details must
+// be gogoproto Protobufs with the gogo/status package. This is caused by
+// WithDetails() and Details() immediately (un)marshalling the detail as
+// an Any field, and since gogoproto types are not registered with the standard
+// Protobuf registry (and vice versa) they cannot be unmarshalled.
+//
+// Furthermore, since we have to encode all errors as gogoproto Status
+// messages to place them in EncodedError (again because of an Any field),
+// only gogoproto status details can be preserved across EncodeError().
+// See also: https://github.com/cockroachdb/errors/issues/63
+
 package extgrpc
 
 import (
@@ -22,28 +47,28 @@ import (
 	"github.com/cockroachdb/errors/errbase"
 	"github.com/cockroachdb/errors/markers"
 	"github.com/cockroachdb/redact"
+	gogorpc "github.com/gogo/googleapis/google/rpc"
 	"github.com/gogo/protobuf/proto"
+	gogostatus "github.com/gogo/status"
 	"google.golang.org/grpc/codes"
+	grpcstatus "google.golang.org/grpc/status"
 )
 
-// This file demonstrates how to add a wrapper type not otherwise
-// known to the rest of the library.
-
-// withGrpcCode is our wrapper type.
+// withGrpcCode wraps an error with a gRPC status code.
 type withGrpcCode struct {
 	cause error
 	code  codes.Code
 }
 
-// WrapWithGrpcCode adds a Grpc code to an existing error.
+// WrapWithGrpcCode wraps an error with a gRPC status code.
 func WrapWithGrpcCode(err error, code codes.Code) error {
 	if err == nil {
 		return nil
 	}
 	return &withGrpcCode{cause: err, code: code}
 }
 
-// GetGrpcCode retrieves the Grpc code from a stack of causes.
+// GetGrpcCode retrieves the gRPC code from a stack of causes.
 func GetGrpcCode(err error) codes.Code {
 	if err == nil {
 		return codes.OK
@@ -70,7 +95,7 @@ func (w *withGrpcCode) Unwrap() error { return w.cause }
 func (w *withGrpcCode) Format(s fmt.State, verb rune) { errors.FormatError(w, s, verb) }
 
 // SafeFormatter implements errors.SafeFormatter.
-// Note: see the documentat ion of errbase.SafeFormatter for details
+// Note: see the documentation of errbase.SafeFormatter for details
 // on how to implement this. In particular beware of not emitting
 // unsafe strings.
 func (w *withGrpcCode) SafeFormatError(p errors.Printer) (next error) {
@@ -96,7 +121,68 @@ func decodeWithGrpcCode(
 	return &withGrpcCode{cause: cause, code: codes.Code(wp.Code)}
 }
 
+// encodeGrpcStatus takes an error generated by a standard gRPC Status and
+// converts it into a GoGo Protobuf representation from
+// github.com/gogo/googleapis/google/rpc.
+//
+// This is necessary since EncodedError uses an Any field for structured errors,
+// and thus can only contain Protobufs that have been registered with the GoGo
+// Protobuf type registry -- the standard gRPC Status type is not a GoGo
+// Protobuf, and is therefore not registered with it and cannot be decoded by
+// DecodeError().
+//
+// Also note that in order to use error details, the input type must be a
+// gogoproto Status from github.com/gogo/status, not from the standard gRPC
+// Status, and all details must be gogoproto types. The reasons for this
+// are the same as for the Any field issue mentioned above.
+func encodeGrpcStatus(_ context.Context, err error) (string, []string, proto.Message) {
+	s := gogostatus.Convert(err)
+	// If there are known safe details, return them.
+	details := []string{}
+	for _, detail := range s.Details() {
+		if safe, ok := detail.(errbase.SafeDetailer); ok {
+			details = append(details, safe.SafeDetails()...)
+		}
+	}
+	return s.Message(), details, s.Proto()
+}
+
+// decodeGrpcStatus is the inverse of encodeGrpcStatus. It takes a gogoproto
+// Status as input, and converts it into a standard gRPC Status error.
+func decodeGrpcStatus(
+	ctx context.Context, msg string, details []string, payload proto.Message,
+) error {
+	return grpcstatus.Convert(decodeGoGoStatus(ctx, msg, details, payload)).Err()
+}
+
+// encodeGoGoStatus encodes a GoGo Status error. It calls encodeGrpcStatus, since
+// it can handle both kinds.
+func encodeGoGoStatus(ctx context.Context, err error) (string, []string, proto.Message) {
+	return encodeGrpcStatus(ctx, err)
+}
+
+// decodeGoGoStatus is similar to decodeGrpcStatus, but decodes into a gogo
+// Status error instead of a gRPC Status error. It is used when the original
+// error was a gogo Status error rather than a gRPC Status error.
+func decodeGoGoStatus(_ context.Context, _ string, _ []string, payload proto.Message) error {
+	s, ok := payload.(*gogorpc.Status)
+	if !ok {
+		// If input type was unexpected (shouldn't happen), we just return nil
+		// which will cause DecodeError() to return an opaqueLeaf.
+		return nil
+	}
+	return gogostatus.ErrorProto(s)
+}
+
 func init() {
+	grpcError := grpcstatus.Error(codes.Unknown, "")
+	errbase.RegisterLeafEncoder(errbase.GetTypeKey(grpcError), encodeGrpcStatus)
+	errbase.RegisterLeafDecoder(errbase.GetTypeKey(grpcError), decodeGrpcStatus)
+
+	gogoError := gogostatus.Error(codes.Unknown, "")
+	errbase.RegisterLeafEncoder(errbase.GetTypeKey(gogoError), encodeGoGoStatus)
+	errbase.RegisterLeafDecoder(errbase.GetTypeKey(gogoError), decodeGoGoStatus)
+
 	errbase.RegisterWrapperEncoder(errbase.GetTypeKey((*withGrpcCode)(nil)), encodeWithGrpcCode)
 	errbase.RegisterWrapperDecoder(errbase.GetTypeKey((*withGrpcCode)(nil)), decodeWithGrpcCode)
 }
diff --git a/extgrpc/ext_grpc_test.go b/extgrpc/ext_grpc_test.go
@@ -20,9 +20,16 @@ import (
 	"testing"
 
 	"github.com/cockroachdb/errors"
+	"github.com/cockroachdb/errors/errbase"
+	"github.com/cockroachdb/errors/errorspb"
 	"github.com/cockroachdb/errors/extgrpc"
 	"github.com/cockroachdb/errors/testutils"
+	"github.com/gogo/protobuf/proto"
+	gogostatus "github.com/gogo/status"
+	"github.com/stretchr/testify/require"
 	"google.golang.org/grpc/codes"
+	grpcstatus "google.golang.org/grpc/status"
+	"google.golang.org/protobuf/runtime/protoiface"
 )
 
 func TestGrpc(t *testing.T) {
@@ -57,3 +64,137 @@ Error types: (1) *extgrpc.withGrpcCode (2) *errors.errorString`)
 	var noErr error
 	tt.Assert(extgrpc.GetGrpcCode(noErr) == codes.OK)
 }
+
+// dummyProto is a dummy Protobuf message which satisfies the proto.Message
+// interface but is not registered with either the standard Protobuf or GoGo
+// Protobuf type registries.
+type dummyProto struct {
+	value string
+}
+
+func (p *dummyProto) Reset()         {}
+func (p *dummyProto) String() string { return "" }
+func (p *dummyProto) ProtoMessage()  {}
+
+// statusIface is a thin interface for common gRPC and gogo Status functionality.
+type statusIface interface {
+	Code() codes.Code
+	Message() string
+	Details() []interface{}
+	Err() error
+}
+
+func TestEncodeDecodeStatus(t *testing.T) {
+	testcases := []struct {
+		desc          string
+		makeStatus    func(*testing.T, codes.Code, string, []proto.Message) statusIface
+		fromError     func(err error) statusIface
+		expectDetails []interface{} // nil elements signify errors
+	}{
+		{
+			desc: "gogo status",
+			makeStatus: func(t *testing.T, code codes.Code, msg string, details []proto.Message) statusIface {
+				s, err := gogostatus.New(code, msg).WithDetails(details...)
+				require.NoError(t, err)
+				return s
+			},
+			fromError: func(err error) statusIface {
+				return gogostatus.Convert(err)
+			},
+			expectDetails: []interface{}{
+				nil, // Protobuf decode fails
+				&errorspb.StringsPayload{Details: []string{"foo", "bar"}}, // gogoproto succeeds
+				nil, // dummy decode fails
+			},
+		},
+		{
+			desc: "grpc status",
+			makeStatus: func(t *testing.T, code codes.Code, msg string, details []proto.Message) statusIface {
+				s := grpcstatus.New(code, msg)
+				for _, detail := range details {
+					var err error
+					s, err = s.WithDetails(protoiface.MessageV1(detail))
+					require.NoError(t, err)
+				}
+				return s
+			},
+			fromError: func(err error) statusIface {
+				return grpcstatus.Convert(err)
+			},
+			expectDetails: []interface{}{
+				grpcstatus.New(codes.Internal, "status").Proto(), // Protobuf succeeds
+				nil, // gogoproto decode fails
+				nil, // dummy decode fails
+			},
+		},
+	}
+	for _, tc := range testcases {
+		tc := tc
+		t.Run(tc.desc, func(t *testing.T) {
+			ctx := context.Background()
+
+			// Create a Status, using statusIface to support gRPC and gogo variants.
+			status := tc.makeStatus(t, codes.NotFound, "message", []proto.Message{
+				grpcstatus.New(codes.Internal, "status").Proto(),          // standard Protobuf
+				&errorspb.StringsPayload{Details: []string{"foo", "bar"}}, // GoGo Protobuf
+				&dummyProto{value: "dummy"},                               // unregistered
+			})
+			require.Equal(t, codes.NotFound, status.Code())
+			require.Equal(t, "message", status.Message())
+
+			// Check the details. This varies by implementation, since different
+			// Protobuf decoders are used -- gRPC Status can only decode
+			// standard Protobufs, while gogo Status can only decode gogoproto
+			// Protobufs.
+			statusDetails := status.Details()
+			require.Equal(t, len(tc.expectDetails), len(statusDetails), "detail mismatch")
+			for i, expectDetail := range tc.expectDetails {
+				if expectDetail == nil {
+					require.Implements(t, (*error)(nil), statusDetails[i], "detail %v", i)
+				} else {
+					require.Equal(t, expectDetail, statusDetails[i], "detail %v", i)
+				}
+			}
+
+			// Encode the error and check some fields.
+			encodedError := errbase.EncodeError(ctx, status.Err())
+			leaf := encodedError.GetLeaf()
+			require.NotNil(t, leaf, "expected leaf")
+			require.Equal(t, status.Message(), leaf.Message)
+			require.Equal(t, []string{}, leaf.Details.ReportablePayload) // test this?
+			require.NotNil(t, leaf.Details.FullDetails, "expected full details")
+			require.Nil(t, encodedError.GetWrapper(), "unexpected wrapper")
+
+			// Marshal and unmarshal the error, checking that
+			// it equals the encoded error.
+			marshaledError, err := encodedError.Marshal()
+			require.NoError(t, err)
+			require.NotEmpty(t, marshaledError)
+
+			unmarshaledError := errorspb.EncodedError{}
+			err = proto.Unmarshal(marshaledError, &unmarshaledError)
+			require.NoError(t, err)
+			require.True(t, proto.Equal(&encodedError, &unmarshaledError),
+				"unmarshaled Protobuf differs")
+
+			// Decode the error.
+			decodedError := errbase.DecodeError(ctx, unmarshaledError)
+			require.Equal(t, status.Err().Error(), decodedError.Error())
+
+			// Convert the error into a status, and check its properties.
+			decodedStatus := tc.fromError(decodedError)
+			require.Equal(t, status.Code(), decodedStatus.Code())
+			require.Equal(t, status.Message(), decodedStatus.Message())
+
+			decodedDetails := decodedStatus.Details()
+			require.Equal(t, len(tc.expectDetails), len(decodedDetails), "detail mismatch")
+			for i, expectDetail := range tc.expectDetails {
+				if expectDetail == nil {
+					require.Implements(t, (*error)(nil), decodedDetails[i], "detail %v", i)
+				} else {
+					require.Equal(t, expectDetail, decodedDetails[i], "detail %v", i)
+				}
+			}
+		})
+	}
+}
diff --git a/go.mod b/go.mod
@@ -7,10 +7,13 @@ require (
 	github.com/cockroachdb/logtags v0.0.0-20190617123548-eb05cc24525f
 	github.com/cockroachdb/redact v1.0.8
 	github.com/cockroachdb/sentry-go v0.6.1-cockroachdb.2
+	github.com/gogo/googleapis v1.2.0 // gogoproto 1.2-compatible, for CRDB
 	github.com/gogo/protobuf v1.3.2
 	github.com/gogo/status v1.1.0
 	github.com/hydrogen18/memlistener v0.0.0-20141126152155-54553eb933fb
 	github.com/kr/pretty v0.1.0
 	github.com/pkg/errors v0.9.1
+	github.com/stretchr/testify v1.4.0
 	google.golang.org/grpc v1.29.1
+	google.golang.org/protobuf v1.23.0
 )
diff --git a/go.sum b/go.sum
