-
Notifications
You must be signed in to change notification settings - Fork 13
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
Grpc reflection #680
Grpc reflection #680
Conversation
|
I just noticed that the reflection RPC is defined as a bidirectional stream and this would cause browser clients to not be able to make use of it basically. I found out this grpc-gateway wrapper that exposes bidi streams as websockets. It might be what we're looking for, but still, we should make changes to the reflection RPC to add google annotations for the gateway. Alternatively, we could just define our own reflection service with a unary RPC instead and related google annotations. @tiero @sekulicd @Janaka-Steph please give a feedback. |
Bidirectional streamIndeed the protocol is structured as a bidirectional stream. Here it describes why: Client supportFrom https://github.com/deeplay-io/nice-grpc/tree/master/packages/nice-grpc-web#compatibility => Here it says the API should use only unary and server streaming methods. But doesn't warn about bidirectional stream of Reflection protocol. So it's ok after all ? How to use reflection from browserI looked at how to call reflection endpoints from JS client. It can be done through @grpc/grpc-js directly or using libraries like https://github.com/redhoyasa/grpc-reflection-js and https://github.com/deeplay-io/nice-grpc Use reflection v1, not v1 alphaThe PR is using v1 alpha but v1 has been released: ConclusionI would try in regtest if it works with this PR (updated to latest v1 if possible) and https://github.com/redhoyasa/grpc-reflection-js on client. |
One option is that tdex-daemon exposes another unary RPC that would internally call reflection stream, PSB POC:
|
@sekulicd solutions works too |
Since ServerReflectionInfo is stream RPC browser clients would not be able to invoke it easily this adds unary RPC ListProtoServices that invokes ServerReflectionInfo internally
ctx context.Context, | ||
req *daemonv1.ListProtoServicesRequest, | ||
) (*daemonv1.ListProtoServicesResponse, error) { | ||
tlsConf := &tls.Config{InsecureSkipVerify: true} // nolint:gosec |
Check failure
Code scanning / CodeQL
Disabled TLS certificate check
I didn't get how the client should call List. It seems it still needs the protos to do that. But the purpose of List is to know which protos to use. Also the PR is still using Reflection v1 alpha. |
@Janaka-Steph with latest commit u can call regular unary RPC ListProtoServices which is internally using reflecation. |
@sekulicd Yes I get that, it is part of Operator proto. Again, the goal here is to know which proto version a daemon is using. So List should be callable without instanciating a |
I see, if i understand issue with using operator client is that u dont know version, right? @Janaka-Steph |
@sekulicd That's right. By the way, for that purpose of discovering daemon version I could also use a more hacky solution where I instanciate a v2 service and calling a method that only exist in v2. If it throw then the daemon is using protos v1. |
// Returns the list of all available proto services | ||
rpc ListProtoServices(ListProtoServicesRequest) returns(ListProtoServicesResponse){} | ||
} | ||
|
||
message ListProtoServicesRequest{} | ||
message ListProtoServicesResponse{ | ||
repeated string services = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this RPC must be moved to a dedicated proto into a ReflectionService
.
It can't stay here because it wouldn't be possible to call the reflection API without knowing the version of the operator proto (v1/v2).
We cant make assumption of the infrastructure where tdex daemons are run by other operators @Janaka-Steph |
I am in favor of keep it simple here, and return an HTTP endpoint (like we do for transport) that tella us the version of protos used byt trader and operator interface |
Let's make use of our fork of grpc-go/reflection that supports both grpc-web and gateway, making the reflection endpoint available with just one line I'm closing this since it's easier to just start from v1 branch for this one-line change instead of reverting all the work done here. @sekulicd can you open another PR please? |
This adds gRPC Server Reflection which provides information about RPC's.
To test locally:
grpcurl -plaintext 127.0.0.1:9945 list
grpcurl -insecure 127.0.0.1:9000 list
This closes #679
@altafan please review.