Skip to content

Commit

Permalink
OAuth2: Respect disable keepalives option; Implement close idle conne…
Browse files Browse the repository at this point in the history
…ctions (#390)

Signed-off-by: Julien Pivotto <[email protected]>
  • Loading branch information
roidelapluie authored Jul 11, 2022
1 parent cdc09f0 commit b86ea81
Showing 1 changed file with 25 additions and 12 deletions.
37 changes: 25 additions & 12 deletions config/http_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,7 @@ type oauth2RoundTripper struct {
secret string
mtx sync.RWMutex
opts *httpClientOptions
client *http.Client
}

func NewOAuth2RoundTripper(config *OAuth2, next http.RoundTripper, opts *httpClientOptions) http.RoundTripper {
Expand Down Expand Up @@ -677,19 +678,24 @@ func (rt *oauth2RoundTripper) RoundTrip(req *http.Request) (*http.Response, erro
return nil, err
}

tlsTransport := func(tlsConfig *tls.Config) (http.RoundTripper, error) {
return &http.Transport{
TLSClientConfig: tlsConfig,
Proxy: http.ProxyURL(rt.config.ProxyURL.URL),
DisableKeepAlives: !rt.opts.keepAlivesEnabled,
MaxIdleConns: 20,
MaxIdleConnsPerHost: 1, // see https://github.com/golang/go/issues/13801

This comment has been minimized.

Copy link
@bboreham

bboreham Mar 6, 2023

Member

Something is wrong here.
The issue 13801 relates to low numbers causing premature closing of connections; the same comment appears on a line nearby where the number is 1,000.

If you intend for the number to be 1, please add a different comment explaining this.

IdleConnTimeout: 10 * time.Second,
TLSHandshakeTimeout: 10 * time.Second,
ExpectContinueTimeout: 1 * time.Second,
}, nil
}

var t http.RoundTripper
if len(rt.config.TLSConfig.CAFile) == 0 {
t = &http.Transport{
TLSClientConfig: tlsConfig,
Proxy: http.ProxyURL(rt.config.ProxyURL.URL),
}
t, _ = tlsTransport(tlsConfig)
} else {
t, err = NewTLSRoundTripper(tlsConfig, rt.config.TLSConfig.CAFile, func(tls *tls.Config) (http.RoundTripper, error) {
return &http.Transport{
TLSClientConfig: tls,
Proxy: http.ProxyURL(rt.config.ProxyURL.URL),
}, nil
})
t, err = NewTLSRoundTripper(tlsConfig, rt.config.TLSConfig.CAFile, tlsTransport)
if err != nil {
return nil, err
}
Expand All @@ -699,7 +705,8 @@ func (rt *oauth2RoundTripper) RoundTrip(req *http.Request) (*http.Response, erro
t = NewUserAgentRoundTripper(rt.opts.userAgent, t)
}

ctx := context.WithValue(context.Background(), oauth2.HTTPClient, &http.Client{Transport: t})
client := &http.Client{Transport: t}
ctx := context.WithValue(context.Background(), oauth2.HTTPClient, client)
tokenSource := config.TokenSource(ctx)

rt.mtx.Lock()
Expand All @@ -708,6 +715,10 @@ func (rt *oauth2RoundTripper) RoundTrip(req *http.Request) (*http.Response, erro
Base: rt.next,
Source: tokenSource,
}
if rt.client != nil {
rt.client.CloseIdleConnections()
}
rt.client = client
rt.mtx.Unlock()
}

Expand All @@ -718,7 +729,9 @@ func (rt *oauth2RoundTripper) RoundTrip(req *http.Request) (*http.Response, erro
}

func (rt *oauth2RoundTripper) CloseIdleConnections() {
// OAuth2 RT does not support CloseIdleConnections() but the next RT might.
if rt.client != nil {
rt.client.CloseIdleConnections()
}
if ci, ok := rt.next.(closeIdler); ok {
ci.CloseIdleConnections()
}
Expand Down

0 comments on commit b86ea81

Please sign in to comment.