Merge "Prevent route replication loop"

Juniper · Jul 1, 2016 · 9d41493 · 9d41493
2 parents 624e453 + 933d30c
commit 9d41493
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
@@ -2498,6 +2498,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>