plumbing: transport/http, refactor transport to cache underlying transport objects

Refactor the in-built http transport to cache the underlying http
transport objects mapped to its specific options for each Git transport
object. This lets us reuse the transport for a specific set of configurations
as recommended. (ref: https://pkg.go.dev/net/http#Transport)
If there are no transport specific options provided, the default
transport is used.

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>

8 files changed, 233 insertions, 45 deletions

diff --git a/go.mod b/go.mod
index 27b74a3..9bcad62 100644
--- a/go.mod
+++ b/go.mod
@@ -11,6 +11,7 @@ require (
 	github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376
 	github.com/go-git/go-billy/v5 v5.4.1
 	github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20230305113008-0c11038e723f
+	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da
 	github.com/google/go-cmp v0.5.9
 	github.com/imdario/mergo v0.3.15
 	github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99
diff --git a/go.sum b/go.sum
index f4fb146..ccc6597 100644
--- a/go.sum
+++ b/go.sum
@@ -26,6 +26,8 @@ github.com/go-git/go-billy/v5 v5.4.1 h1:Uwp5tDRkPr+l/TnbHOQzp+tmJfLceOlbVucgpTz8
 github.com/go-git/go-billy/v5 v5.4.1/go.mod h1:vjbugF6Fz7JIflbVpl1hJsGjSHNltrSw45YK/ukIvQg=
 github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20230305113008-0c11038e723f h1:Pz0DHeFij3XFhoBRGUDPzSJ+w2UcK5/0JvF8DRI58r8=
 github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20230305113008-0c11038e723f/go.mod h1:8LHG1a3SRW71ettAD/jW13h8c6AqjVSeL11RAdgaqpo=
+github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da h1:oI5xCqsCo564l8iNU+DwB5epxmsaqB+rhGL0m5jtYqE=
+github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc=
 github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
 github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
 github.com/imdario/mergo v0.3.15 h1:M8XP7IuFNsqUx6VPK2P9OSmsYsI/YFaGil0uD21V3dM=
diff --git a/plumbing/transport/client/client.go b/plumbing/transport/client/client.go
index 20c3d05..1948c23 100644
--- a/plumbing/transport/client/client.go
+++ b/plumbing/transport/client/client.go
@@ -3,10 +3,7 @@
 package client
 
 import (
-	"crypto/tls"
-	"crypto/x509"
 	"fmt"
-	gohttp "net/http"
 
 	"github.com/go-git/go-git/v5/plumbing/transport"
 	"github.com/go-git/go-git/v5/plumbing/transport/file"
@@ -24,14 +21,6 @@ var Protocols = map[string]transport.Transport{
 	"file":  file.DefaultClient,
 }
 
-var insecureClient = http.NewClient(&gohttp.Client{
-	Transport: &gohttp.Transport{
-		TLSClientConfig: &tls.Config{
-			InsecureSkipVerify: true,
-		},
-	},
-})
-
 // InstallProtocol adds or modifies an existing protocol.
 func InstallProtocol(scheme string, c transport.Transport) {
 	if c == nil {
@@ -50,27 +39,6 @@ func NewClient(endpoint *transport.Endpoint) (transport.Transport, error) {
 }
 
 func getTransport(endpoint *transport.Endpoint) (transport.Transport, error) {
-	if endpoint.Protocol == "https" {
-		if endpoint.InsecureSkipTLS {
-			return insecureClient, nil
-		}
-
-		if len(endpoint.CaBundle) != 0 {
-			rootCAs, _ := x509.SystemCertPool()
-			if rootCAs == nil {
-				rootCAs = x509.NewCertPool()
-			}
-			rootCAs.AppendCertsFromPEM(endpoint.CaBundle)
-			return http.NewClient(&gohttp.Client{
-				Transport: &gohttp.Transport{
-					TLSClientConfig: &tls.Config{
-						RootCAs: rootCAs,
-					},
-				},
-			}), nil
-		}
-	}
-
 	f, ok := Protocols[endpoint.Protocol]
 	if !ok {
 		return nil, fmt.Errorf("unsupported scheme %q", endpoint.Protocol)
diff --git a/plumbing/transport/http/common.go b/plumbing/transport/http/common.go
index d57c0fe..5300341 100644
--- a/plumbing/transport/http/common.go
+++ b/plumbing/transport/http/common.go
@@ -4,16 +4,21 @@ package http
 import (
 	"bytes"
 	"context"
+	"crypto/tls"
+	"crypto/x509"
 	"fmt"
 	"net"
 	"net/http"
+	"reflect"
 	"strconv"
 	"strings"
+	"sync"
 
 	"github.com/go-git/go-git/v5/plumbing"
 	"github.com/go-git/go-git/v5/plumbing/protocol/packp"
 	"github.com/go-git/go-git/v5/plumbing/transport"
 	"github.com/go-git/go-git/v5/utils/ioutil"
+	"github.com/golang/groupcache/lru"
 )
 
 // it requires a bytes.Buffer, because we need to know the length
@@ -74,40 +79,83 @@ func advertisedReferences(ctx context.Context, s *session, serviceName string) (
 }
 
 type client struct {
-	c *http.Client
+	c          *http.Client
+	transports *lru.Cache
+	m          sync.RWMutex
 }
 
-// DefaultClient is the default HTTP client, which uses `http.DefaultClient`.
-var DefaultClient = NewClient(nil)
+// ClientOptions holds user configurable options for the client.
+type ClientOptions struct {
+	// CacheMaxEntries is the max no. of entries that the transport objects
+	// cache will hold at any given point of time. It must be a positive integer.
+	// Calling `client.addTransport()` after the cache has reached the specified
+	// size, will result in the least recently used transport getting deleted
+	// before the provided transport is added to the cache.
+	CacheMaxEntries int
+}
+
+var (
+	// defaultTransportCacheSize is the default capacity of the transport objects cache.
+	// Its value is 0 because transport caching is turned off by default and is an
+	// opt-in feature.
+	defaultTransportCacheSize = 0
+
+	// DefaultClient is the default HTTP client, which uses a net/http client configured
+	// with http.DefaultTransport.
+	DefaultClient = NewClient(nil)
+)
 
 // NewClient creates a new client with a custom net/http client.
 // See `InstallProtocol` to install and override default http client.
-// Unless a properly initialized client is given, it will fall back into
-// `http.DefaultClient`.
+// If the net/http client is nil or empty, it will use a net/http client configured
+// with http.DefaultTransport.
 //
 // Note that for HTTP client cannot distinguish between private repositories and
 // unexistent repositories on GitHub. So it returns `ErrAuthorizationRequired`
 // for both.
 func NewClient(c *http.Client) transport.Transport {
 	if c == nil {
-		return &client{http.DefaultClient}
+		c = &http.Client{
+			Transport: http.DefaultTransport,
+		}
 	}
+	return NewClientWithOptions(c, &ClientOptions{
+		CacheMaxEntries: defaultTransportCacheSize,
+	})
+}
 
-	return &client{
+// NewClientWithOptions returns a new client configured with the provided net/http client
+// and other custom options specific to the client.
+// If the net/http client is nil or empty, it will use a net/http client configured
+// with http.DefaultTransport.
+func NewClientWithOptions(c *http.Client, opts *ClientOptions) transport.Transport {
+	if c == nil {
+		c = &http.Client{
+			Transport: http.DefaultTransport,
+		}
+	}
+	cl := &client{
 		c: c,
 	}
+
+	if opts != nil {
+		if opts.CacheMaxEntries > 0 {
+			cl.transports = lru.New(opts.CacheMaxEntries)
+		}
+	}
+	return cl
 }
 
 func (c *client) NewUploadPackSession(ep *transport.Endpoint, auth transport.AuthMethod) (
 	transport.UploadPackSession, error) {
 
-	return newUploadPackSession(c.c, ep, auth)
+	return newUploadPackSession(c, ep, auth)
 }
 
 func (c *client) NewReceivePackSession(ep *transport.Endpoint, auth transport.AuthMethod) (
 	transport.ReceivePackSession, error) {
 
-	return newReceivePackSession(c.c, ep, auth)
+	return newReceivePackSession(c, ep, auth)
 }
 
 type session struct {
@@ -117,10 +165,87 @@ type session struct {
 	advRefs  *packp.AdvRefs
 }
 
-func newSession(c *http.Client, ep *transport.Endpoint, auth transport.AuthMethod) (*session, error) {
+func transportWithInsecureTLS(transport *http.Transport) {
+	if transport.TLSClientConfig == nil {
+		transport.TLSClientConfig = &tls.Config{}
+	}
+	transport.TLSClientConfig.InsecureSkipVerify = true
+}
+
+func transportWithCABundle(transport *http.Transport, caBundle []byte) error {
+	rootCAs, err := x509.SystemCertPool()
+	if err != nil {
+		return err
+	}
+	if rootCAs == nil {
+		rootCAs = x509.NewCertPool()
+	}
+	rootCAs.AppendCertsFromPEM(caBundle)
+	if transport.TLSClientConfig == nil {
+		transport.TLSClientConfig = &tls.Config{}
+	}
+	transport.TLSClientConfig.RootCAs = rootCAs
+	return nil
+}
+
+func configureTransport(transport *http.Transport, ep *transport.Endpoint) error {
+	if len(ep.CaBundle) > 0 {
+		if err := transportWithCABundle(transport, ep.CaBundle); err != nil {
+			return err
+		}
+	}
+	if ep.InsecureSkipTLS {
+		transportWithInsecureTLS(transport)
+	}
+	return nil
+}
+
+func newSession(c *client, ep *transport.Endpoint, auth transport.AuthMethod) (*session, error) {
+	var httpClient *http.Client
+
+	// We need to configure the http transport if there are transport specific
+	// options present in the endpoint.
+	if len(ep.CaBundle) > 0 || ep.InsecureSkipTLS {
+		var transport *http.Transport
+		// if the client wasn't configured to have a cache for transports then just configure
+		// the transport and use it directly, otherwise try to use the cache.
+		if c.transports == nil {
+			tr, ok := c.c.Transport.(*http.Transport)
+			if !ok {
+				return nil, fmt.Errorf("expected underlying client transport to be of type: %s; got: %s",
+					reflect.TypeOf(transport), reflect.TypeOf(c.c.Transport))
+			}
+
+			transport = tr.Clone()
+			configureTransport(transport, ep)
+		} else {
+			transportOpts := transportOptions{
+				caBundle:        string(ep.CaBundle),
+				insecureSkipTLS: ep.InsecureSkipTLS,
+			}
+			var found bool
+			transport, found = c.fetchTransport(transportOpts)
+
+			if !found {
+				transport = c.c.Transport.(*http.Transport).Clone()
+				configureTransport(transport, ep)
+				c.addTransport(transportOpts, transport)
+			}
+		}
+
+		httpClient = &http.Client{
+			Transport:     transport,
+			CheckRedirect: c.c.CheckRedirect,
+			Jar:           c.c.Jar,
+			Timeout:       c.c.Timeout,
+		}
+	} else {
+		httpClient = c.c
+	}
+
 	s := &session{
 		auth:     basicAuthFromEndpoint(ep),
-		client:   c,
+		client:   httpClient,
 		endpoint: ep,
 	}
 	if auth != nil {
diff --git a/plumbing/transport/http/common_test.go b/plumbing/transport/http/common_test.go
index 4122e62..41188e6 100644
--- a/plumbing/transport/http/common_test.go
+++ b/plumbing/transport/http/common_test.go
@@ -91,6 +91,60 @@ func (s *ClientSuite) TestNewHTTPError40x(c *C) {
 		"unexpected client error.*")
 }
 
+func (s *ClientSuite) Test_newSession(c *C) {
+	cl := NewClientWithOptions(nil, &ClientOptions{
+		CacheMaxEntries: 2,
+	}).(*client)
+
+	insecureEP := s.Endpoint
+	insecureEP.InsecureSkipTLS = true
+	session, err := newSession(cl, insecureEP, nil)
+	c.Assert(err, IsNil)
+
+	sessionTransport := session.client.Transport.(*http.Transport)
+	c.Assert(sessionTransport.TLSClientConfig.InsecureSkipVerify, Equals, true)
+	t, ok := cl.fetchTransport(transportOptions{
+		insecureSkipTLS: true,
+	})
+	// transport should be cached.
+	c.Assert(ok, Equals, true)
+	// cached transport should be the one that's used.
+	c.Assert(sessionTransport, Equals, t)
+
+	caEndpoint := insecureEP
+	caEndpoint.CaBundle = []byte("this is the way")
+	session, err = newSession(cl, caEndpoint, nil)
+	c.Assert(err, IsNil)
+
+	sessionTransport = session.client.Transport.(*http.Transport)
+	c.Assert(sessionTransport.TLSClientConfig.InsecureSkipVerify, Equals, true)
+	c.Assert(sessionTransport.TLSClientConfig.RootCAs, NotNil)
+	t, ok = cl.fetchTransport(transportOptions{
+		insecureSkipTLS: true,
+		caBundle:        "this is the way",
+	})
+	// transport should be cached.
+	c.Assert(ok, Equals, true)
+	// cached transport should be the one that's used.
+	c.Assert(sessionTransport, Equals, t)
+
+	session, err = newSession(cl, caEndpoint, nil)
+	c.Assert(err, IsNil)
+	sessionTransport = session.client.Transport.(*http.Transport)
+	// transport that's going to be used should be cached already.
+	c.Assert(sessionTransport, Equals, t)
+	// no new transport got cached.
+	c.Assert(cl.transports.Len(), Equals, 2)
+
+	// if the cache does not exist, the transport should still be correctly configured.
+	cl.transports = nil
+	session, err = newSession(cl, insecureEP, nil)
+	c.Assert(err, IsNil)
+
+	sessionTransport = session.client.Transport.(*http.Transport)
+	c.Assert(sessionTransport.TLSClientConfig.InsecureSkipVerify, Equals, true)
+}
+
 func (s *ClientSuite) testNewHTTPError(c *C, code int, msg string) {
 	req, _ := http.NewRequest("GET", "foo", nil)
 	res := &http.Response{
diff --git a/plumbing/transport/http/receive_pack.go b/plumbing/transport/http/receive_pack.go
index 4d14ff2..4387ecf 100644
--- a/plumbing/transport/http/receive_pack.go
+++ b/plumbing/transport/http/receive_pack.go
@@ -19,7 +19,7 @@ type rpSession struct {
 	*session
 }
 
-func newReceivePackSession(c *http.Client, ep *transport.Endpoint, auth transport.AuthMethod) (transport.ReceivePackSession, error) {
+func newReceivePackSession(c *client, ep *transport.Endpoint, auth transport.AuthMethod) (transport.ReceivePackSession, error) {
 	s, err := newSession(c, ep, auth)
 	return &rpSession{s}, err
 }
diff --git a/plumbing/transport/http/transport.go b/plumbing/transport/http/transport.go
new file mode 100644
index 0000000..cd6787a
--- /dev/null
+++ b/plumbing/transport/http/transport.go
@@ -0,0 +1,38 @@
+package http
+
+import (
+	"net/http"
+)
+
+// transportOptions contains transport specific configuration.
+type transportOptions struct {
+	insecureSkipTLS bool
+	// []byte is not comparable.
+	caBundle string
+}
+
+func (c *client) addTransport(opts transportOptions, transport *http.Transport) {
+	c.m.Lock()
+	c.transports.Add(opts, transport)
+	c.m.Unlock()
+}
+
+func (c *client) removeTransport(opts transportOptions) {
+	c.m.Lock()
+	c.transports.Remove(opts)
+	c.m.Unlock()
+}
+
+func (c *client) fetchTransport(opts transportOptions) (*http.Transport, bool) {
+	c.m.RLock()
+	t, ok := c.transports.Get(opts)
+	c.m.RUnlock()
+	if !ok {
+		return nil, false
+	}
+	transport, ok := t.(*http.Transport)
+	if !ok {
+		return nil, false
+	}
+	return transport, true
+}
diff --git a/plumbing/transport/http/upload_pack.go b/plumbing/transport/http/upload_pack.go
index e735b3d..4f85145 100644
--- a/plumbing/transport/http/upload_pack.go
+++ b/plumbing/transport/http/upload_pack.go
@@ -19,7 +19,7 @@ type upSession struct {
 	*session
 }
 
-func newUploadPackSession(c *http.Client, ep *transport.Endpoint, auth transport.AuthMethod) (transport.UploadPackSession, error) {
+func newUploadPackSession(c *client, ep *transport.Endpoint, auth transport.AuthMethod) (transport.UploadPackSession, error) {
 	s, err := newSession(c, ep, auth)
 	return &upSession{s}, err
 }