FRR: Adding OSPF to BGP Redistribution Route-Map Support #6052

kylehoferamzn · 2020-08-05T20:09:21Z

No description provided.

batfish-bot · 2020-08-05T20:09:28Z

This change is

progwriter

Reviewed 11 of 11 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kylehoferamzn)

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrr_bgp.g4, line 87 at r1 (raw file):

redist_type = (STATIC | CONNECTED | OSPF)

Please pull out into a helper rule and use both places

projects/batfish/src/main/java/org/batfish/grammar/cumulus_frr/CumulusFrrConfigurationBuilder.java, line 387 at r1 (raw file):

String redistType

Do not use string as type, write a helper function to return a CumulusRoutingProtocol given the newly created rule context as input.

projects/batfish/src/main/java/org/batfish/grammar/cumulus_frr/CumulusFrrConfigurationBuilder.java, line 411 at r1 (raw file):

    BgpRedistributionPolicy oldRedistributionPolicy;

    if (_currentBgpVrf.getIpv4Unicast() == null) {

here would be the perfect place to getOrCreate an ipv4unicast family and just put the redistribution policy there.
isn't that the same as behavior of the box?

kylehoferamzn

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kylehoferamzn and @progwriter)

projects/batfish/src/main/java/org/batfish/grammar/cumulus_frr/CumulusFrrConfigurationBuilder.java, line 411 at r1 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

here would be the perfect place to getOrCreate an ipv4unicast family and just put the redistribution policy there.
isn't that the same as behavior of the box?

So I had one concern here, maybe it's invalid - but that I could end up creating a duplicate ipv4uaf vrf if ordering. Maybe it's not as big of a problem as I think?

REF:

batfish/projects/batfish/src/main/java/org/batfish/grammar/cumulus_frr/CumulusFrrConfigurationBuilder.java

Line 358 in de23e1b

    
           _w.addWarning(ctx, ctx.getText(), _parser, "duplicate 'address-family ipv4 unicast'");

kylehoferamzn

Reviewable status: 7 of 12 files reviewed, 3 unresolved discussions (waiting on @progwriter)

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrr_bgp.g4, line 87 at r1 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

redist_type = (STATIC | CONNECTED | OSPF)
Please pull out into a helper rule and use both places

I think I got this in a fairly generic way where it will work for BGP -> OSPF as well, take a gander.

projects/batfish/src/main/java/org/batfish/grammar/cumulus_frr/CumulusFrrConfigurationBuilder.java, line 387 at r1 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

String redistType
Do not use string as type, write a helper function to return a CumulusRoutingProtocol given the newly created rule context as input.

I'm not sure if I got this quite the way you wanted, I couldn't quite figure out how to pass a generic context that also had the specific context pieces I needed - I'll change it around if you have an example?

projects/batfish/src/main/java/org/batfish/grammar/cumulus_frr/CumulusFrrConfigurationBuilder.java, line 411 at r1 (raw file):

Previously, kylehoferamzn wrote…

So I had one concern here, maybe it's invalid - but that I could end up creating a duplicate ipv4uaf vrf if ordering. Maybe it's not as big of a problem as I think?

REF:

batfish/projects/batfish/src/main/java/org/batfish/grammar/cumulus_frr/CumulusFrrConfigurationBuilder.java

Line 358 in de23e1b

_w.addWarning(ctx, ctx.getText(), _parser, "duplicate 'address-family ipv4 unicast'");

Went ahead and implemented as you requested regardless of my above concern, my concern doesn't seem like a huge deal by my reading of the code.

progwriter

Reviewed 6 of 6 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kylehoferamzn)

projects/batfish/src/main/java/org/batfish/grammar/cumulus_frr/CumulusFrrConfigurationBuilder.java, line 387 at r1 (raw file):

Previously, kylehoferamzn wrote…

I'm not sure if I got this quite the way you wanted, I couldn't quite figure out how to pass a generic context that also had the specific context pieces I needed - I'll change it around if you have an example?

almost there. instead passing in bgp_redist_type().getText() and using valueOf you can just pass in cxt.bgp_redist_type() and use the if-else to check if ctx.CONNECTED() != null or ctx.OSPF() != null and so on.

projects/batfish/src/main/java/org/batfish/representation/cumulus/CumulusRoutingProtocol.java, line 20 at r2 (raw file):

  public static final Map<CumulusRoutingProtocol, Set<RoutingProtocol>> VI_PROTOCOLS_MAP =
      ImmutableMap.of(
          BGP, ImmutableSet.of(RoutingProtocol.BGP),

bgp in VI means only ebgp. what are the semantics for FRR? both bgp/ibgp or just ebgp?

kylehoferamzn

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

projects/batfish/src/main/java/org/batfish/grammar/cumulus_frr/CumulusFrrConfigurationBuilder.java, line 387 at r1 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

almost there. instead passing in bgp_redist_type().getText() and using valueOf you can just pass in cxt.bgp_redist_type() and use the if-else to check if ctx.CONNECTED() != null or ctx.OSPF() != null and so on.

Check out the new structure, I think I've gotten this the way you wanted.

One issue lingers - CumulusRoutingProtocol vs RoutingProtocol - we still need to make this conversion at some point in order to please downstream methods but I dealt with it through a valueOf lookup for now.

projects/batfish/src/main/java/org/batfish/representation/cumulus/CumulusRoutingProtocol.java, line 20 at r2 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

bgp in VI means only ebgp. what are the semantics for FRR? both bgp/ibgp or just ebgp?

Both - bgp just means bgp generically (ala Cisco). I've added iBGP here as well.

progwriter

Reviewed 2 of 3 files at r3.
Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on @kylehoferamzn and @progwriter)

projects/batfish/src/main/java/org/batfish/grammar/cumulus_frr/CumulusFrrConfigurationBuilder.java, line 387 at r1 (raw file):

Previously, kylehoferamzn wrote…

Check out the new structure, I think I've gotten this the way you wanted.

One issue lingers - CumulusRoutingProtocol vs RoutingProtocol - we still need to make this conversion at some point in order to please downstream methods but I dealt with it through a valueOf lookup for now.

why RoutingProtocol srcProtocol instead of CumulusRoutingProtocol srcProtocol? I'm missing where the VI version is used.
It should (I say optimistically) be only used during conversion, and you've already got VI_PROTOCOLS_MAP for that.

kylehoferamzn

Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on @progwriter)

projects/batfish/src/main/java/org/batfish/grammar/cumulus_frr/CumulusFrrConfigurationBuilder.java, line 387 at r1 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

why RoutingProtocol srcProtocol instead of CumulusRoutingProtocol srcProtocol? I'm missing where the VI version is used.
It should (I say optimistically) be only used during conversion, and you've already got VI_PROTOCOLS_MAP for that.

Because BgpRedistributionPolicy takes CumulusRoutingProtocol. It's a good catch - this seems to be different compared to other VS implementations. Basically we were converting to CRP just to convert back to RP later - not converting at this point should be more efficient. I'll unwind this code and see if I can get it working this more straightforward way.

kylehoferamzn

Reviewable status: 10 of 12 files reviewed, 1 unresolved discussion (waiting on @progwriter)

projects/batfish/src/main/java/org/batfish/grammar/cumulus_frr/CumulusFrrConfigurationBuilder.java, line 387 at r1 (raw file):

Previously, kylehoferamzn wrote…

Because BgpRedistributionPolicy takes CumulusRoutingProtocol. It's a good catch - this seems to be different compared to other VS implementations. Basically we were converting to CRP just to convert back to RP later - not converting at this point should be more efficient. I'll unwind this code and see if I can get it working this more straightforward way.

Okay disregard above, using CRP instead of RP makes the most sense here - did that.

progwriter

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

progwriter

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

progwriter

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

codecov · 2020-08-28T00:29:58Z

Codecov Report

Merging #6052 into master will increase coverage by 0.07%.
The diff coverage is 93.02%.

@@             Coverage Diff              @@
##             master    #6052      +/-   ##
============================================
+ Coverage     72.69%   72.76%   +0.07%     
- Complexity    34630    34705      +75     
============================================
  Files          2815     2816       +1     
  Lines        141275   141626     +351     
  Branches      16965    16985      +20     
============================================
+ Hits         102697   103054     +357     
+ Misses        30418    30407      -11     
- Partials       8160     8165       +5

Impacted Files	Coverage Δ	Complexity Δ
...ar/cumulus_frr/CumulusFrrConfigurationBuilder.java	`74.83% <88.88%> (+0.34%)`	`153.00 <8.00> (+5.00)`
...ish/representation/cumulus/CumulusConversions.java	`91.65% <92.85%> (+1.46%)`	`192.00 <2.00> (+2.00)`
...ava/org/batfish/representation/cumulus/BgpVrf.java	`97.95% <100.00%> (+0.28%)`	`28.00 <3.00> (+3.00)`
...representation/cumulus/CumulusRoutingProtocol.java	`100.00% <100.00%> (ø)`	`1.00 <0.00> (ø)`
.../representation/cumulus/CumulusStructureUsage.java	`100.00% <100.00%> (ø)`	`3.00 <0.00> (ø)`
...stion/searchroutepolicies/BgpRouteConstraints.java	`91.83% <0.00%> (-8.17%)`	`14.00% <0.00%> (+6.00%)`	⬇️
...rc/main/java/org/batfish/datamodel/flow/Trace.java	`87.09% <0.00%> (-3.23%)`	`12.00% <0.00%> (-1.00%)`
...ain/java/org/batfish/symbolic/IngressLocation.java	`65.78% <0.00%> (-2.64%)`	`15.00% <0.00%> (-1.00%)`
...col/src/main/java/org/batfish/role/InferRoles.java	`89.54% <0.00%> (-1.37%)`	`50.00% <0.00%> (-1.00%)`
...java/org/batfish/dataplane/ibdp/VirtualRouter.java	`87.64% <0.00%> (-0.15%)`	`263.00% <0.00%> (ø%)`
... and 37 more

progwriter self-requested a review August 6, 2020 21:05

progwriter reviewed Aug 7, 2020

View reviewed changes

kylehoferamzn changed the title ~~Redist rm take2~~ FRR: Adding OSPF to BGP Redistribution Route-Map Support Aug 7, 2020

kylehoferamzn commented Aug 7, 2020

View reviewed changes

progwriter reviewed Aug 14, 2020

View reviewed changes

kylehoferamzn commented Aug 14, 2020

View reviewed changes

progwriter reviewed Aug 14, 2020

View reviewed changes

kylehoferamzn commented Aug 14, 2020

View reviewed changes

progwriter reviewed Aug 14, 2020

View reviewed changes

kylehoferamzn added 8 commits August 18, 2020 15:27

FRR: OSPF to BGP redistribution route-map support

3d0af21

Fixing formatting

ca026b2

updating per comments

5afa204

Adjusting method names and formatting to better match BGP to OSPF redist

bde32ca

Fixups to code structure per comments

59da461

Fixing up formatting and BGP VI map for CRP

c3f896b

Fixing unneeded protocol conversion

5f92928

Fixing formatting

131e2b8

kylehoferamzn force-pushed the redist_rm_take2 branch from 2f003d8 to 131e2b8 Compare August 18, 2020 22:29

Fixing formatting

6218f73

progwriter reviewed Aug 19, 2020

View reviewed changes

kylehoferamzn added 2 commits August 27, 2020 15:51

Fixing up tests

dfe9977

Fixing up some test code formatting

4d03df3

progwriter reviewed Aug 28, 2020

View reviewed changes

progwriter merged commit 26eab85 into batfish:master Aug 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FRR: Adding OSPF to BGP Redistribution Route-Map Support #6052

FRR: Adding OSPF to BGP Redistribution Route-Map Support #6052

kylehoferamzn commented Aug 5, 2020

batfish-bot commented Aug 5, 2020

progwriter left a comment

kylehoferamzn left a comment

kylehoferamzn left a comment

progwriter left a comment

kylehoferamzn left a comment

progwriter left a comment

kylehoferamzn left a comment

kylehoferamzn left a comment

progwriter left a comment

progwriter left a comment

progwriter left a comment

codecov bot commented Aug 28, 2020

FRR: Adding OSPF to BGP Redistribution Route-Map Support #6052

FRR: Adding OSPF to BGP Redistribution Route-Map Support #6052

Conversation

kylehoferamzn commented Aug 5, 2020

batfish-bot commented Aug 5, 2020

progwriter left a comment

Choose a reason for hiding this comment

kylehoferamzn left a comment

Choose a reason for hiding this comment

kylehoferamzn left a comment

Choose a reason for hiding this comment

progwriter left a comment

Choose a reason for hiding this comment

kylehoferamzn left a comment

Choose a reason for hiding this comment

progwriter left a comment

Choose a reason for hiding this comment

kylehoferamzn left a comment

Choose a reason for hiding this comment

kylehoferamzn left a comment

Choose a reason for hiding this comment

progwriter left a comment

Choose a reason for hiding this comment

progwriter left a comment

Choose a reason for hiding this comment

progwriter left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 28, 2020

Codecov Report