From 6616eab227e5da6c05a0956ca35ff3f5e72ccbdd Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Thu, 3 Oct 2024 20:28:56 +0200 Subject: [PATCH 1/3] Enable goleak in oidcauthextension and fix go routine leak while shutting down Signed-off-by: Israel Blancas --- .../enable-goleak-oidcauthextension.yaml | 30 +++++ extension/oidcauthextension/extension.go | 104 ++++++++++-------- extension/oidcauthextension/extension_test.go | 17 ++- .../generated_package_test.go | 6 +- extension/oidcauthextension/go.mod | 1 + extension/oidcauthextension/metadata.yaml | 2 - 6 files changed, 107 insertions(+), 53 deletions(-) create mode 100644 .chloggen/enable-goleak-oidcauthextension.yaml diff --git a/.chloggen/enable-goleak-oidcauthextension.yaml b/.chloggen/enable-goleak-oidcauthextension.yaml new file mode 100644 index 000000000000..740ca6de607b --- /dev/null +++ b/.chloggen/enable-goleak-oidcauthextension.yaml @@ -0,0 +1,30 @@ +# 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. filelogreceiver) +component: oidcauthextension + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Enable goleak in oidcauthextension and fix a goleak issue while shutdown. + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [30438] + +# (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: | + A goroutine leak was found in oidcauthextension. The goroutine leak was caused by the oidcauthextension not closing the idle connections in the client and transport. + + As part of this change, goleak is enabled in the oidcauthextension. + +# If your change doesn't affect end users or the exported elements of any package, +# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. +# 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: [] diff --git a/extension/oidcauthextension/extension.go b/extension/oidcauthextension/extension.go index 52ce78b29154..c79e3231ef3a 100644 --- a/extension/oidcauthextension/extension.go +++ b/extension/oidcauthextension/extension.go @@ -27,10 +27,11 @@ import ( type oidcExtension struct { cfg *Config - provider *oidc.Provider - verifier *oidc.IDTokenVerifier - - logger *zap.Logger + provider *oidc.Provider + verifier *oidc.IDTokenVerifier + client *http.Client + logger *zap.Logger + transport *http.Transport } var ( @@ -53,19 +54,31 @@ func newExtension(cfg *Config, logger *zap.Logger) auth.Server { cfg: cfg, logger: logger, } - return auth.NewServer(auth.WithServerStart(oe.start), auth.WithServerAuthenticate(oe.authenticate)) + return auth.NewServer( + auth.WithServerStart(oe.start), + auth.WithServerAuthenticate(oe.authenticate), + auth.WithServerShutdown(oe.shutdown), + ) } -func (e *oidcExtension) start(context.Context, component.Host) error { - provider, err := getProviderForConfig(e.cfg) +func (e *oidcExtension) start(ctx context.Context, _ component.Host) error { + err := e.setProviderConfig(ctx, e.cfg) if err != nil { return fmt.Errorf("failed to get configuration from the auth server: %w", err) } - e.provider = provider - e.verifier = e.provider.Verifier(&oidc.Config{ ClientID: e.cfg.Audience, }) + return nil +} + +func (e *oidcExtension) shutdown(context.Context) error { + if e.client != nil { + e.client.CloseIdleConnections() + } + if e.transport != nil { + e.transport.CloseIdleConnections() + } return nil } @@ -124,6 +137,44 @@ func (e *oidcExtension) authenticate(ctx context.Context, headers map[string][]s return client.NewContext(ctx, cl), nil } +func (e *oidcExtension) setProviderConfig(ctx context.Context, config *Config) error { + e.transport = &http.Transport{ + Proxy: http.ProxyFromEnvironment, + DialContext: (&net.Dialer{ + Timeout: 5 * time.Second, + KeepAlive: 10 * time.Second, + DualStack: true, + }).DialContext, + ForceAttemptHTTP2: true, + MaxIdleConns: 100, + IdleConnTimeout: 90 * time.Second, + TLSHandshakeTimeout: 5 * time.Second, + ExpectContinueTimeout: 1 * time.Second, + } + + cert, err := getIssuerCACertFromPath(config.IssuerCAPath) + if err != nil { + return err // the errors from this path have enough context already + } + + if cert != nil { + e.transport.TLSClientConfig = &tls.Config{ + RootCAs: x509.NewCertPool(), + } + e.transport.TLSClientConfig.RootCAs.AddCert(cert) + } + + e.client = &http.Client{ + Timeout: 5 * time.Second, + Transport: e.transport, + } + oidcContext := oidc.ClientContext(ctx, e.client) + provider, err := oidc.NewProvider(oidcContext, config.IssuerURL) + e.provider = provider + + return err +} + func getSubjectFromClaims(claims map[string]any, usernameClaim string, fallback string) (string, error) { if len(usernameClaim) > 0 { username, found := claims[usernameClaim] @@ -167,41 +218,6 @@ func getGroupsFromClaims(claims map[string]any, groupsClaim string) ([]string, e return []string{}, nil } -func getProviderForConfig(config *Config) (*oidc.Provider, error) { - t := &http.Transport{ - Proxy: http.ProxyFromEnvironment, - DialContext: (&net.Dialer{ - Timeout: 5 * time.Second, - KeepAlive: 10 * time.Second, - DualStack: true, - }).DialContext, - ForceAttemptHTTP2: true, - MaxIdleConns: 100, - IdleConnTimeout: 90 * time.Second, - TLSHandshakeTimeout: 5 * time.Second, - ExpectContinueTimeout: 1 * time.Second, - } - - cert, err := getIssuerCACertFromPath(config.IssuerCAPath) - if err != nil { - return nil, err // the errors from this path have enough context already - } - - if cert != nil { - t.TLSClientConfig = &tls.Config{ - RootCAs: x509.NewCertPool(), - } - t.TLSClientConfig.RootCAs.AddCert(cert) - } - - client := &http.Client{ - Timeout: 5 * time.Second, - Transport: t, - } - oidcContext := oidc.ClientContext(context.Background(), client) - return oidc.NewProvider(oidcContext, config.IssuerURL) -} - func getIssuerCACertFromPath(path string) (*x509.Certificate, error) { if path == "" { return nil, nil diff --git a/extension/oidcauthextension/extension_test.go b/extension/oidcauthextension/extension_test.go index 72931882a19a..92b72d15fa14 100644 --- a/extension/oidcauthextension/extension_test.go +++ b/extension/oidcauthextension/extension_test.go @@ -112,11 +112,14 @@ func TestOIDCProviderForConfigWithTLS(t *testing.T) { } // test - provider, err := getProviderForConfig(config) + e := &oidcExtension{} + err = e.setProviderConfig(context.Background(), config) // verify assert.NoError(t, err) - assert.NotNil(t, provider) + assert.NotNil(t, e.provider) + assert.NotNil(t, e.client) + assert.NotNil(t, e.transport) } func TestOIDCLoadIssuerCAFromPath(t *testing.T) { @@ -185,11 +188,14 @@ func TestOIDCFailedToLoadIssuerCAFromPathInvalidContent(t *testing.T) { } // test - provider, err := getProviderForConfig(config) // cross test with getIssuerCACertFromPath + e := &oidcExtension{} + err = e.setProviderConfig(context.Background(), config) // verify assert.Error(t, err) - assert.Nil(t, provider) + assert.Nil(t, e.provider) + assert.Nil(t, e.client) + assert.NotNil(t, e.transport) } func TestOIDCInvalidAuthHeader(t *testing.T) { @@ -234,6 +240,9 @@ func TestProviderNotReacheable(t *testing.T) { // verify assert.Error(t, err) + + err = p.Shutdown(context.Background()) + assert.NoError(t, err) } func TestFailedToVerifyToken(t *testing.T) { diff --git a/extension/oidcauthextension/generated_package_test.go b/extension/oidcauthextension/generated_package_test.go index 322491bcffe1..195328a56d6e 100644 --- a/extension/oidcauthextension/generated_package_test.go +++ b/extension/oidcauthextension/generated_package_test.go @@ -3,11 +3,11 @@ package oidcauthextension import ( - "os" "testing" + + "go.uber.org/goleak" ) func TestMain(m *testing.M) { - // skipping goleak test as per metadata.yml configuration - os.Exit(m.Run()) + goleak.VerifyTestMain(m) } diff --git a/extension/oidcauthextension/go.mod b/extension/oidcauthextension/go.mod index 35798b4c5056..ff36fdf6d855 100644 --- a/extension/oidcauthextension/go.mod +++ b/extension/oidcauthextension/go.mod @@ -10,6 +10,7 @@ require ( go.opentelemetry.io/collector/confmap v1.16.0 go.opentelemetry.io/collector/extension v0.110.0 go.opentelemetry.io/collector/extension/auth v0.110.0 + go.uber.org/goleak v1.3.0 go.uber.org/zap v1.27.0 ) diff --git a/extension/oidcauthextension/metadata.yaml b/extension/oidcauthextension/metadata.yaml index 678dd3e58790..a4720591e476 100644 --- a/extension/oidcauthextension/metadata.yaml +++ b/extension/oidcauthextension/metadata.yaml @@ -11,5 +11,3 @@ status: tests: config: skip_lifecycle: true - goleak: - skip: true From 24c3a6dca46cb470a4f4300aaac9338518d203dd Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Fri, 4 Oct 2024 23:29:32 +0200 Subject: [PATCH 2/3] Update .chloggen/enable-goleak-oidcauthextension.yaml Co-authored-by: Curtis Robert --- .chloggen/enable-goleak-oidcauthextension.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.chloggen/enable-goleak-oidcauthextension.yaml b/.chloggen/enable-goleak-oidcauthextension.yaml index 740ca6de607b..c007ec636700 100644 --- a/.chloggen/enable-goleak-oidcauthextension.yaml +++ b/.chloggen/enable-goleak-oidcauthextension.yaml @@ -7,7 +7,7 @@ change_type: bug_fix component: oidcauthextension # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). -note: Enable goleak in oidcauthextension and fix a goleak issue while shutdown. +note: Fix a goroutine leak during shutdown. # Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. issues: [30438] From 71734da10174f9c5bcf64c3a2d312f713b7d61ff Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Fri, 4 Oct 2024 23:29:38 +0200 Subject: [PATCH 3/3] Update .chloggen/enable-goleak-oidcauthextension.yaml Co-authored-by: Curtis Robert --- .chloggen/enable-goleak-oidcauthextension.yaml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.chloggen/enable-goleak-oidcauthextension.yaml b/.chloggen/enable-goleak-oidcauthextension.yaml index c007ec636700..9c389aff7e7c 100644 --- a/.chloggen/enable-goleak-oidcauthextension.yaml +++ b/.chloggen/enable-goleak-oidcauthextension.yaml @@ -17,8 +17,6 @@ issues: [30438] # Use pipe (|) for multiline entries. subtext: | A goroutine leak was found in oidcauthextension. The goroutine leak was caused by the oidcauthextension not closing the idle connections in the client and transport. - - As part of this change, goleak is enabled in the oidcauthextension. # If your change doesn't affect end users or the exported elements of any package, # you should instead start your pull request title with [chore] or use the "Skip Changelog" label.