Merge pull request #10568 from monostream/feature/fix-connection-leak

HTTPGetter: Reuse http transport

pkg/getter/getter.go

@@ -18,6 +18,7 @@ package getter
 
 import (
 	"bytes"
+	"net/http"
 	"time"
 
 	"github.com/pkg/errors"
@@ -43,6 +44,7 @@ type options struct {
 	version               string
 	registryClient        *registry.Client
 	timeout               time.Duration
+	transport             *http.Transport
 }
 
 // Option allows specifying various settings configurable by the user for overriding the defaults
@@ -119,6 +121,13 @@ func WithUntar() Option {
 	}
 }
 
+// WithTransport sets the http.Transport to allow overwriting the HTTPGetter default.
+func WithTransport(transport *http.Transport) Option {
+	return func(opts *options) {
+		opts.transport = transport
+	}
+}
+
 // Getter is an interface to support GET to the specified URL.
 type Getter interface {
 	// Get file content by url string

pkg/getter/httpgetter.go

@@ -21,6 +21,7 @@ import (
 	"io"
 	"net/http"
 	"net/url"
+	"sync"
 
 	"github.com/pkg/errors"
 
@@ -31,7 +32,9 @@ import (
 
 // HTTPGetter is the default HTTP(/S) backend handler
 type HTTPGetter struct {
-	opts options
+	opts      options
+	transport *http.Transport
+	once      sync.Once
 }
 
 // Get performs a Get from repo.Getter and returns the body.
@@ -106,10 +109,20 @@ func NewHTTPGetter(options ...Option) (Getter, error) {
 }
 
 func (g *HTTPGetter) httpClient() (*http.Client, error) {
-	transport := &http.Transport{
-		DisableCompression: true,
-		Proxy:              http.ProxyFromEnvironment,
+	if g.opts.transport != nil {
+		return &http.Client{
+			Transport: g.opts.transport,
+			Timeout:   g.opts.timeout,
+		}, nil
 	}
+
+	g.once.Do(func() {
+		g.transport = &http.Transport{
+			DisableCompression: true,
+			Proxy:              http.ProxyFromEnvironment,
+		}
+	})
+
 	if (g.opts.certFile != "" && g.opts.keyFile != "") || g.opts.caFile != "" {
 		tlsConf, err := tlsutil.NewClientTLS(g.opts.certFile, g.opts.keyFile, g.opts.caFile)
 		if err != nil {
@@ -123,21 +136,21 @@ func (g *HTTPGetter) httpClient() (*http.Client, error) {
 		}
 		tlsConf.ServerName = sni
 
-		transport.TLSClientConfig = tlsConf
+		g.transport.TLSClientConfig = tlsConf
 	}
 
 	if g.opts.insecureSkipVerifyTLS {
-		if transport.TLSClientConfig == nil {
-			transport.TLSClientConfig = &tls.Config{
+		if g.transport.TLSClientConfig == nil {
+			g.transport.TLSClientConfig = &tls.Config{
 				InsecureSkipVerify: true,
 			}
 		} else {
-			transport.TLSClientConfig.InsecureSkipVerify = true
+			g.transport.TLSClientConfig.InsecureSkipVerify = true
 		}
 	}
 
 	client := &http.Client{
-		Transport: transport,
+		Transport: g.transport,
 		Timeout:   g.opts.timeout,
 	}
 

pkg/getter/httpgetter_test.go

@@ -50,6 +50,7 @@ func TestHTTPGetter(t *testing.T) {
 	ca, pub, priv := join(cd, "rootca.crt"), join(cd, "crt.pem"), join(cd, "key.pem")
 	insecure := false
 	timeout := time.Second * 5
+	transport := &http.Transport{}
 
 	// Test with options
 	g, err = NewHTTPGetter(
@@ -59,6 +60,7 @@ func TestHTTPGetter(t *testing.T) {
 		WithTLSClientConfig(pub, priv, ca),
 		WithInsecureSkipVerifyTLS(insecure),
 		WithTimeout(timeout),
+		WithTransport(transport),
 	)
 	if err != nil {
 		t.Fatal(err)
@@ -105,6 +107,10 @@ func TestHTTPGetter(t *testing.T) {
 		t.Errorf("Expected NewHTTPGetter to contain %s as Timeout flag, got %s", timeout, hg.opts.timeout)
 	}
 
+	if hg.opts.transport != transport {
+		t.Errorf("Expected NewHTTPGetter to contain %p as Transport, got %p", transport, hg.opts.transport)
+	}
+
 	// Test if setting insecureSkipVerifyTLS is being passed to the ops
 	insecure = true
 
@@ -396,24 +402,24 @@ func TestHTTPGetterTarDownload(t *testing.T) {
 func TestHttpClientInsecureSkipVerify(t *testing.T) {
 	g := HTTPGetter{}
 	g.opts.url = "https://localhost"
-	verifyInsecureSkipVerify(t, g, "Blank HTTPGetter", false)
+	verifyInsecureSkipVerify(t, &g, "Blank HTTPGetter", false)
 
 	g = HTTPGetter{}
 	g.opts.url = "https://localhost"
 	g.opts.caFile = "testdata/ca.crt"
-	verifyInsecureSkipVerify(t, g, "HTTPGetter with ca file", false)
+	verifyInsecureSkipVerify(t, &g, "HTTPGetter with ca file", false)
 
 	g = HTTPGetter{}
 	g.opts.url = "https://localhost"
 	g.opts.insecureSkipVerifyTLS = true
-	verifyInsecureSkipVerify(t, g, "HTTPGetter with skip cert verification only", true)
+	verifyInsecureSkipVerify(t, &g, "HTTPGetter with skip cert verification only", true)
 
 	g = HTTPGetter{}
 	g.opts.url = "https://localhost"
 	g.opts.certFile = "testdata/client.crt"
 	g.opts.keyFile = "testdata/client.key"
 	g.opts.insecureSkipVerifyTLS = true
-	transport := verifyInsecureSkipVerify(t, g, "HTTPGetter with 2 way ssl", true)
+	transport := verifyInsecureSkipVerify(t, &g, "HTTPGetter with 2 way ssl", true)
 	if len(transport.TLSClientConfig.Certificates) <= 0 {
 		t.Fatal("transport.TLSClientConfig.Certificates is not present")
 	}
@@ -422,7 +428,7 @@ func TestHttpClientInsecureSkipVerify(t *testing.T) {
 	}
 }
 
-func verifyInsecureSkipVerify(t *testing.T, g HTTPGetter, caseName string, expectedValue bool) *http.Transport {
+func verifyInsecureSkipVerify(t *testing.T, g *HTTPGetter, caseName string, expectedValue bool) *http.Transport {
 	returnVal, err := g.httpClient()
 
 	if err != nil {
@@ -443,3 +449,84 @@ func verifyInsecureSkipVerify(t *testing.T, g HTTPGetter, caseName string, expec
 	}
 	return transport
 }
+
+func TestDefaultHTTPTransportReuse(t *testing.T) {
+	g := HTTPGetter{}
+
+	httpClient1, err := g.httpClient()
+
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if httpClient1 == nil {
+		t.Fatalf("Expected non nil value for http client")
+	}
+
+	transport1 := (httpClient1.Transport).(*http.Transport)
+
+	httpClient2, err := g.httpClient()
+
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if httpClient2 == nil {
+		t.Fatalf("Expected non nil value for http client")
+	}
+
+	transport2 := (httpClient2.Transport).(*http.Transport)
+
+	if transport1 != transport2 {
+		t.Fatalf("Expected default transport to be reused")
+	}
+}
+
+func TestHTTPTransportOption(t *testing.T) {
+	transport := &http.Transport{}
+
+	g := HTTPGetter{}
+	g.opts.transport = transport
+	httpClient1, err := g.httpClient()
+
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if httpClient1 == nil {
+		t.Fatalf("Expected non nil value for http client")
+	}
+
+	transport1 := (httpClient1.Transport).(*http.Transport)
+
+	if transport1 != transport {
+		t.Fatalf("Expected transport option to be applied")
+	}
+
+	httpClient2, err := g.httpClient()
+
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if httpClient2 == nil {
+		t.Fatalf("Expected non nil value for http client")
+	}
+
+	transport2 := (httpClient2.Transport).(*http.Transport)
+
+	if transport1 != transport2 {
+		t.Fatalf("Expected applied transport to be reused")
+	}
+
+	g = HTTPGetter{}
+	g.opts.url = "https://localhost"
+	g.opts.certFile = "testdata/client.crt"
+	g.opts.keyFile = "testdata/client.key"
+	g.opts.insecureSkipVerifyTLS = true
+	g.opts.transport = transport
+	usedTransport := verifyInsecureSkipVerify(t, &g, "HTTPGetter with 2 way ssl", false)
+	if usedTransport.TLSClientConfig != nil {
+		t.Fatal("transport.TLSClientConfig should not be set")
+	}
+}