Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Nischal Sheth committed May 19, 2015
1 parent dad6d8d commit 04424af
Show file tree
Hide file tree
Showing 11 changed files with 356 additions and 21 deletions.
35 changes: 15 additions & 20 deletions src/bgp/routing-instance/static_route.cc
Expand Up @@ -120,11 +120,12 @@ class StaticRoute : public ConditionMatch {

void UpdateRtargetList(const std::vector<std::string> &rtargets) {
rtarget_list_.clear();
for(std::vector<std::string>::const_iterator it = rtargets.begin();
for (std::vector<std::string>::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
Expand Down Expand Up @@ -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<std::string>::const_iterator it = rtargets.begin();
it != rtargets.end(); it++) {
for (std::vector<std::string>::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);
}
}
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand All @@ -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,
Expand Down Expand Up @@ -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);
}

Expand All @@ -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++;
Expand Down
168 changes: 167 additions & 1 deletion src/bgp/test/static_route_test.cc
Expand Up @@ -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_);
Expand Down Expand Up @@ -350,6 +350,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
Expand Down
24 changes: 24 additions & 0 deletions 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>
24 changes: 24 additions & 0 deletions 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>
24 changes: 24 additions & 0 deletions 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>
10 changes: 10 additions & 0 deletions 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>
10 changes: 10 additions & 0 deletions 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>
10 changes: 10 additions & 0 deletions 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>

0 comments on commit 04424af

Please sign in to comment.