Skip to content

Commit

Permalink
Merge pull request #1549 from MahnoorAsghar/main
Browse files Browse the repository at this point in the history 
🌱 Allow BMC address to be updated
  • Loading branch information
@metal3-io-bot
metal3-io-bot authored Feb 22, 2024
2 parents 3cd8e38 + 2ec3f0d commit ffad1a5
Show file tree
Hide file tree
Showing 4 changed files with 185 additions and 6 deletions.
7 changes: 5 additions & 2 deletions apis/metal3.io/v1alpha1/baremetalhost_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
39 changes: 38 additions & 1 deletion apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
30 changes: 27 additions & 3 deletions pkg/provisioner/ironic/ironic.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package ironic
import (
"fmt"
"net"
"reflect"
"regexp"
"strings"
"time"

Expand Down Expand Up @@ -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 != "" {
Expand Down Expand Up @@ -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)
}

Expand Down
115 changes: 115 additions & 0 deletions pkg/provisioner/ironic/validatemanagementaccess_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down

0 comments on commit ffad1a5

Please sign in to comment.