Repository: metal3-io/baremetal-operator
Pull request #780 (2021-01-26 18:26:34)

Fail registration when boot MAC conflicts
If we can't find a node by name, and have to resort to using MAC address
lookup, we can't accept a node that has a name.  In that scenario, we
have detected a MAC address collision, and human intervention is
required.  We signal this by returning an `operationFailed` provisioning
result with the offending MAC address in the error message.

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

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/780#" title="Author self-approved">honza</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **hardys**
You can assign the PR to them by writing `/assign @hardys` in a comment when ready.

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":["hardys"]} -->
zaneb: /approve
/cc @andfasano 
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/780#" title="Author self-approved">honza</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/780#issuecomment-768004406" title="Approved">zaneb</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)~~ [zaneb]

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":[]} -->
andfasano: /lgtm
/hold waiting for the completion of the test-integration
honza: /ok-to-test
honza: /test-integration

Patch:
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 862f4624..06971aff 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -60,6 +60,19 @@ var bootModeCapabilities = map[metal3v1alpha1.BootMode]string{
 	metal3v1alpha1.Legacy: "boot_mode:bios",
 }
 
+type macAddressConflictError struct {
+	Address      string
+	ExistingNode string
+}
+
+func (e macAddressConflictError) Error() string {
+	return fmt.Sprintf("MAC address %s conflicts with existing node %s", e.Address, e.ExistingNode)
+}
+
+func NewMacAddressConflictError(address, node string) error {
+	return macAddressConflictError{Address: address, ExistingNode: node}
+}
+
 func init() {
 	// NOTE(dhellmann): Use Fprintf() to report errors instead of
 	// logging, because logging is not configured yet in init().
@@ -310,7 +323,7 @@ func (p *ironicProvisioner) findExistingHost() (ironicNode *nodes.Node, err erro
 
 			// If the node has a name, this means we didn't find it above.
 			if ironicNode.Name != "" {
-				return nil, errors.New(fmt.Sprint("node found by MAC but has a name: ", ironicNode.Name))
+				return nil, NewMacAddressConflictError(p.host.Spec.BootMACAddress, ironicNode.Name)
 			}
 
 			return ironicNode, nil
@@ -342,7 +355,12 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b
 
 	ironicNode, err = p.findExistingHost()
 	if err != nil {
-		result, err = transientError(errors.Wrap(err, "failed to find existing host"))
+		switch err.(type) {
+		case macAddressConflictError:
+			result, err = operationFailed(err.Error())
+		default:
+			result, err = transientError(errors.Wrap(err, "failed to find existing host"))
+		}
 		return
 	}
 


Test patch:
diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go
index 0ab315c6..6ac15ca8 100644
--- a/pkg/provisioner/ironic/validatemanagementaccess_test.go
+++ b/pkg/provisioner/ironic/validatemanagementaccess_test.go
@@ -574,8 +574,9 @@ func TestValidateManagementAccessExistingPortButHasName(t *testing.T) {
 		t.Fatalf("could not create provisioner: %s", err)
 	}
 
-	_, _, err = prov.ValidateManagementAccess(false, false)
-	assert.EqualError(t, err, "failed to find existing host: node found by MAC but has a name: wrong-name")
+	res, _, err := prov.ValidateManagementAccess(false, false)
+	assert.Nil(t, err)
+	assert.Equal(t, res.ErrorMessage, "MAC address 11:11:11:11:11:11 conflicts with existing node wrong-name")
 }
 
 func TestValidateManagementAccessAddTwoHostsWithSameMAC(t *testing.T) {