diff --git a/.chloggen/goleak_configtls.yaml b/.chloggen/goleak_configtls.yaml index 204d782f2b7..a605c27bcd3 100755 --- a/.chloggen/goleak_configtls.yaml +++ b/.chloggen/goleak_configtls.yaml @@ -1,13 +1,13 @@ # Use this changelog template to create an entry for release notes. # One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' -change_type: bug_fix +change_type: breaking # 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 call to TLSServerSetting to fix leaking goroutine +note: Add shutdown call back to TLSServerSetting to fix memory leaks # One or more tracking issues or pull requests related to the change issues: [9165] @@ -15,7 +15,11 @@ 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: +subtext: | + The TLSServerSetting.LoadTLSConfig method signature has been modified to now return an extra + value, a function callback. This function callback must be used to ensure memory isn't leaked + on shutdown. Callers to TLSServerSetting.LoadTLSConfig should store every returned callback + and call them as soon as the relevant TLS config is no longer needed. # Optional: The change log or logs in which this entry should be included. # e.g. '[user]' or '[user, api]' diff --git a/config/configtls/clientcasfilereloader.go b/config/configtls/clientcasfilereloader.go index 1ee9c72ef9f..cd728941a12 100644 --- a/config/configtls/clientcasfilereloader.go +++ b/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) diff --git a/config/configtls/clientcasfilereloader_test.go b/config/configtls/clientcasfilereloader_test.go index 925c77e1ad5..3bc786c2f78 100644 --- a/config/configtls/clientcasfilereloader_test.go +++ b/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) { diff --git a/config/configtls/configtls.go b/config/configtls/configtls.go index 56c0326dae7..f327de4842f 100644 --- a/config/configtls/configtls.go +++ b/config/configtls/configtls.go @@ -106,9 +106,6 @@ type TLSServerSetting struct { // Reload the ClientCAs file when it is modified // (optional, default false) ReloadClientCAFile bool `mapstructure:"client_ca_file_reload"` - - // File reloader for the Client CA. - reloader *clientCAsFileReloader } // certReloader is a wrapper object for certificate reloading @@ -329,29 +326,37 @@ func (c TLSClientSetting) LoadTLSConfig() (*tls.Config, error) { return tlsCfg, nil } -// LoadTLSConfig loads the TLS configuration. -func (c *TLSServerSetting) LoadTLSConfig() (*tls.Config, error) { +// LoadTLSConfig loads the TLS configuration. The returned function is a callback that should +// be used to signal shutdown. +func (c TLSServerSetting) LoadTLSConfig() (*tls.Config, func() error, error) { tlsCfg, err := c.loadTLSConfig() + nopShutdown := func() error { return nil } + var reloader *clientCAsFileReloader + if err != nil { - return nil, fmt.Errorf("failed to load TLS config: %w", err) + return nil, nopShutdown, fmt.Errorf("failed to load TLS config: %w", err) } if c.ClientCAFile != "" { - var err error - c.reloader, err = newClientCAsReloader(c.ClientCAFile, c) + reloader, err = newClientCAsReloader(c.ClientCAFile, &c) if err != nil { - return nil, err + return nil, nopShutdown, err } if c.ReloadClientCAFile { - err = c.reloader.startWatching() + err = reloader.startWatching() if err != nil { - return nil, err + return nil, nopShutdown, err } - tlsCfg.GetConfigForClient = func(t *tls.ClientHelloInfo) (*tls.Config, error) { return c.reloader.getClientConfig(tlsCfg) } + tlsCfg.GetConfigForClient = func(t *tls.ClientHelloInfo) (*tls.Config, error) { return reloader.getClientConfig(tlsCfg) } } - tlsCfg.ClientCAs = c.reloader.certPool + tlsCfg.ClientCAs = reloader.certPool tlsCfg.ClientAuth = tls.RequireAndVerifyClientCert } - return tlsCfg, nil + + if reloader != nil { + return tlsCfg, reloader.shutdown, nil + } + + return tlsCfg, nopShutdown, nil } func (c TLSServerSetting) loadClientCAFile() (*x509.CertPool, error) { @@ -359,9 +364,6 @@ func (c TLSServerSetting) loadClientCAFile() (*x509.CertPool, error) { } func (c TLSServerSetting) Shutdown() error { - if c.ReloadClientCAFile { - return c.reloader.shutdown() - } return nil } diff --git a/config/configtls/configtls_test.go b/config/configtls/configtls_test.go index c1446d2b424..362eea308da 100644 --- a/config/configtls/configtls_test.go +++ b/config/configtls/configtls_test.go @@ -282,22 +282,25 @@ func TestLoadTLSServerConfigError(t *testing.T) { KeyFile: "doesnt/exist", }, } - _, err := tlsSetting.LoadTLSConfig() + _, _, err := tlsSetting.LoadTLSConfig() assert.Error(t, err) tlsSetting = TLSServerSetting{ ClientCAFile: "doesnt/exist", } - _, err = tlsSetting.LoadTLSConfig() + _, _, err = tlsSetting.LoadTLSConfig() assert.Error(t, err) } func TestLoadTLSServerConfig(t *testing.T) { tlsSetting := TLSServerSetting{} - tlsCfg, err := tlsSetting.LoadTLSConfig() + tlsCfg, shutdown, err := tlsSetting.LoadTLSConfig() assert.NoError(t, err) assert.NotNil(t, tlsCfg) - defer func() { assert.NoError(t, tlsSetting.Shutdown()) }() + defer func() { + shutdown() + assert.NoError(t, tlsSetting.Shutdown()) + }() } func TestLoadTLSServerConfigReload(t *testing.T) { @@ -311,10 +314,13 @@ func TestLoadTLSServerConfigReload(t *testing.T) { ReloadClientCAFile: true, } - tlsCfg, err := tlsSetting.LoadTLSConfig() + tlsCfg, shutdown, err := tlsSetting.LoadTLSConfig() assert.NoError(t, err) assert.NotNil(t, tlsCfg) - defer func() { assert.NoError(t, tlsSetting.Shutdown()) }() + defer func() { + shutdown() + assert.NoError(t, tlsSetting.Shutdown()) + }() firstClient, err := tlsCfg.GetConfigForClient(nil) assert.NoError(t, err) @@ -332,6 +338,30 @@ 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 := TLSServerSetting{ + ClientCAFile: tmpCaPath, + ReloadClientCAFile: true, + } + + var allShutdowns []func() error + + for i := 0; i < 10; i++ { + tlsCfg, shutdown, err := tlsSetting.LoadTLSConfig() + assert.NoError(t, err) + assert.NotNil(t, tlsCfg) + allShutdowns = append(allShutdowns, shutdown) + } + + for _, shutdown := range allShutdowns { + assert.NoError(t, shutdown()) + } +} + func TestLoadTLSServerConfigFailingReload(t *testing.T) { tmpCaPath := createTempClientCaFile(t) @@ -343,10 +373,13 @@ func TestLoadTLSServerConfigFailingReload(t *testing.T) { ReloadClientCAFile: true, } - tlsCfg, err := tlsSetting.LoadTLSConfig() + tlsCfg, shutdown, err := tlsSetting.LoadTLSConfig() assert.NoError(t, err) assert.NotNil(t, tlsCfg) - defer func() { assert.NoError(t, tlsSetting.Shutdown()) }() + defer func() { + shutdown() + assert.NoError(t, tlsSetting.Shutdown()) + }() firstClient, err := tlsCfg.GetConfigForClient(nil) assert.NoError(t, err) @@ -375,7 +408,7 @@ func TestLoadTLSServerConfigFailingInitialLoad(t *testing.T) { ReloadClientCAFile: true, } - tlsCfg, err := tlsSetting.LoadTLSConfig() + tlsCfg, _, err := tlsSetting.LoadTLSConfig() assert.Error(t, err) assert.Nil(t, tlsCfg) } @@ -389,7 +422,7 @@ func TestLoadTLSServerConfigWrongPath(t *testing.T) { ReloadClientCAFile: true, } - tlsCfg, err := tlsSetting.LoadTLSConfig() + tlsCfg, _, err := tlsSetting.LoadTLSConfig() assert.Error(t, err) assert.Nil(t, tlsCfg) } @@ -405,10 +438,13 @@ func TestLoadTLSServerConfigFailing(t *testing.T) { ReloadClientCAFile: true, } - tlsCfg, err := tlsSetting.LoadTLSConfig() + tlsCfg, shutdown, err := tlsSetting.LoadTLSConfig() assert.NoError(t, err) assert.NotNil(t, tlsCfg) - defer func() { assert.NoError(t, tlsSetting.Shutdown()) }() + defer func() { + shutdown() + assert.NoError(t, tlsSetting.Shutdown()) + }() firstClient, err := tlsCfg.GetConfigForClient(nil) assert.NoError(t, err)