Charmhub: Use transport interface

Drop utils.Client and instead use the juju/http.Client instead. The following means we can use an interface instead of a http.Client directly. This will make things a lot easier for testing.
sdg0919 · May 26, 2021 · a6380f3 · a6380f3
1 parent efa9006
commit a6380f3
Show file tree

Hide file tree

Showing 12 changed files with 64 additions and 55 deletions.
diff --git a/apiserver/common/charmhub.go b/apiserver/common/charmhub.go
@@ -4,8 +4,6 @@
 package common
 
 import (
-	"net/http"
-
 	"github.com/juju/errors"
 	"github.com/juju/loggo"
 
@@ -24,7 +22,7 @@ type ConfigModel interface {
 }
 
 // CharmhubClient creates a new charmhub Client based on this model's config.
-func CharmhubClient(mg ModelGetter, httpClient *http.Client, logger loggo.Logger, metadata map[string]string) (*charmhub.Client, error) {
+func CharmhubClient(mg ModelGetter, httpClient charmhub.Transport, logger loggo.Logger, metadata map[string]string) (*charmhub.Client, error) {
 	model, err := mg.Model()
 	if err != nil {
 		return nil, errors.Trace(err)
@@ -37,7 +35,9 @@ func CharmhubClient(mg ModelGetter, httpClient *http.Client, logger loggo.Logger
 
 	config, err := charmhub.CharmHubConfigFromURL(url, logger.Child("charmhub"),
 		charmhub.WithMetadataHeaders(metadata),
-		charmhub.WithHTTPClient(httpClient),
+		charmhub.WithHTTPTransport(func(charmhub.Logger) charmhub.Transport {
+			return httpClient
+		}),
 	)
 	if err != nil {
 		return nil, errors.Trace(err)

diff --git a/apiserver/facades/client/application/application.go b/apiserver/facades/client/application/application.go
@@ -277,17 +277,18 @@ func newFacadeBase(ctx facade.Context) (*APIBase, error) {
 		return nil, errors.Trace(err)
 	}
 
+	clientLogger := logger.Child("client")
 	options := []charmhub.Option{
-		// TODO (stickupkid): Get the httpClient from the facade context
-		charmhub.WithHTTPClient(charmhub.DefaultHTTPTransport()),
+		// TODO (stickupkid): Get the http transport from the facade context
+		charmhub.WithHTTPTransport(charmhub.DefaultHTTPTransport),
 	}
 
 	var chCfg charmhub.Config
 	chURL, ok := modelCfg.CharmHubURL()
 	if ok {
-		chCfg, err = charmhub.CharmHubConfigFromURL(chURL, logger.Child("client"), options...)
+		chCfg, err = charmhub.CharmHubConfigFromURL(chURL, clientLogger, options...)
 	} else {
-		chCfg, err = charmhub.CharmHubConfig(logger.Child("client"), options...)
+		chCfg, err = charmhub.CharmHubConfig(clientLogger, options...)
 	}
 	if err != nil {
 		return nil, errors.Trace(err)

diff --git a/apiserver/facades/client/charmhub/charmhub.go b/apiserver/facades/client/charmhub/charmhub.go
@@ -5,7 +5,6 @@ package charmhub
 
 import (
 	"context"
-	"net/http"
 	"time"
 
 	"github.com/juju/errors"
@@ -61,10 +60,9 @@ func NewFacade(ctx facade.Context) (*CharmHubAPI, error) {
 		return nil, errors.Trace(err)
 	}
 
-	// TODO (stickupkid): Get the httpClient from the facade context
-	httpClient := charmhub.DefaultHTTPTransport()
+	// TODO (stickupkid): Get the http transport from the facade context
 	return newCharmHubAPI(m, ctx.Auth(), charmHubClientFactory{
-		httpClient: httpClient,
+		httpTransport: charmhub.DefaultHTTPTransport,
 	})
 }
 
@@ -135,12 +133,12 @@ func (api *CharmHubAPI) Find(ctx context.Context, arg params.Query) (params.Char
 }
 
 type charmHubClientFactory struct {
-	httpClient *http.Client
+	httpTransport func(charmhub.Logger) charmhub.Transport
 }
 
 func (f charmHubClientFactory) Client(url string) (Client, error) {
 	cfg, err := charmhub.CharmHubConfigFromURL(url, logger.Child("client"),
-		charmhub.WithHTTPClient(f.httpClient),
+		charmhub.WithHTTPTransport(f.httpTransport),
 	)
 	if err != nil {
 		return nil, errors.Trace(err)

diff --git a/apiserver/facades/client/charms/client.go b/apiserver/facades/client/charms/client.go
@@ -6,12 +6,12 @@ package charms
 import (
 	"fmt"
 	"io"
-	"net/http"
 	"strings"
 
 	"github.com/juju/charm/v8"
 	"github.com/juju/collections/set"
 	"github.com/juju/errors"
+	"github.com/juju/http"
 	"github.com/juju/juju/state"
 	"github.com/juju/mgo/v2"
 	"github.com/juju/names/v4"
@@ -46,7 +46,7 @@ type API struct {
 	getStrategyFunc      func(source string) StrategyFunc
 	newStorage           func(modelUUID string, session *mgo.Session) storage.Storage
 	tag                  names.ModelTag
-	httpClient           *http.Client
+	httpClient           http.HTTPClient
 }
 
 type APIv2 struct {
@@ -134,8 +134,8 @@ func NewFacadeV4(ctx facade.Context) (*API, error) {
 		getStrategyFunc:      getStrategyFunc,
 		newStorage:           storage.NewStorage,
 		tag:                  m.ModelTag(),
-		// TODO (stickupkid): Get the httpClient from the facade context
-		httpClient: charmhub.DefaultHTTPTransport(),
+		// TODO (stickupkid): Get the http transport from the facade context
+		httpClient: charmhub.DefaultHTTPTransport(logger),
 	}, nil
 }
 
@@ -155,8 +155,8 @@ func NewCharmsAPI(
 		getStrategyFunc:      getStrategyFunc,
 		newStorage:           newStorage,
 		tag:                  m.ModelTag(),
-		// TODO (stickupkid): Get the httpClient from the facade context
-		httpClient: charmhub.DefaultHTTPTransport(),
+		// TODO (stickupkid): Get the http transport from the facade context
+		httpClient: charmhub.DefaultHTTPTransport(logger),
 	}, nil
 }
 
@@ -645,16 +645,19 @@ func (a *API) charmHubRepository() (corecharm.Repository, error) {
 		return nil, errors.Trace(err)
 	}
 
+	clientLogger := logger.Child("client")
 	options := []charmhub.Option{
-		charmhub.WithHTTPClient(a.httpClient),
+		charmhub.WithHTTPTransport(func(charmhub.Logger) charmhub.Transport {
+			return a.httpClient
+		}),
 	}
 
 	var chCfg charmhub.Config
 	chURL, ok := cfg.CharmHubURL()
 	if ok {
-		chCfg, err = charmhub.CharmHubConfigFromURL(chURL, logger.Child("client"), options...)
+		chCfg, err = charmhub.CharmHubConfigFromURL(chURL, clientLogger, options...)
 	} else {
-		chCfg, err = charmhub.CharmHubConfig(logger.Child("client"), options...)
+		chCfg, err = charmhub.CharmHubConfig(clientLogger, options...)
 	}
 	if err != nil {
 		return nil, errors.Trace(err)

diff --git a/apiserver/facades/client/machinemanager/machinemanager.go b/apiserver/facades/client/machinemanager/machinemanager.go
@@ -102,17 +102,18 @@ func NewFacade(ctx facade.Context) (*MachineManagerAPI, error) {
 		return nil, errors.Trace(err)
 	}
 
+	clientLogger := logger.Child("client")
 	options := []charmhub.Option{
-		// TODO (stickupkid): Get the httpClient from the facade context
-		charmhub.WithHTTPClient(charmhub.DefaultHTTPTransport()),
+		// TODO (stickupkid): Get the http transport from the facade context
+		charmhub.WithHTTPTransport(charmhub.DefaultHTTPTransport),
 	}
 
 	var chCfg charmhub.Config
 	chURL, ok := modelCfg.CharmHubURL()
 	if ok {
-		chCfg, err = charmhub.CharmHubConfigFromURL(chURL, logger.Child("client"), options...)
+		chCfg, err = charmhub.CharmHubConfigFromURL(chURL, clientLogger, options...)
 	} else {
-		chCfg, err = charmhub.CharmHubConfig(logger.Child("client"), options...)
+		chCfg, err = charmhub.CharmHubConfig(clientLogger, options...)
 	}
 	if err != nil {
 		return nil, errors.Trace(err)

diff --git a/apiserver/facades/client/resources/facade.go b/apiserver/facades/client/resources/facade.go
@@ -80,17 +80,18 @@ func NewFacadeV2(ctx facade.Context) (*API, error) {
 		switch {
 		case charm.CharmHub.Matches(schema):
 
+			clientLogger := logger.Child("client")
 			options := []charmhub.Option{
 				// TODO (stickupkid): Get the httpClient from the facade context
-				charmhub.WithHTTPClient(charmhub.DefaultHTTPTransport()),
+				charmhub.WithHTTPTransport(charmhub.DefaultHTTPTransport),
 			}
 
 			var chCfg charmhub.Config
 			chURL, ok := modelCfg.CharmHubURL()
 			if ok {
-				chCfg, err = charmhub.CharmHubConfigFromURL(chURL, logger.Child("client"), options...)
+				chCfg, err = charmhub.CharmHubConfigFromURL(chURL, clientLogger, options...)
 			} else {
-				chCfg, err = charmhub.CharmHubConfig(logger.Child("client"), options...)
+				chCfg, err = charmhub.CharmHubConfig(clientLogger, options...)
 			}
 			if err != nil {
 				return nil, errors.Trace(err)

diff --git a/apiserver/facades/controller/charmrevisionupdater/updater.go b/apiserver/facades/controller/charmrevisionupdater/updater.go
@@ -57,9 +57,9 @@ func NewCharmRevisionUpdaterAPI(ctx facade.Context) (*CharmRevisionUpdaterAPI, e
 		return charmstore.NewCachingClient(state.MacaroonCache{MacaroonCacheState: st}, controllerCfg.CharmStoreURL())
 	}
 	newCharmhubClient := func(st State, metadata map[string]string) (CharmhubRefreshClient, error) {
-		// TODO (stickupkid): Get the httpClient from the facade context
-		httpClient := charmhub.DefaultHTTPTransport()
-		return common.CharmhubClient(charmhubClientStateShim{state: st}, httpClient, logger, metadata)
+		// TODO (stickupkid): Get the http transport from the facade context
+		transport := charmhub.DefaultHTTPTransport(logger)
+		return common.CharmhubClient(charmhubClientStateShim{state: st}, transport, logger, metadata)
 	}
 	return NewCharmRevisionUpdaterAPIState(
 		StateShim{State: ctx.State()},

diff --git a/charmhub/client.go b/charmhub/client.go
@@ -79,9 +79,9 @@ type Config struct {
 	// and allow overriding existing headers.
 	Headers http.Header
 
-	// HTTPClient represents the default http.Client to use for all API
+	// Transport represents the default http transport to use for all API
 	// requests.
-	HTTPClient *http.Client
+	Transport Transport
 
 	// Logger to use during the API requests.
 	Logger Logger
@@ -93,7 +93,7 @@ type Option func(*options)
 type options struct {
 	url             *string
 	metadataHeaders map[string]string
-	httpClient      *http.Client
+	transportFunc   func(Logger) Transport
 }
 
 // WithURL sets the url on the option.
@@ -110,19 +110,21 @@ func WithMetadataHeaders(h map[string]string) Option {
 	}
 }
 
-// WithHTTPClient sets the default http.Client to use on the option.
-func WithHTTPClient(client *http.Client) Option {
+// WithHTTPTransport sets the default http transport to use on the option.
+func WithHTTPTransport(transportFn func(Logger) Transport) Option {
 	return func(options *options) {
-		options.httpClient = client
+		options.transportFunc = transportFn
 	}
 }
 
 // Create a options instance with default values.
 func newOptions() *options {
 	u := CharmHubServerURL
 	return &options{
-		url:        &u,
-		httpClient: DefaultHTTPTransport(),
+		url: &u,
+		transportFunc: func(logger Logger) Transport {
+			return DefaultHTTPTransport(logger)
+		},
 	}
 }
 
@@ -156,11 +158,12 @@ func CharmHubConfig(logger Logger, options ...Option) (Config, error) {
 	}
 
 	return Config{
-		URL:     *opts.url,
-		Version: CharmHubServerVersion,
-		Entity:  CharmHubServerEntity,
-		Headers: headers,
-		Logger:  logger,
+		URL:       *opts.url,
+		Version:   CharmHubServerVersion,
+		Entity:    CharmHubServerEntity,
+		Headers:   headers,
+		Transport: opts.transportFunc(logger),
+		Logger:    logger,
 	}, nil
 }
 
@@ -228,7 +231,7 @@ func NewClientWithFileSystem(config Config, fileSystem FileSystem) (*Client, err
 
 	config.Logger.Tracef("NewClient to %q", config.URL)
 
-	apiRequester := NewAPIRequester(config.HTTPClient, config.Logger)
+	apiRequester := NewAPIRequester(config.Transport, config.Logger)
 	restClient := NewHTTPRESTClient(apiRequester, config.Headers)
 
 	return &Client{
@@ -239,7 +242,7 @@ func NewClientWithFileSystem(config Config, fileSystem FileSystem) (*Client, err
 		// download client doesn't require a path here, as the download could
 		// be from any server in theory. That information is found from the
 		// refresh response.
-		downloadClient:  NewDownloadClient(config.HTTPClient, fileSystem, config.Logger),
+		downloadClient:  NewDownloadClient(config.Transport, fileSystem, config.Logger),
 		resourcesClient: NewResourcesClient(resourcesPath, restClient, config.Logger),
 		logger:          config.Logger,
 	}, nil

diff --git a/charmhub/find_test.go b/charmhub/find_test.go
@@ -175,7 +175,7 @@ func (s *FindSuite) TestFindRequestPayload(c *gc.C) {
 	findPath, err := basePath.Join("find")
 	c.Assert(err, jc.ErrorIsNil)
 
-	apiRequester := NewAPIRequester(DefaultHTTPTransport(), &FakeLogger{})
+	apiRequester := NewAPIRequester(DefaultHTTPTransport(&FakeLogger{}), &FakeLogger{})
 	restClient := NewHTTPRESTClient(apiRequester, nil)
 
 	client := NewFindClient(findPath, restClient, &FakeLogger{})
@@ -212,7 +212,7 @@ func (s *FindSuite) TestFindErrorPayload(c *gc.C) {
 	findPath, err := basePath.Join("find")
 	c.Assert(err, jc.ErrorIsNil)
 
-	apiRequester := NewAPIRequester(DefaultHTTPTransport(), &FakeLogger{})
+	apiRequester := NewAPIRequester(DefaultHTTPTransport(&FakeLogger{}), &FakeLogger{})
 	restClient := NewHTTPRESTClient(apiRequester, nil)
 
 	client := NewFindClient(findPath, restClient, &FakeLogger{})

diff --git a/charmhub/http.go b/charmhub/http.go
@@ -14,7 +14,7 @@ import (
 	"sort"
 
 	"github.com/juju/errors"
-	"github.com/juju/utils"
+	jujuhttp "github.com/juju/http"
 	"gopkg.in/httprequest.v1"
 
 	"github.com/juju/juju/charmhub/path"
@@ -36,8 +36,10 @@ type Transport interface {
 }
 
 // DefaultHTTPTransport creates a new HTTPTransport.
-func DefaultHTTPTransport() *http.Client {
-	return utils.GetHTTPClient(utils.VerifySSLHostnames)
+func DefaultHTTPTransport(logger Logger) Transport {
+	return jujuhttp.NewClient(jujuhttp.Config{
+		Logger: logger,
+	})
 }
 
 // APIRequester creates a wrapper around the transport to allow for better

diff --git a/charmhub/info_test.go b/charmhub/info_test.go
@@ -252,7 +252,7 @@ func (s *InfoSuite) TestInfoRequestPayload(c *gc.C) {
 	infoPath, err := basePath.Join("info")
 	c.Assert(err, jc.ErrorIsNil)
 
-	apiRequester := NewAPIRequester(DefaultHTTPTransport(), &FakeLogger{})
+	apiRequester := NewAPIRequester(DefaultHTTPTransport(&FakeLogger{}), &FakeLogger{})
 	restClient := NewHTTPRESTClient(apiRequester, nil)
 
 	client := NewInfoClient(infoPath, restClient, &FakeLogger{})

diff --git a/charmhub/resources_test.go b/charmhub/resources_test.go
@@ -101,7 +101,7 @@ func (s *ResourcesSuite) TestListResourceRevisionsRequestPayload(c *gc.C) {
 	resourcesPath, err := basePath.Join("resources")
 	c.Assert(err, jc.ErrorIsNil)
 
-	apiRequester := NewAPIRequester(DefaultHTTPTransport(), &FakeLogger{})
+	apiRequester := NewAPIRequester(DefaultHTTPTransport(&FakeLogger{}), &FakeLogger{})
 	restClient := NewHTTPRESTClient(apiRequester, nil)
 
 	client := NewResourcesClient(resourcesPath, restClient, &FakeLogger{})