Patch to allow for 'ip route add table XXX unreachable default metric…

[lasselj]

It occurs to me that to create constructions below which allows multicast in vrfs we can't blindly add RTA_DST and RTA_OIF attributes. In some instances they must be omitted.

func (controller *Controller) addDefaultRouteOfLastResortToVrf(tableID int) {

	// Default route high priority
	// Purpose: ip route add table 101 unreachable default metric 4278198272
	vrfDefaultRoute := &netlink.Route{
		Table: tableID,
		Dst: &net.IPNet{
			IP:   net.IP{},
			Mask: net.IPMask{},
		},

		Priority: 4278198272,
		Type:     unix.RTN_UNREACHABLE,
		Scope:    unix.RT_SCOPE_UNIVERSE,
	}

	netlink.RouteAdd(vrfDefaultRoute)
}

[bersoare]

doesn't using nil as the dst equals to a default route?

[lasselj]

No, unfortunately it does not. It was the first thing I checked. The reason is that we need the logic that follows the route.Dst != nil && route.Dst.IP != nil if condition to set things like family etc.

[bersoare]

ya you are right, it looks like this pkg requires us to provide any of that - but we don't necessarily need it.

netlink/route_linux.go

Lines 732 to 735 in e5fd1f8

	func (h *Handle) routeHandle(route *Route, req *nl.NetlinkRequest, msg *nl.RtMsg) error {
	if (route.Dst == nil || route.Dst.IP == nil) && route.Src == nil && route.Gw == nil && route.MPLSDst == nil {
	return fmt.Errorf("one of Dst.IP, Src, or Gw must not be nil")
	}

but i managed to work around that using 0.0.0.0 as the Gw:

package main

import (
	"net"

	"golang.org/x/sys/unix"

	"github.com/vishvananda/netlink"
)

func main() {
	vrfDefaultRoute := &netlink.Route{
		Table:    123,
		Dst:      nil,
		Family:   netlink.FAMILY_V4,
		Gw:       net.IPv4(0, 0, 0, 0),
		Priority: 4278198272,
		Type:     unix.RTN_UNREACHABLE,
		Scope:    unix.RT_SCOPE_UNIVERSE,
	}
	err := netlink.RouteAdd(vrfDefaultRoute)
	if err != nil {
		panic(err)
	}
}
# ip ro sh tab 123
unreachable default metric 4278198272

netlink message (strace):

sendto(3, {{len=52, type=RTM_NEWROUTE, flags=NLM_F_REQUEST|NLM_F_ACK|NLM_F_EXCL|NLM_F_CREATE, seq=1, pid=0}, {rtm_family=AF_INET, rtm_dst_len=0, rtm_src_len=0, rtm_tos=0, rtm_table=0x7b, rtm_protocol=RTPROT_BOOT, rtm_scope=RT_SCOPE_UNIVERSE, rtm_type=RTN_UNREACHABLE, rtm_flags=0}, [{{nla_len=8, nla_type=RTA_GATEWAY}, inet_addr("0.0.0.0")}, {{nla_len=8, nla_type=RTA_PRIORITY}, 4278198272}, {{nla_len=8, nla_type=RTA_OIF}, 0}]}, 52, 0, {sa_family=AF_NETLINK, nl_pid=0, nl_groups=00000000}, 12) = 52

and here is how the netlink message for the ip route add unreachable default metric 4278198272 looks like:

sendmsg(3, {msg_name={sa_family=AF_NETLINK, nl_pid=0, nl_groups=00000000}, msg_namelen=12, msg_iov=[{iov_base={{len=36, type=RTM_NEWROUTE, flags=NLM_F_REQUEST|NLM_F_ACK|NLM_F_EXCL|NLM_F_CREATE, seq=1647613501, pid=0}, {rtm_family=AF_INET, rtm_dst_len=0, rtm_src_len=0, rtm_tos=0, rtm_table=0x7b, rtm_protocol=RTPROT_BOOT, rtm_scope=RT_SCOPE_UNIVERSE, rtm_type=RTN_UNREACHABLE, rtm_flags=0}, {{nla_len=8, nla_type=RTA_PRIORITY}, 4278198272}}, iov_len=36}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 36

git blame points to the initial commit of the package, so maybe instead of handling that attribute specifically, we can revisit that validation

[vishvananda]

we can definitely update the nil check. Is there any other type besides unreachable that you could use for the default route without a gw or src? I'm thinking that we can just add a type != unreachable into that check.

[bersoare]

there are quite a few route types that don't need a gateway or src. an example is a P2P link (GRE tunnel), where you can just specify the interface with dev argument
other route types i can think of are RTN_BLACKHOLE, RTN_UNREACHABLE, RTN_PROHIBIT, and maybe more as im not familiar with all route types.

in my view is better to just get rid of that check, and let netlink return the error message to the library in case it gets something invalid and pass that error back to the user.
im not a maintainer of this pkg, so my opinion doesn't really matter. ccing @aboch to comment on what would be the best approach here

edit: oops, i didn't see it was @vishvananda replying to this heh

[lasselj]

Ok, I have reviewed the comments, and on balance I like my approach best :-) The route to 0.0.0.0/0 with the family inferred from the dst type I think is preferable to specifying it explicitly and providing a non-nil gateway. (If your family and gateway type does not match, then you are in trouble for instance). Similarly, I think the check to omit the OIF attribute when the link is unset is the best approach. But I accept that if the PR is not merged the functionality can still be dragged out of the module... :-) Happy to add checks for RTN_BLACKHOLE and RTN_PROHIBIT in addition to RTN_UNREACHABLE and flatten commit if desired.

[vishvananda]

I'm ok with this approach, but could you please squash into a single commit?

[lasselj]

@vishvananda Cool.... leave it with me. I'll do that shortly.

[lasselj]

@vishvananda All done

[lasselj]

@vishvananda Anything else I can offer to do on this PR?

/Lasse

[aboch]

@lasselj
Sorry for the delay.
Can you please fix the conflicts?
Thanks

[aboch]

@lasselj