Skip to content

Commit

Permalink
baremetal: validate custom mac addresses are unicast
Browse files Browse the repository at this point in the history
When users specify a custom MAC address for the provisioning and
baremetal bridges of the libvirt bootstrap host, libvirt complains
if the user does not give a unicast MAC address:

```
020-11-13 10:52:18 level=debug msg=2020/11/13 10:52:18 [ERROR]
module.bootstrap: eval: *terraform.EvalApplyPost, err: Error defining
libvirt domain: virError(Code=27, Domain=20, Message='XML error:
expected unicast mac address, found multicast '7D:CE:E3:29:35:6F'')
')
```

This adds a function that validates whether a MAC address is unicast. A
multicast MAC address is one where the most significant byte has a least
significant bit of `1`.

      Example 1: Multicast MAC
      ------------------------
      7D:CE:E3:29:35:6F
       ^--> most significant byte

      0x7D is 0b11111101
                       ^--> this is a multicast MAC

      Example 2: Unicast MAC
      ----------------------
      7A:CE:E3:29:35:6F
       ^--> most significant byte

      0x7A is 0b11111010
                       ^--> this is a unicast MAC
  • Loading branch information
stbenjam committed Nov 13, 2020
1 parent 9be316b commit 4508acf
Show file tree
Hide file tree
Showing 6 changed files with 106 additions and 8 deletions.
4 changes: 2 additions & 2 deletions data/data/install.openshift.io_installconfigs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -852,7 +852,7 @@ spec:
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.
description: ExternalMACAddress is used to allow setting a static unicast MAC address for the bootstrap host on the external network. Consider using the QEMU vendor prefix `52:54:00`. If left blank, libvirt will generate one for you.
type: string
hosts:
description: Hosts is the information needed to create the objects in Ironic.
Expand Down Expand Up @@ -953,7 +953,7 @@ spec:
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.
description: ProvisioningMACAddress is used to allow setting a static unicast MAC address for the bootstrap host on the provisioning network. Consider using the QEMU vendor prefix `52:54:00`. If left blank, libvirt will generate one for you.
type: string
provisioningNetwork:
default: Managed
Expand Down
10 changes: 6 additions & 4 deletions pkg/types/baremetal/platform.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,9 @@ 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
// ExternalMACAddress is used to allow setting a static unicast MAC
// address for the bootstrap host on the external network. Consider
// using the QEMU vendor prefix `52:54:00`. If left blank, libvirt will
// generate one for you.
// +optional
ExternalMACAddress string `json:"externalMACAddress,omitempty"`
Expand All @@ -99,8 +100,9 @@ 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
// ProvisioningMACAddress is used to allow setting a static unicast MAC
// address for the bootstrap host on the provisioning network. Consider
// using the QEMU vendor prefix `52:54:00`. If left blank, libvirt will
// generate one for you.
// +optional
ProvisioningMACAddress string `json:"provisioningMACAddress,omitempty"`
Expand Down
4 changes: 2 additions & 2 deletions pkg/types/baremetal/validation/platform.go
Original file line number Diff line number Diff line change
Expand Up @@ -388,11 +388,11 @@ func ValidateProvisioning(p *baremetal.Platform, n *types.Networking, fldPath *f
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 {
if err := validate.UnicastMAC(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 {
if err := validate.UnicastMAC(p.ProvisioningMACAddress); p.ProvisioningMACAddress != "" && err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("provisioningMACAddress"), p.ProvisioningMACAddress, err.Error()))
}

Expand Down
7 changes: 7 additions & 0 deletions pkg/types/baremetal/validation/platform_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,13 @@ func TestValidateProvisioning(t *testing.T) {
ExternalMACAddress("CA:FE:CA:FE:CA:FE").
build(),
},
{
name: "invalid_multicast_mac",
platform: platform().
ExternalMACAddress("7D:CE:E3:29:35:6F").
build(),
expected: "expected unicast mac address, found multicast",
},
{
name: "invalid_bootstrapprovip_wrongCIDR",
platform: platform().
Expand Down
28 changes: 28 additions & 0 deletions pkg/validate/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,34 @@ func MAC(addr string) error {
return err
}

// UnicastMAC validates that a value is a valid unicast mac address. A multicast MAC
// address is an address where the least significant bit of the most significant byte is 1.
//
// Example 1: Multicast MAC
// ------------------------
// 7D:CE:E3:29:35:6F
// ^--> most significant byte
//
// 0x7D is 0b11111101
// ^--> this is a multicast MAC
//
// Example 2: Unicast MAC
// ----------------------
// 7A:CE:E3:29:35:6F
// ^--> most significant byte
//
// 0x7A is 0b11111010
// ^--> this is a unicast MAC
func UnicastMAC(addr string) error {
hwAddr, err := net.ParseMAC(addr)

if err == nil && hwAddr[0]&1 == 1 {
return fmt.Errorf("expected unicast mac address, found multicast: %q", addr)
}

return err
}

// UUID validates that a uuid is non-empty and a valid uuid.
func UUID(val string) error {
_, err := uuid.Parse(val)
Expand Down
61 changes: 61 additions & 0 deletions pkg/validate/validate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -563,3 +563,64 @@ func TestURI(t *testing.T) {
})
}
}

func TestMAC(t *testing.T) {
cases := []struct {
name string
addr string
expected string
}{
{
name: "valid_mac",
addr: "7A:CE:E3:29:35:6F",
},
{
name: "invalid_mac",
addr: "this is a bad mac",
expected: "invalid MAC address",
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
err := MAC(tc.addr)
if tc.expected == "" {
assert.NoError(t, err)
} else {
assert.Regexp(t, tc.expected, err)
}
})
}
}

func TestUnicastMAC(t *testing.T) {
cases := []struct {
name string
addr string
expected string
}{
{
name: "valid_unicast",
addr: "7A:CE:E3:29:35:6F",
},
{
name: "invalid_multicast",
addr: "7D:CE:E3:29:35:6F",
expected: "expected unicast mac address",
},
{
name: "invalid_mac",
addr: "this is a bad mac",
expected: "invalid MAC address",
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
err := UnicastMAC(tc.addr)
if tc.expected == "" {
assert.NoError(t, err)
} else {
assert.Regexp(t, tc.expected, err)
}
})
}
}

0 comments on commit 4508acf

Please sign in to comment.