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

Version 1.8.9 proto: invalid syntax #423

Open
EwanValentine opened this issue Oct 25, 2023 · 13 comments
Open

Version 1.8.9 proto: invalid syntax #423

EwanValentine opened this issue Oct 25, 2023 · 13 comments

Comments

@EwanValentine
Copy link

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

jhump commented Oct 25, 2023

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

@donkeywon
Copy link

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

jhump commented Oct 27, 2023

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

@printfn
Copy link

printfn commented Oct 31, 2023

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

jhump commented Oct 31, 2023

@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
Copy link
Author

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

@printfn
Copy link

printfn commented Nov 1, 2023

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

donkeywon commented Nov 2, 2023

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

jhump commented Nov 6, 2023

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

@jhump thanks for the reply, I will fix it

@printfn
Copy link

printfn commented Nov 7, 2023

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

jhump commented Nov 7, 2023

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

jhump commented Nov 7, 2023

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 grpc/grpc-go#6770 and golang/protobuf#1575 and also opened pull requests to try to fix these issues.

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

4 participants