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

BGP: ensure local-pref clips when adding or subtracting #6639

Merged
merged 2 commits into from
Feb 18, 2021

Conversation

dhalperi
Copy link
Member

No description provided.

@batfish-bot
Copy link

This change is Reviewable

@codecov
Copy link

codecov bot commented Feb 18, 2021

Codecov Report

Merging #6639 (4bd1cf9) into master (f9ef848) will increase coverage by 0.00%.
The diff coverage is 55.55%.

@@            Coverage Diff            @@
##             master    #6639   +/-   ##
=========================================
  Coverage     73.45%   73.45%           
- Complexity    36458    36459    +1     
=========================================
  Files          2917     2917           
  Lines        146884   146880    -4     
  Branches      17706    17709    +3     
=========================================
  Hits         107888   107888           
+ Misses        30493    30486    -7     
- Partials       8503     8506    +3     
Impacted Files Coverage Δ Complexity Δ
.../src/main/java/org/batfish/datamodel/BgpRoute.java 84.46% <ø> (ø) 21.00 <0.00> (ø)
.../routing_policy/expr/DecrementLocalPreference.java 64.70% <50.00%> (+47.05%) 7.00 <0.00> (+5.00)
.../routing_policy/expr/IncrementLocalPreference.java 66.66% <60.00%> (+43.93%) 7.00 <0.00> (+5.00)
...g/batfish/datamodel/flow/IncomingSessionScope.java 84.61% <0.00%> (-15.39%) 7.00% <0.00%> (-1.00%)
.../org/batfish/dataplane/rib/RouteAdvertisement.java 83.67% <0.00%> (-4.09%) 18.00% <0.00%> (-1.00%)
...col/src/main/java/org/batfish/role/InferRoles.java 89.54% <0.00%> (-1.37%) 50.00% <0.00%> (-1.00%)
...java/org/batfish/dataplane/ibdp/VirtualRouter.java 90.32% <0.00%> (-0.32%) 206.00% <0.00%> (-1.00%)
...src/main/java/org/batfish/coordinator/WorkMgr.java 75.96% <0.00%> (-0.16%) 247.00% <0.00%> (-1.00%)
...tfish/representation/cisco/CiscoConfiguration.java 86.51% <0.00%> (-0.14%) 508.00% <0.00%> (-1.00%)
.../org/batfish/dataplane/ibdp/BgpRoutingProcess.java 84.79% <0.00%> (-0.12%) 150.00% <0.00%> (-1.00%)
... and 2 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 5 of 5 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dhalperi)


projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/routing_policy/expr/DecrementLocalPreferenceTest.java, line 34 at r1 (raw file):

    Bgpv4Route testRoute =
        Bgpv4Route.testBuilder().setNetwork(Prefix.ZERO).setLocalPreference(0L).build();
    DecrementLocalPreference add5 = new DecrementLocalPreference(5);

add5 is an unusual name for this object


projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/routing_policy/expr/DecrementLocalPreferenceTest.java, line 37 at r1 (raw file):

    // Clips 0 to 0
    assertThat(
        add5.evaluate(Environment.builder(c).setOriginalRoute(testRoute).build()), equalTo(0L));

if i'm being really nitpicky, i'd say you should test this with a route that has local preference 2 or something, so we can see it clips to 0 rather than has no effect. (same thought with the clip test for IncrementLocalPreference.) nbd though.


projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/routing_policy/expr/DecrementLocalPreferenceTest.java, line 50 at r1 (raw file):

                .setOriginalRoute(testRoute.toBuilder().setLocalPreference(105).build())
                .build()),
        equalTo(100L));

why bother having both of these (10 and 105)?

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

@dhalperi dhalperi merged commit 6dbc6e5 into batfish:master Feb 18, 2021
@dhalperi dhalperi deleted the local-pref-clip branch February 18, 2021 18:38
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