From ee68869d4bd0694085cd92478bdd9db88655123b Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Mon, 18 May 2015 17:41:27 -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: Ibb7780bb367486a31eb4a57b1a61942c404e2342 Partial-Bug: 1456193 --- src/bgp/routing-instance/static_route.cc | 8 +- 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, 345 insertions(+), 5 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 20aa0d628cc..e99a8070672 100644 --- 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 index b16ae2d1942..d709ec290bb 100644 --- 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 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 + +