From 7d96ea8b660bca6f9f2b7a7096b5d1d995b670d1 Mon Sep 17 00:00:00 2001 From: Prakash Bailkeri Date: Sun, 7 Feb 2016 23:49:19 +0530 Subject: [PATCH] OriginVn for aggregate route Currently origin vn is not set for aggregate route. Consider the following example: 1. left-vn -- Service Chain -- Right-vn 2. Internal routing instance created for service chain has route aggregate config 3. Aggregate route for right-vn subnet is added to left-vn Since the aggregate route doesn't have origin vn, it gets leaked backed to right network as service chain route. This can cause black-holing of traffic in right due to wrong next hop for right-vn subnet route Fix: Add origin VN for aggregate route. a. If the routing instance has service chain config, origin vn is the dest-routing-instance b. Else origin-vn is routing instance's virtual network index Re-order origin-vn calculation in service chain code to consider origin-vn present in the BgpAttr(extCommunity) Added test in route aggregate to validate service chain scenario and origin vn Change-Id: I9f1ffb6fedb816eac7b5f070a965cf202b96cb49 Related-bug: #1500698 --- src/bgp/routing-instance/route_aggregator.cc | 11 +++ .../routing-instance/routepath_replicator.cc | 2 +- src/bgp/routing-instance/routing_instance.cc | 11 +++ src/bgp/routing-instance/routing_instance.h | 2 + src/bgp/routing-instance/service_chaining.cc | 30 ++++---- src/bgp/test/route_aggregator_test.cc | 72 +++++++++++++++++++ src/bgp/testdata/route_aggregate_4.xml | 43 +++++++++++ 7 files changed, 156 insertions(+), 15 deletions(-) create mode 100644 src/bgp/testdata/route_aggregate_4.xml diff --git a/src/bgp/routing-instance/route_aggregator.cc b/src/bgp/routing-instance/route_aggregator.cc index c27ae7939db..da4c352b596 100644 --- a/src/bgp/routing-instance/route_aggregator.cc +++ b/src/bgp/routing-instance/route_aggregator.cc @@ -13,6 +13,7 @@ #include "base/lifetime.h" #include "base/map_util.h" #include "base/task_annotations.h" +#include "bgp/origin-vn/origin_vn.h" #include "bgp/routing-instance/path_resolver.h" #include "bgp/routing-instance/routing_instance.h" #include "bgp/routing-instance/route_aggregate_types.h" @@ -380,6 +381,11 @@ void AggregateRoute::AddAggregateRoute() { BgpAttrSpec attrs; BgpAttrNextHop attr_nexthop(this->GetAddress(nexthop())); attrs.push_back(&attr_nexthop); + ExtCommunitySpec extcomm_spec; + OriginVn origin_vn(routing_instance()->server()->autonomous_system(), + routing_instance()->GetOriginVnForAggregateRoute(GetFamily())); + extcomm_spec.communities.push_back(origin_vn.GetExtCommunityValue()); + attrs.push_back(&extcomm_spec); BgpAttrPtr attr = routing_instance()->server()->attr_db()->Locate(attrs); BgpPath *new_path = new BgpPath(BgpPath::Aggregate, attr.get(), BgpPath::ResolveNexthop, 0); @@ -411,6 +417,11 @@ void AggregateRoute::UpdateAggregateRoute() { BgpAttrSpec attrs; BgpAttrNextHop attr_nexthop(this->GetAddress(nexthop())); attrs.push_back(&attr_nexthop); + ExtCommunitySpec extcomm_spec; + OriginVn origin_vn(routing_instance()->server()->autonomous_system(), + routing_instance()->GetOriginVnForAggregateRoute(GetFamily())); + extcomm_spec.communities.push_back(origin_vn.GetExtCommunityValue()); + attrs.push_back(&extcomm_spec); BgpAttrPtr attr = routing_instance()->server()->attr_db()->Locate(attrs); BgpPath *new_path = new BgpPath(BgpPath::Aggregate, attr.get(), BgpPath::ResolveNexthop, 0); diff --git a/src/bgp/routing-instance/routepath_replicator.cc b/src/bgp/routing-instance/routepath_replicator.cc index e07a0d4a3d4..3a153e76275 100644 --- a/src/bgp/routing-instance/routepath_replicator.cc +++ b/src/bgp/routing-instance/routepath_replicator.cc @@ -582,7 +582,7 @@ bool RoutePathReplicator::RouteListener(TableState *ts, continue; // Add OriginVn when replicating self-originated routes from a VRF. - if (!rtinstance->IsDefaultRoutingInstance() && + if (!vn_index && !rtinstance->IsDefaultRoutingInstance() && path->IsVrfOriginated() && rtinstance->virtual_network_index()) { vn_index = rtinstance->virtual_network_index(); OriginVn origin_vn(server_->autonomous_system(), vn_index); diff --git a/src/bgp/routing-instance/routing_instance.cc b/src/bgp/routing-instance/routing_instance.cc index a8b3c4a8d66..d6a841adc5f 100644 --- a/src/bgp/routing-instance/routing_instance.cc +++ b/src/bgp/routing-instance/routing_instance.cc @@ -1234,3 +1234,14 @@ bool RoutingInstance::IsContributingRoute(const BgpTable *table, if (!route_aggregator(table->family())) return false; return route_aggregator(table->family())->IsContributingRoute(route); } + +int RoutingInstance::GetOriginVnForAggregateRoute(Address::Family fmly) const { + const ServiceChainConfig *sc_config = + config_ ? config_->service_chain_info(fmly) : NULL; + if (sc_config && !sc_config->routing_instance.empty()) { + RoutingInstance *dest = + mgr_->GetRoutingInstance(sc_config->routing_instance); + if (dest) return dest->virtual_network_index(); + } + return virtual_network_index_; +} diff --git a/src/bgp/routing-instance/routing_instance.h b/src/bgp/routing-instance/routing_instance.h index 9f851ed6138..7a29f01d555 100644 --- a/src/bgp/routing-instance/routing_instance.h +++ b/src/bgp/routing-instance/routing_instance.h @@ -168,6 +168,8 @@ class RoutingInstance { bool IsContributingRoute(const BgpTable *table, const BgpRoute *route) const; + int GetOriginVnForAggregateRoute(Address::Family family) const; + private: friend class RoutingInstanceMgr; class DeleteActor; diff --git a/src/bgp/routing-instance/service_chaining.cc b/src/bgp/routing-instance/service_chaining.cc index 2643fea1f1d..0d525b6def7 100644 --- a/src/bgp/routing-instance/service_chaining.cc +++ b/src/bgp/routing-instance/service_chaining.cc @@ -37,22 +37,19 @@ static int GetOriginVnIndex(const BgpTable *table, const BgpRoute *route) { if (!path) return 0; - if (path->IsVrfOriginated()) - return table->routing_instance()->virtual_network_index(); - const BgpAttr *attr = path->GetAttr(); const ExtCommunity *ext_community = attr->ext_community(); - if (!ext_community) - return 0; - - BOOST_FOREACH(const ExtCommunity::ExtCommunityValue &comm, - ext_community->communities()) { - if (!ExtCommunity::is_origin_vn(comm)) - continue; - OriginVn origin_vn(comm); - return origin_vn.vn_index(); + if (ext_community) { + BOOST_FOREACH(const ExtCommunity::ExtCommunityValue &comm, + ext_community->communities()) { + if (!ExtCommunity::is_origin_vn(comm)) + continue; + OriginVn origin_vn(comm); + return origin_vn.vn_index(); + } } - + if (path->IsVrfOriginated()) + return table->routing_instance()->virtual_network_index(); return 0; } @@ -988,8 +985,13 @@ bool ServiceChainMgr::LocateServiceChain(RoutingInstance *rtinstance, RoutingInstanceMgr *mgr = server_->routing_instance_mgr(); RoutingInstance *dest = mgr->GetRoutingInstance(config.routing_instance); + // // Destination routing instance is not yet created. - if (dest == NULL || dest->deleted()) { + // Or Destination routing instance is deleted Or + // virtual network index is not yet calculated (due missing virtual network + // link) + // + if (dest == NULL || dest->deleted() || !dest->virtual_network_index()) { // Wait for the creation of RoutingInstance AddPendingServiceChain(rtinstance); return false; diff --git a/src/bgp/test/route_aggregator_test.cc b/src/bgp/test/route_aggregator_test.cc index 6a692aff5dd..fe6393f3f59 100644 --- a/src/bgp/test/route_aggregator_test.cc +++ b/src/bgp/test/route_aggregator_test.cc @@ -314,6 +314,20 @@ class RouteAggregatorTest : public ::testing::Test { return list; } + string GetOriginVnFromRoute(const BgpPath *path) { + const ExtCommunity *ext_comm = path->GetAttr()->ext_community(); + assert(ext_comm); + BOOST_FOREACH(const ExtCommunity::ExtCommunityValue &comm, + ext_comm->communities()) { + if (!ExtCommunity::is_origin_vn(comm)) + continue; + OriginVn origin_vn(comm); + RoutingInstanceMgr *ri_mgr = bgp_server_->routing_instance_mgr(); + return ri_mgr->GetVirtualNetworkByVnIndex(origin_vn.vn_index()); + } + return "unresolved"; + } + void DisableUnregResolveTask(const string &instance, Address::Family fmly) { RoutingInstance *rti = bgp_server_->routing_instance_mgr()->GetRoutingInstance(instance); @@ -888,6 +902,64 @@ TEST_F(RouteAggregatorTest, Basic_LastMoreSpecificDelete) { task_util::WaitForIdle(); } +// +// Add route aggregate config to routing instance with service chain config +// Ensure that origin vn is set correctly on the aggregate route +// +TEST_F(RouteAggregatorTest, ServiceChain) { + string content = + FileRead("controller/src/bgp/testdata/route_aggregate_4.xml"); + EXPECT_TRUE(parser_.Parse(content)); + task_util::WaitForIdle(); + + boost::system::error_code ec; + peers_.push_back( + new BgpPeerMock(Ip4Address::from_string("192.168.0.1", ec))); + + AddRoute(peers_[0], "red.inet.0", "1.1.1.3/32", 100); + AddRoute(peers_[0], "red.inet.0", "2.2.2.1/32", 100); + AddRoute(peers_[0], "blue.inet.0", "1.1.1.3/32", 100); + AddRoute(peers_[0], "blue.inet.0", "1.1.1.1/32", 100); + task_util::WaitForIdle(); + VERIFY_EQ(4, RouteCount("red.inet.0")); + VERIFY_EQ(4, RouteCount("blue.inet.0")); + BgpRoute *rt = RouteLookup("blue.inet.0", "2.2.2.0/24"); + ASSERT_TRUE(rt != NULL); + TASK_UTIL_EXPECT_EQ(rt->count(), 2); + TASK_UTIL_EXPECT_TRUE(rt->BestPath() != NULL); + TASK_UTIL_EXPECT_TRUE(rt->BestPath()->IsFeasible()); + TASK_UTIL_EXPECT_TRUE(GetOriginVnFromRoute(rt->BestPath()) == "red-vn"); + rt = RouteLookup("blue.inet.0", "2.2.2.1/32"); + ASSERT_TRUE(rt != NULL); + TASK_UTIL_EXPECT_EQ(rt->count(), 1); + TASK_UTIL_EXPECT_TRUE(rt->BestPath() != NULL); + TASK_UTIL_EXPECT_TRUE(rt->BestPath()->IsFeasible()); + TASK_UTIL_EXPECT_TRUE(rt->BestPath()->GetSource() == BgpPath::ServiceChain); + TASK_UTIL_EXPECT_TRUE(IsContributingRoute("blue", + "blue.inet.0", "2.2.2.1/32")); + + rt = RouteLookup("red.inet.0", "1.1.1.0/24"); + ASSERT_TRUE(rt != NULL); + TASK_UTIL_EXPECT_EQ(rt->count(), 2); + TASK_UTIL_EXPECT_TRUE(rt->BestPath() != NULL); + TASK_UTIL_EXPECT_TRUE(rt->BestPath()->IsFeasible()); + TASK_UTIL_EXPECT_TRUE(GetOriginVnFromRoute(rt->BestPath()) == "blue-vn"); + rt = RouteLookup("red.inet.0", "1.1.1.1/32"); + ASSERT_TRUE(rt != NULL); + TASK_UTIL_EXPECT_EQ(rt->count(), 1); + TASK_UTIL_EXPECT_TRUE(rt->BestPath() != NULL); + TASK_UTIL_EXPECT_TRUE(rt->BestPath()->IsFeasible()); + TASK_UTIL_EXPECT_TRUE(rt->BestPath()->GetSource() == BgpPath::ServiceChain); + TASK_UTIL_EXPECT_TRUE(IsContributingRoute("red", + "red.inet.0", "1.1.1.1/32")); + + DeleteRoute(peers_[0], "red.inet.0", "1.1.1.3/32"); + DeleteRoute(peers_[0], "red.inet.0", "2.2.2.1/32"); + DeleteRoute(peers_[0], "blue.inet.0", "1.1.1.1/32"); + DeleteRoute(peers_[0], "blue.inet.0", "1.1.1.3/32"); + task_util::WaitForIdle(); +} + // // Remove the route-aggregate object from routing instance and ensure that // aggregated route is deleted diff --git a/src/bgp/testdata/route_aggregate_4.xml b/src/bgp/testdata/route_aggregate_4.xml new file mode 100644 index 00000000000..77181024099 --- /dev/null +++ b/src/bgp/testdata/route_aggregate_4.xml @@ -0,0 +1,43 @@ + + + + 2000 + + + 1000 + + + + 2.2.2.0/24 + + 1.1.1.3 + + + + 1.1.1.0/24 + + 1.1.1.3 + + + red-vn + + blue + red + 1.1.1.0/24 + 1.1.1.3 + + target:1:103 + + + + blue-vn + + red + blue + 2.2.2.0/24 + 1.1.1.3 + + target:1:104 + + +