RIP: fix split horizon instability

When split horizon is active (by default), RIP withdraws routes from
these neighbors which have sent the best route to us. This helps with
reducing routing loops. When ECMP is on (by default), RIP behaves this
way only to the first neighbor in the list.

In BIRD 2, the neighbors were first sorted and then the first one was
chosen. This was lost in BIRD 3 and the first neighbor in the unsorted
list was considered instead.

The overall result in routing does not change so this is not technically
a bug and should not result in misrouting. The final result is unstable
and time-sensitive though, making debugging harder and automatic tests
fail.

We are rectifying this to be stable again so that our tests may get
green again.

Fixes: #284
Reproduced-By: David Petera while checking RIP in VRFs
Identified-By: Ondrej Zajicek

diff --git a/proto/rip/rip.c b/proto/rip/rip.c
index 3491c9beabdf6927da47b5515e6079f4c76a4a84..1bc11d391f3ae4da2fc62108eb9156cd80d261e7 100644 (file)
--- a/proto/rip/rip.c
+++ b/proto/rip/rip.c
@@ -179,6 +179,7 @@ rip_announce_rte(struct rip_proto *p, struct rip_entry *en)
        if (rip_valid_rte(rt))
          num++;
 
+      ASSERT_DIE(num > 0);
       struct nexthop_adata *nhad = (struct nexthop_adata *) tmp_alloc_adata((num+1) * sizeof(struct nexthop));
       struct nexthop *nh = &nhad->nh;
 
@@ -193,9 +194,6 @@ rip_announce_rte(struct rip_proto *p, struct rip_entry *en)
          .weight = rt->from->ifa->cf->ecmp_weight,
        };
 
-       if (!rt_from)
-         rt_from = rt->from->ifa->iface;
-
        nh = NEXTHOP_NEXT(nh);
 
        if (rt->tag != rt_tag)
@@ -203,10 +201,13 @@ rip_announce_rte(struct rip_proto *p, struct rip_entry *en)
       }
 
       nhad->ad.length = ((void *) nh - (void *) nhad->ad.data);
-
+      nhad = nexthop_sort(nhad, tmp_linpool);
       ea_set_attr(&ea,
          EA_LITERAL_DIRECT_ADATA(&ea_gen_nexthop, 0,
-           &(nexthop_sort(nhad, tmp_linpool)->ad)));
+           &(nhad->ad)));
+
+      /* Set from as the first interface in the sorted nexthop list for stable results. */
+      rt_from = nhad->nh.iface;
     }
     else
     {