From eb7ca222b3ab59b14f924d9fc617117dd8341da8 Mon Sep 17 00:00:00 2001 From: Oleg Bespalov Date: Tue, 5 Sep 2023 10:04:41 +0200 Subject: [PATCH 1/2] Reflection v1 This introduces support of the reflaction v1 (and presume support of the reflection v1alpa) --- grpc/client_test.go | 30 +++++++++ lib/netext/grpcext/conn_test.go | 105 -------------------------------- lib/netext/grpcext/reflect.go | 92 +++++++--------------------- 3 files changed, 52 insertions(+), 175 deletions(-) diff --git a/grpc/client_test.go b/grpc/client_test.go index 5475649..4d161c3 100644 --- a/grpc/client_test.go +++ b/grpc/client_test.go @@ -516,6 +516,18 @@ func TestClient(t *testing.T) { code: `client.connect("GRPCBIN_ADDR", {reflect: true})`, }, }, + { + name: "ReflectV1", + setup: func(tb *httpmultibin.HTTPMultiBin) { + reflection.RegisterV1(tb.ServerGRPC) + }, + initString: codeBlock{ + code: `var client = new grpc.Client();`, + }, + vuString: codeBlock{ + code: `client.connect("GRPCBIN_ADDR", {reflect: true})`, + }, + }, { name: "ReflectBadParam", setup: func(tb *httpmultibin.HTTPMultiBin) { @@ -566,6 +578,24 @@ func TestClient(t *testing.T) { `, }, }, + { + name: "ReflectV1Invoke", + setup: func(tb *httpmultibin.HTTPMultiBin) { + reflection.RegisterV1(tb.ServerGRPC) + tb.GRPCStub.EmptyCallFunc = func(ctx context.Context, _ *grpc_testing.Empty) (*grpc_testing.Empty, error) { + return &grpc_testing.Empty{}, nil + } + }, + initString: codeBlock{ + code: `var client = new grpc.Client();`, + }, + vuString: codeBlock{ + code: ` + client.connect("GRPCBIN_ADDR", {reflect: true}) + client.invoke("grpc.testing.TestService/EmptyCall", {}) + `, + }, + }, { name: "MaxReceiveSizeBadParam", setup: func(tb *httpmultibin.HTTPMultiBin) { diff --git a/lib/netext/grpcext/conn_test.go b/lib/netext/grpcext/conn_test.go index 4afcc3e..fe418d7 100644 --- a/lib/netext/grpcext/conn_test.go +++ b/lib/netext/grpcext/conn_test.go @@ -5,7 +5,6 @@ import ( "context" "fmt" "io" - "sync/atomic" "testing" "github.com/jhump/protoreflect/desc/protoparse" @@ -14,12 +13,9 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" - reflectpb "google.golang.org/grpc/reflection/grpc_reflection_v1alpha" "google.golang.org/protobuf/encoding/protojson" - "google.golang.org/protobuf/proto" "google.golang.org/protobuf/reflect/protodesc" "google.golang.org/protobuf/reflect/protoreflect" - "google.golang.org/protobuf/types/descriptorpb" "google.golang.org/protobuf/types/dynamicpb" ) @@ -157,107 +153,6 @@ func TestConnInvokeInvalid(t *testing.T) { } } -func TestResolveFileDescriptors(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - pkgs []string - services []string - expectedDescriptors int - }{ - { - name: "SuccessSamePackage", - pkgs: []string{"mypkg"}, - services: []string{"Service1", "Service2", "Service3"}, - expectedDescriptors: 3, - }, - { - name: "SuccessMultiPackages", - pkgs: []string{"mypkg1", "mypkg2", "mypkg3"}, - services: []string{"Service", "Service", "Service"}, - expectedDescriptors: 3, - }, - { - name: "DeduplicateServices", - pkgs: []string{"mypkg1"}, - services: []string{"Service1", "Service2", "Service1"}, - expectedDescriptors: 2, - }, - { - name: "NoServices", - services: []string{}, - expectedDescriptors: 0, - }, - } - - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - var ( - lsr = &reflectpb.ListServiceResponse{} - mock = &getServiceFileDescriptorMock{} - ) - for i, service := range tt.services { - // if only one package is defined then - // the package is the same for every service - pkg := tt.pkgs[0] - if len(tt.pkgs) > 1 { - pkg = tt.pkgs[i] - } - - lsr.Service = append(lsr.Service, &reflectpb.ServiceResponse{ - Name: fmt.Sprintf("%s.%s", pkg, service), - }) - mock.pkgs = append(mock.pkgs, pkg) - mock.names = append(mock.names, service) - } - - rc := reflectionClient{} - fdset, err := rc.resolveServiceFileDescriptors(mock, lsr) - require.NoError(t, err) - assert.Len(t, fdset.File, tt.expectedDescriptors) - }) - } -} - -type getServiceFileDescriptorMock struct { - pkgs []string - names []string - nreqs int64 -} - -func (m *getServiceFileDescriptorMock) Send(_ *reflectpb.ServerReflectionRequest) error { - // TODO: check that the sent message is expected, - // otherwise return an error - return nil -} - -func (m *getServiceFileDescriptorMock) Recv() (*reflectpb.ServerReflectionResponse, error) { - n := atomic.AddInt64(&m.nreqs, 1) - ptr := func(s string) (sptr *string) { - return &s - } - index := n - 1 - fdp := &descriptorpb.FileDescriptorProto{ - Package: ptr(m.pkgs[index]), - Name: ptr(m.names[index]), - } - b, err := proto.Marshal(fdp) - if err != nil { - return nil, err - } - srr := &reflectpb.ServerReflectionResponse{ - MessageResponse: &reflectpb.ServerReflectionResponse_FileDescriptorResponse{ - FileDescriptorResponse: &reflectpb.FileDescriptorResponse{ - FileDescriptorProto: [][]byte{b}, - }, - }, - } - return srr, nil -} - func methodFromProto(method string) protoreflect.MethodDescriptor { path := "any-path" parser := protoparse.Parser{ diff --git a/lib/netext/grpcext/reflect.go b/lib/netext/grpcext/reflect.go index 6bba926..548e2ae 100644 --- a/lib/netext/grpcext/reflect.go +++ b/lib/netext/grpcext/reflect.go @@ -4,9 +4,9 @@ import ( "context" "fmt" + "github.com/jhump/protoreflect/desc" + "github.com/jhump/protoreflect/grpcreflect" "google.golang.org/grpc" - reflectpb "google.golang.org/grpc/reflection/grpc_reflection_v1alpha" - "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/descriptorpb" ) @@ -18,59 +18,37 @@ type reflectionClient struct { // Reflect will use the grpc reflection api to make the file descriptors available to request. // It is called in the connect function the first time the Client.Connect function is called. func (rc *reflectionClient) Reflect(ctx context.Context) (*descriptorpb.FileDescriptorSet, error) { - client := reflectpb.NewServerReflectionClient(rc.Conn) - methodClient, err := client.ServerReflectionInfo(ctx) - if err != nil { - return nil, fmt.Errorf("can't get server info: %w", err) - } - req := &reflectpb.ServerReflectionRequest{ - MessageRequest: &reflectpb.ServerReflectionRequest_ListServices{}, - } - resp, err := sendReceive(methodClient, req) + client := grpcreflect.NewClientAuto(ctx, rc.Conn) + + services, err := client.ListServices() if err != nil { return nil, fmt.Errorf("can't list services: %w", err) } - listResp := resp.GetListServicesResponse() - if listResp == nil { - return nil, fmt.Errorf("can't list services, nil response") - } - fdset, err := rc.resolveServiceFileDescriptors(methodClient, listResp) - if err != nil { - return nil, fmt.Errorf("can't resolve services' file descriptors: %w", err) - } - return fdset, nil -} -func (rc *reflectionClient) resolveServiceFileDescriptors( - client sendReceiver, - res *reflectpb.ListServiceResponse, -) (*descriptorpb.FileDescriptorSet, error) { - services := res.GetService() seen := make(map[fileDescriptorLookupKey]bool, len(services)) fdset := &descriptorpb.FileDescriptorSet{ File: make([]*descriptorpb.FileDescriptorProto, 0, len(services)), } - for _, service := range services { - req := &reflectpb.ServerReflectionRequest{ - MessageRequest: &reflectpb.ServerReflectionRequest_FileContainingSymbol{ - FileContainingSymbol: service.GetName(), - }, - } - resp, err := sendReceive(client, req) + for _, srv := range services { + srvDescriptor, err := client.ResolveService(srv) if err != nil { - return nil, fmt.Errorf("can't get method on service %q: %w", service, err) + return nil, fmt.Errorf("can't get method on service %q: %w", srv, err) } - fdResp := resp.GetFileDescriptorResponse() - for _, raw := range fdResp.GetFileDescriptorProto() { - var fdp descriptorpb.FileDescriptorProto - if err = proto.Unmarshal(raw, &fdp); err != nil { - return nil, fmt.Errorf("can't unmarshal proto on service %q: %w", service, err) - } + + stack := []*desc.FileDescriptor{srvDescriptor.GetFile()} + + for len(stack) > 0 { + fdp := stack[len(stack)-1] + stack = stack[:len(stack)-1] + fdkey := fileDescriptorLookupKey{ - Package: *fdp.Package, - Name: *fdp.Name, + Package: fdp.GetPackage(), + Name: fdp.GetName(), } + + stack = append(stack, fdp.GetDependencies()...) + if seen[fdkey] { // When a proto file contains declarations for multiple services // then the same proto file is returned multiple times, @@ -78,37 +56,11 @@ func (rc *reflectionClient) resolveServiceFileDescriptors( continue } seen[fdkey] = true - fdset.File = append(fdset.File, &fdp) + fdset.File = append(fdset.File, fdp.AsFileDescriptorProto()) } } - return fdset, nil -} -// sendReceiver is a smaller interface for decoupling -// from `reflectpb.ServerReflection_ServerReflectionInfoClient`, -// that has the dependency from `grpc.ClientStream`, -// which is too much in the case the requirement is to just make a reflection's request. -// It makes the API more restricted and with a controlled surface, -// in this way the testing should be easier also. -type sendReceiver interface { - Send(*reflectpb.ServerReflectionRequest) error - Recv() (*reflectpb.ServerReflectionResponse, error) -} - -// sendReceive sends a request to a reflection client and, -// receives a response. -func sendReceive( - client sendReceiver, - req *reflectpb.ServerReflectionRequest, -) (*reflectpb.ServerReflectionResponse, error) { - if err := client.Send(req); err != nil { - return nil, fmt.Errorf("can't send request: %w", err) - } - resp, err := client.Recv() - if err != nil { - return nil, fmt.Errorf("can't receive response: %w", err) - } - return resp, nil + return fdset, nil } type fileDescriptorLookupKey struct { From 48dddc3005c9fbc2fd9a52990560790535a29f69 Mon Sep 17 00:00:00 2001 From: Oleg Bespalov Date: Tue, 5 Sep 2023 15:27:34 +0200 Subject: [PATCH 2/2] Add reflection v1alpha test case --- grpc/client_test.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/grpc/client_test.go b/grpc/client_test.go index 4d161c3..b617e7e 100644 --- a/grpc/client_test.go +++ b/grpc/client_test.go @@ -23,6 +23,7 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" + v1alphagrpc "google.golang.org/grpc/reflection/grpc_reflection_v1alpha" grpcstats "google.golang.org/grpc/stats" "google.golang.org/grpc/status" @@ -563,7 +564,31 @@ func TestClient(t *testing.T) { { name: "ReflectInvoke", setup: func(tb *httpmultibin.HTTPMultiBin) { + // this register both reflection APIs v1 and v1alpha reflection.Register(tb.ServerGRPC) + + tb.GRPCStub.EmptyCallFunc = func(ctx context.Context, _ *grpc_testing.Empty) (*grpc_testing.Empty, error) { + return &grpc_testing.Empty{}, nil + } + }, + initString: codeBlock{ + code: `var client = new grpc.Client();`, + }, + vuString: codeBlock{ + code: ` + client.connect("GRPCBIN_ADDR", {reflect: true}) + client.invoke("grpc.testing.TestService/EmptyCall", {}) + `, + }, + }, + { + name: "ReflectV1Alpha_Invoke", + setup: func(tb *httpmultibin.HTTPMultiBin) { + // this register only v1alpha (this could be removed with removal v1alpha from grpc-go) + s := tb.ServerGRPC + svr := reflection.NewServer(reflection.ServerOptions{Services: s}) + v1alphagrpc.RegisterServerReflectionServer(s, svr) + tb.GRPCStub.EmptyCallFunc = func(ctx context.Context, _ *grpc_testing.Empty) (*grpc_testing.Empty, error) { return &grpc_testing.Empty{}, nil } @@ -581,7 +606,9 @@ func TestClient(t *testing.T) { { name: "ReflectV1Invoke", setup: func(tb *httpmultibin.HTTPMultiBin) { + // this register only reflection APIs v1 reflection.RegisterV1(tb.ServerGRPC) + tb.GRPCStub.EmptyCallFunc = func(ctx context.Context, _ *grpc_testing.Empty) (*grpc_testing.Empty, error) { return &grpc_testing.Empty{}, nil }