diff --git a/internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go b/internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go index 398c62152d..e6d640fd1d 100644 --- a/internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go +++ b/internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go @@ -58,7 +58,7 @@ func (webhook *BareMetalHost) validateHost(host *metal3api.BareMetalHost) []erro errs = append(errs, raidErrors...) } - errs = append(errs, validateBMCAccess(host.Spec, bmcAccess)...) + errs = append(errs, validateBMCAccess(host, bmcAccess)...) if err := validateBMHName(host.Name); err != nil { errs = append(errs, err) @@ -120,8 +120,9 @@ func (webhook *BareMetalHost) validateChanges(oldObj *metal3api.BareMetalHost, n return errs } -func validateBMCAccess(s metal3api.BareMetalHostSpec, bmcAccess bmc.AccessDetails) []error { +func validateBMCAccess(host *metal3api.BareMetalHost, bmcAccess bmc.AccessDetails) []error { var errs []error + s := host.Spec if bmcAccess == nil { return errs @@ -139,7 +140,11 @@ func validateBMCAccess(s metal3api.BareMetalHostSpec, bmcAccess bmc.AccessDetail } } - if bmcAccess.NeedsMAC() && s.BootMACAddress == "" { + // Check if bootMACAddress is required + // Virtual media drivers (NeedsMAC() returns false) still require MAC when inspection is disabled + requiresMAC := bmcAccess.NeedsMAC() || host.InspectionDisabled() + + if requiresMAC && s.BootMACAddress == "" { errs = append(errs, fmt.Errorf("BMC driver %s requires a BootMACAddress value", bmcAccess.Type())) } diff --git a/internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go b/internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go index 6705f8d815..190abb6aa9 100644 --- a/internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go +++ b/internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go @@ -19,6 +19,8 @@ import ( "testing" metal3api "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1" + // Import BMC drivers to register their factories. + _ "github.com/metal3-io/baremetal-operator/pkg/hardwareutils/bmc" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" @@ -270,6 +272,71 @@ func TestValidateCreate(t *testing.T) { oldBMH: nil, wantedErr: "", }, + { + name: "BootMACAddressNotRequiredForVirtualMedia", + newBMH: &metal3api.BareMetalHost{ + TypeMeta: tm, + ObjectMeta: om, + Spec: metal3api.BareMetalHostSpec{ + BMC: metal3api.BMCDetails{ + Address: "idrac-virtualmedia+https://192.168.1.1/redfish/v1/Systems/System.Embedded.1", + CredentialsName: "test1", + }, + }}, + oldBMH: nil, + wantedErr: "", + }, + { + name: "BootMACAddressRequiredForVirtualMediaWithInspectionDisabled", + newBMH: &metal3api.BareMetalHost{ + TypeMeta: tm, + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "test-namespace", + Annotations: map[string]string{ + metal3api.InspectAnnotationPrefix: "disabled", + }, + }, + Spec: metal3api.BareMetalHostSpec{ + BMC: metal3api.BMCDetails{ + Address: "idrac-virtualmedia+https://192.168.1.1/redfish/v1/Systems/System.Embedded.1", + CredentialsName: "test1", + }, + }}, + oldBMH: nil, + wantedErr: "BMC driver idrac-virtualmedia+https requires a BootMACAddress value", + }, + { + name: "BootMACAddressProvidedForVirtualMediaWithInspectionDisabled", + newBMH: &metal3api.BareMetalHost{ + TypeMeta: tm, + ObjectMeta: om, + Spec: metal3api.BareMetalHostSpec{ + BMC: metal3api.BMCDetails{ + Address: "idrac-virtualmedia+https://192.168.1.1/redfish/v1/Systems/System.Embedded.1", + CredentialsName: "test1", + }, + BootMACAddress: "00:00:00:00:00:00", + InspectionMode: metal3api.InspectionModeDisabled, + }}, + oldBMH: nil, + wantedErr: "", + }, + { + name: "BootMACAddressNotRequiredForVirtualMediaWithInspectionEnabled", + newBMH: &metal3api.BareMetalHost{ + TypeMeta: tm, + ObjectMeta: om, + Spec: metal3api.BareMetalHostSpec{ + BMC: metal3api.BMCDetails{ + Address: "redfish-virtualmedia+https://192.168.1.1/redfish/v1/Systems/1", + CredentialsName: "test1", + }, + InspectionMode: metal3api.InspectionModeAgent, + }}, + oldBMH: nil, + wantedErr: "", + }, { name: "BootMACAddressRequired", newBMH: &metal3api.BareMetalHost{ @@ -1055,40 +1122,11 @@ func TestValidateUpdate(t *testing.T) { { name: "updateBootMAC", newBMH: &metal3api.BareMetalHost{ - TypeMeta: tm, ObjectMeta: om, Spec: metal3api.BareMetalHostSpec{BootMACAddress: "00:11:22:33:44:66"}}, + TypeMeta: tm, ObjectMeta: om, Spec: metal3api.BareMetalHostSpec{BootMACAddress: "test-mac-changed"}}, oldBMH: &metal3api.BareMetalHost{ - TypeMeta: tm, ObjectMeta: om, Spec: metal3api.BareMetalHostSpec{BootMACAddress: "00:11:22:33:44:55"}}, + TypeMeta: tm, ObjectMeta: om, Spec: metal3api.BareMetalHostSpec{BootMACAddress: "test-mac"}}, wantedErr: "bootMACAddress can not be changed once it is set", }, - { - name: "updateBootMACCaseOnly", - newBMH: &metal3api.BareMetalHost{ - TypeMeta: tm, ObjectMeta: om, Spec: metal3api.BareMetalHostSpec{BootMACAddress: "AA:BB:CC:DD:EE:FF"}}, - oldBMH: &metal3api.BareMetalHost{ - TypeMeta: tm, ObjectMeta: om, Spec: metal3api.BareMetalHostSpec{BootMACAddress: "aa:bb:cc:dd:ee:ff"}}, - wantedErr: "", - }, - { - name: "updateBootMACCaseMixed", - newBMH: &metal3api.BareMetalHost{ - TypeMeta: tm, ObjectMeta: om, Spec: metal3api.BareMetalHostSpec{BootMACAddress: "aA:Bb:cC:Dd:Ee:Ff"}}, - oldBMH: &metal3api.BareMetalHost{ - TypeMeta: tm, ObjectMeta: om, Spec: metal3api.BareMetalHostSpec{BootMACAddress: "AA:BB:CC:DD:EE:FF"}}, - wantedErr: "", - }, - { - name: "updateBootMACInvalidNew", - newBMH: &metal3api.BareMetalHost{ - TypeMeta: tm, ObjectMeta: om, Spec: metal3api.BareMetalHostSpec{ - BMC: metal3api.BMCDetails{ - Address: "redfish://127.0.0.1", - CredentialsName: "test1", - }, - BootMACAddress: "invalid-mac"}}, - oldBMH: &metal3api.BareMetalHost{ - TypeMeta: tm, ObjectMeta: om, Spec: metal3api.BareMetalHostSpec{BootMACAddress: "00:11:22:33:44:55"}}, - wantedErr: "address invalid-mac: invalid MAC address", - }, { name: "updateExternallyProvisioned", newBMH: &metal3api.BareMetalHost{ diff --git a/internal/webhooks/metal3.io/v1alpha1/baremetalhost_webhook_test.go b/internal/webhooks/metal3.io/v1alpha1/baremetalhost_webhook_test.go index 28b161916b..cc4abb45d8 100644 --- a/internal/webhooks/metal3.io/v1alpha1/baremetalhost_webhook_test.go +++ b/internal/webhooks/metal3.io/v1alpha1/baremetalhost_webhook_test.go @@ -52,6 +52,22 @@ func TestBareMetalHostCreate(t *testing.T) { }, Spec: metal3api.BareMetalHostSpec{}}, wantedErr: "", }, + { + name: "invalid-no-bootMACAddress-no-preprovisioningNetworkData", + bmh: &metal3api.BareMetalHost{TypeMeta: metav1.TypeMeta{ + Kind: "BareMetalHost", + APIVersion: "metal3.io/v1alpha1", + }, ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "test-namespace", + }, Spec: metal3api.BareMetalHostSpec{ + BMC: metal3api.BMCDetails{ + Address: "libvirt://192.168.122.1:16509/", + }, + BootMACAddress: "", + }}, + wantedErr: "BMC driver libvirt requires a BootMACAddress value", + }, } for _, tt := range tests { diff --git a/pkg/hardwareutils/bmc/access_test.go b/pkg/hardwareutils/bmc/access_test.go index be84e33936..4faafae5b8 100644 --- a/pkg/hardwareutils/bmc/access_test.go +++ b/pkg/hardwareutils/bmc/access_test.go @@ -339,7 +339,7 @@ func TestStaticDriverInfo(t *testing.T) { { Scenario: "redfish virtual media", input: "redfish-virtualmedia://192.168.122.1", - needsMac: true, + needsMac: false, driver: "redfish", bios: "", boot: "redfish-virtual-media", @@ -351,7 +351,7 @@ func TestStaticDriverInfo(t *testing.T) { { Scenario: "redfish virtual media HTTP", input: "redfish-virtualmedia+http://192.168.122.1", - needsMac: true, + needsMac: false, driver: "redfish", bios: "", boot: "redfish-virtual-media", @@ -363,7 +363,7 @@ func TestStaticDriverInfo(t *testing.T) { { Scenario: "redfish virtual media HTTPS", input: "redfish-virtualmedia+https://192.168.122.1", - needsMac: true, + needsMac: false, driver: "redfish", bios: "", boot: "redfish-virtual-media", @@ -400,7 +400,7 @@ func TestStaticDriverInfo(t *testing.T) { { Scenario: "ilo5 virtual media", input: "ilo5-virtualmedia://192.168.122.1", - needsMac: true, + needsMac: false, driver: "redfish", bios: "", boot: "redfish-virtual-media", @@ -410,7 +410,7 @@ func TestStaticDriverInfo(t *testing.T) { { Scenario: "ilo5 virtual media HTTP", input: "ilo5-virtualmedia+http://192.168.122.1", - needsMac: true, + needsMac: false, driver: "redfish", bios: "", boot: "redfish-virtual-media", @@ -420,7 +420,7 @@ func TestStaticDriverInfo(t *testing.T) { { Scenario: "ilo5 virtual media HTTPS", input: "ilo5-virtualmedia+https://192.168.122.1", - needsMac: true, + needsMac: false, driver: "redfish", bios: "", boot: "redfish-virtual-media", @@ -430,7 +430,7 @@ func TestStaticDriverInfo(t *testing.T) { { Scenario: "idrac virtual media", input: "idrac-virtualmedia://192.168.122.1", - needsMac: true, + needsMac: false, driver: "idrac", bios: "idrac-redfish", boot: "idrac-redfish-virtual-media", @@ -443,7 +443,7 @@ func TestStaticDriverInfo(t *testing.T) { { Scenario: "idrac virtual media HTTP", input: "idrac-virtualmedia+http://192.168.122.1", - needsMac: true, + needsMac: false, driver: "idrac", bios: "idrac-redfish", boot: "idrac-redfish-virtual-media", @@ -456,7 +456,7 @@ func TestStaticDriverInfo(t *testing.T) { { Scenario: "idrac virtual media HTTPS", input: "idrac-virtualmedia+https://192.168.122.1", - needsMac: true, + needsMac: false, driver: "idrac", bios: "idrac-redfish", boot: "idrac-redfish-virtual-media", diff --git a/pkg/hardwareutils/bmc/idrac_virtualmedia.go b/pkg/hardwareutils/bmc/idrac_virtualmedia.go index 2ad0a0ca1b..838d62fa0f 100644 --- a/pkg/hardwareutils/bmc/idrac_virtualmedia.go +++ b/pkg/hardwareutils/bmc/idrac_virtualmedia.go @@ -34,12 +34,11 @@ func (a *redfishiDracVirtualMediaAccessDetails) Type() string { return a.bmcType } -// NeedsMAC returns true when the host is going to need a separate -// port created rather than having it discovered. +// NeedsMAC returns false for virtual media drivers since they can boot +// from virtual media without requiring a pre-configured boot MAC address. +// The MAC address can be populated after hardware inspection completes. func (a *redfishiDracVirtualMediaAccessDetails) NeedsMAC() bool { - // For the inspection to work, we need a MAC address - // https://github.com/metal3-io/baremetal-operator/pull/284#discussion_r317579040 - return true + return false } func (a *redfishiDracVirtualMediaAccessDetails) DisableCertificateVerification() bool { diff --git a/pkg/hardwareutils/bmc/redfish_virtualmedia.go b/pkg/hardwareutils/bmc/redfish_virtualmedia.go index cd98edff33..1178a1f4e2 100644 --- a/pkg/hardwareutils/bmc/redfish_virtualmedia.go +++ b/pkg/hardwareutils/bmc/redfish_virtualmedia.go @@ -29,12 +29,11 @@ func (a *redfishVirtualMediaAccessDetails) Type() string { return a.bmcType } -// NeedsMAC returns true when the host is going to need a separate -// port created rather than having it discovered. +// NeedsMAC returns false for virtual media drivers since they can boot +// from virtual media without requiring a pre-configured boot MAC address. +// The MAC address can be populated after hardware inspection completes. func (a *redfishVirtualMediaAccessDetails) NeedsMAC() bool { - // For the inspection to work, we need a MAC address - // https://github.com/metal3-io/baremetal-operator/pull/284#discussion_r317579040 - return true + return false } func (a *redfishVirtualMediaAccessDetails) Driver() string { diff --git a/pkg/provisioner/ironic/findhost_test.go b/pkg/provisioner/ironic/findhost_test.go index 16c8c4d369..c5ecf0731d 100644 --- a/pkg/provisioner/ironic/findhost_test.go +++ b/pkg/provisioner/ironic/findhost_test.go @@ -1,6 +1,7 @@ package ironic import ( + "strings" "testing" "github.com/gophercloud/gophercloud/v2/openstack/baremetal/v1/nodes" @@ -79,3 +80,87 @@ func TestFindExistingHost(t *testing.T) { }) } } + +func TestFindExistingHostEmptyMAC(t *testing.T) { + // Test that when bootMACAddress is empty, MAC-based port lookup is skipped + // and no false MAC conflicts are reported. This is important for pre-provisioned + // hardware workflows where the boot MAC will be populated by external controllers. + + cases := []struct { + name string + ironic *testserver.IronicMock + hostName string + provisioningID string + bootMAC string + expectNode bool + expectError bool + }{ + { + name: "empty-mac-no-node", + hostName: "name", + provisioningID: "uuid", + bootMAC: "", + ironic: testserver.NewIronic(t).NoNode("myns" + nameSeparator + "name").NoNode("name").NoNode("uuid"), + expectNode: false, + expectError: false, + }, + { + name: "empty-mac-node-exists-by-name", + hostName: "name", + provisioningID: "uuid", + bootMAC: "", + ironic: testserver.NewIronic(t).NoNode("uuid"). + Node(nodes.Node{ + Name: "myns" + nameSeparator + "name", + UUID: "different-uuid", + }), + expectNode: true, + expectError: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if tc.ironic != nil { + tc.ironic.Start() + defer tc.ironic.Stop() + } + + auth := clients.AuthConfig{Type: clients.NoAuth} + + // Create host with empty bootMACAddress + host := makeHost() + host.ObjectMeta.Name = tc.hostName + host.Status.Provisioning.ID = tc.provisioningID + host.Spec.BootMACAddress = tc.bootMAC + + prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nil, tc.ironic.Endpoint(), auth) + if err != nil { + t.Fatalf("could not create provisioner: %s", err) + } + + node, err := prov.findExistingHost(tc.bootMAC) + + // Verify no port queries were made when MAC is empty + if tc.bootMAC == "" { + if strings.Contains(tc.ironic.Requests, "/v1/ports") { + t.Errorf("unexpected port query when bootMACAddress is empty, requests: %s", tc.ironic.Requests) + } + } + + if tc.expectError && err == nil { + t.Fatalf("expected error but got none") + } + if !tc.expectError && err != nil { + t.Fatalf("unexpected error: %s", err) + } + + if tc.expectNode && node == nil { + t.Fatalf("expected node but got nil") + } + if !tc.expectNode && node != nil { + t.Fatalf("expected no node but got %s (%s)", node.Name, node.UUID) + } + }) + } +} diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go index c914723de3..0703eba039 100644 --- a/pkg/provisioner/ironic/ironic.go +++ b/pkg/provisioner/ironic/ironic.go @@ -261,34 +261,37 @@ func (p *ironicProvisioner) findExistingHost(bootMACAddress string) (ironicNode } // Try to load the node by port address - p.log.Info("looking for existing node by MAC", "MAC", bootMACAddress) - allPorts, err := p.listAllPorts(bootMACAddress) + // Skip MAC-based lookup if bootMACAddress is empty to avoid false conflicts + if bootMACAddress != "" { + p.log.Info("looking for existing node by MAC", "MAC", bootMACAddress) + allPorts, err := p.listAllPorts(bootMACAddress) - if err != nil { - p.log.Info("failed to find an existing port with address", "MAC", bootMACAddress) - return nil, nil //nolint:nilerr,nilnil - } + if err != nil { + p.log.Info("failed to find an existing port with address", "MAC", bootMACAddress) + return nil, nil //nolint:nilerr,nilnil + } - if len(allPorts) > 0 { - nodeUUID := allPorts[0].NodeUUID - ironicNode, err = nodes.Get(p.ctx, p.client, nodeUUID).Extract() - if err == nil { - p.debugLog.Info("found existing node by MAC", "MAC", bootMACAddress, "node", ironicNode.UUID, "name", ironicNode.Name) + if len(allPorts) > 0 { + nodeUUID := allPorts[0].NodeUUID + ironicNode, err = nodes.Get(p.ctx, p.client, nodeUUID).Extract() + if err == nil { + p.debugLog.Info("found existing node by MAC", "MAC", bootMACAddress, "node", ironicNode.UUID, "name", ironicNode.Name) - // If the node has a name, this means we didn't find it above. - if ironicNode.Name != "" { - return nil, NewMacAddressConflictError(bootMACAddress, ironicNode.Name) - } + // If the node has a name, this means we didn't find it above. + if ironicNode.Name != "" { + return nil, NewMacAddressConflictError(bootMACAddress, ironicNode.Name) + } - return ironicNode, nil - } - if gophercloud.ResponseCodeIs(err, http.StatusNotFound) { - return nil, fmt.Errorf("port %s exists but linked node %s doesn't: %w", bootMACAddress, nodeUUID, err) + return ironicNode, nil + } + if gophercloud.ResponseCodeIs(err, http.StatusNotFound) { + return nil, fmt.Errorf("port %s exists but linked node %s doesn't: %w", bootMACAddress, nodeUUID, err) + } + return nil, fmt.Errorf("port %s exists but failed to find linked node %s by ID: %w", bootMACAddress, nodeUUID, err) } - return nil, fmt.Errorf("port %s exists but failed to find linked node %s by ID: %w", bootMACAddress, nodeUUID, err) - } - p.log.Info("port with address doesn't exist", "MAC", bootMACAddress) + p.log.Info("port with address doesn't exist", "MAC", bootMACAddress) + } // Either the node was never created or the Ironic database has // been dropped. return nil, nil //nolint:nilnil