Skip to content

Commit

Permalink
Merge "Prevent replication loop for re-originated route" into R3.1
Browse files Browse the repository at this point in the history
  • Loading branch information
@opencontrail-ci-admin
Zuul authored and opencontrail-ci-admin committed Aug 2, 2016
2 parents 9b215e0 + dcb897a commit 645e754
Show file tree
Hide file tree
Showing 5 changed files with 146 additions and 29 deletions.
12 changes: 10 additions & 2 deletions src/bgp/inet/inet_table.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,15 @@ 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()));
} 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 @@ -85,8 +87,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
13 changes: 10 additions & 3 deletions src/bgp/inet6/inet6_table.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,16 @@ 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()));
} 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 @@ -85,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 @@ -395,6 +395,7 @@ void ServiceChain<T>::AddServiceChainRoute(PrefixT prefix,
bool load_balance_present = false;
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 @@ -405,6 +406,7 @@ void ServiceChain<T>::AddServiceChainRoute(PrefixT 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 @@ -453,6 +455,7 @@ void ServiceChain<T>::AddServiceChainRoute(PrefixT prefix,
continue;

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

ExtCommunityPtr new_ext_community;

// Strip any RouteTargets from the connected attributes.
Expand Down Expand Up @@ -510,6 +513,7 @@ void ServiceChain<T>::AddServiceChainRoute(PrefixT 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 @@ -528,6 +532,10 @@ void ServiceChain<T>::AddServiceChainRoute(PrefixT 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 @@ -628,6 +628,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
113 changes: 89 additions & 24 deletions src/bgp/test/service_chain_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,8 @@ class ServiceChainTest : public ::testing::Test {
set<string> encap = set<string>(),
const SiteOfOrigin &soo = SiteOfOrigin(),
string nexthop_str = "", uint32_t flags = 0,
int label = 0, const LoadBalance &lb = LoadBalance()) {
int label = 0, const LoadBalance &lb = LoadBalance(),
const RouteDistinguisher &rd = RouteDistinguisher()) {
boost::system::error_code error;
PrefixT nlri = PrefixT::FromString(prefix, &error);
EXPECT_FALSE(error);
Expand All @@ -298,6 +299,11 @@ class ServiceChainTest : public ::testing::Test {
new BgpAttrLocalPref(localpref));
attr_spec.push_back(local_pref.get());

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

if (nexthop_str.empty())
nexthop_str = this->BuildNextHopAddress("7.8.9.1");
IpAddress chain_addr = Ip4Address::from_string(nexthop_str, error);
Expand Down Expand Up @@ -367,17 +373,22 @@ class ServiceChainTest : public ::testing::Test {
set<string> encap = set<string>(),
string nexthop_str = "",
uint32_t flags = 0, int label = 0,
const LoadBalance &lb = LoadBalance()) {
const LoadBalance &lb = LoadBalance(),
const RouteDistinguisher &rd = RouteDistinguisher()) {
BgpTable *table = GetTable(instance_name);
ASSERT_TRUE(table != NULL);
const RoutingInstance *rtinstance = table->routing_instance();
ASSERT_TRUE(rtinstance != NULL);
int rti_index = rtinstance->index();

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

boost::system::error_code error;
VpnPrefixT nlri = VpnPrefixT::FromString(vpn_prefix, &error);
Expand Down Expand Up @@ -445,16 +456,21 @@ class ServiceChainTest : public ::testing::Test {
const string &prefix, int localpref,
string nexthop_str = "",
uint32_t flags = 0, int label = 0,
const LoadBalance &lb = LoadBalance()) {
const LoadBalance &lb = LoadBalance(),
const RouteDistinguisher &rd = RouteDistinguisher()) {
RoutingInstance *rtinstance =
ri_mgr_->GetRoutingInstance(instance_names[0]);
ASSERT_TRUE(rtinstance != NULL);
int rti_index = rtinstance->index();

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

boost::system::error_code error;
VpnPrefixT nlri = VpnPrefixT::FromString(vpn_prefix, &error);
Expand Down Expand Up @@ -498,19 +514,21 @@ class ServiceChainTest : public ::testing::Test {
}

void DeleteVpnRoute(IPeer *peer, const string &instance_name,
const string &prefix) {
const string &prefix,
const RouteDistinguisher &rd = RouteDistinguisher()) {
BgpTable *table = GetTable(instance_name);
ASSERT_TRUE(table != NULL);
const RoutingInstance *rtinstance = table->routing_instance();
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() :
BuildNextHopAddress("7.7.7.7");
vpn_prefix = peer_str + ":" + integerToString(rti_index) + ":" + prefix;
}

boost::system::error_code error;
Expand All @@ -531,17 +549,19 @@ class ServiceChainTest : public ::testing::Test {
uint32_t flags = 0, int label = 0,
vector<uint32_t> sglist = vector<uint32_t>(),
set<string> encap = set<string>(),
const LoadBalance &lb = LoadBalance()) {
const LoadBalance &lb = LoadBalance(),
const RouteDistinguisher &rd = RouteDistinguisher()) {
AddConnectedRoute(1, peer, prefix, localpref, nexthop, flags, label,
sglist, encap, lb);
sglist, encap, lb, rd);
}

void AddConnectedRoute(int chain_idx, IPeer *peer, const string &prefix,
int localpref, string nexthop = "",
uint32_t flags = 0, int label = 0,
vector<uint32_t> sglist = vector<uint32_t>(),
set<string> encap = set<string>(),
const LoadBalance &lb = LoadBalance()) {
const LoadBalance &lb = LoadBalance(),
const RouteDistinguisher &rd = RouteDistinguisher()) {
assert(1 <= chain_idx && chain_idx <= 3);
string connected_table;
if (chain_idx == 1) {
Expand All @@ -555,27 +575,28 @@ class ServiceChainTest : public ::testing::Test {
nexthop = BuildNextHopAddress("7.8.9.1");
if (connected_rt_is_vpn_) {
AddVpnRoute(peer, connected_table, prefix,
localpref, sglist, encap, nexthop, flags, label, lb);
localpref, sglist, encap, nexthop, flags, label, lb, rd);
} else {
AddRoute(peer, connected_table, prefix, localpref,
vector<uint32_t>(), sglist, encap, SiteOfOrigin(), nexthop,
flags, label, lb);
flags, label, lb, 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_vpn_) {
DeleteVpnRoute(peer, connected_table, prefix);
DeleteVpnRoute(peer, connected_table, prefix, rd);
} else {
DeleteRoute(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 @@ -586,7 +607,7 @@ class ServiceChainTest : public ::testing::Test {
connected_table = service_is_transparent_ ? "core-i5" : "core";
}
if (connected_rt_is_vpn_) {
DeleteVpnRoute(peer, connected_table, prefix);
DeleteVpnRoute(peer, connected_table, prefix, rd);
} else {
DeleteRoute(peer, connected_table, prefix);
}
Expand Down Expand Up @@ -4228,6 +4249,50 @@ TYPED_TEST(ServiceChainTest, TransitNetworkOriginVnLoop) {
this->DeleteConnectedRoute(2, NULL, this->BuildPrefix("192.168.2.253", 32));
}

//
// Verify that routes are not re-originated when source RD of the route matches
// connected path's source RD
//
TYPED_TEST(ServiceChainTest, 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");
this->NetworkConfig(instance_names, connections);
this->VerifyNetworkConfig(instance_names);

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

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

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

// Verify that MX leaked route is present in red
this->VerifyRouteExists("red", this->BuildPrefix("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
this->VerifyRouteNoExists("blue", this->BuildPrefix("10.1.1.0", 24));

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

class TestEnvironment : public ::testing::Environment {
virtual ~TestEnvironment() { }
};
Expand Down

0 comments on commit 645e754

Please sign in to comment.