Reflection library is tightly coupled with golang/protobuf

Question

Reflection library is tightly coupled with golang/protobuf

johanbrandhorst opened this issue 7 years ago · 20 comments

johanbrandhorst commented 7 years ago

What version of gRPC are you using?

gRPC 1.10

What version of Go are you using (`go version`)?

go version go1.10 linux/amd64

What operating system (Linux, Windows, …) and version?

$ uname -a
Linux johan-x1 4.15.3-2-ARCH #1 SMP PREEMPT Thu Feb 15 00:13:49 UTC 2018 x86_64 GNU/Linux

What did you do?

I tried using grpc/reflection library with gogo/protobuf.

What did you expect to see?

I expected to be able to use an alternative to golang/protobuf with gRPC reflection.

What did you see instead?

I found that the gRPC reflection library imports github.com/golang/protobuf. I also found it is tightly coupled to this implementation. As I understand, gRPC is meant to be codec agnostic, what with official support for flatbuffers, and the nice Codec interface existing in grpc-go. Given this, it is surprising that it is so tighly coupled to golang/protobuf, and perhaps by extension so coupled to protobuf itself.

I propose some sort of interface is defined in the reflection library so that a user can choose which backend to make use of for resolving protobuf types. As I see it, the reflection library makes use of the following functions (illustrated as an interface) from golang/protobuf:

type Protoer interface {
    MessageType(string) reflect.Type
    FileDescriptor(string) []byte
    Unmarshal([]byte, proto.Message) error
    Marshal(proto.Message) ([]byte, error)
    RegisteredExtensions(proto.Message) map[int32]*proto.ExtensionDesc
}

Ideally, I'd like to be able to use gogo/protobuf with gRPC reflection, but the current implementation would force me to reimplement the reflection server in gogo/protobuf manually.

Any thoughts on this greatly appreciated.

Answer 1 · 2018-02-21T01:11:42.000Z

This proposal SGTM.

Assuming this interface will be defined with golang/protobuf. The gogo/protobuf implementation would also need to take golang/protobuf.Message as input, and also return golang/proto.ExtensionDesc.
I'm not familiar with those. Will it be easy to convert between golang/protobuf types and gogo/protobuf types?

Some other thoughts:
Another possible abstraction is to separate the reflection service handler with the layer that provides the data. The reflection protocol is designed for protobuf, for example extension, but it's still potentially useful for other codecs.

This would also require the underlying message interchange format to support similar features as protobuf's file descriptor.
We can hold this and implement the proto only change you suggested first.

Answer 2 · 2018-02-21T09:33:05.000Z

@menghanl Thanks for your reply. The nested nature of the proto.Message definition is a problem, as gogo/protobuf has managed to avoid importing anything defined in golang/protobuf thus far. This could still be solved with a client compatibility layer wrapping a Protoer implementation in gogo/protobuf since any gogo/protobuf.Message and golang/protobuf.Message will be functionally equivalent, if technically separate types.

Inviting @awalterschulze (maintainer of gogo/protobuf) to chime in.

Answer 3 · 2018-02-22T15:01:22.000Z

Totally, gogo/protobuf/proto would like to avoid importing golang/protobuf/proto, because currently gogo/protobuf has zero dependencies and would like to keep it that way.
Also some users, including (past) me would prefer to have one implementation of the proto library as a dependency. This also becomes really hard with grpc-go.

A few years ago, when I was a user, I used to take a git subtree of grpc-go and change all the import paths from github.com/golang/protobuf/proto to github.com/gogo/protobuf/proto.
That way goimports can automatically resolve to the correct import.

So maybe I would ask, is it possible to create an interface that avoids importing any serialization library, including flatbuffers?

For two reasons:

The reason above
golang interfaces don't really nest well, which means that you must import interface A if you want to implement interface B which has a method that uses interface A, which goes against the whole implicit interface goal in my opinion, but thats a separate issue.

Answer 4 · 2018-04-23T22:46:02.000Z

@awalterschulze, I'm porting github.com/jhump/protoreflect to gogo and have an experimental branch with a working protoer interface along the lines of what @johanbrandhorst suggested with zero dependencies: https://github.com/prasek/protoreflect/blob/protoer/proto/protoer.go

The gogo and golang wrappers implement UntypedProtoer
https://github.com/prasek/protoreflect/blob/protoer/proto/gogo/protoer.go
https://github.com/prasek/protoreflect/blob/protoer/proto/golang/protoer.go

type Protoer interface {
	MessageType(name string) reflect.Type
	MessageName(pb Message) string
	FileDescriptor(file string) []byte
	Unmarshal(b []byte, pb Message) error
	Marshal(pb Message) ([]byte, error)
	GetExtension(pb Message, field int32) (extval interface{}, err error)
	EnsureNativeMessage(pb Message) (pbout Message, err error)
}

type Message interface {
	Reset()
	String() string
	ProtoMessage()
}

type UntypedProtoer interface {
	MessageType(name string) reflect.Type
	MessageName(pb interface{}) string
	FileDescriptor(file string) []byte
	Unmarshal(b []byte, pb interface{}) error
	Marshal(pb interface{}) ([]byte, error)
	GetExtension(pb interface{}, field int32) (extval interface{}, err error)
	EnsureNativeMessage(pb interface{}) (pbout interface{}, err error)
}

Answer 5 · 2018-04-24T08:10:46.000Z

@prasek fantastic work! What would be required to integrate this with gogo/golang/gRPC? I'm guessing we'd need these wrappers added to each implementation? I'm sure @dsnet will have opinions on this as well.

Answer 6 · 2018-04-24T09:59:15.000Z

Cool :)

May I ask what is the use case for EnsureNativeMessage?

Answer 7 · 2018-04-24T19:05:36.000Z

@awalterschulze, EnsureNativeMessage should really be called NativeDescriptor. There are two use cases for NativeDescriptor, one internal to the wrappers to ensure GetExtension() and RegisteredExtensions() get types they recognize, and the other allows libraries to accept either gogo or golang descriptor types by converting them with NativeDescriptor of a separate protoer instance that matches the package used by the library.

For example https://github.com/prasek/protoreflect/blob/protoer/desc/builder.go#L19 (experimental branch) accepts gogo and golang descriptors for some methods but internally has to pick one to use. NativeDescriptor() checks the descriptor type (gogo or golang) and converts it if not the desired type so it will work with descriptor types (gogo or golang) used by the library. The downside to this approach is descriptors are exported as Message and have to be type asserted by the caller, so I'd be fine removing NativeDescriptor from the Protoer interface and just using it internally in the wrappers.

Answer 8 · 2018-04-24T19:12:38.000Z

@johanbrandhorst, ideally the packages would get split into grpc, golang/protobuf, and gogo/protobuf respectively. There are no dependencies across the packages.

grpc: https://github.com/prasek/protoreflect/blob/protoer/proto/protoer.go
golang/protobuf: https://github.com/prasek/protoreflect/blob/protoer/proto/golang/protoer.go
gogo/protobuf: https://github.com/prasek/protoreflect/blob/protoer/proto/gogo/protoer.go

The only difference between the gogo and golang protobuf protoer are the imports and default aliases.

Answer 9 · 2018-04-25T00:03:09.000Z

Here are the updated interfaces with RegisteredExtensions added and NativeDescriptor removed.
https://github.com/prasek/protoreflect/blob/protoer/proto/protoer.go

type Protoer interface {
	MessageType(name string) reflect.Type
	MessageName(pb Message) string
	FileDescriptor(file string) []byte
	Unmarshal(b []byte, pb Message) error
	Marshal(pb Message) ([]byte, error)
	RegisteredExtensions(pb Message, desiredType interface{}) (extensions interface{}, err error)
	GetExtension(pb Message, field int32) (extval interface{}, err error)
}

type Message interface {
	Reset()
	String() string
	ProtoMessage()
}

type UntypedProtoer interface {
	MessageType(name string) reflect.Type
	MessageName(pb interface{}) string
	FileDescriptor(file string) []byte
	Unmarshal(b []byte, pb interface{}) error
	Marshal(pb interface{}) ([]byte, error)
	RegisteredExtensions(pb interface{}, desiredType interface{}) (interface{}, error)
	GetExtension(pb interface{}, field int32) (extval interface{}, err error)
}

Note for RegisteredExtensions() the desiredType is needed to check and convert if needed.

extensions, err := proto.RegisteredExtensions(pb, (map[int32]*gogo.ExtensionDesc)(nil))

Answer 10 · 2018-04-25T00:48:08.000Z

A protobuf reflection API that has Unmarshal and Marshal in its interface is probably too high level. Just as Go reflection is abstraction around the Go language itself, protobuf reflection should be an abstraction around the protobuf language itself. As such, it needs to go even lower-level than Unmarshal and Marshal and provide behavior at the field level in order to be reflexive over all aspect of the proto language.

Adding reflection functionality into the mainline golang/protobuf repo has been a project I've been working on for some time now and turns out to be a difficult task as there are many devils in the details. A reflection API needs to thought in the context of the totality of how Go protobufs operates. As I've been working on a reflection API, I've had to scrap my design and start over many times.

I recently finished writing up the design and published some documents regarding the design: golang/protobuf#364 (comment)

Answer 11 · 2019-08-21T11:08:30.000Z

I realize this is an old-ish issue, but wanted to check if the root cause of an issue I observed is the result of this coupling.

I implemented the Envoy v2 external auth service (proto file and generated Go using gogo/protobuf). When using grpcurl or grpc_cli, I can list the services, e.g.

% grpc_cli ls localhost:9001
envoy.service.auth.v2.Authorization
grpc.reflection.v1alpha.ServerReflection

However when I attempt to describe/call the service, I get a "not found" result:

% grpc_cli ls localhost:9001 envoy.service.auth.v2.Authorization -l
Service or method envoy.service.auth.v2.Authorization not found.

Answer 12 · 2019-08-21T11:13:01.000Z

I think you'd get an error about trying to lookup a file if it was a gogoproto problem, but it could well be, I haven't tried using reflection with gogoproto recently. I suppose it'd be worthwhile trying to generate with golang/protobuf and see if that works?

Answer 13 · 2019-08-21T12:02:55.000Z

I tried the gogo plugins with a simpler service to build (the Envoy one has quite a few dependencies). It appears that with the protoc-gen-gofast plugin, everything works fine. However, with the protoc-gen-gogofast plugin I get the same behavior and message as above. The build process for Envoy uses gogofast.

Answer 14 · 2019-08-21T12:07:32.000Z

That result is consistent with a gogoproto registry issue - gofast uses golang/protobuf. Sorry :(.

Answer 15 · 2019-08-21T12:10:20.000Z

The "registry issue" being the gogo types are not registered with the standard proto registry that the reflection service relies on? (sorry I am fairly new to gogo and getting up to speed on the incompatibilities.)

Answer 16 · 2019-08-21T12:15:07.000Z

That's right - I wrote a blog post on the topic of gogoprotobuf compatibility that should cover it: https://jbrandhorst.com/post/gogoproto/.

Answer 17 · 2019-08-21T12:18:03.000Z

Ah yes. I read this a while ago (thank you for writing it), but never dove into gogo after that until now. I see the relevant section:

Unfortunately, gogo/protobuf is currently not working perfectly with server reflection, because the grpc-go implementation is very tightly coupled with golang/protobuf

Answer 18 · 2019-09-06T15:36:55.000Z

This issue is labeled as requiring an update from the reporter, and no update has been received after 7 days. If no update is provided in the next 7 days, this issue will be automatically closed.

Answer 19 · 2019-09-06T15:41:19.000Z

Still a problem

Answer 20 · 2021-05-03T21:38:16.000Z

Closing due to lack of activity and priority. If someone feels strongly about this, please write a concrete proposal & commit to implementing a solution, and we can do reviews.