Add support for "force"/"all" with next-hop-self in Cumulus configs. (#…

…5800)
batfish · May 14, 2020 · efee783 · efee783
1 parent a15d2c4
commit efee783
Show file tree

Hide file tree

Showing 6 changed files with 129 additions and 9 deletions.
diff --git a/projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrLexer.g4 b/projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrLexer.g4
@@ -65,6 +65,11 @@ ALERTS
    'alerts'
 ;
 
+ALL
+:
+  'all'
+;
+
 ALLOWAS_IN
 :
   'allowas-in'
@@ -291,6 +296,11 @@ FILE
   'file' -> pushMode(M_Remark)
 ;
 
+FORCE
+:
+  'force'
+;
+
 FORWARDING
 :
   'forwarding'

diff --git a/projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrr_bgp.g4 b/projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrr_bgp.g4
@@ -303,7 +303,7 @@ sbafin_default_originate
 
 sbafin_next_hop_self
 :
-  NEXT_HOP_SELF
+  NEXT_HOP_SELF (FORCE | ALL)?
 ;
 
 sbafin_route_reflector_client

diff --git a/...batfish/src/main/java/org/batfish/grammar/cumulus_frr/CumulusFrrConfigurationBuilder.java b/...batfish/src/main/java/org/batfish/grammar/cumulus_frr/CumulusFrrConfigurationBuilder.java
@@ -691,6 +691,12 @@ public void exitSbafin_next_hop_self(Sbafin_next_hop_selfContext ctx) {
       return;
     }
     _currentBgpNeighborIpv4UnicastAddressFamily.setNextHopSelf(true);
+    // "force" and "all" are aliases. See https://github.com/FRRouting/frr/pull/4200.
+    // The functionality is described at
+    // http://docs.frrouting.org/en/latest/bgp.html#clicmd-[no]neighborPEERnext-hop-self[all].
+    if (ctx.FORCE() != null || ctx.ALL() != null) {
+      _currentBgpNeighborIpv4UnicastAddressFamily.setNextHopSelfAll(true);
+    }
   }
 
   @Override

diff --git a/...src/main/java/org/batfish/representation/cumulus/BgpNeighborIpv4UnicastAddressFamily.java b/...src/main/java/org/batfish/representation/cumulus/BgpNeighborIpv4UnicastAddressFamily.java
@@ -11,6 +11,7 @@ public class BgpNeighborIpv4UnicastAddressFamily implements Serializable {
   @Nullable private Boolean _defaultOriginate;
   @Nullable private Boolean _routeReflectorClient;
   @Nullable private Boolean _nextHopSelf;
+  @Nullable private Boolean _nextHopSelfAll;
   @Nullable private String _routeMapIn;
   @Nullable private String _routeMapOut;
 
@@ -44,6 +45,15 @@ public void setNextHopSelf(boolean nextHopSelf) {
     _nextHopSelf = nextHopSelf;
   }
 
+  @Nullable
+  public Boolean getNextHopSelfAll() {
+    return _nextHopSelfAll;
+  }
+
+  public void setNextHopSelfAll(boolean nextHopSelfAll) {
+    _nextHopSelfAll = nextHopSelfAll;
+  }
+
   void inheritFrom(@Nonnull BgpNeighborIpv4UnicastAddressFamily other) {
     if (_activated == null) {
       _activated = other.getActivated();

diff --git a/projects/batfish/src/main/java/org/batfish/representation/cumulus/CumulusConversions.java b/projects/batfish/src/main/java/org/batfish/representation/cumulus/CumulusConversions.java
@@ -771,17 +771,29 @@ private static List<Statement> getAcceptStatements(BgpNeighbor neighbor, BgpVrf
 
   @VisibleForTesting
   static @Nullable SetNextHop getSetNextHop(BgpNeighbor neighbor, BgpVrf bgpVrf) {
-    if (neighbor.getRemoteAs() == null
-        || bgpVrf.getAutonomousSystem() == null
-        || !neighbor.getRemoteAs().equals(bgpVrf.getAutonomousSystem())) {
+    if (neighbor.getRemoteAs() == null || bgpVrf.getAutonomousSystem() == null) {
       return null;
     }
 
+    boolean isIBgp = neighbor.getRemoteAs().equals(bgpVrf.getAutonomousSystem());
+    // TODO: Need to handle dynamic neighbors.
     boolean nextHopSelf =
         Optional.ofNullable(neighbor.getIpv4UnicastAddressFamily())
             .map(BgpNeighborIpv4UnicastAddressFamily::getNextHopSelf)
             .orElse(false);
 
+    if (isIBgp) {
+      // Check for "force".
+      // TODO: Handle v6 AFI.
+      BgpNeighborIpv4UnicastAddressFamily ipv4af = neighbor.getIpv4UnicastAddressFamily();
+      if (ipv4af != null) {
+        Boolean nextHopSelfAll = ipv4af.getNextHopSelfAll();
+        if (nextHopSelfAll == null || !nextHopSelfAll) {
+          nextHopSelf = false;
+        }
+      }
+    }
+
     return nextHopSelf ? new SetNextHop(SelfNextHop.getInstance()) : null;
   }
 

diff --git a/...ects/batfish/src/test/java/org/batfish/representation/cumulus/CumulusConversionsTest.java b/...ects/batfish/src/test/java/org/batfish/representation/cumulus/CumulusConversionsTest.java
@@ -384,16 +384,14 @@ public void testGetAcceptStatements() {
     }
 
     {
-      // if is ibgp and neighbor configuration set next-hop-self set, then set
-      // next-hop-self
+      // if is ibgp and neighbor configuration has next-hop-self set, then getSetNextHop should
+      // return null.
       bgpNeighbor.setRemoteAs(10000L);
       bgpVrf.setAutonomousSystem(10000L);
       BgpNeighborIpv4UnicastAddressFamily ipv4af = new BgpNeighborIpv4UnicastAddressFamily();
       bgpNeighbor.setIpv4UnicastAddressFamily(ipv4af);
       ipv4af.setNextHopSelf(true);
-
-      assertThat(
-          getSetNextHop(bgpNeighbor, bgpVrf), equalTo(new SetNextHop(SelfNextHop.getInstance())));
+      assertNull(getSetNextHop(bgpNeighbor, bgpVrf));
     }
   }
 
@@ -1696,4 +1694,88 @@ public void testConvertVxlan_localIpPrecedence() {
         c, vsConfig, ImmutableMap.of(1001, vrf.getName()), null, loopbackTunnelIp, new Warnings());
     assertThat(vrf.getLayer3Vnis().get(1001).getSourceAddress(), equalTo(loopbackTunnelIp));
   }
+
+  @Test
+  public void testNextHopSelfAll() {
+    BgpNeighbor bgpNeighbor = new BgpIpNeighbor("10.0.0.1");
+    BgpVrf bgpVrf = new BgpVrf("bgpVrf");
+    {
+      // If eBGP and next-hop-self is NOT set, then next-hop should not be self.
+      bgpNeighbor.setRemoteAs(10000L);
+      bgpVrf.setAutonomousSystem(20000L);
+      BgpNeighborIpv4UnicastAddressFamily ipv4af = new BgpNeighborIpv4UnicastAddressFamily();
+      bgpNeighbor.setIpv4UnicastAddressFamily(ipv4af);
+      ipv4af.setNextHopSelf(false);
+      ipv4af.setNextHopSelfAll(false);
+      assertThat(getSetNextHop(bgpNeighbor, bgpVrf), equalTo(null));
+    }
+
+    {
+      // If eBGP and next-hop-self is set WITHOUT force, then next-hop should be self.
+      bgpNeighbor.setRemoteAs(10000L);
+      bgpVrf.setAutonomousSystem(20000L);
+      BgpNeighborIpv4UnicastAddressFamily ipv4af = new BgpNeighborIpv4UnicastAddressFamily();
+      bgpNeighbor.setIpv4UnicastAddressFamily(ipv4af);
+      ipv4af.setNextHopSelf(true);
+      ipv4af.setNextHopSelfAll(false);
+      assertThat(
+          getSetNextHop(bgpNeighbor, bgpVrf), equalTo(new SetNextHop(SelfNextHop.getInstance())));
+    }
+
+    {
+      // If eBGP and next-hop-self is set WITH force, then next-hop should be self.
+      bgpNeighbor.setRemoteAs(10000L);
+      bgpVrf.setAutonomousSystem(20000L);
+      BgpNeighborIpv4UnicastAddressFamily ipv4af = new BgpNeighborIpv4UnicastAddressFamily();
+      bgpNeighbor.setIpv4UnicastAddressFamily(ipv4af);
+      ipv4af.setNextHopSelf(true);
+      ipv4af.setNextHopSelfAll(true);
+      assertThat(
+          getSetNextHop(bgpNeighbor, bgpVrf), equalTo(new SetNextHop(SelfNextHop.getInstance())));
+    }
+
+    {
+      // If iBGP and next-hop-self NOT set, then next-hop should be null.
+      bgpNeighbor.setRemoteAs(10000L);
+      bgpVrf.setAutonomousSystem(10000L);
+      BgpNeighborIpv4UnicastAddressFamily ipv4af = new BgpNeighborIpv4UnicastAddressFamily();
+      bgpNeighbor.setIpv4UnicastAddressFamily(ipv4af);
+      ipv4af.setNextHopSelf(false);
+      ipv4af.setNextHopSelfAll(false);
+      assertThat(getSetNextHop(bgpNeighbor, bgpVrf), equalTo(null));
+    }
+
+    {
+      // If iBGP and next-hop-self is set WITHOUT force, then next-hop should be null.
+      bgpNeighbor.setRemoteAs(10000L);
+      bgpVrf.setAutonomousSystem(10000L);
+      BgpNeighborIpv4UnicastAddressFamily ipv4af = new BgpNeighborIpv4UnicastAddressFamily();
+      bgpNeighbor.setIpv4UnicastAddressFamily(ipv4af);
+      ipv4af.setNextHopSelf(true);
+      ipv4af.setNextHopSelfAll(false);
+      assertThat(getSetNextHop(bgpNeighbor, bgpVrf), equalTo(null));
+    }
+
+    {
+      // If iBGP and next-hop-self is set WITH force, then next-hop should be self.
+      bgpNeighbor.setRemoteAs(10000L);
+      bgpVrf.setAutonomousSystem(10000L);
+      BgpNeighborIpv4UnicastAddressFamily ipv4af = new BgpNeighborIpv4UnicastAddressFamily();
+      bgpNeighbor.setIpv4UnicastAddressFamily(ipv4af);
+      ipv4af.setNextHopSelf(true);
+      ipv4af.setNextHopSelfAll(true);
+      assertThat(
+          getSetNextHop(bgpNeighbor, bgpVrf), equalTo(new SetNextHop(SelfNextHop.getInstance())));
+    }
+
+    {
+      // If iBGP and force is null, then next-hop should be null.
+      bgpNeighbor.setRemoteAs(10000L);
+      bgpVrf.setAutonomousSystem(10000L);
+      BgpNeighborIpv4UnicastAddressFamily ipv4af = new BgpNeighborIpv4UnicastAddressFamily();
+      bgpNeighbor.setIpv4UnicastAddressFamily(ipv4af);
+      ipv4af.setNextHopSelfAll(true);
+      assertThat(getSetNextHop(bgpNeighbor, bgpVrf), equalTo(null));
+    }
+  }
 }