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

Juniper add-path parsing, extraction, and reference tracking #8369

Merged
merged 3 commits into from
Jun 14, 2022

Conversation

arifogel
Copy link
Member

No description provided.

@batfish-bot
Copy link

This change is Reviewable

@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #8369 (af18f5d) into master (8382f6a) will decrease coverage by 0.00%.
The diff coverage is 98.21%.

@@             Coverage Diff              @@
##             master    #8369      +/-   ##
============================================
- Coverage     74.74%   74.73%   -0.01%     
- Complexity    44028    44053      +25     
============================================
  Files          3419     3422       +3     
  Lines        170080   170136      +56     
  Branches      20289    20291       +2     
============================================
+ Hits         127119   127159      +40     
- Misses        33367    33381      +14     
- Partials       9594     9596       +2     
Impacted Files Coverage Δ
...fish/grammar/flatjuniper/ConfigurationBuilder.java 75.90% <96.00%> (+0.12%) ⬆️
...va/org/batfish/representation/juniper/AddPath.java 100.00% <100.00%> (ø)
...rg/batfish/representation/juniper/AddPathSend.java 100.00% <100.00%> (ø)
...a/org/batfish/representation/juniper/BgpGroup.java 90.90% <100.00%> (+0.39%) ⬆️
.../representation/juniper/JuniperStructureUsage.java 100.00% <100.00%> (ø)
...fish/representation/juniper/PathSelectionMode.java 100.00% <100.00%> (ø)
...fish/bddreachability/BDDLoopDetectionAnalysis.java 83.82% <0.00%> (-2.95%) ⬇️
...src/main/java/org/batfish/coordinator/PoolMgr.java 59.52% <0.00%> (-1.20%) ⬇️
...ain/java/org/batfish/storage/FileBasedStorage.java 86.00% <0.00%> (-0.84%) ⬇️
...batfish/src/main/java/org/batfish/main/Driver.java 49.18% <0.00%> (-0.55%) ⬇️
... and 4 more

Copy link
Contributor

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

Reviewed 8 of 10 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @arifogel)


projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 7140 at r2 (raw file):

  @Override
  public void exitBfiuas_prefix_policy(Bfiuas_prefix_policyContext ctx) {
    todo(ctx);

Seems redundant to warn here since we already warn on every add-path line.


projects/batfish/src/main/java/org/batfish/representation/juniper/BgpGroup.java line 62 at r2 (raw file):

      _parent.cascadeInheritance();
      if (_addPath == null) {
        _addPath = _parent._addPath;

Do individual properties cascade? E.g. if the parent sets add-path receive and the group has add-path send properties, does the group use the parent's receive setting?


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

    BgpGroup n3 = ri.getIpBgpGroups().get(Prefix.strict("10.0.0.3/32"));

    assertTrue(master.getAddPath().getReceive());

Please test that receive is false for some other add-path object.

Copy link
Member Author

@arifogel arifogel 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, 2 unresolved discussions (waiting on @corinaminer)


projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 7140 at r2 (raw file):

Previously, corinaminer (Corina Miner) wrote…

Seems redundant to warn here since we already warn on every add-path line.

Ah yeah started being specific but switched to general and missed this.
Fixed.


projects/batfish/src/main/java/org/batfish/representation/juniper/BgpGroup.java line 62 at r2 (raw file):

Previously, corinaminer (Corina Miner) wrote…

Do individual properties cascade? E.g. if the parent sets add-path receive and the group has add-path send properties, does the group use the parent's receive setting?

The entire add-path hierarchy is the finest level of inheritance here.
If you have any add-path settings at all at a level, you don't inherit from coarser level.
This also means that each add-path hierarchy must be valid by itself, since there are no merged sub-properties.
Tested on device and in wireshark.


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

Previously, corinaminer (Corina Miner) wrote…

Please test that receive is false for some other add-path object.

done

Copy link
Contributor

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

:lgtm:

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @arifogel)


projects/batfish/src/main/java/org/batfish/representation/juniper/BgpGroup.java line 62 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

The entire add-path hierarchy is the finest level of inheritance here.
If you have any add-path settings at all at a level, you don't inherit from coarser level.
This also means that each add-path hierarchy must be valid by itself, since there are no merged sub-properties.
Tested on device and in wireshark.

sweet, thanks

@arifogel arifogel merged commit ea84746 into batfish:master Jun 14, 2022
@arifogel arifogel deleted the ari-juniper-add-path-parsing branch June 14, 2022 21:19
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