Tidy up the way we create HTTP clients

- Instead of providing our own interface around http.Client, stub out
  RoundTripper instead. I've observed that most third-party packages
  don't declare their own interfaces around http.Client, and I don't
  feel like patching those up. RoundTripper will do.

- Create a special Protobuf message for HTTP client configuration
  options. This will initially just contain TLS settings, but later on
  we can add stuff like proxy settings there as well.

- Make HTTP client settings configurable in a couple more places where
  this is currently not allowed.

internal/mock/BUILD.bazel

@@ -8,6 +8,7 @@ gomock(
     out = "aliases.go",
     interfaces = [
         "ReadCloser",
+        "RoundTripper",
         "Writer",
     ],
     library = "//internal/mock/aliases",
@@ -191,19 +192,6 @@ gomock(
     package = "mock",
 )
 
-gomock(
-    name = "http",
-    out = "http.go",
-    interfaces = [
-        "Client",
-    ],
-    library = "//pkg/http",
-    mock_names = {
-        "Client": "MockHTTPClient",
-    },
-    package = "mock",
-)
-
 gomock(
     name = "redis",
     out = "redis.go",
@@ -265,7 +253,6 @@ go_library(
         ":filesystem_path.go",
         ":grpc.go",
         ":grpc_go.go",
-        ":http.go",
         ":redis.go",
         ":remoteexecution.go",
         ":trace.go",

internal/mock/aliases/aliases.go

@@ -2,6 +2,7 @@ package aliases
 
 import (
 	"io"
+	"net/http"
 )
 
 // This file contains aliases for some of the interfaces provided by the
@@ -13,5 +14,8 @@ import (
 // ReadCloser is an alias of io.ReadCloser.
 type ReadCloser = io.ReadCloser
 
+// RoundTripper is an alias of http.RoundTripper.
+type RoundTripper = http.RoundTripper
+
 // Writer is an alias of io.Writer.
 type Writer = io.Writer

pkg/blobstore/BUILD.bazel

@@ -30,7 +30,6 @@ go_library(
         "//pkg/clock",
         "//pkg/cloud/aws",
         "//pkg/digest",
-        "//pkg/http",
         "//pkg/proto/icas",
         "//pkg/proto/iscc",
         "//pkg/util",

pkg/blobstore/configuration/cas_blob_access_creator.go

@@ -1,6 +1,7 @@
 package configuration
 
 import (
+	"net/http"
 	"sync"
 
 	"github.com/aws/aws-sdk-go-v2/service/s3"
@@ -10,7 +11,7 @@ import (
 	"github.com/buildbarn/bb-storage/pkg/cloud/aws"
 	"github.com/buildbarn/bb-storage/pkg/digest"
 	"github.com/buildbarn/bb-storage/pkg/grpc"
-	"github.com/buildbarn/bb-storage/pkg/http"
+	bb_http "github.com/buildbarn/bb-storage/pkg/http"
 	pb "github.com/buildbarn/bb-storage/pkg/proto/configuration/blobstore"
 	"github.com/buildbarn/bb-storage/pkg/util"
 	"github.com/google/uuid"
@@ -103,15 +104,14 @@ func (bac *casBlobAccessCreator) NewCustomBlobAccess(configuration *pb.BlobAcces
 		if err != nil {
 			return BlobAccessInfo{}, "", util.StatusWrap(err, "Failed to create AWS config")
 		}
-		// TODO: Add TLS client options to configuration schema.
-		httpClient, err := http.NewClient(nil)
+		roundTripper, err := bb_http.NewRoundTripperFromConfiguration(backend.ReferenceExpanding.HttpClient)
 		if err != nil {
 			return BlobAccessInfo{}, "", util.StatusWrap(err, "Failed to create HTTP client")
 		}
 		return BlobAccessInfo{
 			BlobAccess: blobstore.NewReferenceExpandingBlobAccess(
 				base.BlobAccess,
-				httpClient,
+				&http.Client{Transport: roundTripper},
 				s3.NewFromConfig(cfg),
 				bac.maximumMessageSizeBytes),
 			DigestKeyFormat: base.DigestKeyFormat,

pkg/blobstore/configuration/new_blob_access.go

@@ -1,6 +1,7 @@
 package configuration
 
 import (
+	"net/http"
 	"sync"
 	"time"
 
@@ -15,7 +16,7 @@ import (
 	"github.com/buildbarn/bb-storage/pkg/digest"
 	"github.com/buildbarn/bb-storage/pkg/filesystem"
 	"github.com/buildbarn/bb-storage/pkg/grpc"
-	"github.com/buildbarn/bb-storage/pkg/http"
+	bb_http "github.com/buildbarn/bb-storage/pkg/http"
 	pb "github.com/buildbarn/bb-storage/pkg/proto/configuration/blobstore"
 	"github.com/buildbarn/bb-storage/pkg/random"
 	"github.com/buildbarn/bb-storage/pkg/util"
@@ -169,12 +170,16 @@ func newNestedBlobAccessBare(configuration *pb.BlobAccessConfiguration, creator
 			DigestKeyFormat: digestKeyFormat,
 		}, "redis", nil
 	case *pb.BlobAccessConfiguration_Http:
-		httpClient, err := http.NewClient(backend.Http.Tls)
+		roundTripper, err := bb_http.NewRoundTripperFromConfiguration(backend.Http.Client)
 		if err != nil {
 			return BlobAccessInfo{}, "", util.StatusWrap(err, "Failed to create HTTP client")
 		}
 		return BlobAccessInfo{
-			BlobAccess:      blobstore.NewHTTPBlobAccess(backend.Http.Address, storageTypeName, readBufferFactory, httpClient),
+			BlobAccess: blobstore.NewHTTPBlobAccess(
+				backend.Http.Address,
+				storageTypeName,
+				readBufferFactory,
+				&http.Client{Transport: roundTripper}),
 			DigestKeyFormat: digest.KeyWithInstance,
 		}, "remote", nil
 	case *pb.BlobAccessConfiguration_Sharding:

pkg/blobstore/http_blob_access.go

@@ -7,7 +7,6 @@ import (
 
 	"github.com/buildbarn/bb-storage/pkg/blobstore/buffer"
 	"github.com/buildbarn/bb-storage/pkg/digest"
-	bb_http "github.com/buildbarn/bb-storage/pkg/http"
 	"github.com/buildbarn/bb-storage/pkg/util"
 
 	"google.golang.org/grpc/codes"
@@ -18,7 +17,7 @@ type httpBlobAccess struct {
 	address           string
 	prefix            string
 	readBufferFactory ReadBufferFactory
-	httpClient        bb_http.Client
+	httpClient        *http.Client
 }
 
 func convertHTTPUnexpectedStatus(resp *http.Response) error {
@@ -28,7 +27,7 @@ func convertHTTPUnexpectedStatus(resp *http.Response) error {
 // NewHTTPBlobAccess for use of HTTP/1.1 cache backend.
 //
 // See: https://docs.bazel.build/versions/master/remote-caching.html#http-caching-protocol
-func NewHTTPBlobAccess(address, prefix string, readBufferFactory ReadBufferFactory, httpClient bb_http.Client) BlobAccess {
+func NewHTTPBlobAccess(address, prefix string, readBufferFactory ReadBufferFactory, httpClient *http.Client) BlobAccess {
 	return &httpBlobAccess{
 		address:           address,
 		prefix:            prefix,

pkg/blobstore/reference_expanding_blob_access.go

@@ -13,7 +13,6 @@ import (
 	"github.com/buildbarn/bb-storage/pkg/blobstore/buffer"
 	cloud_aws "github.com/buildbarn/bb-storage/pkg/cloud/aws"
 	"github.com/buildbarn/bb-storage/pkg/digest"
-	bb_http "github.com/buildbarn/bb-storage/pkg/http"
 	"github.com/buildbarn/bb-storage/pkg/proto/icas"
 	"github.com/buildbarn/bb-storage/pkg/util"
 	"github.com/klauspost/compress/zstd"
@@ -24,7 +23,7 @@ import (
 
 type referenceExpandingBlobAccess struct {
 	blobAccess              BlobAccess
-	httpClient              bb_http.Client
+	httpClient              *http.Client
 	s3Client                cloud_aws.S3Client
 	maximumMessageSizeBytes int
 }
@@ -43,7 +42,7 @@ func getHTTPRangeHeader(reference *icas.Reference) string {
 // Storage (CAS) backend. Any object requested through this BlobAccess
 // will cause its reference to be loaded from the ICAS, followed by
 // fetching its data from the referenced location.
-func NewReferenceExpandingBlobAccess(blobAccess BlobAccess, httpClient bb_http.Client, s3Client cloud_aws.S3Client, maximumMessageSizeBytes int) BlobAccess {
+func NewReferenceExpandingBlobAccess(blobAccess BlobAccess, httpClient *http.Client, s3Client cloud_aws.S3Client, maximumMessageSizeBytes int) BlobAccess {
 	return &referenceExpandingBlobAccess{
 		blobAccess:              blobAccess,
 		httpClient:              httpClient,

pkg/blobstore/reference_expanding_blob_access_test.go

@@ -6,7 +6,6 @@ import (
 	"errors"
 	"io"
 	"net/http"
-	"net/url"
 	"testing"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
@@ -29,11 +28,11 @@ func TestReferenceExpandingBlobAccessGet(t *testing.T) {
 	ctrl, ctx := gomock.WithContext(context.Background(), t)
 
 	baseBlobAccess := mock.NewMockBlobAccess(ctrl)
-	httpClient := mock.NewMockHTTPClient(ctrl)
+	roundTripper := mock.NewMockRoundTripper(ctrl)
 	s3Client := mock.NewMockS3Client(ctrl)
 	blobAccess := blobstore.NewReferenceExpandingBlobAccess(
 		baseBlobAccess,
-		httpClient,
+		&http.Client{Transport: roundTripper},
 		s3Client,
 		100)
 	helloDigest := digest.MustNewDigest("instance", "8b1a9953c4611296a827abf8c47804d7", 5)
@@ -89,11 +88,7 @@ func TestReferenceExpandingBlobAccessGet(t *testing.T) {
 					SizeBytes:   5,
 				},
 				buffer.BackendProvided(buffer.Irreparable(helloDigest))))
-		httpClient.EXPECT().Do(gomock.Any()).Return(nil, &url.Error{
-			Op:  "Get",
-			URL: "http://example.com/file.txt",
-			Err: errors.New("dial tcp 1.2.3.4:80: connect: connection refused"),
-		})
+		roundTripper.EXPECT().RoundTrip(gomock.Any()).Return(nil, errors.New("dial tcp 1.2.3.4:80: connect: connection refused"))
 
 		_, err := blobAccess.Get(ctx, helloDigest).ToByteSlice(100)
 		require.Equal(t, status.Error(codes.Internal, "HTTP request failed: Get \"http://example.com/file.txt\": dial tcp 1.2.3.4:80: connect: connection refused"), err)
@@ -113,7 +108,7 @@ func TestReferenceExpandingBlobAccessGet(t *testing.T) {
 				},
 				buffer.BackendProvided(buffer.Irreparable(helloDigest))))
 		body := mock.NewMockReadCloser(ctrl)
-		httpClient.EXPECT().Do(gomock.Any()).Return(&http.Response{
+		roundTripper.EXPECT().RoundTrip(gomock.Any()).Return(&http.Response{
 			Status:     "404 Not Found",
 			StatusCode: 404,
 			Body:       body,
@@ -138,7 +133,7 @@ func TestReferenceExpandingBlobAccessGet(t *testing.T) {
 				},
 				buffer.BackendProvided(buffer.Irreparable(helloDigest))))
 		body := mock.NewMockReadCloser(ctrl)
-		httpClient.EXPECT().Do(gomock.Any()).Return(&http.Response{
+		roundTripper.EXPECT().RoundTrip(gomock.Any()).Return(&http.Response{
 			Status:     "206 Partial Content",
 			StatusCode: 206,
 			Body:       body,
@@ -166,7 +161,7 @@ func TestReferenceExpandingBlobAccessGet(t *testing.T) {
 				},
 				buffer.BackendProvided(buffer.Irreparable(helloDigest))))
 		body := mock.NewMockReadCloser(ctrl)
-		httpClient.EXPECT().Do(gomock.Any()).DoAndReturn(
+		roundTripper.EXPECT().RoundTrip(gomock.Any()).DoAndReturn(
 			func(req *http.Request) (*http.Response, error) {
 				require.Equal(t, "GET", req.Method)
 				require.Equal(t, "http://example.com/file.txt", req.URL.String())
@@ -332,11 +327,11 @@ func TestReferenceExpandingBlobAccessPut(t *testing.T) {
 	ctrl, ctx := gomock.WithContext(context.Background(), t)
 
 	baseBlobAccess := mock.NewMockBlobAccess(ctrl)
-	httpClient := mock.NewMockHTTPClient(ctrl)
+	roundTripper := mock.NewMockRoundTripper(ctrl)
 	s3Client := mock.NewMockS3Client(ctrl)
 	blobAccess := blobstore.NewReferenceExpandingBlobAccess(
 		baseBlobAccess,
-		httpClient,
+		&http.Client{Transport: roundTripper},
 		s3Client,
 		100)
 
@@ -361,11 +356,11 @@ func TestReferenceExpandingBlobAccessFindMissing(t *testing.T) {
 	ctrl, ctx := gomock.WithContext(context.Background(), t)
 
 	baseBlobAccess := mock.NewMockBlobAccess(ctrl)
-	httpClient := mock.NewMockHTTPClient(ctrl)
+	roundTripper := mock.NewMockRoundTripper(ctrl)
 	s3Client := mock.NewMockS3Client(ctrl)
 	blobAccess := blobstore.NewReferenceExpandingBlobAccess(
 		baseBlobAccess,
-		httpClient,
+		&http.Client{Transport: roundTripper},
 		s3Client,
 		100)
 

pkg/global/apply_configuration.go

@@ -139,11 +139,13 @@ func ApplyConfiguration(configuration *pb.Configuration) (*LifecycleState, bb_gr
 					if endpoint := jaegerConfiguration.Endpoint; endpoint != "" {
 						collectorEndpointOptions = append(collectorEndpointOptions, jaeger.WithEndpoint(endpoint))
 					}
-					httpClient, err := bb_http.NewClient(jaegerConfiguration.Tls)
+					roundTripper, err := bb_http.NewRoundTripperFromConfiguration(jaegerConfiguration.HttpClient)
 					if err != nil {
 						return nil, nil, util.StatusWrap(err, "Failed to create Jaeger collector HTTP client")
 					}
-					collectorEndpointOptions = append(collectorEndpointOptions, jaeger.WithHTTPClient(httpClient))
+					collectorEndpointOptions = append(collectorEndpointOptions, jaeger.WithHTTPClient(&http.Client{
+						Transport: roundTripper,
+					}))
 					if password := jaegerConfiguration.Password; password != "" {
 						collectorEndpointOptions = append(collectorEndpointOptions, jaeger.WithPassword(password))
 					}
@@ -264,12 +266,22 @@ func ApplyConfiguration(configuration *pb.Configuration) (*LifecycleState, bb_gr
 	if pushgateway := configuration.GetPrometheusPushgateway(); pushgateway != nil {
 		pusher := push.New(pushgateway.Url, pushgateway.Job)
 		pusher.Gatherer(prometheus.DefaultGatherer)
+		// TODO: Move this functionality into
+		// NewRoundTripperFromConfiguration, so that every HTTP
+		// client has support for specifying basic
+		// authentication credentials.
 		if basicAuthentication := pushgateway.BasicAuthentication; basicAuthentication != nil {
 			pusher.BasicAuth(basicAuthentication.Username, basicAuthentication.Password)
 		}
 		for key, value := range pushgateway.Grouping {
 			pusher.Grouping(key, value)
 		}
+		roundTripper, err := bb_http.NewRoundTripperFromConfiguration(pushgateway.HttpClient)
+		if err != nil {
+			return nil, nil, util.StatusWrap(err, "Failed to create Prometheus Pushgateway HTTP client")
+		}
+		pusher.Client(&http.Client{Transport: roundTripper})
+
 		pushInterval := pushgateway.PushInterval
 		if err := pushInterval.CheckValid(); err != nil {
 			return nil, nil, util.StatusWrap(err, "Failed to parse push interval")

pkg/http/BUILD.bazel

@@ -3,13 +3,13 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library")
 go_library(
     name = "http",
     srcs = [
-        "client.go",
+        "configuration.go",
         "status_code.go",
     ],
     importpath = "github.com/buildbarn/bb-storage/pkg/http",
     visibility = ["//visibility:public"],
     deps = [
-        "//pkg/proto/configuration/tls",
+        "//pkg/proto/configuration/http",
         "//pkg/util",
         "@org_golang_google_grpc//codes",
     ],