Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cisco prefix-lists for BGP exports -> VI model #3341

Merged
merged 5 commits into from
Mar 6, 2019
Merged

Conversation

corinaminer
Copy link
Contributor

Adds a MatchPrefixList to BGP export policy for peers with an export prefix-list configured. Until now, those prefix-lists were extracted but ignored.

Follow-up work:

  • Get the prefix-list name to appear in the generated export policy's sources
  • Support peers that use both an outbound route-map and an outbound prefix-list (right now we use the route-map and ignore the prefix-list)

@batfish-bot
Copy link

This change is Reviewable

Copy link
Contributor Author

@corinaminer corinaminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


projects/batfish/src/main/java/org/batfish/representation/cisco/CiscoConfiguration.java, line 1923 at r1 (raw file):

                new MatchPrefixSet(
                    DestinationNetwork.instance(), new NamedPrefixSet(outboundPrefixListName)));
      }

Once this is reviewed, i'll move the export policy generation logic into a CiscoConversions method. for now I kept it here to make the diff more useful.

@codecov
Copy link

codecov bot commented Mar 6, 2019

Codecov Report

Merging #3341 into master will increase coverage by 0.01%.
The diff coverage is 89.83%.

@@             Coverage Diff              @@
##             master    #3341      +/-   ##
============================================
+ Coverage     72.86%   72.88%   +0.01%     
+ Complexity    23350    23324      -26     
============================================
  Files          2032     2028       -4     
  Lines         98122    98027      -95     
  Branches      11731    11714      -17     
============================================
- Hits          71499    71444      -55     
+ Misses        21399    21377      -22     
+ Partials       5224     5206      -18
Impacted Files Coverage Δ Complexity Δ
...batfish/representation/cisco/CiscoConversions.java 89.96% <88.37%> (-0.15%) 134 <10> (+10)
...tfish/representation/cisco/CiscoConfiguration.java 82.88% <93.75%> (-0.36%) 509 <5> (-4)
...ipsecsessionstatus/IpsecSessionStatusQuestion.java 70.58% <0%> (-15.13%) 6% <0%> (ø)
...specifier/InterfaceNameRegexLocationSpecifier.java 53.33% <0%> (-13.34%) 4% <0%> (-1%)
...tfish/specifier/VrfNameRegexLocationSpecifier.java 53.33% <0%> (-13.34%) 4% <0%> (-1%)
...er/InterfaceDescriptionRegexLocationSpecifier.java 53.33% <0%> (-13.34%) 4% <0%> (-1%)
...uestion/comparefilters/CompareFiltersQuestion.java 81.25% <0%> (-11.06%) 7% <0%> (+1%)
...tocol/src/main/java/org/batfish/role/NodeRole.java 57.14% <0%> (-9.53%) 11% <0%> (ø)
...rg/batfish/datamodel/questions/NodesSpecifier.java 71.31% <0%> (-7.38%) 37% <0%> (-2%)
...n/definedstructures/DefinedStructuresQuestion.java 71.42% <0%> (-7.15%) 6% <0%> (ø)
... and 49 more

Copy link
Contributor

@progwriter progwriter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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

Copy link
Contributor Author

@corinaminer corinaminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

a discussion (no related file):
pending tests for CiscoConversions.generateBgpExportPolicy()


Copy link
Contributor

@progwriter progwriter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


projects/batfish/src/main/java/org/batfish/representation/cisco/CiscoConfiguration.java, line 1896 at r3 (raw file):

      // Generate import and export policies
      String peerImportPolicyName = generateBgpImportPolicy(lpg, vrfName, c, _w);
      generateBgpExportPolicy(lpg, vrfName, c, _w);

why does this not return the name (for symmetry with import)?


projects/batfish/src/main/java/org/batfish/representation/cisco/CiscoConfiguration.java, line 1898 at r3 (raw file):

      generateBgpExportPolicy(lpg, vrfName, c, _w);

      // set up default export policy for this peer group

"default route" to disambiguate which default?


projects/batfish/src/main/java/org/batfish/representation/cisco/CiscoConfiguration.java, line 1902 at r3 (raw file):

      GeneratedRoute6.Builder defaultRoute6;
      if (lpg.getDefaultOriginate()) {
        String defaultRouteExportPolicyName =

if you can move this to compute... that'd be great

Copy link
Contributor Author

@corinaminer corinaminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


projects/batfish/src/main/java/org/batfish/representation/cisco/CiscoConfiguration.java, line 1896 at r3 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

why does this not return the name (for symmetry with import)?

the name is guaranteed to equal computeBgpPeerExportPolicyName(...), and i didn't want to introduce two sources of truth for that. the import policy is the odd one out because it's not consistent: it can be null (if no import policy was created) or the name of an inbound route-map (if that's the only constraint on inbound routes), or it can be computeBgpPeerImportPolicyName(...).

I too want these to be symmetric, but i think we should work towards the import policy always being the standard generated import policy name rather than having generateBgpExportPolicy() needlessly return an export policy name.


projects/batfish/src/main/java/org/batfish/representation/cisco/CiscoConfiguration.java, line 1898 at r3 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

"default route" to disambiguate which default?

changed since default route export policy is now generated alongside general export policy


projects/batfish/src/main/java/org/batfish/representation/cisco/CiscoConfiguration.java, line 1902 at r3 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

if you can move this to compute... that'd be great

done

Copy link
Contributor

@progwriter progwriter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@corinaminer corinaminer merged commit 263cf19 into master Mar 6, 2019
@corinaminer corinaminer deleted the bgp-prefix-lists branch March 6, 2019 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants