BDP: implement BGP route VRF leaking #6554
Conversation
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #6554 +/- ##
============================================
- Coverage 73.36% 73.36% -0.01%
- Complexity 35666 35678 +12
============================================
Files 2837 2837
Lines 143981 144079 +98
Branches 17410 17417 +7
============================================
+ Hits 105632 105700 +68
- Misses 29970 29988 +18
- Partials 8379 8391 +12 |
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @progwriter)
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 149 at r1 (raw file):
@Nonnull private Builder<Bgpv4Route> _ebgpv4DeltaBuilder = RibDelta.builder(); /** * Keep track of redistributed routes that we have merged into our local RIB).
nit: stray parenthesis
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 261 at r1 (raw file):
_localBgpv4Rib = new Bgpv4Rib( _mainRib, bestPathTieBreaker, 1, multiPathMatchMode, true, clusterListAsIgpCost);
how come max paths should always be 1 for this rib?
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 496 at r1 (raw file):
* exists */ private RibDelta<Bgpv4Route> redistributeRouteToLocalRib(
This method doesn't really seem necessary. If you resolve the RoutingPolicy in the caller, you can call redistributeRouteToLocalRib(adv, policy) directly, plus you'll end up resolving the policy once per iteration rather than once per mainRibDelta entry.
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 526 at r1 (raw file):
.setNonRouting(true); // Hopefully, the direction should not matter here. boolean accept = policy.process(route, bgpBuilder, OUT);
It looks like the only direction-dependent routing policy clause is AutoAs, which represents this process's AS for outbound traffic and the last AS of the route for inbound traffic. So OUT seems like the better option for that case, i think. no idea why you'd want this redistribution policy to mess with the AS path anyway
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 736 at r1 (raw file):
Bgpv4Route remoteRoute = remoteRouteAdvert.getRoute(); // LOGGER.debug("{} Processing bgpv4 route {}", _hostname, remoteRoute);
uncomment or remove
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 1701 at r1 (raw file):
if (importPolicyName != null) { accept = _policies.getOrThrow(importPolicyName).processBgpRoute(route, builder, null, IN);
here as well, why not resolve importPolicy outside the forEach?
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/VirtualRouter.java, line 1289 at r1 (raw file):
String importFromVrf = leakConfig.getImportFromVrf(); VirtualRouter exportingVR = _node.getVirtualRouterOrThrow(importFromVrf); CrossVrfEdgeId otherVrfToOurRib = new CrossVrfEdgeId(importFromVrf, RibId.DEFAULT_RIB_NAME);
why move this stuff before short-circuit if?
projects/batfish/src/main/java/org/batfish/dataplane/rib/Bgpv4Rib.java, line 44 at r1 (raw file):
However, due to some complications with how we create routes, we must skip this check for: - routes with link-local address as next hop (i.e., next-hop interface is set to something) - routes with Ip.AUTO as next hop or protocol AGGREGATE (for locally-generated routes/aggregates)
why did this change (why should we now avoid merging routes with next hop Ip.AUTO)?
projects/batfish/src/test/java/org/batfish/dataplane/ibdp/BgpRoutingProcessTest.java, line 568 at r1 (raw file):
// Fake up end of round before other test _routingProcess.endOfRound();
why is this necessary? shouldn't the routing process be unaffected by the above redistribution since the policy denies the route?
projects/batfish/src/test/java/org/batfish/dataplane/ibdp/BgpRoutingProcessTest.java, line 582 at r1 (raw file):
_routingProcess.getV4LocalRoutes(), contains( isBgpv4RouteThat(
Better to create the complete expected bgp route to make sure all the fields are accounted for correctly. Eg this ought to be nonrouting, right?
Same for cases in the next test where a route is successfully imported.
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/BgpProcess.java, line 421 at r1 (raw file):
} public void setRedistributionPolicy(@Nullable String redistributionPolicy) {
ugh, is it really necessary for _redistributionPolicy not to be final?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @corinaminer and @progwriter)
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 149 at r1 (raw file):
Previously, corinaminer (Corina Miner) wrote…
nit: stray parenthesis
done
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 261 at r1 (raw file):
Previously, corinaminer (Corina Miner) wrote…
how come max paths should always be 1 for this rib?
good call, fixed
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 496 at r1 (raw file):
Previously, corinaminer (Corina Miner) wrote…
This method doesn't really seem necessary. If you resolve the
RoutingPolicyin the caller, you can callredistributeRouteToLocalRib(adv, policy)directly, plus you'll end up resolving the policy once per iteration rather than once permainRibDeltaentry.
sure, simplified.
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 1701 at r1 (raw file):
Previously, corinaminer (Corina Miner) wrote…
here as well, why not resolve
importPolicyoutside theforEach?
done
projects/batfish/src/main/java/org/batfish/dataplane/rib/Bgpv4Rib.java, line 44 at r1 (raw file):
Previously, corinaminer (Corina Miner) wrote…
why did this change (why should we now avoid merging routes with next hop
Ip.AUTO)?
After #6545 Ip.AUTO is not a valid next hop ip for routes
projects/batfish/src/test/java/org/batfish/dataplane/ibdp/BgpRoutingProcessTest.java, line 568 at r1 (raw file):
Previously, corinaminer (Corina Miner) wrote…
why is this necessary? shouldn't the routing process be unaffected by the above redistribution since the policy denies the route?
not really, because process state is still modified e.g., the _toRedistribute map for old-style redistribution
projects/batfish/src/test/java/org/batfish/dataplane/ibdp/BgpRoutingProcessTest.java, line 582 at r1 (raw file):
Previously, corinaminer (Corina Miner) wrote…
Better to create the complete expected bgp route to make sure all the fields are accounted for correctly. Eg this ought to be nonrouting, right?
Same for cases in the next test where a route is successfully imported.
I actually do not care to test properties that aren't crucial, that makes tests harder to maintain. Good call on the non-routing though.
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/BgpProcess.java, line 421 at r1 (raw file):
Previously, corinaminer (Corina Miner) wrote…
ugh, is it really necessary for
_redistributionPolicynot to be final?
:)
while i share the same feeling, I wasn't about to start rewriting a bunch of vendor conversion code
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @progwriter)
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 261 at r1 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
good call, fixed
all the other ribs have _process.getMultipathIbgp() ? null : 1 -- is that not appropriate here?
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 481 at r2 (raw file):
// Place redistributed routes into our local RIB String policyName = _process.getRedistributionPolicy(); assert policyName != null;
this was just confirmed in the if condition. not even really necessary to define policyName if checkstyle is giving you warnings
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/BgpProcess.java, line 421 at r1 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
:)
while i share the same feeling, I wasn't about to start rewriting a bunch of vendor conversion code
understandable 😞
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 261 at r1 (raw file):
Previously, corinaminer (Corina Miner) wrote…
all the other ribs have
_process.getMultipathIbgp() ? null : 1-- is that not appropriate here?
my understanding is that those knobs are for received e/i bgp routes, local rib will keep all redistributed routes.
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 481 at r2 (raw file):
Previously, corinaminer (Corina Miner) wrote…
this was just confirmed in the if condition. not even really necessary to define
policyNameif checkstyle is giving you warnings
done
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status:
Implements cisco-style VRF leaking using BGP routes in our dataplane.
To do so, we
BgpRoutingProcesses (if configured, of course)NextHopVrfnext hop to ensure that FIBs are built correctly and packets hop VRFs during forwarding.