Skip to content

Commit

Permalink
OspfRoutingProcess: fix redistribution of inter-area routes
Browse files Browse the repository at this point in the history 
Need to use the summarization filter from the source area,
not the destination area.
  • Loading branch information
@dhalperi
dhalperi committed Feb 27, 2021
1 parent 7bbbec7 commit 9cce4d2
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 20 deletions.
37 changes: 28 additions & 9 deletions projects/batfish/src/main/java/org/batfish/dataplane/ibdp/OspfRoutingProcess.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,16 @@

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.Streams;
import java.util.AbstractMap.SimpleEntry;
import java.util.Collection;
import java.util.Comparator;
import java.util.HashSet;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Objects;
import java.util.Optional;
import java.util.Queue;
Expand Down Expand Up @@ -78,6 +81,8 @@ final class OspfRoutingProcess implements RoutingProcess<OspfTopology, OspfRoute
@Nonnull private final String _vrfName;
/** The current known topology */
@Nonnull private OspfTopology _topology;
/** The area summary filter for each area, if present. */
@Nonnull private final Map<Long, RouteFilterList> _areaFilters;

/* Computed configuration & cached variables */
private final boolean _useMinMetricForSummaries;
Expand Down Expand Up @@ -132,6 +137,16 @@ final class OspfRoutingProcess implements RoutingProcess<OspfTopology, OspfRoute
_c = configuration;
_vrfName = vrfName;
_process = process;
_areaFilters =
_process.getAreas().values().stream()
.map(
a ->
Optional.ofNullable(a.getSummaryFilter())
.map(_c.getRouteFilterLists()::get)
.map(l -> new SimpleEntry<>(a.getAreaNumber(), l))
.orElse(null))
.filter(Objects::nonNull)
.collect(ImmutableMap.toImmutableMap(Entry::getKey, Entry::getValue));
_topology = topology;

_intraAreaRib = new OspfIntraAreaRib();
Expand Down Expand Up @@ -721,7 +736,7 @@ private Optional<OspfInterAreaRoute> computeSummaryRoute(
OspfInterAreaRoute summaryRoute =
OspfInterAreaRoute.builder()
.setNetwork(prefix)
.setNextHopIp(Ip.ZERO)
.setNextHop(NextHopDiscard.instance())
.setAdmin(_process.getSummaryAdminCost())
.setMetric(firstNonNull(summary.getMetric(), computedMetric))
.setArea(areaNumber)
Expand Down Expand Up @@ -883,10 +898,6 @@ to neighbors in different areas (Y, Z, ...)
filter list in batfish)
*/

RouteFilterList filterList = null;
if (areaConfig.getSummaryFilter() != null) {
filterList = _c.getRouteFilterLists().get(areaConfig.getSummaryFilter());
}
// Note: localIp == ip2
Ip nextHopIp = sessionProperties.getIpLink().getIp2();

Expand All @@ -896,7 +907,7 @@ to neighbors in different areas (Y, Z, ...)
filterInterAreaRoutesToPropagateAtABR(
delta,
areaConfig,
filterList,
_areaFilters,
sessionProperties.getIpLink().getIp1(),
nextHopIp,
_process.getMaxMetricSummaryNetworks()),
Expand All @@ -911,7 +922,8 @@ to neighbors in different areas (Y, Z, ...)
*
* @param delta ABR's inter- or intra- RIB delta
* @param areaConfig area configuration at the ABR for this neighbor adjacency
* @param filterList route filter list defined at the ABR (to enable correct summarization)
* @param areaFilters per-area map of route filter list defined at the ABR (to enable correct
* summarization)
* @param neighborIp IP of the neighbor to which we're sending the route
* @param nextHopIp next hop ip to use when creating the route.
* @param customMetric if provided (i.e., not {@code null}) it will be used instead of the routes
Expand All @@ -921,7 +933,7 @@ to neighbors in different areas (Y, Z, ...)
static Stream<RouteAdvertisement<OspfInterAreaRoute>> filterInterAreaRoutesToPropagateAtABR(
RibDelta<OspfInterAreaRoute> delta,
OspfArea areaConfig,
@Nullable RouteFilterList filterList,
Map<Long, RouteFilterList> areaFilters,
Ip neighborIp,
Ip nextHopIp,
@Nullable Long customMetric) {
Expand All @@ -938,7 +950,14 @@ static Stream<RouteAdvertisement<OspfInterAreaRoute>> filterInterAreaRoutesToPro
.filter(r -> !r.getRoute().getNextHopIp().equals(neighborIp))
// Only propagate routes permitted by the filter list
// Fail open. Treat missing filter list as "allow all".
.filter(r -> filterList == null || filterList.permits(r.getRoute().getNetwork()))
.filter(
r -> {
RouteFilterList areaSummaryFilter = areaFilters.get(r.getRoute().getArea());
if (areaSummaryFilter == null) {
return true;
}
return areaSummaryFilter.permits(r.getRoute().getNetwork());
})
// Overwrite area on the route before sending it out
.map(
r ->
Expand Down
61 changes: 50 additions & 11 deletions projects/batfish/src/test/java/org/batfish/dataplane/ibdp/OspfRoutingProcessTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@
import org.batfish.datamodel.RouteFilterList;
import org.batfish.datamodel.RoutingProtocol;
import org.batfish.datamodel.StaticRoute;
import org.batfish.datamodel.SubRange;
import org.batfish.datamodel.Vrf;
import org.batfish.datamodel.ospf.NssaSettings;
import org.batfish.datamodel.ospf.OspfArea;
Expand Down Expand Up @@ -579,7 +578,7 @@ public void testFilterInterAreaRoutesToPropagateAtABR() {
OspfInterAreaRoute route1 =
OspfInterAreaRoute.builder()
.setArea(1)
.setNetwork(Prefix.parse("1.0.0.0/8"))
.setNetwork(Prefix.parse("1.0.0.0/16"))
.setMetric(42)
.setNextHopIp(Ip.parse("8.8.8.8"))
.build();
Expand All @@ -592,7 +591,7 @@ public void testFilterInterAreaRoutesToPropagateAtABR() {
// Regular conversion
List<RouteAdvertisement<OspfInterAreaRoute>> transformed =
filterInterAreaRoutesToPropagateAtABR(
delta, AREA0_CONFIG, null, neighborIp, nextHopIp, null)
delta, AREA0_CONFIG, ImmutableMap.of(), neighborIp, nextHopIp, null)
.collect(Collectors.toList());
assertThat(
transformed,
Expand All @@ -606,7 +605,7 @@ public void testFilterInterAreaRoutesToPropagateAtABR() {
// Regular conversion but with custom metric
transformed =
filterInterAreaRoutesToPropagateAtABR(
delta, AREA0_CONFIG, null, neighborIp, nextHopIp, 999L)
delta, AREA0_CONFIG, ImmutableMap.of(), neighborIp, nextHopIp, 999L)
.collect(Collectors.toList());
assertThat(
transformed,
Expand All @@ -618,21 +617,61 @@ public void testFilterInterAreaRoutesToPropagateAtABR() {
hasMetric(999L)))));

// Convert and filter with route filter list
// - Area 1 has a summarization for 1.0.0.0/8
// - Sending routes to area 0, should block the area 0 route (same area) and
// should block the area 1 route (summarized).
transformed =
filterInterAreaRoutesToPropagateAtABR(
delta,
AREA0_CONFIG,
new RouteFilterList(
"filter",
ImmutableList.of(
new RouteFilterLine(
LineAction.DENY, Prefix.parse("1.0.0.0/8"), SubRange.singleton(8)))),
ImmutableMap.of(
1L,
new RouteFilterList(
"filter",
ImmutableList.of(
new RouteFilterLine(
LineAction.DENY,
PrefixRange.moreSpecificThan(Prefix.parse("1.0.0.0/8"))),
new RouteFilterLine(LineAction.PERMIT, PrefixRange.ALL)))),
neighborIp,
nextHopIp,
999L)
.collect(Collectors.toList());
assertThat(transformed, empty());

// Convert and filter with route filter list
// - Area 1 has a summarization for 2.0.0.0/8
// - Sending routes to area 0, should block the area 0 route (same area) and
// should permit the area 1 route.
// Note that area 0's filter denies all routes, but area 1's route will still be allowed.
transformed =
filterInterAreaRoutesToPropagateAtABR(
delta,
AREA0_CONFIG,
ImmutableMap.of(
0L,
new RouteFilterList("filter0"),
1L,
new RouteFilterList(
"filter1",
ImmutableList.of(
new RouteFilterLine(
LineAction.DENY,
PrefixRange.moreSpecificThan(Prefix.parse("2.0.0.0/8"))),
new RouteFilterLine(LineAction.PERMIT, PrefixRange.ALL)))),
neighborIp,
nextHopIp,
null)
.collect(Collectors.toList());
assertThat(
transformed,
contains(
hasRoute(
allOf(
hasPrefix(route1.getNetwork()),
hasNextHopIp(equalTo(nextHopIp)),
hasMetric(route1.getMetric())))));

// Convert but filter because STUB and suppress type 3
transformed =
filterInterAreaRoutesToPropagateAtABR(
Expand All @@ -641,7 +680,7 @@ public void testFilterInterAreaRoutesToPropagateAtABR() {
.setStub(StubSettings.builder().setSuppressType3(true).build())
.setNumber(2)
.build(),
null,
ImmutableMap.of(),
neighborIp,
nextHopIp,
null)
Expand All @@ -656,7 +695,7 @@ public void testFilterInterAreaRoutesToPropagateAtABR() {
.setNssa(NssaSettings.builder().setSuppressType3(true).build())
.setNumber(2)
.build(),
null,
ImmutableMap.of(),
neighborIp,
nextHopIp,
null)
Expand Down

0 comments on commit 9cce4d2

Please sign in to comment.