Repository: metal3-io/baremetal-operator
Pull request #914 (2021-06-11 20:09:32)

Eliminate global variables in ironic provisioner
Create a provisioner factory that holds global config, and initialise it on demand instead of in the ironic provisioner package's init() function.

metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **zaneb** after the PR has been reviewed.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["zaneb"]} -->
metal3-io-bot: @zaneb: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
golint | 03724a025679f7abaf5e23e08451dc11a095644d | [link](https://prow.apps.test.metal3.io/view/gs/metal3-prow/pr-logs/pull/metal3-io_baremetal-operator/914/golint/1403444317384609792) | `/test golint`

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index afb1065b..ae3e40e5 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -217,7 +217,7 @@ func (r *BareMetalHostReconciler) Reconcile(ctx context.Context, request ctrl.Re
 		bmcCredsSecret: bmcCredsSecret,
 	}
 
-	prov, err := r.ProvisionerFactory(provisioner.BuildHostData(*host, *bmcCreds), info.publishEvent)
+	prov, err := r.ProvisionerFactory.NewProvisioner(provisioner.BuildHostData(*host, *bmcCreds), info.publishEvent)
 	if err != nil {
 		return ctrl.Result{}, errors.Wrap(err, "failed to create provisioner")
 	}
diff --git a/main.go b/main.go
index 5fea6ea6..c93a9add 100644
--- a/main.go
+++ b/main.go
@@ -126,14 +126,12 @@ func main() {
 	var provisionerFactory provisioner.Factory
 	if runInTestMode {
 		ctrl.Log.Info("using test provisioner")
-		fix := fixture.Fixture{}
-		provisionerFactory = fix.New
+		provisionerFactory = &fixture.Fixture{}
 	} else if runInDemoMode {
 		ctrl.Log.Info("using demo provisioner")
-		provisionerFactory = demo.New
+		provisionerFactory = &demo.Demo{}
 	} else {
-		ironic.LogStartup()
-		provisionerFactory = ironic.New
+		provisionerFactory = ironic.NewProvisionerFactory()
 	}
 
 	if err = (&metal3iocontroller.BareMetalHostReconciler{
diff --git a/pkg/provisioner/demo/demo.go b/pkg/provisioner/demo/demo.go
index 9ac61e81..14c04de6 100644
--- a/pkg/provisioner/demo/demo.go
+++ b/pkg/provisioner/demo/demo.go
@@ -65,8 +65,10 @@ type demoProvisioner struct {
 	publisher provisioner.EventPublisher
 }
 
+type Demo struct{}
+
 // New returns a new demo Provisioner
-func New(hostData provisioner.HostData, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) {
+func (d Demo) NewProvisioner(hostData provisioner.HostData, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) {
 	p := &demoProvisioner{
 		objectMeta: hostData.ObjectMeta,
 		provID:     hostData.ProvisionerID,
diff --git a/pkg/provisioner/fixture/fixture.go b/pkg/provisioner/fixture/fixture.go
index 1615fe3b..30cc6a40 100644
--- a/pkg/provisioner/fixture/fixture.go
+++ b/pkg/provisioner/fixture/fixture.go
@@ -74,7 +74,7 @@ type Fixture struct {
 }
 
 // New returns a new Fixture Provisioner
-func (f *Fixture) New(hostData provisioner.HostData, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) {
+func (f *Fixture) NewProvisioner(hostData provisioner.HostData, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) {
 	p := &fixtureProvisioner{
 		provID:    hostData.ProvisionerID,
 		bmcCreds:  hostData.BMCCredentials,
diff --git a/pkg/provisioner/ironic/clients/client.go b/pkg/provisioner/ironic/clients/client.go
index 6115afaa..bc14f28b 100644
--- a/pkg/provisioner/ironic/clients/client.go
+++ b/pkg/provisioner/ironic/clients/client.go
@@ -92,6 +92,11 @@ func IronicClient(ironicEndpoint string, auth AuthConfig, tls TLSConfig) (client
 	if err != nil {
 		return
 	}
+
+	// Ensure we have a microversion high enough to get the features
+	// we need.
+	client.Microversion = "1.56"
+
 	return updateHTTPClient(client, tls)
 }
 
diff --git a/pkg/provisioner/ironic/dependencies.go b/pkg/provisioner/ironic/dependencies.go
index 84444be1..92549258 100644
--- a/pkg/provisioner/ironic/dependencies.go
+++ b/pkg/provisioner/ironic/dependencies.go
@@ -47,7 +47,7 @@ func (i *ironicDependenciesChecker) checkEndpoint(client *gophercloud.ServiceCli
 
 	_, err := client.Get(endpoint, nil, nil)
 	if err != nil {
-		log.Info("error caught while checking endpoint", "endpoint", client.Endpoint, "error", err)
+		i.log.Info("error caught while checking endpoint", "endpoint", client.Endpoint, "error", err)
 	}
 
 	return err == nil
diff --git a/pkg/provisioner/ironic/factory.go b/pkg/provisioner/ironic/factory.go
new file mode 100644
index 00000000..473d342f
--- /dev/null
+++ b/pkg/provisioner/ironic/factory.go
@@ -0,0 +1,187 @@
+package ironic
+
+import (
+	"errors"
+	"fmt"
+	"os"
+	"strconv"
+	"strings"
+
+	"github.com/go-logr/logr"
+	"github.com/gophercloud/gophercloud"
+	logz "sigs.k8s.io/controller-runtime/pkg/log/zap"
+
+	"github.com/metal3-io/baremetal-operator/pkg/provisioner"
+	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/clients"
+)
+
+type ironicProvisionerFactory struct {
+	log    logr.Logger
+	config ironicConfig
+
+	// Keep pointers to ironic and inspector clients configured with
+	// the global auth settings to reuse the connection between
+	// reconcilers.
+	clientIronic    *gophercloud.ServiceClient
+	clientInspector *gophercloud.ServiceClient
+}
+
+func NewProvisionerFactory() provisioner.Factory {
+	factory := ironicProvisionerFactory{}
+
+	factory.log = logz.New().WithName("provisioner").WithName("ironic")
+
+	err := factory.init()
+	if err != nil {
+		factory.log.Error(err, "Cannot start ironic provisioner")
+		os.Exit(1)
+	}
+	return factory
+}
+
+func (f *ironicProvisionerFactory) init() error {
+	ironicAuth, inspectorAuth, err := clients.LoadAuth()
+	if err != nil {
+		return err
+	}
+
+	f.config, err = loadConfigFromEnv()
+	if err != nil {
+		return err
+	}
+
+	ironicEndpoint, inspectorEndpoint, err := loadEndpointsFromEnv()
+	if err != nil {
+		return err
+	}
+
+	tlsConf := loadTLSConfigFromEnv()
+
+	f.log.Info("ironic settings",
+		"endpoint", ironicEndpoint,
+		"ironicAuthType", ironicAuth.Type,
+		"inspectorEndpoint", inspectorEndpoint,
+		"inspectorAuthType", inspectorAuth.Type,
+		"deployKernelURL", f.config.deployKernelURL,
+		"deployRamdiskURL", f.config.deployRamdiskURL,
+		"deployISOURL", f.config.deployISOURL,
+		"CACertFile", tlsConf.TrustedCAFile,
+		"ClientCertFile", tlsConf.ClientCertificateFile,
+		"ClientPrivKeyFile", tlsConf.ClientPrivateKeyFile,
+		"TLSInsecure", tlsConf.InsecureSkipVerify,
+		"SkipClientSANVerify", tlsConf.SkipClientSANVerify,
+	)
+
+	f.clientIronic, err = clients.IronicClient(
+		ironicEndpoint, ironicAuth, tlsConf)
+	if err != nil {
+		return err
+	}
+
+	f.clientInspector, err = clients.InspectorClient(
+		inspectorEndpoint, inspectorAuth, tlsConf)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func (f ironicProvisionerFactory) ironicProvisioner(hostData provisioner.HostData, publisher provisioner.EventPublisher) (*ironicProvisioner, error) {
+	provisionerLogger := f.log.WithValues("host", ironicNodeName(hostData.ObjectMeta))
+
+	p := &ironicProvisioner{
+		config:                  f.config,
+		objectMeta:              hostData.ObjectMeta,
+		nodeID:                  hostData.ProvisionerID,
+		bmcCreds:                hostData.BMCCredentials,
+		bmcAddress:              hostData.BMCAddress,
+		disableCertVerification: hostData.DisableCertificateVerification,
+		bootMACAddress:          hostData.BootMACAddress,
+		client:                  f.clientIronic,
+		inspector:               f.clientInspector,
+		log:                     provisionerLogger,
+		debugLog:                provisionerLogger.V(1),
+		publisher:               publisher,
+	}
+
+	return p, nil
+}
+
+// NewProvisioner returns a new Ironic Provisioner using the global
+// configuration for finding the Ironic services.
+func (f ironicProvisionerFactory) NewProvisioner(hostData provisioner.HostData, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) {
+	return f.ironicProvisioner(hostData, publisher)
+}
+
+func loadConfigFromEnv() (ironicConfig, error) {
+	c := ironicConfig{}
+
+	c.deployKernelURL = os.Getenv("DEPLOY_KERNEL_URL")
+	c.deployRamdiskURL = os.Getenv("DEPLOY_RAMDISK_URL")
+	c.deployISOURL = os.Getenv("DEPLOY_ISO_URL")
+	if c.deployISOURL == "" &&
+		(c.deployKernelURL == "" || c.deployRamdiskURL == "") {
+		return c, errors.New("Either DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL or DEPLOY_ISO_URL must be set")
+	}
+	if (c.deployKernelURL == "" && c.deployRamdiskURL != "") ||
+		(c.deployKernelURL != "" && c.deployRamdiskURL == "") {
+		return c, errors.New("DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL can only be set together")
+	}
+
+	c.maxBusyHosts = 20
+	if maxHostsStr := os.Getenv("PROVISIONING_LIMIT"); maxHostsStr != "" {
+		value, err := strconv.Atoi(maxHostsStr)
+		if err != nil {
+			return c, fmt.Errorf("Invalid value set for variable PROVISIONING_LIMIT=%s", maxHostsStr)
+		}
+		c.maxBusyHosts = value
+	}
+
+	return c, nil
+}
+
+func loadEndpointsFromEnv() (ironicEndpoint, inspectorEndpoint string, err error) {
+	ironicEndpoint = os.Getenv("IRONIC_ENDPOINT")
+	if ironicEndpoint == "" {
+		err = errors.New("No IRONIC_ENDPOINT variable set")
+	}
+	inspectorEndpoint = os.Getenv("IRONIC_INSPECTOR_ENDPOINT")
+	if inspectorEndpoint == "" {
+		err = errors.New("No IRONIC_INSPECTOR_ENDPOINT variable set")
+	}
+
+	return
+}
+
+func loadTLSConfigFromEnv() clients.TLSConfig {
+	ironicCACertFile := os.Getenv("IRONIC_CACERT_FILE")
+	if ironicCACertFile == "" {
+		ironicCACertFile = "/opt/metal3/certs/ca/tls.crt"
+	}
+	ironicClientCertFile := os.Getenv("IRONIC_CLIENT_CERT_FILE")
+	if ironicClientCertFile == "" {
+		ironicClientCertFile = "/opt/metal3/certs/client/tls.crt"
+	}
+	ironicClientPrivKeyFile := os.Getenv("IRONIC_CLIENT_PRIVATE_KEY_FILE")
+	if ironicClientPrivKeyFile == "" {
+		ironicClientPrivKeyFile = "/opt/metal3/certs/client/tls.key"
+	}
+	insecure := false
+	ironicInsecureStr := os.Getenv("IRONIC_INSECURE")
+	if strings.ToLower(ironicInsecureStr) == "true" {
+		insecure = true
+	}
+	skipClientSANVerify := false
+	ironicSkipClientSANVerifyStr := os.Getenv("IRONIC_SKIP_CLIENT_SAN_VERIFY")
+	if strings.ToLower(ironicSkipClientSANVerifyStr) == "true" {
+		skipClientSANVerify = true
+	}
+	return clients.TLSConfig{
+		TrustedCAFile:         ironicCACertFile,
+		ClientCertificateFile: ironicClientCertFile,
+		ClientPrivateKeyFile:  ironicClientPrivKeyFile,
+		InsecureSkipVerify:    insecure,
+		SkipClientSANVerify:   skipClientSANVerify,
+	}
+}
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index eb18ab57..221b4f71 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -2,8 +2,6 @@ package ironic
 
 import (
 	"fmt"
-	"os"
-	"strconv"
 	"strings"
 	"time"
 
@@ -15,43 +13,21 @@ import (
 	"github.com/gophercloud/gophercloud/openstack/baremetal/v1/ports"
 	"github.com/gophercloud/gophercloud/openstack/baremetalintrospection/v1/introspection"
 	"github.com/pkg/errors"
-	logz "sigs.k8s.io/controller-runtime/pkg/log/zap"
 	"sigs.k8s.io/yaml"
 
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 	"github.com/metal3-io/baremetal-operator/pkg/bmc"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner"
-	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/clients"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/devicehints"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/hardwaredetails"
 )
 
 var (
-	log                       = logz.New().WithName("provisioner").WithName("ironic")
 	deprovisionRequeueDelay   = time.Second * 10
 	provisionRequeueDelay     = time.Second * 10
 	powerRequeueDelay         = time.Second * 10
 	introspectionRequeueDelay = time.Second * 15
 	softPowerOffTimeout       = time.Second * 180
-	deployKernelURL           string
-	deployRamdiskURL          string
-	deployISOURL              string
-	ironicEndpoint            string
-	inspectorEndpoint         string
-	ironicTrustedCAFile       string
-	ironicClientCertFile      string
-	ironicClientPrivKeyFile   string
-	ironicInsecure            bool
-	ironicSkipClientSANVerify bool
-	ironicAuth                clients.AuthConfig
-	inspectorAuth             clients.AuthConfig
-	maxBusyHosts              int = 20
-
-	// Keep pointers to ironic and inspector clients configured with
-	// the global auth settings to reuse the connection between
-	// reconcilers.
-	clientIronicSingleton    *gophercloud.ServiceClient
-	clientInspectorSingleton *gophercloud.ServiceClient
 )
 
 const (
@@ -83,77 +59,18 @@ func NewMacAddressConflictError(address, node string) error {
 	return macAddressConflictError{Address: address, ExistingNode: node}
 }
 
-func loadConfigFromEnv() error {
-	deployKernelURL = os.Getenv("DEPLOY_KERNEL_URL")
-	deployRamdiskURL = os.Getenv("DEPLOY_RAMDISK_URL")
-	deployISOURL = os.Getenv("DEPLOY_ISO_URL")
-	if deployISOURL == "" && (deployKernelURL == "" || deployRamdiskURL == "") {
-		return errors.New("Either DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL or DEPLOY_ISO_URL must be set")
-	}
-	if (deployKernelURL == "" && deployRamdiskURL != "") || (deployKernelURL != "" && deployRamdiskURL == "") {
-		return errors.New("DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL can only be set together")
-	}
-	ironicEndpoint = os.Getenv("IRONIC_ENDPOINT")
-	if ironicEndpoint == "" {
-		return errors.New("No IRONIC_ENDPOINT variable set")
-	}
-	inspectorEndpoint = os.Getenv("IRONIC_INSPECTOR_ENDPOINT")
-	if inspectorEndpoint == "" {
-		return errors.New("No IRONIC_INSPECTOR_ENDPOINT variable set")
-	}
-	ironicTrustedCAFile = os.Getenv("IRONIC_CACERT_FILE")
-	if ironicTrustedCAFile == "" {
-		ironicTrustedCAFile = "/opt/metal3/certs/ca/tls.crt"
-	}
-	ironicClientCertFile = os.Getenv("IRONIC_CLIENT_CERT_FILE")
-	if ironicClientCertFile == "" {
-		ironicClientCertFile = "/opt/metal3/certs/client/tls.crt"
-	}
-	ironicClientPrivKeyFile = os.Getenv("IRONIC_CLIENT_PRIVATE_KEY_FILE")
-	if ironicClientPrivKeyFile == "" {
-		ironicClientPrivKeyFile = "/opt/metal3/certs/client/tls.key"
-	}
-	ironicInsecureStr := os.Getenv("IRONIC_INSECURE")
-	if strings.ToLower(ironicInsecureStr) == "true" {
-		ironicInsecure = true
-	}
-	ironicSkipClientSANVerifyStr := os.Getenv("IRONIC_SKIP_CLIENT_SAN_VERIFY")
-	if strings.ToLower(ironicSkipClientSANVerifyStr) == "true" {
-		ironicSkipClientSANVerify = true
-	}
-
-	if maxHostsStr := os.Getenv("PROVISIONING_LIMIT"); maxHostsStr != "" {
-		value, err := strconv.Atoi(maxHostsStr)
-		if err != nil {
-			return errors.Errorf("Invalid value set for variable PROVISIONING_LIMIT=%s", maxHostsStr)
-		}
-		maxBusyHosts = value
-	}
-
-	return nil
-}
-
-func init() {
-	// NOTE(dhellmann): Use Fprintf() to report errors instead of
-	// logging, because logging is not configured yet in init().
-
-	var authErr error
-	ironicAuth, inspectorAuth, authErr = clients.LoadAuth()
-	if authErr != nil {
-		fmt.Fprintf(os.Stderr, "Cannot start: %s\n", authErr)
-		os.Exit(1)
-	}
-
-	err := loadConfigFromEnv()
-	if err != nil {
-		fmt.Fprintf(os.Stderr, "Cannot start: %s\n", err)
-		os.Exit(1)
-	}
+type ironicConfig struct {
+	deployKernelURL  string
+	deployRamdiskURL string
+	deployISOURL     string
+	maxBusyHosts     int
 }
 
 // Provisioner implements the provisioning.Provisioner interface
 // and uses Ironic to manage the host.
 type ironicProvisioner struct {
+	// the global ironic settings
+	config ironicConfig
 	// the object metadata of the BareMetalHost resource
 	objectMeta metav1.ObjectMeta
 	// the UUID of the node in Ironic
@@ -178,98 +95,6 @@ type ironicProvisioner struct {
 	publisher provisioner.EventPublisher
 }
 
-// LogStartup produces useful logging information that we only want to
-// emit once on startup but that is interal to this package.
-func LogStartup() {
-	log.Info("ironic settings",
-		"endpoint", ironicEndpoint,
-		"ironicAuthType", ironicAuth.Type,
-		"inspectorEndpoint", inspectorEndpoint,
-		"inspectorAuthType", inspectorAuth.Type,
-		"deployKernelURL", deployKernelURL,
-		"deployRamdiskURL", deployRamdiskURL,
-		"deployISOURL", deployISOURL,
-	)
-}
-
-// A private function to construct an ironicProvisioner (rather than a
-// Provisioner interface) in a consistent way for tests.
-func newProvisionerWithSettings(host metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher, ironicURL string, ironicAuthSettings clients.AuthConfig, inspectorURL string, inspectorAuthSettings clients.AuthConfig) (*ironicProvisioner, error) {
-	hostData := provisioner.BuildHostData(host, bmcCreds)
-
-	tlsConf := clients.TLSConfig{
-		TrustedCAFile:         ironicTrustedCAFile,
-		ClientCertificateFile: ironicClientCertFile,
-		ClientPrivateKeyFile:  ironicClientPrivKeyFile,
-		InsecureSkipVerify:    ironicInsecure,
-		SkipClientSANVerify:   ironicSkipClientSANVerify,
-	}
-	clientIronic, err := clients.IronicClient(ironicURL, ironicAuthSettings, tlsConf)
-	if err != nil {
-		return nil, err
-	}
-
-	clientInspector, err := clients.InspectorClient(inspectorURL, inspectorAuthSettings, tlsConf)
-	if err != nil {
-		return nil, err
-	}
-
-	return newProvisionerWithIronicClients(hostData, publisher,
-		clientIronic, clientInspector)
-}
-
-func newProvisionerWithIronicClients(hostData provisioner.HostData, publisher provisioner.EventPublisher, clientIronic *gophercloud.ServiceClient, clientInspector *gophercloud.ServiceClient) (*ironicProvisioner, error) {
-	// Ensure we have a microversion high enough to get the features
-	// we need.
-	clientIronic.Microversion = "1.56"
-
-	provisionerLogger := log.WithValues("host", ironicNodeName(hostData.ObjectMeta))
-
-	p := &ironicProvisioner{
-		objectMeta:              hostData.ObjectMeta,
-		nodeID:                  hostData.ProvisionerID,
-		bmcCreds:                hostData.BMCCredentials,
-		bmcAddress:              hostData.BMCAddress,
-		disableCertVerification: hostData.DisableCertificateVerification,
-		bootMACAddress:          hostData.BootMACAddress,
-		client:                  clientIronic,
-		inspector:               clientInspector,
-		log:                     provisionerLogger,
-		debugLog:                provisionerLogger.V(1),
-		publisher:               publisher,
-	}
-
-	return p, nil
-}
-
-// New returns a new Ironic Provisioner using the global configuration
-// for finding the Ironic services.
-func New(hostData provisioner.HostData, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) {
-	var err error
-	if clientIronicSingleton == nil || clientInspectorSingleton == nil {
-		tlsConf := clients.TLSConfig{
-			TrustedCAFile:         ironicTrustedCAFile,
-			ClientCertificateFile: ironicClientCertFile,
-			ClientPrivateKeyFile:  ironicClientPrivKeyFile,
-			InsecureSkipVerify:    ironicInsecure,
-			SkipClientSANVerify:   ironicSkipClientSANVerify,
-		}
-		clientIronicSingleton, err = clients.IronicClient(
-			ironicEndpoint, ironicAuth, tlsConf)
-		if err != nil {
-			return nil, err
-		}
-
-		clientInspectorSingleton, err = clients.InspectorClient(
-			inspectorEndpoint, inspectorAuth, tlsConf)
-		if err != nil {
-			return nil, err
-		}
-	}
-	return newProvisionerWithIronicClients(hostData, publisher,
-		clientIronicSingleton, clientInspectorSingleton)
-}
-
 func (p *ironicProvisioner) bmcAccess() (bmc.AccessDetails, error) {
 	bmcAccess, err := bmc.NewAccessDetails(p.bmcAddress, p.disableCertVerification)
 	if err != nil {
@@ -518,12 +343,12 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
 	driverInfo := bmcAccess.DriverInfo(p.bmcCreds)
 	// FIXME(dhellmann): We need to get our IP on the
 	// provisioning network from somewhere.
-	if deployKernelURL != "" && deployRamdiskURL != "" {
-		driverInfo["deploy_kernel"] = deployKernelURL
-		driverInfo["deploy_ramdisk"] = deployRamdiskURL
+	if p.config.deployKernelURL != "" && p.config.deployRamdiskURL != "" {
+		driverInfo["deploy_kernel"] = p.config.deployKernelURL
+		driverInfo["deploy_ramdisk"] = p.config.deployRamdiskURL
 	}
-	if deployISOURL != "" {
-		driverInfo["deploy_iso"] = deployISOURL
+	if p.config.deployISOURL != "" {
+		driverInfo["deploy_iso"] = p.config.deployISOURL
 	}
 
 	// If we have not found a node yet, we need to create one
@@ -1749,7 +1574,7 @@ func (p *ironicProvisioner) HasCapacity() (result bool, err error) {
 		return true, nil
 	}
 
-	return len(hosts) < maxBusyHosts, nil
+	return len(hosts) < p.config.maxBusyHosts, nil
 }
 
 func (p *ironicProvisioner) loadBusyHosts() (hosts map[string]struct{}, err error) {
diff --git a/pkg/provisioner/provisioner.go b/pkg/provisioner/provisioner.go
index 4765cada..1ec4889a 100644
--- a/pkg/provisioner/provisioner.go
+++ b/pkg/provisioner/provisioner.go
@@ -40,7 +40,9 @@ func BuildHostData(host metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials)
 }
 
 // Factory is the interface for creating new Provisioner objects.
-type Factory func(hostData HostData, publish EventPublisher) (Provisioner, error)
+type Factory interface {
+	NewProvisioner(hostData HostData, publish EventPublisher) (Provisioner, error)
+}
 
 // HostConfigData retrieves host configuration data
 type HostConfigData interface {


Test patch:
diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go
index 26b62d18..1f9ad10c 100644
--- a/controllers/metal3.io/baremetalhost_controller_test.go
+++ b/controllers/metal3.io/baremetalhost_controller_test.go
@@ -98,7 +98,7 @@ func newTestReconcilerWithFixture(fix *fixture.Fixture, initObjs ...runtime.Obje
 
 	return &BareMetalHostReconciler{
 		Client:             c,
-		ProvisionerFactory: fix.New,
+		ProvisionerFactory: fix,
 		Log:                ctrl.Log.WithName("controllers").WithName("BareMetalHost"),
 	}
 }
diff --git a/controllers/metal3.io/demo_test.go b/controllers/metal3.io/demo_test.go
index af5c3735..3b5ec0b3 100644
--- a/controllers/metal3.io/demo_test.go
+++ b/controllers/metal3.io/demo_test.go
@@ -24,7 +24,7 @@ func newDemoReconciler(initObjs ...runtime.Object) *BareMetalHostReconciler {
 
 	return &BareMetalHostReconciler{
 		Client:             c,
-		ProvisionerFactory: demo.New,
+		ProvisionerFactory: &demo.Demo{},
 		Log:                ctrl.Log.WithName("controller").WithName("BareMetalHost"),
 	}
 }
diff --git a/controllers/metal3.io/host_state_machine_test.go b/controllers/metal3.io/host_state_machine_test.go
index 670f1425..fe31c980 100644
--- a/controllers/metal3.io/host_state_machine_test.go
+++ b/controllers/metal3.io/host_state_machine_test.go
@@ -21,7 +21,7 @@ import (
 
 func testStateMachine(host *metal3v1alpha1.BareMetalHost) *hostStateMachine {
 	r := newTestReconciler()
-	p, _ := r.ProvisionerFactory(provisioner.BuildHostData(*host, bmc.Credentials{}),
+	p, _ := r.ProvisionerFactory.NewProvisioner(provisioner.BuildHostData(*host, bmc.Credentials{}),
 		func(reason, message string) {})
 	return newHostStateMachine(host, r, p, true)
 }
diff --git a/pkg/provisioner/ironic/factory_test.go b/pkg/provisioner/ironic/factory_test.go
new file mode 100644
index 00000000..06d6b411
--- /dev/null
+++ b/pkg/provisioner/ironic/factory_test.go
@@ -0,0 +1,182 @@
+package ironic
+
+import (
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+type EnvFixture struct {
+	ironicEndpoint    string
+	inspectorEndpoint string
+	kernelURL         string
+	ramdiskURL        string
+	isoURL            string
+
+	origEnv map[string]string
+}
+
+func (f *EnvFixture) TearDown() {
+	for e, v := range f.origEnv {
+		if v == "" {
+			os.Unsetenv(e)
+		} else {
+			os.Setenv(e, v)
+		}
+	}
+}
+
+func (f *EnvFixture) replace(env, value string) {
+	f.origEnv[env] = os.Getenv(env)
+	if value == "" {
+		os.Unsetenv(env)
+	} else {
+		os.Setenv(env, value)
+	}
+}
+
+func (f *EnvFixture) SetUp() {
+	f.origEnv = map[string]string{}
+	f.replace("IRONIC_ENDPOINT", f.ironicEndpoint)
+	f.replace("IRONIC_INSPECTOR_ENDPOINT", f.inspectorEndpoint)
+	f.replace("DEPLOY_KERNEL_URL", f.kernelURL)
+	f.replace("DEPLOY_RAMDISK_URL", f.ramdiskURL)
+	f.replace("DEPLOY_ISO_URL", f.isoURL)
+}
+
+func (f EnvFixture) VerifyConfig(t *testing.T, c ironicConfig) {
+	assert.Equal(t, f.kernelURL, c.deployKernelURL)
+	assert.Equal(t, f.ramdiskURL, c.deployRamdiskURL)
+	assert.Equal(t, f.isoURL, c.deployISOURL)
+}
+
+func (f EnvFixture) VerifyEndpoints(t *testing.T, ironic, inspector string) {
+	assert.Equal(t, f.ironicEndpoint, ironic)
+	assert.Equal(t, f.inspectorEndpoint, inspector)
+}
+
+func TestLoadConfigFromEnv(t *testing.T) {
+	cases := []struct {
+		name          string
+		env           EnvFixture
+		expectedError string
+	}{
+		{
+			name: "kernel and ramdisk",
+			env: EnvFixture{
+				kernelURL:  "http://kernel",
+				ramdiskURL: "http://ramdisk",
+			},
+		},
+		{
+			name: "ISO only",
+			env: EnvFixture{
+				isoURL: "http://iso",
+			},
+		},
+		{
+			name: "ISO and kernel/ramdisk",
+			env: EnvFixture{
+				isoURL:     "http://iso",
+				kernelURL:  "http://kernel",
+				ramdiskURL: "http://ramdisk",
+			},
+		},
+		{
+			name:          "no deploy info",
+			env:           EnvFixture{},
+			expectedError: "Either DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL or DEPLOY_ISO_URL must be set",
+		},
+		{
+			name: "only kernel",
+			env: EnvFixture{
+				kernelURL: "http://kernel",
+			},
+			expectedError: "Either DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL or DEPLOY_ISO_URL must be set",
+		},
+		{
+			name: "only ramdisk",
+			env: EnvFixture{
+				ramdiskURL: "http://ramdisk",
+			},
+			expectedError: "Either DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL or DEPLOY_ISO_URL must be set",
+		},
+		{
+			name: "ISO and kernel",
+			env: EnvFixture{
+				kernelURL: "http://kernel",
+				isoURL:    "http://iso",
+			},
+			expectedError: "DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL can only be set together",
+		},
+		{
+			name: "ISO and ramdisk",
+			env: EnvFixture{
+				ramdiskURL: "http://ramdisk",
+				isoURL:     "http://iso",
+			},
+			expectedError: "DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL can only be set together",
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			defer tc.env.TearDown()
+			tc.env.SetUp()
+			config, err := loadConfigFromEnv()
+			if tc.expectedError != "" {
+				assert.Regexp(t, tc.expectedError, err, "error message")
+			} else {
+				assert.Nil(t, err)
+				tc.env.VerifyConfig(t, config)
+			}
+		})
+	}
+}
+
+func TestLoadEndpointsFromEnv(t *testing.T) {
+	cases := []struct {
+		name        string
+		env         EnvFixture
+		expectError bool
+	}{
+		{
+			name: "both",
+			env: EnvFixture{
+				ironicEndpoint:    "http://ironic.test",
+				inspectorEndpoint: "http://ironic-inspector.test",
+			},
+		}, {
+			name: "ironic-only",
+			env: EnvFixture{
+				ironicEndpoint: "http://ironic.test",
+			},
+			expectError: true,
+		}, {
+			name: "inspector-only",
+			env: EnvFixture{
+				inspectorEndpoint: "http://ironic-inspector.test",
+			},
+			expectError: true,
+		}, {
+			name:        "neither",
+			env:         EnvFixture{},
+			expectError: true,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			defer tc.env.TearDown()
+			tc.env.SetUp()
+			i, ii, err := loadEndpointsFromEnv()
+			if tc.expectError {
+				assert.NotNil(t, err)
+			} else {
+				assert.Nil(t, err)
+				tc.env.VerifyEndpoints(t, i, ii)
+			}
+		})
+	}
+}
diff --git a/pkg/provisioner/ironic/init_test.go b/pkg/provisioner/ironic/init_test.go
deleted file mode 100644
index e15fdbe3..00000000
--- a/pkg/provisioner/ironic/init_test.go
+++ /dev/null
@@ -1,152 +0,0 @@
-package ironic
-
-import (
-	"os"
-	"strings"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-)
-
-type EnvFixture struct {
-	ironicEndpoint    string
-	inspectorEndpoint string
-	kernelURL         string
-	ramdiskURL        string
-	isoURL            string
-}
-
-func tearDown() {
-	for _, e := range os.Environ() {
-		if !strings.HasPrefix(e, "IRONIC_") && !strings.HasPrefix(e, "DEPLOY_") {
-			continue
-		}
-		parts := strings.SplitN(e, "=", 2)
-		os.Unsetenv(parts[0])
-	}
-}
-
-func (f EnvFixture) SetUp() {
-	tearDown()
-
-	if f.ironicEndpoint != "" {
-		os.Setenv("IRONIC_ENDPOINT", f.ironicEndpoint)
-	}
-	if f.inspectorEndpoint != "" {
-		os.Setenv("IRONIC_INSPECTOR_ENDPOINT", f.inspectorEndpoint)
-	}
-	if f.kernelURL != "" {
-		os.Setenv("DEPLOY_KERNEL_URL", f.kernelURL)
-	}
-	if f.ramdiskURL != "" {
-		os.Setenv("DEPLOY_RAMDISK_URL", f.ramdiskURL)
-	}
-	if f.isoURL != "" {
-		os.Setenv("DEPLOY_ISO_URL", f.isoURL)
-	}
-}
-
-func (f EnvFixture) Verify(t *testing.T) {
-	assert.Equal(t, f.ironicEndpoint, ironicEndpoint)
-	assert.Equal(t, f.inspectorEndpoint, inspectorEndpoint)
-	assert.Equal(t, f.kernelURL, deployKernelURL)
-	assert.Equal(t, f.ramdiskURL, deployRamdiskURL)
-	assert.Equal(t, f.isoURL, deployISOURL)
-}
-
-func TestLoadFromEnv(t *testing.T) {
-	defer tearDown()
-
-	cases := []struct {
-		name          string
-		env           EnvFixture
-		expectedError string
-	}{
-		{
-			name: "kernel and ramdisk",
-			env: EnvFixture{
-				ironicEndpoint:    "http://ironic",
-				inspectorEndpoint: "http://inspector",
-				kernelURL:         "http://kernel",
-				ramdiskURL:        "http://ramdisk",
-			},
-		},
-		{
-			name: "ISO only",
-			env: EnvFixture{
-				ironicEndpoint:    "http://ironic",
-				inspectorEndpoint: "http://inspector",
-				isoURL:            "http://iso",
-			},
-		},
-		{
-			name: "ISO and kernel/ramdisk",
-			env: EnvFixture{
-				ironicEndpoint:    "http://ironic",
-				inspectorEndpoint: "http://inspector",
-				isoURL:            "http://iso",
-				kernelURL:         "http://kernel",
-				ramdiskURL:        "http://ramdisk",
-			},
-		},
-		{
-			name: "no deploy info",
-			env: EnvFixture{
-				ironicEndpoint:    "http://ironic",
-				inspectorEndpoint: "http://inspector",
-			},
-			expectedError: "Either DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL or DEPLOY_ISO_URL must be set",
-		},
-		{
-			name: "only kernel",
-			env: EnvFixture{
-				ironicEndpoint:    "http://ironic",
-				inspectorEndpoint: "http://inspector",
-				kernelURL:         "http://kernel",
-			},
-			expectedError: "Either DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL or DEPLOY_ISO_URL must be set",
-		},
-		{
-			name: "only ramdisk",
-			env: EnvFixture{
-				ironicEndpoint:    "http://ironic",
-				inspectorEndpoint: "http://inspector",
-				ramdiskURL:        "http://ramdisk",
-			},
-			expectedError: "Either DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL or DEPLOY_ISO_URL must be set",
-		},
-		{
-			name: "ISO and kernel",
-			env: EnvFixture{
-				ironicEndpoint:    "http://ironic",
-				inspectorEndpoint: "http://inspector",
-				kernelURL:         "http://kernel",
-				isoURL:            "http://iso",
-			},
-			expectedError: "DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL can only be set together",
-		},
-		{
-			name: "ISO and ramdisk",
-			env: EnvFixture{
-				ironicEndpoint:    "http://ironic",
-				inspectorEndpoint: "http://inspector",
-				ramdiskURL:        "http://ramdisk",
-				isoURL:            "http://iso",
-			},
-			expectedError: "DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL can only be set together",
-		},
-	}
-
-	for _, tc := range cases {
-		t.Run(tc.name, func(t *testing.T) {
-			tc.env.SetUp()
-			err := loadConfigFromEnv()
-			if tc.expectedError != "" {
-				assert.Regexp(t, tc.expectedError, err, "error message")
-			} else {
-				assert.Nil(t, err)
-				tc.env.Verify(t)
-			}
-		})
-	}
-}
diff --git a/pkg/provisioner/ironic/ironic_test.go b/pkg/provisioner/ironic/ironic_test.go
index 49e97c5b..80ed4be8 100644
--- a/pkg/provisioner/ironic/ironic_test.go
+++ b/pkg/provisioner/ironic/ironic_test.go
@@ -11,6 +11,7 @@ import (
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 	"github.com/metal3-io/baremetal-operator/pkg/bmc"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner"
+	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/clients"
 
 	// We don't use this package directly here, but need it imported
 	// so it registers its test fixture with the other BMC access
@@ -22,6 +23,40 @@ func init() {
 	logf.SetLogger(logz.New(logz.UseDevMode(true)))
 }
 
+func newTestProvisionerFactory() ironicProvisionerFactory {
+	return ironicProvisionerFactory{
+		log: logf.Log,
+		config: ironicConfig{
+			deployKernelURL:  "http://deploy.test/ipa.kernel",
+			deployRamdiskURL: "http://deploy.test/ipa.initramfs",
+			deployISOURL:     "http://deploy.test/ipa.iso",
+			maxBusyHosts:     20,
+		},
+	}
+}
+
+// A private function to construct an ironicProvisioner (rather than a
+// Provisioner interface) in a consistent way for tests.
+func newProvisionerWithSettings(host metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher, ironicURL string, ironicAuthSettings clients.AuthConfig, inspectorURL string, inspectorAuthSettings clients.AuthConfig) (*ironicProvisioner, error) {
+	hostData := provisioner.BuildHostData(host, bmcCreds)
+
+	tlsConf := clients.TLSConfig{}
+	clientIronic, err := clients.IronicClient(ironicURL, ironicAuthSettings, tlsConf)
+	if err != nil {
+		return nil, err
+	}
+
+	clientInspector, err := clients.InspectorClient(inspectorURL, inspectorAuthSettings, tlsConf)
+	if err != nil {
+		return nil, err
+	}
+
+	factory := newTestProvisionerFactory()
+	factory.clientIronic = clientIronic
+	factory.clientInspector = clientInspector
+	return factory.ironicProvisioner(hostData, publisher)
+}
+
 func makeHost() metal3v1alpha1.BareMetalHost {
 	rotational := true
 
@@ -92,7 +127,8 @@ func TestNewNoBMCDetails(t *testing.T) {
 	host := makeHost()
 	host.Spec.BMC = metal3v1alpha1.BMCDetails{}
 
-	prov, err := New(provisioner.BuildHostData(host, bmc.Credentials{}), nullEventPublisher)
+	factory := newTestProvisionerFactory()
+	prov, err := factory.NewProvisioner(provisioner.BuildHostData(host, bmc.Credentials{}), nullEventPublisher)
 	assert.Equal(t, nil, err)
 	assert.NotEqual(t, nil, prov)
 }
diff --git a/pkg/provisioner/ironic/provisioncapacity_test.go b/pkg/provisioner/ironic/provisioncapacity_test.go
index a1c9a8ce..abb0436c 100644
--- a/pkg/provisioner/ironic/provisioncapacity_test.go
+++ b/pkg/provisioner/ironic/provisioncapacity_test.go
@@ -81,14 +81,13 @@ func TestHasCapacity(t *testing.T) {
 
 			auth := clients.AuthConfig{Type: clients.NoAuth}
 
-			maxBusyHosts = tc.provisioningLimit
-
 			prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nullEventPublisher,
 				ironic.Endpoint(), auth, inspector.Endpoint(), auth,
 			)
 			if err != nil {
 				t.Fatalf("could not create provisioner: %s", err)
 			}
+			prov.config.maxBusyHosts = tc.provisioningLimit
 
 			result, err := prov.HasCapacity()
 
diff --git a/pkg/provisioner/ironic/updateopts_test.go b/pkg/provisioner/ironic/updateopts_test.go
index d5def408..515158fc 100644
--- a/pkg/provisioner/ironic/updateopts_test.go
+++ b/pkg/provisioner/ironic/updateopts_test.go
@@ -9,6 +9,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/pointer"
+	logf "sigs.k8s.io/controller-runtime/pkg/log"
 
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 	"github.com/metal3-io/baremetal-operator/pkg/bmc"
@@ -392,7 +393,7 @@ func TestGetUpdateOperation(t *testing.T) {
 }
 
 func TestTopLevelUpdateOpt(t *testing.T) {
-	u := updateOptsBuilder(log)
+	u := updateOptsBuilder(logf.Log)
 	u.SetTopLevelOpt("foo", "baz", "bar")
 	ops := u.Updates
 	assert.Len(t, ops, 1)
@@ -401,7 +402,7 @@ func TestTopLevelUpdateOpt(t *testing.T) {
 	assert.Equal(t, "baz", op.Value)
 	assert.Equal(t, "/foo", op.Path)
 
-	u = updateOptsBuilder(log)
+	u = updateOptsBuilder(logf.Log)
 	u.SetTopLevelOpt("foo", "bar", "bar")
 	assert.Len(t, u.Updates, 0)
 }
@@ -417,7 +418,7 @@ func TestPropertiesUpdateOpts(t *testing.T) {
 		},
 	}
 
-	u := updateOptsBuilder(log)
+	u := updateOptsBuilder(logf.Log)
 	u.SetPropertiesOpts(newValues, &node)
 	ops := u.Updates
 	assert.Len(t, ops, 1)
@@ -438,7 +439,7 @@ func TestInstanceInfoUpdateOpts(t *testing.T) {
 		},
 	}
 
-	u := updateOptsBuilder(log)
+	u := updateOptsBuilder(logf.Log)
 	u.SetInstanceInfoOpts(newValues, &node)
 	ops := u.Updates
 	assert.Len(t, ops, 1)