Improve error handling for static route config parsing

Do not assert when we encounter parse errors in prefixes, nexthops and route targets. Add tests to cover the above scenarios. Change-Id: Ibb7780bb367486a31eb4a57b1a61942c404e2342 Partial-Bug: 1456193
Juniper · May 19, 2015 · ee68869 · ee68869
1 parent d09ef26
commit ee68869
Show file tree

Hide file tree

Showing 11 changed files with 345 additions and 5 deletions.
diff --git a/src/bgp/routing-instance/static_route.cc b/src/bgp/routing-instance/static_route.cc
@@ -131,7 +131,8 @@ class StaticRoute : public ConditionMatch {
              it != rtargets.end(); ++it) {
             error_code ec;
             RouteTarget rtarget = RouteTarget::FromString(*it, &ec);
-            assert(ec == 0);
+            if (ec != 0)
+                continue;
             rtarget_list_.insert(rtarget);
         }
 
@@ -211,7 +212,8 @@ StaticRoute::StaticRoute(RoutingInstance *rtinst,
         it != rtargets.end(); it++) {
         error_code ec;
         RouteTarget rtarget = RouteTarget::FromString(*it, &ec);
-        assert(ec == 0);
+        if (ec != 0)
+            continue;
         rtarget_list_.insert(rtarget);
     }
 }
@@ -613,8 +615,6 @@ StaticRouteMgr::LocateStaticRoutePrefix(const StaticRouteConfig &cfg) {
 
     listener_->AddMatchCondition(match->bgp_table(), static_route_match.get(),
                                 BgpConditionListener::RequestDoneCb());
-
-    return;
 }
 
 void StaticRouteMgr::StopStaticRouteDone(BgpTable *table,

diff --git a/src/bgp/test/static_route_test.cc b/src/bgp/test/static_route_test.cc
@@ -100,7 +100,7 @@ class BgpPeerMock : public IPeer {
 class StaticRouteTest : public ::testing::Test {
 protected:
     StaticRouteTest()
-        : bgp_server_(new BgpServer(&evm_)) {
+        : bgp_server_(new BgpServer(&evm_)), ri_mgr_(NULL) {
         IFMapLinkTable_Init(&config_db_, &config_graph_);
         vnc_cfg_Server_ModuleInit(&config_db_, &config_graph_);
         bgp_schema_Server_ModuleInit(&config_db_, &config_graph_);
@@ -357,6 +357,172 @@ class StaticRouteTest : public ::testing::Test {
 };
 
 int StaticRouteTest::validate_done_;
+
+TEST_F(StaticRouteTest, InvalidNextHop) {
+    vector<string> instance_names = list_of("nat");
+    multimap<string, string> connections;
+    NetworkConfig(instance_names, connections);
+    task_util::WaitForIdle();
+
+    // Add Nexthop route.
+    AddInetRoute(NULL, "nat", "192.168.1.254/32", 100, "2.3.4.5");
+    task_util::WaitForIdle();
+
+    std::auto_ptr<autogen::StaticRouteEntriesType> params =
+        GetStaticRouteConfig("controller/src/bgp/testdata/static_route_9a.xml");
+    ifmap_test_util::IFMapMsgPropertyAdd(&config_db_, "routing-instance",
+        "nat", "static-route-entries", params.release(), 0);
+    task_util::WaitForIdle();
+    TASK_UTIL_WAIT_NE_NO_MSG(InetRouteLookup("nat", "192.168.1.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat instance..");
+    TASK_UTIL_WAIT_NE_NO_MSG(InetRouteLookup("nat", "192.168.3.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat instance..");
+    TASK_UTIL_WAIT_EQ_NO_MSG(InetRouteLookup("nat", "192.168.2.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat instance..");
+
+    params =
+        GetStaticRouteConfig("controller/src/bgp/testdata/static_route_9b.xml");
+    ifmap_test_util::IFMapMsgPropertyAdd(&config_db_, "routing-instance",
+        "nat", "static-route-entries", params.release(), 0);
+    task_util::WaitForIdle();
+    TASK_UTIL_WAIT_NE_NO_MSG(InetRouteLookup("nat", "192.168.2.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat instance..");
+    TASK_UTIL_WAIT_NE_NO_MSG(InetRouteLookup("nat", "192.168.3.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat instance..");
+    TASK_UTIL_WAIT_EQ_NO_MSG(InetRouteLookup("nat", "192.168.1.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat instance..");
+
+    params =
+        GetStaticRouteConfig("controller/src/bgp/testdata/static_route_9c.xml");
+    ifmap_test_util::IFMapMsgPropertyAdd(&config_db_, "routing-instance",
+        "nat", "static-route-entries", params.release(), 0);
+    task_util::WaitForIdle();
+    TASK_UTIL_WAIT_NE_NO_MSG(InetRouteLookup("nat", "192.168.1.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat instance..");
+    TASK_UTIL_WAIT_NE_NO_MSG(InetRouteLookup("nat", "192.168.2.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat instance..");
+    TASK_UTIL_WAIT_EQ_NO_MSG(InetRouteLookup("nat", "192.168.3.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat instance..");
+
+    // Delete nexthop route.
+    DeleteInetRoute(NULL, "nat", "192.168.1.254/32");
+    task_util::WaitForIdle();
+}
+
+TEST_F(StaticRouteTest, InvalidPrefix) {
+    vector<string> instance_names = list_of("nat");
+    multimap<string, string> connections;
+    NetworkConfig(instance_names, connections);
+    task_util::WaitForIdle();
+
+    // Add Nexthop route.
+    AddInetRoute(NULL, "nat", "192.168.1.254/32", 100, "2.3.4.5");
+    task_util::WaitForIdle();
+
+    std::auto_ptr<autogen::StaticRouteEntriesType> params =
+        GetStaticRouteConfig("controller/src/bgp/testdata/static_route_10a.xml");
+    ifmap_test_util::IFMapMsgPropertyAdd(&config_db_, "routing-instance",
+        "nat", "static-route-entries", params.release(), 0);
+    task_util::WaitForIdle();
+    TASK_UTIL_WAIT_NE_NO_MSG(InetRouteLookup("nat", "192.168.1.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat instance..");
+    TASK_UTIL_WAIT_NE_NO_MSG(InetRouteLookup("nat", "192.168.3.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat instance..");
+
+    params =
+        GetStaticRouteConfig("controller/src/bgp/testdata/static_route_10b.xml");
+    ifmap_test_util::IFMapMsgPropertyAdd(&config_db_, "routing-instance",
+        "nat", "static-route-entries", params.release(), 0);
+    task_util::WaitForIdle();
+    TASK_UTIL_WAIT_NE_NO_MSG(InetRouteLookup("nat", "192.168.2.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat instance..");
+    TASK_UTIL_WAIT_NE_NO_MSG(InetRouteLookup("nat", "192.168.3.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat instance..");
+
+    params =
+        GetStaticRouteConfig("controller/src/bgp/testdata/static_route_10c.xml");
+    ifmap_test_util::IFMapMsgPropertyAdd(&config_db_, "routing-instance",
+        "nat", "static-route-entries", params.release(), 0);
+    task_util::WaitForIdle();
+    TASK_UTIL_WAIT_NE_NO_MSG(InetRouteLookup("nat", "192.168.1.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat instance..");
+    TASK_UTIL_WAIT_NE_NO_MSG(InetRouteLookup("nat", "192.168.2.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat instance..");
+
+    // Delete nexthop route.
+    DeleteInetRoute(NULL, "nat", "192.168.1.254/32");
+    task_util::WaitForIdle();
+}
+
+TEST_F(StaticRouteTest, InvalidRouteTarget) {
+    vector<string> instance_names = list_of("nat");
+    multimap<string, string> connections;
+    NetworkConfig(instance_names, connections);
+    task_util::WaitForIdle();
+
+    // Add Nexthop route.
+    AddInetRoute(NULL, "nat", "192.168.1.254/32", 100, "2.3.4.5");
+    task_util::WaitForIdle();
+
+    std::auto_ptr<autogen::StaticRouteEntriesType> params =
+        GetStaticRouteConfig("controller/src/bgp/testdata/static_route_11a.xml");
+    ifmap_test_util::IFMapMsgPropertyAdd(&config_db_, "routing-instance",
+        "nat", "static-route-entries", params.release(), 0);
+    task_util::WaitForIdle();
+    TASK_UTIL_WAIT_NE_NO_MSG(InetRouteLookup("nat", "192.168.1.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat instance..");
+    BgpRoute *static_rt = InetRouteLookup("nat", "192.168.1.0/24");
+    const BgpPath *static_path = static_rt->BestPath();
+    set<string> config_list = list_of("target:64496:1")("target:64496:3");
+    TASK_UTIL_EXPECT_TRUE(GetRTargetFromPath(static_path) == config_list);
+
+    params =
+        GetStaticRouteConfig("controller/src/bgp/testdata/static_route_11b.xml");
+    ifmap_test_util::IFMapMsgPropertyAdd(&config_db_, "routing-instance",
+        "nat", "static-route-entries", params.release(), 0);
+    task_util::WaitForIdle();
+    TASK_UTIL_WAIT_NE_NO_MSG(InetRouteLookup("nat", "192.168.1.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat instance..");
+    static_rt = InetRouteLookup("nat", "192.168.1.0/24");
+    static_path = static_rt->BestPath();
+    config_list = list_of("target:64496:2")("target:64496:3");
+    TASK_UTIL_EXPECT_TRUE(GetRTargetFromPath(static_path) == config_list);
+
+    params =
+        GetStaticRouteConfig("controller/src/bgp/testdata/static_route_11c.xml");
+    ifmap_test_util::IFMapMsgPropertyAdd(&config_db_, "routing-instance",
+        "nat", "static-route-entries", params.release(), 0);
+    task_util::WaitForIdle();
+    TASK_UTIL_WAIT_NE_NO_MSG(InetRouteLookup("nat", "192.168.1.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat instance..");
+    static_rt = InetRouteLookup("nat", "192.168.1.0/24");
+    static_path = static_rt->BestPath();
+    config_list = list_of("target:64496:1")("target:64496:2");
+    TASK_UTIL_EXPECT_TRUE(GetRTargetFromPath(static_path) == config_list);
+
+    // Delete nexthop route.
+    DeleteInetRoute(NULL, "nat", "192.168.1.254/32");
+    task_util::WaitForIdle();
+}
+
 //
 // Basic Test
 // 1. Configure routing instance with static route property

diff --git a/src/bgp/testdata/static_route_10a.xml b/src/bgp/testdata/static_route_10a.xml
@@ -0,0 +1,24 @@
+<?xml version="1.0" encoding="utf-8"?>
+<static-route-entries>
+    <route>
+        <prefix>192.168.1.0/24</prefix>
+        <next-hop>192.168.1.254</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+    <route>
+        <prefix></prefix>
+        <next-hop>192.168.1.254</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+    <route>
+        <prefix>192.168.3.0/24</prefix>
+        <next-hop>192.168.1.254</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+</static-route-entries>
diff --git a/src/bgp/testdata/static_route_10b.xml b/src/bgp/testdata/static_route_10b.xml
@@ -0,0 +1,24 @@
+<?xml version="1.0" encoding="utf-8"?>
+<static-route-entries>
+    <route>
+        <prefix>192.168.1.0/33</prefix>
+        <next-hop>192.168.1.254</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+    <route>
+        <prefix>192.168.2.0/24</prefix>
+        <next-hop>192.168.1.254</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+    <route>
+        <prefix>192.168.3.0/24</prefix>
+        <next-hop>192.168.1.254</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+</static-route-entries>
diff --git a/src/bgp/testdata/static_route_10c.xml b/src/bgp/testdata/static_route_10c.xml
@@ -0,0 +1,24 @@
+<?xml version="1.0" encoding="utf-8"?>
+<static-route-entries>
+    <route>
+        <prefix>192.168.1.0/24</prefix>
+        <next-hop>192.168.1.254</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+    <route>
+        <prefix>192.168.2.0/24</prefix>
+        <next-hop>192.168.1.254</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+    <route>
+        <prefix>2001:abcd:abcd:abcd::/64</prefix>
+        <next-hop>192.168.1.254</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+</static-route-entries>
diff --git a/src/bgp/testdata/static_route_11a.xml b/src/bgp/testdata/static_route_11a.xml
@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="utf-8"?>
+<static-route-entries>
+    <route>
+        <prefix>192.168.1.0/24</prefix>
+        <next-hop>192.168.1.254</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>xxxxxx:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+</static-route-entries>
diff --git a/src/bgp/testdata/static_route_11b.xml b/src/bgp/testdata/static_route_11b.xml
@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="utf-8"?>
+<static-route-entries>
+    <route>
+        <prefix>192.168.1.0/24</prefix>
+        <next-hop>192.168.1.254</next-hop>
+        <route-target>target:64496:5000000000</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+</static-route-entries>
diff --git a/src/bgp/testdata/static_route_11c.xml b/src/bgp/testdata/static_route_11c.xml
@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="utf-8"?>
+<static-route-entries>
+    <route>
+        <prefix>192.168.1.0/24</prefix>
+        <next-hop>192.168.1.254</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496</route-target>
+    </route>
+</static-route-entries>
diff --git a/src/bgp/testdata/static_route_9a.xml b/src/bgp/testdata/static_route_9a.xml
@@ -0,0 +1,24 @@
+<?xml version="1.0" encoding="utf-8"?>
+<static-route-entries>
+    <route>
+        <prefix>192.168.1.0/24</prefix>
+        <next-hop>192.168.1.254</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+    <route>
+        <prefix>192.168.2.0/24</prefix>
+        <next-hop></next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+    <route>
+        <prefix>192.168.3.0/24</prefix>
+        <next-hop>192.168.1.254</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+</static-route-entries>
diff --git a/src/bgp/testdata/static_route_9b.xml b/src/bgp/testdata/static_route_9b.xml
@@ -0,0 +1,24 @@
+<?xml version="1.0" encoding="utf-8"?>
+<static-route-entries>
+    <route>
+        <prefix>192.168.1.0/24</prefix>
+        <next-hop>192.168.1.256</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+    <route>
+        <prefix>192.168.2.0/24</prefix>
+        <next-hop>192.168.1.254</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+    <route>
+        <prefix>192.168.3.0/24</prefix>
+        <next-hop>192.168.1.254</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+</static-route-entries>
diff --git a/src/bgp/testdata/static_route_9c.xml b/src/bgp/testdata/static_route_9c.xml
@@ -0,0 +1,24 @@
+<?xml version="1.0" encoding="utf-8"?>
+<static-route-entries>
+    <route>
+        <prefix>192.168.1.0/24</prefix>
+        <next-hop>192.168.1.254</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+    <route>
+        <prefix>192.168.2.0/24</prefix>
+        <next-hop>192.168.1.254</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+    <route>
+        <prefix>192.168.3.0/24</prefix>
+        <next-hop>2001::2001</next-hop>
+        <route-target>target:64496:1</route-target>
+        <route-target>target:64496:2</route-target>
+        <route-target>target:64496:3</route-target>
+    </route>
+</static-route-entries>