Openstack: Provide client options

To prevent large optional function parameters, use client options.
sdg0919 · Jun 9, 2021 · a98291b · a98291b
1 parent 4ed26c8
commit a98291b
Show file tree

Hide file tree

Showing 2 changed files with 90 additions and 24 deletions.
diff --git a/provider/openstack/client.go b/provider/openstack/client.go
@@ -4,7 +4,10 @@
 package openstack
 
 import (
+	"net/http"
+
 	"github.com/go-goose/goose/v3/client"
+	goosehttp "github.com/go-goose/goose/v3/http"
 	"github.com/go-goose/goose/v3/identity"
 	gooselogging "github.com/go-goose/goose/v3/logging"
 	"github.com/go-goose/goose/v3/neutron"
@@ -16,6 +19,67 @@ import (
 	environscloudspec "github.com/juju/juju/environs/cloudspec"
 )
 
+// ClientOption to be passed into the transport construction to customize the
+// default transport.
+type ClientOption func(*clientOptions)
+
+type clientOptions struct {
+	caCertificates           []string
+	skipHostnameVerification bool
+	httpHeadersFunc          goosehttp.HeadersFunc
+	httpClient               *http.Client
+}
+
+// WithCACertificates contains Authority certificates to be used to validate
+// certificates of cloud infrastructure components.
+// The contents are Base64 encoded x.509 certs.
+func WithCACertificates(value ...string) ClientOption {
+	return func(opt *clientOptions) {
+		opt.caCertificates = value
+	}
+}
+
+// WithSkipHostnameVerification will skip hostname verification on the TLS/SSL
+// certificates.
+func WithSkipHostnameVerification(value bool) ClientOption {
+	return func(opt *clientOptions) {
+		opt.skipHostnameVerification = value
+	}
+}
+
+// WithHTTPHeadersFunc allows passing in a new HTTP headers func for the client
+// to execute for each request.
+func WithHTTPHeadersFunc(httpHeadersFunc goosehttp.HeadersFunc) ClientOption {
+	return func(clientOptions *clientOptions) {
+		clientOptions.httpHeadersFunc = httpHeadersFunc
+	}
+}
+
+// WithHTTPClient allows to define the http.Client to use.
+func WithHTTPClient(value *http.Client) ClientOption {
+	return func(opt *clientOptions) {
+		opt.httpClient = value
+	}
+}
+
+// Create a clientOptions instance with default values.
+func newOptions() *clientOptions {
+	// In this case, use a default http.Client.
+	// Ideally we should always use the NewHTTPTLSTransport,
+	// however test suites such as JujuConnSuite and some facade
+	// tests rely on settings to the http.DefaultTransport for
+	// tests to run with different protocol scheme such as "test"
+	// and some replace the RoundTripper to answer test scenarios.
+	//
+	// https://bugs.launchpad.net/juju/+bug/1888888
+	defaultCopy := *http.DefaultClient
+
+	return &clientOptions{
+		httpHeadersFunc: goosehttp.DefaultHeaders,
+		httpClient:      &defaultCopy,
+	}
+}
+
 // SSLHostnameConfig defines the options for host name verification
 type SSLHostnameConfig interface {
 	SSLHostnameVerification() bool
@@ -24,9 +88,7 @@ type SSLHostnameConfig interface {
 // ClientFunc is used to create a goose client.
 type ClientFunc = func(cred identity.Credentials,
 	authMode identity.AuthMode,
-	sslHostnameVerification bool,
-	certs []string,
-	options ...client.Option) (client.AuthenticatingClient, error)
+	options ...ClientOption) (client.AuthenticatingClient, error)
 
 // ClientFactory creates various goose (openstack) clients.
 // TODO (stickupkid): This should be moved into goose and the factory should
@@ -48,7 +110,7 @@ func NewClientFactory(spec environscloudspec.CloudSpec, sslHostnameConfig SSLHos
 	return &ClientFactory{
 		spec:              spec,
 		sslHostnameConfig: sslHostnameConfig,
-		clientFunc:        newClientByType,
+		clientFunc:        newClient,
 	}
 }
 
@@ -83,16 +145,14 @@ func (c *ClientFactory) Nova() (*nova.Client, error) {
 // Note: we override the http.Client headers with specific neutron client
 // headers.
 func (c *ClientFactory) Neutron() (*neutron.Client, error) {
-	httpOption := client.WithHTTPHeadersFunc(neutron.NeutronHeaders)
-
-	client, err := c.getClientState(httpOption)
+	client, err := c.getClientState(WithHTTPHeadersFunc(neutron.NeutronHeaders))
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
 	return neutron.New(client), nil
 }
 
-func (c *ClientFactory) getClientState(options ...client.Option) (client.AuthenticatingClient, error) {
+func (c *ClientFactory) getClientState(options ...ClientOption) (client.AuthenticatingClient, error) {
 	identityClientVersion, err := identityClientVersion(c.spec.Endpoint)
 	if err != nil {
 		return nil, errors.Annotate(err, "cannot create a client")
@@ -149,8 +209,13 @@ func (c *ClientFactory) getClientState(options ...client.Option) (client.Authent
 }
 
 // getClientByAuthMode creates a new client for the given AuthMode.
-func (c *ClientFactory) getClientByAuthMode(authMode identity.AuthMode, cred identity.Credentials, options ...client.Option) (client.AuthenticatingClient, error) {
-	newClient, err := c.clientFunc(cred, authMode, c.sslHostnameConfig.SSLHostnameVerification(), c.spec.CACertificates, options...)
+func (c *ClientFactory) getClientByAuthMode(authMode identity.AuthMode, cred identity.Credentials, options ...ClientOption) (client.AuthenticatingClient, error) {
+	newClient, err := c.clientFunc(cred, authMode,
+		append(options,
+			WithSkipHostnameVerification(!c.sslHostnameConfig.SSLHostnameVerification()),
+			WithCACertificates(c.spec.CACertificates...),
+		)...,
+	)
 	if err != nil {
 		return nil, errors.NewNotValid(err, "cannot create a new client")
 	}
@@ -162,26 +227,31 @@ func (c *ClientFactory) getClientByAuthMode(authMode identity.AuthMode, cred ide
 	return newClient, nil
 }
 
-// newClientByType returns an authenticating client to talk to the
+// newClient returns an authenticating client to talk to the
 // OpenStack cloud.  CACertificate and SSLHostnameVerification == false
 // config options are mutually exclusive here.
-func newClientByType(
+func newClient(
 	cred identity.Credentials,
 	authMode identity.AuthMode,
-	sslHostnameVerification bool,
-	certs []string,
-	options ...client.Option,
+	clientOptions ...ClientOption,
 ) (client.AuthenticatingClient, error) {
+	opts := newOptions()
+	for _, option := range clientOptions {
+		option(opts)
+	}
+
 	logger := loggo.GetLogger("goose")
 	gooseLogger := gooselogging.LoggoLogger{
 		Logger: logger,
 	}
 
 	httpClient := jujuhttp.NewClient(
-		jujuhttp.WithSkipHostnameVerification(!sslHostnameVerification),
-		jujuhttp.WithCACertificates(certs...),
+		jujuhttp.WithSkipHostnameVerification(opts.skipHostnameVerification),
+		jujuhttp.WithCACertificates(opts.caCertificates...),
 		jujuhttp.WithLogger(logger.Child("http")),
 	)
-	options = append(options, client.WithHTTPClient(httpClient.Client()))
-	return client.NewClient(&cred, authMode, gooseLogger, options...), nil
+	return client.NewClient(&cred, authMode, gooseLogger,
+		client.WithHTTPClient(httpClient.Client()),
+		client.WithHTTPHeadersFunc(opts.httpHeadersFunc),
+	), nil
 }
diff --git a/provider/openstack/client_test.go b/provider/openstack/client_test.go
@@ -88,11 +88,7 @@ func (s *clientSuite) setupMockFactory(ctrl *gomock.Controller, times int) *Clie
 }
 
 func makeClientFunc(mockClient *MockAuthenticatingClient) ClientFunc {
-	return func(cred identity.Credentials,
-		authMode identity.AuthMode,
-		sslHostnameVerification bool,
-		certs []string,
-		options ...client.Option) (client.AuthenticatingClient, error) {
+	return func(identity.Credentials, identity.AuthMode, ...ClientOption) (client.AuthenticatingClient, error) {
 		return mockClient, nil
 	}
 }