From 742657067045ee314c47b7b7e17085692be152da Mon Sep 17 00:00:00 2001 From: Robert Lin Date: Tue, 2 Aug 2022 13:12:10 +0000 Subject: [PATCH] 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 Co-authored-by: Thorsten Ball --- internal/httpcli/client.go | 53 +++++++++++++++++++-------------- internal/httpcli/client_test.go | 33 ++++++++++++++++++-- internal/httpcli/transport.go | 38 +++++++++++++++++++++++ internal/repos/awscodecommit.go | 8 ++--- internal/repos/gitolite.go | 3 +- internal/repos/gitolite_test.go | 25 ++++++++++++++++ internal/repos/sources_test.go | 15 ++++++++-- 7 files changed, 142 insertions(+), 33 deletions(-) create mode 100644 internal/httpcli/transport.go create mode 100644 internal/repos/gitolite_test.go diff --git a/internal/httpcli/client.go b/internal/httpcli/client.go index d8d46e046e6..423111e7d57 100644 --- a/internal/httpcli/client.go +++ b/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 } diff --git a/internal/httpcli/client_test.go b/internal/httpcli/client_test.go index 35300e07574..74ebcaae0c1 100644 --- a/internal/httpcli/client_test.go +++ b/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) { diff --git a/internal/httpcli/transport.go b/internal/httpcli/transport.go new file mode 100644 index 00000000000..7a1f2bac3af --- /dev/null +++ b/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 } diff --git a/internal/repos/awscodecommit.go b/internal/repos/awscodecommit.go index f7a53ee1e9a..c583847158e 100644 --- a/internal/repos/awscodecommit.go +++ b/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 } diff --git a/internal/repos/gitolite.go b/internal/repos/gitolite.go index 68abf20dc03..435f49317ab 100644 --- a/internal/repos/gitolite.go +++ b/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 } diff --git a/internal/repos/gitolite_test.go b/internal/repos/gitolite_test.go new file mode 100644 index 00000000000..318433422a1 --- /dev/null +++ b/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) + } +} diff --git a/internal/repos/sources_test.go b/internal/repos/sources_test.go index aa11cea2f91..0ef5df5cec8 100644 --- a/internal/repos/sources_test.go +++ b/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 {