From 99eac21503deac2cf42f5ec04695f322034fff5a Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Thu, 12 Dec 2024 21:15:13 -0500 Subject: [PATCH] Upgrade agent grpc reporter Signed-off-by: Yuri Shkuro --- cmd/agent/app/reporter/grpc/builder.go | 19 ++--- cmd/agent/app/reporter/grpc/builder_test.go | 73 ++++++++++--------- .../app/reporter/grpc/collector_proxy.go | 16 ++-- cmd/agent/app/reporter/grpc/flags.go | 5 +- 4 files changed, 57 insertions(+), 56 deletions(-) diff --git a/cmd/agent/app/reporter/grpc/builder.go b/cmd/agent/app/reporter/grpc/builder.go index a8953a8e644..939b82156ae 100644 --- a/cmd/agent/app/reporter/grpc/builder.go +++ b/cmd/agent/app/reporter/grpc/builder.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/retry" + "go.opentelemetry.io/collector/config/configtls" "go.uber.org/zap" "google.golang.org/grpc" "google.golang.org/grpc/connectivity" @@ -19,7 +20,6 @@ import ( "google.golang.org/grpc/resolver/manual" "github.com/jaegertracing/jaeger/cmd/agent/app/reporter" - "github.com/jaegertracing/jaeger/pkg/config/tlscfg" "github.com/jaegertracing/jaeger/pkg/discovery" "github.com/jaegertracing/jaeger/pkg/discovery/grpcresolver" "github.com/jaegertracing/jaeger/pkg/metrics" @@ -30,13 +30,11 @@ import ( type ConnBuilder struct { // CollectorHostPorts is list of host:port Jaeger Collectors. CollectorHostPorts []string `yaml:"collectorHostPorts"` - - MaxRetry uint - TLS tlscfg.Options - - DiscoveryMinPeers int - Notifier discovery.Notifier - Discoverer discovery.Discoverer + TLS *configtls.ClientConfig + MaxRetry uint + DiscoveryMinPeers int + Notifier discovery.Notifier + Discoverer discovery.Discoverer AdditionalDialOptions []grpc.DialOption } @@ -50,13 +48,12 @@ func NewConnBuilder() *ConnBuilder { func (b *ConnBuilder) CreateConnection(ctx context.Context, logger *zap.Logger, mFactory metrics.Factory) (*grpc.ClientConn, error) { var dialOptions []grpc.DialOption var dialTarget string - if b.TLS.Enabled { // user requested a secure connection + if b.TLS != nil { // user requested a secure connection logger.Info("Agent requested secure grpc connection to collector(s)") - tlsConf, err := b.TLS.Config(logger) + tlsConf, err := b.TLS.LoadTLSConfig(ctx) if err != nil { return nil, fmt.Errorf("failed to load TLS config: %w", err) } - creds := credentials.NewTLS(tlsConf) dialOptions = append(dialOptions, grpc.WithTransportCredentials(creds)) } else { // insecure connection diff --git a/cmd/agent/app/reporter/grpc/builder_test.go b/cmd/agent/app/reporter/grpc/builder_test.go index 1643fb2960a..a8a9cf952a5 100644 --- a/cmd/agent/app/reporter/grpc/builder_test.go +++ b/cmd/agent/app/reporter/grpc/builder_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.opentelemetry.io/collector/config/configtls" "go.uber.org/zap" "google.golang.org/grpc" "google.golang.org/grpc/credentials" @@ -151,9 +152,10 @@ func TestProxyBuilder(t *testing.T) { name: "should fail with secure grpc connection and a CA file which does not exist", grpcBuilder: &ConnBuilder{ CollectorHostPorts: []string{"localhost:0000"}, - TLS: tlscfg.Options{ - Enabled: true, - CAPath: testCertKeyLocation + "/not/valid", + TLS: &configtls.ClientConfig{ + Config: configtls.Config{ + CAFile: testCertKeyLocation + "/not/valid", + }, }, }, expectError: true, @@ -162,11 +164,12 @@ func TestProxyBuilder(t *testing.T) { name: "should pass with secure grpc connection and valid TLS Client settings", grpcBuilder: &ConnBuilder{ CollectorHostPorts: []string{"localhost:0000"}, - TLS: tlscfg.Options{ - Enabled: true, - CAPath: testCertKeyLocation + "/wrong-CA-cert.pem", - CertPath: testCertKeyLocation + "/example-client-cert.pem", - KeyPath: testCertKeyLocation + "/example-client-key.pem", + TLS: &configtls.ClientConfig{ + Config: configtls.Config{ + CAFile: testCertKeyLocation + "/wrong-CA-cert.pem", + CertFile: testCertKeyLocation + "/example-client-cert.pem", + KeyFile: testCertKeyLocation + "/example-client-key.pem", + }, }, }, expectError: false, @@ -197,7 +200,7 @@ func TestProxyBuilder(t *testing.T) { func TestProxyClientTLS(t *testing.T) { tests := []struct { name string - clientTLS tlscfg.Options + clientTLS *configtls.ClientConfig serverTLS tlscfg.Options expectError bool }{ @@ -207,7 +210,7 @@ func TestProxyClientTLS(t *testing.T) { }, { name: "should fail with TLS client to non-TLS server", - clientTLS: tlscfg.Options{Enabled: true}, + clientTLS: &configtls.ClientConfig{}, expectError: true, }, { @@ -217,8 +220,7 @@ func TestProxyClientTLS(t *testing.T) { CertPath: testCertKeyLocation + "/example-server-cert.pem", KeyPath: testCertKeyLocation + "/example-server-key.pem", }, - clientTLS: tlscfg.Options{ - Enabled: true, + clientTLS: &configtls.ClientConfig{ ServerName: "example.com", }, expectError: true, @@ -230,9 +232,10 @@ func TestProxyClientTLS(t *testing.T) { CertPath: testCertKeyLocation + "/example-server-cert.pem", KeyPath: testCertKeyLocation + "/example-server-key.pem", }, - clientTLS: tlscfg.Options{ - Enabled: true, - CAPath: testCertKeyLocation + "/example-CA-cert.pem", + clientTLS: &configtls.ClientConfig{ + Config: configtls.Config{ + CAFile: testCertKeyLocation + "/example-CA-cert.pem", + }, }, expectError: true, }, @@ -243,9 +246,10 @@ func TestProxyClientTLS(t *testing.T) { CertPath: testCertKeyLocation + "/example-server-cert.pem", KeyPath: testCertKeyLocation + "/example-server-key.pem", }, - clientTLS: tlscfg.Options{ - Enabled: true, - CAPath: testCertKeyLocation + "/example-CA-cert.pem", + clientTLS: &configtls.ClientConfig{ + Config: configtls.Config{ + CAFile: testCertKeyLocation + "/example-CA-cert.pem", + }, ServerName: "example.com", }, expectError: false, @@ -258,9 +262,10 @@ func TestProxyClientTLS(t *testing.T) { KeyPath: testCertKeyLocation + "/example-server-key.pem", ClientCAPath: testCertKeyLocation + "/example-CA-cert.pem", }, - clientTLS: tlscfg.Options{ - Enabled: true, - CAPath: testCertKeyLocation + "/example-CA-cert.pem", + clientTLS: &configtls.ClientConfig{ + Config: configtls.Config{ + CAFile: testCertKeyLocation + "/example-CA-cert.pem", + }, ServerName: "example.com", }, expectError: true, @@ -273,12 +278,13 @@ func TestProxyClientTLS(t *testing.T) { KeyPath: testCertKeyLocation + "/example-server-key.pem", ClientCAPath: testCertKeyLocation + "/wrong-CA-cert.pem", // NB: wrong CA }, - clientTLS: tlscfg.Options{ - Enabled: true, - CAPath: testCertKeyLocation + "/example-CA-cert.pem", + clientTLS: &configtls.ClientConfig{ + Config: configtls.Config{ + CAFile: testCertKeyLocation + "/example-CA-cert.pem", + CertFile: testCertKeyLocation + "/example-client-cert.pem", + KeyFile: testCertKeyLocation + "/example-client-key.pem", + }, ServerName: "example.com", - CertPath: testCertKeyLocation + "/example-client-cert.pem", - KeyPath: testCertKeyLocation + "/example-client-key.pem", }, expectError: true, }, @@ -290,12 +296,13 @@ func TestProxyClientTLS(t *testing.T) { KeyPath: testCertKeyLocation + "/example-server-key.pem", ClientCAPath: testCertKeyLocation + "/example-CA-cert.pem", }, - clientTLS: tlscfg.Options{ - Enabled: true, - CAPath: testCertKeyLocation + "/example-CA-cert.pem", + clientTLS: &configtls.ClientConfig{ + Config: configtls.Config{ + CAFile: testCertKeyLocation + "/example-CA-cert.pem", + CertFile: testCertKeyLocation + "/example-client-cert.pem", + KeyFile: testCertKeyLocation + "/example-client-key.pem", + }, ServerName: "example.com", - CertPath: testCertKeyLocation + "/example-client-cert.pem", - KeyPath: testCertKeyLocation + "/example-client-key.pem", }, expectError: false, }, @@ -308,12 +315,10 @@ func TestProxyClientTLS(t *testing.T) { defer cancel() var opts []grpc.ServerOption if test.serverTLS.Enabled { - tlsCfg, err := test.serverTLS.Config(zap.NewNop()) + tlsCfg, err := test.serverTLS.ToOtelServerConfig().LoadTLSConfig(ctx) require.NoError(t, err) opts = []grpc.ServerOption{grpc.Creds(credentials.NewTLS(tlsCfg))} } - - defer test.serverTLS.Close() spanHandler := &mockSpanHandler{} s, addr := initializeGRPCTestServer(t, func(s *grpc.Server) { api_v2.RegisterCollectorServiceServer(s, spanHandler) diff --git a/cmd/agent/app/reporter/grpc/collector_proxy.go b/cmd/agent/app/reporter/grpc/collector_proxy.go index 554926459b0..bb7b3ff64e8 100644 --- a/cmd/agent/app/reporter/grpc/collector_proxy.go +++ b/cmd/agent/app/reporter/grpc/collector_proxy.go @@ -6,7 +6,6 @@ package grpc import ( "context" "errors" - "io" "go.uber.org/zap" "google.golang.org/grpc" @@ -19,10 +18,9 @@ import ( // ProxyBuilder holds objects communicating with collector type ProxyBuilder struct { - reporter *reporter.ClientMetricsReporter - manager configmanager.ClientConfigManager - conn *grpc.ClientConn - tlsCloser io.Closer + reporter *reporter.ClientMetricsReporter + manager configmanager.ClientConfigManager + conn *grpc.ClientConn } // NewCollectorProxy creates ProxyBuilder @@ -40,10 +38,9 @@ func NewCollectorProxy(ctx context.Context, builder *ConnBuilder, agentTags map[ MetricsFactory: mFactory, }) return &ProxyBuilder{ - conn: conn, - reporter: r3, - manager: configmanager.WrapWithMetrics(grpcManager.NewConfigManager(conn), grpcMetrics), - tlsCloser: &builder.TLS, + conn: conn, + reporter: r3, + manager: configmanager.WrapWithMetrics(grpcManager.NewConfigManager(conn), grpcMetrics), }, nil } @@ -66,7 +63,6 @@ func (b ProxyBuilder) GetManager() configmanager.ClientConfigManager { func (b ProxyBuilder) Close() error { return errors.Join( b.reporter.Close(), - b.tlsCloser.Close(), b.GetConn().Close(), ) } diff --git a/cmd/agent/app/reporter/grpc/flags.go b/cmd/agent/app/reporter/grpc/flags.go index 47645c20cde..a03c957509e 100644 --- a/cmd/agent/app/reporter/grpc/flags.go +++ b/cmd/agent/app/reporter/grpc/flags.go @@ -44,7 +44,10 @@ func (b *ConnBuilder) InitFromViper(v *viper.Viper) (*ConnBuilder, error) { if err != nil { return b, fmt.Errorf("failed to process TLS options: %w", err) } - b.TLS = tls + if tls.Enabled { + tlscfg := tls.ToOtelClientConfig() + b.TLS = &tlscfg + } b.DiscoveryMinPeers = v.GetInt(discoveryMinPeers) return b, nil }