grpcui icon indicating copy to clipboard operation
grpcui copied to clipboard

Incompatible with validators

Open hoveychen opened this issue 6 years ago • 6 comments

Tested with both github.com/mwitkow/go-proto-validators and github.com/envoyproxy/protoc-gen-validate/, it all failed with File not found errors.

This is caused by these validators lib not registering themselves into reflection. However, these protos are used in field/method options, and in fact not actual dependencies to make the GRPC call.

Our current workaround is to fork the underlying project jhump/protoreflect, and provide an opt-out option to ignore failure to reflect these lib.

Looking forward to a better solution.

hoveychen avatar Aug 02 '19 05:08 hoveychen

@hoveychen, this is not really a grpcui issue. It's a known issue inside of the protobuf runtime for Go, regarding how descriptors are registered: https://github.com/golang/protobuf/issues/567

I also have an existing issue in protoreflect (https://github.com/jhump/protoreflect/issues/170) that would allow this to be worked around in the client (though it would require a custom main that wires up the correct import paths since it would not be safe to assume they are correct for all possible services and it also could never be authoritative). Perhaps the mappings could be configurable via command-line arguments though.

Another likely better way to solve this is in the reflection service itself. The reflection service is all filename-based, so it suffers from the same brittleness as the (issue mentioned above)[https://github.com/golang/protobuf/issues/567]. If a file is compiled with a different relative path than how it is referenced in import statements, clients will be unable to crawl the entire transitive closure of dependencies. So I think the reflection service needs an extra option that allows the server to provide the entire closure (since it may have better ability to find dependencies that don't rely solely on name/path, which is brittle).

jhump avatar Aug 02 '19 15:08 jhump

Now that I think about it, if you are using service reflection, it really is an issue in service reflection, that effects all runtimes, not just Go. So it's not really caused by that linked golang/protobuf issue (though the root cause is the same: linking via filename is brittle).

jhump avatar Aug 02 '19 15:08 jhump

@hoveychen, I filed https://github.com/grpc/grpc/issues/19832 because issues of this kind can be a real thorn in the side for anyone that uses service reflection.

However, I also realized that the problem here may be slightly different: are you possibly building with protoc-gen-go, but then using libraries that were built with protoc-gen-gogo? Is the issue that the validators packages register the descriptors with the gogo/protobuf package, not the normal golang/protobuf package?

If so, I bet you could create a simple fork of the gRPC server reflection implementation that can consult both when loading descriptors. It could consult golang/protobuf first and then fallback to gogo/protobuf.

FWIW, v2 of the golang/protobuf should finally solve a lot of this mess. It should address a lot of the linking brittleness issues already present in golang/protobuf. But it will also provide mechanisms to interoperate with gogo/protobuf, so gogo/protobuf can extend the core runtime instead of forking it. The protoc-gen-gogo plugin can generate the code it wants and still register its types and files in the core runtime's registry, by supplying custom implementations of some of the reflection interfaces that know how to reflect over its alternate generated types. Here's the v2 design doc, and here's the in-progress work.

jhump avatar Aug 03 '19 13:08 jhump

Also, while far less convenient, you should also be able to run grpcui and provide it proto sources and import paths. That will not encounter the same linking issues because it will basically re-compile everything from proto sources to compute the schema.

jhump avatar Aug 03 '19 13:08 jhump

Thanks. @jhump

This issue has been extended a bit more away from my expectation. As far as I know, this issue is probably the case in your statement.

However, I also realized that the problem here may be slightly different: are you possibly building with protoc-gen-go, but then using libraries that were built with protoc-gen-gogo? Is the issue that the validators packages register the descriptors with the gogo/protobuf package, not the normal golang/protobuf package?

I looked into the implementations of these two validator packages, and realized that indeed they are depending on https://github.com/gogo/protobuf. There is an PR https://github.com/gogo/protobuf/pull/574 pending to merge, so there are few options we can do right now, either:

  • Fix reflect in gogo/protobuf.
  • Ignore incompatible proto stub generated by gogo/protobuf or other program, and give out some warnings instead of crashing the whole app.
  • Wait for protobuf-v2, and gogo/protobuf to migrate to it.

The 2nd option seems to be preferable to us anyway.

hoveychen avatar Oct 09 '19 09:10 hoveychen

My workaround is just to copy & paste the validate.proto and modify the go_package to your self-hosted one, just remember to match the import path.

https://github.com/envoyproxy/protoc-gen-validate/pull/443

lixin9311 avatar Mar 23 '21 08:03 lixin9311