Add gRPC Tools

[tooolbox]

gRPC Tools

1. Created grpc/middleware package which provides server-side and client-side unary gRPC middleware which can be attached to connections to allow transparent encoding & decoding of errors.
2. Created extgrpc package, copied from exthttp, which provides a wrapper to annotate an error with a gRPC status.
3. Created grpc/status package which reproduces a subset of the github.com/gogo/status functionality, namely:
   • Error(c codes.Code, msg string) error for returning a gRPC status inside a server implementation method (no error wrapping occurs).
   • WrapErr(c codes.Code, msg string, err error) error for returning a gRPC status with an included error.
   • Code(err error) codes.Code for evaluating the gRPC status code returned by a server.

With the middleware in place, errors from this package will transparently encode & decode when returned by the server and received by the client.

The other packages work together to allow a developer to return & read gRPC status codes in addition to the cockroachdb/errors functionality, if desired.

Caveats

• This work was done using go modules, which I added support for in another PR. To keep the PRs separate, I removed the go.mod and go.sum files. I haven't tested it in a GOPATH configuration.
• I generated ext_grpc.pb.go using gogoproto to ensure compatibility. However, it has generated with const _ = proto.GoGoProtoPackageIsVersion3 whereas the other .pb.go files in the repo are v2. I tried to roll back my libproto to 2.x, but apparently that const is the progeny of your go plugin for protoc. Shortly afterwards I gave up; I don't mind fixing it if someone has pointers for me.

Note: all tests passing, including the new ones.

---

This change is 

[knz]

This is very good work. I only have minor concerns, see below.

Reviewed 14 of 14 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @tooolbox)

---

.gitignore, line 14 at r1 (raw file):

*.out

.DS_Store

OK with this change but please send this change in a separate commit or PR

---

extgrpc/ext_grpc.go, line 83 at r1 (raw file):

func encodeWithGrpcCode(_ context.Context, err error) (string, []string, proto.Message) {
	w := err.(*withGrpcCode)
	details := []string{fmt.Sprintf("Grpc %d", w.code)}

nit: "gRPC ...

That said, I wonder if you really need the new type EncodedGrpcCode. It seems to me that you can extract the code from the details string.

---

grpc/middleware/client.go, line 21 at r1 (raw file):

	opts ...grpc.CallOption,
) error {

nit: no empty line needed here

---

grpc/middleware/server.go, line 19 at r1 (raw file):

	handler grpc.UnaryHandler,
) (interface{}, error) {

nit: ditto

---

grpc/middleware/server.go, line 32 at r1 (raw file):

		st, err = st.WithDetails(&enc)
		if err != nil {
			panic(err) // Programmer error

Is this the best available? I'm thinking this could return an assertion error instead.

[tooolbox]

Thanks. I resolved most of your points and have some comments on two of them.

Reviewable status: 10 of 13 files reviewed, 2 unresolved discussions (waiting on @knz)

---

.gitignore, line 14 at r1 (raw file):

Previously, knz (kena) wrote…

OK with this change but please send this change in a separate commit or PR

Sure--removed for now.

---

extgrpc/ext_grpc.go, line 83 at r1 (raw file):

Previously, knz (kena) wrote…

nit: "gRPC ...

That said, I wonder if you really need the new type EncodedGrpcCode. It seems to me that you can extract the code from the details string.

Handled the casing, as well as the casing of another "grpc" string adjacent to this one.

Re the 2nd point, you are probably right, but I think it's better to have a uniform approach, and just have the gRPC Code match the HTTP Code implementation exactly.

---

grpc/middleware/server.go, line 32 at r1 (raw file):
I was following the pattern from https://jbrandhorst.com/post/grpc-errors/

// If this errored, it will always error
// here, so better panic so we can figure
// out why than have this silently passing.

Let's look a bit deeper: the underlying call is ptypes.MarshalAny(detail). Since the argument type is proto.Message, it seems like the only way this can fail is if that type is not actually registered with the proto package doing the encoding.

If we leave it as-is and panic, either your service goes down or (if you have a recovery middleware active) the call fails in an ugly fashion. This way, at least, attracts quite some attention.

If we create an Assertion error:

1. Our entire error stack vanishes and does not cross the boundary. We just failed to append it to the gRPC status, so the only things that are getting back across are the stringification of our Assertion and a status code. (The status would need to be Internal.)
2. When the client receives the error back, it will not decode into anything special. What would we expect the client code to do at that point? Will it create subtle bugs because none of the expected errors are received?
3. The server has no negative effects or ways to detect the serious misconfiguration, unless we put some sort of logging in the middleware. Using the default logging package seems proscriptive, unless the middleware takes a logger as an argument, which seems silly when the only reason it would log anything would be because of a serious programmer error.

I am not stuck on either route but I do think that a straight-up panic is more appropriate. Let me know if you want me to change it.

[tooolbox]

Also @knz right now the helper method status.WrapErr(code, msg, error) does an errors.Wrap() which adds the message and a stacktrace. Do you think the assumption of adding a stacktrace at that point is much? Could make it just an errors.WithMessage().

Reviewable status: 8 of 13 files reviewed, 2 unresolved discussions (waiting on @knz)

[knz]

Thanks for the changes. I am going to look again later today. Sorry for the delay.

[knz]

Reviewed 6 of 6 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tooolbox)

---

grpc/middleware/server.go, line 32 at r1 (raw file):

Previously, tooolbox (Matt Mc) wrote…

I was following the pattern from https://jbrandhorst.com/post/grpc-errors/

// If this errored, it will always error
// here, so better panic so we can figure
// out why than have this silently passing.

Let's look a bit deeper: the underlying call is ptypes.MarshalAny(detail). Since the argument type is proto.Message, it seems like the only way this can fail is if that type is not actually registered with the proto package doing the encoding.

If we leave it as-is and panic, either your service goes down or (if you have a recovery middleware active) the call fails in an ugly fashion. This way, at least, attracts quite some attention.

If we create an Assertion error:

1. Our entire error stack vanishes and does not cross the boundary. We just failed to append it to the gRPC status, so the only things that are getting back across are the stringification of our Assertion and a status code. (The status would need to be Internal.)
2. When the client receives the error back, it will not decode into anything special. What would we expect the client code to do at that point? Will it create subtle bugs because none of the expected errors are received?
3. The server has no negative effects or ways to detect the serious misconfiguration, unless we put some sort of logging in the middleware. Using the default logging package seems proscriptive, unless the middleware takes a logger as an argument, which seems silly when the only reason it would log anything would be because of a serious programmer error.

I am not stuck on either route but I do think that a straight-up panic is more appropriate. Let me know if you want me to change it.

First I'd recommend adding a link to the aforementioned blog post in an inline comment here.

Then I'd also extend that comment with the rationale you just wrote above.

Then I'd remove "// Programmer error" and instead make a comment about how to address this panic if it ever happens in practice.

[tooolbox]

No problem! Your last point is addressed.

Reviewable status: 12 of 13 files reviewed, 1 unresolved discussion (waiting on @knz)

---

grpc/middleware/server.go, line 32 at r1 (raw file):

Previously, knz (kena) wrote…

First I'd recommend adding a link to the aforementioned blog post in an inline comment here.

Then I'd also extend that comment with the rationale you just wrote above.

Then I'd remove "// Programmer error" and instead make a comment about how to address this panic if it ever happens in practice.

Makes sense. I included the quote, the link, and a shortened version of the rationale.

[knz]

Reviewed 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

[knz]

Sweet. 🎉
I'm going to merge this now - feel free to send a follow-up PR that introduces a README file in the package's sub-directory to explain how to use it. No pressure.

[knz]

Oh wait I'm not seeing the contributor license agreement notification. Did you receive the CLA notification?

Something like this: https://cla-assistant.io/cockroachdb/errors?pullRequest=14

[knz]

Just got it. Thanks!

[tooolbox]

feel free to send a follow-up PR that introduces a README file in the package's sub-directory to explain how to use it

Yeah, good point, I'll let you know.