Support "set comm-list delete" for standard comm-lists in FRR. #5549

raveranj · 2020-02-12T23:34:03Z

No description provided.

batfish-bot · 2020-02-12T23:34:09Z

This change is

codecov · 2020-02-13T00:24:22Z

Codecov Report

Merging #5549 into master will increase coverage by 0.14%.
The diff coverage is 48.78%.

@@             Coverage Diff              @@
##             master    #5549      +/-   ##
============================================
+ Coverage     73.53%   73.67%   +0.14%     
- Complexity    32410    32505      +95     
============================================
  Files          2643     2648       +5     
  Lines        130041   130090      +49     
  Branches      15593    15554      -39     
============================================
+ Hits          95625    95844     +219     
+ Misses        26826    26661     -165     
+ Partials       7590     7585       -5

Impacted Files	Coverage Δ	Complexity Δ
...ish/representation/cumulus/CumulusConversions.java	`89.26% <100%> (+1.76%)`	`168 <0> (+5)`	⬆️
...sentation/cumulus/IpCommunityListStandardLine.java	`100% <100%> (+100%)`	`3 <1> (+3)`	⬆️
...epresentation/cumulus/IpCommunityListStandard.java	`80% <100%> (+80%)`	`2 <0> (+2)`	⬆️
...ar/cumulus_frr/CumulusFrrConfigurationBuilder.java	`74.1% <44.73%> (-1.76%)`	`135 <4> (+5)`
...va/org/batfish/specifier/SpecifierContextImpl.java	`70% <0%> (-18.24%)`	`3% <0%> (-1%)`
.../src/main/java/org/batfish/datamodel/Protocol.java	`83.87% <0%> (-6.46%)`	`8% <0%> (-2%)`
...specifier/parboiled/ParboiledEnumSetSpecifier.java	`87.17% <0%> (-5.13%)`	`5% <0%> (-1%)`
...rc/main/java/org/batfish/datamodel/flow/Trace.java	`87.09% <0%> (-3.23%)`	`12% <0%> (-1%)`
...fish/question/testfilters/TestFiltersAnswerer.java	`87.4% <0%> (-2.82%)`	`19% <0%> (+2%)`
...main/java/org/batfish/datamodel/pojo/BfObject.java	`88.88% <0%> (-2.03%)`	`8% <0%> (ø)`
... and 80 more

arifogel

Reviewed 6 of 9 files at r1, 1 of 2 files at r2.
Reviewable status: 7 of 9 files reviewed, 5 unresolved discussions (waiting on @arifogel and @raveranj)

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

  }

  private @Nonnull String toString(ParserRuleContext ctx) {

Don't make this change. If you need to add another toString function taking a different type, do so.
The point of using specific arguments is to force a compile error if someone accidentally passes in the wrong thing.

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

    Set<Community> blacklist = new HashSet<>();
    for (IpCommunityListStandardLine line : ipCommunityListStandard.getLines()) {
      // Cumulus lets one specify multiple communities in a line.

Add a TODO comment, e.g.
// TODO: support set comm-list delete semantics for line with more than one community

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

      }
      line.getCommunities()
          .forEach(

You cannot do this for each community in the line. It will not implement the correct semantics.
Cumulus semantics for set comm-list delete with more than one community on a line in the referenced list are not expressible via a context-free CommunityMatchExpr. Whether or not a given community is removed by a list depends on all the communities in the route, not just the community under consideration.
We do not support this type of operation at the moment further down the pipeline.
I suggest for now you keep the old implementation for this function, and just add the TODO as suggested above.

projects/batfish/src/test/java/org/batfish/grammar/cumulus_concatenated/CumulusConcatenatedGrammarTest.java, line 417 at r2 (raw file):

    // RMs using expanded comm-lists.
    RoutingPolicy rpExtended1 =

nit: Expanded, not Extended. Otherwise it sounds like a policy for extended communities.

projects/batfish/src/test/java/org/batfish/grammar/cumulus_concatenated/CumulusConcatenatedGrammarTest.java, line 436 at r2 (raw file):

        c.getRoutingPolicies().get("RM_STANDARD_TEST_DELETE_COMM_BEGIN_WITH_3");

    Bgpv4Route outputRoute1 = processRouteIn(rpExtended1, inRoute);

This style is very error-prone. You can get around declaring multiple variables and keeping track of numbers by using lexical scoping.
For instance:

...
{
  RoutingPolicy rp = c.getRoutingPolicies().get("RM_STANDARD_TEST_DELETE_ALL_COMMUNITIES");
  assertThat(processRouteIn(rp, inRoute).getCommunities(), empty()); // prefer empty() to hasSize(0)
}
{
  RoutingPolicy rp = c.getRoutingPolicies().get("RM_STANDARD_TEST_DELETE_COMM_BEGIN_WITH_1");
  assertThat(
    processRouteIn(rp, inRoute).getCommunities(),
    containsInAnyOrder( // use containsInAnyOrder because we are asserting properties of a set, not a list
            StandardCommunity.of(2, 1),
            StandardCommunity.of(2, 2),
            StandardCommunity.of(3, 1),
            StandardCommunity.of(3, 2)));
}
...

arifogel

Reviewable status: 7 of 9 files reviewed, 5 unresolved discussions (waiting on @arifogel and @raveranj)

projects/batfish/src/test/java/org/batfish/grammar/cumulus_concatenated/CumulusConcatenatedGrammarTest.java, line 436 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

This style is very error-prone. You can get around declaring multiple variables and keeping track of numbers by using lexical scoping.
For instance:

...
{
  RoutingPolicy rp = c.getRoutingPolicies().get("RM_STANDARD_TEST_DELETE_ALL_COMMUNITIES");
  assertThat(processRouteIn(rp, inRoute).getCommunities(), empty()); // prefer empty() to hasSize(0)
}
{
  RoutingPolicy rp = c.getRoutingPolicies().get("RM_STANDARD_TEST_DELETE_COMM_BEGIN_WITH_1");
  assertThat(
    processRouteIn(rp, inRoute).getCommunities(),
    containsInAnyOrder( // use containsInAnyOrder because we are asserting properties of a set, not a list
            StandardCommunity.of(2, 1),
            StandardCommunity.of(2, 2),
            StandardCommunity.of(3, 1),
            StandardCommunity.of(3, 2)));
}
...

As I side note, this suggested style keeps the name of the policy near the thing being asserted; this makes it much easier to review.

arifogel

Reviewed 1 of 2 files at r2.
Reviewable status: 8 of 9 files reviewed, 6 unresolved discussions (waiting on @arifogel and @raveranj)

projects/batfish/src/test/java/org/batfish/grammar/cumulus_concatenated/CumulusConcatenatedGrammarTest.java, line 428 at r2 (raw file):

    // RMs using standard comm-lists.
    RoutingPolicy rpStandard1 =
        c.getRoutingPolicies().get("RM_STANDARD_TEST_DELETE_ALL_COMMUNITIES");

My takeaway from our offline conversation about multiple communities per line was that this policy would delete all communities only if all are present. If only some are present, it should do nothing. In this case, you really need to test more than one input route to prove correct behavior.
If I misunderstood, we should have a longer discussion offline.

My suggestion above to add a TODO would make this moot; you wouldn't then be able to test correct semantics for this policy yet.

arifogel

Reviewed 1 of 9 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @raveranj)

…support only comm-lists with one community; refactor UT.

raveranj · 2020-02-13T18:16:41Z

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

Previously, arifogel (Ari Fogel) wrote…

Don't make this change. If you need to add another toString function taking a different type, do so.
The point of using specific arguments is to force a compile error if someone accidentally passes in the wrong thing.

Reverted it.

raveranj · 2020-02-13T18:16:54Z

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

Previously, arifogel (Ari Fogel) wrote…

Add a TODO comment, e.g.
// TODO: support set comm-list delete semantics for line with more than one community

Done.

raveranj · 2020-02-13T18:19:35Z

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

Previously, arifogel (Ari Fogel) wrote…

You cannot do this for each community in the line. It will not implement the correct semantics.
Cumulus semantics for set comm-list delete with more than one community on a line in the referenced list are not expressible via a context-free CommunityMatchExpr. Whether or not a given community is removed by a list depends on all the communities in the route, not just the community under consideration.
We do not support this type of operation at the moment further down the pipeline.
I suggest for now you keep the old implementation for this function, and just add the TODO as suggested above.

Done. Reverted to align with the TODO.

raveranj · 2020-02-13T18:20:39Z

projects/batfish/src/test/java/org/batfish/grammar/cumulus_concatenated/CumulusConcatenatedGrammarTest.java, line 417 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

nit: Expanded, not Extended. Otherwise it sounds like a policy for extended communities.

Removed the variables. Used lexical scoping instead as mentioned below. Thanks!

raveranj · 2020-02-13T18:23:18Z

projects/batfish/src/test/java/org/batfish/grammar/cumulus_concatenated/CumulusConcatenatedGrammarTest.java, line 428 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

My takeaway from our offline conversation about multiple communities per line was that this policy would delete all communities only if all are present. If only some are present, it should do nothing. In this case, you really need to test more than one input route to prove correct behavior.
If I misunderstood, we should have a longer discussion offline.

My suggestion above to add a TODO would make this moot; you wouldn't then be able to test correct semantics for this policy yet.

Yes, we are on the page w.r.t. the expected aforementioned behavior. Fixed up the tests to have only 1 community per line to align with the TODO for now.

raveranj · 2020-02-13T18:24:01Z

projects/batfish/src/test/java/org/batfish/grammar/cumulus_concatenated/CumulusConcatenatedGrammarTest.java, line 436 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

As I side note, this suggested style keeps the name of the policy near the thing being asserted; this makes it much easier to review.

Done. Agree. This is much cleaner & easier to maintain. Thanks.

arifogel

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @raveranj)

projects/batfish/src/test/resources/org/batfish/grammar/cumulus_concatenated/testconfigs/set_comm_list_delete_test, line 25 at r3 (raw file):

ip community-list expanded test_expanded_delete_all_communities permit "[0-9]+:[0-9]+"
ip community-list expanded test_expanded_delete_comm_begin_with_1 permit "1:.*"
ip community-list expanded test_expanded_delete_comm_begin_with_2 permit "2:.*"

For coverage, please add 1 or more community lists that have multiple lines, at least one of which is a deny line.

E.g. the following should only delete 1:2

deny 1:1
permit 1:1
permit 1:2

raveranj · 2020-02-13T22:09:03Z

projects/batfish/src/test/resources/org/batfish/grammar/cumulus_concatenated/testconfigs/set_comm_list_delete_test, line 25 at r3 (raw file):

Previously, arifogel (Ari Fogel) wrote…

For coverage, please add 1 or more community lists that have multiple lines, at least one of which is a deny line.

E.g. the following should only delete 1:2
deny 1:1
permit 1:1
permit 1:2

Done. Added coverage for both expanded and standard comm-lists.

arifogel

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

Raveendran added 4 commits February 10, 2020 08:58

Changes to support standard community-lists for FRR in Batfish.

ed4d469

Removed a comment block.

85018a5

Few fixes uncovered while testing.

8a77680

Fixed formatting.

6c3c838

Raveendran added 2 commits February 12, 2020 15:50

Fixed Checkstyle violations.

7b2616b

Fix PMD error.

cadc3c9

arifogel self-requested a review February 13, 2020 03:13

arifogel reviewed Feb 13, 2020

View reviewed changes

Addressed first round of review comments from Ari. Primary changes - …

87f7887

…support only comm-lists with one community; refactor UT.

arifogel reviewed Feb 13, 2020

View reviewed changes

Added few more test cases to improve coverage per Ari's comment.

6988c32

arifogel reviewed Feb 14, 2020

View reviewed changes

arifogel merged commit 8640da8 into batfish:master Feb 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support "set comm-list delete" for standard comm-lists in FRR. #5549

Support "set comm-list delete" for standard comm-lists in FRR. #5549

raveranj commented Feb 12, 2020

batfish-bot commented Feb 12, 2020

codecov bot commented Feb 13, 2020 •

edited

arifogel left a comment

arifogel left a comment

arifogel left a comment

arifogel left a comment

raveranj commented Feb 13, 2020

raveranj commented Feb 13, 2020

raveranj commented Feb 13, 2020

raveranj commented Feb 13, 2020

raveranj commented Feb 13, 2020

raveranj commented Feb 13, 2020

arifogel left a comment

raveranj commented Feb 13, 2020

arifogel left a comment

Support "set comm-list delete" for standard comm-lists in FRR. #5549

Support "set comm-list delete" for standard comm-lists in FRR. #5549

Conversation

raveranj commented Feb 12, 2020

batfish-bot commented Feb 12, 2020

codecov bot commented Feb 13, 2020 • edited

Codecov Report

arifogel left a comment

Choose a reason for hiding this comment

arifogel left a comment

Choose a reason for hiding this comment

arifogel left a comment

Choose a reason for hiding this comment

arifogel left a comment

Choose a reason for hiding this comment

raveranj commented Feb 13, 2020

raveranj commented Feb 13, 2020

raveranj commented Feb 13, 2020

raveranj commented Feb 13, 2020

raveranj commented Feb 13, 2020

raveranj commented Feb 13, 2020

arifogel left a comment

Choose a reason for hiding this comment

raveranj commented Feb 13, 2020

arifogel left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 13, 2020 •

edited