From 28aa2b547f23763116c7174919f49277d975a44f Mon Sep 17 00:00:00 2001 From: Mahnoor Asghar Date: Wed, 24 Jan 2024 06:58:08 -0500 Subject: [PATCH] Allow BMC address to be updated Allow BMC address to be updated if the BMH is in the Registering state or if it is detached. Signed-off-by: Mahnoor Asghar --- .../v1alpha1/baremetalhost_validation.go | 2 +- .../v1alpha1/baremetalhost_validation_test.go | 43 ++++++- pkg/provisioner/ironic/ironic.go | 30 ++++- .../ironic/validatemanagementaccess_test.go | 115 ++++++++++++++++++ 4 files changed, 183 insertions(+), 7 deletions(-) diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation.go b/apis/metal3.io/v1alpha1/baremetalhost_validation.go index 67c65027da..35c113d532 100644 --- a/apis/metal3.io/v1alpha1/baremetalhost_validation.go +++ b/apis/metal3.io/v1alpha1/baremetalhost_validation.go @@ -79,7 +79,7 @@ func (host *BareMetalHost) validateChanges(old *BareMetalHost) []error { 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 once it is set")) + 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/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go index 2af42a4acf..c76fcd944e 100644 --- a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go +++ b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go @@ -720,7 +720,7 @@ 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", @@ -732,7 +732,7 @@ func TestValidateUpdate(t *testing.T) { Address: "test-address-changed"}}, Status: BareMetalHostStatus{ Provisioning: ProvisionStatus{ - State: ProvisioningState(StateRegistering)}}}, + State: StateRegistering}}}, oldBMH: &BareMetalHost{ TypeMeta: tm, ObjectMeta: om, @@ -750,7 +750,44 @@ func TestValidateUpdate(t *testing.T) { BMC: BMCDetails{ Address: "test-address-changed"}}, Status: BareMetalHostStatus{ - OperationalStatus: OperationalStatus(OperationalStatusDetached)}}, + OperationalStatus: OperationalStatusDetached}}, + oldBMH: &BareMetalHost{ + TypeMeta: tm, + ObjectMeta: om, + Spec: BareMetalHostSpec{ + BMC: BMCDetails{ + Address: "test-address"}}}, + wantedErr: "", + }, + { + 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, diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go index 190e54f5c2..3117ca2447 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) } diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go index 8848b144b9..1a4d3bc327 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 \"\": 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}