feat: add regex to as path access lists

[lukaskoenen]

Use a regular expression to match AS_PATH in access-list instead of converting a single integer to a regular expression.

• update lexer / parser
• update conversion
• update tests

[batfish-bot]

This change is 

[codecov]

Codecov Report

Merging #7250 (13563e6) into master (194f9b7) will increase coverage by 0.03%.
The diff coverage is 90.90%.

@@             Coverage Diff              @@
##             master    #7250      +/-   ##
============================================
+ Coverage     73.04%   73.07%   +0.03%     
- Complexity    39752    39842      +90     
============================================
  Files          3176     3188      +12     
  Lines        159140   159385     +245     
  Branches      19156    19171      +15     
============================================
+ Hits         116239   116467     +228     
- Misses        33687    33696       +9     
- Partials       9214     9222       +8     

Impacted Files	Coverage Δ
...ar/cumulus_frr/CumulusFrrConfigurationBuilder.java	78.48% <85.71%> (-0.02%)	⬇️
...ish/representation/cumulus/CumulusConversions.java	84.01% <100.00%> (-0.04%)	⬇️
...representation/cumulus/IpAsPathAccessListLine.java	100.00% <100.00%> (ø)
...main/java/org/batfish/datamodel/acl/AclTracer.java	63.69% <0.00%> (-1.28%)	⬇️
...src/main/java/org/batfish/coordinator/PoolMgr.java	59.52% <0.00%> (-1.20%)	⬇️
...atfish/datamodel/routing_policy/RoutingPolicy.java	93.61% <0.00%> (-1.07%)	⬇️
...rg/batfish/representation/juniper/AsPathRegex.java	32.89% <0.00%> (-0.88%)	⬇️
...ain/java/org/batfish/storage/FileBasedStorage.java	85.31% <0.00%> (-0.61%)	⬇️
...atfish/src/main/java/org/batfish/main/Batfish.java	74.86% <0.00%> (-0.47%)	⬇️
...batfish/src/main/java/org/batfish/main/Driver.java	50.42% <0.00%> (-0.43%)	⬇️
... and 38 more

[arifogel]

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

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrLexer.g4, line 1111 at r1 (raw file):

M_AccessList_DENY
:
   'deny' -> type( DENY )

nit: you don't need to follow the garbage produced by the autoformatter we abandoned.
Can ditch the spaces inside the parentheses.

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrParser.g4, line 42 at r1 (raw file):

ip_as_path
:
  AS_PATH ACCESS_LIST name = word action = access_list_action as_path_regex = word NEWLINE

Is access_list_action required by FRR?

---

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

    if (ctx.PERMIT() != null) {
      return LineAction.PERMIT;
    } else if (ctx.DENY() != null) {

Make this an else, and assert ctx.DENY() != null. I believe this is guaranteed by the grammar.
No need for the last branch.

---

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

    String name = ctx.name.getText();
    LineAction action = toLineAction(ctx.action);
    String regex = ctx.as_path_regex.getText();

Is this always treated as a regex? For instance, if you put 4, does it match 44 or just 4? If the latter, then IpAsPathAccessListLine should be broken up into two classes to be converted separately.
Otherwise this is fine.

---

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

  public AsPathAccessListLine toAsPathAccessListLine() {
    String regex = CumulusConversions.toJavaRegex(_regex);
    return new AsPathAccessListLine(_action, regex);

AsPathAccessList is deprecated, broken in certain circumstances, and will not be fixed.
Please use AsPathMatchExpr and associated classes in that package.
See CiscoXrConverions.AsPathSetElemToAsPathMatchExpr and related visitors.

---

projects/batfish/src/test/java/org/batfish/grammar/cumulus_frr/CumulusFrrGrammarTest.java, line 1450 at r1 (raw file):

  public void testCumulusFrrIpAsPathAccessList() {
    String name = "NAME";
    String as1 = "^11111$";

Rather than modifying this test, make a new one for the syntax you are trying to support.

[lukaskoenen]

Hi @arifogel,
first of all thank you for your review. The following is my input on what was addressed. If no comment exists, the suggestion was accepted and implemented.

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrParser.g4, line 42 at r1 (raw file):

ip_as_path
:
  AS_PATH ACCESS_LIST name = word action = access_list_action as_path_regex = word NEWLINE

As far as I can tell from the documentation the access_list_action is necessary.
-> bgp as-path access-list WORD [seq (0-4294967295)] permit|deny LINE

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

    String name = ctx.name.getText();
    LineAction action = toLineAction(ctx.action);
    String regex = ctx.as_path_regex.getText();

I've just tested this. When defining the regex as 0 the output included all AS including a 0 so for example 65001. So it seem the implementation is fine.

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

  public AsPathAccessListLine toAsPathAccessListLine() {
    String regex = CumulusConversions.toJavaRegex(_regex);
    return new AsPathAccessListLine(_action, regex);

I've only modified the existing class here. My Question therefore is if I should rewrite the whole thing to use AsPathMatchExpr or is this something planned from your side.

projects/batfish/src/test/java/org/batfish/grammar/cumulus_frr/CumulusFrrGrammarTest.java, line 1450 at r1 (raw file):

  public void testCumulusFrrIpAsPathAccessList() {
    String name = "NAME";
    String as1 = "^11111$";

I thought this test case was only there to ensure the correct

I thought this tests exists to ensure the correct generation of the respective data structure. I have extended the test accordingly with another possible regular expression. While doing so I saw that some required characters led to parsing errors which I also fixed (see b65c72a).

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrParser.g4, line 811 at r1 (raw file):

fragment
F_WordChar
:
 [0-9A-Za-z!@#$^*_=+.;:{}\\[\]|()]
 | '-'
;

This meant extending the valid characters for the word token. Since I am not sure whether this can lead to problems I would be grateful for feedback.

[lukaskoenen]

I've just read the last comment on my other PR and I'm afraid that before doing so I just rebased this PR I hope this does not lead to additional problems.

[arifogel]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lukaskoenen)

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrLexer.g4, line 1111 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

nit: you don't need to follow the garbage produced by the autoformatter we abandoned.
Can ditch the spaces inside the parentheses.

Missed a couple, but no need to spend any more time on it. In the end they are all over the file, and one day when we are bored we will fix in an automated fashion.

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrLexer.g4, line 811 at r3 (raw file):

F_WordChar
:
  [0-9A-Za-z!@#$^*_=+.;:{}\\[\]|()]

I missed this the first time around.
Please do not alter this lexer fragment to make a regex char.

Instead, make and use a separate fragment F_RegexChar (and fragment F_Regex: F_RegexChar+;)

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrLexer.g4, line 1119 at r3 (raw file):

;

M_AsPathAccessList_WORD

Does a regex terminate after a single word? What if you have spaces in the middle? Does it use the whole rest of the line, or just the first word? Is there any form of quoting? Will a regex with spaces never match?
It's fine to defer addressing this question, but if you do, please leave a // TODO: is it always a single word? comment or something like that around here.

Also, this should use F_Regex as suggested above.

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrParser.g4, line 42 at r1 (raw file):

As far as I can tell from the documentation the access_list_action is necessary.
-> bgp as-path access-list WORD [seq (0-4294967295)] permit|deny LINE

When I ask if something is required by FRR, I mean, "Is it is accepted in vtysh when you type it and hit return? And does the exact line, or something else, appear in the config afterward hitting return?" I ask because as we are both familiar, there is a disconnect between what is documented and what is implemented. Many vendors even have a disconnect between what is tab-completed, what pops up when you hit ?, what the box actually accepts, and what the config looks like if you dump it right after entering the command.

While it is possible we had implemented something that was incorrect, we need to be extra careful not to make breaking changes for other users. And this is clearly a breaking change from the syntax we previously allowed.
Please test on the box and share your results.

---

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

I've just tested this. When defining the regex as 0 the output included all AS including a 0 so for example 65001. So it seem the implementation is fine.

👍

---

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

I've only modified the existing class here. My Question therefore is if I should rewrite the whole thing to use AsPathMatchExpr or is this something planned from your side.

A couple things, now that I've looked more closely.

1. Please remove conversion code from this class, and leave it in CumulusConversions where it was before. Our current patten is for vendor-specific representation classes to contain data only, and not behavior.
2. The more I look at this, the more I realize we are going to want to optimize this in a way that is unreasonable to ask from an outsider not familiar with the new API. So you can keep your changes as long as they are passing (but again, move the changes to CumulusConversions).

---

projects/batfish/src/test/java/org/batfish/grammar/cumulus_frr/CumulusFrrGrammarTest.java, line 1450 at r1 (raw file):

I thought this tests exists to ensure the correct generation of the respective data structure. I have extended the test accordingly with another possible regular expression. While doing so I saw that some required characters led to parsing errors which I also fixed (see b65c72a).

Sorry, I misinterpreted the purpose of this test while reviewing late at night.
I see you are adding grammar coverage for different regex strings. This is fine.

[lukaskoenen]

Hi @arifogel,
thanks again for the detailed Feedback! I've also tried to figure out a way to use AsPathMatchExpr instead of AsPathAccessList but currently don't see how to integrate the action. If I see how, I'm happy to implement this as well.

Reviewable status: 5 of 8 files reviewed, 4 unresolved discussions (waiting on @arifogel)

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrLexer.g4, line 811 at r3 (raw file):

Previously, arifogel (Ari Fogel) wrote…

I missed this the first time around.
Please do not alter this lexer fragment to make a regex char.

Instead, make and use a separate fragment F_RegexChar (and fragment F_Regex: F_RegexChar+;)

I added a new fragment like you suggested. I checked what characters are supported using vtysh.

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrLexer.g4, line 1119 at r3 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Does a regex terminate after a single word? What if you have spaces in the middle? Does it use the whole rest of the line, or just the first word? Is there any form of quoting? Will a regex with spaces never match?
It's fine to defer addressing this question, but if you do, please leave a // TODO: is it always a single word? comment or something like that around here.

Also, this should use F_Regex as suggested above.

I've checked this using vtysh. Using a space inside the regular expression leads to a unknown command.

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrParser.g4, line 42 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

As far as I can tell from the documentation the access_list_action is necessary.
-> bgp as-path access-list WORD [seq (0-4294967295)] permit|deny LINE

When I ask if something is required by FRR, I mean, "Is it is accepted in vtysh when you type it and hit return? And does the exact line, or something else, appear in the config afterward hitting return?" I ask because as we are both familiar, there is a disconnect between what is documented and what is implemented. Many vendors even have a disconnect between what is tab-completed, what pops up when you hit ?, what the box actually accepts, and what the config looks like if you dump it right after entering the command.

While it is possible we had implemented something that was incorrect, we need to be extra careful not to make breaking changes for other users. And this is clearly a breaking change from the syntax we previously allowed.
Please test on the box and share your results.

Thank you for the clarification! Creating an access-list without permit|deny results in an unknown command.

---

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

Previously, arifogel (Ari Fogel) wrote…

I've only modified the existing class here. My Question therefore is if I should rewrite the whole thing to use AsPathMatchExpr or is this something planned from your side.

A couple things, now that I've looked more closely.

1. Please remove conversion code from this class, and leave it in CumulusConversions where it was before. Our current patten is for vendor-specific representation classes to contain data only, and not behavior.
2. The more I look at this, the more I realize we are going to want to optimize this in a way that is unreasonable to ask from an outsider not familiar with the new API. So you can keep your changes as long as they are passing (but again, move the changes to CumulusConversions).

Moved Conversion to CumulusConversions

[arifogel]

Reviewed 1 of 3 files at r4, 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukaskoenen)

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrLexer.g4, line 1132 at r5 (raw file):

;

M_AsPathAccessList_REGEX

nit: please use same suffix as the type you are setting (REGEX vs REGEXP)

[arifogel]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukaskoenen)

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrLexer.g4, line 1132 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

nit: please use same suffix as the type you are setting (REGEX vs REGEXP)

I would suggest changing REGEXP -> REGEX.
If either regexp or regex is an FRR keyword, then use something else entirely.

[arifogel]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukaskoenen)

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrLexer.g4, line 1132 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

I would suggest changing REGEXP -> REGEX.
If either regexp or regex is an FRR keyword, then use something else entirely.

I mean if both are

[lukaskoenen]

Reviewable status: 5 of 8 files reviewed, 1 unresolved discussion (waiting on @arifogel)

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrLexer.g4, line 1132 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

I mean if both are

Hi @arifogel,
I've fixed the suffix and changed REGEXP to REGEX. I also removed the regexp statement from CumulusFrr_common.g4, since I don't see a reason why this would be needed (correct me if I'm wrong).

[arifogel]

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

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrLexer.g4, line 1132 at r5 (raw file):

Previously, lukaskoenen wrote…

Hi @arifogel,
I've fixed the suffix and changed REGEXP to REGEX. I also removed the regexp statement from CumulusFrr_common.g4, since I don't see a reason why this would be needed (correct me if I'm wrong).

There is a reason to leave the parser rule that is not worth getting into. The team can address it later when we get a chance clean up this parser. You don't need to worry about it, and can leave as is.

[arifogel]

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