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: Parse AS path access lists #4895

Merged
merged 9 commits into from
Oct 8, 2019
Merged

FRR: Parse AS path access lists #4895

merged 9 commits into from
Oct 8, 2019

Conversation

corinaminer
Copy link
Contributor

Still to do: route-map clauses referencing AS path access lists.

@batfish-bot
Copy link

This change is Reviewable

@codecov
Copy link

codecov bot commented Oct 7, 2019

Codecov Report

Merging #4895 into master will decrease coverage by 0.32%.
The diff coverage is 94.59%.

@@             Coverage Diff              @@
##             master    #4895      +/-   ##
============================================
- Coverage     76.64%   76.32%   -0.33%     
+ Complexity    29595    29269     -326     
============================================
  Files          2319     2321       +2     
  Lines        114944   113187    -1757     
  Branches      14301    13488     -813     
============================================
- Hits          88102    86385    -1717     
- Misses        20117    20219     +102     
+ Partials       6725     6583     -142
Impacted Files Coverage Δ Complexity Δ
...ish/representation/cumulus/IpAsPathAccessList.java 100% <100%> (ø) 4 <4> (?)
...ar/cumulus_frr/CumulusFrrConfigurationBuilder.java 80.98% <100%> (+0.4%) 95 <2> (+2) ⬆️
...h/representation/cumulus/CumulusStructureType.java 84% <100%> (+0.66%) 8 <0> (ø) ⬇️
...representation/cumulus/IpAsPathAccessListLine.java 100% <100%> (ø) 3 <3> (?)
...presentation/cumulus/CumulusNcluConfiguration.java 92.74% <85.71%> (-1.04%) 232 <7> (-75)
...fish/grammar/flatjuniper/ConfigurationBuilder.java 63.3% <0%> (-7.34%) 718% <0%> (-173%)
...java/org/batfish/symbolic/state/EdgeStateExpr.java 73.91% <0%> (-4.35%) 10% <0%> (-1%)
...h/representation/juniper/JuniperConfiguration.java 89.15% <0%> (-2.73%) 401% <0%> (-81%)
...org/batfish/datamodel/flow/BidirectionalTrace.java 79.54% <0%> (-2.28%) 13% <0%> (ø)
...col/src/main/java/org/batfish/role/InferRoles.java 90.15% <0%> (-1.52%) 63% <0%> (-2%)
... and 9 more

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 2 of 4 files at r1, 9 of 9 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @corinaminer)


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

    List<AsPathAccessListLine> lines =
        asPathAccessList.getLines().stream()
            .map(l -> new AsPathAccessListLine(l.getAction(), String.valueOf(l.getAsNum())))

VI AsPathAccessListLine takes a regex. I sense we're not getting conversion to regex right here.


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

    assertThat(definedStructureInfo.getDefinitionLines(), contains(1, 2));
  }

add parsing test for match/set routemap clauses (you can expand to extraction later).


projects/batfish/src/test/java/org/batfish/representation/cumulus/CumulusNcluConfigurationTest.java, line 150 at r2 (raw file):

            new AsPathAccessListLine(LineAction.PERMIT, "12345"),
            new AsPathAccessListLine(LineAction.DENY, "54321"));
    assertThat(viList, equalTo(new AsPathAccessList("name", expectedViLines)));

add semantic test

Copy link
Contributor Author

@corinaminer corinaminer 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: 5 of 11 files reviewed, all discussions resolved (waiting on @progwriter)


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

Previously, progwriter (Victor Heorhiadi) wrote…

VI AsPathAccessListLine takes a regex. I sense we're not getting conversion to regex right here.

nope. added a todo


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

Previously, progwriter (Victor Heorhiadi) wrote…

add parsing test for match/set routemap clauses (you can expand to extraction later).

Done


projects/batfish/src/test/java/org/batfish/representation/cumulus/CumulusNcluConfigurationTest.java, line 150 at r2 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

add semantic test

done-ish? I know it's going to be incorrect for anything remotely complex because of how we match AS paths. open to recommendations

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 6 of 6 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @corinaminer)


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

:
  (
  asns += uint32

nit: plz indent


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

        pushMode(M_Word);
    }
  }

😞


projects/batfish/src/test/java/org/batfish/representation/cumulus/CumulusNcluConfigurationTest.java, line 150 at r2 (raw file):

Previously, corinaminer (Corina Miner) wrote…

done-ish? I know it's going to be incorrect for anything remotely complex because of how we match AS paths. open to recommendations

be sad. file issue. add comment referencing issue. send PR that fixes it later.

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 6 of 6 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @corinaminer)

Copy link
Contributor Author

@corinaminer corinaminer 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 11 files reviewed, all discussions resolved (waiting on @progwriter)


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

Previously, progwriter (Victor Heorhiadi) wrote…

nit: plz indent

done


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

Previously, progwriter (Victor Heorhiadi) wrote…

😞

yeah, if you have a better idea lmk

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 3 of 3 files at r5.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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 1 of 1 files at r6.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@corinaminer corinaminer merged commit 8d4a0aa into master Oct 8, 2019
@corinaminer corinaminer deleted the frr-as-path branch October 8, 2019 03:20
@corinaminer corinaminer mentioned this pull request Oct 8, 2019
8 tasks
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