Skip to content

Parsing for Junos static ipv6 routes#9241

Merged
SLarkworthy merged 1 commit intobatfish:masterfrom
SLarkworthy:sarah-junos-ipv6
Oct 8, 2024
Merged

Parsing for Junos static ipv6 routes#9241
SLarkworthy merged 1 commit intobatfish:masterfrom
SLarkworthy:sarah-junos-ipv6

Conversation

@SLarkworthy
Copy link
Copy Markdown
Contributor

@SLarkworthy SLarkworthy requested a review from dhalperi October 8, 2024 14:41
@batfish-bot
Copy link
Copy Markdown

This change is Reviewable

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 8, 2024

Codecov Report

Attention: Patch coverage is 76.92308% with 6 lines in your changes missing coverage. Please review.

Project coverage is 72.72%. Comparing base (ab731e3) to head (2cb421b).
Report is 51 commits behind head on master.

Files with missing lines Patch % Lines
...fish/grammar/flatjuniper/ConfigurationBuilder.java 76.92% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9241      +/-   ##
==========================================
- Coverage   72.73%   72.72%   -0.01%     
==========================================
  Files        3314     3314              
  Lines      170027   170046      +19     
  Branches    20046    20045       -1     
==========================================
+ Hits       123665   123674       +9     
- Misses      37203    37211       +8     
- Partials     9159     9161       +2     
Files with missing lines Coverage Δ
...fish/grammar/flatjuniper/ConfigurationBuilder.java 69.47% <76.92%> (-0.01%) ⬇️

... and 3 files with indirect coverage changes

@dhalperi
Copy link
Copy Markdown
Member

dhalperi commented Oct 8, 2024

projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniper_routing_instances.g4 line 340 at r1 (raw file):

;

ro_static6

I might suggest this should be ror6_static:

routing-options rib <somethingv6 like inet6.0> is why I put the 6 there after the r for rib. Then it's just static.

This would imply more for child prefixes.

Copy link
Copy Markdown
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 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @SLarkworthy)


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniper_common.g4 line 245 at r1 (raw file):

ipv6_prefix: IPV6_PREFIX;

ipv6_address: IPV6_ADDRESS;

isn't this the same as 2 lines up?


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniper_routing_instances.g4 line 661 at r1 (raw file):

:
    ROUTE
   (

you can get rid of the parens now to clean it up.


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniper_routing_instances.g4 line 662 at r1 (raw file):

    ROUTE
   (
      prefix = ip_prefix_default_32

note I refer to this one below.


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniper_routing_instances.g4 line 673 at r1 (raw file):

:
    ROUTE
   (

ditto on parens.


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniper_routing_instances.g4 line 674 at r1 (raw file):

    ROUTE
   (
       prefix = ipv6_prefix

you may want ipv6_prefix_default_128 like we have ip_prefix_default_32. It's basically prefix or address both allowed, and if address we make it a /128.


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniperLexer.g4 line 2388 at r1 (raw file):

RIB: 'rib'
    {
        switch(lastTokenType()) {

yeah let's chat about this one. Probably totally reasonable, not sure.


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniperLexer.g4 line 3629 at r1 (raw file):

F_RoutingInstanceNameChar
:
   [A-Za-z] | '-'

maybe add a link?


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniperLexer.g4 line 4983 at r1 (raw file):

M_RibName_PERIOD: '.' -> type(PERIOD);
M_RibName_UINT8: F_Uint8 -> type(UINT8), popMode;
M_RibName_WS: F_WhitespaceChar+ -> skip;

hmm this is what I expected would handle those spaces, so what the heck :)


projects/batfish/src/test/java/org/batfish/grammar/flatjuniper/FlatJuniperGrammarTest.java line 6504 at r1 (raw file):

            .get("TEST-VRF.inet6.0");
    {
      assertTrue(routesDefault.getStaticRoutes().isEmpty());

since the variable names are different we don't need the scoping {}. That's needed only if you want to prevent leaks out of context.

I might say:

assertThat(c.getMasterLogicalSystem().getRoutingInstances().get("default").getRibs().get(...).getStaticRoutes(),
    isEmpty())
``` or something like that.

The use of `isEmpty()` is nice because it will not just present failure, it will say "expected empty, but got [...]"

Copy link
Copy Markdown
Contributor Author

@SLarkworthy SLarkworthy 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: 1 of 8 files reviewed, 5 unresolved discussions (waiting on @dhalperi)


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniper_common.g4 line 245 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

isn't this the same as 2 lines up?

whoops, removed.


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniper_routing_instances.g4 line 340 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

I might suggest this should be ror6_static:

routing-options rib <somethingv6 like inet6.0> is why I put the 6 there after the r for rib. Then it's just static.

This would imply more for child prefixes.

done


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniper_routing_instances.g4 line 661 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

you can get rid of the parens now to clean it up.

done


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniper_routing_instances.g4 line 674 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

you may want ipv6_prefix_default_128 like we have ip_prefix_default_32. It's basically prefix or address both allowed, and if address we make it a /128.

I was wondering about that - done.


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniperLexer.g4 line 2388 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

yeah let's chat about this one. Probably totally reasonable, not sure.

As discussed, changed the lexer so the name is a single token.


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniperLexer.g4 line 3629 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

maybe add a link?

done


projects/batfish/src/test/java/org/batfish/grammar/flatjuniper/FlatJuniperGrammarTest.java line 6504 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

since the variable names are different we don't need the scoping {}. That's needed only if you want to prevent leaks out of context.

I might say:

assertThat(c.getMasterLogicalSystem().getRoutingInstances().get("default").getRibs().get(...).getStaticRoutes(),
    isEmpty())
``` or something like that.

The use of `isEmpty()` is nice because it will not just present failure, it will say "expected empty, but got [...]"

I couldn't find a matcher for an empty map, but there is one for an empty collection, so I went with making sure the keySet was empty.

Also I know it's weird to factor out the routesDefault/routesTestVrf vars but I'll be adding more tests for each of them once extraction is implemented.

Copy link
Copy Markdown
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 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SLarkworthy)


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniperLexer.g4 line 4972 at r2 (raw file):

M_RibName_INET: (F_RoutingInstanceNameChar+ PERIOD)? INET PERIOD UINT8 -> type(INET_RIB_NAME), popMode;
M_RibName_INET6: (F_RoutingInstanceNameChar+ PERIOD)? INET6 PERIOD UINT8 -> type(INET6_RIB_NAME), popMode;
M_RibName_MPLS: MPLS PERIOD UINT8 -> type(MPLS_RIB_NAME), popMode;

why omit VRF names here? I assume they're still relevant.

Copy link
Copy Markdown
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 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @SLarkworthy)

@SLarkworthy SLarkworthy enabled auto-merge (squash) October 8, 2024 23:08
@SLarkworthy SLarkworthy merged commit 8d51ebc into batfish:master Oct 8, 2024
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.

3 participants