Only reject routes with neighbor's router-id for ibgp #541

arifogel · 2017-10-11T00:42:07Z

@progwriter Please add tests. (Not ready for review).

dhalperi · 2017-10-11T01:15:03Z

Re-add me as reviewer when ready for review.

arifogel · 2017-10-11T04:12:49Z

...test/resources/org/batfish/grammar/cisco/testrigs/ibgp-only-reject-routerid-match/configs/r1

+interface Loopback0
+   ip address 1.0.0.1/32
+!
+interface Loopback1


Could this interface be superfluous? Is it used any differently than Loopback0?

arifogel · 2017-10-11T04:13:44Z

...test/resources/org/batfish/grammar/cisco/testrigs/ibgp-only-reject-routerid-match/configs/r1

+   router-id 2.0.0.1
+   neighbor 10.12.0.2 remote-as 2
+   network 1.0.0.1/32
+   network 1.0.0.2/32


See earlier comment about Loopback1.

arifogel · 2017-10-11T04:18:20Z

projects/batfish/src/test/java/org/batfish/bdp/BdpDataPlanePluginTest.java

+    assertTrue(r2aPrefixes.contains(r1Loopback0Prefix));
+    assertTrue(r2aPrefixes.contains(r1Loopback1Prefix));
+    assertTrue(r2bPrefixes.contains(r1Loopback0Prefix));
+    assertTrue(r2bPrefixes.contains(r1Loopback1Prefix));


What do we learn from the presence of the advertised prefixes on r2b?

arifogel

Please address comments about unnecessary structures and iBGP connection discipline.

arifogel · 2017-10-11T21:28:40Z

projects/batfish/src/test/java/org/batfish/bdp/BdpDataPlanePluginTest.java

+
+  @Test
+  public void testEbgpAcceptSameNeighborID() throws IOException {
+    String testrigName = "ibgp-only-reject-routerid-match";


The name of this testrig is not meaningfully different from the name of the other testrig. Perhaps name them after the test function names?

arifogel · 2017-10-11T21:31:18Z

...test/resources/org/batfish/grammar/cisco/testrigs/ibgp-only-reject-routerid-match/configs/r1

+router bgp 1
+!!! This router ID is the same as r3
+   router-id 9.9.9.9
+   neighbor 10.12.0.2 remote-as 2


Please peer on loopbacks for iBGP. Use static routes for each loopback that must be reached as a neighbor address.

arifogel · 2017-10-11T21:33:04Z

...test/resources/org/batfish/grammar/cisco/testrigs/ibgp-only-reject-routerid-match/configs/r2

+ip routing
+!
+route-map 1-to-2 permit 200
+route-map 3-to-2 permit 200


These route-maps and their applications in the bgp stanza serve no purpose.

arifogel · 2017-10-11T21:34:36Z

.../src/test/resources/org/batfish/grammar/cisco/testrigs/ibgp-reject-routerid-match/configs/r1

+router bgp 1
+!!! This router ID is the same as r3
+   router-id 9.9.9.9
+   neighbor 10.14.0.4 remote-as 1


Please use loopbacks to establish iBGP sessions. Add static routes to those loopbacks wherever they are referred to. The next hop should be the physical interface address of the neighbor.

arifogel · 2017-10-11T21:34:47Z

.../src/test/resources/org/batfish/grammar/cisco/testrigs/ibgp-reject-routerid-match/configs/r2

+!
+router bgp 1
+   router-id 2.2.2.2
+   neighbor 10.24.0.4 remote-as 1


Please use loopbacks to establish iBGP sessions. Add static routes to those loopbacks wherever they are referred to. The next hop should be the physical interface address of the neighbor.

arifogel · 2017-10-11T21:34:54Z

.../src/test/resources/org/batfish/grammar/cisco/testrigs/ibgp-reject-routerid-match/configs/r3

+router bgp 1
+!!! same ID as r1
+   router-id 9.9.9.9
+   neighbor 10.34.0.4 remote-as 1


Please use loopbacks to establish iBGP sessions. Add static routes to those loopbacks wherever they are referred to. The next hop should be the physical interface address of the neighbor.

arifogel · 2017-10-11T21:35:00Z

.../src/test/resources/org/batfish/grammar/cisco/testrigs/ibgp-reject-routerid-match/configs/r4

+!
+router bgp 1
+   router-id 4.4.4.4
+   neighbor 10.14.0.1 remote-as 1


Please use loopbacks to establish iBGP sessions. Add static routes to those loopbacks wherever they are referred to. The next hop should be the physical interface address of the neighbor.

arifogel

Almost ready.. just a couple more simple changes needed.

arifogel · 2017-10-11T22:05:31Z

.../src/test/resources/org/batfish/grammar/cisco/testrigs/ebgp-accept-routerid-match/configs/r3

+interface Loopback0
+   ip address 3.0.0.3/32
+!
+ip route 2.0.0.1/32 10.22.0.1


This is not necessary for eBGP.

arifogel · 2017-10-11T22:06:18Z

.../src/test/resources/org/batfish/grammar/cisco/testrigs/ebgp-accept-routerid-match/configs/r2

+ip routing
+!
+route-map 1-to-2 permit 200
+route-map 3-to-2 permit 200


Please remove these route-maps and their references below.

arifogel · 2017-10-11T22:09:33Z

.../src/test/resources/org/batfish/grammar/cisco/testrigs/ibgp-reject-routerid-match/configs/r4

+!
+!ip route 1.0.0.1/32 10.14.0.1
+!ip route 2.0.0.1/32 10.24.0.2
+!ip route 3.0.0.1/32 10.34.0.3


These routes should not be commented.

…es as highlighted by Ari.

arifogel · 2017-10-11T23:43:09Z

Looks good. Sending to @dhalperi for review.

dhalperi

Minor suggestions.

dhalperi · 2017-10-12T01:33:02Z

projects/batfish/src/main/java/org/batfish/bdp/VirtualRouter.java

@@ -820,7 +820,8 @@ public int propagateBgpRoutes(Map<String, Node> nodes, Map<Ip, Set<String>> ipOw

              Ip remoteOriginatorIp = bgpRemoteRoute.getOriginatorIp();
              // don't accept routes whose originator ip is my BGP id


update comment? external BGP routes?

dhalperi · 2017-10-12T01:36:31Z

projects/batfish/src/test/java/org/batfish/bdp/BdpDataPlanePluginTest.java

+    Prefix r1Loopback0Prefix = new Prefix("1.0.0.1/32");
+    Prefix r3Loopback0Prefix = new Prefix("3.0.0.3/32");
+    // Ensure that r3loopback was accepted by r1
+    assertTrue(r1Prefixes.contains(r3Loopback0Prefix));


assertThat(r1Prefixes, contains(r3Loopback0prefix))

Will produce better error messages.

(e.g.,

"Expecting an iterable containing X, but was an iterable: [a,b,c]"

dhalperi · 2017-10-12T01:40:17Z

projects/batfish/src/test/java/org/batfish/bdp/BdpDataPlanePluginTest.java

+    // Ensure that the prefix is accepted by r2, because router ids are different
+    assertTrue(r2Prefixes.contains(r1AdvertisedPrefix));
+    // Ensure that the prefix is rejected by r3, because router ids are the same
+    assertFalse(r3Prefixes.contains(r1AdvertisedPrefix));


same assert questions as before

dhalperi · 2017-10-12T01:42:48Z

projects/batfish/src/test/java/org/batfish/bdp/BdpDataPlanePluginTest.java

+    SortedSet<AbstractRoute> r2Routes = routes.get("r2").get(Configuration.DEFAULT_VRF_NAME);
+    SortedSet<AbstractRoute> r3Routes = routes.get("r3").get(Configuration.DEFAULT_VRF_NAME);
+    Set<Prefix> r2Prefixes = r2Routes.stream().map(r -> r.getNetwork()).collect(Collectors.toSet());
+    Set<Prefix> r3Prefixes = r3Routes.stream().map(r -> r.getNetwork()).collect(Collectors.toSet());


Style advice: I like to structure my tests into 2/3 clear sections.

Initialization ("nothing interesting here") and constants.

The actual code that is under test. As short as possible.

Assertions. (perhaps merged with 2, if 2 is one line.)

I usually have whitespace and/or comments between them.

Also, make use of helpers for any common code. (see makeFlow, makeAcl, etc.)

An example: fd022f5#diff-7ee7600d68323ad23eae06bf4434516cR71

I learned this over time as a pattern that makes tests readable -- all the "background" is at the top and all the "tests" are at the bottom. Whitespace helps.

Feel free to go your own way.

only reject routes with neighbor's router-id for ibgp

2a9a129

arifogel assigned progwriter Oct 11, 2017

arifogel requested a review from dhalperi October 11, 2017 00:42

dhalperi removed their request for review October 11, 2017 01:14

Attempt at crafting a test for ebgp router id match

25a9532

progwriter requested a review from dhalperi October 11, 2017 03:42

arifogel removed the request for review from dhalperi October 11, 2017 03:57

arifogel commented Oct 11, 2017

View reviewed changes

Victor Heorhiadi added 4 commits October 11, 2017 10:08

Trimming down superfluous interfaces and checks

713f754

Updated test rig to test presence of routes in ebgp case.

c22cb12

Separate rig with only ibgp test.

f3b62b9

Removed debugging output

c31e80c

arifogel commented Oct 11, 2017

View reviewed changes

Victor Heorhiadi added 2 commits October 11, 2017 14:54

Switched to loopback pairing for ibgp test rig

c0ff3db

Renamed one of the test rigs to better reflect the point of the test

6999706

arifogel commented Oct 11, 2017

View reviewed changes

Removed unnecessary directives for ebgp test case; fixes to ibgp rout…

da4ff7b

…es as highlighted by Ari.

arifogel requested a review from dhalperi October 11, 2017 23:43

arifogel and others added 4 commits October 11, 2017 18:09

Merge remote-tracking branch 'origin/master' into ari-fix-routerid-usage

f76f5c8

use new unit test utility api

22ee7d6

formatting

efb8ffc

Merge remote-tracking branch 'origin/master' into ari-fix-routerid-usage

1f7c9b9

dhalperi requested changes Oct 12, 2017

View reviewed changes

address review comments

a75d46c

arifogel requested a review from dhalperi October 12, 2017 02:00

use correct matcher

302fc52

arifogel requested review from dhalperi and removed request for dhalperi October 12, 2017 02:06

dhalperi approved these changes Oct 12, 2017

View reviewed changes

dhalperi merged commit 62ace61 into master Oct 12, 2017

dhalperi deleted the ari-fix-routerid-usage branch October 12, 2017 04:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only reject routes with neighbor's router-id for ibgp #541

Only reject routes with neighbor's router-id for ibgp #541

arifogel commented Oct 11, 2017 •

edited

dhalperi commented Oct 11, 2017

arifogel Oct 11, 2017

arifogel Oct 11, 2017

arifogel Oct 11, 2017

arifogel left a comment

arifogel Oct 11, 2017

arifogel Oct 11, 2017

arifogel Oct 11, 2017

arifogel Oct 11, 2017

arifogel Oct 11, 2017

arifogel Oct 11, 2017

arifogel Oct 11, 2017

arifogel left a comment

arifogel Oct 11, 2017

arifogel Oct 11, 2017

arifogel Oct 11, 2017

arifogel commented Oct 11, 2017

dhalperi left a comment

dhalperi Oct 12, 2017

dhalperi Oct 12, 2017

dhalperi Oct 12, 2017

dhalperi Oct 12, 2017

		@@ -820,7 +820,8 @@ public int propagateBgpRoutes(Map<String, Node> nodes, Map<Ip, Set<String>> ipOw

		Ip remoteOriginatorIp = bgpRemoteRoute.getOriginatorIp();
		// don't accept routes whose originator ip is my BGP id

Only reject routes with neighbor's router-id for ibgp #541

Only reject routes with neighbor's router-id for ibgp #541

Conversation

arifogel commented Oct 11, 2017 • edited

dhalperi commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arifogel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arifogel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arifogel commented Oct 11, 2017

dhalperi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arifogel commented Oct 11, 2017 •

edited