fullstorydev / grpcurl

Like cURL, but for gRPC: Command-line tool for interacting with gRPC servers
MIT License
10.35k stars 497 forks source link

Version 1.8.9 proto: invalid syntax #423

Open EwanValentine opened 8 months ago

EwanValentine commented 8 months ago

Just letting you know, version 1.8.9 gives us this error:

Failed to resolve symbol "<redacted>": proto: invalid syntax: "<unknown:0>"

Downgrading to 1.8.5 fixes the error. I'm not sure what's causing the error itself, but just a heads up

jhump commented 8 months ago

@EwanValentine, can you provide details about the source file you are trying to use? A small repro case for example?

donkeywon commented 8 months ago

same error since 1.8.8

Error invoking method "proto.Xxx/xxx": failed to query for service descriptor "proto.Xxx": proto: invalid syntax: "<unknown:0>"

downgrading to 1.8.7 fixes

jhump commented 8 months ago

@donkeywon, @EwanValentine, I am not able to reproduce this issue. Can you provide information about your source files? A small repro case maybe?

printfn commented 8 months ago

I'm seeing what looks like the same bug as well.

v1.8.8: Failed to resolve symbol "xxx.xxx": proto: invalid syntax: "<unknown:0>"

v1.8.7: Failed to resolve symbol "xxx.xxx": file "xxx.proto" included an unresolvable reference to ".xxx.xxx"

After fixing the unresolvable reference grpcurl could correctly describe and call the service, on v1.8.7 and v1.8.8 (and the latest version).

jhump commented 8 months ago

@printfn, some details on the files you are using, like a repro case, would be incredibly helpful for us to reproduce and fix. Could you give examples on why the reference was unresolvable in your sources? What did you have to change to get them working?

EwanValentine commented 8 months ago

@jhump apologies, I'll try and come up with a repro case, will take some time/unpicking though

printfn commented 8 months ago

@jhump I've created a minimal reproduction here: https://github.com/printfn/grpcurl-repro See the included README file for specific instructions, including the change I needed to make to the .proto source file to work around the bug.

donkeywon commented 8 months ago

same error since 1.8.8

Error invoking method "proto.Xxx/xxx": failed to query for service descriptor "proto.Xxx": proto: invalid syntax: "<unknown:0>"

downgrading to 1.8.7 fixes

@jhump here is the demo, it can reproduce the error

https://github.com/donkeywon/proto_demo

I found that the error was caused by adding validate

jhump commented 7 months ago

@donkeywon, it turns out that this is a problem in your proto code, and it just happened to be overlooked by earlier versions of grpcurl. That's because the earlier versions used github.com/jhump/protoreflect (v1.14 and earlier) as the descriptor/reflection implementation. But as of v1.8.8, it now uses the official Go protobuf runtime APIs for descriptors and reflection, in google.golang.org/protobuf/reflect/protoreflect. These newer APIs are much more strict. In this case, they refuse to process the validate.proto descriptor because it is invalid. My protoreflect library was more lenient in this regard.

The problem is that the validation proto's proper import path is validate/validate.proto. That is the path used to compile the file itself, and is thus how the file's descriptor is registered in the Go runtime. But your sources import it as proto/validate/validate.proto -- an extra "proto/" prefix in the path. That means when grpcurl is trying to download the schema for your service, the server is sending a placeholder for proto/validate/validate.proto because that file is not actually known (since the file was actually registered as validate/validate.proto). However, the placeholder it is sending is invalid since it contains the literal string "<unknown:0>" as the value of the descriptor's syntax field (this could arguably be considered a bug in the Go protobuf runtime or the Go implementation of the gRPC reflection service).

I'm almost certain that the problems that @EwanValentine and @printfn are seeing has the same root cause: a proto is being imported using the wrong path, so the descriptors can't be linked. The file that is being imported incorrectly is certain to be a file that defines custom options. If it defined actual types needed by your sources (like messages and enums that you use as field types), then even my protoreflect implementation would refuse to process the descriptors.

Read this article for more context on getting import paths right and best practices.

In any event, @donkeywon, you can fix the code in your demo repo with the following diff:

diff --git a/gen_proto.sh b/gen_proto.sh
index 3d9f4e5..827dab7 100755
--- a/gen_proto.sh
+++ b/gen_proto.sh
@@ -1 +1 @@
-protoc -I=. --go-grpc_out=pb --go_out=pb --validate_out="lang=go:pb" proto/*.proto
+protoc -I=. -I=./proto --go-grpc_out=pb --go_out=pb --validate_out="lang=go:pb" proto/*.proto
diff --git a/pb/service.pb.go b/pb/service.pb.go
index d5d6a05..a95a268 100644
--- a/pb/service.pb.go
+++ b/pb/service.pb.go
@@ -119,18 +119,17 @@ var File_proto_service_proto protoreflect.FileDescriptor

 var file_proto_service_proto_rawDesc = []byte{
    0x0a, 0x13, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x2f, 0x73, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x2e,
-   0x70, 0x72, 0x6f, 0x74, 0x6f, 0x12, 0x05, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x1a, 0x1d, 0x70, 0x72,
-   0x6f, 0x74, 0x6f, 0x2f, 0x76, 0x61, 0x6c, 0x69, 0x64, 0x61, 0x74, 0x65, 0x2f, 0x76, 0x61, 0x6c,
-   0x69, 0x64, 0x61, 0x74, 0x65, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x22, 0x24, 0x0a, 0x07, 0x43,
-   0x61, 0x6c, 0x6c, 0x52, 0x65, 0x71, 0x12, 0x19, 0x0a, 0x03, 0x6d, 0x73, 0x67, 0x18, 0x01, 0x20,
-   0x01, 0x28, 0x09, 0x42, 0x07, 0xfa, 0x42, 0x04, 0x72, 0x02, 0x10, 0x01, 0x52, 0x03, 0x6d, 0x73,
-   0x67, 0x22, 0x1a, 0x0a, 0x08, 0x43, 0x61, 0x6c, 0x6c, 0x52, 0x65, 0x73, 0x70, 0x12, 0x0e, 0x0a,
-   0x02, 0x6f, 0x6b, 0x18, 0x01, 0x20, 0x01, 0x28, 0x08, 0x52, 0x02, 0x6f, 0x6b, 0x32, 0x33, 0x0a,
-   0x06, 0x43, 0x65, 0x6e, 0x74, 0x65, 0x72, 0x12, 0x29, 0x0a, 0x04, 0x43, 0x61, 0x6c, 0x6c, 0x12,
-   0x0e, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x2e, 0x43, 0x61, 0x6c, 0x6c, 0x52, 0x65, 0x71, 0x1a,
-   0x0f, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x2e, 0x43, 0x61, 0x6c, 0x6c, 0x52, 0x65, 0x73, 0x70,
-   0x22, 0x00, 0x42, 0x06, 0x5a, 0x04, 0x2e, 0x3b, 0x70, 0x62, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74,
-   0x6f, 0x33,
+   0x70, 0x72, 0x6f, 0x74, 0x6f, 0x12, 0x05, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x1a, 0x17, 0x76, 0x61,
+   0x6c, 0x69, 0x64, 0x61, 0x74, 0x65, 0x2f, 0x76, 0x61, 0x6c, 0x69, 0x64, 0x61, 0x74, 0x65, 0x2e,
+   0x70, 0x72, 0x6f, 0x74, 0x6f, 0x22, 0x24, 0x0a, 0x07, 0x43, 0x61, 0x6c, 0x6c, 0x52, 0x65, 0x71,
+   0x12, 0x19, 0x0a, 0x03, 0x6d, 0x73, 0x67, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x42, 0x07, 0xfa,
+   0x42, 0x04, 0x72, 0x02, 0x10, 0x01, 0x52, 0x03, 0x6d, 0x73, 0x67, 0x22, 0x1a, 0x0a, 0x08, 0x43,
+   0x61, 0x6c, 0x6c, 0x52, 0x65, 0x73, 0x70, 0x12, 0x0e, 0x0a, 0x02, 0x6f, 0x6b, 0x18, 0x01, 0x20,
+   0x01, 0x28, 0x08, 0x52, 0x02, 0x6f, 0x6b, 0x32, 0x33, 0x0a, 0x06, 0x43, 0x65, 0x6e, 0x74, 0x65,
+   0x72, 0x12, 0x29, 0x0a, 0x04, 0x43, 0x61, 0x6c, 0x6c, 0x12, 0x0e, 0x2e, 0x70, 0x72, 0x6f, 0x74,
+   0x6f, 0x2e, 0x43, 0x61, 0x6c, 0x6c, 0x52, 0x65, 0x71, 0x1a, 0x0f, 0x2e, 0x70, 0x72, 0x6f, 0x74,
+   0x6f, 0x2e, 0x43, 0x61, 0x6c, 0x6c, 0x52, 0x65, 0x73, 0x70, 0x22, 0x00, 0x42, 0x06, 0x5a, 0x04,
+   0x2e, 0x3b, 0x70, 0x62, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33,
 }

 var (
diff --git a/proto/service.proto b/proto/service.proto
index c8446ee..ac71f92 100644
--- a/proto/service.proto
+++ b/proto/service.proto
@@ -2,7 +2,7 @@ syntax = "proto3";
 package proto;
 option go_package = ".;pb";

-import "proto/validate/validate.proto";
+import "validate/validate.proto";

 service Center {
   rpc Call (CallReq) returns (CallResp) {}
donkeywon commented 7 months ago

@jhump thanks for the reply, I will fix it

printfn commented 7 months ago

@jhump Thanks for the information, and yeah it does look like we’re seeing the same issue.

Would it maybe be possible to improve the error message in grpcurl, to better describe the issue? The error message I get on v1.8.7 (Failed to resolve symbol "servicepb.TestService": file "service.proto" included an unresolvable reference to ".sharedpb.HelloWorldResponse") is definitely a bit more useful than Failed to resolve symbol "servicepb.TestService": proto: invalid syntax: "<unknown:0>".

jhump commented 7 months ago

@printfn, well, the issue is the way the gRPC Go runtime returns descriptors in this case, from its reflection implementation. Instead of just returning a "not found" error for the missing file, it returns a placeholder descriptor that is invalid. While we could try to special-case this in grpcurl -- like have it look for this particular syntax string and report a different error when it's found -- it would be better IMO to fix the reflection endpoint so that it doesn't return junk descriptors in the first place.

With that fix, the error that grpcurl would report would be about the file not being found (in @donkeywon's example, it would complain about "proto/validate/validate.proto". Personally I think that is actually more clear than the error that v1.8.7 would show. The error v1.8.7 reports is because it basically ignored the invalid syntax. And since the placeholder file is otherwise empty, any symbols it contained that were used by the importing file would be missing (like sharedpb.HelloWorldResponse in your example).

jhump commented 7 months ago

However, the placeholder it is sending is invalid since it contains the literal string "<unknown:0>" as the value of the descriptor's syntax field (this could arguably be considered a bug in the Go protobuf runtime or the Go implementation of the gRPC reflection service).

I've filed https://github.com/grpc/grpc-go/issues/6770 and https://github.com/golang/protobuf/issues/1575 and also opened pull requests to try to fix these issues.