Repository: metal3-io/baremetal-operator
Pull request #647 (2020-09-23 13:51:55)

start adding unit tests for ironic.ValidateManagementAccess

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

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/647#" title="Author self-approved">dhellmann</a>*

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

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

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

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

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":[]} -->
metal3-io-bot: @dhellmann: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
golint | 89806b8873d6fa0afa61c3026572e90ecf6cb514 | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/647/golint/1308766126636797955) | `/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 -->
dhellmann: /test-integration

dhellmann: I would like to freeze non-essential changes for a few days to try to land https://github.com/metal3-io/baremetal-operator/pull/650 without having to rebase it, because rebasing will mean redoing the work from scratch.

/hold

metal3-io-bot: @dhellmann: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
generate | d791e5b286468f3d7f7d48bf9973c14234eed286 | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/647/generate/1309754645085687812) | `/test generate`



<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 -->
dhellmann: #655 has merged

/hold cancel

dhellmann: /test-integration
dhellmann: I've rebased this on the work @andfasano did in #649 

/test-integration

metal3-io-bot: @dhellmann: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
golint | 28d2d40c082bcda72b7896e4457b07bdc5f63561 | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/647/golint/1314636209305161732) | `/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 -->
metal3-io-bot: @dhellmann: The following tests **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
golint | 28d2d40c082bcda72b7896e4457b07bdc5f63561 | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/647/golint/1314636209305161732) | `/test golint`
govet | 28d2d40c082bcda72b7896e4457b07bdc5f63561 | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/647/govet/1314636209305161735) | `/test govet`



<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 -->
metal3-io-bot: @dhellmann: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
golint | 2f1cebc1a4e213ba044c27f02db23ad28c29628b | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/647/golint/1316022270199599110) | `/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 -->
metal3-io-bot: @dhellmann: The following tests **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
golint | 2f1cebc1a4e213ba044c27f02db23ad28c29628b | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/647/golint/1316022270199599110) | `/test golint`
govet | 2f1cebc1a4e213ba044c27f02db23ad28c29628b | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/647/govet/1316022270199599108) | `/test govet`



<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 -->
metal3-io-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/647#pullrequestreview-507570982" title="Approved">andfasano</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/647#" title="Author self-approved">dhellmann</a>*

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

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

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

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

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":[]} -->

Patch:
diff --git a/pkg/bmc/access.go b/pkg/bmc/access.go
index 98a90d2c..02d3f96e 100644
--- a/pkg/bmc/access.go
+++ b/pkg/bmc/access.go
@@ -15,9 +15,12 @@ type AccessDetailsFactory func(parsedURL *url.URL, disableCertificateVerificatio
 
 var factories = map[string]AccessDetailsFactory{}
 
-// We could make this function public if we want to support
-// out-of-tree factories.
-func registerFactory(name string, factory AccessDetailsFactory, schemes []string) {
+// RegisterFactory maps a BMC type name to an AccessDetailsFactory,
+// with optional scheme extensions.
+//
+// RegisterFactory("bmcname", theFunc, []string{"http", "https"})
+// maps "bmcname", "bmcname+http", and "bmcname+https" to theFunc
+func RegisterFactory(name string, factory AccessDetailsFactory, schemes []string) {
 	factories[name] = factory
 
 	for _, scheme := range schemes {
@@ -106,6 +109,10 @@ func getParsedURL(address string) (parsedURL *url.URL, err error) {
 // for a BMC.
 func NewAccessDetails(address string, disableCertificateVerification bool) (AccessDetails, error) {
 
+	if address == "" {
+		return nil, errors.New("missing BMC address")
+	}
+
 	parsedURL, err := getParsedURL(address)
 	if err != nil {
 		return nil, err
diff --git a/pkg/bmc/ibmc.go b/pkg/bmc/ibmc.go
index f726d56e..613e3168 100644
--- a/pkg/bmc/ibmc.go
+++ b/pkg/bmc/ibmc.go
@@ -6,7 +6,7 @@ import (
 )
 
 func init() {
-	registerFactory("ibmc", newIbmcAccessDetails, []string{"http", "https"})
+	RegisterFactory("ibmc", newIbmcAccessDetails, []string{"http", "https"})
 }
 
 func newIbmcAccessDetails(parsedURL *url.URL, disableCertificateVerification bool) (AccessDetails, error) {
diff --git a/pkg/bmc/idrac.go b/pkg/bmc/idrac.go
index 943f005a..ee739e1a 100644
--- a/pkg/bmc/idrac.go
+++ b/pkg/bmc/idrac.go
@@ -6,7 +6,7 @@ import (
 )
 
 func init() {
-	registerFactory("idrac", newIDRACAccessDetails, []string{"http", "https"})
+	RegisterFactory("idrac", newIDRACAccessDetails, []string{"http", "https"})
 }
 
 func newIDRACAccessDetails(parsedURL *url.URL, disableCertificateVerification bool) (AccessDetails, error) {
diff --git a/pkg/bmc/idrac_virtualmedia.go b/pkg/bmc/idrac_virtualmedia.go
index bf06ae2e..6b22b2a8 100644
--- a/pkg/bmc/idrac_virtualmedia.go
+++ b/pkg/bmc/idrac_virtualmedia.go
@@ -6,7 +6,7 @@ import (
 
 func init() {
 	schemes := []string{"http", "https"}
-	registerFactory("idrac-virtualmedia", newRedfishiDracVirtualMediaAccessDetails, schemes)
+	RegisterFactory("idrac-virtualmedia", newRedfishiDracVirtualMediaAccessDetails, schemes)
 }
 
 func newRedfishiDracVirtualMediaAccessDetails(parsedURL *url.URL, disableCertificateVerification bool) (AccessDetails, error) {
diff --git a/pkg/bmc/ilo4.go b/pkg/bmc/ilo4.go
index 0dca7050..25b88fc1 100644
--- a/pkg/bmc/ilo4.go
+++ b/pkg/bmc/ilo4.go
@@ -7,7 +7,7 @@ import (
 )
 
 func init() {
-	registerFactory("ilo4", newILOAccessDetails, []string{"https"})
+	RegisterFactory("ilo4", newILOAccessDetails, []string{"https"})
 }
 
 func newILOAccessDetails(parsedURL *url.URL, disableCertificateVerification bool) (AccessDetails, error) {
diff --git a/pkg/bmc/ilo5.go b/pkg/bmc/ilo5.go
index 5ba56ee0..b15c0fcd 100644
--- a/pkg/bmc/ilo5.go
+++ b/pkg/bmc/ilo5.go
@@ -7,7 +7,7 @@ import (
 )
 
 func init() {
-	registerFactory("ilo5", newILO5AccessDetails, []string{"https"})
+	RegisterFactory("ilo5", newILO5AccessDetails, []string{"https"})
 }
 
 func newILO5AccessDetails(parsedURL *url.URL, disableCertificateVerification bool) (AccessDetails, error) {
diff --git a/pkg/bmc/ipmi.go b/pkg/bmc/ipmi.go
index a65cea1d..6646ce14 100644
--- a/pkg/bmc/ipmi.go
+++ b/pkg/bmc/ipmi.go
@@ -5,8 +5,8 @@ import (
 )
 
 func init() {
-	registerFactory("ipmi", newIPMIAccessDetails, []string{})
-	registerFactory("libvirt", newIPMIAccessDetails, []string{})
+	RegisterFactory("ipmi", newIPMIAccessDetails, []string{})
+	RegisterFactory("libvirt", newIPMIAccessDetails, []string{})
 }
 
 func newIPMIAccessDetails(parsedURL *url.URL, disableCertificateVerification bool) (AccessDetails, error) {
diff --git a/pkg/bmc/irmc.go b/pkg/bmc/irmc.go
index cc7cfab7..a997dde7 100644
--- a/pkg/bmc/irmc.go
+++ b/pkg/bmc/irmc.go
@@ -5,7 +5,7 @@ import (
 )
 
 func init() {
-	registerFactory("irmc", newIRMCAccessDetails, []string{})
+	RegisterFactory("irmc", newIRMCAccessDetails, []string{})
 }
 
 func newIRMCAccessDetails(parsedURL *url.URL, disableCertificateVerification bool) (AccessDetails, error) {
diff --git a/pkg/bmc/redfish.go b/pkg/bmc/redfish.go
index fe624de9..82746db6 100644
--- a/pkg/bmc/redfish.go
+++ b/pkg/bmc/redfish.go
@@ -7,8 +7,8 @@ import (
 
 func init() {
 	schemes := []string{"http", "https"}
-	registerFactory("redfish", newRedfishAccessDetails, schemes)
-	registerFactory("ilo5-redfish", newRedfishAccessDetails, schemes)
+	RegisterFactory("redfish", newRedfishAccessDetails, schemes)
+	RegisterFactory("ilo5-redfish", newRedfishAccessDetails, schemes)
 }
 
 func redfishDetails(parsedURL *url.URL, disableCertificateVerification bool) *redfishAccessDetails {
diff --git a/pkg/bmc/redfish_virtualmedia.go b/pkg/bmc/redfish_virtualmedia.go
index 1339afb9..a601fc18 100644
--- a/pkg/bmc/redfish_virtualmedia.go
+++ b/pkg/bmc/redfish_virtualmedia.go
@@ -6,8 +6,8 @@ import (
 
 func init() {
 	schemes := []string{"http", "https"}
-	registerFactory("redfish-virtualmedia", newRedfishVirtualMediaAccessDetails, schemes)
-	registerFactory("ilo5-virtualmedia", newRedfishVirtualMediaAccessDetails, schemes)
+	RegisterFactory("redfish-virtualmedia", newRedfishVirtualMediaAccessDetails, schemes)
+	RegisterFactory("ilo5-virtualmedia", newRedfishVirtualMediaAccessDetails, schemes)
 }
 
 func newRedfishVirtualMediaAccessDetails(parsedURL *url.URL, disableCertificateVerification bool) (AccessDetails, error) {


Test patch:
diff --git a/pkg/provisioner/ironic/findhost_test.go b/pkg/provisioner/ironic/findhost_test.go
index 25619a7d..26cb7b2d 100644
--- a/pkg/provisioner/ironic/findhost_test.go
+++ b/pkg/provisioner/ironic/findhost_test.go
@@ -3,6 +3,8 @@ package ironic
 import (
 	"testing"
 
+	"github.com/gophercloud/gophercloud/openstack/baremetal/v1/nodes"
+
 	"github.com/metal3-io/baremetal-operator/pkg/bmc"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/clients"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/testserver"
@@ -11,7 +13,7 @@ import (
 func TestFindExistingHost(t *testing.T) {
 	cases := []struct {
 		name   string
-		ironic *testserver.MockServer
+		ironic *testserver.IronicMock
 
 		hostName       string
 		provisioningID string
@@ -21,23 +23,28 @@ func TestFindExistingHost(t *testing.T) {
 			name:           "no-node",
 			hostName:       "name",
 			provisioningID: "uuid",
-			ironic: testserver.New(t, "ironic").NotFound("/v1/nodes/name").
-				NotFound("/v1/nodes/uuid"),
+			ironic:         testserver.NewIronic(t).NoNode("name").NoNode("uuid"),
 		},
 		{
 			name:           "by-name",
 			hostName:       "name",
 			provisioningID: "uuid",
-			ironic: testserver.New(t, "ironic").NotFound("/v1/nodes/uuid").
-				Response("/v1/nodes/name", `{"name": "name", "uuid": "different-uuid"}`),
+			ironic: testserver.NewIronic(t).NoNode("uuid").
+				WithNode(nodes.Node{
+					Name: "name",
+					UUID: "different-uuid",
+				}),
 			nodeName: "name",
 		},
 		{
 			name:           "by-uuid",
 			hostName:       "name",
 			provisioningID: "uuid",
-			ironic: testserver.New(t, "ironic").NotFound("/v1/nodes/name").
-				Response("/v1/nodes/uuid", `{"name": "different-name", "uuid": "uuid"}`),
+			ironic: testserver.NewIronic(t).NoNode("name").
+				WithNode(nodes.Node{
+					Name: "different-name",
+					UUID: "uuid",
+				}),
 			nodeName: "different-name",
 		},
 	}
diff --git a/pkg/provisioner/ironic/ironic_test.go b/pkg/provisioner/ironic/ironic_test.go
index b553a0d3..fbf2cba0 100644
--- a/pkg/provisioner/ironic/ironic_test.go
+++ b/pkg/provisioner/ironic/ironic_test.go
@@ -5,6 +5,11 @@ import (
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
 
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
+
+	// We don't use this package directly here, but need it imported
+	// so it registers its test fixture with the other BMC access
+	// types.
+	_ "github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/testbmc"
 )
 
 func init() {
@@ -21,6 +26,9 @@ func makeHost() *metal3v1alpha1.BareMetalHost {
 			UID:       "27720611-e5d1-45d3-ba3a-222dcfaa4ca2",
 		},
 		Spec: metal3v1alpha1.BareMetalHostSpec{
+			BMC: metal3v1alpha1.BMCDetails{
+				Address: "test://test.bmc/",
+			},
 			Image: &metal3v1alpha1.Image{
 				URL: "not-empty",
 			},
@@ -62,3 +70,6 @@ func makeHost() *metal3v1alpha1.BareMetalHost {
 		},
 	}
 }
+
+// Implements provisioner.EventPublisher to swallow events for tests.
+func nullEventPublisher(reason, message string) {}
diff --git a/pkg/provisioner/ironic/testbmc/testbmc.go b/pkg/provisioner/ironic/testbmc/testbmc.go
new file mode 100644
index 00000000..cdb525ed
--- /dev/null
+++ b/pkg/provisioner/ironic/testbmc/testbmc.go
@@ -0,0 +1,83 @@
+package testbmc
+
+import (
+	"net/url"
+
+	"github.com/metal3-io/baremetal-operator/pkg/bmc"
+)
+
+func init() {
+	bmc.RegisterFactory("test", newTestBMCAccessDetails, []string{})
+	bmc.RegisterFactory("test-needs-mac", newTestBMCAccessDetails, []string{})
+}
+
+func newTestBMCAccessDetails(parsedURL *url.URL, disableCertificateVerification bool) (bmc.AccessDetails, error) {
+	return &testAccessDetails{
+		bmcType:                        parsedURL.Scheme,
+		hostname:                       parsedURL.Hostname(),
+		disableCertificateVerification: disableCertificateVerification,
+	}, nil
+}
+
+type testAccessDetails struct {
+	bmcType                        string
+	hostname                       string
+	disableCertificateVerification bool
+}
+
+func (a *testAccessDetails) Type() string {
+	return a.bmcType
+}
+
+// NeedsMAC returns true when the host is going to need a separate
+// port created rather than having it discovered.
+func (a *testAccessDetails) NeedsMAC() bool {
+	return a.bmcType == "test-needs-mac"
+}
+
+func (a *testAccessDetails) Driver() string {
+	return "test"
+}
+
+func (a *testAccessDetails) DisableCertificateVerification() bool {
+	return a.disableCertificateVerification
+}
+
+// DriverInfo returns a data structure to pass as the DriverInfo
+// parameter when creating a node in Ironic. The structure is
+// pre-populated with the access information, and the caller is
+// expected to add any other information that might be needed (such as
+// the kernel and ramdisk locations).
+func (a *testAccessDetails) DriverInfo(bmcCreds bmc.Credentials) map[string]interface{} {
+	result := map[string]interface{}{
+		"test_port":     "42",
+		"test_username": bmcCreds.Username,
+		"test_password": bmcCreds.Password,
+		"test_address":  a.hostname,
+	}
+
+	if a.disableCertificateVerification {
+		result["test_verify_ca"] = false
+	}
+	return result
+}
+
+func (a *testAccessDetails) BootInterface() string {
+	return "ipxe"
+}
+
+func (a *testAccessDetails) ManagementInterface() string {
+	return ""
+}
+
+func (a *testAccessDetails) PowerInterface() string {
+	return ""
+}
+
+func (a *testAccessDetails) RAIDInterface() string {
+	return ""
+}
+
+func (a *testAccessDetails) VendorInterface() string {
+	return ""
+}
diff --git a/pkg/provisioner/ironic/testserver/ironic.go b/pkg/provisioner/ironic/testserver/ironic.go
index 9974a6b7..2cfb982e 100644
--- a/pkg/provisioner/ironic/testserver/ironic.go
+++ b/pkg/provisioner/ironic/testserver/ironic.go
@@ -1,21 +1,33 @@
 package testserver
 
 import (
+	"fmt"
+	"io/ioutil"
 	"net/http"
+	"strings"
 	"testing"
 
 	"github.com/gophercloud/gophercloud/openstack/baremetal/v1/nodes"
 )
 
+// CreatedNode holds the body of the request to create the node and
+// the details generated by the server and included in the response.
+type CreatedNode struct {
+	Body string
+	UUID string
+}
+
 // IronicMock is a test server that implements Ironic's semantics
 type IronicMock struct {
 	*MockServer
+	CreatedNodes []CreatedNode
 }
 
 // NewIronic builds an ironic mock server
 func NewIronic(t *testing.T) *IronicMock {
 	return &IronicMock{
-		New(t, "ironic"),
+		MockServer:   New(t, "ironic"),
+		CreatedNodes: nil,
 	}
 }
 
@@ -66,7 +78,12 @@ func (m *IronicMock) WithDrivers() *IronicMock {
 
 // WithNode configures the server with a valid response for /v1/nodes
 func (m *IronicMock) WithNode(node nodes.Node) *IronicMock {
-	m.ResponseJSON("/v1/nodes/"+node.UUID, node)
+	if node.UUID != "" {
+		m.ResponseJSON("/v1/nodes/"+node.UUID, node)
+	}
+	if node.Name != "" {
+		m.ResponseJSON("/v1/nodes/"+node.Name, node)
+	}
 	return m
 }
 
@@ -75,3 +92,45 @@ func (m *IronicMock) WithNodeStatesProvision(nodeUUID string) *IronicMock {
 	m.ResponseWithCode("/v1/nodes/"+nodeUUID+"/states/provision", "{}", http.StatusAccepted)
 	return m
 }
+
+// NoNode configures the server so /v1/nodes/name returns a 404
+func (m *IronicMock) NoNode(name string) *IronicMock {
+	m.NotFound(fmt.Sprintf("/v1/nodes/%s", name))
+	return m
+}
+
+// CreateNodes configures the server so POSTing to /v1/nodes saves the data
+func (m *IronicMock) CreateNodes() *IronicMock {
+	m.Handler("/v1/nodes", func(w http.ResponseWriter, r *http.Request) {
+		bodyRaw, err := ioutil.ReadAll(r.Body)
+		if err != nil {
+			m.logRequest(r, fmt.Sprintf("ERROR: %s", err))
+			http.Error(w, fmt.Sprintf("%s", err), http.StatusInternalServerError)
+			return
+		}
+
+		body := string(bodyRaw)
+		m.t.Logf("%s: create nodes request %v", m.name, body)
+
+		// The UUID value doesn't actually have to be a UUID, so we
+		// just make a new string based on the count of nodes already
+		// created.
+		uuid := fmt.Sprintf("node-%d", len(m.CreatedNodes))
+		m.t.Logf("%s: uuid %s", m.name, uuid)
+
+		// Record what we have so the test can examine it later
+		m.CreatedNodes = append(m.CreatedNodes, CreatedNode{
+			Body: body,
+			UUID: uuid,
+		})
+
+		// hackily add uuid to the json response by inserting it to the front of the string
+		response := fmt.Sprintf("{\"uuid\": \"%s\", %s", uuid, strings.TrimLeft(body, "{"))
+
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusCreated)
+		fmt.Fprint(w, response)
+		m.logRequest(r, response)
+	})
+	return m
+}
diff --git a/pkg/provisioner/ironic/updateopts_test.go b/pkg/provisioner/ironic/updateopts_test.go
index 48ac66d6..88714db7 100644
--- a/pkg/provisioner/ironic/updateopts_test.go
+++ b/pkg/provisioner/ironic/updateopts_test.go
@@ -5,6 +5,7 @@ import (
 	"testing"
 
 	"github.com/gophercloud/gophercloud/openstack/baremetal/v1/nodes"
+	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/pointer"
@@ -89,13 +90,17 @@ func TestGetUpdateOptsForNodeVirtual(t *testing.T) {
 			UID:       "27720611-e5d1-45d3-ba3a-222dcfaa4ca2",
 		},
 		Spec: metal3v1alpha1.BareMetalHostSpec{
+			BMC: metal3v1alpha1.BMCDetails{
+				Address: "test://test.bmc/",
+			},
 			Image: &metal3v1alpha1.Image{
 				URL:          "not-empty",
 				Checksum:     "checksum",
 				ChecksumType: metal3v1alpha1.MD5,
 				DiskFormat:   pointer.StringPtr("raw"),
 			},
-			Online: true,
+			Online:          true,
+			HardwareProfile: "unknown",
 		},
 		Status: metal3v1alpha1.BareMetalHostStatus{
 			HardwareProfile: "libvirt",
@@ -112,7 +117,7 @@ func TestGetUpdateOptsForNodeVirtual(t *testing.T) {
 		"https://ironic.test", auth, "https://ironic.test", auth,
 	)
 	if err != nil {
-		t.Fatal(err)
+		t.Fatal(errors.Wrap(err, "could not create provisioner"))
 	}
 	ironicNode := &nodes.Node{}
 
@@ -190,6 +195,9 @@ func TestGetUpdateOptsForNodeDell(t *testing.T) {
 			UID:       "27720611-e5d1-45d3-ba3a-222dcfaa4ca2",
 		},
 		Spec: metal3v1alpha1.BareMetalHostSpec{
+			BMC: metal3v1alpha1.BMCDetails{
+				Address: "test://test.bmc/",
+			},
 			Image: &metal3v1alpha1.Image{
 				URL:          "not-empty",
 				Checksum:     "checksum",
diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go
new file mode 100644
index 00000000..af717d65
--- /dev/null
+++ b/pkg/provisioner/ironic/validatemanagementaccess_test.go
@@ -0,0 +1,97 @@
+package ironic
+
+import (
+	"testing"
+
+	"github.com/gophercloud/gophercloud/openstack/baremetal/v1/nodes"
+	"github.com/stretchr/testify/assert"
+
+	"github.com/metal3-io/baremetal-operator/pkg/bmc"
+	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/clients"
+	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/testserver"
+)
+
+func TestValidateManagementAccessNoMAC(t *testing.T) {
+	// Create a host without a bootMACAddress and with a BMC that
+	// requires one.
+	host := makeHost()
+	host.Spec.BMC.Address = "test-needs-mac://"
+	host.Spec.BootMACAddress = ""
+	host.Status.Provisioning.ID = "" // so we don't lookup by uuid
+
+	ironic := testserver.NewIronic(t).Ready().NoNode(host.Name)
+	ironic.Start()
+	defer ironic.Stop()
+
+	auth := clients.AuthConfig{Type: clients.NoAuth}
+	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nil,
+		ironic.Endpoint(), auth, testserver.NewInspector(t).Endpoint(), auth,
+	)
+	if err != nil {
+		t.Fatalf("could not create provisioner: %s", err)
+	}
+
+	result, err := prov.ValidateManagementAccess(false)
+	if err != nil {
+		t.Fatalf("error from ValidateManagementAccess: %s", err)
+	}
+	assert.Contains(t, result.ErrorMessage, "requires a BootMACAddress")
+}
+
+func TestValidateManagementAccessMACOptional(t *testing.T) {
+	// Create a host without a bootMACAddress and with a BMC that
+	// does not require one.
+	host := makeHost()
+	host.Spec.BootMACAddress = ""
+
+	// Set up ironic server to return the node
+	ironic := testserver.NewIronic(t).Ready().
+		WithNode(nodes.Node{
+			Name: host.Name,
+			UUID: host.Status.Provisioning.ID,
+		})
+	ironic.Start()
+	defer ironic.Stop()
+
+	auth := clients.AuthConfig{Type: clients.NoAuth}
+	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nil,
+		ironic.Endpoint(), auth, testserver.NewInspector(t).Endpoint(), auth,
+	)
+	if err != nil {
+		t.Fatalf("could not create provisioner: %s", err)
+	}
+
+	result, err := prov.ValidateManagementAccess(false)
+	if err != nil {
+		t.Fatalf("error from ValidateManagementAccess: %s", err)
+	}
+	assert.Equal(t, "", result.ErrorMessage)
+}
+
+func TestValidateManagementAccessCreateNode(t *testing.T) {
+	// Create a host without a bootMACAddress and with a BMC that
+	// does not require one.
+	host := makeHost()
+	host.Spec.BootMACAddress = ""
+	host.Status.Provisioning.ID = "" // so we don't lookup by uuid
+
+	ironic := testserver.NewIronic(t).Ready().CreateNodes().NoNode(host.Name)
+	ironic.Start()
+	defer ironic.Stop()
+
+	auth := clients.AuthConfig{Type: clients.NoAuth}
+	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nullEventPublisher,
+		ironic.Endpoint(), auth, testserver.NewInspector(t).Endpoint(), auth,
+	)
+	if err != nil {
+		t.Fatalf("could not create provisioner: %s", err)
+	}
+
+	result, err := prov.ValidateManagementAccess(false)
+	if err != nil {
+		t.Fatalf("error from ValidateManagementAccess: %s", err)
+	}
+	assert.Equal(t, "", result.ErrorMessage)
+	assert.NotEqual(t, "", host.Status.Provisioning.ID)
+	assert.Equal(t, ironic.CreatedNodes[0].UUID, host.Status.Provisioning.ID)
+}