Skip to content

Commit dcd4003

Browse files
donpenneydonpenney
committed
Allow BMH registration without bootMACAddress when preprovisioningNetworkDataName is set
This change enables workflows where BareMetalHosts are created with pre-provisioned network configuration but without a pre-set bootMACAddress. In these workflows, the boot interface is identified via interface labels and the bootMACAddress is populated by external controllers after hardware inspection completes, based on the discovered NIC details and the interface labels. The implementation addresses three key areas: 1. Webhook validation: Allow empty bootMACAddress when PreprovisioningNetworkDataName is set, since the boot MAC will be populated later by external controllers based on network configuration. 2. Registration validation: Skip the bootMACAddress requirement check when PreprovisioningNetworkDataName is set. Add HasPreprovisioningNetworkDataName field to ManagementAccessData to distinguish between "reference exists but is empty" and "no reference exists". 3. Ironic node lookup: Skip MAC-based port queries when bootMACAddress is empty to prevent false MAC address conflict errors during registration. Test Coverage: - internal/webhooks/metal3.io/v1alpha1/baremetalhost_webhook_test.go: Added tests for webhook validation with and without PreprovisioningNetworkDataName when bootMACAddress is empty. - internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go: Added test case verifying bootMACAddress is not required when PreprovisioningNetworkDataName is set. - pkg/provisioner/ironic/register_test.go: Added TestRegisterNoMACWithPreprovisioningNetworkData to verify registration succeeds without bootMACAddress when HasPreprovisioningNetworkDataName is true. - pkg/provisioner/ironic/findhost_test.go: Added TestFindExistingHostEmptyMAC to verify no port queries are made to Ironic when bootMACAddress is empty, preventing false MAC conflicts while still allowing node lookup by name. Assisted-By: Claude <[email protected]> Signed-off-by: Don Penney <[email protected]>
1 parent 230a562 commit dcd4003

File tree

9 files changed

+232
-44
lines changed

9 files changed

+232
-44
lines changed

‎internal/controller/metal3.io/baremetalhost_controller.go‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -867,16 +867,17 @@ func (r *BareMetalHostReconciler) registerHost(prov provisioner.Provisioner, inf
867867

868868
provResult, provID, err := prov.Register(
869869
provisioner.ManagementAccessData{
870-
BootMode: info.host.Status.Provisioning.BootMode,
871-
AutomatedCleaningMode: info.host.Spec.AutomatedCleaningMode,
872-
State: info.host.Status.Provisioning.State,
873-
OperationalStatus: info.host.Status.OperationalStatus,
874-
CurrentImage: getCurrentImage(info.host),
875-
PreprovisioningImage: preprovImg,
876-
PreprovisioningNetworkData: preprovisioningNetworkData,
877-
HasCustomDeploy: hasCustomDeploy(info.host),
878-
DisablePowerOff: info.host.Spec.DisablePowerOff,
879-
CPUArchitecture: getHostArchitecture(info.host),
870+
BootMode: info.host.Status.Provisioning.BootMode,
871+
AutomatedCleaningMode: info.host.Spec.AutomatedCleaningMode,
872+
State: info.host.Status.Provisioning.State,
873+
OperationalStatus: info.host.Status.OperationalStatus,
874+
CurrentImage: getCurrentImage(info.host),
875+
PreprovisioningImage: preprovImg,
876+
PreprovisioningNetworkData: preprovisioningNetworkData,
877+
HasPreprovisioningNetworkDataName: info.host.Spec.PreprovisioningNetworkDataName != "",
878+
HasCustomDeploy: hasCustomDeploy(info.host),
879+
DisablePowerOff: info.host.Spec.DisablePowerOff,
880+
CPUArchitecture: getHostArchitecture(info.host),
880881
},
881882
credsChanged,
882883
info.host.Status.ErrorType == metal3api.RegistrationError)

‎internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,12 @@ func validateBMCAccess(s metal3api.BareMetalHostSpec, bmcAccess bmc.AccessDetail
140140
}
141141

142142
if bmcAccess.NeedsMAC() && s.BootMACAddress == "" {
143-
errs = append(errs, fmt.Errorf("BMC driver %s requires a BootMACAddress value", bmcAccess.Type()))
143+
// Allow unset bootMACAddress when using pre-provisioned network configuration.
144+
// In this case, the bootMACAddress will be populated by external controllers
145+
// based on the network configuration (e.g., from interface labels).
146+
if s.PreprovisioningNetworkDataName == "" {
147+
errs = append(errs, fmt.Errorf("BMC driver %s requires a BootMACAddress value", bmcAccess.Type()))
148+
}
144149
}
145150

146151
if s.BootMACAddress != "" {

‎internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,21 @@ func TestValidateCreate(t *testing.T) {
270270
oldBMH: nil,
271271
wantedErr: "",
272272
},
273+
{
274+
name: "BootMACAddressNotRequiredWithPreprovisioningNetworkData",
275+
newBMH: &metal3api.BareMetalHost{
276+
TypeMeta: tm,
277+
ObjectMeta: om,
278+
Spec: metal3api.BareMetalHostSpec{
279+
BMC: metal3api.BMCDetails{
280+
Address: "idrac-virtualmedia+https://192.168.1.1/redfish/v1/Systems/System.Embedded.1",
281+
CredentialsName: "test1",
282+
},
283+
PreprovisioningNetworkDataName: "network-data-test",
284+
}},
285+
oldBMH: nil,
286+
wantedErr: "",
287+
},
273288
{
274289
name: "BootMACAddressRequired",
275290
newBMH: &metal3api.BareMetalHost{

‎internal/webhooks/metal3.io/v1alpha1/baremetalhost_webhook_test.go‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,39 @@ func TestBareMetalHostCreate(t *testing.T) {
5252
}, Spec: metal3api.BareMetalHostSpec{}},
5353
wantedErr: "",
5454
},
55+
{
56+
name: "valid-no-bootMACAddress-with-preprovisioningNetworkData",
57+
bmh: &metal3api.BareMetalHost{TypeMeta: metav1.TypeMeta{
58+
Kind: "BareMetalHost",
59+
APIVersion: "metal3.io/v1alpha1",
60+
}, ObjectMeta: metav1.ObjectMeta{
61+
Name: "test",
62+
Namespace: "test-namespace",
63+
}, Spec: metal3api.BareMetalHostSpec{
64+
BMC: metal3api.BMCDetails{
65+
Address: "libvirt://192.168.122.1:16509/",
66+
},
67+
BootMACAddress: "",
68+
PreprovisioningNetworkDataName: "network-data-test",
69+
}},
70+
wantedErr: "",
71+
},
72+
{
73+
name: "invalid-no-bootMACAddress-no-preprovisioningNetworkData",
74+
bmh: &metal3api.BareMetalHost{TypeMeta: metav1.TypeMeta{
75+
Kind: "BareMetalHost",
76+
APIVersion: "metal3.io/v1alpha1",
77+
}, ObjectMeta: metav1.ObjectMeta{
78+
Name: "test",
79+
Namespace: "test-namespace",
80+
}, Spec: metal3api.BareMetalHostSpec{
81+
BMC: metal3api.BMCDetails{
82+
Address: "libvirt://192.168.122.1:16509/",
83+
},
84+
BootMACAddress: "",
85+
}},
86+
wantedErr: "BMC driver libvirt requires a BootMACAddress value",
87+
},
5588
}
5689

5790
for _, tt := range tests {

‎pkg/provisioner/ironic/findhost_test.go‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package ironic
22

33
import (
4+
"strings"
45
"testing"
56

67
"github.com/gophercloud/gophercloud/v2/openstack/baremetal/v1/nodes"
@@ -79,3 +80,87 @@ func TestFindExistingHost(t *testing.T) {
7980
})
8081
}
8182
}
83+
84+
func TestFindExistingHostEmptyMAC(t *testing.T) {
85+
// Test that when bootMACAddress is empty, MAC-based port lookup is skipped
86+
// and no false MAC conflicts are reported. This is important for pre-provisioned
87+
// hardware workflows where the boot MAC will be populated by external controllers.
88+
89+
cases := []struct {
90+
name string
91+
ironic *testserver.IronicMock
92+
hostName string
93+
provisioningID string
94+
bootMAC string
95+
expectNode bool
96+
expectError bool
97+
}{
98+
{
99+
name: "empty-mac-no-node",
100+
hostName: "name",
101+
provisioningID: "uuid",
102+
bootMAC: "",
103+
ironic: testserver.NewIronic(t).NoNode("myns" + nameSeparator + "name").NoNode("name").NoNode("uuid"),
104+
expectNode: false,
105+
expectError: false,
106+
},
107+
{
108+
name: "empty-mac-node-exists-by-name",
109+
hostName: "name",
110+
provisioningID: "uuid",
111+
bootMAC: "",
112+
ironic: testserver.NewIronic(t).NoNode("uuid").
113+
Node(nodes.Node{
114+
Name: "myns" + nameSeparator + "name",
115+
UUID: "different-uuid",
116+
}),
117+
expectNode: true,
118+
expectError: false,
119+
},
120+
}
121+
122+
for _, tc := range cases {
123+
t.Run(tc.name, func(t *testing.T) {
124+
if tc.ironic != nil {
125+
tc.ironic.Start()
126+
defer tc.ironic.Stop()
127+
}
128+
129+
auth := clients.AuthConfig{Type: clients.NoAuth}
130+
131+
// Create host with empty bootMACAddress
132+
host := makeHost()
133+
host.ObjectMeta.Name = tc.hostName
134+
host.Status.Provisioning.ID = tc.provisioningID
135+
host.Spec.BootMACAddress = tc.bootMAC
136+
137+
prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nil, tc.ironic.Endpoint(), auth)
138+
if err != nil {
139+
t.Fatalf("could not create provisioner: %s", err)
140+
}
141+
142+
node, err := prov.findExistingHost(tc.bootMAC)
143+
144+
// Verify no port queries were made when MAC is empty
145+
if tc.bootMAC == "" {
146+
if strings.Contains(tc.ironic.Requests, "/v1/ports") {
147+
t.Errorf("unexpected port query when bootMACAddress is empty, requests: %s", tc.ironic.Requests)
148+
}
149+
}
150+
151+
if tc.expectError && err == nil {
152+
t.Fatalf("expected error but got none")
153+
}
154+
if !tc.expectError && err != nil {
155+
t.Fatalf("unexpected error: %s", err)
156+
}
157+
158+
if tc.expectNode && node == nil {
159+
t.Fatalf("expected node but got nil")
160+
}
161+
if !tc.expectNode && node != nil {
162+
t.Fatalf("expected no node but got %s (%s)", node.Name, node.UUID)
163+
}
164+
})
165+
}
166+
}

‎pkg/provisioner/ironic/ironic.go‎

Lines changed: 25 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -261,34 +261,37 @@ func (p *ironicProvisioner) findExistingHost(bootMACAddress string) (ironicNode
261261
}
262262

263263
// Try to load the node by port address
264-
p.log.Info("looking for existing node by MAC", "MAC", bootMACAddress)
265-
allPorts, err := p.listAllPorts(bootMACAddress)
264+
// Skip MAC-based lookup if bootMACAddress is empty to avoid false conflicts
265+
if bootMACAddress != "" {
266+
p.log.Info("looking for existing node by MAC", "MAC", bootMACAddress)
267+
allPorts, err := p.listAllPorts(bootMACAddress)
266268

267-
if err != nil {
268-
p.log.Info("failed to find an existing port with address", "MAC", bootMACAddress)
269-
return nil, nil //nolint:nilerr,nilnil
270-
}
269+
if err != nil {
270+
p.log.Info("failed to find an existing port with address", "MAC", bootMACAddress)
271+
return nil, nil //nolint:nilerr,nilnil
272+
}
271273

272-
if len(allPorts) > 0 {
273-
nodeUUID := allPorts[0].NodeUUID
274-
ironicNode, err = nodes.Get(p.ctx, p.client, nodeUUID).Extract()
275-
if err == nil {
276-
p.debugLog.Info("found existing node by MAC", "MAC", bootMACAddress, "node", ironicNode.UUID, "name", ironicNode.Name)
274+
if len(allPorts) > 0 {
275+
nodeUUID := allPorts[0].NodeUUID
276+
ironicNode, err = nodes.Get(p.ctx, p.client, nodeUUID).Extract()
277+
if err == nil {
278+
p.debugLog.Info("found existing node by MAC", "MAC", bootMACAddress, "node", ironicNode.UUID, "name", ironicNode.Name)
277279

278-
// If the node has a name, this means we didn't find it above.
279-
if ironicNode.Name != "" {
280-
return nil, NewMacAddressConflictError(bootMACAddress, ironicNode.Name)
281-
}
280+
// If the node has a name, this means we didn't find it above.
281+
if ironicNode.Name != "" {
282+
return nil, NewMacAddressConflictError(bootMACAddress, ironicNode.Name)
283+
}
282284

283-
return ironicNode, nil
284-
}
285-
if gophercloud.ResponseCodeIs(err, http.StatusNotFound) {
286-
return nil, fmt.Errorf("port %s exists but linked node %s doesn't: %w", bootMACAddress, nodeUUID, err)
285+
return ironicNode, nil
286+
}
287+
if gophercloud.ResponseCodeIs(err, http.StatusNotFound) {
288+
return nil, fmt.Errorf("port %s exists but linked node %s doesn't: %w", bootMACAddress, nodeUUID, err)
289+
}
290+
return nil, fmt.Errorf("port %s exists but failed to find linked node %s by ID: %w", bootMACAddress, nodeUUID, err)
287291
}
288-
return nil, fmt.Errorf("port %s exists but failed to find linked node %s by ID: %w", bootMACAddress, nodeUUID, err)
289-
}
290292

291-
p.log.Info("port with address doesn't exist", "MAC", bootMACAddress)
293+
p.log.Info("port with address doesn't exist", "MAC", bootMACAddress)
294+
}
292295
// Either the node was never created or the Ironic database has
293296
// been dropped.
294297
return nil, nil //nolint:nilnil

‎pkg/provisioner/ironic/register.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,10 @@ func (p *ironicProvisioner) Register(data provisioner.ManagementAccessData, cred
9393

9494
// Some BMC types require a MAC address, so ensure we have one
9595
// when we need it. If not, place the host in an error state.
96-
if bmcAccess.NeedsMAC() && p.bootMACAddress == "" {
96+
// Allow unset bootMACAddress when using pre-provisioned network configuration.
97+
// In this case, the bootMACAddress will be populated by external controllers
98+
// based on the network configuration (e.g., from interface labels).
99+
if bmcAccess.NeedsMAC() && p.bootMACAddress == "" && !data.HasPreprovisioningNetworkDataName {
97100
msg := fmt.Sprintf("BMC driver %s requires a BootMACAddress value", bmcAccess.Type())
98101
p.log.Info(msg)
99102
result, err = operationFailed(msg)

‎pkg/provisioner/ironic/register_test.go‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,48 @@ func TestRegisterNoMAC(t *testing.T) {
4141
assert.Contains(t, result.ErrorMessage, "requires a BootMACAddress")
4242
}
4343

44+
func TestRegisterNoMACWithPreprovisioningNetworkData(t *testing.T) {
45+
// Create a host without a bootMACAddress, with a BMC that requires one,
46+
// but with PreprovisioningNetworkData set (which allows empty bootMACAddress).
47+
host := makeHost()
48+
host.Spec.BMC.Address = "test-needs-mac://"
49+
host.Spec.BootMACAddress = ""
50+
host.Spec.PreprovisioningNetworkDataName = "network-data-test"
51+
host.Status.Provisioning.ID = "" // so we don't lookup by uuid
52+
53+
var createdNode *nodes.Node
54+
55+
createCallback := func(node nodes.Node) {
56+
createdNode = &node
57+
}
58+
59+
// Set up ironic server to create a new node
60+
ironic := testserver.NewIronic(t).WithDrivers().CreateNodes(createCallback).NoNode(host.Namespace + nameSeparator + host.Name).NoNode(host.Name)
61+
ironic.NodeUpdate(nodes.Node{
62+
UUID: "node-0",
63+
})
64+
ironic.Start()
65+
defer ironic.Stop()
66+
67+
auth := clients.AuthConfig{Type: clients.NoAuth}
68+
prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nullEventPublisher, ironic.Endpoint(), auth)
69+
if err != nil {
70+
t.Fatalf("could not create provisioner: %s", err)
71+
}
72+
73+
result, provID, err := prov.Register(provisioner.ManagementAccessData{
74+
PreprovisioningNetworkData: `{"networks": []}`,
75+
HasPreprovisioningNetworkDataName: true,
76+
}, false, false)
77+
if err != nil {
78+
t.Fatalf("error from Register: %s", err)
79+
}
80+
// Should not have an error about bootMACAddress when PreprovisioningNetworkData is set
81+
assert.Empty(t, result.ErrorMessage)
82+
assert.NotEmpty(t, createdNode.UUID)
83+
assert.Equal(t, createdNode.UUID, provID)
84+
}
85+
4486
func TestRegisterMACOptional(t *testing.T) {
4587
// Create a host without a bootMACAddress and with a BMC that
4688
// does not require one.

‎pkg/provisioner/provisioner.go‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -74,16 +74,17 @@ type PreprovisioningImage struct {
7474
}
7575

7676
type ManagementAccessData struct {
77-
BootMode metal3api.BootMode
78-
AutomatedCleaningMode metal3api.AutomatedCleaningMode
79-
State metal3api.ProvisioningState
80-
OperationalStatus metal3api.OperationalStatus
81-
CurrentImage *metal3api.Image
82-
PreprovisioningImage *PreprovisioningImage
83-
PreprovisioningNetworkData string
84-
HasCustomDeploy bool
85-
DisablePowerOff bool
86-
CPUArchitecture string
77+
BootMode metal3api.BootMode
78+
AutomatedCleaningMode metal3api.AutomatedCleaningMode
79+
State metal3api.ProvisioningState
80+
OperationalStatus metal3api.OperationalStatus
81+
CurrentImage *metal3api.Image
82+
PreprovisioningImage *PreprovisioningImage
83+
PreprovisioningNetworkData string
84+
HasPreprovisioningNetworkDataName bool
85+
HasCustomDeploy bool
86+
DisablePowerOff bool
87+
CPUArchitecture string
8788
}
8889

8990
type AdoptData struct {

0 commit comments

Comments
 (0)