Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from May 11, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
51 changes: 34 additions & 17 deletions ...s/batfish-common-protocol/src/main/java/org/batfish/common/util/isp/IspModelingUtils.java
View file
Open in desktop
Expand Up @@ -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) {
Expand Down Expand Up @@ -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();
}

/**
Expand Down
4 changes: 2 additions & 2 deletions ...ects/batfish-common-protocol/src/main/java/org/batfish/datamodel/BgpActivePeerConfig.java
View file
Open in desktop
Expand Up @@ -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;
}
Expand Down
64 changes: 63 additions & 1 deletion ...tfish-common-protocol/src/test/java/org/batfish/common/util/isp/IspModelingUtilsTest.java
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down