Skip to content

Commit

Permalink
Allow BMC address to be updated
Browse files Browse the repository at this point in the history 
Allow BMC address to be updated if the BMH is in the Registering state
or if it is detached.

Signed-off-by: Mahnoor Asghar <masghar@redhat.com>
  • Loading branch information
@MahnoorAsghar
MahnoorAsghar committed Feb 21, 2024
1 parent 7be20f3 commit 28aa2b5
Show file tree
Hide file tree
Showing 4 changed files with 183 additions and 7 deletions.
2 changes: 1 addition & 1 deletion apis/metal3.io/v1alpha1/baremetalhost_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
43 changes: 40 additions & 3 deletions 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,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",
Expand All @@ -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,
Expand All @@ -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,
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 28aa2b5

Please sign in to comment.