Skip to content

Commit

Permalink
[chore] Enable goleak oidcauthextension (#35282)
Browse files Browse the repository at this point in the history
Related to #30438

---------

Signed-off-by: Israel Blancas <iblancas@redhat.com>
Co-authored-by: Curtis Robert <crobert@splunk.com>
  • Loading branch information
iblancasa and crobert-1 authored Oct 9, 2024
1 parent a1d7a33 commit aa89dc7
Show file tree
Hide file tree
Showing 6 changed files with 105 additions and 53 deletions.
28 changes: 28 additions & 0 deletions .chloggen/enable-goleak-oidcauthextension.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# 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: 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]

# (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.
# 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: []
104 changes: 60 additions & 44 deletions extension/oidcauthextension/extension.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -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
}
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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
Expand Down
17 changes: 13 additions & 4 deletions extension/oidcauthextension/extension_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
6 changes: 3 additions & 3 deletions extension/oidcauthextension/generated_package_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions extension/oidcauthextension/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ require (
go.opentelemetry.io/collector/confmap v1.17.1-0.20241008154146-ea48c09c31ae
go.opentelemetry.io/collector/extension v0.111.1-0.20241008154146-ea48c09c31ae
go.opentelemetry.io/collector/extension/auth v0.111.1-0.20241008154146-ea48c09c31ae
go.uber.org/goleak v1.3.0
go.uber.org/zap v1.27.0
)

Expand Down
2 changes: 0 additions & 2 deletions extension/oidcauthextension/metadata.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,3 @@ status:
tests:
config:
skip_lifecycle: true
goleak:
skip: true

0 comments on commit aa89dc7

Please sign in to comment.