juniper conversion for 'from condition' #6749

arifogel · 2021-03-18T21:56:13Z

add route policy primitives for getting main rib routes
add route policy primitives for checking presence of another route in
the main rib matching some condition
implement conversion for juniper 'from condition' based on new route
policy primitives

- add route policy primitives for getting main rib routes - add route policy primitives for checking presence of another route in the main rib matching some condition - implement conversion for juniper 'from condition' based on new route policy primitives

batfish-bot · 2021-03-18T21:56:20Z

This change is

codecov · 2021-03-18T23:14:36Z

Codecov Report

Merging #6749 (f186455) into master (90f3c44) will increase coverage by 0.13%.
The diff coverage is 93.10%.

@@             Coverage Diff              @@
##             master    #6749      +/-   ##
============================================
+ Coverage     69.51%   69.65%   +0.13%     
- Complexity    35916    36081     +165     
============================================
  Files          3011     3020       +9     
  Lines        153079   153262     +183     
  Branches      18402    18428      +26     
============================================
+ Hits         106420   106751     +331     
+ Misses        38065    37900     -165     
- Partials       8594     8611      +17

Impacted Files	Coverage Δ	Complexity Δ
...atfish/datamodel/routing_policy/RoutingPolicy.java	`93.40% <83.33%> (-0.92%)`	`31.00 <3.00> (+1.00)`	⬇️
.../org/batfish/dataplane/ibdp/BgpRoutingProcess.java	`84.92% <83.33%> (+0.01%)`	`151.00 <0.00> (ø)`
.../routing_policy/expr/RibIntersectsPrefixSpace.java	`86.95% <86.95%> (ø)`	`12.00 <12.00> (?)`
...java/org/batfish/datamodel/PrefixTrieMultiMap.java	`85.95% <91.66%> (+0.41%)`	`55.00 <1.00> (+1.00)`
...del/bgp/community/CommunityStructuresVerifier.java	`96.70% <100.00%> (+0.05%)`	`13.00 <0.00> (ø)`
.../batfish/datamodel/routing_policy/Environment.java	`87.64% <100.00%> (+0.28%)`	`42.00 <1.00> (+1.00)`
...tamodel/routing_policy/expr/ExplicitPrefixSet.java	`61.53% <100.00%> (+9.53%)`	`8.00 <1.00> (+2.00)`
...batfish/datamodel/routing_policy/expr/MainRib.java	`100.00% <100.00%> (ø)`	`6.00 <6.00> (?)`
...tfish/datamodel/visitors/PrefixSpaceEvaluator.java	`100.00% <100.00%> (ø)`	`4.00 <4.00> (?)`
...org/batfish/dataplane/ibdp/OspfRoutingProcess.java	`92.87% <100.00%> (ø)`	`188.00 <0.00> (ø)`
... and 58 more

dhalperi

Reviewed 24 of 26 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @anothermattbrown and @arifogel)

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/HasMatchingRoute.java, line 46 at r2 (raw file):

    Collection<AbstractRoute> routeCandidates =
        _routesExpr.accept(RoutesExprEvaluator.instance(), environment);

This feels really heavyweight - build a collection of all the routes in the main RIB as a precursor to any work, when the most typical (only known?) is "is this prefix present". Is there a better way?

- provide RibExpr instead of RoutesExpr, which is evaluated by data plane code - instead of testing each rib route, check for for intersection of Rib and PrefixSpace

arifogel

Reviewable status: 3 of 36 files reviewed, 1 unresolved discussion (waiting on @anothermattbrown and @dhalperi)

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/HasMatchingRoute.java, line 46 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

    Collection<AbstractRoute> routeCandidates =
        _routesExpr.accept(RoutesExprEvaluator.instance(), environment);
This feels really heavyweight - build a collection of all the routes in the main RIB as a precursor to any work, when the most typical (only known?) is "is this prefix present". Is there a better way?

Now GenericRib has function to check intersection with a PrefixSpace, which should be much more efficient.

anothermattbrown

Reviewed 2 of 37 files at r3.
Reviewable status: 5 of 36 files reviewed, 1 unresolved discussion (waiting on @dhalperi)

anothermattbrown

Reviewable status: 5 of 36 files reviewed, 1 unresolved discussion (waiting on @dhalperi)

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/PrefixTrieMultiMap.java, line 254 at r3 (raw file):

return true.

probably worth documenting here that this this node has any elements means _prefix is a key.

dhalperi

Reviewed 35 of 37 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @arifogel)

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/GenericRibReadOnly.java, line 56 at r3 (raw file):

 to routes in this rib

Is it worth clarifying what we mean by this? Active routes, installed routes, forwarding routes, routing routes, etc?

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/route/nh/NextHop.java, line 19 at r3 (raw file):

revert?

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/RoutingPolicy.java, line 243 at r3 (raw file):

public boolean processBgpRoute(

kill this one? When we have a ton of them, it just leads to us using the wrong one over and over again.

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/RibIntersectsPrefixSpace.java, line 50 at r3 (raw file):

Cannot check RIB routes outside of data plane generation

Maybe something that's mildly more user-facing (just less specific to Batfish internals), such as "Cannot check RIB routes; main RIB state is not available at this time"

Also would be robust to code where we call the wrong process function and forget to build environment correctly.

projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/MockRib.java, line 83 at r3 (raw file):

Quoted 7 lines of code…

    return !new PrefixSpace(
            _routes.stream()
                .map(AnnotatedRoute::getNetwork)
                .map(PrefixRange::fromPrefix)
                .collect(ImmutableList.toImmutableList()))
        .intersection(prefixSpace)
        .isEmpty();

optional: can this not be moved inside the stream / no collection needed?

Also: PrefixSpace has overlaps for intersection!empty and also has containsPrefix which is maybe even simpler?

arifogel

Reviewable status: 30 of 39 files reviewed, 3 unresolved discussions (waiting on @anothermattbrown and @dhalperi)

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/GenericRibReadOnly.java, line 56 at r3 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

 to routes in this rib
Is it worth clarifying what we mean by this? Active routes, installed routes, forwarding routes, routing routes, etc?

It means any routes in this rib. Is there a need to clarify when there is no restriction?

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/PrefixTrieMultiMap.java, line 254 at r3 (raw file):

Previously, anothermattbrown (Matt Brown) wrote…

return true.
probably worth documenting here that this this node has any elements means _prefix is a key.

done

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/route/nh/NextHop.java, line 19 at r3 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

4 
revert?

done

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/RoutingPolicy.java, line 243 at r3 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

public boolean processBgpRoute(
kill this one? When we have a ton of them, it just leads to us using the wrong one over and over again.

ok. i feel like more cleanup is warranted, but stopping at removing this one for now.

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/RibIntersectsPrefixSpace.java, line 50 at r3 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Cannot check RIB routes outside of data plane generation
Maybe something that's mildly more user-facing (just less specific to Batfish internals), such as "Cannot check RIB routes; main RIB state is not available at this time"

Also would be robust to code where we call the wrong process function and forget to build environment correctly.

changed

projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/MockRib.java, line 83 at r3 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

    return !new PrefixSpace(
            _routes.stream()
                .map(AnnotatedRoute::getNetwork)
                .map(PrefixRange::fromPrefix)
                .collect(ImmutableList.toImmutableList()))
        .intersection(prefixSpace)
        .isEmpty();
optional: can this not be moved inside the stream / no collection needed?

Also: PrefixSpace has overlaps for intersection!empty and also has containsPrefix which is maybe even simpler?

simplified

dhalperi

Reviewed 10 of 10 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @arifogel)

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 821 at r4 (raw file):

          acceptIncoming =
              importPolicy.processBgpRoute(
                  remoteRoute, transformedIncomingRouteBuilder, sessionProperties, IN, null);

So now I'm confused which parts of the pipeline need the evaluator. Isn't importing routes from peers the main one?

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/GenericRibReadOnly.java, line 56 at r3 (raw file):

Previously, arifogel (Ari Fogel) wrote…

It means any routes in this rib. Is there a need to clarify when there is no restriction?

I guess it depends on what "in" means? The RIB has transitive pointers to backup routes, non-forwarding routes, (maybe?) non-routing routes, etc.

arifogel

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @arifogel and @dhalperi)

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 821 at r4 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

So now I'm confused which parts of the pipeline need the evaluator. Isn't importing routes from peers the main one?

On XR, any part of the pipeline can use the evaluator. Seems the most common is generating default routes for various protocols.

arifogel

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @arifogel and @dhalperi)

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 821 at r4 (raw file):

Previously, arifogel (Ari Fogel) wrote…

On XR, any part of the pipeline can use the evaluator. Seems the most common is generating default routes for various protocols.

..but on Juniper, I think it's really any part that uses a policy.

dhalperi

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @arifogel and @dhalperi)

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/GenericRibReadOnly.java, line 56 at r3 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

I guess it depends on what "in" means? The RIB has transitive pointers to backup routes, non-forwarding routes, (maybe?) non-routing routes, etc.

Offline discussion; "in the rib" means "returned by {@link #getRoutes}"

arifogel

Reviewable status: 38 of 39 files reviewed, 2 unresolved discussions (waiting on @dhalperi)

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/GenericRibReadOnly.java, line 56 at r3 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Offline discussion; "in the rib" means "returned by {@link #getRoutes}"

updated

dhalperi

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dhalperi)

dhalperi

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

arifogel

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

arifogel requested review from dhalperi and anothermattbrown March 18, 2021 21:56

arifogel added 2 commits March 18, 2021 15:28

javadoc

2b970aa

pmd

d8b5092

dhalperi reviewed Mar 19, 2021

View reviewed changes

arifogel added 2 commits March 19, 2021 16:59

alternate strategy just handling route prefixes

6e00d80

- provide RibExpr instead of RoutesExpr, which is evaluated by data plane code - instead of testing each rib route, check for for intersection of Rib and PrefixSpace

remove crap

eeb797b

arifogel commented Mar 20, 2021

View reviewed changes

anothermattbrown reviewed Mar 22, 2021

View reviewed changes

dhalperi reviewed Mar 22, 2021

View reviewed changes

review

e974968

arifogel commented Mar 22, 2021

View reviewed changes

dhalperi reviewed Mar 22, 2021

View reviewed changes

arifogel commented Mar 22, 2021

View reviewed changes

dhalperi reviewed Mar 22, 2021

View reviewed changes

clarify

c40b7f5

arifogel commented Mar 22, 2021

View reviewed changes

dhalperi reviewed Mar 22, 2021

View reviewed changes

always pass evaluator in BgpRoutingProcess

cb28d07

dhalperi reviewed Mar 22, 2021

View reviewed changes

test

f186455

arifogel commented Mar 22, 2021

View reviewed changes

arifogel merged commit 91537bc into batfish:master Mar 22, 2021

arifogel deleted the ari-has-matching-route branch March 22, 2021 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

juniper conversion for 'from condition' #6749

juniper conversion for 'from condition' #6749

arifogel commented Mar 18, 2021

batfish-bot commented Mar 18, 2021

codecov bot commented Mar 18, 2021 •

edited

Loading

dhalperi left a comment

arifogel left a comment

anothermattbrown left a comment

anothermattbrown left a comment

dhalperi left a comment

arifogel left a comment

dhalperi left a comment

arifogel left a comment

arifogel left a comment

dhalperi left a comment

arifogel left a comment

dhalperi left a comment

dhalperi left a comment

arifogel left a comment

juniper conversion for 'from condition' #6749

juniper conversion for 'from condition' #6749

Conversation

arifogel commented Mar 18, 2021

batfish-bot commented Mar 18, 2021

codecov bot commented Mar 18, 2021 • edited Loading

Codecov Report

dhalperi left a comment

Choose a reason for hiding this comment

arifogel left a comment

Choose a reason for hiding this comment

anothermattbrown left a comment

Choose a reason for hiding this comment

anothermattbrown left a comment

Choose a reason for hiding this comment

dhalperi left a comment

Choose a reason for hiding this comment

arifogel left a comment

Choose a reason for hiding this comment

dhalperi left a comment

Choose a reason for hiding this comment

arifogel left a comment

Choose a reason for hiding this comment

arifogel left a comment

Choose a reason for hiding this comment

dhalperi left a comment

Choose a reason for hiding this comment

arifogel left a comment

Choose a reason for hiding this comment

dhalperi left a comment

Choose a reason for hiding this comment

dhalperi left a comment

Choose a reason for hiding this comment

arifogel left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 18, 2021 •

edited

Loading