From 1cf4737fc2534fed4e4544d516bf5c52a3a943b0 Mon Sep 17 00:00:00 2001 From: Dan Halperin Date: Wed, 11 May 2022 10:21:55 -0700 Subject: [PATCH] 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 --- .../common/util/isp/IspModelingUtils.java | 51 ++++++++++----- .../datamodel/BgpActivePeerConfig.java | 4 +- .../common/util/isp/IspModelingUtilsTest.java | 64 ++++++++++++++++++- 3 files changed, 99 insertions(+), 20 deletions(-) diff --git a/projects/batfish-common-protocol/src/main/java/org/batfish/common/util/isp/IspModelingUtils.java b/projects/batfish-common-protocol/src/main/java/org/batfish/common/util/isp/IspModelingUtils.java index 34f7ab514b0..a9359e70e32 100644 --- a/projects/batfish-common-protocol/src/main/java/org/batfish/common/util/isp/IspModelingUtils.java +++ b/projects/batfish-common-protocol/src/main/java/org/batfish/common/util/isp/IspModelingUtils.java @@ -535,11 +535,13 @@ static Optional getSnapshotConnectionForBgpPeerInfo( return Optional.empty(); } BgpActivePeerConfig snapshotBgpPeer = snapshotBgpPeerOpt.get(); - if (!isValidBgpPeerConfigForBgpPeerInfo(snapshotBgpPeer, remoteIps, remoteAsns)) { + Optional 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 validateOrExplainProblemCreatingIspConfig( BgpActivePeerConfig bgpPeerConfig, Set 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(); } /** diff --git a/projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/BgpActivePeerConfig.java b/projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/BgpActivePeerConfig.java index d3ac1250fcb..00842350aa3 100644 --- a/projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/BgpActivePeerConfig.java +++ b/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; } diff --git a/projects/batfish-common-protocol/src/test/java/org/batfish/common/util/isp/IspModelingUtilsTest.java b/projects/batfish-common-protocol/src/test/java/org/batfish/common/util/isp/IspModelingUtilsTest.java index 61597688c6c..6d9484aaf15 100644 --- a/projects/batfish-common-protocol/src/test/java/org/batfish/common/util/isp/IspModelingUtilsTest.java +++ b/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