Remove TLS loading and replace with OTEL configtls (#6345)

## Which problem is this PR solving? - Resolves #4316 - The last places where racy TLS cert loading code was used ## Description of the changes - Remove our custom logic for loading certificates - Fully rely on `configtls` module from otel-collector ## How was this change tested? - CI --------- Signed-off-by: Yuri Shkuro <github@ysh.us>
jaegertracing · Dec 13, 2024 · 989dcb0 · 989dcb0
1 parent e98f7f5
commit 989dcb0
Show file tree

Hide file tree

Showing 13 changed files with 69 additions and 1,079 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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 {
+		tlsConf := tls.ToOtelClientConfig()
+		b.TLS = &tlsConf
+	}
 	b.DiscoveryMinPeers = v.GetInt(discoveryMinPeers)
 	return b, nil
 }
diff --git a/cmd/agent/app/reporter/grpc/flags_test.go b/cmd/agent/app/reporter/grpc/flags_test.go
@@ -11,6 +11,7 @@ import (
 	"github.com/spf13/viper"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.opentelemetry.io/collector/config/configtls"
 
 	"github.com/jaegertracing/jaeger/pkg/config"
 )
@@ -32,6 +33,17 @@ func TestBindFlags(t *testing.T) {
 			cOpts:    []string{"--reporter.grpc.host-port=localhost:1111,localhost:2222", "--reporter.grpc.discovery.min-peers=5"},
 			expected: &ConnBuilder{CollectorHostPorts: []string{"localhost:1111", "localhost:2222"}, MaxRetry: defaultMaxRetry, DiscoveryMinPeers: 5},
 		},
+		{
+			cOpts: []string{"--reporter.grpc.tls.enabled=true"},
+			expected: &ConnBuilder{
+				TLS: &configtls.ClientConfig{
+					Config: configtls.Config{
+						IncludeSystemCACertsPool: true,
+					},
+				},
+				MaxRetry: defaultMaxRetry, DiscoveryMinPeers: 3,
+			},
+		},
 	}
 	for _, test := range tests {
 		v := viper.New()