Implement recursive static routes and resolution policy #6799
Conversation
- distinguish between recursive and non-recursive static routes - use resolution policy to filter which routes can be used to resolve next hops, activate static routes
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #6799 +/- ##
============================================
+ Coverage 69.86% 69.88% +0.01%
- Complexity 36222 36256 +34
============================================
Files 3026 3029 +3
Lines 153317 153421 +104
Branches 18396 18404 +8
============================================
+ Hits 107121 107215 +94
- Misses 37563 37565 +2
- Partials 8633 8641 +8 |
There was a problem hiding this comment.
Reviewed 16 of 17 files at r1.
Reviewable status: 16 of 28 files reviewed, 9 unresolved discussions (waiting on @anothermattbrown and @arifogel)
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/VirtualRouter.java, line 234 at r1 (raw file):
@Nonnull private final RibExprEvaluator _ribExprEvaluator; @Nullable private final Predicate<AnnotatedRoute<AbstractRoute>> _resolutionRestriction;
Why nullable vs default to always-true? (throughout all APIs)
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/VirtualRouter.java, line 234 at r1 (raw file):
Predicate
Probably would be a lot more clear to introduce a simple interface that we can document that is equivalent to Predicate. ResolutionPolicy? UseForResolution? etc. for example?
projects/batfish/src/main/java/org/batfish/dataplane/rib/BgpRib.java, line 300 at r1 (raw file):
return Long.MAX_VALUE; } Set<AnnotatedRoute<AbstractRoute>> s = _mainRib.longestPrefixMatch(nextHopIp, null);
TODO?
projects/batfish/src/main/java/org/batfish/dataplane/rib/Bgpv4Rib.java, line 53 at r1 (raw file):
if (shouldCheckNextHopReachability(route) .map( nextHopIp -> _mainRib != null && _mainRib.longestPrefixMatch(nextHopIp, null).isEmpty())
TODO?
projects/batfish/src/test/java/org/batfish/dataplane/rib/AbstractRibTest.java, line 416 at r1 (raw file):
b.setNetwork(Prefix.parse("1.2.3.4/8")).setNonForwarding(false).build()); // Looking for 1.2.3.4, should skip the /32 and /31, and then find the single forwarding /30
missing test of nontrivial predicate?
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/FibImpl.java, line 121 at r1 (raw file):
* @param rib {@link GenericRib} for which to do the resolution. * @param route {@link AbstractRoute} with a next hop IP to be resolved. * @param restriction A restriction on which routes may be used to
... finish sentence?
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/StaticRoute.java, line 46 at r1 (raw file):
@JsonProperty(PROP_TAG) long tag)
why exclude the new field?
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Vrf.java, line 383 at r1 (raw file):
_resolutionPolicy;
how are next-vrf routes handled? can the vrfs have different resolution policies, and if so what happens?
projects/question/src/main/java/org/batfish/question/lpmroutes/LpmRoutesAnswerer.java, line 81 at r1 (raw file):
vrfEntry.getValue().longestPrefixMatch(ip, null),
There was a problem hiding this comment.
Reviewable status: 16 of 28 files reviewed, 10 unresolved discussions (waiting on @anothermattbrown and @arifogel)
projects/batfish/src/main/java/org/batfish/dataplane/protocols/StaticRouteHelper.java, line 52 at r2 (raw file):
|| route.equals(routeToNextHop)
were you planning on addressing this one? later?
|
projects/question/src/main/java/org/batfish/question/lpmroutes/LpmRoutesAnswerer.java, line 81 at r1 (raw file): Previously, dhalperi (Dan Halperin) wrote…
plan for what? |
There was a problem hiding this comment.
Reviewable status: 6 of 29 files reviewed, 10 unresolved discussions (waiting on @anothermattbrown, @dhalperi, and @saparikh)
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/VirtualRouter.java, line 234 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
PredicateProbably would be a lot more clear to introduce a simple interface that we can document that is equivalent to
Predicate.ResolutionPolicy?UseForResolution? etc. for example?
Added ResolutionRestriction interface.
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/VirtualRouter.java, line 234 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
Why nullable vs default to always-true? (throughout all APIs)
I suppose having unimplemented callers pass in Predicates.alwaysTrue() makes the implementation simpler. Changed.
Incidentally, found a missing null check while updating.
projects/batfish/src/main/java/org/batfish/dataplane/protocols/StaticRouteHelper.java, line 52 at r2 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
|| route.equals(routeToNextHop)were you planning on addressing this one? later?
out of scope, will address later.
added TODO, though.
projects/batfish/src/main/java/org/batfish/dataplane/rib/BgpRib.java, line 300 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
TODO?
added
projects/batfish/src/main/java/org/batfish/dataplane/rib/Bgpv4Rib.java, line 53 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
TODO?
added
projects/batfish/src/test/java/org/batfish/dataplane/rib/AbstractRibTest.java, line 416 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
missing test of nontrivial predicate?
Technically true, but that function trivially calls RibTree.getLongestPrefixMatch, which is tested non-trivially.
Added something, regardless.
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/FibImpl.java, line 121 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
... finish sentence?
fixed
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/StaticRoute.java, line 46 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
@JsonProperty(PROP_TAG) long tag)why exclude the new field?
Wasn't sure it was desired. Other fields are excluded, like non-forwarding. I can add it if you think I should.
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Vrf.java, line 383 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
_resolutionPolicy;how are next-vrf routes handled? can the vrfs have different resolution policies, and if so what happens?
There are no explicit tests. I was hoping to address this in a follow on when it comes up.
I suspect this will use the current vrf for resolution, and that that is the desired behavior.
I don't know if the vendor this was originally implemented for (Juniper) ever uses next-vrf.
projects/question/src/main/java/org/batfish/question/lpmroutes/LpmRoutesAnswerer.java, line 81 at r1 (raw file):
Previously, saparikh (Samir Parikh) wrote…
plan for what?
Added TODO. This can be addressed when we converge on desired behavior.
There was a problem hiding this comment.
Reviewed 12 of 12 files at r3.
Reviewable status: 18 of 29 files reviewed, 5 unresolved discussions (waiting on @anothermattbrown, @arifogel, and @saparikh)
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/FibImpl.java, line 257 at r1 (raw file):
restriction == null
I saw you deleted the nonForwarding check. Is the implication that restriction always must include that? (So can never be TRUE?
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/StaticRoute.java, line 46 at r1 (raw file):
Previously, arifogel (Ari Fogel) wrote…
Wasn't sure it was desired. Other fields are excluded, like non-forwarding. I can add it if you think I should.
🤷 okay as-is, I guess? Non-forwarding is internal concept vs external, but maybe that's fine.
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Vrf.java, line 383 at r1 (raw file):
Previously, arifogel (Ari Fogel) wrote…
There are no explicit tests. I was hoping to address this in a follow on when it comes up.
I suspect this will use the current vrf for resolution, and that that is the desired behavior.
I don't know if the vendor this was originally implemented for (Juniper) ever uses next-vrf.
I think we need to decide that before merging this PR.
projects/question/src/main/java/org/batfish/question/lpmroutes/LpmRoutesAnswerer.java, line 81 at r1 (raw file):
Previously, arifogel (Ari Fogel) wrote…
Added TODO. This can be addressed when we converge on desired behavior.
I think we need to decide that before merging this PR.
@saparikh - the question is, what does LPM routes mean? Does it take vendor specific RIB Resolution policies into account? Does it do something else? I don't quite understand what it means today.
projects/question/src/main/java/org/batfish/question/lpmroutes/LpmRoutesAnswerer.java, line 84 at r3 (raw file):
alwaysTrue()
Not sure how this can be always true if we removed the forwarding check elsewhere.
There was a problem hiding this comment.
Reviewable status: 17 of 29 files reviewed, 4 unresolved discussions (waiting on @anothermattbrown, @arifogel, @dhalperi, and @saparikh)
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/FibImpl.java, line 257 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
restriction == nullI saw you deleted the nonForwarding check. Is the implication that restriction always must include that? (So can never be TRUE?
I didn't remove the forwarding check. It was redundant here, because that flag was already checked further down. I.e. you were guaranteed that every returned route from rib.longestPrefixMatch was a forwarding route.
projects/question/src/main/java/org/batfish/question/lpmroutes/LpmRoutesAnswerer.java, line 84 at r3 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
alwaysTrue()Not sure how this can be always true if we removed the forwarding check elsewhere.
See other comment regarding forwarding routes.
There was a problem hiding this comment.
Reviewable status: 17 of 29 files reviewed, 4 unresolved discussions (waiting on @anothermattbrown, @arifogel, @dhalperi, and @saparikh)
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/FibImpl.java, line 257 at r1 (raw file):
Previously, arifogel (Ari Fogel) wrote…
I didn't remove the forwarding check. It was redundant here, because that flag was already checked further down. I.e. you were guaranteed that every returned route from
rib.longestPrefixMatchwas a forwarding route.
Note javadoc:
/**
* Execute the longest prefix match for a given IP address.
*
* <p><strong>Note</strong>: this function returns only forwarding routes, aka, routes where
* {@link AbstractRoute#getNonForwarding()} returns false.
*
* @param address the IP address to match
* @param restriction A predicate restricting which routes may be returned. Note that in general
* this may shorten the longest prefix.
* @return a set of routes with the maximum allowable prefix length that match the {@code address}
*/
Set<R> longestPrefixMatch(Ip address, ResolutionRestriction<R> restriction);
And see implementation (in RibTree):
/**
* Returns a set of routes in this tree which 1) are forwarding routes and match restriction if
* present, 2) match the given IP address, and 3) have the longest prefix length within the
* specified maximum.
*
* <p>Returns the empty set if there are no forwarding routes that match.
*/
@Nonnull
Set<R> getLongestPrefixMatch(
Ip address, int maxPrefixLength, @Nullable Predicate<R> restriction) {
for (int pl = maxPrefixLength; pl >= 0; pl--) {
Set<R> routes = _root.longestPrefixMatch(address, pl);
if (hasAllowedForwardingRoute(routes, restriction)) {
if (onlyAllowedForwardingRoutes(routes, restriction)) {
return routes;
}
return routes.stream()
.filter(
r ->
!r.getAbstractRoute().getNonForwarding()
&& (restriction == null || restriction.test(r)))
.collect(ImmutableSet.toImmutableSet());
}
}
return ImmutableSet.of();
}
There was a problem hiding this comment.
Reviewable status: 14 of 29 files reviewed, 4 unresolved discussions (waiting on @anothermattbrown, @arifogel, @dhalperi, and @saparikh)
projects/question/src/main/java/org/batfish/question/lpmroutes/LpmRoutesAnswerer.java, line 81 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
I think we need to decide that before merging this PR.
@saparikh - the question is, what does LPM routes mean? Does it take vendor specific RIB Resolution policies into account? Does it do something else? I don't quite understand what it means today.
For context, the current situation is that this question returns protocol routes corresponding to fib entries, i.e. forwarding routes. If we are not concerned about recursive resolution of the final next hop or of resolving the next hop interface, then I believe the correct behavior is to not apply resolution policy when answering this question.
There was a problem hiding this comment.
Reviewable status: 14 of 29 files reviewed, 4 unresolved discussions (waiting on @anothermattbrown, @dhalperi, and @saparikh)
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Vrf.java, line 383 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
I think we need to decide that before merging this PR.
From my read of the code, resolution policy is no longer in scope once you have a FIB. The visitors of FibNextVrf are all working with FIB entries rather than routes. So:
- Each VRF builds a FIB based on its resolution policy. next-vrf routes still point to the next VRF.
- During forwarding, a fiblookup is done on the original vrf. If a next-vrf fib entry matches, a lookup is done in the next VRF. Resolution policy is never consulted during forwarding.
I believe the behavior it this PR is correct. I'm not sure whether there is anything to test here.
|
projects/question/src/main/java/org/batfish/question/lpmroutes/LpmRoutesAnswerer.java, line 81 at r1 (raw file): Previously, arifogel (Ari Fogel) wrote…
LPM route was meant to allow the user to see which routing table entry a specific destination IP address would match against. It is supposed to do what the name says - show the longest prefix match in a VRF routing table for the user provided IP address. It is not meant to give insight into the FIB. That is something we need to address, but is separate from the routes and LPMroutes questions. |
There was a problem hiding this comment.
Reviewed 11 of 11 files at r2, 1 of 1 files at r4, 3 of 3 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @anothermattbrown, @arifogel, and @saparikh)
projects/question/src/main/java/org/batfish/question/lpmroutes/LpmRoutesAnswerer.java, line 81 at r1 (raw file):
Previously, saparikh (Samir Parikh) wrote…
LPM route was meant to allow the user to see which routing table entry a specific destination IP address would match against. It is supposed to do what the name says - show the longest prefix match in a VRF routing table for the user provided IP address.
It is not meant to give insight into the FIB. That is something we need to address, but is separate from the routes and LPMroutes questions.
Hmm. At master, for example, it ignores non-forwarding routes (no-install, iiuc). Is that not desired?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @anothermattbrown, @dhalperi, and @saparikh)
projects/question/src/main/java/org/batfish/question/lpmroutes/LpmRoutesAnswerer.java, line 81 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
Hmm. At master, for example, it ignores non-forwarding routes (
no-install, iiuc). Is that not desired?
I cannot think of a valid use case for matching a non-forwarding route. However, traffic can be forwarded using a route that does not match resolution policy.
So the question is whether we want to return the route that would match the destination ip of a packet, or some intermediate next-hop-ip. If we care only about the former, then there is no need to apply resolution policy.
|
yes, it is supposed to ignore non-forwarding routes (which are a juniper
only concept).
…On Mon, Mar 29, 2021 at 2:26 PM Dan Halperin ***@***.***> wrote:
***@***.**** commented on this pull request.
Reviewed 11 of 11 files at r2, 1 of 1 files at r4, 3 of 3 files at r5.
*Reviewable <https://reviewable.io/reviews/batfish/batfish/6799>* status:
all files reviewed, 1 unresolved discussion (waiting on @anothermattbrown
<https://github.com/anothermattbrown>, @arifogel
<https://github.com/arifogel>, and @saparikh <https://github.com/saparikh>
)
------------------------------
*projects/question/src/main/java/org/batfish/question/lpmroutes/LpmRoutesAnswerer.java,
line 81 at r1
<https://reviewable.io/reviews/batfish/batfish/6799#-MWzQ00tEQA3P8QqyCxD:-MWzs6wa8tTTwllrCQkT:b-d13e71>
(raw file
<https://github.com/batfish/batfish/blob/3919133b098f74f4aa9fc00854d6f2f7d92c8af3/projects/question/src/main/java/org/batfish/question/lpmroutes/LpmRoutesAnswerer.java#L81>):*
*Previously, saparikh (Samir Parikh) wrote…*
LPM route was meant to allow the user to see which routing table entry a
specific destination IP address would match against. It is supposed to do
what the name says - show the longest prefix match in a VRF routing table
for the user provided IP address.
It is not meant to give insight into the FIB. That is something we need to
address, but is separate from the routes and LPMroutes questions.
Hmm. At master, for example, it ignores non-forwarding routes (no-install,
iiuc). Is that not desired?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6799 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHBAJ65OXN7ZYQJZ5AFBMBTTGDWALANCNFSM42ACBUOQ>
.
--
Samir
|
next hops, activate static routes
static route resolveandresolution rib import