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

Supporting per neighbor EIGRP distribute list in dataplane + accompanying DP changes #4368

Merged
merged 17 commits into from
Jul 29, 2019

Conversation

haverma
Copy link

@haverma haverma commented Jul 27, 2019

To be reviewed after #4367
Changes include

  • exporting and sending out EIGRP external routes per neighbor after applying per neighbor export policy
  • applying export policy filter while sending out EIGRP internal routes
  • calling redistribute in OSPF and EIGRP with main RIB delta instead of the entire main RIB from second iterations onwards

also should fix #4133

@batfish-bot
Copy link

This change is Reviewable

@haverma haverma requested a review from progwriter July 27, 2019 06:28
@codecov
Copy link

codecov bot commented Jul 27, 2019

Codecov Report

Merging #4368 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #4368   +/-   ##
=========================================
  Coverage      74.8%    74.8%           
  Complexity    26322    26322           
=========================================
  Files          2135     2135           
  Lines        105499   105499           
  Branches      12912    12912           
=========================================
  Hits          78922    78922           
  Misses        20614    20614           
  Partials       5963     5963

Copy link
Author

@haverma haverma 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, 1 unresolved discussion (waiting on @progwriter)

a discussion (no related file):
ready for review


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 10 of 12 files at r1, 1 of 1 files at r2, 1 of 2 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @haverma)


projects/batfish/src/main/java/org/batfish/dataplane/ibdp/EigrpRoutingProcess.java, line 195 at r3 (raw file):

   * export policy
   */
  private void exportRedistributedForAllNeighbors(

nit: just for sake of consistency with existing code, drop the forAllNeighbors suffix and rename the function below to have perNeighbor suffix.


projects/batfish/src/main/java/org/batfish/dataplane/ibdp/VirtualRouter.java, line 2015 at r3 (raw file):

                    // For the time being use all main RIB routes
                    numIterations == 1
                        ? RibDelta.<AnnotatedRoute<AbstractRoute>>builder()

I feel like this bit is important to separate into its own PR. (should just be two files: this and the engine)


projects/batfish/src/test/java/org/batfish/dataplane/ibdp/EigrpTest.java, line 930 at r3 (raw file):

    // only 1.1.1.0/24 allowed to be exported to r2
    assertNoRoute(routes, "r2", Prefix.parse("3.3.3.0/24"));
    assertRoute(routes, EIGRP_EX, "r2", Prefix.parse("1.1.1.0/24"), 2585856L);
  1. assertNoRoute 2.2.2.0/24
  2. how did you arrive at the metric value?

projects/batfish/src/test/resources/org/batfish/dataplane/ibdp/eigrp-distribute-lists/configs/r1, line 2 at r3 (raw file):

!
hostname r1

nit: call this advertiser, and the other receiver/listener.


projects/batfish/src/test/resources/org/batfish/dataplane/ibdp/eigrp-distribute-lists/configs/r2, line 4 at r3 (raw file):

hostname r2
!
access-list 2 permit 172.21.30.0 0.0.0.255

no need for this line, right?

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.

:lgtm:

Reviewed 5 of 5 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion

Copy link
Author

@haverma haverma 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: :shipit: complete! all files reviewed, all discussions resolved


projects/batfish/src/main/java/org/batfish/dataplane/ibdp/EigrpRoutingProcess.java, line 195 at r3 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

nit: just for sake of consistency with existing code, drop the forAllNeighbors suffix and rename the function below to have perNeighbor suffix.

done, in this and below


projects/batfish/src/main/java/org/batfish/dataplane/ibdp/VirtualRouter.java, line 2015 at r3 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

I feel like this bit is important to separate into its own PR. (should just be two files: this and the engine)

done


projects/batfish/src/test/java/org/batfish/dataplane/ibdp/EigrpTest.java, line 930 at r3 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…
  1. assertNoRoute 2.2.2.0/24
  2. how did you arrive at the metric value?
  1. discussed offline, skipping it because it will be overridden by connected route so never will be installed
  2. removed metric from the assertion

projects/batfish/src/test/resources/org/batfish/dataplane/ibdp/eigrp-distribute-lists/configs/r1, line 2 at r3 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

nit: call this advertiser, and the other receiver/listener.

done


projects/batfish/src/test/resources/org/batfish/dataplane/ibdp/eigrp-distribute-lists/configs/r2, line 4 at r3 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

no need for this line, right?

removed

@haverma haverma merged commit 13647b7 into master Jul 29, 2019
@haverma haverma deleted the eigrp-dl-dp branch July 29, 2019 22:00
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.

EIGRP route filtering support
3 participants