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

FRR: implement route-map match ip address prefix-len #6617

Merged
merged 2 commits into from
Feb 10, 2021

Conversation

dhalperi
Copy link
Member

No description provided.

@batfish-bot
Copy link

This change is Reviewable

Copy link
Member Author

@dhalperi dhalperi 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 9 files reviewed, all discussions resolved (waiting on @arifogel and @ratulm)


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

ip_prefix_length

FYI moved here from FRR_prefix_list.g4, renamed to be less context-specific, and expanded to include 0. I checked.


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

Quoted 18 lines of code…
  /**
   * Convert a {@link ParserRuleContext} whose text is guaranteed to represent a valid signed 32-bit
   * decimal integer to an {@link Integer} if it is contained in the provided {@code space}, or else
   * {@link Optional#empty}.
   */
  private @Nonnull Optional<Integer> toIntegerInSpace(
      ParserRuleContext messageCtx, ParserRuleContext ctx, IntegerSpace space, String name) {
    int num = Integer.parseInt(ctx.getText());
    if (!space.contains(num)) {
      _w.addWarning(
          messageCtx,
          getFullText(messageCtx),
          _parser,
          String.format("Expected %s in range %s, but got '%d'", name, space, num));
      return Optional.empty();
    }
    return Optional.of(num);
  }

FYI: brought all this over from NX-OS.

Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 9 files at r1, 1 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dhalperi and @ratulm)


projects/batfish/src/main/java/org/batfish/representation/cumulus/RouteMapEntry.java, line 55 at r1 (raw file):

  /** Return stream of match statements for this entry. */
  public @Nonnull Stream<RouteMapMatch> getMatches() {

Don't you need to modify this function?

Copy link
Member Author

@dhalperi dhalperi 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: 8 of 10 files reviewed, all discussions resolved (waiting on @arifogel and @ratulm)


projects/batfish/src/main/java/org/batfish/representation/cumulus/RouteMapEntry.java, line 55 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Don't you need to modify this function?

Good catch!

I added it to the existing tests in the same way.

@codecov
Copy link

codecov bot commented Feb 10, 2021

Codecov Report

Merging #6617 (1ec37c4) into master (0083db5) will decrease coverage by 0.00%.
The diff coverage is 80.76%.

@@             Coverage Diff              @@
##             master    #6617      +/-   ##
============================================
- Coverage     73.42%   73.41%   -0.01%     
- Complexity    35854    35855       +1     
============================================
  Files          2844     2845       +1     
  Lines        144689   144714      +25     
  Branches      17514    17515       +1     
============================================
+ Hits         106237   106245       +8     
- Misses        30023    30032       +9     
- Partials       8429     8437       +8     
Impacted Files Coverage Δ Complexity Δ
...ar/cumulus_frr/CumulusFrrConfigurationBuilder.java 77.46% <68.75%> (-0.24%) 193.00 <5.00> (+4.00) ⬇️
.../batfish/representation/cumulus/RouteMapEntry.java 98.36% <100.00%> (+0.08%) 38.00 <2.00> (+2.00)
...ation/cumulus/RouteMapMatchIpAddressPrefixLen.java 100.00% <100.00%> (ø) 3.00 <3.00> (?)
.../datamodel/routing_policy/statement/Statement.java 72.72% <0.00%> (-9.10%) 6.00% <0.00%> (-1.00%)
.../org/batfish/dataplane/rib/RouteAdvertisement.java 83.67% <0.00%> (-4.09%) 18.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%)
...src/main/java/org/batfish/coordinator/PoolMgr.java 59.52% <0.00%> (-1.20%) 15.00% <0.00%> (-1.00%)
...java/org/batfish/dataplane/ibdp/VirtualRouter.java 90.32% <0.00%> (-0.32%) 206.00% <0.00%> (-1.00%)
...src/main/java/org/batfish/coordinator/WorkMgr.java 75.81% <0.00%> (-0.16%) 247.00% <0.00%> (-1.00%)
... and 1 more

Copy link
Member

@arifogel arifogel 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 r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ratulm)

@dhalperi dhalperi merged commit 4f1b7e9 into batfish:master Feb 10, 2021
@dhalperi dhalperi deleted the cumulus-pl branch February 10, 2021 21:31
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