[config/configtls] Enable goleak check

.chloggen/goleak_configtls.yaml

@@ -0,0 +1,25 @@
+# Use this changelog template to create an entry for release notes.
+
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
+component: config/configtls
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Add shutdown method to TLS ServerConfig to fix memory leaks
+
+# One or more tracking issues or pull requests related to the change
+issues: [9165]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext:
+
+# Optional: The change log or logs in which this entry should be included.
+# e.g. '[user]' or '[user, api]'
+# Include 'user' if the change is relevant to end users.
+# Include 'api' if there is a change to a library API.
+# Default: '[user]'
+change_logs: []

config/configtls/clientcasfilereloader.go

@@ -132,7 +132,7 @@ func (r *clientCAsFileReloader) handleWatcherEvents() {
 
 func (r *clientCAsFileReloader) shutdown() error {
 	if r.shutdownCH == nil {
-		return fmt.Errorf("client CAs file watcher is not running")
+		return nil
 	}
 	r.shutdownCH <- true
 	close(r.shutdownCH)

config/configtls/clientcasfilereloader_test.go

@@ -15,10 +15,10 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestCannotShutdownIfNotWatching(t *testing.T) {
+func TestCanShutdownIfNotWatching(t *testing.T) {
 	reloader, _, _ := createReloader(t)
 	err := reloader.shutdown()
-	assert.Error(t, err)
+	assert.NoError(t, err)
 }
 
 func TestCannotStartIfAlreadyWatching(t *testing.T) {

config/configtls/configtls.go

@@ -123,6 +123,9 @@ type ServerConfig struct {
 	// Reload the ClientCAs file when it is modified
 	// (optional, default false)
 	ReloadClientCAFile bool `mapstructure:"client_ca_file_reload"`
+
+	// Shutdown functions used to shutdown the file reloader for the Client CA.
+	reloaderShutdownFuncs []func() error
 }
 
 // NewDefaultServerConfig creates a new TLSServerSetting with any default values set.
@@ -390,13 +393,13 @@ func (c ClientConfig) LoadTLSConfig(_ context.Context) (*tls.Config, error) {
 }
 
 // LoadTLSConfig loads the TLS configuration.
-func (c ServerConfig) LoadTLSConfig(_ context.Context) (*tls.Config, error) {
+func (c *ServerConfig) LoadTLSConfig(_ context.Context) (*tls.Config, error) {
 	tlsCfg, err := c.loadTLSConfig()
 	if err != nil {
 		return nil, fmt.Errorf("failed to load TLS config: %w", err)
 	}
 	if c.ClientCAFile != "" {
-		reloader, err := newClientCAsReloader(c.ClientCAFile, &c)
+		reloader, err := newClientCAsReloader(c.ClientCAFile, c)
 		if err != nil {
 			return nil, err
 		}
@@ -406,17 +409,29 @@ func (c ServerConfig) LoadTLSConfig(_ context.Context) (*tls.Config, error) {
 				return nil, err
 			}
 			tlsCfg.GetConfigForClient = func(*tls.ClientHelloInfo) (*tls.Config, error) { return reloader.getClientConfig(tlsCfg) }
+			c.reloaderShutdownFuncs = append(c.reloaderShutdownFuncs, reloader.shutdown)
 		}
 		tlsCfg.ClientCAs = reloader.certPool
 		tlsCfg.ClientAuth = tls.RequireAndVerifyClientCert
 	}
+
 	return tlsCfg, nil
 }
 
-func (c ServerConfig) loadClientCAFile() (*x509.CertPool, error) {
+func (c *ServerConfig) loadClientCAFile() (*x509.CertPool, error) {
 	return c.loadCert(c.ClientCAFile)
 }
 
+func (c *ServerConfig) Shutdown() error {
+	var errs []error
+
+	for _, shutdown := range c.reloaderShutdownFuncs {
+		errs = append(errs, shutdown())
+	}
+
+	return errors.Join(errs...)
+}
+
 func (c Config) hasCA() bool   { return c.hasCAFile() || c.hasCAPem() }
 func (c Config) hasCert() bool { return c.hasCertFile() || c.hasCertPem() }
 func (c Config) hasKey() bool  { return c.hasKeyFile() || c.hasKeyPem() }

config/configtls/configtls_test.go

@@ -310,12 +310,14 @@ func TestLoadTLSServerConfigError(t *testing.T) {
 			KeyFile:  "doesnt/exist",
 		},
 	}
+
 	_, err := tlsSetting.LoadTLSConfig(context.Background())
 	assert.Error(t, err)
 
 	tlsSetting = ServerConfig{
 		ClientCAFile: "doesnt/exist",
 	}
+
 	_, err = tlsSetting.LoadTLSConfig(context.Background())
 	assert.Error(t, err)
 }
@@ -325,6 +327,9 @@ func TestLoadTLSServerConfig(t *testing.T) {
 	tlsCfg, err := tlsSetting.LoadTLSConfig(context.Background())
 	assert.NoError(t, err)
 	assert.NotNil(t, tlsCfg)
+	defer func() {
+		assert.NoError(t, tlsSetting.Shutdown())
+	}()
 }
 
 func TestLoadTLSServerConfigReload(t *testing.T) {
@@ -341,6 +346,9 @@ func TestLoadTLSServerConfigReload(t *testing.T) {
 	tlsCfg, err := tlsSetting.LoadTLSConfig(context.Background())
 	assert.NoError(t, err)
 	assert.NotNil(t, tlsCfg)
+	defer func() {
+		assert.NoError(t, tlsSetting.Shutdown())
+	}()
 
 	firstClient, err := tlsCfg.GetConfigForClient(nil)
 	assert.NoError(t, err)
@@ -358,6 +366,25 @@ func TestLoadTLSServerConfigReload(t *testing.T) {
 	assert.NotEqual(t, firstClient.ClientCAs, secondClient.ClientCAs)
 }
 
+func TestLoadTLSServerMultipleConfigs(t *testing.T) {
+	tmpCaPath := createTempClientCaFile(t)
+
+	overwriteClientCA(t, tmpCaPath, "ca-1.crt")
+
+	tlsSetting := ServerConfig{
+		ClientCAFile:       tmpCaPath,
+		ReloadClientCAFile: true,
+	}
+
+	for i := 0; i < 10; i++ {
+		tlsCfg, err := tlsSetting.LoadTLSConfig(context.Background())
+		assert.NoError(t, err)
+		assert.NotNil(t, tlsCfg)
+	}
+
+	assert.NoError(t, tlsSetting.Shutdown())
+}
+
 func TestLoadTLSServerConfigFailingReload(t *testing.T) {
 
 	tmpCaPath := createTempClientCaFile(t)
@@ -372,6 +399,9 @@ func TestLoadTLSServerConfigFailingReload(t *testing.T) {
 	tlsCfg, err := tlsSetting.LoadTLSConfig(context.Background())
 	assert.NoError(t, err)
 	assert.NotNil(t, tlsCfg)
+	defer func() {
+		assert.NoError(t, tlsSetting.Shutdown())
+	}()
 
 	firstClient, err := tlsCfg.GetConfigForClient(nil)
 	assert.NoError(t, err)
@@ -433,6 +463,9 @@ func TestLoadTLSServerConfigFailing(t *testing.T) {
 	tlsCfg, err := tlsSetting.LoadTLSConfig(context.Background())
 	assert.NoError(t, err)
 	assert.NotNil(t, tlsCfg)
+	defer func() {
+		assert.NoError(t, tlsSetting.Shutdown())
+	}()
 
 	firstClient, err := tlsCfg.GetConfigForClient(nil)
 	assert.NoError(t, err)

config/configtls/go.mod

@@ -6,6 +6,7 @@ require (
 	github.com/fsnotify/fsnotify v1.7.0
 	github.com/stretchr/testify v1.9.0
 	go.opentelemetry.io/collector/config/configopaque v1.12.0
+	go.uber.org/goleak v1.3.0
 )
 
 require (

config/configtls/package_test.go

@@ -0,0 +1,17 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package configtls
+
+import (
+	"testing"
+
+	"go.uber.org/goleak"
+)
+
+// The IgnoreTopFunction call prevents catching the leak generated by opencensus
+// defaultWorker.Start which at this time is part of the package's init call.
+// See https://github.com/open-telemetry/opentelemetry-collector/issues/9165#issuecomment-1874836336 for more context.
+func TestMain(m *testing.M) {
+	goleak.VerifyTestMain(m, goleak.IgnoreTopFunction("go.opencensus.io/stats/view.(*worker).start"))
+}

receiver/otlpreceiver/otlp.go

@@ -184,18 +184,28 @@ func (r *otlpReceiver) Start(ctx context.Context, host component.Host) error {
 
 // Shutdown is a method to turn off receiving.
 func (r *otlpReceiver) Shutdown(ctx context.Context) error {
-	var err error
+	var errs []error
+
+	if r.cfg != nil {
+		if r.cfg.HTTP != nil && r.cfg.HTTP.TLSSetting != nil {
+			errs = append(errs, r.cfg.HTTP.TLSSetting.Shutdown())
+		}
+
+		if r.cfg.GRPC != nil && r.cfg.GRPC.TLSSetting != nil {
+			errs = append(errs, r.cfg.GRPC.TLSSetting.Shutdown())
+		}
+	}
 
 	if r.serverHTTP != nil {
-		err = r.serverHTTP.Shutdown(ctx)
+		errs = append(errs, r.serverHTTP.Shutdown(ctx))
 	}
 
 	if r.serverGRPC != nil {
 		r.serverGRPC.GracefulStop()
 	}
 
 	r.shutdownWG.Wait()
-	return err
+	return errors.Join(errs...)
 }
 
 func (r *otlpReceiver) registerTraceConsumer(tc consumer.Traces) {

receiver/otlpreceiver/otlp_test.go

@@ -13,6 +13,7 @@ import (
 	"io"
 	"net"
 	"net/http"
+	"path/filepath"
 	"strings"
 	"sync"
 	"testing"
@@ -712,6 +713,62 @@ func TestGRPCInvalidTLSCredentials(t *testing.T) {
 		`failed to load TLS config: failed to load TLS cert and key: for auth via TLS, provide both certificate and key, or neither`)
 }
 
+func TestGRPCValidTLSCredentials(t *testing.T) {
+
+	tests := []struct {
+		name   string
+		config *configtls.ServerConfig
+	}{
+		{
+			name: "Base case",
+			config: &configtls.ServerConfig{
+				Config: configtls.Config{
+					CAFile:   filepath.Join("testdata", "ca.crt"),
+					CertFile: filepath.Join("testdata", "server.crt"),
+					KeyFile:  filepath.Join("testdata", "server.key"),
+				},
+			},
+		},
+		{
+			name: "Test reload enabled",
+			config: &configtls.ServerConfig{
+				Config: configtls.Config{
+					CAFile:   filepath.Join("testdata", "ca.crt"),
+					CertFile: filepath.Join("testdata", "server.crt"),
+					KeyFile:  filepath.Join("testdata", "server.key"),
+				},
+				ReloadClientCAFile: true,
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			cfg := &Config{
+				Protocols: Protocols{
+					GRPC: &configgrpc.ServerConfig{
+						NetAddr: confignet.AddrConfig{
+							Endpoint:  testutil.GetAvailableLocalAddress(t),
+							Transport: "tcp",
+						},
+						TLSSetting: test.config,
+					},
+				},
+			}
+
+			r, err := NewFactory().CreateTracesReceiver(
+				context.Background(),
+				receivertest.NewNopSettings(),
+				cfg,
+				consumertest.NewNop())
+			require.NoError(t, err)
+			assert.NotNil(t, r)
+			assert.NoError(t, r.Start(context.Background(), componenttest.NewNopHost()))
+			assert.NoError(t, r.Shutdown(context.Background()))
+		})
+	}
+}
+
 func TestGRPCMaxRecvSize(t *testing.T) {
 	addr := testutil.GetAvailableLocalAddress(t)
 	sink := newErrOrSinkConsumer()
@@ -779,6 +836,64 @@ func TestHTTPInvalidTLSCredentials(t *testing.T) {
 		`failed to load TLS config: failed to load TLS cert and key: for auth via TLS, provide both certificate and key, or neither`)
 }
 
+func TestHTTPTLSCredentials(t *testing.T) {
+	// Add test cases like GRPC test
+	tests := []struct {
+		name   string
+		config *configtls.ServerConfig
+	}{
+		{
+			name: "Base case",
+			config: &configtls.ServerConfig{
+				Config: configtls.Config{
+					CAFile:   filepath.Join("testdata", "ca.crt"),
+					CertFile: filepath.Join("testdata", "server.crt"),
+					KeyFile:  filepath.Join("testdata", "server.key"),
+				},
+			},
+		},
+		{
+			name: "Test reload enabled",
+			config: &configtls.ServerConfig{
+				Config: configtls.Config{
+					CAFile:   filepath.Join("testdata", "ca.crt"),
+					CertFile: filepath.Join("testdata", "server.crt"),
+					KeyFile:  filepath.Join("testdata", "server.key"),
+				},
+				ReloadClientCAFile: true,
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			cfg := &Config{
+				Protocols: Protocols{
+					HTTP: &HTTPConfig{
+						ServerConfig: &confighttp.ServerConfig{
+							Endpoint:   testutil.GetAvailableLocalAddress(t),
+							TLSSetting: test.config,
+						},
+						TracesURLPath:  defaultTracesURLPath,
+						MetricsURLPath: defaultMetricsURLPath,
+						LogsURLPath:    defaultLogsURLPath,
+					},
+				},
+			}
+
+			r, err := NewFactory().CreateTracesReceiver(
+				context.Background(),
+				receivertest.NewNopSettings(),
+				cfg,
+				consumertest.NewNop())
+			require.NoError(t, err)
+			assert.NotNil(t, r)
+			assert.NoError(t, r.Start(context.Background(), componenttest.NewNopHost()))
+			assert.NoError(t, r.Shutdown(context.Background()))
+		})
+	}
+}
+
 func testHTTPMaxRequestBodySize(t *testing.T, path string, contentType string, payload []byte, size int, expectedStatusCode int) {
 	addr := testutil.GetAvailableLocalAddress(t)
 	url := "http://" + addr + path