Repository: metal3-io/baremetal-operator
Pull request #1549 (2024-02-08 10:18:37)

🌱 Allow BMC address to be updated
Allow BMC address to be updated if the BMH is in the Registering state or is detached.

This allows the BMC address to be updated if there are changes in the BMC network, or if a mistake was made when setting it the first time.

MahnoorAsghar: /test metal3-bmo-e2e-test-pull
/test-centos-e2e-integration-main
kashifest: /test-centos-e2e-integration-main
kashifest: Finally jenkins woke up
tuminoid: For the record, here is the original one with reviews and discussions: https://github.com/metal3-io/baremetal-operator/pull/1532 
elfosardo: /lgtm
metal3-io-bot: New changes are detected. LGTM label has been removed.
metal3-io-bot: @MahnoorAsghar: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
golint | bb8c2a7f083f7571a4a92f2d8795ad305641832e | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/1549/golint/1760297576512360448) | true | `/test golint`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=1549). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:MahnoorAsghar).

<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 -->
MahnoorAsghar: /test metal3-bmo-e2e-test-pull
/test-centos-e2e-integration-main

Patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation.go b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
index ad2c85b2..35c113d5 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation.go
+++ b/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 {
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 190e54f5..3117ca24 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/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)
 		}
 


Test patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
index bad5ec27..521a6a7e 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
+++ b/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",
diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go
index 8848b144..1a4d3bc3 100644
--- a/pkg/provisioner/ironic/validatemanagementaccess_test.go
+++ b/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}