From 85d3d31b4404304992f6f8f53bcf74d7c50d829b Mon Sep 17 00:00:00 2001 From: Thomas Miller Date: Mon, 3 Apr 2023 21:03:31 +1000 Subject: [PATCH] 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 --- .../client/modelupgrader/upgrader_test.go | 10 ++++------ provider/lxd/export_test.go | 4 ++++ provider/lxd/provider.go | 11 +++++++---- provider/lxd/server.go | 19 +++++++++++++------ upgrades/upgradevalidation/migrate_test.go | 4 +--- upgrades/upgradevalidation/upgrade_test.go | 6 ++---- upgrades/upgradevalidation/validation.go | 10 ++++++---- upgrades/upgradevalidation/validation_test.go | 7 +++---- 8 files changed, 40 insertions(+), 31 deletions(-) diff --git a/apiserver/facades/client/modelupgrader/upgrader_test.go b/apiserver/facades/client/modelupgrader/upgrader_test.go index 30b37a35cc0f..a629f8aa97d0 100644 --- a/apiserver/facades/client/modelupgrader/upgrader_test.go +++ b/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 }, ) diff --git a/provider/lxd/export_test.go b/provider/lxd/export_test.go index b55ce4a5a372..07833a69b14d 100644 --- a/provider/lxd/export_test.go +++ b/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{} + }), } } diff --git a/provider/lxd/provider.go b/provider/lxd/provider.go index e35c57e432bf..502947e936d8 100644 --- a/provider/lxd/provider.go +++ b/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{}, diff --git a/provider/lxd/server.go b/provider/lxd/server.go index e8477139ccf6..a1dbb9fd6d62 100644 --- a/provider/lxd/server.go +++ b/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,11 +142,14 @@ 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() @@ -150,8 +157,8 @@ func NewServerFactory(httpClient *http.Client) ServerFactory { 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) diff --git a/upgrades/upgradevalidation/migrate_test.go b/upgrades/upgradevalidation/migrate_test.go index 3240243032a1..2372ee919bd8 100644 --- a/upgrades/upgradevalidation/migrate_test.go +++ b/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 }, ) diff --git a/upgrades/upgradevalidation/upgrade_test.go b/upgrades/upgradevalidation/upgrade_test.go index bf051403f49e..513d3f24fd83 100644 --- a/upgrades/upgradevalidation/upgrade_test.go +++ b/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 }, ) diff --git a/upgrades/upgradevalidation/validation.go b/upgrades/upgradevalidation/validation.go index 834e6b58dc0a..153c0848ec72 100644 --- a/upgrades/upgradevalidation/validation.go +++ b/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) } diff --git a/upgrades/upgradevalidation/validation_test.go b/upgrades/upgradevalidation/validation_test.go index 028bc177bfcd..609e957286be 100644 --- a/upgrades/upgradevalidation/validation_test.go +++ b/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 }, )