Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

baremetal: validate that macs are EUI-48 and unicast #4378

Merged
merged 1 commit into from Nov 18, 2020

Conversation

stbenjam
Copy link
Member

@stbenjam stbenjam commented Nov 13, 2020

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

Additionally, the built-in ParseMAC in go net accepts EUI-64 and
Infiniband macs. This updates our validator to make sure we have
EUI-48 (6-octet) MAC address as well.

@stbenjam
Copy link
Member Author

/label platform/baremetal

@stbenjam stbenjam force-pushed the multicast-mac branch 4 times, most recently from 4508acf to 83d26b7 Compare November 13, 2020 18:40
@stbenjam
Copy link
Member Author

/retitle baremetal: validate that macs are EUI-48 and unicast

@openshift-ci-robot openshift-ci-robot changed the title baremetal: validate custom mac addresses are unicast baremetal: validate that macs are EUI-48 and unicast Nov 13, 2020
pkg/validate/validate.go Show resolved Hide resolved
pkg/validate/validate.go Outdated Show resolved Hide resolved
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

Additionally, the built-in ParseMAC in go net accepts EUI-64 and
Infiniband macs. This updates our validator to make sure we have
EUI-48 (6-octet) MAC address as well.
@staebler
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann, staebler

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2020
@dhellmann
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

6 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

12 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@dhellmann
Copy link
Contributor

/skip e2e-aws-upgrade
/skip e2e-crc

@stbenjam
Copy link
Member Author

/skip

@stbenjam
Copy link
Member Author

/test e2e-aws-upgrade

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

5 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@stbenjam
Copy link
Member Author

/test e2e-aws

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot
Copy link
Contributor

openshift-merge-robot commented Nov 18, 2020

@stbenjam: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-crc 659db16 link /test e2e-crc

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 289093e into openshift:master Nov 18, 2020
@stbenjam stbenjam deleted the multicast-mac branch November 18, 2020 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. platform/baremetal IPI bare metal hosts platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants