FRR: support route map set as-path exclude

[lukaskoenen]

Add exclude option to modify AS_PATH in route map.
See bgp_routemap.c

• update grammar
• update representation
• add test

[batfish-bot]

This change is 

[codecov]

Codecov Report

Merging #7251 (7088e03) into master (e223081) will increase coverage by 0.21%.
The diff coverage is 83.01%.

@@             Coverage Diff              @@
##             master    #7251      +/-   ##
============================================
+ Coverage     73.02%   73.23%   +0.21%     
- Complexity    39663    40388     +725     
============================================
  Files          3169     3218      +49     
  Lines        158852   160661    +1809     
  Branches      19121    19323     +202     
============================================
+ Hits         116001   117666    +1665     
- Misses        33653    33710      +57     
- Partials       9198     9285      +87     

Impacted Files	Coverage Δ
...amodel/routing_policy/statement/ExcludeAsPath.java	62.50% <62.50%> (ø)
...ol/src/main/java/org/batfish/datamodel/AsPath.java	94.23% <100.00%> (+0.75%)	⬆️
...col/src/main/java/org/batfish/datamodel/AsSet.java	85.71% <100.00%> (+0.25%)	⬆️
...del/bgp/community/CommunityStructuresVerifier.java	96.70% <100.00%> (+0.01%)	⬆️
...uting_policy/as_path/AsPathStructuresVerifier.java	96.20% <100.00%> (+0.66%)	⬆️
...amodel/routing_policy/statement/PrependAsPath.java	66.66% <100.00%> (-0.84%)	⬇️
...ar/cumulus_frr/CumulusFrrConfigurationBuilder.java	78.58% <100.00%> (+0.08%)	⬆️
.../batfish/representation/cumulus/RouteMapEntry.java	98.57% <100.00%> (+0.06%)	⬆️
...presentation/cumulus/RouteMapSetExcludeAsPath.java	100.00% <100.00%> (ø)
...presentation/cumulus/RouteMapSetPrependAsPath.java	100.00% <100.00%> (ø)
... and 146 more

[dhalperi]

Hi @lukaskoenen , thanks for these nice contributions.

Asking explicitly, since FRR is GPL code: you actually read the CONTRIBUTING.md file and ensured your contributions abide by its terms, and you and/or your employer take responsibility for this?

[lukaskoenen]

Hi @dhalperi,

Yes I'm aware of the implications of the CONTRIBUTING.md. While implementing I only used existing Batfish code, more specific the implemented extend functionality as a guideline. The link to the file from the FRR repository was unfortunate and is only there to reference the existence of the exclude functionality since it is not yet part of the official documentation.

[arifogel]

Reviewed 8 of 16 files at r1, 2 of 2 files at r2, 11 of 12 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @lukaskoenen)

a discussion (no related file):
Lukas,
First of all thank you for your contribution.
Unfortunately, I am hesitant to accept code for a wholly undocumented feature.

At a bare minimum, this PR needs documentation of the semantics of as-path exclude (in general, every new class needs javadocs). And that documentation should be provided by someone who either:

• has not read bgp_routemap.c, which is under GPL
• has the right to release documentation learned from bgp_routemap.c under the Apache 2.0 license, e.g. the copyright holder.

I would also accept documentation attributed entirely to lab testing of the feature under relevant scenarios. Notes from the lab test should be provided in the javadoc for RouteMapSetExcludeAsPath.

---

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrr_routemap.g4, line 197 at r4 (raw file):

rms_as_path
:
  AS_PATH action = as_path_action as_path = literal_as_path NEWLINE

First, is as_path_action required? Was the grammar wrong before?
Is there not a separate command set as-path <as-path>?

Second:
Rather than using a single rule, just do:

AS_PATH (rms_as_path_exclude | rms_as_path_prepend)
...
rms_as_path_exclude
:
...
;

rms_as_path_prepend
:
...
;

Then in CumulusFrrConfigurationBuilder, use separate listener functions for the more specific rules.
(but this PR should not include as-path prepend, as noted separately)

---

projects/batfish/src/main/java/org/batfish/representation/cumulus/RouteMapSetExcludeAsPath.java, line 15 at r4 (raw file):

import org.batfish.datamodel.routing_policy.statement.Statement;

public class RouteMapSetExcludeAsPath implements RouteMapSet {

final, javadoc

---

projects/batfish/src/test/java/org/batfish/dataplane/protocols/GeneratedRouteHelperTest.java, line 164 at r4 (raw file):

                        new LiteralAsList(
                            ImmutableList.of(
                                new ExplicitAs(1L), new ExplicitAs(2L), new ExplicitAs(65100L)))),

No need to modify this.
New features should be tested in separate junit tests.

In particular, there should be tests of all conditions.

For exclude, you should have tests where the thing to be excluded is already absent in addition to when it is present.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/statement/ExcludeAsPath.java, line 80 at r4 (raw file):

                      .addAll(
                          ir.getAsPath().getAsSets().stream()
                              .filter(currentAsSet -> !excludeAsPath.contains(currentAsSet))

This seems fishy, but I don't know because I need to see documentation of the semantics.
Should you remove the entire as-set? Or just the elements that are excluded?

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/statement/ExcludeAsPath.java, line 98 at r4 (raw file):

  @Override
  public int hashCode() {
    final int prime = 31;

This entire function can be: return _expr.hashCode().

---

projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/routing_policy/statement/ExcludeAsPathTest.java, line 21 at r4 (raw file):

@RunWith(JUnit4.class)
public class ExcludeAsPathTest {

nit: final, javadoc

---

projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/routing_policy/statement/ExcludeAsPathTest.java, line 29 at r4 (raw file):

  @Test
  public void testExclude() {

You should also add tests for non-singleton as-sets (once the semantics have been clarified and confirmed).

---

projects/minesweeper/src/main/java/org/batfish/minesweeper/aspath/RoutePolicyStatementAsPathCollector.java, line 78 at r4 (raw file):

  @Override
  public Set<SymbolicAsPathRegex> visitExcludeAsPath(
      ExcludeAsPath excludeAsPath, Configuration arg) {

I don't think this should return an empty set.
@millstein should be able to clarify once the semantics of set as-path exclude have been documented.

Same goes for RoutePolicyStatementVarCollector.

---

projects/batfish/src/test/java/org/batfish/representation/cumulus/RouteMapSetPrependAsPathTest.java, line 15 at r4 (raw file):

/** Test of {@link RouteMapSetPrependAsPath} */
public class RouteMapSetPrependAsPathTest {

Sorry if duplicate: I need confirmation that a set as-path action is required.

[arifogel]

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @lukaskoenen)

a discussion (no related file):
By the way, please do not rebase or amend further commits on this PR, as this can interfere with review.
The whole thing will be squashed when we are done.

---

[lukaskoenen]

Hi @arifogel,
again thanks for the Feedback. In respect of the problems with the documentation, I will discuss the whole thing internally and delay changes accordingly. The earliest time for this is unfortunately next week, so I apologize in advance for the delay.

[lukaskoenen]

Reviewable status: 18 of 26 files reviewed, 8 unresolved discussions (waiting on @arifogel and @millstein)

a discussion (no related file):

Previously, arifogel (Ari Fogel) wrote…

Lukas,
First of all thank you for your contribution.
Unfortunately, I am hesitant to accept code for a wholly undocumented feature.

At a bare minimum, this PR needs documentation of the semantics of as-path exclude (in general, every new class needs javadocs). And that documentation should be provided by someone who either:

• has not read bgp_routemap.c, which is under GPL
• has the right to release documentation learned from bgp_routemap.c under the Apache 2.0 license, e.g. the copyright holder.

I would also accept documentation attributed entirely to lab testing of the feature under relevant scenarios. Notes from the lab test should be provided in the javadoc for RouteMapSetExcludeAsPath.

Since the creation of this PR there has been a PR adding basic documentation for this feature which has also already been merged.

---

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrr_routemap.g4, line 197 at r4 (raw file):

Previously, arifogel (Ari Fogel) wrote…

First, is as_path_action required? Was the grammar wrong before?
Is there not a separate command set as-path <as-path>?

Second:
Rather than using a single rule, just do:

AS_PATH (rms_as_path_exclude | rms_as_path_prepend)
...
rms_as_path_exclude
:
...
;

rms_as_path_prepend
:
...
;

Then in CumulusFrrConfigurationBuilder, use separate listener functions for the more specific rules.
(but this PR should not include as-path prepend, as noted separately)

As far as I can tell from the documentation and doing some testing using vtysh using exclude or prepend is required inside a route map. Regarding the division of the rules, I don't currently see the advantage here, since in both cases the rules would consist of only one token. I however decided to use the tokens directly in the rms_as_path rule and changed listener in CumulusFrrConfigurationBuilder accordingly.

  public void exitRms_as_path(Rms_as_pathContext ctx) {
    List<Long> asns =
        ctx.as_path.asns.stream().map(this::toLong).collect(ImmutableList.toImmutableList());

    if (ctx.PREPEND() != null) {
      _currentRouteMapEntry.setSetAsPath(new RouteMapSetPrependAsPath(asns));
    } else {
      assert ctx.EXCLUDE() != null;
      _currentRouteMapEntry.setSetExcludeAsPath(new RouteMapSetExcludeAsPath(asns));
    }
  }

I replaced the if else with an else statement and an assert since as discussed early it seems either exclude or prepend are required.

---

projects/batfish/src/main/java/org/batfish/representation/cumulus/RouteMapSetExcludeAsPath.java, line 15 at r4 (raw file):

Previously, arifogel (Ari Fogel) wrote…

final, javadoc

Done.

---

projects/batfish/src/test/java/org/batfish/dataplane/protocols/GeneratedRouteHelperTest.java, line 164 at r4 (raw file):

Previously, arifogel (Ari Fogel) wrote…

No need to modify this.
New features should be tested in separate junit tests.

In particular, there should be tests of all conditions.

For exclude, you should have tests where the thing to be excluded is already absent in addition to when it is present.

I do think this is relevant since this test is used to validate the set statements. I however expanded other tests to increase the test coverage to especially look at edge cases.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/statement/ExcludeAsPath.java, line 80 at r4 (raw file):

Previously, arifogel (Ari Fogel) wrote…

This seems fishy, but I don't know because I need to see documentation of the semantics.
Should you remove the entire as-set? Or just the elements that are excluded?

You are right this is wrong I have changed it accordingly.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/statement/ExcludeAsPath.java, line 98 at r4 (raw file):

Previously, arifogel (Ari Fogel) wrote…

This entire function can be: return _expr.hashCode().

Done.

---

projects/minesweeper/src/main/java/org/batfish/minesweeper/aspath/RoutePolicyStatementAsPathCollector.java, line 78 at r4 (raw file):

Previously, arifogel (Ari Fogel) wrote…

I don't think this should return an empty set.
@millstein should be able to clarify once the semantics of set as-path exclude have been documented.

Same goes for RoutePolicyStatementVarCollector.

I might need a bit more information here. I based this on the prepend implementation and will take a closer look on the effect of these functions.

---

projects/batfish/src/test/java/org/batfish/representation/cumulus/RouteMapSetPrependAsPathTest.java, line 15 at r4 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Sorry if duplicate: I need confirmation that a set as-path action is required.

I mentioned this early. If the information does not cover this I might need a clarification.

[arifogel]

Reviewed 8 of 8 files at r5, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @lukaskoenen and @millstein)

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrr_routemap.g4, line 197 at r4 (raw file):

Previously, lukaskoenen wrote…

As far as I can tell from the documentation and doing some testing using vtysh using exclude or prepend is required inside a route map. Regarding the division of the rules, I don't currently see the advantage here, since in both cases the rules would consist of only one token. I however decided to use the tokens directly in the rms_as_path rule and changed listener in CumulusFrrConfigurationBuilder accordingly.

  public void exitRms_as_path(Rms_as_pathContext ctx) {
    List<Long> asns =
        ctx.as_path.asns.stream().map(this::toLong).collect(ImmutableList.toImmutableList());

    if (ctx.PREPEND() != null) {
      _currentRouteMapEntry.setSetAsPath(new RouteMapSetPrependAsPath(asns));
    } else {
      assert ctx.EXCLUDE() != null;
      _currentRouteMapEntry.setSetExcludeAsPath(new RouteMapSetExcludeAsPath(asns));
    }
  }

I replaced the if else with an else statement and an assert since as discussed early it seems either exclude or prepend are required.

The reason to use separate rules is:

• they are completely independent commands
• you can avoid branching on tokens at all with separate listener functions
• you get better error messages on a config with invalid stuff after exclude or prepend.
• and most importantly - they actually have separate grammars! prepend is followed by as-path (can include sets), while exclude is followed by a list of ASNs (does not include sets). This is indicated by the documentation, and also by your implementation of RouteMapSetExcludeAsPath:
  https://docs.frrouting.org/en/latest/routemap.html#clicmd-set-as-path-exclude-AS-NUMBER...

---

projects/batfish/src/main/java/org/batfish/representation/cumulus/RouteMapSetExcludeAsPath.java, line 15 at r4 (raw file):

Previously, lukaskoenen wrote…

Done.

Add link to doc in this javadoc:
https://docs.frrouting.org/en/latest/routemap.html#clicmd-set-as-path-exclude-AS-NUMBER...

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/statement/ExcludeAsPath.java, line 58 at r5 (raw file):

  /**
   * Function that executes the statement based on an {@link Environment}

1. This is an overridden function. No need to javadoc unless the signature or contract differs significantly from the base.
2. The javadoc you wrote does not provide any information not in the signature.
3. Also FYI we aren't strict about using @param or @return. Plain English is fine.

But honestly you can just omit this javadoc altogether.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/statement/ExcludeAsPath.java, line 71 at r5 (raw file):

      outputRoute.setAsPath(
          AsPath.of(
              ImmutableList.<AsSet>builder()

Instead of constructing a list, then an immutable list builder, then adding the elements of the list to the immutable list, then building the immutable list, there is a much simpler way to do this.

• Use ImmutableList.toImmutableList() instead of Collectors.toList(), and you are done.

Another thing to be aware of, though it's not needed here if you use above solution:

• If you want an ImmutableList that contains all the elements of another list, don't make a builder. Just do ImmutableList.copyOf(otherList).

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/statement/ExcludeAsPath.java, line 73 at r5 (raw file):

              ImmutableList.<AsSet>builder()
                  .addAll(
                      outputRoute.getAsPath().getAsSets().stream()

This is extremely difficult to read, and AFAICT you are duplicating complex code in an unmaintainable fashion.

Please factor out whatever it is you are doing to outputRoute.getAsPath() into a helper function.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/statement/ExcludeAsPath.java, line 80 at r5 (raw file):

                                          currentAsSet.getAsns().stream()
                                              .filter(currentAs -> !asToExclude.contains(currentAs))
                                              .collect(Collectors.toList()))))

Ditch Longs.toArray and the collect call, and instead use do fitler(...).toArray(Long[]::new) to avoid unnecessary object construction

[millstein]

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

---

projects/minesweeper/src/main/java/org/batfish/minesweeper/aspath/RoutePolicyStatementAsPathCollector.java, line 78 at r4 (raw file):

Previously, lukaskoenen wrote…

I might need a bit more information here. I based this on the prepend implementation and will take a closer look on the effect of these functions.

I think an empty set is fine for now. There is something better we could do, but it's not useful anyway until the symbolic analysis can handle these kinds of AS-path transformations. So I would leave it as is but add a comment similar to the comment in the case for PrependAsPath in this file.

[lukaskoenen]

Reviewable status: 19 of 27 files reviewed, 5 unresolved discussions (waiting on @arifogel and @millstein)

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrr_routemap.g4, line 197 at r4 (raw file):

Previously, arifogel (Ari Fogel) wrote…

The reason to use separate rules is:

• they are completely independent commands
• you can avoid branching on tokens at all with separate listener functions
• you get better error messages on a config with invalid stuff after exclude or prepend.
• and most importantly - they actually have separate grammars! prepend is followed by as-path (can include sets), while exclude is followed by a list of ASNs (does not include sets). This is indicated by the documentation, and also by your implementation of RouteMapSetExcludeAsPath:
  https://docs.frrouting.org/en/latest/routemap.html#clicmd-set-as-path-exclude-AS-NUMBER...

Thanks for the clarification. Done.

---

projects/batfish/src/main/java/org/batfish/representation/cumulus/RouteMapSetExcludeAsPath.java, line 15 at r4 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Add link to doc in this javadoc:
https://docs.frrouting.org/en/latest/routemap.html#clicmd-set-as-path-exclude-AS-NUMBER...

I added the link so it should render as docs.frrouting.org I hope this is ok for you.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/statement/ExcludeAsPath.java, line 71 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Instead of constructing a list, then an immutable list builder, then adding the elements of the list to the immutable list, then building the immutable list, there is a much simpler way to do this.

• Use ImmutableList.toImmutableList() instead of Collectors.toList(), and you are done.

Another thing to be aware of, though it's not needed here if you use above solution:

• If you want an ImmutableList that contains all the elements of another list, don't make a builder. Just do ImmutableList.copyOf(otherList).

Done.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/statement/ExcludeAsPath.java, line 73 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

This is extremely difficult to read, and AFAICT you are duplicating complex code in an unmaintainable fashion.

Please factor out whatever it is you are doing to outputRoute.getAsPath() into a helper function.

Added a function removeASNs to the class AsPath that removes specified ASNs from the path by calling the new function removeASNsIfExists that removes a list of ASNs from a AsSet should they exist in the set.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/statement/ExcludeAsPath.java, line 80 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Ditch Longs.toArray and the collect call, and instead use do fitler(...).toArray(Long[]::new) to avoid unnecessary object construction

Not required anymore because factored out.

---

projects/minesweeper/src/main/java/org/batfish/minesweeper/aspath/RoutePolicyStatementAsPathCollector.java, line 78 at r4 (raw file):

Previously, millstein (Todd Millstein) wrote…

I think an empty set is fine for now. There is something better we could do, but it's not useful anyway until the symbolic analysis can handle these kinds of AS-path transformations. So I would leave it as is but add a comment similar to the comment in the case for PrependAsPath in this file.

Done.

[arifogel]

Reviewed 8 of 8 files at r6, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @lukaskoenen)

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrr_routemap.g4, line 197 at r4 (raw file):

Previously, lukaskoenen wrote…

Thanks for the clarification. Done.

Sorry for this last bit:
For reasons related to parse-failure recovery which are hard to explain, we'd prefer you put the NEWLINE token at the end of the leaf rules rms_as_path_exclude and rms_as_path_prepend rather than at the end of rms_as_path.

---

projects/batfish/src/main/java/org/batfish/representation/cumulus/RouteMapSetExcludeAsPath.java, line 17 at r6 (raw file):

/**
 * Clause of set as-path exclude in route map {@see <a
 * href="https://docs.frrouting.org/en/latest/routemap.html#clicmd-set-as-path-exclude-AS-NUMBER">docs.frrouting.org/a>}.

Your URL is missing the three dots at the end: NUMBER -> NUMBER...
As is it just takes you to the top of the route-map page.

Note that if you want to test this, you should first scroll to the top of the route-map page, navigate away, and then try the URL. Otherwise your browser may simply remember your scroll position.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/AsPath.java, line 109 at r6 (raw file):

   * path.
   */
  public AsPath removeASNs(List<Long> asns) {

nit: put @Nonnull between public and removeASNs

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/AsSet.java, line 164 at r6 (raw file):

  /**
   * Returns a new {@link AsSet} that consists of this set with the provided ASNs removed if they

nit: Just end the sentence at removed (and put a period) for consistency with javadoc above.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/AsSet.java, line 167 at r6 (raw file):

   * exists
   */
  public AsSet removeASNsIfExists(List<Long> asns) {

nits:

• style: The IfExists portion of the name should be dropped for consistency with above function
• put @Nonnull between public and AsSet (new code should have null annotations)
• Make the argument Collection<Long>; no need to be so restrictive.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/statement/ExcludeAsPath.java, line 73 at r5 (raw file):

Previously, lukaskoenen wrote…

Added a function removeASNs to the class AsPath that removes specified ASNs from the path by calling the new function removeASNsIfExists that removes a list of ASNs from a AsSet should they exist in the set.

Much better!

[arifogel]

Reviewed 4 of 4 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lukaskoenen)

[arifogel]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lukaskoenen)