pkg/ipam: IP based macaddr #254
Conversation
|
@steveej I'm a bit nervous about hardcoding this behavior in ConfigureInterface(). A plugin might create its own veth pair and then use ConfigureInterface() to do the actual IPAM setup, but may want to preserve the MAC it already set when the veth was created. Is there a way to make this behavior optional, or only performed if the 'ptp' plugin is used? Maybe it could be a separate function to set the hwaddr that the standard plugins call instead of being in ConfigureInteface(). |
I see your concern, and I think it's worth re-designing this. Instead of checking the interface type within IPAM, it should be called by the main plugins after IPAM allocated the IP address. |
steveej
added
reviewed/needs rework
missing/tests
needs review
and removed
needs review
reviewed/needs rework
labels
|
I've restructured the code and started to add tests for the Reviews welcome at this point! @containernetworking/cni-maintainers |
|
ping @containernetworking/cni-maintainers /cc @squeed you're invited to review this too! |
| } | ||
| } | ||
|
|
||
| mac := prefix |
There was a problem hiding this comment.
This string conversion is a little awkward. Both of these types are just wrappers around []byte. So, you can do:
ip = ip.To4()
addr := (net.HardwareAddr)(append([]byte{0x0a, 0x58}, ip...))
This removes one of the err types
There was a problem hiding this comment.
I can't see it working that easily because we want to strip out the . characters.
There was a problem hiding this comment.
It really does :-). The byte arrays are the real numerical address value, not the string.
|
Looks mostly good to me. You could squash some of the commits together since some are just fixups based on review comments. And the coveralls thing... |
* _suite.go and _test.go file should be in the same package, using the _test package for that, which requires some fields and methods to be exported * Introduce error type for cleaner error handling * test adaptions for error type checking
This will give deterministic MAC addresses for all interfaces CNI creates and manages the IP for: * bridge: container veth and host bridge * macvlan: container veth * ptp: container veth and host veth
* bridge: Test the following interface's hardware address for the CNI specific prefix: - bridge with IP address - container veth * plugins/macvlan test: ensure hardware addr
|
@dcbw I squashed as much as I felt comfortable doing so. (hopefully) last review please ;-) |
|
@steveej see my comment here 924b30b#r71946686 , that still needs to be addressed. |
|
After that, LGTM. |
|
👍 LGTM |
Closes #196.