AddrAdd shouldn't add broadcast address if not directly asked to

[jjastrze-ovh]

Let's consider following code:

	link := &netlink.Dummy{
		LinkAttrs: netlink.LinkAttrs{
			Flags: net.FlagUp,
			Name:  "ntest",
		},
	}

	if err := netlink.LinkAdd(link); err != nil {
		fmt.Println("add", err)
		return
	}

	addr := &netlink.Addr{
		IPNet: &net.IPNet{
			IP:   net.ParseIP("192.168.22.19"),
			Mask: net.CIDRMask(24, 32),
		},
	}

	if err := netlink.AddrAdd(link, addr); err != nil {
		fmt.Println("add addr", err)
		return
	}

The code should add 192.168.22.19 address to device ntest, but it also adds broadcast address, which is some scenarios breaks eg. adding routes with error=invalid argument.

[root@machine] ip -4 a s ntest
6488: ntest: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default 
    inet 192.168.22.19/24 brd 192.168.22.255 scope global ntest
       valid_lft forever preferred_lft forever

For me presented code should be equivalent to what following ip command does:

[root@root@machine] ip l a ntest type dummy
[root@root@machine] ip a a 192.168.22.19/24 dev ntest
[root@root@machine] ip -4 a s ntest
6489: ntest: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN group default 
    inet 192.168.22.19/24 scope global ntest
       valid_lft forever preferred_lft forever

Not to this:

[root@root@machine] ip a a 192.168.22.19/24 brd 192.168.22.255 dev ntest
[root@root@machine] ip -4 a s ntest
6489: ntest: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default 
    inet 192.168.22.19/24 brd 192.168.22.255 scope global ntest
       valid_lft forever preferred_lft forever

iptool makes end user responsibility to specify broadcast address if it is needed. In my opinion this library should behave in the same way

The proposal for this issue is: https://github.com/jjastrze-ovh/netlink/commit/9a85a619d2f74114626ca04b83b56f5a84f711df

[jjastrze-ovh]

@aboch @vishvananda any comments?

[aboch]

@jjastrze-ovh sorry for missing this.

Please go ahead and feel free to open a PR with your proposed change. No need to wait for an ACK.

Please be aware of #248 which added the automatic brd addition. The behavior has been there for more than a year, therefore we cannot revert it now.

I'd suggest you to look for a way to extend the Addr structure so that clients can ask not to auto compute the brd.

I know it would be the opposite of what iproute2 does. But I do not see another solution ATM.

Thanks

[dannyk81]

@aboch, @vishvananda

As part of https://github.com/kubernetes/dns/issues/282, it seems we've hit an issue with the current implementation of broadcast address auto-calculation.

Specifically, the issue is with /32 prefixlen addresses.

In the linked issue above, you can see that when adding a /32 address using iproute2 commands, the IFA_BROADCAST attribute is not sent to NETLINK (as expected).

However, when using the netlink package an incorrect IFA_BROADCAST is calculated and send to NETLINK, which in the case of CoreOS based systems seems to prevent from the IP address to be added to the interface.

Here are the decoded messages sent to NETLINK using iproute2 vs. netlink pkg:

iproute2:

28:00:00:00:14:00:05:06:21:d2:45:5c:00:00:00:00:02:20:00:00:b5:01:00:00:08:00:02:00:0a:0a:0a:0a:08:00:01:00:0a:0a:0a:0a
{'attrs': [('IFA_LOCAL', '10.10.10.10'), ('IFA_ADDRESS', '10.10.10.10')],
 'family': 2,
 'flags': 0,
 'header': {'flags': 1541,
            'length': 40,
            'pid': 0,
            'sequence_number': 1548079649,
            'type': 20},
 'index': 437,
 'prefixlen': 32,
 'scope': 0}
........................................

Go netlink:

30:00:00:00:14:00:05:06:07:00:00:00:00:00:00:00:02:20:00:00:19:00:00:00:08:00:02:00:0a:0a:0a:0a:08:00:01:00:0a:0a:0a:0a:08:00:04:00:0a:0a:0a:00
{'attrs': [('IFA_LOCAL', '10.10.10.10'),
           ('IFA_ADDRESS', '10.10.10.10'),
           ('IFA_BROADCAST', '10.10.10.0')],
 'family': 2,
 'flags': 0,
 'header': {'flags': 1541,
            'length': 48,
            'pid': 0,
            'sequence_number': 7,
            'type': 20},
 'index': 25,
 'prefixlen': 32,
 'scope': 0}
........................................

as you can see, the calculated broadcast address 'IFA_BROADCAST', '10.10.10.0' that is submitted is incorrect, considering 'IFA_ADDRESS', '10.10.10.10' and prefixlen of 32, it can't actually have a broadcast address (or more accurately it's IFA_BROADCAST == IFA_ADDRESS).

Considering the above, perhaps the if here https://github.com/vishvananda/netlink/blob/1e2e7ab670e05e2fd29bf5d0bcab2f9907e9f9f4/addr_linux.go#L111 should be modified to ensure that prefixlen < 31

[aboch]

Thank you @dannyk81. Feel free to open a PR with the fix.

[alexanderstephan]

I had a case where I don't want to have any brodcast IP at all, even if prefixlen < 31. To achieve this, I just set addr.Broadcast = net.IPv4Zero before calling netlink.AddrAdd.

I think this comment should get updated. For me, calling ip addr add $addr dev $link does not add the broadcast IP, so it is presumably not equalivalent.

[aboch]

@alexanderstephan Feel free to open a PR for that.