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: Ia0fb3404dafe734ef4ad6e13885d53766aa6d95b Partial-Bug: 1456193
Juniper · May 19, 2015 · 10e2976 · 10e2976
1 parent b4e01e8
commit 10e2976
Show file tree

Hide file tree

Showing 11 changed files with 354 additions and 19 deletions.
diff --git a/src/bgp/routing-instance/static_route.cc b/src/bgp/routing-instance/static_route.cc
@@ -132,7 +132,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);
         }
 
@@ -208,7 +209,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);
     }
 }
@@ -565,11 +567,9 @@ void
 StaticRouteMgr::LocateStaticRoutePrefix(const autogen::StaticRouteType &cfg) {
     CHECK_CONCURRENCY("bgp::Config");
     error_code ec;
-    IpAddress nexthop = Ip4Address::from_string(cfg.next_hop, ec);
-    assert(ec == 0);
-
     Ip4Prefix prefix = Ip4Prefix::FromString(cfg.prefix, &ec);
-    assert(ec == 0);
+    if (ec != 0)
+        return;
 
     BgpConditionListener *listener =
         routing_instance()->server()->condition_listener();
@@ -609,6 +609,10 @@ StaticRouteMgr::LocateStaticRoutePrefix(const autogen::StaticRouteType &cfg) {
         return;
     }
 
+    IpAddress nexthop = Ip4Address::from_string(cfg.next_hop, ec);
+    if (ec != 0)
+        return;
+
     StaticRoute *match =
         new StaticRoute(routing_instance(), prefix, cfg.route_target, nexthop);
     StaticRoutePtr static_route_match = StaticRoutePtr(match);
@@ -617,8 +621,6 @@ StaticRouteMgr::LocateStaticRoutePrefix(const autogen::StaticRouteType &cfg) {
 
     listener->AddMatchCondition(match->bgp_table(), static_route_match.get(),
                                 BgpConditionListener::RequestDoneCb());
-
-    return;
 }
 
 void StaticRouteMgr::StopStaticRouteDone(BgpTable *table,
@@ -673,15 +675,9 @@ StaticRouteMgr::~StaticRouteMgr() {
 bool CompareStaticRouteConfig(autogen::StaticRouteType lhs,
                               autogen::StaticRouteType rhs) {
     error_code ec;
-    Ip4Prefix lhs_static_route_prefix =
-        Ip4Prefix::FromString(lhs.prefix, &ec);
-    assert(*ec == 0);
-
-    Ip4Prefix rhs_static_route_prefix =
-        Ip4Prefix::FromString(rhs.prefix, &ec);
-    assert(*ec == 0);
-
-    return (lhs_static_route_prefix < rhs_static_route_prefix);
+    Ip4Prefix lhs_static_route_prefix = Ip4Prefix::FromString(lhs.prefix, &ec);
+    Ip4Prefix rhs_static_route_prefix = Ip4Prefix::FromString(rhs.prefix, &ec);
+    return (lhs_static_route_prefix < rhs_static_route_prefix); 
 }
 
 
@@ -702,7 +698,6 @@ void StaticRouteMgr::UpdateStaticRouteConfig() {
         error_code ec;
         Ip4Prefix static_route_prefix =
             Ip4Prefix::FromString(static_route_cfg_it->prefix, &ec);
-        assert(*ec == 0);
         if (static_route_prefix < oper_it->first) {
             LocateStaticRoutePrefix(*static_route_cfg_it);
             static_route_cfg_it++;

diff --git a/src/bgp/test/static_route_test.cc b/src/bgp/test/static_route_test.cc
@@ -98,7 +98,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_);
@@ -353,6 +353,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>