New http client for lxd connections.

A change introduced to Juju introduced the concept of re-using the HTTP
client for LXD connections to multiple different servers. The LXD client
however when passed it's own http server modifies the transport for the
http client. This causes the same http client to keep having it's
transport modified for different connections.

The result is that https validation fails for some lxd remotes because
the transport is configured for a different server.

Fixes LP2003135

apiserver/facades/client/modelupgrader/upgrader_test.go

@@ -4,8 +4,6 @@
 package modelupgrader_test
 
 import (
-	"net/http"
-
 	"github.com/golang/mock/gomock"
 	"github.com/juju/errors"
 	"github.com/juju/names/v4"
@@ -146,7 +144,7 @@ func (s *modelUpgradeSuite) assertUpgradeModelForControllerModelJuju3(c *gc.C, d
 	server := upgradevalidationmocks.NewMockServer(ctrl)
 	serverFactory := upgradevalidationmocks.NewMockServerFactory(ctrl)
 	s.PatchValue(&upgradevalidation.NewServerFactory,
-		func(httpClient *http.Client) lxd.ServerFactory {
+		func(_ lxd.NewHTTPClientFunc) lxd.ServerFactory {
 			return serverFactory
 		},
 	)
@@ -320,7 +318,7 @@ func (s *modelUpgradeSuite) TestUpgradeModelForControllerModelJuju3Failed(c *gc.
 	server := upgradevalidationmocks.NewMockServer(ctrl)
 	serverFactory := upgradevalidationmocks.NewMockServerFactory(ctrl)
 	s.PatchValue(&upgradevalidation.NewServerFactory,
-		func(httpClient *http.Client) lxd.ServerFactory {
+		func(_ lxd.NewHTTPClientFunc) lxd.ServerFactory {
 			return serverFactory
 		},
 	)
@@ -497,7 +495,7 @@ func (s *modelUpgradeSuite) assertUpgradeModelJuju3(c *gc.C, dryRun bool) {
 	server := upgradevalidationmocks.NewMockServer(ctrl)
 	serverFactory := upgradevalidationmocks.NewMockServerFactory(ctrl)
 	s.PatchValue(&upgradevalidation.NewServerFactory,
-		func(httpClient *http.Client) lxd.ServerFactory {
+		func(_ lxd.NewHTTPClientFunc) lxd.ServerFactory {
 			return serverFactory
 		},
 	)
@@ -603,7 +601,7 @@ func (s *modelUpgradeSuite) TestUpgradeModelJuju3Failed(c *gc.C) {
 	server := upgradevalidationmocks.NewMockServer(ctrl)
 	serverFactory := upgradevalidationmocks.NewMockServerFactory(ctrl)
 	s.PatchValue(&upgradevalidation.NewServerFactory,
-		func(httpClient *http.Client) lxd.ServerFactory {
+		func(_ lxd.NewHTTPClientFunc) lxd.ServerFactory {
 			return serverFactory
 		},
 	)

migration/precheck_test.go

@@ -4,7 +4,6 @@
 package migration_test
 
 import (
-	"net/http"
 	"strings"
 
 	"github.com/golang/mock/gomock"
@@ -92,7 +91,7 @@ func (s *SourcePrecheckSuite) TestTargetController3Failed(c *gc.C) {
 	server := upgradevalidationmocks.NewMockServer(ctrl)
 	serverFactory := upgradevalidationmocks.NewMockServerFactory(ctrl)
 	s.PatchValue(&upgradevalidation.NewServerFactory,
-		func(httpClient *http.Client) lxd.ServerFactory {
+		func(_ lxd.NewHTTPClientFunc) lxd.ServerFactory {
 			return serverFactory
 		},
 	)

provider/lxd/export_test.go

@@ -5,6 +5,7 @@ package lxd
 
 import (
 	"errors"
+	"net/http"
 
 	"github.com/juju/clock"
 
@@ -56,6 +57,9 @@ func NewServerFactoryWithMocks(localServerFunc func() (Server, error),
 		newRemoteServerFunc: remoteServerFunc,
 		interfaceAddress:    interfaceAddress,
 		clock:               clock,
+		newHTTPClientFunc: NewHTTPClientFunc(func() *http.Client {
+			return &http.Client{}
+		}),
 	}
 }
 

provider/lxd/provider.go

@@ -6,6 +6,7 @@ package lxd
 import (
 	stdcontext "context"
 	"io/ioutil"
+	"net/http"
 	"os"
 	"path/filepath"
 	"strings"
@@ -116,11 +117,13 @@ var cloudSchema = &jsonschema.Schema{
 
 // NewProvider returns a new LXD EnvironProvider.
 func NewProvider() environs.CloudEnvironProvider {
-	httpClient := jujuhttp.NewClient(
-		jujuhttp.WithLogger(logger.ChildWithLabels("http", corelogger.HTTP)),
-	)
 	configReader := lxcConfigReader{}
-	factory := NewServerFactory(httpClient.Client())
+	factory := NewServerFactory(NewHTTPClientFunc(func() *http.Client {
+		return jujuhttp.NewClient(
+			jujuhttp.WithLogger(logger.ChildWithLabels("http", corelogger.HTTP)),
+		).Client()
+	}))
+
 	credentials := environProviderCredentials{
 		certReadWriter:  certificateReadWriter{},
 		certGenerator:   certificateGenerator{},

provider/lxd/server.go

@@ -130,6 +130,10 @@ func (interfaceAddress) InterfaceAddress(interfaceName string) (string, error) {
 	return utils.GetAddressForInterface(interfaceName)
 }
 
+// NewHTTPClientFunc is responsible for generating a new http client every time
+// it is called.
+type NewHTTPClientFunc func() *http.Client
+
 type serverFactory struct {
 	newLocalServerFunc  func() (Server, error)
 	newRemoteServerFunc func(lxd.ServerSpec) (Server, error)
@@ -138,20 +142,23 @@ type serverFactory struct {
 	interfaceAddress    InterfaceAddress
 	clock               clock.Clock
 	mutex               sync.Mutex
-	httpClient          *http.Client
+	newHTTPClientFunc   NewHTTPClientFunc
 }
 
 // NewServerFactory creates a new ServerFactory with sane defaults.
-func NewServerFactory(httpClient *http.Client) ServerFactory {
+// A NewHTTPClientFunc is taken as an argument to address LP2003135. Previously
+// we reused the same http client for all LXD connections. This can't happen
+// as the LXD client code modifies the HTTP server.
+func NewServerFactory(newHttpFn NewHTTPClientFunc) ServerFactory {
 	return &serverFactory{
 		newLocalServerFunc: func() (Server, error) {
 			return lxd.NewLocalServer()
 		},
 		newRemoteServerFunc: func(spec lxd.ServerSpec) (Server, error) {
 			return lxd.NewRemoteServer(spec)
 		},
-		interfaceAddress: interfaceAddress{},
-		httpClient:       httpClient,
+		interfaceAddress:  interfaceAddress{},
+		newHTTPClientFunc: newHttpFn,
 	}
 }
 
@@ -209,7 +216,7 @@ func (s *serverFactory) RemoteServer(spec environscloudspec.CloudSpec) (Server,
 
 	serverSpec := lxd.NewServerSpec(spec.Endpoint, serverCert, clientCert).
 		WithProxy(proxy.DefaultConfig.GetProxy).
-		WithHTTPClient(s.httpClient)
+		WithHTTPClient(s.newHTTPClientFunc())
 
 	svr, err := s.newRemoteServerFunc(serverSpec)
 	if err == nil {
@@ -236,7 +243,7 @@ func (s *serverFactory) InsecureRemoteServer(spec environscloudspec.CloudSpec) (
 	serverSpec := lxd.NewInsecureServerSpec(spec.Endpoint).
 		WithClientCertificate(clientCert).
 		WithSkipGetServer(true).
-		WithHTTPClient(s.httpClient)
+		WithHTTPClient(s.newHTTPClientFunc())
 
 	svr, err := s.newRemoteServerFunc(serverSpec)
 	return svr, errors.Trace(err)

upgrades/upgradevalidation/migrate_test.go

@@ -4,8 +4,6 @@
 package upgradevalidation_test
 
 import (
-	"net/http"
-
 	"github.com/golang/mock/gomock"
 	jc "github.com/juju/testing/checkers"
 	"github.com/juju/version/v2"
@@ -30,7 +28,7 @@ func (s *upgradeValidationSuite) TestValidatorsForModelMigrationSourceJuju3(c *g
 	server := mocks.NewMockServer(ctrl)
 	serverFactory := mocks.NewMockServerFactory(ctrl)
 	s.PatchValue(&upgradevalidation.NewServerFactory,
-		func(httpClient *http.Client) lxd.ServerFactory {
+		func(_ lxd.NewHTTPClientFunc) lxd.ServerFactory {
 			return serverFactory
 		},
 	)

upgrades/upgradevalidation/upgrade_test.go

@@ -4,8 +4,6 @@
 package upgradevalidation_test
 
 import (
-	"net/http"
-
 	"github.com/golang/mock/gomock"
 	"github.com/juju/names/v4"
 	"github.com/juju/replicaset/v2"
@@ -42,7 +40,7 @@ func (s *upgradeValidationSuite) TestValidatorsForControllerUpgradeJuju3(c *gc.C
 	server := mocks.NewMockServer(ctrl)
 	serverFactory := mocks.NewMockServerFactory(ctrl)
 	s.PatchValue(&upgradevalidation.NewServerFactory,
-		func(httpClient *http.Client) lxd.ServerFactory {
+		func(_ lxd.NewHTTPClientFunc) lxd.ServerFactory {
 			return serverFactory
 		},
 	)
@@ -236,7 +234,7 @@ func (s *upgradeValidationSuite) TestValidatorsForModelUpgradeJuju3(c *gc.C) {
 	server := mocks.NewMockServer(ctrl)
 	serverFactory := mocks.NewMockServerFactory(ctrl)
 	s.PatchValue(&upgradevalidation.NewServerFactory,
-		func(httpClient *http.Client) lxd.ServerFactory {
+		func(_ lxd.NewHTTPClientFunc) lxd.ServerFactory {
 			return serverFactory
 		},
 	)

upgrades/upgradevalidation/validation.go

@@ -5,6 +5,7 @@ package upgradevalidation
 
 import (
 	"fmt"
+	"net/http"
 	"sort"
 	"strings"
 
@@ -298,10 +299,11 @@ func getCheckForLXDVersion(
 		if !lxdnames.IsDefaultCloud(cloudspec.Type) {
 			return nil, nil
 		}
-		httpClient := jujuhttp.NewClient(
-			jujuhttp.WithLogger(logger.ChildWithLabels("http", corelogger.HTTP)),
-		)
-		server, err := NewServerFactory(httpClient.Client()).RemoteServer(cloudspec)
+		server, err := NewServerFactory(lxd.NewHTTPClientFunc(func() *http.Client {
+			return jujuhttp.NewClient(
+				jujuhttp.WithLogger(logger.ChildWithLabels("http", corelogger.HTTP)),
+			).Client()
+		})).RemoteServer(cloudspec)
 		if err != nil {
 			return nil, errors.Trace(err)
 		}

upgrades/upgradevalidation/validation_test.go

@@ -5,7 +5,6 @@ package upgradevalidation_test
 
 import (
 	"fmt"
-	"net/http"
 
 	"github.com/golang/mock/gomock"
 	"github.com/juju/errors"
@@ -373,7 +372,7 @@ func (s *upgradeValidationSuite) assertGetCheckForLXDVersion(c *gc.C, cloudType
 	serverFactory := mocks.NewMockServerFactory(ctrl)
 
 	s.PatchValue(&upgradevalidation.NewServerFactory,
-		func(httpClient *http.Client) lxd.ServerFactory {
+		func(_ lxd.NewHTTPClientFunc) lxd.ServerFactory {
 			return serverFactory
 		},
 	)
@@ -406,7 +405,7 @@ func (s *upgradeValidationSuite) TestGetCheckForLXDVersionSkippedForNonLXDCloud(
 	serverFactory := mocks.NewMockServerFactory(ctrl)
 
 	s.PatchValue(&upgradevalidation.NewServerFactory,
-		func(httpClient *http.Client) lxd.ServerFactory {
+		func(_ lxd.NewHTTPClientFunc) lxd.ServerFactory {
 			return serverFactory
 		},
 	)
@@ -426,7 +425,7 @@ func (s *upgradeValidationSuite) TestGetCheckForLXDVersionFailed(c *gc.C) {
 	serverFactory := mocks.NewMockServerFactory(ctrl)
 
 	s.PatchValue(&upgradevalidation.NewServerFactory,
-		func(httpClient *http.Client) lxd.ServerFactory {
+		func(_ lxd.NewHTTPClientFunc) lxd.ServerFactory {
 			return serverFactory
 		},
 	)