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

Arista: implement peer-filters #5940

Merged
merged 2 commits into from Jun 26, 2020
Merged

Conversation

progwriter
Copy link
Contributor

Fist pass at implementing BGP peer-filters for dynamic neighbors.

Fist pass at implementing BGP peer-filters for dynamic neighbors.
@progwriter progwriter requested a review from dhalperi June 26, 2020 09:35
@batfish-bot
Copy link

This change is Reviewable

@codecov
Copy link

codecov bot commented Jun 26, 2020

Codecov Report

Merging #5940 into master will increase coverage by 0.00%.
The diff coverage is 87.14%.

@@            Coverage Diff            @@
##             master    #5940   +/-   ##
=========================================
  Coverage     72.10%   72.11%           
- Complexity    34149    34167   +18     
=========================================
  Files          2807     2809    +2     
  Lines        140390   140450   +60     
  Branches      16862    16878   +16     
=========================================
+ Hits         101235   101283   +48     
- Misses        31253    31260    +7     
- Partials       7902     7907    +5     
Impacted Files Coverage Δ Complexity Δ
...tfish/representation/arista/AristaConversions.java 75.38% <83.33%> (+0.19%) 81.00 <4.00> (+5.00)
...representation/arista/eos/AristaBgpPeerFilter.java 85.71% <85.71%> (ø) 9.00 <9.00> (?)
...sh/grammar/arista/AristaControlPlaneExtractor.java 29.62% <86.36%> (+0.25%) 535.00 <5.00> (+5.00)
...esentation/arista/eos/AristaBgpPeerFilterLine.java 90.90% <90.90%> (ø) 3.00 <3.00> (?)
...ish/representation/arista/AristaConfiguration.java 53.07% <100.00%> (+0.06%) 154.00 <1.00> (+1.00)
...ish/representation/arista/AristaStructureType.java 100.00% <100.00%> (ø) 3.00 <0.00> (ø)
...sh/representation/arista/AristaStructureUsage.java 100.00% <100.00%> (ø) 3.00 <0.00> (ø)
.../src/main/java/org/batfish/datamodel/flow/Hop.java 53.33% <0.00%> (-6.67%) 5.00% <0.00%> (-1.00%)
...rc/main/java/org/batfish/datamodel/flow/Trace.java 87.09% <0.00%> (-3.23%) 11.00% <0.00%> (-2.00%)
...org/batfish/datamodel/flow/BidirectionalTrace.java 81.81% <0.00%> (-2.28%) 14.00% <0.00%> (-1.00%)
... and 6 more

Copy link
Member

@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.

Reviewed 14 of 14 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @progwriter)


projects/batfish/src/main/antlr4/org/batfish/grammar/arista/AristaParser.g4, line 2746 at r1 (raw file):

seq = DEC MATCH | MATCH

I would write this as (seq = DEC)? MATCH

but i'm not sure if this matters in practice - ANTLR4 might optimize away the redundancy anyway. [@arifogel]


projects/batfish/src/main/java/org/batfish/grammar/arista/AristaControlPlaneExtractor.java, line 4066 at r1 (raw file):

AristaBgpPeerFilterLine.Action.valueOf(ctx.action.getText().toUpperCase());

nit: Not in love with using String based conversion with enum.valueOf when we just have things like (ctx.ACCEPT() != null)...


projects/batfish/src/main/java/org/batfish/representation/arista/AristaConversions.java, line 328 at r1 (raw file):

LongSpace.EMPTY;

Confirming: no remoteAs and no peer filter means accept nothing?


projects/batfish/src/main/java/org/batfish/representation/arista/eos/AristaBgpPeerFilter.java, line 59 at r1 (raw file):

            });
    /*
    TODO: fix inclusion semantics

does building from the last line forward work? that is, building the current "stuff allowed after this line" set every time and using intersect/difference/union operators.

Copy link
Member

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


projects/batfish/src/main/antlr4/org/batfish/grammar/arista/AristaParser.g4, line 2737 at r1 (raw file):

PEER_FILTER name = variable 

can we pushMode(M_word) whenever we see PEER_FILTER? That pattern we adopted in NX-OS makes things like names that are 1.2.3.4 cause fewer problems in general :).

Copy link
Contributor Author

@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.

Reviewable status: 9 of 15 files reviewed, all discussions resolved (waiting on @dhalperi)


projects/batfish/src/main/antlr4/org/batfish/grammar/arista/AristaParser.g4, line 2737 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…
PEER_FILTER name = variable 

can we pushMode(M_word) whenever we see PEER_FILTER? That pattern we adopted in NX-OS makes things like names that are 1.2.3.4 cause fewer problems in general :).

done


projects/batfish/src/main/antlr4/org/batfish/grammar/arista/AristaParser.g4, line 2746 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…
seq = DEC MATCH | MATCH

I would write this as (seq = DEC)? MATCH

but i'm not sure if this matters in practice - ANTLR4 might optimize away the redundancy anyway. [@arifogel]

done. wasn't sure if having first token be optional in a rule is a hit w/ adaptive prediction.


projects/batfish/src/main/java/org/batfish/grammar/arista/AristaControlPlaneExtractor.java, line 4066 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…
AristaBgpPeerFilterLine.Action.valueOf(ctx.action.getText().toUpperCase());

nit: Not in love with using String based conversion with enum.valueOf when we just have things like (ctx.ACCEPT() != null)...

done


projects/batfish/src/main/java/org/batfish/representation/arista/AristaConversions.java, line 328 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…
LongSpace.EMPTY;

Confirming: no remoteAs and no peer filter means accept nothing?

correct, i.e., it's not a valid peer


projects/batfish/src/main/java/org/batfish/representation/arista/eos/AristaBgpPeerFilter.java, line 59 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

does building from the last line forward work? that is, building the current "stuff allowed after this line" set every time and using intersect/difference/union operators.

done, good call

Copy link
Member

@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.

Reviewed 6 of 6 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@progwriter progwriter merged commit 5b61952 into batfish:master Jun 26, 2020
@progwriter progwriter deleted the peer-filter branch June 26, 2020 19:10
kylehoferamzn pushed a commit to kylehoferamzn/batfish that referenced this pull request Jul 21, 2020
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