From 04424af92ff86beff005a079cf1f46c64ec8918e Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Mon, 18 May 2015 11:07:04 -0700 Subject: [PATCH] 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 --- src/bgp/routing-instance/static_route.cc | 35 ++--- src/bgp/test/static_route_test.cc | 168 ++++++++++++++++++++++- src/bgp/testdata/static_route_10a.xml | 24 ++++ src/bgp/testdata/static_route_10b.xml | 24 ++++ src/bgp/testdata/static_route_10c.xml | 24 ++++ src/bgp/testdata/static_route_11a.xml | 10 ++ src/bgp/testdata/static_route_11b.xml | 10 ++ src/bgp/testdata/static_route_11c.xml | 10 ++ src/bgp/testdata/static_route_9a.xml | 24 ++++ src/bgp/testdata/static_route_9b.xml | 24 ++++ src/bgp/testdata/static_route_9c.xml | 24 ++++ 11 files changed, 356 insertions(+), 21 deletions(-) create mode 100644 src/bgp/testdata/static_route_10a.xml create mode 100644 src/bgp/testdata/static_route_10b.xml create mode 100644 src/bgp/testdata/static_route_10c.xml create mode 100644 src/bgp/testdata/static_route_11a.xml create mode 100644 src/bgp/testdata/static_route_11b.xml create mode 100644 src/bgp/testdata/static_route_11c.xml create mode 100644 src/bgp/testdata/static_route_9a.xml create mode 100644 src/bgp/testdata/static_route_9b.xml create mode 100644 src/bgp/testdata/static_route_9c.xml diff --git a/src/bgp/routing-instance/static_route.cc b/src/bgp/routing-instance/static_route.cc index 2961c99d78d..468259dcbc4 100644 --- a/src/bgp/routing-instance/static_route.cc +++ b/src/bgp/routing-instance/static_route.cc @@ -120,11 +120,12 @@ class StaticRoute : public ConditionMatch { void UpdateRtargetList(const std::vector &rtargets) { rtarget_list_.clear(); - for(std::vector::const_iterator it = rtargets.begin(); + for (std::vector::const_iterator it = rtargets.begin(); it != rtargets.end(); it++) { error_code ec; RouteTarget rtarget = RouteTarget::FromString(*it, &ec); - assert(ec == 0); + if (ec != 0) + continue; rtarget_list_.insert(rtarget); } // update static route if added already @@ -195,11 +196,12 @@ StaticRoute::StaticRoute(RoutingInstance *rtinst, IpAddress nexthop) : routing_instance_(rtinst), static_route_prefix_(static_route), nexthop_(nexthop), nexthop_route_(NULL), unregistered_(false) { - for(std::vector::const_iterator it = rtargets.begin(); - it != rtargets.end(); it++) { + for (std::vector::const_iterator it = rtargets.begin(); + it != rtargets.end(); it++) { error_code ec; RouteTarget rtarget = RouteTarget::FromString(*it, &ec); - assert(ec == 0); + if (ec != 0) + continue; rtarget_list_.insert(rtarget); } } @@ -556,11 +558,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(); @@ -600,6 +600,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); @@ -608,8 +612,6 @@ StaticRouteMgr::LocateStaticRoutePrefix(const autogen::StaticRouteType &cfg) { listener->AddMatchCondition(match->bgp_table(), static_route_match.get(), BgpConditionListener::RequestDoneCb()); - - return; } void StaticRouteMgr::StopStaticRouteDone(BgpTable *table, @@ -664,14 +666,8 @@ 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); - + 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); } @@ -693,7 +689,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 index 85e4402416c..684654709a8 100644 --- a/src/bgp/test/static_route_test.cc +++ b/src/bgp/test/static_route_test.cc @@ -95,7 +95,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_); @@ -350,6 +350,172 @@ class StaticRouteTest : public ::testing::Test { }; int StaticRouteTest::validate_done_; + +TEST_F(StaticRouteTest, InvalidNextHop) { + vector instance_names = list_of("nat"); + multimap 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 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 instance_names = list_of("nat"); + multimap 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 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 instance_names = list_of("nat"); + multimap 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 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 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 new file mode 100644 index 00000000000..73405c42272 --- /dev/null +++ b/src/bgp/testdata/static_route_10a.xml @@ -0,0 +1,24 @@ + + + + 192.168.1.0/24 + 192.168.1.254 + target:64496:1 + target:64496:2 + target:64496:3 + + + + 192.168.1.254 + target:64496:1 + target:64496:2 + target:64496:3 + + + 192.168.3.0/24 + 192.168.1.254 + target:64496:1 + target:64496:2 + target:64496:3 + + diff --git a/src/bgp/testdata/static_route_10b.xml b/src/bgp/testdata/static_route_10b.xml new file mode 100644 index 00000000000..367697b4f82 --- /dev/null +++ b/src/bgp/testdata/static_route_10b.xml @@ -0,0 +1,24 @@ + + + + 192.168.1.0/33 + 192.168.1.254 + target:64496:1 + target:64496:2 + target:64496:3 + + + 192.168.2.0/24 + 192.168.1.254 + target:64496:1 + target:64496:2 + target:64496:3 + + + 192.168.3.0/24 + 192.168.1.254 + target:64496:1 + target:64496:2 + target:64496:3 + + diff --git a/src/bgp/testdata/static_route_10c.xml b/src/bgp/testdata/static_route_10c.xml new file mode 100644 index 00000000000..4f08f099d3d --- /dev/null +++ b/src/bgp/testdata/static_route_10c.xml @@ -0,0 +1,24 @@ + + + + 192.168.1.0/24 + 192.168.1.254 + target:64496:1 + target:64496:2 + target:64496:3 + + + 192.168.2.0/24 + 192.168.1.254 + target:64496:1 + target:64496:2 + target:64496:3 + + + 2001:abcd:abcd:abcd::/64 + 192.168.1.254 + target:64496:1 + target:64496:2 + target:64496:3 + + diff --git a/src/bgp/testdata/static_route_11a.xml b/src/bgp/testdata/static_route_11a.xml new file mode 100644 index 00000000000..b7400115f32 --- /dev/null +++ b/src/bgp/testdata/static_route_11a.xml @@ -0,0 +1,10 @@ + + + + 192.168.1.0/24 + 192.168.1.254 + target:64496:1 + xxxxxx:64496:2 + target:64496:3 + + diff --git a/src/bgp/testdata/static_route_11b.xml b/src/bgp/testdata/static_route_11b.xml new file mode 100644 index 00000000000..1ed27ebf288 --- /dev/null +++ b/src/bgp/testdata/static_route_11b.xml @@ -0,0 +1,10 @@ + + + + 192.168.1.0/24 + 192.168.1.254 + target:64496:5000000000 + target:64496:2 + target:64496:3 + + diff --git a/src/bgp/testdata/static_route_11c.xml b/src/bgp/testdata/static_route_11c.xml new file mode 100644 index 00000000000..07a5cf572ec --- /dev/null +++ b/src/bgp/testdata/static_route_11c.xml @@ -0,0 +1,10 @@ + + + + 192.168.1.0/24 + 192.168.1.254 + target:64496:1 + target:64496:2 + target:64496 + + diff --git a/src/bgp/testdata/static_route_9a.xml b/src/bgp/testdata/static_route_9a.xml new file mode 100644 index 00000000000..794e50debc2 --- /dev/null +++ b/src/bgp/testdata/static_route_9a.xml @@ -0,0 +1,24 @@ + + + + 192.168.1.0/24 + 192.168.1.254 + target:64496:1 + target:64496:2 + target:64496:3 + + + 192.168.2.0/24 + + target:64496:1 + target:64496:2 + target:64496:3 + + + 192.168.3.0/24 + 192.168.1.254 + target:64496:1 + target:64496:2 + target:64496:3 + + diff --git a/src/bgp/testdata/static_route_9b.xml b/src/bgp/testdata/static_route_9b.xml new file mode 100644 index 00000000000..2c0056d39da --- /dev/null +++ b/src/bgp/testdata/static_route_9b.xml @@ -0,0 +1,24 @@ + + + + 192.168.1.0/24 + 192.168.1.256 + target:64496:1 + target:64496:2 + target:64496:3 + + + 192.168.2.0/24 + 192.168.1.254 + target:64496:1 + target:64496:2 + target:64496:3 + + + 192.168.3.0/24 + 192.168.1.254 + target:64496:1 + target:64496:2 + target:64496:3 + + diff --git a/src/bgp/testdata/static_route_9c.xml b/src/bgp/testdata/static_route_9c.xml new file mode 100644 index 00000000000..68d8a4fa4d8 --- /dev/null +++ b/src/bgp/testdata/static_route_9c.xml @@ -0,0 +1,24 @@ + + + + 192.168.1.0/24 + 192.168.1.254 + target:64496:1 + target:64496:2 + target:64496:3 + + + 192.168.2.0/24 + 192.168.1.254 + target:64496:1 + target:64496:2 + target:64496:3 + + + 192.168.3.0/24 + 2001::2001 + target:64496:1 + target:64496:2 + target:64496:3 + +