🌱 Allow BMC address to be updated

apis/metal3.io/v1alpha1/baremetalhost_validation.go

@@ -75,8 +75,11 @@ func (host *BareMetalHost) validateChanges(old *BareMetalHost) []error {
 		errs = append(errs, err...)
 	}
 
-	if old.Spec.BMC.Address != "" && host.Spec.BMC.Address != old.Spec.BMC.Address {
-		errs = append(errs, errors.New("BMC address can not be changed once it is set"))
+	if old.Spec.BMC.Address != "" &&
+		host.Spec.BMC.Address != old.Spec.BMC.Address &&
+		host.Status.OperationalStatus != OperationalStatusDetached &&
+		host.Status.Provisioning.State != StateRegistering {
+		errs = append(errs, errors.New("BMC address can not be changed if the BMH is not in the Registering state, or if the BMH is not detached"))
 	}
 
 	if old.Spec.BootMACAddress != "" && host.Spec.BootMACAddress != old.Spec.BootMACAddress {

apis/metal3.io/v1alpha1/baremetalhost_validation_test.go

@@ -720,7 +720,44 @@ func TestValidateUpdate(t *testing.T) {
 				Spec: BareMetalHostSpec{
 					BMC: BMCDetails{
 						Address: "test-address"}}},
-			wantedErr: "BMC address can not be changed once it is set",
+			wantedErr: "BMC address can not be changed if the BMH is not in the Registering state, or if the BMH is not detached",
+		},
+		{
+			name: "updateAddressBMHRegistering",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					BMC: BMCDetails{
+						Address: "test-address-changed"}},
+				Status: BareMetalHostStatus{
+					Provisioning: ProvisionStatus{
+						State: StateRegistering}}},
+			oldBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					BMC: BMCDetails{
+						Address: "test-address"}}},
+			wantedErr: "",
+		},
+		{
+			name: "updateAddressBMHDetached",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					BMC: BMCDetails{
+						Address: "test-address-changed"}},
+				Status: BareMetalHostStatus{
+					OperationalStatus: OperationalStatusDetached}},
+			oldBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					BMC: BMCDetails{
+						Address: "test-address"}}},
+			wantedErr: "",
 		},
 		{
 			name: "updateBootMAC",

pkg/provisioner/ironic/ironic.go

@@ -3,6 +3,8 @@ package ironic
 import (
 	"fmt"
 	"net"
+	"reflect"
+	"regexp"
 	"strings"
 	"time"
 
@@ -443,6 +445,26 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
 
 		updater.SetTopLevelOpt("name", ironicNodeName(p.objectMeta), ironicNode.Name)
 
+		var bmcAddressChanged bool
+		newAddress := make(map[string]interface{})
+		ironicAddress := make(map[string]interface{})
+		reg := regexp.MustCompile("_address$")
+		for key, value := range driverInfo {
+			if reg.MatchString(key) {
+				newAddress[key] = value
+				break
+			}
+		}
+		for key, value := range ironicNode.DriverInfo {
+			if reg.MatchString(key) {
+				ironicAddress[key] = value
+				break
+			}
+		}
+		if !reflect.DeepEqual(newAddress, ironicAddress) {
+			bmcAddressChanged = true
+		}
+
 		// When node exists but has no assigned port to it by Ironic and actuall address (MAC) is present
 		// in host config and is not allocated to different node lets try to create port for this node.
 		if p.bootMACAddress != "" {
@@ -472,9 +494,11 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
 		}
 
 		// The actual password is not returned from ironic, so we want to
-		// update the whole DriverInfo only if the credentials have changed
-		// otherwise we will be writing on every call to this function.
-		if credentialsChanged {
+		// update the whole DriverInfo only if the credentials or BMC address
+		// has changed, otherwise we will be writing on every call to this
+		// function.
+		if credentialsChanged || bmcAddressChanged {
+			p.log.Info("Updating driver info because the credentials and/or the BMC address changed")
 			updater.SetTopLevelOpt("driver_info", driverInfo, ironicNode.DriverInfo)
 		}
 

pkg/provisioner/ironic/validatemanagementaccess_test.go

@@ -892,6 +892,121 @@ func TestValidateManagementAccessMalformedBMCAddress(t *testing.T) {
 	assert.Equal(t, "failed to parse BMC address information: failed to parse BMC address information: parse \"<ipmi://192.168.122.1:6233>\": first path segment in URL cannot contain colon", result.ErrorMessage)
 }
 
+func TestValidateManagementAccessUpdateBMCAddressIP(t *testing.T) {
+	host := makeHost()
+	host.Spec.BMC.Address = "ipmi://192.168.122.10:623"
+	host.Status.Provisioning.ID = "uuid"
+
+	ironic := testserver.NewIronic(t).
+		Node(nodes.Node{
+			Name: host.Namespace + nameSeparator + host.Name,
+			UUID: "uuid",
+			DriverInfo: map[string]interface{}{
+				"ipmi_address":    "192.168.122.1",
+				"ipmi_port":       623,
+				"ipmi_username":   "",
+				"ipmi_password":   "",
+				"ipmi_priv_level": "ADMINISTRATOR",
+			},
+			ProvisionState: string(nodes.Verifying),
+		}).NodeUpdate(
+		nodes.Node{
+			Name: host.Namespace + nameSeparator + host.Name,
+			UUID: "uuid",
+			DriverInfo: map[string]interface{}{
+				"ipmi_address":    "192.168.122.10",
+				"ipmi_port":       623,
+				"ipmi_username":   "",
+				"ipmi_password":   "",
+				"ipmi_priv_level": "ADMINISTRATOR",
+			},
+			ProvisionState: string(nodes.Verifying),
+		})
+
+	ironic.Start()
+	defer ironic.Stop()
+
+	auth := clients.AuthConfig{Type: clients.NoAuth}
+	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nil, ironic.Endpoint(), auth)
+
+	if err != nil {
+		t.Fatalf("could not create provisioner: %s", err)
+	}
+
+	result, provID, err := prov.ValidateManagementAccess(provisioner.ManagementAccessData{}, false, false)
+	if err != nil {
+		t.Fatalf("error from ValidateManagementAccess: %s", err)
+	}
+	assert.Equal(t, "", result.ErrorMessage)
+	assert.Equal(t, "uuid", provID)
+
+	updates := ironic.GetLastNodeUpdateRequestFor("uuid")
+	assert.Equal(t, "/driver_info", updates[0].Path)
+	newValues := updates[0].Value.(map[string]interface{})
+	assert.Equal(t, "192.168.122.10", newValues["ipmi_address"])
+}
+
+func TestValidateManagementAccessUpdateBMCAddressProtocol(t *testing.T) {
+	host := makeHost()
+	host.Spec.BMC.Address = "redfish://192.168.122.1:623"
+	host.Spec.BootMACAddress = "11:11:11:11:11:11"
+	host.Status.Provisioning.ID = "uuid"
+
+	nodePort := ports.Port{
+		NodeUUID: "uuid",
+		Address:  "11:11:11:11:11:11",
+	}
+
+	ironic := testserver.NewIronic(t).
+		Node(nodes.Node{
+			Name: host.Namespace + nameSeparator + host.Name,
+			UUID: "uuid",
+			DriverInfo: map[string]interface{}{
+				"ipmi_address":    "192.168.122.1",
+				"ipmi_port":       623,
+				"ipmi_username":   "",
+				"ipmi_password":   "",
+				"ipmi_priv_level": "ADMINISTRATOR",
+				"ipmi_verify_ca":  false,
+			},
+			ProvisionState: string(nodes.Verifying),
+		}).NodeUpdate(
+		nodes.Node{
+			Name: host.Namespace + nameSeparator + host.Name,
+			UUID: "uuid",
+			DriverInfo: map[string]interface{}{
+				"redfish_address":   "https://192.168.122.1:443",
+				"redfish_username":  "",
+				"redfish_password":  "",
+				"redfish_system_id": "/redfish/v1/Systems/1/",
+				"redfish_verify_ca": false,
+			},
+			ProvisionState: string(nodes.Verifying),
+		}).Port(nodePort)
+
+	ironic.Start()
+	defer ironic.Stop()
+
+	auth := clients.AuthConfig{Type: clients.NoAuth}
+	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nil, ironic.Endpoint(), auth)
+
+	if err != nil {
+		t.Fatalf("could not create provisioner: %s", err)
+	}
+
+	result, provID, err := prov.ValidateManagementAccess(provisioner.ManagementAccessData{}, false, false)
+	if err != nil {
+		t.Fatalf("error from ValidateManagementAccess: %s", err)
+	}
+	assert.Equal(t, "", result.ErrorMessage)
+	assert.Equal(t, "uuid", provID)
+
+	updates := ironic.GetLastNodeUpdateRequestFor("uuid")
+	assert.Equal(t, "/driver_info", updates[0].Path)
+	newValues := updates[0].Value.(map[string]interface{})
+	assert.Equal(t, "https://192.168.122.1:623", newValues["redfish_address"])
+}
+
 func TestPreprovisioningImageFormats(t *testing.T) {
 	ironicEndpoint := "http://ironic.test"
 	auth := clients.AuthConfig{Type: clients.NoAuth}