OSPF distribute-list data-plane change

[haverma]

data-plane support for filtering of OSPF routes based on OSPF distribute list. More details here
distribute-list can be used to filter OSPF routes from getting installed in the main RIB.
The filtering can be done using prefix-list, route-maps or ACLs. Currently Batfish only supports prefix-list to be used in distribute-lists.
This PR applies filter on applicable OSPF routes going in the main RIB by declaring them as non-routing.

[batfish-bot]

This change is 

[codecov]

Codecov Report

Merging #3667 into master will increase coverage by 0.02%.
The diff coverage is 96.42%.

@@             Coverage Diff              @@
##             master    #3667      +/-   ##
============================================
+ Coverage     73.78%   73.81%   +0.02%     
- Complexity    24801    24830      +29     
============================================
  Files          2115     2118       +3     
  Lines        101926   101662     -264     
  Branches      12064    12010      -54     
============================================
- Hits          75210    75039     -171     
+ Misses        21348    21263      -85     
+ Partials       5368     5360       -8

Impacted Files	Coverage Δ	Complexity Δ
...org/batfish/dataplane/ibdp/OspfRoutingProcess.java	89% <96.42%> (-2.26%)	161 <15> (-80)
.../src/main/java/org/batfish/datamodel/flow/Hop.java	53.33% <0%> (-6.67%)	5% <0%> (-1%)
...g/batfish/datamodel/acl/IpAccessListLineIndex.java	33.33% <0%> (-5.56%)	4% <0%> (-1%)
...rc/main/java/org/batfish/dataplane/rib/BgpRib.java	80% <0%> (-4.71%)	44% <0%> (ø)
...ol/src/main/java/org/batfish/datamodel/Prefix.java	85.05% <0%> (-3.45%)	42% <0%> (-1%)
...l/src/main/java/org/batfish/datamodel/Prefix6.java	50.79% <0%> (-1.59%)	12% <0%> (-1%)
...col/src/main/java/org/batfish/role/InferRoles.java	91.01% <0%> (-1.18%)	69% <0%> (-1%)
...src/main/java/org/batfish/coordinator/PoolMgr.java	64.04% <0%> (-1.13%)	15% <0%> (-1%)
...in/java/org/batfish/datamodel/RoutingProtocol.java	36.21% <0%> (-0.69%)	33% <0%> (-2%)
... and 27 more

[progwriter]

Reviewed 1 of 2 files at r2.
Reviewable status: 1 of 6 files reviewed, 6 unresolved discussions (waiting on @haverma and @progwriter)

---

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/OspfRoutingProcess.java, line 267 at r2 (raw file):

  @VisibleForTesting
  static void applyDistributeList(

please javadoc

---

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/OspfRoutingProcess.java, line 271 at r2 (raw file):

    Interface iface = c.getAllInterfaces().get(ifaceName);
    assert iface != null;
    if (iface.getOspfInboundDistributeListPolicy() != null) {

flip to early exit?

---

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/OspfRoutingProcess.java, line 275 at r2 (raw file):

          c.getRoutingPolicies().get(iface.getOspfInboundDistributeListPolicy());
      assert routingPolicy != null;
      routeBuilder.setNonRouting(

Explain this boolean logic in a comment.

---

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/OspfRoutingProcess.java, line 468 at r2 (raw file):

    _interAreaIncomingRoutes.forEach(
        (edgeId, queue) -> {
          String headIfaceName = edgeId.getHead().getInterfaceName();

I think it's fine to call these just 'ifaceName' (applies other places as well) It's clear that this is the local interface.

---

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/OspfRoutingProcess.java, line 473 at r2 (raw file):

            RouteAdvertisement<OspfInterAreaRoute> routeAdvertisement = queue.remove();

            transformInterAreaRouteOnImport(routeAdvertisement, incrementalCost)

Make transformOnImport functions either apply the distribute list or return a builder. Why bother building/unbuilding multiple times? This will make the pattern consistent across the functions.

---

projects/batfish/src/test/java/org/batfish/dataplane/ibdp/OspfRoutingProcessTest.java, line 242 at r2 (raw file):

                    LineAction.DENY, PrefixRange.fromPrefix(Prefix.parse("1.1.1.0/24")))));
    c.getRouteFilterLists().put(prefixList.getName(), prefixList);
    RoutingPolicy rp = new RoutingPolicy("policy", c);

nit: use builder for routing policy

[progwriter]

Reviewed 4 of 6 files at r1, 1 of 2 files at r2.
Reviewable status: 5 of 6 files reviewed, 7 unresolved discussions (waiting on @haverma and @progwriter)

---

projects/batfish/src/test/resources/org/batfish/dataplane/ibdp/ospf-distribute-lists/configs/r1, line 20 at r3 (raw file):

ip route 192.168.15.0 255.255.255.0 172.22.1.5
ip route 192.168.16.0 255.255.255.0 172.22.1.5
ip route 0.0.0.0 0.0.0.0 GigabitEthernet0/0

why do you need this default route (and by extension the route map when redistributing static)?
Your test has no assertions on the default route, and it will be immediately blocked, right?

I could be misunderstanding the test setup.

[progwriter]

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @haverma)

[haverma]

Reviewable status: 4 of 6 files reviewed, 2 unresolved discussions (waiting on @haverma and @progwriter)

---

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/OspfRoutingProcess.java, line 267 at r2 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

please javadoc

done

---

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/OspfRoutingProcess.java, line 271 at r2 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

flip to early exit?

done

---

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/OspfRoutingProcess.java, line 275 at r2 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

Explain this boolean logic in a comment.

done

---

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/OspfRoutingProcess.java, line 468 at r2 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

I think it's fine to call these just 'ifaceName' (applies other places as well) It's clear that this is the local interface.

done

---

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/OspfRoutingProcess.java, line 473 at r2 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

Make transformOnImport functions either apply the distribute list or return a builder. Why bother building/unbuilding multiple times? This will make the pattern consistent across the functions.

that is right. Going with making transformOnImport to return builders as they will need additional parameters if they apply the distribute lists as well (thus making testing a bit more cumbersome). Also I think passing the routes to them should be enough as they are only transforming the routes.

---

projects/batfish/src/test/java/org/batfish/dataplane/ibdp/OspfRoutingProcessTest.java, line 242 at r2 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

nit: use builder for routing policy

done, thanks

[haverma]

Reviewable status: 2 of 6 files reviewed, 2 unresolved discussions (waiting on @progwriter)

---

projects/batfish/src/test/resources/org/batfish/dataplane/ibdp/ospf-distribute-lists/configs/r1, line 20 at r3 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

why do you need this default route (and by extension the route map when redistributing static)?
Your test has no assertions on the default route, and it will be immediately blocked, right?

I could be misunderstanding the test setup.

The default route is needed to get the static routes to install. If I remove it, the two static routes don't install. I think it is because there is no way to reach the next hop IP of the static routes without the default route (this is consistent with GNS3-IOS behavior). I don't want to advertise the default route so I am not asserting on it. Added a comment in the configuration for the reason of default route.

Also the route-map is needed because I want to set the type to OSPF E1 when redistributing. Otherwise if I just use redistribute static, by default it picks E2 (which is again consistent with GNS3-IOS)

[progwriter]

Reviewed 4 of 4 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @haverma)

---

projects/batfish/src/test/resources/org/batfish/dataplane/ibdp/ospf-distribute-lists/configs/r1, line 20 at r3 (raw file):

Previously, haverma (Harsh Verma) wrote…

The default route is needed to get the static routes to install. If I remove it, the two static routes don't install. I think it is because there is no way to reach the next hop IP of the static routes without the default route (this is consistent with GNS3-IOS behavior). I don't want to advertise the default route so I am not asserting on it. Added a comment in the configuration for the reason of default route.

Also the route-map is needed because I want to set the type to OSPF E1 when redistributing. Otherwise if I just use redistribute static, by default it picks E2 (which is again consistent with GNS3-IOS)

1. Any reason not to make the static routes null-routed? So for example ip route 192.168.16.0 255.255.255.0 Null0 ? then they will be installed and redistributed.
2. You are not actually setting the type in the route map though, redistribute static metric-type 1 is a valid command as far as I can tell

[haverma]

Reviewable status: 4 of 6 files reviewed, all discussions resolved (waiting on @progwriter)

---

projects/batfish/src/test/resources/org/batfish/dataplane/ibdp/ospf-distribute-lists/configs/r1, line 20 at r3 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

1. Any reason not to make the static routes null-routed? So for example ip route 192.168.16.0 255.255.255.0 Null0 ? then they will be installed and redistributed.
2. You are not actually setting the type in the route map though, redistribute static metric-type 1 is a valid command as far as I can tell

Thanks, that worked. done (1, 2)

[progwriter]

Reviewed 1 of 2 files at r5, 2 of 2 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved