On 2016 Mar 21 (Mon) at 16:22:53 +0100 (+0100), Claudio Jeker wrote:
:On Mon, Mar 21, 2016 at 03:54:38PM +0100, Peter Hessler wrote:
:> We ran into a situation where we accidentally blackholed traffic going to
:> a new Internet Exchange. When we added the new vlans and new peers, the
:> nexthop address on that vlan was *not* our neighbor's address, but
:> instead used our own IP address on that new interface. "bgpctl show rib",
:> showed the correct IP, but the wrong one was used when installing into
:> the fib.
:>
:> Restarting bgpd installed the correct nexthop.
:>
:> Additionally, a user had reported that "network inet connected" didn't
:> work any more. This fix also convinces bgpd to redistribute the network
:> as soon as it is created. The underlying cause was a combination of
:> missing the F_CONNECTED flag, as well as having an ifindex of 0, which
:> is lo0's index.
:>
:> OK?
:
:I noticed this breakage as well some time ago but never managed to debug
:it (too many production systems and not enough test systems).
:This was caused by the change of cloning routes to show the IP address
:instead of the link local one.
:Now about the diff I'm not 100% sure. It for sure changes beheviour a bit.
:ifindex is now always set and not only for connected routes. Not sure if
:this is bad or not...
:mpath should probably be set to 0 in the RTF_CONNECTED case to keep same
:behaviour. On the other hand it is now possible to have multiple routes to
:the same network even for connected routes (not sure if it is possible on
:the same prio or not).
:
:IMO this is a more conservative version...
:
: case AF_INET:
: case AF_INET6:
: if (rtm->rtm_flags & RTF_CONNECTED) {
: flags |= F_CONNECTED;
: ifindex = rtm->rtm_index;
: sa = NULL;
: mpath = 0;
: }
: break;
:
:I think this is less dangerous or what you think?
setting ifindex should always be safe, because that is the ifindex that
is valid for that route. Setting it to zero (lo0) is Wrong(tm).
I'm not 100% convinced about mpath=0, or about setting sa=NULL, so I
left those alone for this version. I did see that we only used sa when
we already had a route, not when we are creating one. I don't have
super-strong feelings about them, though.
Since mpi@ says that RTAX_GATEWAY now returns the af, I could also simply
add those as FALLTHROUGH for the main switch statement...
:
:> Index: kroute.c
:> ===================================================================
:> RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
:> retrieving revision 1.207
:> diff -u -p -u -p -r1.207 kroute.c
:> --- kroute.c 5 Dec 2015 18:28:04 -0000 1.207
:> +++ kroute.c 21 Mar 2016 14:44:52 -0000
:> @@ -3178,6 +3178,12 @@ dispatch_rtmsg_addr(struct rt_msghdr *rt
:> sa = NULL;
:> mpath = 0; /* link local stuff can't be mpath */
:> break;
:> + case AF_INET:
:> + case AF_INET6:
:> + if (rtm->rtm_flags & RTF_CONNECTED)
:> + flags |= F_CONNECTED;
:> + ifindex = rtm->rtm_index;
:> + break;
:> }
:>
:> if (rtm->rtm_type == RTM_DELETE) {
:>
:>
:>
:>
:> --
:> If you are a fatalist, what can you do about it?
:> -- Ann Edwards-Duff
:
:--
::wq Claudio
--
Death is nature's way of telling you to slow down.
