Bug 1867165: baremetal: allow configuring bootstrap mac addresses

data/data/baremetal/bootstrap/main.tf

@@ -40,8 +40,8 @@ resource "libvirt_domain" "bootstrap" {
   dynamic "network_interface" {
     for_each = var.bridges
     content {
-      bridge = network_interface.value
+      bridge = network_interface.value["name"]
+      mac    = network_interface.value["mac"]
     }
   }
 }
-

data/data/baremetal/bootstrap/variables.tf

@@ -14,6 +14,6 @@ variable "ignition" {
 }
 
 variable "bridges" {
-  type        = list(string)
-  description = "A list of network interfaces to attach to the bootstrap virtual machine"
+  type        = list(map(string))
+  description = "A list of network bridge maps, containing the interface name and optionally the MAC address"
 }

data/data/baremetal/main.tf

@@ -20,7 +20,7 @@ module "bootstrap" {
   cluster_id = var.cluster_id
   image      = var.bootstrap_os_image
   ignition   = var.ignition_bootstrap
-  bridges    = compact([var.external_bridge, var.provisioning_bridge])
+  bridges    = var.bridges
 }
 
 module "masters" {

data/data/baremetal/variables-baremetal.tf

@@ -13,16 +13,6 @@ variable "bootstrap_os_image" {
   description = "The URL of the bootstrap OS disk image"
 }
 
-variable "external_bridge" {
-  type        = string
-  description = "The name of the external bridge"
-}
-
-variable "provisioning_bridge" {
-  type        = string
-  description = "The name of the provisioning bridge"
-}
-
 variable "ironic_username" {
   type        = string
   description = "Username for authentication to Ironic"
@@ -38,6 +28,11 @@ variable "hosts" {
   description = "Hardware details for hosts"
 }
 
+variable "bridges" {
+  type        = list(map(string))
+  description = "A list of network bridge maps, containing the interface name and optionally the MAC address"
+}
+
 variable "properties" {
   type        = list(map(string))
   description = "Properties for hosts"

data/data/install.openshift.io_installconfigs.yaml

@@ -851,6 +851,9 @@ spec:
                   externalBridge:
                     description: External bridge is used for external communication.
                     type: string
+                  externalMACAddress:
+                    description: ExternalMACAddress is used to allow setting a static MAC address for the bootstrap host on the external network. If left blank, libvirt will generate one for you.
+                    type: string
                   hosts:
                     description: Hosts is the information needed to create the objects in Ironic.
                     items:
@@ -949,6 +952,9 @@ spec:
                   provisioningHostIP:
                     description: ClusterProvisioningIP is the IP on the dedicated provisioning network where the baremetal-operator pod runs provisioning services, and an http server to cache some downloaded content e.g RHCOS/IPA images
                     type: string
+                  provisioningMACAddress:
+                    description: ProvisioningMACAddress is used to allow setting a static MAC address for the bootstrap host on the provisioning network. If left blank, libvirt will generate one for you.
+                    type: string
                   provisioningNetwork:
                     default: Managed
                     description: ProvisioningNetwork is used to indicate if we will have a provisioning network, and how it will be managed.

docs/user/metal/customization_ipi.md

@@ -10,7 +10,9 @@
 `clusterProvisioningIP` | The third address on the provisioning network. `172.22.0.3` | The IP within the cluster where the provisioning services run. |
 `bootstrapProvisioningIP` | The second address on the provisioning network. `172.22.0.2` | The IP on the bootstrap VM where the provisioning services run while the control plane is being deployed. |
 `externalBridge` | `baremetal` | The name of the bridge of the hypervisor attached to the external network. |
+`externalMACAddress` | `` | A MAC address to use for the external bridge. This is optional and if blank is generated by libvirt. |
 `provisioningBridge` | `provisioning` | The name of the bridge on the hypervisor attached to the provisioning network. |
+`provisioningMACAddress` | `` | A MAC address to use for the provisioning bridge. This is optional and if blank is generated by libvirt. |
 `provisioningNetworkCIDR` | `172.22.0.0/24` | The CIDR for the network to use for provisioning. |
 `provisioningDHCPExternal` | `false` | Flag indicating that DHCP for the provisioning network is managed outside of the cluster by existing infrastructure services. |
 `provisioningDHCPRange` | The tenth through the second last IP on the provisioning network. `172.22.0.10,172.22.0.254` | The IP range to use for hosts on the provisioning network. |

pkg/asset/cluster/tfvars.go

@@ -430,7 +430,9 @@ func (t *TerraformVariables) Generate(parents asset.Parents) error {
 			installConfig.Config.Platform.BareMetal.BootstrapProvisioningIP,
 			string(*rhcosBootstrapImage),
 			installConfig.Config.Platform.BareMetal.ExternalBridge,
+			installConfig.Config.Platform.BareMetal.ExternalMACAddress,
 			installConfig.Config.Platform.BareMetal.ProvisioningBridge,
+			installConfig.Config.Platform.BareMetal.ProvisioningMACAddress,
 			installConfig.Config.Platform.BareMetal.Hosts,
 			string(*rhcosImage),
 			ironicCreds.Username,

pkg/tfvars/baremetal/baremetal.go

@@ -17,11 +17,10 @@ import (
 )
 
 type config struct {
-	LibvirtURI              string `json:"libvirt_uri"`
-	BootstrapProvisioningIP string `json:"bootstrap_provisioning_ip"`
-	BootstrapOSImage        string `json:"bootstrap_os_image"`
-	ExternalBridge          string `json:"external_bridge"`
-	ProvisioningBridge      string `json:"provisioning_bridge"`
+	LibvirtURI              string              `json:"libvirt_uri,omitempty"`
+	BootstrapProvisioningIP string              `json:"bootstrap_provisioning_ip,omitempty"`
+	BootstrapOSImage        string              `json:"bootstrap_os_image,omitempty"`
+	Bridges                 []map[string]string `json:"bridges"`
 
 	IronicUsername string `json:"ironic_username"`
 	IronicPassword string `json:"ironic_password"`
@@ -35,7 +34,7 @@ type config struct {
 }
 
 // TFVars generates bare metal specific Terraform variables.
-func TFVars(libvirtURI, bootstrapProvisioningIP, bootstrapOSImage, externalBridge, provisioningBridge string, platformHosts []*baremetal.Host, image, ironicUsername, ironicPassword string) ([]byte, error) {
+func TFVars(libvirtURI, bootstrapProvisioningIP, bootstrapOSImage, externalBridge, externalMAC, provisioningBridge, provisioningMAC string, platformHosts []*baremetal.Host, image, ironicUsername, ironicPassword string) ([]byte, error) {
 	bootstrapOSImage, err := cache.DownloadImageFile(bootstrapOSImage)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to use cached bootstrap libvirt image")
@@ -140,15 +139,31 @@ func TFVars(libvirtURI, bootstrapProvisioningIP, bootstrapOSImage, externalBridg
 		instanceInfos = append(instanceInfos, instanceInfo)
 	}
 
+	var bridges []map[string]string
+
+	bridges = append(bridges,
+		map[string]string{
+			"name": externalBridge,
+			"mac":  externalMAC,
+		})
+
+	if provisioningBridge != "" {
+		bridges = append(
+			bridges,
+			map[string]string{
+				"name": provisioningBridge,
+				"mac":  provisioningMAC,
+			})
+	}
+
 	cfg := &config{
 		LibvirtURI:              libvirtURI,
 		BootstrapProvisioningIP: bootstrapProvisioningIP,
 		BootstrapOSImage:        bootstrapOSImage,
-		ExternalBridge:          externalBridge,
-		ProvisioningBridge:      provisioningBridge,
 		IronicUsername:          ironicUsername,
 		IronicPassword:          ironicPassword,
 		Hosts:                   hosts,
+		Bridges:                 bridges,
 		Properties:              properties,
 		DriverInfos:             driverInfos,
 		RootDevices:             rootDevices,

pkg/types/baremetal/platform.go

@@ -83,6 +83,12 @@ type Platform struct {
 	// +optional
 	ExternalBridge string `json:"externalBridge,omitempty"`
 
+	// ExternalMACAddress is used to allow setting a static MAC address for
+	// the bootstrap host on the external network. If left blank, libvirt will
+	// generate one for you.
+	// +optional
+	ExternalMACAddress string `json:"externalMACAddress,omitempty"`
+
 	// ProvisioningNetwork is used to indicate if we will have a provisioning network, and how it will be managed.
 	// +kubebuilder:default=Managed
 	// +optional
@@ -93,6 +99,12 @@ type Platform struct {
 	// +optional
 	ProvisioningBridge string `json:"provisioningBridge,omitempty"`
 
+	// ProvisioningMACAddress is used to allow setting a static MAC address for
+	// the bootstrap host on the provisioning network. If left blank, libvirt will
+	// generate one for you.
+	// +optional
+	ProvisioningMACAddress string `json:"provisioningMACAddress,omitempty"`
+
 	// ProvisioningNetworkInterface is the name of the network interface on a control plane
 	// baremetal host that is connected to the provisioning network.
 	ProvisioningNetworkInterface string `json:"provisioningNetworkInterface"`

pkg/types/baremetal/validation/libvirt.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"github.com/libvirt/libvirt-go"
 	"github.com/openshift/installer/pkg/types/baremetal"
+	"github.com/openshift/installer/pkg/validate"
 	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"strings"
@@ -29,15 +30,28 @@ func validateInterfaces(p *baremetal.Platform, fldPath *field.Path) field.ErrorL
 		errorList = append(errorList, field.Invalid(fldPath.Child("externalBridge"), p.ExternalBridge, err.Error()))
 	}
 
+	if err := validate.MAC(p.ExternalMACAddress); p.ExternalMACAddress != "" && err != nil {
+		errorList = append(errorList, field.Invalid(fldPath.Child("externalMACAddress"), p.ExternalMACAddress, err.Error()))
+	}
+
 	if err := findInterface(p.ProvisioningBridge); p.ProvisioningNetwork != baremetal.DisabledProvisioningNetwork && err != nil {
 		errorList = append(errorList, field.Invalid(fldPath.Child("provisioningBridge"), p.ProvisioningBridge, err.Error()))
 	}
 
+	if err := validate.MAC(p.ProvisioningMACAddress); p.ProvisioningMACAddress != "" && err != nil {
+		errorList = append(errorList, field.Invalid(fldPath.Child("provisioningMACAddress"), p.ProvisioningMACAddress, err.Error()))
+	}
+
+	if p.ProvisioningMACAddress != "" && strings.EqualFold(p.ProvisioningMACAddress, p.ExternalMACAddress) {
+		errorList = append(errorList, field.Duplicate(fldPath.Child("provisioningMACAddress"), "provisioning and external MAC addresses may not be identical"))
+	}
+
 	return errorList
 }
 
 // interfaceValidator fetches the valid interface names from a particular libvirt instance, and returns a closure
 // to validate if an interface is found among them
+
 func interfaceValidator(libvirtURI string) (func(string) error, error) {
 	// Connect to libvirt and obtain a list of interface names
 	interfaces := make(map[string]struct{})

pkg/types/baremetal/validation/platform.go

@@ -387,6 +387,18 @@ func ValidateProvisioning(p *baremetal.Platform, n *types.Networking, fldPath *f
 		if p.ProvisioningNetworkInterface == "" {
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("provisioningNetworkInterface"), p.ProvisioningNetworkInterface, "no provisioning network interface is configured, please set this value to be the interface on the provisioning network on your cluster's baremetal hosts"))
 		}
+
+		if err := validate.MAC(p.ExternalMACAddress); p.ExternalMACAddress != "" && err != nil {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("externalMACAddress"), p.ExternalMACAddress, err.Error()))
+		}
+
+		if err := validate.MAC(p.ProvisioningMACAddress); p.ProvisioningMACAddress != "" && err != nil {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("provisioningMACAddress"), p.ProvisioningMACAddress, err.Error()))
+		}
+
+		if p.ProvisioningMACAddress != "" && strings.EqualFold(p.ProvisioningMACAddress, p.ExternalMACAddress) {
+			allErrs = append(allErrs, field.Duplicate(fldPath.Child("provisioningMACAddress"), "provisioning and external MAC addresses may not be identical"))
+		}
 	}
 
 	if err := validate.URI(p.LibvirtURI); err != nil {

pkg/types/baremetal/validation/platform_test.go

@@ -280,12 +280,35 @@ func TestValidateProvisioning(t *testing.T) {
 				ExternalBridge("noexist").build(),
 			expected: "Invalid value: \"noexist\": invalid external bridge",
 		},
+		{
+			name:     "valid_extbridge_mac",
+			platform: platform().ExternalMACAddress("CA:FE:CA:FE:CA:FE").build(),
+		},
 		{
 			name: "invalid_provbridge",
 			platform: platform().
 				ProvisioningBridge("noexist").build(),
 			expected: "Invalid value: \"noexist\": invalid provisioning bridge",
 		},
+		{
+			name:     "valid_provbridge_mac",
+			platform: platform().ProvisioningMACAddress("CA:FE:CA:FE:CA:FE").build(),
+		},
+		{
+			name: "invalid_duplicate_bridge_macs",
+			platform: platform().
+				ProvisioningMACAddress("CA:FE:CA:FE:CA:FE").
+				ExternalMACAddress("CA:FE:CA:FE:CA:FE").
+				build(),
+			expected: "Duplicate value: \"provisioning and external MAC addresses may not be identical\"",
+		},
+		{
+			name: "valid_both_macs_specified",
+			platform: platform().
+				ProvisioningMACAddress("CA:FE:CA:FE:CA:FD").
+				ExternalMACAddress("CA:FE:CA:FE:CA:FE").
+				build(),
+		},
 		{
 			name: "invalid_bootstrapprovip_wrongCIDR",
 			platform: platform().
@@ -568,11 +591,21 @@ func (pb *platformBuilder) ExternalBridge(value string) *platformBuilder {
 	return pb
 }
 
+func (pb *platformBuilder) ExternalMACAddress(value string) *platformBuilder {
+	pb.Platform.ExternalMACAddress = value
+	return pb
+}
+
 func (pb *platformBuilder) ProvisioningBridge(value string) *platformBuilder {
 	pb.Platform.ProvisioningBridge = value
 	return pb
 }
 
+func (pb *platformBuilder) ProvisioningMACAddress(value string) *platformBuilder {
+	pb.Platform.ProvisioningMACAddress = value
+	return pb
+}
+
 func (pb *platformBuilder) ProvisioningNetworkInterface(value string) *platformBuilder {
 	pb.Platform.ProvisioningNetworkInterface = value
 	return pb