fix static route activation condition (#6824)

- if a static route matches its own next-hop, a matching route must be more specific
batfish · Apr 1, 2021 · 50c04b8 · 50c04b8
1 parent fd170a7
commit 50c04b8
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 11 deletions.
diff --git a/projects/batfish/src/main/java/org/batfish/dataplane/protocols/StaticRouteHelper.java b/projects/batfish/src/main/java/org/batfish/dataplane/protocols/StaticRouteHelper.java
@@ -4,6 +4,8 @@
 import javax.annotation.ParametersAreNonnullByDefault;
 import org.batfish.datamodel.AbstractRouteDecorator;
 import org.batfish.datamodel.GenericRib;
+import org.batfish.datamodel.Ip;
+import org.batfish.datamodel.Prefix;
 import org.batfish.datamodel.ResolutionRestriction;
 import org.batfish.datamodel.RoutingProtocol;
 import org.batfish.datamodel.StaticRoute;
@@ -25,9 +27,10 @@ public class StaticRouteHelper {
   public static <R extends AbstractRouteDecorator> boolean shouldActivateNextHopIpRoute(
       StaticRoute route, GenericRib<R> rib, ResolutionRestriction<R> restriction) {
     boolean recursive = route.getRecursive();
+    Ip nextHopIp = route.getNextHopIp();
     Set<R> matchingRoutes =
         rib.longestPrefixMatch(
-            route.getNextHopIp(),
+            nextHopIp,
             r -> {
               if (r.getAbstractRoute().getProtocol() == RoutingProtocol.CONNECTED) {
                 // All static routes can be activated by a connected route.
@@ -41,14 +44,18 @@ public static <R extends AbstractRouteDecorator> boolean shouldActivateNextHopIp
               return restriction.test(r);
             });
 
-    // If matchingRoutes is empty, cannot activate because next hop ip is unreachable
+    // - If matchingRoutes is empty, cannot activate because the next hop ip is unreachable.
+    // - If the prefix of the route to be activated contains the route's next hop, then
+    //   a matching route must have a longer prefix. Otherwise, the route will become its own
+    //   longest prefix match upon activation, creating a loop.
+    Prefix network = route.getNetwork();
+    int prefixLength = network.getPrefixLength();
+    boolean containsOwnNextHop = network.containsIp(nextHopIp);
     return matchingRoutes.stream()
         .map(AbstractRouteDecorator::getAbstractRoute)
         .anyMatch(
             routeToNextHop ->
-                // Next hop has to be reachable through a route with a different prefix
-                !routeToNextHop.getNetwork().equals(route.getNetwork())
-                    // TODO: fix properly and stop allowing self-activation
-                    || route.equals(routeToNextHop));
+                !containsOwnNextHop
+                    || routeToNextHop.getNetwork().getPrefixLength() > prefixLength);
   }
 }
diff --git a/projects/batfish/src/test/java/org/batfish/dataplane/protocols/StaticRouteHelperTest.java b/projects/batfish/src/test/java/org/batfish/dataplane/protocols/StaticRouteHelperTest.java
@@ -103,16 +103,20 @@ public void testShouldActivateSelfReferential() {
     assertThat(shouldActivateNextHopIpRoute(sr, _rib, alwaysTrue()), equalTo(false));
   }
 
-  /** Activate the route with next hop IP within route's prefix, if it is already in the RIB */
+  /**
+   * Activate a route matching its own next-hop IP if there is a more specific matching route
+   * already in the RIB
+   */
   @Test
   public void testShouldActivateIfExists() {
+    StaticRoute matching =
+        StaticRoute.testBuilder().setNetwork(Prefix.strict("1.1.1.1/32")).build();
     StaticRoute sr =
         StaticRoute.testBuilder()
             .setNetwork(Prefix.parse("1.1.1.0/24"))
             .setNextHopIp(Ip.parse("1.1.1.1"))
-            .setAdministrativeCost(1)
             .build();
-    _rib.mergeRoute(annotateRoute(sr));
+    _rib.mergeRoute(annotateRoute(matching));
 
     // Test & Assert
     assertThat(shouldActivateNextHopIpRoute(sr, _rib, alwaysTrue()), equalTo(true));
@@ -184,7 +188,10 @@ public void testShouldActivateWithLoop() {
     assertThat(shouldActivateNextHopIpRoute(sr, _rib, alwaysTrue()), equalTo(true));
   }
 
-  /** Allow installation of a covered/more specific route */
+  /**
+   * Do not allow installation of a route that would become longest prefix match for its own next
+   * hop IP.
+   */
   @Test
   public void testShouldActivateIfCovered() {
     _rib.mergeRoute(annotateRoute(new ConnectedRoute(Prefix.parse("9.9.0.0/16"), "Eth0")));
@@ -198,7 +205,7 @@ public void testShouldActivateIfCovered() {
             .build();
 
     // Test & Assert
-    assertThat(shouldActivateNextHopIpRoute(sr, _rib, alwaysTrue()), equalTo(true));
+    assertFalse(shouldActivateNextHopIpRoute(sr, _rib, alwaysTrue()));
   }
 
   /**