Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incompatible with validators #46

Open
hoveychen opened this issue Aug 2, 2019 · 6 comments
Open

Incompatible with validators #46

hoveychen opened this issue Aug 2, 2019 · 6 comments

Comments

@hoveychen
Copy link

hoveychen commented Aug 2, 2019

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.

@jhump
Copy link
Contributor

jhump commented Aug 2, 2019

@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: golang/protobuf#567

I also have an existing issue in protoreflect (jhump/protoreflect#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
Copy link
Contributor

jhump commented Aug 2, 2019

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
Copy link
Contributor

jhump commented Aug 3, 2019

@hoveychen, I filed grpc/grpc#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
Copy link
Contributor

jhump commented Aug 3, 2019

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.

@hoveychen
Copy link
Author

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 gogo/protobuf#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.

@lixin9311
Copy link

lixin9311 commented Mar 23, 2021

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.

bufbuild/protoc-gen-validate#443

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants