internal/source: inject *http.Client into source.NewClient

This removes the dependency from package source onto ochttp. The users
of source.NewClient that want an ochttp.Transport can set the
transport on the *http.Client.

For golang/go#61399

Change-Id: Ifb7126c482f664ee5a359f594d9324f0fd90f8b2
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/523510
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
kokoro-CI: kokoro <noreply+kokoro@google.com>

13 files changed, 39 insertions, 37 deletions

diff --git a/cmd/frontend/main.go b/cmd/frontend/main.go
index 13324a3c..1530c9d4 100644
--- a/cmd/frontend/main.go
+++ b/cmd/frontend/main.go
@@ -15,6 +15,7 @@ import (
 	"cloud.google.com/go/profiler"
 	"github.com/go-redis/redis/v8"
 	"github.com/google/safehtml/template"
+	"go.opencensus.io/plugin/ochttp"
 	"golang.org/x/pkgsite/cmd/internal/cmdconfig"
 	"golang.org/x/pkgsite/internal"
 	"golang.org/x/pkgsite/internal/config"
@@ -84,8 +85,12 @@ func main() {
 	}
 
 	if *directProxy {
+		sourceClient := source.NewClient(&http.Client{Transport: &ochttp.Transport{}, Timeout: 1 * time.Minute})
 		ds := fetchdatasource.Options{
-			Getters:              []fetch.ModuleGetter{fetch.NewProxyModuleGetter(proxyClient, source.NewClient(1*time.Minute)), fetch.NewStdlibZipModuleGetter()},
+			Getters: []fetch.ModuleGetter{
+				fetch.NewProxyModuleGetter(proxyClient, sourceClient),
+				fetch.NewStdlibZipModuleGetter(),
+			},
 			ProxyClientForLatest: proxyClient,
 			BypassLicenseCheck:   *bypassLicenseCheck,
 		}.New()
@@ -97,7 +102,10 @@ func main() {
 		}
 		defer db.Close()
 		dsg = func(context.Context) internal.DataSource { return db }
-		sourceClient := source.NewClient(config.SourceTimeout)
+		sourceClient := source.NewClient(&http.Client{
+			Transport: &ochttp.Transport{},
+			Timeout:   config.SourceTimeout,
+		})
 		// The closure passed to queue.New is only used for testing and local
 		// execution, not in production. So it's okay that it doesn't use a
 		// per-request connection.
diff --git a/cmd/pkgsite/main.go b/cmd/pkgsite/main.go
index 9f5eaefd..1883dbd8 100644
--- a/cmd/pkgsite/main.go
+++ b/cmd/pkgsite/main.go
@@ -396,7 +396,7 @@ func buildGetters(ctx context.Context, cfg getterConfig) ([]fetch.ModuleGetter,
 
 	// Add a proxy
 	if cfg.proxy != nil {
-		getters = append(getters, fetch.NewProxyModuleGetter(cfg.proxy, source.NewClient(time.Second)))
+		getters = append(getters, fetch.NewProxyModuleGetter(cfg.proxy, source.NewClient(&http.Client{Timeout: time.Second})))
 	}
 
 	getters = append(getters, fetch.NewStdlibZipModuleGetter())
diff --git a/cmd/worker/main.go b/cmd/worker/main.go
index 495d1b37..df8fed0e 100644
--- a/cmd/worker/main.go
+++ b/cmd/worker/main.go
@@ -17,6 +17,7 @@ import (
 	"github.com/go-redis/redis/v8"
 	"github.com/google/safehtml/template"
 	_ "github.com/jackc/pgx/v4/stdlib" // for pgx driver
+	"go.opencensus.io/plugin/ochttp"
 	"golang.org/x/pkgsite/cmd/internal/cmdconfig"
 	"golang.org/x/pkgsite/internal/config"
 	"golang.org/x/pkgsite/internal/config/serverconfig"
@@ -75,7 +76,10 @@ func main() {
 	if err != nil {
 		log.Fatal(ctx, err)
 	}
-	sourceClient := source.NewClient(config.SourceTimeout)
+	sourceClient := source.NewClient(&http.Client{
+		Transport: &ochttp.Transport{},
+		Timeout:   config.SourceTimeout,
+	})
 	expg := cmdconfig.ExperimentGetter(ctx, cfg)
 	fetchQueue, err := gcpqueue.New(ctx, cfg, queueName, *workers, expg,
 		func(ctx context.Context, modulePath, version string) (int, error) {
diff --git a/devtools/cmd/seeddb/main.go b/devtools/cmd/seeddb/main.go
index 5fd7b065..e2002a30 100644
--- a/devtools/cmd/seeddb/main.go
+++ b/devtools/cmd/seeddb/main.go
@@ -18,6 +18,7 @@ import (
 	"time"
 
 	_ "github.com/jackc/pgx/v4/stdlib" // for pgx driver
+	"go.opencensus.io/plugin/ochttp"
 	"golang.org/x/pkgsite/internal"
 	"golang.org/x/pkgsite/internal/config"
 	"golang.org/x/pkgsite/internal/config/dynconfig"
@@ -76,7 +77,10 @@ func run(ctx context.Context, db *database.DB, proxyURL string) error {
 		return err
 	}
 
-	sourceClient := source.NewClient(config.SourceTimeout)
+	sourceClient := source.NewClient(&http.Client{
+		Transport: &ochttp.Transport{},
+		Timeout:   config.SourceTimeout,
+	})
 	seedModules, err := readSeedFile(ctx, *seedfile)
 	if err != nil {
 		return err
diff --git a/internal/frontend/fetchserver/fetch_test.go b/internal/frontend/fetchserver/fetch_test.go
index c98585df..7f05fd56 100644
--- a/internal/frontend/fetchserver/fetch_test.go
+++ b/internal/frontend/fetchserver/fetch_test.go
@@ -28,7 +28,6 @@ import (
 )
 
 var (
-	sourceTimeout       = 1 * time.Second
 	testModulePath      = "github.com/module"
 	testSemver          = "v1.5.2"
 	testFetchTimeout    = 300 * time.Second
@@ -48,7 +47,7 @@ var (
 func newTestServerWithFetch(t *testing.T, proxyModules []*proxytest.Module, cacher frontend.Cacher) (*frontend.Server, *FetchServer, http.Handler, func()) {
 	t.Helper()
 	proxyClient, teardown := proxytest.SetupTestClient(t, proxyModules)
-	sourceClient := source.NewClient(sourceTimeout)
+	sourceClient := source.NewClient(http.DefaultClient)
 	ctx := context.Background()
 
 	q := queue.NewInMemory(ctx, 1, nil,
diff --git a/internal/source/source.go b/internal/source/source.go
index 813b5d55..3435d589 100644
--- a/internal/source/source.go
+++ b/internal/source/source.go
@@ -30,9 +30,7 @@ import (
 	"regexp"
 	"strconv"
 	"strings"
-	"time"
 
-	"go.opencensus.io/plugin/ochttp"
 	"go.opencensus.io/trace"
 	"golang.org/x/net/context/ctxhttp"
 	"golang.org/x/pkgsite/internal/derrors"
@@ -206,14 +204,9 @@ type Client struct {
 	httpClient *http.Client
 }
 
-// New constructs a *Client using the provided timeout.
-func NewClient(timeout time.Duration) *Client {
-	return &Client{
-		httpClient: &http.Client{
-			Transport: &ochttp.Transport{},
-			Timeout:   timeout,
-		},
-	}
+// New constructs a *Client using the provided *http.Client.
+func NewClient(httpClient *http.Client) *Client {
+	return &Client{httpClient: httpClient}
 }
 
 // NewClientForTesting returns a Client suitable for testing. It returns the
diff --git a/internal/source/source_test.go b/internal/source/source_test.go
index 451e39f9..5343ae83 100644
--- a/internal/source/source_test.go
+++ b/internal/source/source_test.go
@@ -15,7 +15,6 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-replayers/httpreplay"
@@ -730,12 +729,7 @@ func TestRemoveVersionSuffix(t *testing.T) {
 func TestAdjustVersionedModuleDirectory(t *testing.T) {
 	ctx := context.Background()
 
-	testTimeout := 72 * time.Hour // arbitrary
-	if deadline, ok := t.Deadline(); ok {
-		testTimeout = time.Until(deadline) * 9 / 10 // Allow 10% for error reporting and cleanup.
-	}
-
-	client := NewClient(testTimeout)
+	client := NewClient(http.DefaultClient)
 	client.httpClient.Transport = testTransport(map[string]string{
 		// Repo "branch" follows the "major branch" convention: versions 2 and higher
 		// live in the same directory as versions 0 and 1, but on a different branch (or tag).
diff --git a/internal/testing/integration/frontend_test.go b/internal/testing/integration/frontend_test.go
index 4b660637..e9cbe997 100644
--- a/internal/testing/integration/frontend_test.go
+++ b/internal/testing/integration/frontend_test.go
@@ -88,7 +88,7 @@ func setupFrontend(ctx context.Context, t *testing.T, q queue.Queue, rc *redis.C
 func setupQueue(ctx context.Context, t *testing.T, proxyModules []*proxytest.Module, experimentNames ...string) (queue.Queue, func()) {
 	cctx, cancel := context.WithCancel(ctx)
 	proxyClient, teardown := proxytest.SetupTestClient(t, proxyModules)
-	sourceClient := source.NewClient(1 * time.Second)
+	sourceClient := source.NewClient(http.DefaultClient)
 	q := queue.NewInMemory(cctx, 1, experimentNames,
 		func(ctx context.Context, mpath, version string) (_ int, err error) {
 			return fetchserver.FetchAndUpdateState(ctx, mpath, version, proxyClient, sourceClient, testDB)
@@ -110,7 +110,7 @@ func processVersions(ctx context.Context, t *testing.T, testModules []*proxytest
 }
 
 func fetchAndInsertModule(ctx context.Context, t *testing.T, tm *proxytest.Module, proxyClient *proxy.Client) {
-	sourceClient := source.NewClient(1 * time.Second)
+	sourceClient := source.NewClient(http.DefaultClient)
 	res := fetch.FetchModule(ctx, tm.ModulePath, tm.Version, fetch.NewProxyModuleGetter(proxyClient, sourceClient))
 	if res.Error != nil {
 		t.Fatal(res.Error)
diff --git a/internal/testing/integration/worker_test.go b/internal/testing/integration/worker_test.go
index 110a21b8..d6f75766 100644
--- a/internal/testing/integration/worker_test.go
+++ b/internal/testing/integration/worker_test.go
@@ -14,7 +14,6 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"testing"
-	"time"
 
 	"github.com/alicebob/miniredis/v2"
 	"github.com/go-redis/redis/v8"
@@ -34,7 +33,7 @@ func setupWorker(ctx context.Context, t *testing.T, proxyClient *proxy.Client, i
 
 	fetcher := &worker.Fetcher{
 		ProxyClient:  proxyClient,
-		SourceClient: source.NewClient(1 * time.Second),
+		SourceClient: source.NewClient(http.DefaultClient),
 		DB:           testDB,
 		Cache:        cache.New(redisCacheClient),
 	}
@@ -49,7 +48,7 @@ func setupWorker(ctx context.Context, t *testing.T, proxyClient *proxy.Client, i
 		DB:               testDB,
 		IndexClient:      indexClient,
 		ProxyClient:      proxyClient,
-		SourceClient:     source.NewClient(1 * time.Second),
+		SourceClient:     source.NewClient(http.DefaultClient),
 		RedisCacheClient: redisCacheClient,
 		Queue:            queue,
 		StaticPath:       template.TrustedSourceFromConstant("../../../static"),
diff --git a/internal/worker/fetch_test.go b/internal/worker/fetch_test.go
index d0bdf98a..8bbe09b9 100644
--- a/internal/worker/fetch_test.go
+++ b/internal/worker/fetch_test.go
@@ -7,6 +7,7 @@ package worker
 import (
 	"context"
 	"fmt"
+	"net/http"
 	"sort"
 	"strings"
 	"testing"
@@ -37,7 +38,6 @@ const (
 )
 
 var (
-	sourceTimeout       = 1 * time.Second
 	testProxyCommitTime = time.Date(2019, 1, 30, 0, 0, 0, 0, time.UTC)
 )
 
@@ -190,7 +190,7 @@ func TestFetchAndUpdateState(t *testing.T) {
 		},
 	}
 
-	sourceClient := source.NewClient(sourceTimeout)
+	sourceClient := source.NewClient(http.DefaultClient)
 	for _, test := range testCases {
 		t.Run(strings.ReplaceAll(test.pkg+"@"+test.version, "/", " "), func(t *testing.T) {
 			defer postgres.ResetTestDB(testDB, t)
@@ -294,7 +294,7 @@ func TestFetchAndUpdateStateCacheZip(t *testing.T) {
 	defer teardownProxy()
 
 	// With a plain proxy, we download the zip twice.
-	f := &Fetcher{proxyClient, source.NewClient(sourceTimeout), testDB, nil, nil, ""}
+	f := &Fetcher{proxyClient, source.NewClient(http.DefaultClient), testDB, nil, nil, ""}
 	if _, _, err := f.FetchAndUpdateState(ctx, "m.com", "v1.0.0", testAppVersion); err != nil {
 		t.Fatal(err)
 	}
@@ -323,7 +323,7 @@ func TestFetchAndUpdateLatest(t *testing.T) {
 	const modulePath = "example.com/retractions"
 	f := &Fetcher{
 		ProxyClient:  prox,
-		SourceClient: source.NewClient(sourceTimeout),
+		SourceClient: source.NewClient(http.DefaultClient),
 		DB:           testDB,
 	}
 	got, err := f.FetchAndUpdateLatest(ctx, modulePath)
@@ -363,7 +363,7 @@ func TestFetchGo121(t *testing.T) {
 	})
 	defer teardownProxy()
 
-	sourceClient := source.NewClient(sourceTimeout)
+	sourceClient := source.NewClient(http.DefaultClient)
 	f := &Fetcher{proxyClient, sourceClient, testDB, nil, nil, ""}
 	got, _, err := f.FetchAndUpdateState(context.Background(), modulePath, version, testAppVersion)
 	if err != nil {
diff --git a/internal/worker/fetcherror_test.go b/internal/worker/fetcherror_test.go
index 89367835..6b57a10d 100644
--- a/internal/worker/fetcherror_test.go
+++ b/internal/worker/fetcherror_test.go
@@ -325,7 +325,7 @@ func TestTrimLargeCode(t *testing.T) {
 
 func fetchAndCheckStatus(ctx context.Context, t *testing.T, proxyClient *proxy.Client, modulePath, version string, wantCode int) {
 	t.Helper()
-	f := Fetcher{proxyClient, source.NewClient(sourceTimeout), testDB, nil, nil, ""}
+	f := Fetcher{proxyClient, source.NewClient(http.DefaultClient), testDB, nil, nil, ""}
 	code, _, err := f.FetchAndUpdateState(ctx, modulePath, version, testAppVersion)
 	switch code {
 	case http.StatusOK:
diff --git a/internal/worker/refetch_test.go b/internal/worker/refetch_test.go
index fd97b622..d8e87d18 100644
--- a/internal/worker/refetch_test.go
+++ b/internal/worker/refetch_test.go
@@ -7,6 +7,7 @@ package worker
 import (
 	"context"
 	"errors"
+	"net/http"
 	"testing"
 	"time"
 
@@ -59,7 +60,7 @@ func TestReFetch(t *testing.T) {
 		},
 	})
 	defer teardownProxy()
-	sourceClient := source.NewClient(sourceTimeout)
+	sourceClient := source.NewClient(http.DefaultClient)
 	f := &Fetcher{proxyClient, sourceClient, testDB, nil, nil, ""}
 	if _, _, err := f.FetchAndUpdateState(ctx, sample.ModulePath, version, testAppVersion); err != nil {
 		t.Fatalf("FetchAndUpdateState(%q, %q): %v", sample.ModulePath, version, err)
diff --git a/internal/worker/server_test.go b/internal/worker/server_test.go
index d4a055a0..d87d6f2c 100644
--- a/internal/worker/server_test.go
+++ b/internal/worker/server_test.go
@@ -176,7 +176,7 @@ func TestWorker(t *testing.T) {
 			proxyClient, teardownProxy := proxytest.SetupTestClient(t, test.proxy)
 			defer teardownProxy()
 			defer postgres.ResetTestDB(testDB, t)
-			f := &Fetcher{proxyClient, source.NewClient(sourceTimeout), testDB, nil, nil, ""}
+			f := &Fetcher{proxyClient, source.NewClient(http.DefaultClient), testDB, nil, nil, ""}
 
 			// Use 10 workers to have parallelism consistent with the worker binary.
 			q := queue.NewInMemory(ctx, 10, nil, func(ctx context.Context, mpath, version string) (int, error) {