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: set received from IP on import #6501

Merged
merged 1 commit into from
Dec 12, 2020
Merged

Conversation

dhalperi
Copy link
Member

Cleaner than trying to set it on export, which we have to get right across lots
of different paths. Also resolves issues with IBGP routes sometimes being sent
too far.

Fix #6490

@batfish-bot
Copy link

This change is Reviewable

Copy link
Member Author

@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: 0 of 10 files reviewed, 2 unresolved discussions (waiting on @dhalperi)


projects/batfish/src/main/java/org/batfish/dataplane/protocols/BgpProtocolHelper.java, line 108 at r1 (raw file):

Quoted 14 lines of code…
    if (route.getCommunities().getCommunities().contains(StandardCommunity.NO_ADVERTISE)) {
      return null;
    }
    // Do not export route if its AS path contains the remote peer's AS and local peer has not set
    // getAllowRemoteOut
    if (sessionProperties.isEbgp()
        && route.getAsPath().containsAs(sessionProperties.getTailAs())
        && !af.getAddressFamilyCapabilities().getAllowRemoteAsOut()) {
      return null;
    }
    // Do not export if route has NO_EXPORT community and this is a true ebgp session
    if (route.getCommunities().getCommunities().contains(StandardCommunity.NO_EXPORT)
        && sessionProperties.isEbgp()
        && sessionProperties.getConfedSessionType() != ConfedSessionType.WITHIN_CONFED) {

revert


projects/batfish/src/main/java/org/batfish/representation/cumulus/CumulusConversions.java, line 822 at r1 (raw file):

        boolean nextHopSelfAll = firstNonNull(ipv4af.getNextHopSelfAll(), Boolean.FALSE);
        if (!nextHopSelfAll) {
          nextHopSelf = false;

revert


projects/batfish/src/test/java/org/batfish/dataplane/ibdp/BgpNextHopUnchangedTest.java, line 177 at r1 (raw file):

                        .build()
                        .getName())
                .setRouteReflectorClient(!ebgp)

FYI This is the only non-trivial test change. Had to make the middle router a reflector so it actually does reflect routes to the third router in the chain.

Cleaner than trying to set it on export, which we have to get right across lots
of different paths. Also resolves issues with IBGP routes sometimes being sent
too far.

Fix batfish#6490
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 8 of 10 files at r1, 5 of 6 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@codecov
Copy link

codecov bot commented Dec 12, 2020

Codecov Report

Merging #6501 (0174588) into master (1d829e9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #6501   +/-   ##
=========================================
  Coverage     73.31%   73.31%           
- Complexity    35512    35517    +5     
=========================================
  Files          2824     2824           
  Lines        143511   143513    +2     
  Branches      17337    17337           
=========================================
+ Hits         105216   105220    +4     
- Misses        29960    29961    +1     
+ Partials       8335     8332    -3     
Impacted Files Coverage Δ Complexity Δ
.../src/main/java/org/batfish/datamodel/BgpRoute.java 83.18% <ø> (ø) 24.00 <0.00> (ø)
.../org/batfish/dataplane/ibdp/BgpRoutingProcess.java 81.98% <100.00%> (+0.04%) 130.00 <0.00> (ø)
...batfish/dataplane/protocols/BgpProtocolHelper.java 94.65% <100.00%> (+0.62%) 45.00 <0.00> (-1.00) ⬆️
...tfish/representation/cisco/CiscoConfiguration.java 86.28% <0.00%> (-0.14%) 502.00% <0.00%> (-1.00%)
...java/org/batfish/common/topology/TopologyUtil.java 92.95% <0.00%> (+0.05%) 154.00% <0.00%> (+4.00%)
...ar/cumulus_frr/CumulusFrrConfigurationBuilder.java 74.48% <0.00%> (+0.15%) 165.00% <0.00%> (+1.00%)
...ish/representation/cumulus/CumulusConversions.java 91.75% <0.00%> (+0.36%) 190.00% <0.00%> (+2.00%)

@dhalperi
Copy link
Member Author

a discussion (no related file):
Working: Self-merge only.


@dhalperi dhalperi merged commit 2c06e02 into batfish:master Dec 12, 2020
@dhalperi dhalperi deleted the gh-6490 branch December 12, 2020 01:58
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.

FRR: Admin distance + NHS causes route to not appear upstream
3 participants