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

Grpc reflection #680

Closed
wants to merge 5 commits into from
Closed

Conversation

sekulicd
Copy link
Collaborator

@sekulicd sekulicd commented Mar 3, 2023

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.

@what-the-diff
Copy link

what-the-diff bot commented Mar 3, 2023

  • Add Reflection to the list of entities in permissions.go
  • Register reflection service for both grpcOperatorServer and grpcTradeServer

@altafan
Copy link
Collaborator

altafan commented Mar 7, 2023

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.

@Janaka-Steph
Copy link

Bidirectional stream

Indeed the protocol is structured as a bidirectional stream. Here it describes why:
https://github.com/grpc/grpc/blob/master/doc/server-reflection.md#reverse-proxy-traversal
Mainly to ensure that all related requests go to a single server.

Client support

From https://github.com/deeplay-io/nice-grpc/tree/master/packages/nice-grpc-web#compatibility
"Most browsers do not support sending streams in fetch requests. This means that client streaming and bidirectional streaming will not work. The only browser that supports client streams is Chrome 105+ (and other Chromium-based browsers, see compatibility table), and only over http2, which in turn requires https. Client streams work in NodeJS native fetch implementation as well. Note, however, that fetch streams are currently whatwg/fetch#1254, which means that any response data will be buffered until the request stream is sent until the end. This unfortunately makes it impossible to use infinite bidirectional streaming. To overcome this limitation, it is recommended to design your API to use only unary and server streaming methods. If you still need to use client streams, you can use a Websocket transport with grpcwebproxy."

=> 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 browser

I 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 alpha

The PR is using v1 alpha but v1 has been released:
https://github.com/grpc/grpc/blob/master/src/proto/grpc/reflection/v1/reflection.proto

Conclusion

I 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.
I am not sure if we need a proxy.

@sekulicd
Copy link
Collaborator Author

sekulicd commented Mar 7, 2023

One option is that tdex-daemon exposes another unary RPC that would internally call reflection stream, PSB POC:

func (o operatorHandler) GetSvcInfo(
	ctx context.Context,
	request *daemonv1.GetSvcInfoRequest,
) (*daemonv1.GetSvcInfoResponse, error) {
	conn, err := grpc.Dial("127.0.0.1:9945", grpc.WithInsecure())
	if err != nil {
		return nil, err
	}
	defer conn.Close()

	stub := rpb.NewServerReflectionClient(conn)
	client, err := stub.ServerReflectionInfo(context.Background())
	if err != nil {
		return nil, err
	}
	defer client.CloseSend()

	if err := client.Send(&rpb.ServerReflectionRequest{
		MessageRequest: &rpb.ServerReflectionRequest_ListServices{},
	}); err != nil {
		return nil, err
	}

	resp, err := client.Recv()
	if err != nil {
		return nil, err
	}

	services := make([]string, 0, len(resp.GetListServicesResponse().Service))
	for _, service := range resp.GetListServicesResponse().Service {
		services = append(services, service.Name)
	}

	return &daemonv1.GetSvcInfoResponse{
		Services: services,
	}, nil
}

@tiero
Copy link
Collaborator

tiero commented Mar 7, 2023

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

sekulicd commented Mar 8, 2023

@tiero @altafan please review.

req *daemonv1.ListProtoServicesRequest,
) (*daemonv1.ListProtoServicesResponse, error) {
tlsConf := &tls.Config{
InsecureSkipVerify: true,

Check failure

Code scanning / CodeQL

Disabled TLS certificate check

InsecureSkipVerify should not be used in production code.
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

InsecureSkipVerify should not be used in production code.
@Janaka-Steph
Copy link

Janaka-Steph commented Mar 9, 2023

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.

@sekulicd
Copy link
Collaborator Author

sekulicd commented Mar 9, 2023

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.

@Janaka-Steph
Copy link

@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 OperatorServiceClient.

@sekulicd
Copy link
Collaborator Author

sekulicd commented Mar 9, 2023

.. goal here is to know which proto version a daemon is using. So List should be callable without instanciating a OperatorServiceClient.

I see, if i understand issue with using operator client is that u dont know version, right? @Janaka-Steph
If this is the true one option would be to put this code inside HTTP endpoint, but endpoint would be able to serve infos only after wallet is initialised.
If this is not acceptable then we need to re-implement reflection by ourself and expose it via HTTP that would be reachable even if wallet is not init.

@tiero @altafan

@Janaka-Steph
Copy link

@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.

@Janaka-Steph
Copy link

Screenshot 2023-03-09 at 11 21 06

Why not setting up Envoy?

Comment on lines +163 to +169
// Returns the list of all available proto services
rpc ListProtoServices(ListProtoServicesRequest) returns(ListProtoServicesResponse){}
}

message ListProtoServicesRequest{}
message ListProtoServicesResponse{
repeated string services = 1;
Copy link
Collaborator

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).

@tiero
Copy link
Collaborator

tiero commented Mar 9, 2023

Why not setting up Envoy?

We cant make assumption of the infrastructure where tdex daemons are run by other operators @Janaka-Steph

@tiero
Copy link
Collaborator

tiero commented Mar 9, 2023

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

@altafan
Copy link
Collaborator

altafan commented Mar 20, 2023

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 reflection.Register(grpcServer) on every protocol.

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?

@altafan altafan closed this Mar 20, 2023
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

Successfully merging this pull request may close these issues.

Reflection support
4 participants