httpcli: introduce mutable, wrapped transports (#39775)

Makes our wrapped transports fully unwrappable, such that a pointer to the implementation can be retrieved and replaced, namely with a clone of the underlying *http.Transport. This fixes the issue described in #39702, and removes what seems to be an old workaround around some options not supporting wrapped transports.

Co-authored-by: Molly Weitzel <molly.weitzel@sourcegraph.com>
Co-authored-by: Thorsten Ball <mrnugget@gmail.com>

internal/httpcli/client.go

@@ -283,12 +283,6 @@ func GerritUnauthenticateMiddleware(cli Doer) Doer {
 func ExternalTransportOpt(cli *http.Client) error {
 	tr, err := getTransportForMutation(cli)
 	if err != nil {
-		// TODO(keegancsmith) for now we don't support unwrappable
-		// transports. https://github.com/sourcegraph/sourcegraph/pull/7741
-		// https://github.com/sourcegraph/sourcegraph/pull/71
-		if isUnwrappableTransport(cli) {
-			return nil
-		}
 		return errors.Wrap(err, "httpcli.ExternalTransportOpt")
 	}
 
@@ -296,14 +290,6 @@ func ExternalTransportOpt(cli *http.Client) error {
 	return nil
 }
 
-func isUnwrappableTransport(cli *http.Client) bool {
-	if cli.Transport == nil {
-		return false
-	}
-	_, ok := cli.Transport.(interface{ UnwrappableTransport() })
-	return ok
-}
-
 // NewCertPoolOpt returns a Opt that sets the RootCAs pool of an http.Client's
 // transport.
 func NewCertPoolOpt(certs ...string) Opt {
@@ -610,15 +596,30 @@ func getTransportForMutation(cli *http.Client) (*http.Transport, error) {
 		cli.Transport = http.DefaultTransport
 	}
 
-	tr, ok := cli.Transport.(*http.Transport)
-	if !ok {
-		return nil, errors.Errorf("http.Client.Transport is not an *http.Transport: %T", cli.Transport)
+	// Try to get the underlying, concrete *http.Transport implementation, copy it, and
+	// replace it.
+	var transport *http.Transport
+	switch v := cli.Transport.(type) {
+	case *http.Transport:
+		transport = v.Clone()
+		// Replace underlying implementation
+		cli.Transport = transport
+
+	case WrappedTransport:
+		wrapped := unwrapAll(v)
+		t, ok := (*wrapped).(*http.Transport)
+		if !ok {
+			return nil, errors.Errorf("http.Client.Transport cannot be unwrapped as *http.Transport: %T", cli.Transport)
+		}
+		transport = t.Clone()
+		// Replace underlying implementation
+		*wrapped = transport
+
+	default:
+		return nil, errors.Errorf("http.Client.Transport cannot be cast as a *http.Transport: %T", cli.Transport)
 	}
 
-	tr = tr.Clone()
-	cli.Transport = tr
-
-	return tr, nil
+	return transport, nil
 }
 
 // ActorTransportOpt wraps an existing http.Transport of an http.Client to pull the actor
@@ -630,7 +631,10 @@ func ActorTransportOpt(cli *http.Client) error {
 		cli.Transport = http.DefaultTransport
 	}
 
-	cli.Transport = &actor.HTTPTransport{RoundTripper: cli.Transport}
+	cli.Transport = &wrappedTransport{
+		RoundTripper: &actor.HTTPTransport{RoundTripper: cli.Transport},
+		Wrapped:      cli.Transport,
+	}
 
 	return nil
 }
@@ -644,7 +648,10 @@ func RequestClientTransportOpt(cli *http.Client) error {
 		cli.Transport = http.DefaultTransport
 	}
 
-	cli.Transport = &requestclient.HTTPTransport{RoundTripper: cli.Transport}
+	cli.Transport = &wrappedTransport{
+		RoundTripper: &requestclient.HTTPTransport{RoundTripper: cli.Transport},
+		Wrapped:      cli.Transport,
+	}
 
 	return nil
 }

internal/httpcli/client_test.go

@@ -23,7 +23,9 @@ import (
 
 	"github.com/PuerkitoBio/rehttp"
 	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/assert"
 
+	"github.com/sourcegraph/sourcegraph/internal/actor"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 )
 
@@ -178,7 +180,7 @@ func TestNewCertPool(t *testing.T) {
 			name:  "fails if transport isn't an http.Transport",
 			cli:   &http.Client{Transport: bogusTransport{}},
 			certs: []string{cert},
-			err:   "httpcli.NewCertPoolOpt: http.Client.Transport is not an *http.Transport: httpcli.bogusTransport",
+			err:   "httpcli.NewCertPoolOpt: http.Client.Transport cannot be cast as a *http.Transport: httpcli.bogusTransport",
 		},
 		{
 			name:  "pool is set to what is given",
@@ -216,6 +218,11 @@ func TestNewCertPool(t *testing.T) {
 
 func TestNewIdleConnTimeoutOpt(t *testing.T) {
 	timeout := 33 * time.Second
+
+	// originalRoundtripper must only be used in one test, set at this scope for
+	// convenience.
+	originalRoundtripper := &http.Transport{}
+
 	for _, tc := range []struct {
 		name    string
 		cli     *http.Client
@@ -235,7 +242,7 @@ func TestNewIdleConnTimeoutOpt(t *testing.T) {
 		{
 			name: "fails if transport isn't an http.Transport",
 			cli:  &http.Client{Transport: bogusTransport{}},
-			err:  "httpcli.NewIdleConnTimeoutOpt: http.Client.Transport is not an *http.Transport: httpcli.bogusTransport",
+			err:  "httpcli.NewIdleConnTimeoutOpt: http.Client.Transport cannot be cast as a *http.Transport: httpcli.bogusTransport",
 		},
 		{
 			name:    "IdleConnTimeout is set to what is given",
@@ -248,6 +255,28 @@ func TestNewIdleConnTimeoutOpt(t *testing.T) {
 				}
 			},
 		},
+		{
+			name: "IdleConnTimeout is set to what is given on a wrapped transport",
+			cli: func() *http.Client {
+				return &http.Client{Transport: &wrappedTransport{
+					RoundTripper: &actor.HTTPTransport{RoundTripper: originalRoundtripper},
+					Wrapped:      originalRoundtripper,
+				}}
+			}(),
+			timeout: timeout,
+			assert: func(t testing.TB, cli *http.Client) {
+				unwrapped := unwrapAll(cli.Transport.(WrappedTransport))
+				have := (*unwrapped).(*http.Transport).IdleConnTimeout
+
+				// Timeout is set on the underlying transport
+				if want := timeout; !reflect.DeepEqual(have, want) {
+					t.Fatal(cmp.Diff(have, want))
+				}
+
+				// Original roundtripper unchanged!
+				assert.Equal(t, time.Duration(0), originalRoundtripper.IdleConnTimeout)
+			},
+		},
 	} {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {

internal/httpcli/transport.go

@@ -0,0 +1,38 @@
+package httpcli
+
+import "net/http"
+
+// WrappedTransport can be implemented to allow a wrapped transport to expose its
+// underlying transport for modification.
+type WrappedTransport interface {
+	// RoundTripper is the transport implementation that should be exposed.
+	http.RoundTripper
+
+	// Unwrap should provide a pointer to the underlying transport that has been wrapped.
+	// The returned value should never be nil.
+	Unwrap() *http.RoundTripper
+}
+
+// unwrapAll performs a recursive unwrap on transport until we reach a transport that
+// cannot be unwrapped. The pointer to the pointer can be used to replace the underlying
+// transport, most commonly by attempting to cast it as an *http.Transport.
+//
+// WrappedTransport.Unwrap should never return nil, so unwrapAll will never return nil.
+func unwrapAll(transport WrappedTransport) *http.RoundTripper {
+	wrapped := transport.Unwrap()
+	if unwrappable, ok := (*wrapped).(WrappedTransport); ok {
+		return unwrapAll(unwrappable)
+	}
+	return wrapped
+}
+
+// wrappedTransport is an http.RoundTripper that allows the underlying RoundTripper to be
+// exposed for modification.
+type wrappedTransport struct {
+	http.RoundTripper
+	Wrapped http.RoundTripper
+}
+
+var _ WrappedTransport = &wrappedTransport{}
+
+func (wt *wrappedTransport) Unwrap() *http.RoundTripper { return &wt.Wrapped }

internal/repos/awscodecommit.go

@@ -206,6 +206,8 @@ type stubBadHTTPRedirectTransport struct {
 	tr http.RoundTripper
 }
 
+var _ httpcli.WrappedTransport = &stubBadHTTPRedirectTransport{}
+
 const stubBadHTTPRedirectLocation = `https://amazonaws.com/badhttpredirectlocation`
 
 func (t stubBadHTTPRedirectTransport) RoundTrip(r *http.Request) (*http.Response, error) {
@@ -226,8 +228,4 @@ func (t stubBadHTTPRedirectTransport) RoundTrip(r *http.Request) (*http.Response
 	return resp, err
 }
 
-// UnwrappableTransport signals that this transport can't be wrapped. In
-// particular this means we won't respect global external
-// settings. https://github.com/sourcegraph/sourcegraph/issues/71 and
-// https://github.com/sourcegraph/sourcegraph/issues/7738
-func (stubBadHTTPRedirectTransport) UnwrappableTransport() {}
+func (t stubBadHTTPRedirectTransport) Unwrap() *http.RoundTripper { return &t.tr }

internal/repos/gitolite.go

@@ -40,7 +40,8 @@ func NewGitoliteSource(svc *types.ExternalService, cf *httpcli.Factory) (*Gitoli
 		// The provided httpcli.Factory is one used for external services - however,
 		// GitoliteSource asks gitserver to communicate to gitolite instead, so we
 		// have to ensure that the actor transport used for internal clients is provided.
-		httpcli.ActorTransportOpt)
+		httpcli.ActorTransportOpt,
+	)
 	if err != nil {
 		return nil, err
 	}

internal/repos/gitolite_test.go

@@ -0,0 +1,25 @@
+package repos
+
+import (
+	"testing"
+
+	"github.com/sourcegraph/sourcegraph/internal/extsvc"
+	"github.com/sourcegraph/sourcegraph/internal/httpcli"
+	"github.com/sourcegraph/sourcegraph/internal/types"
+	"github.com/sourcegraph/sourcegraph/schema"
+)
+
+func TestGitoliteSource(t *testing.T) {
+	cf, save := newClientFactoryWithOpt(t, "basic", httpcli.ExternalTransportOpt)
+	defer save(t)
+
+	svc := &types.ExternalService{
+		Kind:   extsvc.KindGitolite,
+		Config: marshalJSON(t, &schema.GitoliteConnection{}),
+	}
+
+	_, err := NewGitoliteSource(svc, cf)
+	if err != nil {
+		t.Fatal(err)
+	}
+}

internal/repos/sources_test.go

@@ -645,12 +645,23 @@ func TestSources_ListRepos(t *testing.T) {
 }
 
 func newClientFactory(t testing.TB, name string, mws ...httpcli.Middleware) (*httpcli.Factory, func(testing.TB)) {
+	mw, rec := testClientFactorySetup(t, name, mws...)
+	return httpcli.NewFactory(mw, httptestutil.NewRecorderOpt(rec)),
+		func(t testing.TB) { save(t, rec) }
+}
+
+func newClientFactoryWithOpt(t testing.TB, name string, opt httpcli.Opt) (*httpcli.Factory, func(testing.TB)) {
+	mw, rec := testClientFactorySetup(t, name)
+	return httpcli.NewFactory(mw, opt, httptestutil.NewRecorderOpt(rec)),
+		func(t testing.TB) { save(t, rec) }
+}
+
+func testClientFactorySetup(t testing.TB, name string, mws ...httpcli.Middleware) (httpcli.Middleware, *recorder.Recorder) {
 	cassete := filepath.Join("testdata", "sources", strings.ReplaceAll(name, " ", "-"))
 	rec := newRecorder(t, cassete, update(name))
 	mws = append(mws, httpcli.GitHubProxyRedirectMiddleware, gitserverRedirectMiddleware)
 	mw := httpcli.NewMiddleware(mws...)
-	return httpcli.NewFactory(mw, httptestutil.NewRecorderOpt(rec)),
-		func(t testing.TB) { save(t, rec) }
+	return mw, rec
 }
 
 func gitserverRedirectMiddleware(cli httpcli.Doer) httpcli.Doer {