Prevent route replication loop

Scenario: With two VRF importing each other and with routing policy attached to both VRF to edit the BgpAttr of the routes, each DBEntry notification for route replication would end up deleting and re-adding back the replicated route and notifying the secondary route. This would cause an endless route replication loop Solution: Search for secondary path while replicating the route, should compare the BgpAttr with original BgpAttr of the secondary path. Added test to validate the above fix. i.e. time stamp of the replicated path is not updated on triggering the notification of the primary route. Change-Id: I3e6b31e8b36efbc091353e828b4f9dfa809cf5ff Closes-Bug: #1597687
Juniper · Jul 1, 2016 · ed92b45 · ed92b45
1 parent 463d159
commit ed92b45
Show file tree

Hide file tree

Showing 8 changed files with 172 additions and 6 deletions.
diff --git a/src/bgp/ermvpn/ermvpn_table.cc b/src/bgp/ermvpn/ermvpn_table.cc
@@ -120,7 +120,7 @@ BgpRoute *ErmVpnTable::RouteReplicate(BgpServer *server,
             src_path->GetSource(), src_path->GetPeer(),
             src_path->GetPathId());
     if (dest_path != NULL) {
-        if (new_attr != dest_path->GetAttr()) {
+        if (new_attr != dest_path->GetOriginalAttr()) {
             bool success = dest_route->RemoveSecondaryPath(src_rt,
                 src_path->GetSource(), src_path->GetPeer(),
                 src_path->GetPathId());

diff --git a/src/bgp/evpn/evpn_table.cc b/src/bgp/evpn/evpn_table.cc
@@ -175,7 +175,7 @@ BgpRoute *EvpnTable::RouteReplicate(BgpServer *server,
             src_path->GetSource(), src_path->GetPeer(),
             src_path->GetPathId());
     if (dest_path != NULL) {
-        if ((new_attr != dest_path->GetAttr()) ||
+        if ((new_attr != dest_path->GetOriginalAttr()) ||
             (src_path->GetLabel() != dest_path->GetLabel())) {
             bool success = dest_route->RemoveSecondaryPath(src_rt,
                 src_path->GetSource(), src_path->GetPeer(),

diff --git a/src/bgp/inet/inet_table.cc b/src/bgp/inet/inet_table.cc
@@ -93,7 +93,7 @@ BgpRoute *InetTable::RouteReplicate(BgpServer *server,
                                           path->GetSource(), path->GetPeer(),
                                           path->GetPathId());
     if (dest_path != NULL) {
-        if ((new_attr != dest_path->GetAttr()) ||
+        if ((new_attr != dest_path->GetOriginalAttr()) ||
             (path->GetLabel() != dest_path->GetLabel())) {
             // Update Attributes and notify (if needed)
             assert(dest_route->RemoveSecondaryPath(src_rt, path->GetSource(),

diff --git a/src/bgp/inet6/inet6_table.cc b/src/bgp/inet6/inet6_table.cc
@@ -94,7 +94,7 @@ BgpRoute *Inet6Table::RouteReplicate(BgpServer *server, BgpTable *src_table,
         dest_route->FindSecondaryPath(src_rt, path->GetSource(),
                                       path->GetPeer(), path->GetPathId());
     if (dest_path != NULL) {
-        if ((new_attr != dest_path->GetAttr()) ||
+        if ((new_attr != dest_path->GetOriginalAttr()) ||
             (path->GetLabel() != dest_path->GetLabel())) {
             // Update Attributes and notify (if needed)
             assert(dest_route->RemoveSecondaryPath(src_rt, path->GetSource(),

diff --git a/src/bgp/inet6vpn/inet6vpn_table.cc b/src/bgp/inet6vpn/inet6vpn_table.cc
@@ -98,7 +98,7 @@ BgpRoute *Inet6VpnTable::RouteReplicate(BgpServer *server, BgpTable *src_table,
                                       src_path->GetPeer(),
                                       src_path->GetPathId());
     if (dest_path != NULL) {
-        if ((new_attr != dest_path->GetAttr()) ||
+        if ((new_attr != dest_path->GetOriginalAttr()) ||
             (src_path->GetLabel() != dest_path->GetLabel())) {
             // Update Attributes and notify (if needed)
             assert(dest_route->RemoveSecondaryPath(source_rt,

diff --git a/src/bgp/l3vpn/inetvpn_table.cc b/src/bgp/l3vpn/inetvpn_table.cc
@@ -102,7 +102,7 @@ BgpRoute *InetVpnTable::RouteReplicate(BgpServer *server,
                                       src_path->GetPeer(),
                                       src_path->GetPathId());
     if (dest_path != NULL) {
-        if ((new_attr != dest_path->GetAttr()) ||
+        if ((new_attr != dest_path->GetOriginalAttr()) ||
             (src_path->GetLabel() != dest_path->GetLabel())) {
             // Update Attributes and notify (if needed)
             assert(dest_route->RemoveSecondaryPath(src_rt,

diff --git a/src/bgp/test/routing_policy_test.cc b/src/bgp/test/routing_policy_test.cc
@@ -2495,6 +2495,131 @@ TEST_F(RoutingPolicyTest, ProtocolMatchStaticRoute) {
     DeleteRoute<InetDefinition>(peers_[0], "nat.inet.0", "192.168.1.254/32");
 }
 
+//
+// 1. two routing instance test_0 and test_1
+// 2. Routing instnaces are connected with each other,
+// 3. two routing policies to edit local pref of matching route.
+// With above configuration, validate that route notify doesn't trigger
+// re-addition of replicated route paths.
+//
+TEST_F(RoutingPolicyTest, Policy_RouteReplicate) {
+    string content =
+        FileRead("controller/src/bgp/testdata/routing_policy_7i.xml");
+    EXPECT_TRUE(parser_.Parse(content));
+    task_util::WaitForIdle();
+
+    ifmap_test_util::IFMapMsgLink(&config_db_, "routing-instance", "test_0",
+                                  "routing-instance", "test_1", "connection");
+    boost::system::error_code ec;
+    peers_.push_back(
+        new BgpPeerMock(Ip4Address::from_string("192.168.0.1", ec)));
+    peers_.push_back(
+        new BgpPeerMock(Ip4Address::from_string("192.168.0.2", ec)));
+
+    AddRoute<Inet6Definition>(peers_[0], "test_0.inet6.0",
+                  "2001:db8:85a3::8a2e:370:7335/128", 100, list_of("23:13"));
+    AddRoute<Inet6Definition>(peers_[1], "test_1.inet6.0",
+                  "2001:db8:85a3::8a2e:370:7335/128", 100, list_of("23:13"));
+    task_util::WaitForIdle();
+
+    VERIFY_EQ(1, RouteCount("test_1.inet6.0"));
+    VERIFY_EQ(1, RouteCount("test_0.inet6.0"));
+    BgpRoute *rt = RouteLookup<Inet6Definition>("test_0.inet6.0",
+                                            "2001:db8:85a3::8a2e:370:7335/128");
+    ASSERT_TRUE(rt != NULL);
+    ASSERT_TRUE(rt->count() == 2);
+    VERIFY_EQ(peers_[0], rt->BestPath()->GetPeer());
+    ASSERT_TRUE(rt->BestPath()->IsFeasible() == true);
+    const BgpAttr *attr = rt->BestPath()->GetAttr();
+    const BgpAttr *orig_attr = rt->BestPath()->GetOriginalAttr();
+    uint32_t original_local_pref = orig_attr->local_pref();
+    uint32_t policy_local_pref = attr->local_pref();
+    ASSERT_TRUE(policy_local_pref == 200);
+    ASSERT_TRUE(original_local_pref == 100);
+
+    // Take the time stamp of the replicated path on the route.
+    // This will be compared with replicated path after triggering dummy
+    // notification of the primary route.
+    uint64_t rt1_replicated_path_time_stamp;
+    for (Route::PathList::iterator it = rt->GetPathList().begin();
+         it != rt->GetPathList().end(); it++) {
+        BgpPath *path = static_cast<BgpPath *>(it.operator->());
+        if (path->IsReplicated())
+            rt1_replicated_path_time_stamp = path->time_stamp_usecs();
+    }
+
+    rt = RouteLookup<Inet6Definition>("test_1.inet6.0",
+                                      "2001:db8:85a3::8a2e:370:7335/128");
+    ASSERT_TRUE(rt != NULL);
+    ASSERT_TRUE(rt->count() == 2);
+    VERIFY_EQ(peers_[0], rt->BestPath()->GetPeer());
+    ASSERT_TRUE(rt->BestPath()->IsFeasible() == true);
+    attr = rt->BestPath()->GetAttr();
+    orig_attr = rt->BestPath()->GetOriginalAttr();
+    original_local_pref = orig_attr->local_pref();
+    policy_local_pref = attr->local_pref();
+    ASSERT_TRUE(policy_local_pref == 50);
+    ASSERT_TRUE(original_local_pref == 200);
+
+    uint64_t rt2_replicated_path_time_stamp;
+    for (Route::PathList::iterator it = rt->GetPathList().begin();
+         it != rt->GetPathList().end(); it++) {
+        BgpPath *path = static_cast<BgpPath *>(it.operator->());
+        if (path->IsReplicated())
+            rt2_replicated_path_time_stamp = path->time_stamp_usecs();
+    }
+
+    // Trigger dummy change notification on the primary routes
+    AddRoute<Inet6Definition>(peers_[0], "test_0.inet6.0",
+                  "2001:db8:85a3::8a2e:370:7335/128", 100, list_of("23:13"));
+    AddRoute<Inet6Definition>(peers_[1], "test_1.inet6.0",
+                  "2001:db8:85a3::8a2e:370:7335/128", 100, list_of("23:13"));
+    task_util::WaitForIdle();
+
+    VERIFY_EQ(1, RouteCount("test_1.inet6.0"));
+    VERIFY_EQ(1, RouteCount("test_0.inet6.0"));
+
+    rt = RouteLookup<Inet6Definition>("test_0.inet6.0",
+                                            "2001:db8:85a3::8a2e:370:7335/128");
+    ASSERT_TRUE(rt != NULL);
+    ASSERT_TRUE(rt->count() == 2);
+
+    uint64_t rt1_replicated_path_time_stamp_after_change;
+    for (Route::PathList::iterator it = rt->GetPathList().begin();
+         it != rt->GetPathList().end(); it++) {
+        BgpPath *path = static_cast<BgpPath *>(it.operator->());
+        if (path->IsReplicated())
+            rt1_replicated_path_time_stamp_after_change
+                = path->time_stamp_usecs();
+    }
+    ASSERT_TRUE(rt1_replicated_path_time_stamp_after_change
+                == rt1_replicated_path_time_stamp);
+
+    rt = RouteLookup<Inet6Definition>("test_1.inet6.0",
+                                      "2001:db8:85a3::8a2e:370:7335/128");
+    ASSERT_TRUE(rt != NULL);
+    ASSERT_TRUE(rt->count() == 2);
+
+    uint64_t rt2_replicated_path_time_stamp_after_change;
+    for (Route::PathList::iterator it = rt->GetPathList().begin();
+         it != rt->GetPathList().end(); it++) {
+        BgpPath *path = static_cast<BgpPath *>(it.operator->());
+        if (path->IsReplicated())
+            rt2_replicated_path_time_stamp_after_change
+                = path->time_stamp_usecs();
+    }
+    ASSERT_TRUE(rt2_replicated_path_time_stamp_after_change
+                == rt2_replicated_path_time_stamp);
+
+    DeleteRoute<Inet6Definition>(peers_[0], "test_0.inet6.0",
+                             "2001:db8:85a3::8a2e:370:7335/128");
+    DeleteRoute<Inet6Definition>(peers_[1], "test_1.inet6.0",
+                             "2001:db8:85a3::8a2e:370:7335/128");
+    task_util::WaitForIdle();
+    ifmap_test_util::IFMapMsgUnlink(&config_db_, "routing-instance", "test_0",
+                                  "routing-instance", "test_1", "connection");
+}
+
 static void ValidateShowRoutingPolicyResponse(
     Sandesh *sandesh, bool *done,
     const vector<ShowRoutingPolicyInfo> &policy_list) {

diff --git a/src/bgp/testdata/routing_policy_7i.xml b/src/bgp/testdata/routing_policy_7i.xml
@@ -0,0 +1,41 @@
+<?xml version="1.0" encoding="utf-8"?>
+<config>
+    <routing-policy name='basic_1'>
+        <term>
+            <term-match-condition>
+                <community>23:13</community>
+            </term-match-condition>
+            <term-action-list>
+                <update>
+                    <local-pref>200</local-pref>
+                </update>
+                <action>accept</action>
+            </term-action-list>
+        </term>
+    </routing-policy>
+    <routing-policy name='basic_2'>
+        <term>
+            <term-match-condition>
+                <community>23:13</community>
+            </term-match-condition>
+            <term-action-list>
+                <update>
+                    <local-pref>50</local-pref>
+                </update>
+                <action>accept</action>
+            </term-action-list>
+        </term>
+    </routing-policy>
+    <routing-instance name="test_0">
+        <routing-policy to="basic_1">
+            <sequence>1.0</sequence>
+        </routing-policy>
+        <vrf-target>target:1:101</vrf-target>
+    </routing-instance>
+    <routing-instance name="test_1">
+        <routing-policy to="basic_2">
+            <sequence>1.0</sequence>
+        </routing-policy>
+        <vrf-target>target:1:102</vrf-target>
+    </routing-instance>
+</config>