IspModelingUtils: add explanations when ISP-side generation fails (#8303

)

Help users debug ISP configuration, and also document current gaps, which
we are in the process of fixing.

commit-id:ef80c576

projects/batfish-common-protocol/src/main/java/org/batfish/common/util/isp/IspModelingUtils.java

@@ -535,11 +535,13 @@ static Optional<SnapshotConnection> getSnapshotConnectionForBgpPeerInfo(
       return Optional.empty();
     }
     BgpActivePeerConfig snapshotBgpPeer = snapshotBgpPeerOpt.get();
-    if (!isValidBgpPeerConfigForBgpPeerInfo(snapshotBgpPeer, remoteIps, remoteAsns)) {
+    Optional<String> invalidReason =
+        validateOrExplainProblemCreatingIspConfig(snapshotBgpPeer, remoteIps, remoteAsns);
+    if (invalidReason.isPresent()) {
       warnings.redFlag(
           String.format(
-              "ISP Modeling: BGP neighbor %s on node %s is invalid.",
-              bgpPeerInfo.getPeerAddress(), bgpPeerInfo.getHostname()));
+              "ISP Modeling: BGP neighbor %s on node %s is invalid: %s.",
+              bgpPeerInfo.getPeerAddress(), bgpPeerInfo.getHostname(), invalidReason.get()));
       return Optional.empty();
     }
     if (bgpPeerInfo.getIspAttachment() == null) {
@@ -1231,22 +1233,37 @@ && getSessionType(bgpPeerConfig) == EBGP_SINGLEHOP) {
     return Optional.empty();
   }
 
+  /**
+   * Determines whether the ISP for the given BGP Peer is allowed by filters and has enough
+   * information to generate, or explain the reason why this cannot happen.
+   */
   @VisibleForTesting
-  static boolean isValidBgpPeerConfigForBgpPeerInfo(
+  static Optional<String> validateOrExplainProblemCreatingIspConfig(
       BgpActivePeerConfig bgpPeerConfig, Set<Ip> allowedRemoteIps, LongSpace allowedRemoteAsns) {
-    return Objects.nonNull(bgpPeerConfig.getLocalAs()) // local AS is defined
-        && Objects.nonNull(
-            bgpPeerConfig.getLocalIp()) // local IP is defined -- used as peer address on ISP
-        && !bgpPeerConfig
-            .getRemoteAsns()
-            .equals(LongSpace.of(bgpPeerConfig.getLocalAs())) // not iBGP
-        && !allowedRemoteAsns
-            .intersection(bgpPeerConfig.getRemoteAsns())
-            .isEmpty() // remote AS is defined and allowed
-        && Objects.nonNull(bgpPeerConfig.getPeerAddress()) // peer address is defined
-        && (allowedRemoteIps.isEmpty()
-            || allowedRemoteIps.contains(
-                bgpPeerConfig.getPeerAddress())); // peer address is allowed
+    if (bgpPeerConfig.getLocalAs() == null) {
+      return Optional.of("unable to determine local AS");
+    } else if (bgpPeerConfig.getRemoteAsns().equals(LongSpace.of(bgpPeerConfig.getLocalAs()))) {
+      return Optional.of("iBGP peers are not supported");
+    } else if (bgpPeerConfig.getRemoteAsns().isEmpty()) {
+      return Optional.of("unable to determine remote AS");
+    } else if (allowedRemoteAsns.intersection(bgpPeerConfig.getRemoteAsns()).isEmpty()) {
+      return Optional.of(
+          String.format(
+              "remote AS %s is not allowed by the filter", bgpPeerConfig.getRemoteAsns()));
+    } else if (bgpPeerConfig.getPeerAddress() == null) {
+      return Optional.of("remote IP is not configured");
+    } else if (!allowedRemoteIps.isEmpty()
+        && !allowedRemoteIps.contains(bgpPeerConfig.getPeerAddress())) {
+      return Optional.of(
+          String.format(
+              "remote IP %s is not allowed by the filter", bgpPeerConfig.getPeerAddress()));
+    } else if (bgpPeerConfig.getLocalIp() == null) {
+      return Optional.of(
+          "unable to determine local IP address. Note that Batfish ISP config today "
+              + "requires local IP or update source to be configured explicitly in order "
+              + "to determine the ISP's BGP configuration");
+    }
+    return Optional.empty();
   }
 
   /**

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/BgpActivePeerConfig.java

@@ -162,8 +162,8 @@ public BgpActivePeerConfig build() {
               _remoteAsns,
               _ipv4UnicastAddressFamily,
               _evpnAddressFamily);
-      if (_bgpProcess != null) {
-        _bgpProcess.getActiveNeighbors().put(Objects.requireNonNull(_peerAddress), bgpPeerConfig);
+      if (_bgpProcess != null && _peerAddress != null) {
+        _bgpProcess.getActiveNeighbors().put(_peerAddress, bgpPeerConfig);
       }
       return bgpPeerConfig;
     }

projects/batfish-common-protocol/src/test/java/org/batfish/common/util/isp/IspModelingUtilsTest.java

@@ -40,6 +40,7 @@
 import static org.batfish.common.util.isp.IspModelingUtils.ispToSnapshotInterfaceName;
 import static org.batfish.common.util.isp.IspModelingUtils.makeBgpProcess;
 import static org.batfish.common.util.isp.IspModelingUtils.toIspModel;
+import static org.batfish.common.util.isp.IspModelingUtils.validateOrExplainProblemCreatingIspConfig;
 import static org.batfish.datamodel.BgpPeerConfig.ALL_AS_NUMBERS;
 import static org.batfish.datamodel.BgpProcess.testBgpProcess;
 import static org.batfish.datamodel.Configuration.DEFAULT_VRF_NAME;
@@ -98,6 +99,7 @@
 import org.batfish.datamodel.InterfaceAddress;
 import org.batfish.datamodel.Ip;
 import org.batfish.datamodel.IpAccessList;
+import org.batfish.datamodel.LongSpace;
 import org.batfish.datamodel.NetworkFactory;
 import org.batfish.datamodel.OriginType;
 import org.batfish.datamodel.Prefix;
@@ -1003,7 +1005,67 @@ public void testGetSnapshotConnectionForBgpPeerInfo_invalidBgpPeer() {
     assertFalse(connection.isPresent());
     assertThat(
         warnings.getRedFlagWarnings(),
-        contains(hasText("ISP Modeling: BGP neighbor 0.0.0.0 on node conf is invalid.")));
+        contains(
+            hasText(
+                "ISP Modeling: BGP neighbor 0.0.0.0 on node conf is invalid: unable to determine"
+                    + " remote AS.")));
+  }
+
+  /** Helper to generate a peer for which ISP can be generated. */
+  private BgpActivePeerConfig.Builder correctBuilder() {
+    return BgpActivePeerConfig.builder()
+        .setPeerAddress(_ispIp)
+        .setLocalIp(_snapshotIp)
+        .setLocalAs(_snapshotAsn)
+        .setRemoteAs(_ispAsn)
+        .setIpv4UnicastAddressFamily(Ipv4UnicastAddressFamily.builder().build())
+        .setBgpProcess(_snapshotHost.getDefaultVrf().getBgpProcess());
+  }
+
+  @Test
+  public void testValidateOrExplainProblemCreatingIspConfig() {
+    assertThat(
+        validateOrExplainProblemCreatingIspConfig(
+            correctBuilder().build(), ImmutableSet.of(), ALL_AS_NUMBERS),
+        equalTo(Optional.empty()));
+
+    assertThat(
+        validateOrExplainProblemCreatingIspConfig(
+                correctBuilder().setLocalAs(null).build(), ImmutableSet.of(), ALL_AS_NUMBERS)
+            .get(),
+        containsString("unable to determine local AS"));
+    assertThat(
+        validateOrExplainProblemCreatingIspConfig(
+                correctBuilder().setLocalAs(_ispAsn).build(), ImmutableSet.of(), ALL_AS_NUMBERS)
+            .get(),
+        containsString("iBGP peers are not supported"));
+    assertThat(
+        validateOrExplainProblemCreatingIspConfig(
+                correctBuilder().setRemoteAsns(LongSpace.EMPTY).build(),
+                ImmutableSet.of(),
+                ALL_AS_NUMBERS)
+            .get(),
+        containsString("unable to determine remote AS"));
+    assertThat(
+        validateOrExplainProblemCreatingIspConfig(
+                correctBuilder().build(), ImmutableSet.of(), LongSpace.of(_ispAsn + 3))
+            .get(),
+        containsString("remote AS 1 is not allowed by the filter"));
+    assertThat(
+        validateOrExplainProblemCreatingIspConfig(
+                correctBuilder().setPeerAddress(null).build(), ImmutableSet.of(), ALL_AS_NUMBERS)
+            .get(),
+        containsString("remote IP is not configured"));
+    assertThat(
+        validateOrExplainProblemCreatingIspConfig(
+                correctBuilder().build(), ImmutableSet.of(Ip.ZERO), ALL_AS_NUMBERS)
+            .get(),
+        containsString("remote IP " + _ispIp + " is not allowed by the filter"));
+    assertThat(
+        validateOrExplainProblemCreatingIspConfig(
+                correctBuilder().setLocalIp(null).build(), ImmutableSet.of(), ALL_AS_NUMBERS)
+            .get(),
+        containsString("unable to determine local IP address"));
   }
 
   @Test