cni hwaddr

gistfile1.txt

// GenerateHardwareAddr generates 48 bit virtual mac addresses based on the IP4 or IPv6 input.
func GenerateHardwareAddr(ip net.IP, prefix []byte) (net.HardwareAddr, error) {
	switch {

	case (ip.To4() || ip.To16()) == nil:
		return nil, SupportIpErr{msg: "GenerateHardwareAddr requires an IPv4 or IPv6 address as input"}

	case len(prefix) != len(PrivateMACPrefix):
		return nil, InvalidPrefixLengthErr{msg: fmt.Sprintf(
			"Prefix has length %d instead  of %d", len(prefix), len(PrivateMACPrefix)),
		}
	}

	ipByteLen := len(ip)
	return (net.HardwareAddr)(
		append(
			prefix,
			ip[ipByteLen-ipRelevantByteLen:ipByteLen]...),
	), nil
}

gistfile2.txt

func SetHWAddrByIP(ifName string, ip4 net.IP, ip6 net.IP) error {
	iface, err := netlink.LinkByName(ifName)
	if err != nil {
		return fmt.Errorf("failed to lookup %q: %v", ifName, err)
	}

	switch {
	case ip4 == nil && ip6 == nil:
		return fmt.Errorf("neither ip4 or ip6 specified")

	case ip4 != nil:
		{
			hwAddr, err := hwaddr.GenerateHardwareAddr(ip4, hwaddr.PrivateMACPrefix)
			if err != nil {
				return fmt.Errorf("failed to generate hardware addr: %v", err)
			}
			if err = netlink.LinkSetHardwareAddr(iface, hwAddr); err != nil {
				return fmt.Errorf("failed to add hardware addr to %q: %v", ifName, err)
			}
		}
	case ip4 == nil && ip6 != nil:
		{
			hwAddr, err := hwaddr.GenerateHardwareAddr(ip6, hwaddr.PrivateMACPrefix)
			if err != nil {
				return fmt.Errorf("failed to generate hardware addr: %v", err)
			}
			if err = netlink.LinkSetHardwareAddr(iface, hwAddr); err != nil {
				return fmt.Errorf("failed to add hardware addr to %q: %v", ifName, err)
			}
		}	}

	return nil
}

Trying to expound on what I was saying in our meeting:
(1) In your gistfile1.txt, your test:
case (ip.To4() || ip.To16()) == nil:
will not catch an IPv4 corner case error that gets caught with today's CNI code (even before my changes). Today's code won't let an outside entity pass in a typical IPv6 address as an IPv4 address. e.g. it will reject something like 2001:2::1 if that is passed as an IPv4 address. Here's the source code for ip.to4():

func (ip IP) To4() IP {
if len(ip) == IPv4len {
return ip
}
if len(ip) == IPv6len &&
isZeros(ip[0:10]) && <=== Note: 16-byte slice with 10 zeros, 2 bytes of ff, and 4 bytes of address is okay
ip[10] == 0xff && but that's the only form of 16-byte slice allowed. "Normal" v6 addrs are rejected.
ip[11] == 0xff {
return ip[12:16]
}
return nil
}

So using your test on, say 2001:2::1, the ip.To4() expression will return invalid (nil) but the ip.To6() expression will return valid (non-nil), so using v6 address as a V4 address will be allowed, whereas today's code won't allow this.

Do we care if someone mistakenly passes in an IPv6 as an IPv4 address? Probably not, it's a very unlikely corner case, but the reason that I left this logic in is that in my experience, whenever some validity test is taken out (logic is not preserved), it is always rejected in code review unless there's a really good reason that the logic should be removed (there's a huge bias towards the least amount of change). Proving this corner case will never happen is pretty difficult.

(2) In your gistfile2.txt, you have a block of code (lines 13-19) that gets essentially duplicated (lines 23-29), with the only difference being the variable ip4 used vs. ip6. My preference would be have only one copy of these lines, and just switch what you're passing to that block using an intermediate variable, e.g.:

switch {
case ip4 != nil:
ip = ip4
case ip6 != nil:
ip = ip6
default:
return fmt.Errorf("neither ip4 or ip6 specified")
}

hwAddr, err := hwaddr.GenerateHardwareAddr(ip4, hwaddr.PrivateMACPrefix)
if err != nil {
return fmt.Errorf("failed to generate hardware addr: %v", err)
}
if err = netlink.LinkSetHardwareAddr(iface, hwAddr); err != nil {
return fmt.Errorf("failed to add hardware addr to %q: %v", ifName, err)
}

(3) Going a little further with the approach I suggested in (2), we can add in the ip.To4() and ip.To16() test cases:

switch {
case ip4 != nil && ip.To4(ip4): <=== NOTE validity test here
ip = ip4
case ip6 != nil && ip.To16(ip6): <=== NOTE validity test here
ip = ip6
default:
return fmt.Errorf("A valid ip4 or valid ip6 is required")
}

hwAddr, err := hwaddr.GenerateHardwareAddr(ip4, hwaddr.PrivateMACPrefix)
if err != nil {
return fmt.Errorf("failed to generate hardware addr: %v", err)
}
if err = netlink.LinkSetHardwareAddr(iface, hwAddr); err != nil {
return fmt.Errorf("failed to add hardware addr to %q: %v", ifName, err)
}

And then the only test in hwaddr.GenerateHardwareAddr() would be a check on the 2-byte prefix length.

In any event, I think this assignment of MAC address based on IP addresses is broken and is going to lead to many problems down the road.