Skip to content

Commit

Permalink
Prevent replication loop for re-originated route
Browse files Browse the repository at this point in the history 
Problem Scenario:
The problem happens due following sequence of events:
1. Default route is pushed by SDN gateway to bgp.l3vpn.0 table of control-node
   with RD of service chain internal routing instance.
2. This route gets copied to public VN(due to configured route target).
3. The route from “public” is re-originated to service chain internal
   routing instance of left VN.
4. This re-originated route is replicated back to bgp.l3vpn.0 table.
   Since the RDs are same, it gets replicated to same bgp.l3vpn.0 route (step 1)
   with one Path from SDN Gateway and One path from re-orignated route.
5. Since the re-originated path has better local-pref than SDN Gateway path,
   previously replicated route(in step 2) is flushed.
   This causes the deletion of re-orinated route.
6. The path that was inserted in step 4 is deleted as re-originated route got
   deleted in previous step. This results in repeating from step-1.

Solution:
Prevent the re-origination of the route from public network if the source RD of
the connected route/path is same as original route.
1. Copy the source RD when route is replicated from vpn table to inet/inet6 table
2. Prevent adding service chain route when source RD of the original route matches
   the  connected route's source RD.
3. Add UTs to verify the scenario

Change-Id: I0485e9b192c9e3f76bb1905ea68834a13f6385bc
Closes-Bug: #1599588
(cherry picked from commit 5ee836d)
  • Loading branch information
@bailkeri
bailkeri committed Aug 12, 2016
1 parent c06d6b6 commit 011fe56
Show file tree
Hide file tree
Showing 5 changed files with 140 additions and 31 deletions.
13 changes: 11 additions & 2 deletions src/bgp/inet/inet_table.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,16 @@ BgpRoute *InetTable::RouteReplicate(BgpServer *server,
InetRoute *inet= dynamic_cast<InetRoute *> (src_rt);

boost::scoped_ptr<Ip4Prefix> inet_prefix;
RouteDistinguisher rd;

if (inet) {
inet_prefix.reset(new Ip4Prefix(inet->GetPrefix().ip4_addr(),
inet->GetPrefix().prefixlen()));
rd = RouteDistinguisher::kZeroRd;
} else {
InetVpnRoute *inetvpn = dynamic_cast<InetVpnRoute *> (src_rt);
assert(inetvpn);
rd = inetvpn->GetPrefix().route_distinguisher();
inet_prefix.reset(new Ip4Prefix(inetvpn->GetPrefix().addr(),
inetvpn->GetPrefix().prefixlen()));
}
Expand All @@ -88,8 +91,14 @@ BgpRoute *InetTable::RouteReplicate(BgpServer *server,
}

// Replace the extended community with the one provided.
BgpAttrPtr new_attr = server->attr_db()->ReplaceExtCommunityAndLocate(
path->GetAttr(), community);
BgpAttrDB *attr_db = server->attr_db();
BgpAttrPtr new_attr = attr_db->ReplaceExtCommunityAndLocate(path->GetAttr(),
community);

// Set the RD attr if route is replicated from vpn table
if (!inet) {
new_attr = attr_db->ReplaceSourceRdAndLocate(new_attr.get(), rd);
}

// Check whether there's already a path with the given peer and path id.
BgpPath *dest_path = dest_route->FindSecondaryPath(src_rt,
Expand Down
14 changes: 11 additions & 3 deletions src/bgp/inet6/inet6_table.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,17 @@ BgpRoute *Inet6Table::RouteReplicate(BgpServer *server, BgpTable *src_table,

Inet6Route *source = dynamic_cast<Inet6Route *>(src_rt);

RouteDistinguisher rd;

boost::scoped_ptr<Inet6Prefix> prefix;
if (source) {
prefix.reset(new Inet6Prefix(source->GetPrefix().ip6_addr(),
source->GetPrefix().prefixlen()));
rd = RouteDistinguisher::kZeroRd;
} else {
Inet6VpnRoute *vpn_route = dynamic_cast<Inet6VpnRoute *> (src_rt);
assert(vpn_route);
rd = vpn_route->GetPrefix().route_distinguisher();
prefix.reset(new Inet6Prefix(vpn_route->GetPrefix().addr(),
vpn_route->GetPrefix().prefixlen()));
}
Expand All @@ -84,9 +88,13 @@ BgpRoute *Inet6Table::RouteReplicate(BgpServer *server, BgpTable *src_table,
}

// Replace the extended community with the one provided.
BgpAttrPtr new_attr =
server->attr_db()->ReplaceExtCommunityAndLocate(path->GetAttr(),
community);
BgpAttrDB *attr_db = server->attr_db();
BgpAttrPtr new_attr = attr_db->ReplaceExtCommunityAndLocate(path->GetAttr(),
community);

if (!source) {
new_attr = attr_db->ReplaceSourceRdAndLocate(new_attr.get(), rd);
}

// Check whether there's already a path with the given peer and path id.
BgpPath *dest_path =
Expand Down
8 changes: 8 additions & 0 deletions src/bgp/routing-instance/service_chaining.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,7 @@ void ServiceChain::AddServiceChainRoute(Ip4Prefix prefix,
ExtCommunity::ExtCommunityList sgid_list;
const Community *orig_community = NULL;
const OriginVnPath *orig_ovnpath = NULL;
RouteDistinguisher orig_rd;
if (orig_route) {
const BgpPath *orig_path = orig_route->BestPath();
const BgpAttr *orig_attr = NULL;
Expand All @@ -337,6 +338,7 @@ void ServiceChain::AddServiceChainRoute(Ip4Prefix prefix,
orig_community = orig_attr->community();
ext_community = orig_attr->ext_community();
orig_ovnpath = orig_attr->origin_vn_path();
orig_rd = orig_attr->source_rd();
}
if (ext_community) {
BOOST_FOREACH(const ExtCommunity::ExtCommunityValue &comm,
Expand Down Expand Up @@ -381,6 +383,7 @@ void ServiceChain::AddServiceChainRoute(Ip4Prefix prefix,
continue;

const BgpAttr *attr = connected_path->GetAttr();

ExtCommunityPtr new_ext_community;

// Strip any RouteTargets from the connected attributes.
Expand Down Expand Up @@ -427,6 +430,7 @@ void ServiceChain::AddServiceChainRoute(Ip4Prefix prefix,
RouteDistinguisher connected_rd = attr->source_rd();
if (connected_rd.Type() != RouteDistinguisher::TypeIpAddressBased)
continue;

RouteDistinguisher rd(connected_rd.GetAddress(), instance_id);
new_attr = attr_db->ReplaceSourceRdAndLocate(new_attr.get(), rd);
}
Expand All @@ -445,6 +449,10 @@ void ServiceChain::AddServiceChainRoute(Ip4Prefix prefix,
}
}

// Skip paths with Source RD same as source RD of the connected path
if (!orig_rd.IsZero() && new_attr->source_rd() == orig_rd)
continue;

// Check whether we already have a path with the associated path id.
uint32_t path_id =
connected_path->GetAttr()->nexthop().to_v4().to_ulong();
Expand Down
29 changes: 29 additions & 0 deletions src/bgp/test/routepath_replicator_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -635,6 +635,35 @@ TEST_F(ReplicationTest, NoExtCommunities) {
VERIFY_EQ(0, RouteCount("red"));
}

//
// Verify that when route is replicated form vpn table, source RD attribute
// is added to replicated route with RouteDistinguisher of the vpn prefix
//
TEST_F(ReplicationTest, SourceRD) {
vector<string> instance_names = list_of("blue")("red")("green");
multimap<string, string> connections = map_list_of("blue", "red");
NetworkConfig(instance_names, connections);
task_util::WaitForIdle();

boost::system::error_code ec;
peers_.push_back(
new BgpPeerMock(Ip4Address::from_string("192.168.0.1", ec)));

// VPN route with target "blue".
AddVPNRoute(peers_[0], "192.168.0.1:1:10.0.1.1/32", 100, list_of("blue"));
task_util::WaitForIdle();
VERIFY_EQ(1, RouteCount("blue"));

BgpRoute *rt = InetRouteLookup("blue", "10.0.1.1/32");
TASK_UTIL_EXPECT_TRUE(RouteDistinguisher::FromString("192.168.0.1:1") ==
rt->BestPath()->GetAttr()->source_rd());

DeleteVPNRoute(peers_[0], "192.168.0.1:1:10.0.1.1/32");
task_util::WaitForIdle();
VERIFY_EQ(0, RouteCount("blue"));
VERIFY_EQ(0, RouteCount("red"));
}

TEST_F(ReplicationTest, Delete) {
vector<string> instance_names = list_of("blue")("red")("green");
multimap<string, string> connections = map_list_of("blue", "red");
Expand Down
107 changes: 81 additions & 26 deletions src/bgp/test/service_chain_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,8 @@ class ServiceChainTest : public ::testing::Test {
set<string> encap = set<string>(),
const SiteOfOrigin &soo = SiteOfOrigin(),
string nexthop = "7.8.9.1",
uint32_t flags = 0, int label = 0) {
uint32_t flags = 0, int label = 0,
const RouteDistinguisher &rd = RouteDistinguisher()) {
boost::system::error_code error;
Ip4Prefix nlri = Ip4Prefix::FromString(prefix, &error);
EXPECT_FALSE(error);
Expand All @@ -229,6 +230,11 @@ class ServiceChainTest : public ::testing::Test {
new BgpAttrNextHop(chain_addr.to_v4().to_ulong()));
attr_spec.push_back(nexthop_attr.get());

BgpAttrSourceRd source_rd(rd);
if (!rd.IsZero()) {
attr_spec.push_back(&source_rd);
}

CommunitySpec comm;
if (!commlist.empty()) {
comm.communities = commlist;
Expand Down Expand Up @@ -288,7 +294,8 @@ class ServiceChainTest : public ::testing::Test {
vector<uint32_t> sglist = vector<uint32_t>(),
set<string> encap = set<string>(),
string nexthop = "7.8.9.1",
uint32_t flags = 0, int label = 0) {
uint32_t flags = 0, int label = 0,
const RouteDistinguisher &rd = RouteDistinguisher()) {
BgpTable *table = static_cast<BgpTable *>(
bgp_server_->database()->FindTable(instance_name + ".inet.0"));
ASSERT_TRUE(table != NULL);
Expand All @@ -297,11 +304,11 @@ class ServiceChainTest : public ::testing::Test {
int rti_index = rtinstance->index();

string vpn_prefix;
if (peer) {
vpn_prefix = peer->ToString() + ":" + integerToString(rti_index) +
":" + prefix;
if (!rd.IsZero()) {
vpn_prefix = rd.ToString() + ":" + prefix;
} else {
vpn_prefix = "7.7.7.7:" + integerToString(rti_index) + ":" + prefix;
string peer_str = peer ? peer->ToString() : "7.7.7.7";
vpn_prefix = peer_str + ":" + integerToString(rti_index) + ":" + prefix;
}

boost::system::error_code error;
Expand Down Expand Up @@ -354,18 +361,19 @@ class ServiceChainTest : public ::testing::Test {
void AddInetVpnRoute(IPeer *peer, const vector<string> &instance_names,
const string &prefix, int localpref,
string nexthop = "7.8.9.1",
uint32_t flags = 0, int label = 0) {
uint32_t flags = 0, int label = 0,
const RouteDistinguisher &rd = RouteDistinguisher()) {
RoutingInstance *rtinstance =
ri_mgr_->GetRoutingInstance(instance_names[0]);
ASSERT_TRUE(rtinstance != NULL);
int rti_index = rtinstance->index();

string vpn_prefix;
if (peer) {
vpn_prefix = peer->ToString() + ":" + integerToString(rti_index) +
":" + prefix;
if (!rd.IsZero()) {
vpn_prefix = rd.ToString() + ":" + prefix;
} else {
vpn_prefix = "7.7.7.7:" + integerToString(rti_index) + ":" + prefix;
string peer_str = peer ? peer->ToString() : "7.7.7.7";
vpn_prefix = peer_str + ":" + integerToString(rti_index) + ":" + prefix;
}

boost::system::error_code error;
Expand Down Expand Up @@ -405,7 +413,8 @@ class ServiceChainTest : public ::testing::Test {
}

void DeleteInetVpnRoute(IPeer *peer, const string &instance_name,
const string &prefix) {
const string &prefix,
const RouteDistinguisher &rd = RouteDistinguisher()) {
BgpTable *table = static_cast<BgpTable *>(
bgp_server_->database()->FindTable(instance_name + ".inet.0"));
ASSERT_TRUE(table != NULL);
Expand All @@ -414,11 +423,11 @@ class ServiceChainTest : public ::testing::Test {
int rti_index = rtinstance->index();

string vpn_prefix;
if (peer) {
vpn_prefix = peer->ToString() + ":" + integerToString(rti_index) +
":" + prefix;
if (!rd.IsZero()) {
vpn_prefix = rd.ToString() + ":" + prefix;
} else {
vpn_prefix = "7.7.7.7:" + integerToString(rti_index) + ":" + prefix;
string peer_str = peer ? peer->ToString() : "7.7.7.7";
vpn_prefix = peer_str + ":" + integerToString(rti_index) + ":" + prefix;
}

boost::system::error_code error;
Expand All @@ -439,15 +448,16 @@ class ServiceChainTest : public ::testing::Test {
int localpref, string nexthop = "7.8.9.1",
uint32_t flags = 0, int label = 0,
vector<uint32_t> sglist = vector<uint32_t>(),
set<string> encap = set<string>()) {
set<string> encap = set<string>(),
const RouteDistinguisher &rd = RouteDistinguisher()) {
string connected_table = service_is_transparent_ ? "blue-i1" : "blue";
if (connected_rt_is_inetvpn_) {
AddInetVpnRoute(peer, connected_table, prefix, localpref,
sglist, encap, nexthop, flags, label);
sglist, encap, nexthop, flags, label, rd);
} else {
AddInetRoute(peer, connected_table, prefix, localpref,
vector<uint32_t>(), sglist, encap, SiteOfOrigin(), nexthop,
flags, label);
flags, label, rd);
}
task_util::WaitForIdle();
}
Expand All @@ -456,7 +466,8 @@ class ServiceChainTest : public ::testing::Test {
int localpref, string nexthop = "7.8.9.1",
uint32_t flags = 0, int label = 0,
vector<uint32_t> sglist = vector<uint32_t>(),
set<string> encap = set<string>()) {
set<string> encap = set<string>(),
const RouteDistinguisher &rd = RouteDistinguisher()) {
assert(1 <= chain_idx && chain_idx <= 3);
string connected_table;
if (chain_idx == 1) {
Expand All @@ -468,26 +479,28 @@ class ServiceChainTest : public ::testing::Test {
}
if (connected_rt_is_inetvpn_) {
AddInetVpnRoute(peer, connected_table, prefix,
localpref, sglist, encap, nexthop, flags, label);
localpref, sglist, encap, nexthop, flags, label, rd);
} else {
AddInetRoute(peer, connected_table, prefix, localpref,
vector<uint32_t>(), sglist, encap, SiteOfOrigin(), nexthop,
flags, label);
flags, label, rd);
}
task_util::WaitForIdle();
}

void DeleteConnectedRoute(IPeer *peer, const string &prefix) {
void DeleteConnectedRoute(IPeer *peer, const string &prefix,
const RouteDistinguisher &rd = RouteDistinguisher()) {
string connected_table = service_is_transparent_ ? "blue-i1" : "blue";
if (connected_rt_is_inetvpn_) {
DeleteInetVpnRoute(peer, connected_table, prefix);
DeleteInetVpnRoute(peer, connected_table, prefix, rd);
} else {
DeleteInetRoute(peer, connected_table, prefix);
}
task_util::WaitForIdle();
}

void DeleteConnectedRoute(int chain_idx, IPeer *peer, const string &prefix) {
void DeleteConnectedRoute(int chain_idx, IPeer *peer, const string &prefix,
const RouteDistinguisher &rd = RouteDistinguisher()) {
assert(1 <= chain_idx && chain_idx <= 3);
string connected_table;
if (chain_idx == 1) {
Expand All @@ -498,7 +511,7 @@ class ServiceChainTest : public ::testing::Test {
connected_table = service_is_transparent_ ? "core-i5" : "core";
}
if (connected_rt_is_inetvpn_) {
DeleteInetVpnRoute(peer, connected_table, prefix);
DeleteInetVpnRoute(peer, connected_table, prefix, rd);
} else {
DeleteInetRoute(peer, connected_table, prefix);
}
Expand Down Expand Up @@ -3446,6 +3459,48 @@ TEST_P(ServiceChainParamTest, TransitNetworkOriginVnLoop) {
DeleteConnectedRoute(2, NULL, "192.168.2.253/32");
}

//
// Verify that routes are not re-originated when source RD of the route matches
// connected path's source RD
//
TEST_P(ServiceChainParamTest, ExtConnectRouteSourceRDSame) {
vector<string> instance_names =
list_of("blue")("blue-i1")("red-i2")("red");
multimap<string, string> connections =
map_list_of("blue", "blue-i1") ("red-i2", "red");
NetworkConfig(instance_names, connections);
VerifyNetworkConfig(instance_names);

SetServiceChainInformation("blue-i1",
"controller/src/bgp/testdata/service_chain_1.xml");

// Add Connected
AddConnectedRoute(NULL, "1.1.2.3/32", 100,
"3.4.5.6",
0, 0, vector<uint32_t>(), set<string>(),
RouteDistinguisher::FromString("192.168.1.1:2"));

// Add Ext connect route with targets of red
vector<string> instances = list_of("red");
AddInetVpnRoute(NULL, instances, "10.1.1.0/24", 100,
"1.2.3.4",
0, 0, RouteDistinguisher::FromString("192.168.1.1:2"));

// Verify that MX leaked route is present in red
VerifyInetRouteExists("red", "10.1.1.0/24");

// Verify that ExtConnect route is NOT present in blue
// Verify that re-origination skipped as original route has same source RD
// as connected route source RD
VerifyInetRouteNoExists("blue", "10.1.1.0/24");

// Delete ExtRoute and connected route
DeleteInetVpnRoute(NULL, "red", "10.1.1.0/24",
RouteDistinguisher::FromString("192.168.1.1:2"));
DeleteConnectedRoute(NULL, "1.1.2.3/32",
RouteDistinguisher::FromString("192.168.1.1:2"));
}

INSTANTIATE_TEST_CASE_P(Instance, ServiceChainParamTest,
::testing::Combine(::testing::Bool(), ::testing::Bool()));

Expand Down

0 comments on commit 011fe56

Please sign in to comment.