tdex-network / tdex-daemon

Go implementation of the TDEX Beta Daemon
https://tdex.network
MIT License
11 stars 13 forks source link

Grpc reflection #680

Closed sekulicd closed 1 year ago

sekulicd commented 1 year ago

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[bot] commented 1 year ago
altafan commented 1 year ago

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 commented 1 year ago

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 https://github.com/whatwg/fetch/issues/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 commented 1 year ago

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 commented 1 year ago

@sekulicd solutions works too

sekulicd commented 1 year ago

@tiero @altafan please review.

Janaka-Steph commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

.. 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 commented 1 year ago

@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 commented 1 year ago
Screenshot 2023-03-09 at 11 21 06

Why not setting up Envoy?

tiero commented 1 year ago

Why not setting up Envoy?

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

tiero commented 1 year ago

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 commented 1 year ago

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?