Store MAC as lowercase #279
Conversation
detiber
force-pushed
the
lowercaseMAC
branch
from
6778cf6 to
e124b4b
Compare
Codecov Report
@@ Coverage Diff @@
## master #279 +/- ##
==========================================
+ Coverage 24.01% 24.24% +0.22%
==========================================
Files 15 15
Lines 1345 1353 +8
==========================================
+ Hits 323 328 +5
- Misses 1003 1006 +3
Partials 19 19
Continue to review full report at Codecov.
|
detiber
force-pushed
the
lowercaseMAC
branch
from
e124b4b to
a345d58
Compare
| if data != "" { | ||
| logger.With("MAC", interfaces[i].GetDhcp().GetMac()).Info(duplicateMAC) | ||
| for _, iface := range hw.GetNetwork().GetInterfaces() { | ||
| mac := iface.GetDhcp().GetMac() |
There was a problem hiding this comment.
I'm wondering if it would make sense to add a test for the empty string here prior to calling s.db.GetByMAC().
Would we expect that to be an error condition or would we allow a hardware definition without a mac address?
There was a problem hiding this comment.
I was questioning similar, whether we should transform the input at all, or just let validate/push throw an error if the hardware mac was not "valid" (lowercase, non-empty).
That said, I don't know if hardware addresses can be treated as optional.
There was a problem hiding this comment.
I worry that transforming the data means that any stateful tools (terraform, k8s controllers, salt, etc) that read back the lowercase value will need to be aware of the transform and copy that logic themselves.
There was a problem hiding this comment.
The alternative I thought of was to make the comparisons case-insensitive, but with it being stored embedded inside of a stored json blob that seemed a bit more error prone.
|
|
||
| func normalizeHardwareData(hw *hardware.Hardware) { | ||
| // Ensure MAC is stored as lowercase | ||
| for _, iface := range hw.GetNetwork().GetInterfaces() { |
There was a problem hiding this comment.
I switched things to use the generated proto getters rather than nil checks, since they already include the nil checks and it also eliminates the use of slice indexing.
detiber
force-pushed
the
lowercaseMAC
branch
from
a345d58 to
2b62973
Compare
Signed-off-by: Jason DeTiberus <[email protected]>
detiber
force-pushed
the
lowercaseMAC
branch
from
2b62973 to
9a80ff7
Compare
Description
Always stores any MAC addresses provided as lowercase to avoid issues with comparisons later on.
Why is this needed
boots is attempting to compare pxe booted systems using a lowercased MAC address, so if the user inputs the MAC addresses that are capitalized they will not match.
Fixes: #278
How Has This Been Tested?
Still in progress, will be testing against the Vagrant getting started guide, and manually changing the MAC used to be uppercased.
How are existing users impacted? What migration steps/scripts do we need?
This should not effect existing users (at least ones that have functioning environments), since existing hardware definitions using upper cased MAC addresses would be failing already.
Checklist:
I have: