From 299d94262393c7656375b96050e4315cac926b7e Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sat, 11 May 2024 16:03:46 -0400 Subject: [PATCH] [grpc-storage] Use grpc.NewClient (#5393) ## Which problem is this PR solving? - Part of #5330 ## Description of the changes - use grpc.NewClient - add extra test ## How was this change tested? - CI --------- Signed-off-by: Yuri Shkuro --- plugin/storage/grpc/config/config.go | 15 ++++++--------- plugin/storage/grpc/config/config_test.go | 23 +++++++++++++++++++++++ plugin/storage/grpc/factory_test.go | 10 ++++------ 3 files changed, 33 insertions(+), 15 deletions(-) create mode 100644 plugin/storage/grpc/config/config_test.go diff --git a/plugin/storage/grpc/config/config.go b/plugin/storage/grpc/config/config.go index c22e9f9d6f1..fe96ee35582 100644 --- a/plugin/storage/grpc/config/config.go +++ b/plugin/storage/grpc/config/config.go @@ -15,7 +15,6 @@ package config import ( - "context" "fmt" "os/exec" "time" @@ -77,7 +76,7 @@ func (c *Configuration) Build(logger *zap.Logger, tracerProvider trace.TracerPro if c.PluginBinary != "" { return c.buildPlugin(logger, tracerProvider) } else { - return c.buildRemote(logger, tracerProvider) + return c.buildRemote(logger, tracerProvider, grpc.NewClient) } } @@ -93,7 +92,9 @@ func (c *Configuration) Close() error { return c.RemoteTLS.Close() } -func (c *Configuration) buildRemote(logger *zap.Logger, tracerProvider trace.TracerProvider) (*ClientPluginServices, error) { +type newClientFn func(target string, opts ...grpc.DialOption) (conn *grpc.ClientConn, err error) + +func (c *Configuration) buildRemote(logger *zap.Logger, tracerProvider trace.TracerProvider, newClient newClientFn) (*ClientPluginServices, error) { opts := []grpc.DialOption{ grpc.WithStatsHandler(otelgrpc.NewClientHandler(otelgrpc.WithTracerProvider(tracerProvider))), grpc.WithBlock(), @@ -109,19 +110,15 @@ func (c *Configuration) buildRemote(logger *zap.Logger, tracerProvider trace.Tra opts = append(opts, grpc.WithTransportCredentials(insecure.NewCredentials())) } - ctx, cancel := context.WithTimeout(context.Background(), c.RemoteConnectTimeout) - defer cancel() - tenancyMgr := tenancy.NewManager(&c.TenancyOpts) if tenancyMgr.Enabled { opts = append(opts, grpc.WithUnaryInterceptor(tenancy.NewClientUnaryInterceptor(tenancyMgr))) opts = append(opts, grpc.WithStreamInterceptor(tenancy.NewClientStreamInterceptor(tenancyMgr))) } var err error - // TODO: Need to replace grpc.DialContext with grpc.NewClient and pass test - c.remoteConn, err = grpc.DialContext(ctx, c.RemoteServerAddr, opts...) + c.remoteConn, err = newClient(c.RemoteServerAddr, opts...) if err != nil { - return nil, fmt.Errorf("error connecting to remote storage: %w", err) + return nil, fmt.Errorf("error creating remote storage client: %w", err) } grpcClient := shared.NewGRPCClient(c.remoteConn) diff --git a/plugin/storage/grpc/config/config_test.go b/plugin/storage/grpc/config/config_test.go new file mode 100644 index 00000000000..64f54e58418 --- /dev/null +++ b/plugin/storage/grpc/config/config_test.go @@ -0,0 +1,23 @@ +// Copyright (c) 2024 The Jaeger Authors. +// SPDX-License-Identifier: Apache-2.0 + +package config + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "google.golang.org/grpc" +) + +func TestBuildRemoteNewClientError(t *testing.T) { + // this is a silly test to verify handling of error from grpc.NewClient, which cannot be induced via params. + c := &Configuration{} + _, err := c.buildRemote(zap.NewNop(), nil, func(target string, opts ...grpc.DialOption) (conn *grpc.ClientConn, err error) { + return nil, errors.New("test error") + }) + require.Error(t, err) + require.Contains(t, err.Error(), "error creating remote storage client") +} diff --git a/plugin/storage/grpc/factory_test.go b/plugin/storage/grpc/factory_test.go index 2dae30fe202..3d357a1346d 100644 --- a/plugin/storage/grpc/factory_test.go +++ b/plugin/storage/grpc/factory_test.go @@ -148,10 +148,6 @@ func TestGRPCStorageFactory(t *testing.T) { } func TestGRPCStorageFactoryWithConfig(t *testing.T) { - cfg := grpcConfig.Configuration{} - _, err := NewFactoryWithConfig(cfg, metrics.NullFactory, zap.NewNop()) - require.ErrorContains(t, err, "grpc-plugin builder failed to create a store: error connecting to remote storage") - lis, err := net.Listen("tcp", ":0") require.NoError(t, err, "failed to listen") @@ -163,8 +159,10 @@ func TestGRPCStorageFactoryWithConfig(t *testing.T) { }() defer s.Stop() - cfg.RemoteServerAddr = lis.Addr().String() - cfg.RemoteConnectTimeout = 1 * time.Second + cfg := grpcConfig.Configuration{ + RemoteServerAddr: lis.Addr().String(), + RemoteConnectTimeout: 1 * time.Second, + } f, err := NewFactoryWithConfig(cfg, metrics.NullFactory, zap.NewNop()) require.NoError(t, err) require.NoError(t, f.Close())