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

Add support for "force" with next-hop-self in Cumulus configs. #5800

Merged
merged 5 commits into from May 14, 2020

Conversation

raveranj
Copy link
Contributor

No description provided.

@batfish-bot
Copy link

This change is Reviewable

@codecov
Copy link

codecov bot commented May 12, 2020

Codecov Report

Merging #5800 into master will increase coverage by 0.56%.
The diff coverage is 84.61%.

@@             Coverage Diff              @@
##             master    #5800      +/-   ##
============================================
+ Coverage     71.35%   71.92%   +0.56%     
- Complexity    33676    33847     +171     
============================================
  Files          2788     2789       +1     
  Lines        139668   139489     -179     
  Branches      16801    16770      -31     
============================================
+ Hits          99664   100329     +665     
+ Misses        32132    31272     -860     
- Partials       7872     7888      +16     
Impacted Files Coverage Δ Complexity Δ
...ar/cumulus_frr/CumulusFrrConfigurationBuilder.java 74.08% <0.00%> (-0.26%) 141.00 <0.00> (ø)
...n/cumulus/BgpNeighborIpv4UnicastAddressFamily.java 100.00% <100.00%> (ø) 22.00 <2.00> (+2.00)
...ish/representation/cumulus/CumulusConversions.java 90.03% <100.00%> (+0.06%) 184.00 <8.00> (+3.00)
...org/batfish/datamodel/vendor_family/AwsFamily.java 84.61% <0.00%> (-15.39%) 8.00% <0.00%> (+1.00%) ⬇️
...tfish/representation/aws/LoadBalancerListener.java 53.84% <0.00%> (-9.80%) 5.00% <0.00%> (ø%)
.../src/main/java/org/batfish/datamodel/flow/Hop.java 53.33% <0.00%> (-6.67%) 5.00% <0.00%> (-1.00%)
...tfish/bddreachability/BDDReachabilityAnalysis.java 62.50% <0.00%> (-6.02%) 11.00% <0.00%> (ø%)
...ain/java/org/batfish/common/util/UnzipUtility.java 72.72% <0.00%> (-4.42%) 8.00% <0.00%> (ø%)
...atfish/bddreachability/SessionInstrumentation.java 87.34% <0.00%> (-4.21%) 27.00% <0.00%> (-6.00%)
...rc/main/java/org/batfish/datamodel/flow/Trace.java 87.09% <0.00%> (-3.23%) 11.00% <0.00%> (-2.00%)
... and 110 more

@progwriter progwriter self-requested a review May 13, 2020 00:48
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 4 of 6 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @raveranj)


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

sbafin_next_hop_self
:
  NEXT_HOP_SELF (FORCE)?

Let's handle both force and all since they are the exact same command apparently


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

    _currentBgpNeighborIpv4UnicastAddressFamily.setNextHopSelf(true);
    if (ctx.FORCE() != null) {
      _currentBgpNeighborIpv4UnicastAddressFamily.setForceNextHopSelf(true);

IIUC, all is new (and preferred syntax) while force is legacy syntax?
If that's indeed the case, let's have the setters/getters use All as the suffix to match new documentation. Then here can leave comments linking to
http://docs.frrouting.org/en/latest/bgp.html#clicmd-[no]neighborPEERnext-hop-self[all]
and
FRRouting/frr#4200
indicating that they are indeed the same command.


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

    }

    boolean isIBgp = neighbor.getRemoteAs().equals(bgpVrf.getAutonomousSystem());

Leave a TODO comment indicating this won't work for dynamic neighbors.

@raveranj
Copy link
Contributor Author


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

Previously, progwriter (Victor Heorhiadi) wrote…

Let's handle both force and all since they are the exact same command apparently

Done.

@raveranj
Copy link
Contributor Author


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

Previously, progwriter (Victor Heorhiadi) wrote…

IIUC, all is new (and preferred syntax) while force is legacy syntax?
If that's indeed the case, let's have the setters/getters use All as the suffix to match new documentation. Then here can leave comments linking to
http://docs.frrouting.org/en/latest/bgp.html#clicmd-[no]neighborPEERnext-hop-self[all]
and
FRRouting/frr#4200
indicating that they are indeed the same command.

Thats right. Added comments with the links.

@raveranj
Copy link
Contributor Author


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

Previously, progwriter (Victor Heorhiadi) wrote…

Leave a TODO comment indicating this won't work for dynamic neighbors.

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.

:lgtm:

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

@progwriter progwriter merged commit efee783 into batfish:master May 14, 2020
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