From abfda24015ecb8e5148913c3d5516612885d990a Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Mon, 14 Mar 2016 09:56:32 -0700 Subject: [PATCH] BGPaaSv2: Further progress on AS override implementation Highlights: - Schema and config parsing changes to enable AS override - Implement split horizon check within an eBGP RibOut - Strip non-transitive attributes when sending routes to eBGP peers - Tweak resolver to retain IPeer from original BgpPath so that split horizon check can work for resolved paths - Make sure that resolved paths don't get deleted prematurely - Add logic to prevent BgpLifetimeManager from deleting objects when any resolver tasks are on the wait queue Change-Id: I2872faf1d382192ea401f1948d842a59bfa0a4a8 Partial-Bug: 1552952 --- src/base/task.cc | 14 ++ src/base/task.h | 4 +- src/base/task_trigger.cc | 5 + src/base/task_trigger.h | 14 ++ src/bgp/bgp_config_ifmap.cc | 1 + src/bgp/bgp_path.h | 4 + src/bgp/bgp_peer_close.cc | 15 +- src/bgp/bgp_ribout.cc | 14 ++ src/bgp/bgp_ribout.h | 1 + src/bgp/bgp_server.cc | 14 ++ src/bgp/bgp_table.cc | 40 ++-- src/bgp/routing-instance/path_resolver.cc | 50 ++++- src/bgp/routing-instance/path_resolver.h | 9 +- src/bgp/test/bgp_config_test.cc | 45 +++++ src/bgp/test/bgp_server_test_util.cc | 5 +- src/bgp/test/bgp_server_test_util.h | 2 +- src/bgp/test/bgp_table_export_test.cc | 217 +++++++++++++++++----- src/bgp/test/path_resolver_test.cc | 54 ++++++ src/bgp/testdata/config_test_41a.xml | 76 ++++++++ src/bgp/testdata/config_test_41b.xml | 77 ++++++++ src/schema/bgp_schema.xsd | 3 + 21 files changed, 591 insertions(+), 73 deletions(-) create mode 100644 src/bgp/testdata/config_test_41a.xml create mode 100644 src/bgp/testdata/config_test_41b.xml diff --git a/src/base/task.cc b/src/base/task.cc index 61863c6e520..3acc394e78e 100644 --- a/src/base/task.cc +++ b/src/base/task.cc @@ -12,6 +12,7 @@ #include "tbb/enumerable_thread_specific.h" #include "base/logging.h" #include "base/task.h" +#include "base/task_annotations.h" #include #include @@ -404,6 +405,19 @@ TaskGroup *TaskScheduler::QueryTaskGroup(int task_id) { return task_group_db_[task_id]; } +// +// Check if there are any Tasks in the given TaskGroup. +// Assumes that all task ids are mutually exclusive with bgp::Config. +// +bool TaskScheduler::IsTaskGroupEmpty(int task_id) const { + CHECK_CONCURRENCY("bgp::Config"); + tbb::mutex::scoped_lock lock(mutex_); + TaskGroup *group = task_group_db_[task_id]; + assert(group); + assert(group->TaskRunCount() == 0); + return group->IsWaitQEmpty(); +} + // Get TaskGroup for a task_id. Grows task_entry_db_ if necessary TaskEntry *TaskScheduler::GetTaskEntry(int task_id, int task_instance) { TaskGroup *group = GetTaskGroup(task_id); diff --git a/src/base/task.h b/src/base/task.h index bdb7dd7af3b..5af1ea5763f 100644 --- a/src/base/task.h +++ b/src/base/task.h @@ -175,6 +175,8 @@ class TaskScheduler { TaskGroup *GetTaskGroup(int task_id); TaskGroup *QueryTaskGroup(int task_id); + bool IsTaskGroupEmpty(int task_id) const; + TaskEntry *GetTaskEntry(int task_id, int instance_id); TaskEntry *QueryTaskEntry(int task_id, int instance_id); void OnTaskExit(Task *task); @@ -233,7 +235,7 @@ class TaskScheduler { TaskEntry *stop_entry_; tbb::task_scheduler_init task_scheduler_; - tbb::mutex mutex_; + mutable tbb::mutex mutex_; bool running_; uint64_t seqno_; TaskGroupDb task_group_db_; diff --git a/src/base/task_trigger.cc b/src/base/task_trigger.cc index 6213407f54e..a4d9b9a50fb 100644 --- a/src/base/task_trigger.cc +++ b/src/base/task_trigger.cc @@ -16,6 +16,9 @@ class TaskTrigger::WorkerTask : public Task { parent_->Reset(); return true; } + if (parent_->deferred()) { + return false; + } bool done = (parent_->func_)(); if (done) { parent_->Reset(); @@ -23,6 +26,7 @@ class TaskTrigger::WorkerTask : public Task { return done; } std::string Description() const { return "TaskTrigger::WorkerTask"; } + private: TaskTrigger *parent_; }; @@ -31,6 +35,7 @@ TaskTrigger::TaskTrigger(FunctionPtr func, int task_id, int task_instance) : func_(func), task_id_(task_id), task_instance_(task_instance) { trigger_ = false; disabled_ = false; + deferred_ = false; } TaskTrigger::~TaskTrigger() { diff --git a/src/base/task_trigger.h b/src/base/task_trigger.h index 0df070efb7a..0bb6c9afd48 100644 --- a/src/base/task_trigger.h +++ b/src/base/task_trigger.h @@ -29,6 +29,19 @@ class TaskTrigger { return disabled_; } + // For Test only + void set_deferred() { + bool current = deferred_.fetch_and_store(true); + assert(!current); + } + void clear_deferred() { + bool current = deferred_.fetch_and_store(false); + assert(current); + } + bool deferred() { + return deferred_; + } + private: class WorkerTask; @@ -37,6 +50,7 @@ class TaskTrigger { int task_instance_; tbb::atomic trigger_; tbb::atomic disabled_; + tbb::atomic deferred_; }; #endif /* defined(__ctrlplane__task_trigger__) */ diff --git a/src/bgp/bgp_config_ifmap.cc b/src/bgp/bgp_config_ifmap.cc index 090e6b70996..2ca98dedf18 100644 --- a/src/bgp/bgp_config_ifmap.cc +++ b/src/bgp/bgp_config_ifmap.cc @@ -243,6 +243,7 @@ static void NeighborSetSessionAttributes( } if (attributes != NULL) { neighbor->set_passive(attributes->passive); + neighbor->set_as_override(attributes->as_override); neighbor->set_loop_count(attributes->loop_count); if (attributes->admin_down) { neighbor->set_admin_down(true); diff --git a/src/bgp/bgp_path.h b/src/bgp/bgp_path.h index ccc30af4b42..1a5507d07b1 100644 --- a/src/bgp/bgp_path.h +++ b/src/bgp/bgp_path.h @@ -105,6 +105,10 @@ class BgpPath : public Path { return ((flags_ & (INFEASIBLE_MASK & ~ResolveNexthop)) == 0); } + bool IsResolved() const { + return ((flags_ & ResolvedPath) != 0); + } + uint32_t GetFlags() const { return flags_; } diff --git a/src/bgp/bgp_peer_close.cc b/src/bgp/bgp_peer_close.cc index e5ae94d4fe0..857aa509553 100644 --- a/src/bgp/bgp_peer_close.cc +++ b/src/bgp/bgp_peer_close.cc @@ -306,14 +306,21 @@ void PeerCloseManager::ProcessRibIn(DBTablePartBase *root, BgpRoute *rt, it != rt->GetPathList().end(); it = next) { next++; - // Skip secondary paths. - if (dynamic_cast(it.operator->())) - continue; BgpPath *path = static_cast(it.operator->()); + + // Skip paths from other peers. if (path->GetPeer() != peer_) continue; - uint32_t stale = 0; + // Skip resolved paths - PathResolver is responsible for them. + if (path->IsResolved()) + continue; + + // Skip secondary paths. + if (dynamic_cast(path)) + continue; + + uint32_t stale = 0; switch (action) { case MembershipRequest::RIBIN_SWEEP: diff --git a/src/bgp/bgp_ribout.cc b/src/bgp/bgp_ribout.cc index f0bd9de0d4e..669434557dc 100644 --- a/src/bgp/bgp_ribout.cc +++ b/src/bgp/bgp_ribout.cc @@ -445,6 +445,20 @@ const RibPeerSet &RibOut::PeerSet() const { return active_peerset_; } +// +// Clear the bit index corresponding to the specified peer. +// Used to implement split horizon within an EBGP Ribout. +// +void RibOut::GetSubsetPeerSet(RibPeerSet *peerset, + const IPeerUpdate *cpeer) const { + assert(policy_.type == BgpProto::EBGP); + IPeerUpdate *peer = const_cast(cpeer); + int index = GetPeerIndex(peer); + if (index < 0) + return; + peerset->reset(index); +} + // // Return the peer corresponding to the specified bit index. // diff --git a/src/bgp/bgp_ribout.h b/src/bgp/bgp_ribout.h index 8e1cdecead4..60ce3f8da0a 100644 --- a/src/bgp/bgp_ribout.h +++ b/src/bgp/bgp_ribout.h @@ -259,6 +259,7 @@ class RibOut { // Returns a bitmask with all the peers that are advertising this RibOut. const RibPeerSet &PeerSet() const; + void GetSubsetPeerSet(RibPeerSet *peerset, const IPeerUpdate *cpeer) const; BgpTable* table() const { return table_; } diff --git a/src/bgp/bgp_server.cc b/src/bgp/bgp_server.cc index 7109f7a36bf..2a5cb154361 100644 --- a/src/bgp/bgp_server.cc +++ b/src/bgp/bgp_server.cc @@ -258,6 +258,20 @@ void BgpServer::RetryDelete() { bool BgpServer::IsReadyForDeletion() { CHECK_CONCURRENCY("bgp::Config"); + static TaskScheduler *scheduler = TaskScheduler::GetInstance(); + static int resolver_path_task_id = + scheduler->GetTaskId("bgp::ResolverPath"); + static int resolver_nexthop_task_id = + scheduler->GetTaskId("bgp::ResolverNexthop"); + + // Check if any PathResolver is active. + // Need to ensure that there's no pending deletes of BgpPaths added by + // PathResolver since they hold pointers to IPeer. + if (!scheduler->IsTaskGroupEmpty(resolver_path_task_id) || + !scheduler->IsTaskGroupEmpty(resolver_nexthop_task_id)) { + return false; + } + // Check if the IPeer membership manager queue is empty. if (!membership_mgr_->IsQueueEmpty()) { return false; diff --git a/src/bgp/bgp_table.cc b/src/bgp/bgp_table.cc index 064015dc523..69ef30aa5ab 100644 --- a/src/bgp/bgp_table.cc +++ b/src/bgp/bgp_table.cc @@ -205,22 +205,35 @@ UpdateInfo *BgpTable::GetUpdateInfo(RibOut *ribout, BgpRoute *route, return NULL; } - // Sender side AS path loop check. - if (!ribout->as_override() && attr->as_path() && - attr->as_path()->path().AsPathLoop(ribout->peer_as())) { - return NULL; - } - // Handle route-target filtering. - if (attr->ext_community() != NULL) { + if (IsVpnTable() && attr->ext_community() != NULL) { server()->rtarget_group_mgr()->GetRibOutInterestedPeers( ribout, attr->ext_community(), peerset, &new_peerset); if (new_peerset.empty()) return NULL; } + // Sender side AS path loop check and split horizon within RibOut. + if (!ribout->as_override()) { + if (attr->as_path() && + attr->as_path()->path().AsPathLoop(ribout->peer_as())) { + return NULL; + } + } else { + if (peer && peer->PeerType() == BgpProto::EBGP) { + ribout->GetSubsetPeerSet(&new_peerset, peer); + if (new_peerset.empty()) + return NULL; + } + } + BgpAttr *clone = new BgpAttr(*attr); + // Remove non-transitive attributes. + // Note that med is handled further down. + clone->set_originator_id(Ip4Address()); + clone->set_cluster_list(NULL); + // Update nexthop. if (!ribout->nexthop().is_unspecified()) clone->set_nexthop(ribout->nexthop()); @@ -390,12 +403,17 @@ void BgpTable::Input(DBTablePartition *root, DBClient *client, // Mark this peer's all paths as deleted. for (Route::PathList::iterator it = rt->GetPathList().begin(); it != rt->GetPathList().end(); ++it) { + BgpPath *path = static_cast(it.operator->()); + + // Skip resolved paths. + if (path->IsResolved()) + continue; + // Skip secondary paths. - if (dynamic_cast(it.operator->())) continue; + if (dynamic_cast(path)) + continue; - BgpPath *path = static_cast(it.operator->()); - if (path->GetPeer() == peer && - path->GetSource() == BgpPath::BGP_XMPP) { + if (path->GetPeer() == peer && path->GetSource() == BgpPath::BGP_XMPP) { deleted_paths.insert(make_pair(path, true)); } } diff --git a/src/bgp/routing-instance/path_resolver.cc b/src/bgp/routing-instance/path_resolver.cc index cbfaab18e89..c12d17d479f 100644 --- a/src/bgp/routing-instance/path_resolver.cc +++ b/src/bgp/routing-instance/path_resolver.cc @@ -476,6 +476,26 @@ void PathResolver::EnableResolverPathUpdateProcessing() { } } +// +// Pause processing of the path update list in all partitions. +// For testing only. +// +void PathResolver::PauseResolverPathUpdateProcessing() { + for (int part_id = 0; part_id < DB::PartitionCount(); ++part_id) { + partitions_[part_id]->PauseResolverPathUpdateProcessing(); + } +} + +// +// Resume processing of the path update list in all partitions. +// For testing only. +// +void PathResolver::ResumeResolverPathUpdateProcessing() { + for (int part_id = 0; part_id < DB::PartitionCount(); ++part_id) { + partitions_[part_id]->ResumeResolverPathUpdateProcessing(); + } +} + // // Get size of the update list. // For testing only. @@ -713,6 +733,22 @@ void PathResolverPartition::EnableResolverPathUpdateProcessing() { rpath_update_trigger_->set_enable(); } +// +// Pause processing of the update list. +// For testing only. +// +void PathResolverPartition::PauseResolverPathUpdateProcessing() { + rpath_update_trigger_->set_deferred(); +} + +// +// Resume processing of the update list. +// For testing only. +// +void PathResolverPartition::ResumeResolverPathUpdateProcessing() { + rpath_update_trigger_->clear_deferred(); +} + // // Get size of the update list. // For testing only. @@ -800,10 +836,12 @@ ResolverPath::~ResolverPath() { // void ResolverPath::AddResolvedPath(ResolvedPathList::const_iterator it) { BgpPath *path = *it; + const IPeer *peer = path->GetPeer(); resolved_path_list_.insert(path); route_->InsertPath(path); BGP_LOG_STR(BgpMessage, SandeshLevel::SYS_DEBUG, BGP_LOG_FLAG_TRACE, "Added resolved path " << route_->ToString() << + " peer " << (peer ? peer->ToString() : "None") << " path_id " << BgpPath::PathIdString(path->GetPathId()) << " nexthop " << path->GetAttr()->nexthop().to_string() << " label " << path->GetLabel() << @@ -816,8 +854,10 @@ void ResolverPath::AddResolvedPath(ResolvedPathList::const_iterator it) { // void ResolverPath::DeleteResolvedPath(ResolvedPathList::const_iterator it) { BgpPath *path = *it; + const IPeer *peer = path->GetPeer(); BGP_LOG_STR(BgpMessage, SandeshLevel::SYS_DEBUG, BGP_LOG_FLAG_TRACE, "Deleted resolved path " << route_->ToString() << + " peer " << (peer ? peer->ToString() : "None") << " path_id " << BgpPath::PathIdString(path->GetPathId()) << " nexthop " << path->GetAttr()->nexthop().to_string() << " label " << path->GetLabel() << @@ -829,12 +869,13 @@ void ResolverPath::DeleteResolvedPath(ResolvedPathList::const_iterator it) { // // Find or create the matching resolved BgpPath. // -BgpPath *ResolverPath::LocateResolvedPath(uint32_t path_id, +BgpPath *ResolverPath::LocateResolvedPath(const IPeer *peer, uint32_t path_id, const BgpAttr *attr, uint32_t label) { for (ResolvedPathList::iterator it = resolved_path_list_.begin(); it != resolved_path_list_.end(); ++it) { BgpPath *path = *it; - if (path->GetPathId() == path_id && + if (path->GetPeer() == peer && + path->GetPathId() == path_id && path->GetAttr() == attr && path->GetLabel() == label) { return path; @@ -844,7 +885,7 @@ BgpPath *ResolverPath::LocateResolvedPath(uint32_t path_id, BgpPath::PathSource src = path_->GetSource(); uint32_t flags = (path_->GetFlags() & ~BgpPath::ResolveNexthop) | BgpPath::ResolvedPath; - return (new BgpPath(path_id, src, attr, flags, label)); + return (new BgpPath(peer, path_id, src, attr, flags, label)); } // @@ -910,6 +951,7 @@ bool ResolverPath::UpdateResolvedPaths() { // resolved paths. ResolvedPathList future_resolved_path_list; const BgpRoute *nh_route = rnexthop_->route(); + const IPeer *peer = path_ ? path_->GetPeer() : NULL; Route::PathList::const_iterator it; if (path_ && nh_route) it = nh_route->GetPathList().begin(); @@ -954,7 +996,7 @@ bool ResolverPath::UpdateResolvedPaths() { // Locate the resolved path. uint32_t path_id = nh_path->GetAttr()->nexthop().to_v4().to_ulong(); BgpPath *resolved_path = - LocateResolvedPath(path_id, attr.get(), nh_path->GetLabel()); + LocateResolvedPath(peer, path_id, attr.get(), nh_path->GetLabel()); future_resolved_path_list.insert(resolved_path); } diff --git a/src/bgp/routing-instance/path_resolver.h b/src/bgp/routing-instance/path_resolver.h index bf22ed54755..6c62b2ed996 100644 --- a/src/bgp/routing-instance/path_resolver.h +++ b/src/bgp/routing-instance/path_resolver.h @@ -29,6 +29,7 @@ class BgpTable; class DBEntryBase; class DBTablePartBase; class DeleteActor; +class IPeer; class PathResolverPartition; class ResolverNexthop; class ResolverPath; @@ -146,6 +147,8 @@ class PathResolver { void DisableResolverPathUpdateProcessing(); void EnableResolverPathUpdateProcessing(); + void PauseResolverPathUpdateProcessing(); + void ResumeResolverPathUpdateProcessing(); size_t GetResolverPathUpdateListSize() const; BgpTable *table_; @@ -216,6 +219,8 @@ class PathResolverPartition { void DisableResolverPathUpdateProcessing(); void EnableResolverPathUpdateProcessing(); + void PauseResolverPathUpdateProcessing(); + void ResumeResolverPathUpdateProcessing(); size_t GetResolverPathUpdateListSize() const; int part_id_; @@ -309,8 +314,8 @@ class ResolverPath { void AddResolvedPath(ResolvedPathList::const_iterator it); void DeleteResolvedPath(ResolvedPathList::const_iterator it); - BgpPath *LocateResolvedPath(uint32_t path_id, const BgpAttr *attr, - uint32_t label); + BgpPath *LocateResolvedPath(const IPeer *peer, uint32_t path_id, + const BgpAttr *attr, uint32_t label); PathResolverPartition *partition_; const BgpPath *path_; diff --git a/src/bgp/test/bgp_config_test.cc b/src/bgp/test/bgp_config_test.cc index 1b6da2832bb..7a11842bd4a 100644 --- a/src/bgp/test/bgp_config_test.cc +++ b/src/bgp/test/bgp_config_test.cc @@ -75,6 +75,7 @@ class BgpConfigTest : public ::testing::Test { } bool GetPeerResolvePaths(BgpPeer *peer) { return peer->resolve_paths_; } + bool GetPeerAsOverride(BgpPeer *peer) { return peer->as_override_; } EventManager evm_; BgpServer server_; @@ -712,6 +713,50 @@ TEST_F(BgpConfigTest, BGPaaSNeighbors7) { TASK_UTIL_EXPECT_EQ(0, rti->peer_manager()->size()); } +TEST_F(BgpConfigTest, BGPaaSNeighbors8) { + string content; + content = FileRead("controller/src/bgp/testdata/config_test_41a.xml"); + EXPECT_TRUE(parser_.Parse(content)); + task_util::WaitForIdle(); + + RoutingInstance *rti = + server_.routing_instance_mgr()->GetRoutingInstance("test"); + TASK_UTIL_ASSERT_TRUE(rti != NULL); + TASK_UTIL_EXPECT_EQ(2, rti->peer_manager()->size()); + + TASK_UTIL_EXPECT_TRUE( + rti->peer_manager()->PeerLookup("test:vm1:0") != NULL); + BgpPeer *peer1 = rti->peer_manager()->PeerLookup("test:vm1:0"); + TASK_UTIL_EXPECT_TRUE(GetPeerResolvePaths(peer1)); + TASK_UTIL_EXPECT_TRUE(GetPeerAsOverride(peer1)); + + TASK_UTIL_EXPECT_TRUE( + rti->peer_manager()->PeerLookup("test:vm2:0") != NULL); + BgpPeer *peer2 = rti->peer_manager()->PeerLookup("test:vm2:0"); + TASK_UTIL_EXPECT_TRUE(GetPeerResolvePaths(peer2)); + TASK_UTIL_EXPECT_FALSE(GetPeerAsOverride(peer2)); + + // Update as-override. + content = FileRead("controller/src/bgp/testdata/config_test_41b.xml"); + EXPECT_TRUE(parser_.Parse(content)); + task_util::WaitForIdle(); + + TASK_UTIL_EXPECT_EQ(peer1, server_.FindPeer(peer1->endpoint())); + TASK_UTIL_EXPECT_TRUE(GetPeerResolvePaths(peer1)); + TASK_UTIL_EXPECT_FALSE(GetPeerAsOverride(peer1)); + + TASK_UTIL_EXPECT_EQ(peer2, server_.FindPeer(peer2->endpoint())); + TASK_UTIL_EXPECT_TRUE(GetPeerResolvePaths(peer2)); + TASK_UTIL_EXPECT_TRUE(GetPeerAsOverride(peer2)); + + // Cleanup. + boost::replace_all(content, "", ""); + boost::replace_all(content, "", ""); + EXPECT_TRUE(parser_.Parse(content)); + task_util::WaitForIdle(); + TASK_UTIL_EXPECT_EQ(0, rti->peer_manager()->size()); +} + TEST_F(BgpConfigTest, MasterNeighborAttributes) { string content_a = FileRead("controller/src/bgp/testdata/config_test_35a.xml"); EXPECT_TRUE(parser_.Parse(content_a)); diff --git a/src/bgp/test/bgp_server_test_util.cc b/src/bgp/test/bgp_server_test_util.cc index 6e4e9211ee4..2dde2b6dda1 100644 --- a/src/bgp/test/bgp_server_test_util.cc +++ b/src/bgp/test/bgp_server_test_util.cc @@ -79,8 +79,9 @@ void BgpServerTest::PostShutdown() { config_db_->Clear(); } -void BgpServerTest::Shutdown(bool verify) { - task_util::WaitForIdle(); +void BgpServerTest::Shutdown(bool verify, bool wait_for_idle) { + if (wait_for_idle) + task_util::WaitForIdle(); BgpServer::Shutdown(); if (verify) VerifyShutdown(); diff --git a/src/bgp/test/bgp_server_test_util.h b/src/bgp/test/bgp_server_test_util.h index a9f956ac2bb..88823812849 100644 --- a/src/bgp/test/bgp_server_test_util.h +++ b/src/bgp/test/bgp_server_test_util.h @@ -198,7 +198,7 @@ class BgpServerTest : public BgpServer { const std::string &name); void DisableAllPeers(); void EnableAllPeers(); - void Shutdown(bool verify = true); + void Shutdown(bool verify = true, bool wait_for_idle = true); void VerifyShutdown() const; DB *config_db() { return config_db_.get(); } diff --git a/src/bgp/test/bgp_table_export_test.cc b/src/bgp/test/bgp_table_export_test.cc index 98722524dd9..15ec493e9ee 100644 --- a/src/bgp/test/bgp_table_export_test.cc +++ b/src/bgp/test/bgp_table_export_test.cc @@ -2,7 +2,9 @@ * Copyright (c) 2013 Juniper Networks, Inc. All rights reserved. */ +#include +#include "base/task_annotations.h" #include "bgp/bgp_factory.h" #include "bgp/bgp_route.h" #include "bgp/bgp_update.h" @@ -13,11 +15,13 @@ #include "control-node/control_node.h" #include "net/community_type.h" +using boost::assign::list_of; using std::auto_ptr; using std::cout; using std::endl; using std::ostringstream; using std::string; +using std::vector; // // OVERVIEW @@ -52,14 +56,17 @@ using std::string; class BgpTestPeer : public IPeer { public: - explicit BgpTestPeer(bool internal) : internal_(internal) { } + explicit BgpTestPeer(int index, bool internal) + : index_(index), + internal_(internal) { + } virtual ~BgpTestPeer() { } virtual string ToString() const { - return internal_ ? "TestPeerInt" : "TestPeerExt"; + return (string("Peer ") + integerToString(index_)); } virtual string ToUVEKey() const { - return internal_ ? "TestPeerInt" : "TestPeerExt"; + return (string("Peer ") + integerToString(index_)); } virtual bool SendUpdate(const uint8_t *msg, size_t msgsize) { return true; } virtual BgpServer *server() { return NULL; } @@ -84,6 +91,7 @@ class BgpTestPeer : public IPeer { virtual int GetPrimaryPathCount() const { return 0; } private: + int index_; bool internal_; }; @@ -122,9 +130,6 @@ class BgpTableExportTest : public ::testing::Test { ribout_(NULL), result_(false) { server_.set_autonomous_system(200); - active_peerset_.set(1); - active_peerset_.set(3); - active_peerset_.set(5); } virtual void SetUp() { @@ -146,7 +151,7 @@ class BgpTableExportTest : public ::testing::Test { virtual void TearDown() { rt_.RemovePath(peer_.get()); - table_->RibOutDelete(ribout_->ExportPolicy()); + UnregisterRibOutPeers(); server_.Shutdown(); task_util::WaitForIdle(); } @@ -163,8 +168,12 @@ class BgpTableExportTest : public ::testing::Test { return internal_; } + bool TableIsVpn() { + return table_->IsVpnTable(); + } + void CreatePeer() { - peer_.reset(new BgpTestPeer(internal_)); + peer_.reset(new BgpTestPeer(0, internal_)); } void CreateAttr() { @@ -207,6 +216,7 @@ class BgpTableExportTest : public ::testing::Test { RibExportPolicy::Encoding encoding, as_t as_number = 0) { RibExportPolicy policy(type, encoding, as_number, false, -1, 0); ribout_ = table_->RibOutLocate(&mgr_, policy); + RegisterRibOutPeers(); } void CreateRibOut(BgpProto::BgpPeerType type, @@ -215,6 +225,34 @@ class BgpTableExportTest : public ::testing::Test { RibExportPolicy policy( type, encoding, as_number, as_override, nexthop, -1, 0); ribout_ = table_->RibOutLocate(&mgr_, policy); + RegisterRibOutPeers(); + } + + void RegisterRibOut(BgpTestPeer *peer = NULL) { + ConcurrencyScope scope("bgp::PeerMembership"); + ribout_->Register(peer ? peer : peer_.get()); + } + + void UnregisterRibOut(BgpTestPeer *peer = NULL) { + ConcurrencyScope scope("bgp::PeerMembership"); + ribout_->Deactivate(peer ? peer : peer_.get()); + ribout_->Unregister(peer ? peer : peer_.get()); + } + + void RegisterRibOutPeers() { + for (int idx = 0; idx < 3; ++idx) { + bool internal = (ribout_->peer_type() == BgpProto::IBGP); + BgpTestPeer *peer = new BgpTestPeer(16 + idx, internal); + ribout_peers_.push_back(peer); + RegisterRibOut(ribout_peers_[idx]); + } + } + + void UnregisterRibOutPeers() { + for (int idx = 0; idx < 3; ++idx) { + UnregisterRibOut(ribout_peers_[idx]); + } + STLDeleteValues(&ribout_peers_); } void SetAttrAsPath(as_t as_number) { @@ -232,6 +270,15 @@ class BgpTableExportTest : public ::testing::Test { attr_ptr_ = server_.attr_db()->Locate(attr); } + void SetAttrClusterList(const vector cluster_list) { + BgpAttr *attr = new BgpAttr(*attr_ptr_); + ClusterListSpec clist_spec; + clist_spec.cluster_list = cluster_list; + attr->set_cluster_list(&clist_spec); + EXPECT_EQ(cluster_list.size(), attr->cluster_list_length()); + attr_ptr_ = server_.attr_db()->Locate(attr); + } + void SetAttrCommunity(uint32_t comm_value) { BgpAttr *attr = new BgpAttr(*attr_ptr_); CommunitySpec community; @@ -264,6 +311,12 @@ class BgpTableExportTest : public ::testing::Test { attr_ptr_ = server_.attr_db()->Locate(attr); } + void SetAttrOriginatorId(Ip4Address originator_id) { + BgpAttr *attr = new BgpAttr(*attr_ptr_); + attr->set_originator_id(originator_id); + attr_ptr_ = server_.attr_db()->Locate(attr); + } + void AddPath() { BgpPath *path = new BgpPath(peer_.get(), BgpPath::BGP_XMPP, attr_ptr_, 0, 0); @@ -278,7 +331,8 @@ class BgpTableExportTest : public ::testing::Test { } void RunExport() { - result_ = table_->Export(ribout_, &rt_, active_peerset_, uinfo_slist_); + RibPeerSet peerset = ribout_->PeerSet(); + result_ = table_->Export(ribout_, &rt_, peerset, uinfo_slist_); } void VerifyExportReject() { @@ -290,7 +344,11 @@ class BgpTableExportTest : public ::testing::Test { EXPECT_TRUE(true == result_); EXPECT_EQ(1, uinfo_slist_->size()); const UpdateInfo &uinfo = uinfo_slist_->front(); - EXPECT_EQ(active_peerset_, uinfo.target); + RibPeerSet peerset = ribout_->PeerSet(); + int index = ribout_->GetPeerIndex(peer_.get()); + if (index >= 0) + peerset.reset(index); + EXPECT_EQ(peerset, uinfo.target); } void VerifyAttrNoChange() { @@ -345,6 +403,13 @@ class BgpTableExportTest : public ::testing::Test { EXPECT_FALSE(as_path->path().AsPathLoop(as_number, 0)); } + void VerifyAttrNoClusterList() { + const UpdateInfo &uinfo = uinfo_slist_->front(); + const BgpAttr *attr = uinfo.roattr.attr(); + EXPECT_EQ(NULL, attr->cluster_list()); + EXPECT_EQ(0, attr->cluster_list_length()); + } + void VerifyAttrExtCommunity(bool is_null) { const UpdateInfo &uinfo = uinfo_slist_->front(); const BgpAttr *attr = uinfo.roattr.attr(); @@ -357,6 +422,12 @@ class BgpTableExportTest : public ::testing::Test { EXPECT_EQ(nexthop_str, attr->nexthop().to_string()); } + void VerifyAttrNoOriginatorId() { + const UpdateInfo &uinfo = uinfo_slist_->front(); + const BgpAttr *attr = uinfo.roattr.attr(); + EXPECT_TRUE(attr->originator_id().is_unspecified()); + } + EventManager evm_; BgpServerTest server_; SchedulingGroupManager mgr_; @@ -368,7 +439,7 @@ class BgpTableExportTest : public ::testing::Test { BgpTable *table_; RibOut *ribout_; auto_ptr peer_; - RibPeerSet active_peerset_; + vector ribout_peers_; BgpRouteMock rt_; BgpAttrPtr attr_ptr_; @@ -537,6 +608,24 @@ TEST_P(BgpTableExportParamTest1, AsPathLoop) { VerifyExportReject(); } +// +// Table : inet.0, bgp.l3vpn.0 +// Source: eBGP, iBGP +// RibOut: eBGP with AS override. +// Intent: RibOut AS override does not affect impact learnt from IBGP source +// or EBGP source in different AS. +// Note: Assumes that RibOut AS is not in the AS Path. +// +// +TEST_P(BgpTableExportParamTest1, AsOverride) { + CreateRibOut(BgpProto::EBGP, RibExportPolicy::BGP, 300, true, IpAddress()); + AddPath(); + RunExport(); + VerifyExportAccept(); + VerifyAttrAsPrepend(); + VerifyAttrAsPathCount(PeerIsInternal() ? 1 : 2); +} + INSTANTIATE_TEST_CASE_P(Instance, BgpTableExportParamTest1, ::testing::Combine( ::testing::Values("inet.0", "bgp.l3vpn.0"), @@ -588,6 +677,7 @@ TEST_P(BgpTableExportParamTest2, EBgpRetainMed1) { VerifyAttrMed(100); VerifyAttrAsPrepend(); } + // // Table : inet.0, bgp.l3vpn.0 // Source: iBGP @@ -621,6 +711,25 @@ TEST_P(BgpTableExportParamTest2, EBgpNoRetainMed) { VerifyAttrAsPrepend(); } +// +// Table : inet.0, bgp.l3vpn.0 +// Source: iBGP +// RibOut: eBGP +// Intent: Non transitive attributes are stripped. +// +TEST_P(BgpTableExportParamTest2, EBgpStripNonTransitive) { + boost::system::error_code ec; + Ip4Address originator_id = Ip4Address::from_string("10.1.1.1", ec); + CreateRibOut(BgpProto::EBGP, RibExportPolicy::BGP, 300); + SetAttrOriginatorId(originator_id); + SetAttrClusterList(list_of(1)(2)(3)); + AddPath(); + RunExport(); + VerifyExportAccept(); + VerifyAttrNoOriginatorId(); + VerifyAttrNoClusterList(); +} + INSTANTIATE_TEST_CASE_P(Instance, BgpTableExportParamTest2, ::testing::Values("inet.0", "bgp.l3vpn.0")); @@ -766,6 +875,55 @@ TEST_P(BgpTableExportParamTest3, EBgpNoRetainMed) { VerifyAttrAsPrepend(); } +// +// Table : inet.0, bgp.l3vpn.0 +// Source: eBGP +// RibOut: eBGP with AS override. +// Intent: RibOut AS is overridden to local AS. +// Split horizon within RibOut. +// Note: The source peer is registered to the RibOutbut and AS Override is +// enabled in the RibOut, but the route shouldn't be advertised back +// to the source. +// +TEST_P(BgpTableExportParamTest3, AsOverride) { + CreateRibOut(BgpProto::EBGP, RibExportPolicy::BGP, 100, true, IpAddress()); + RegisterRibOut(); + AddPath(); + RunExport(); + VerifyExportAccept(); + VerifyAttrAsPrepend(); + VerifyAttrAsPathCount(PeerIsInternal() ? 1 : 2); + VerifyAttrNoAsPathLoop(100); + UnregisterRibOut(); +} + +// +// Table : inet.0, bgp.l3vpn.0 +// Source: eBGP +// RibOut: eBGP with AS override and nexthop rewrite. +// Intent: Nexthop rewrite and AS override work together. +// Split horizon within RibOut. +// Note: The source peer is registered to the RibOutbut and AS Override is +// enabled in the RibOut, but the route shouldn't be advertised back +// to the source. +// +TEST_P(BgpTableExportParamTest3, AsOverrideAndRewriteNexthop) { + boost::system::error_code ec; + IpAddress nexthop = IpAddress::from_string("2.2.2.2", ec); + CreateRibOut(BgpProto::EBGP, RibExportPolicy::BGP, 100, true, nexthop); + RegisterRibOut(); + SetAttrExtCommunity(12345); + AddPath(); + RunExport(); + VerifyExportAccept(); + VerifyAttrExtCommunity(TableIsVpn() ? false : true); + VerifyAttrNexthop("2.2.2.2"); + VerifyAttrAsPrepend(); + VerifyAttrAsPathCount(PeerIsInternal() ? 1 : 2); + VerifyAttrNoAsPathLoop(100); + UnregisterRibOut(); +} + INSTANTIATE_TEST_CASE_P(Instance, BgpTableExportParamTest3, ::testing::Combine( ::testing::Values("inet.0", "bgp.l3vpn.0"), @@ -842,43 +1000,6 @@ TEST_P(BgpTableExportParamTest4a, RewriteNexthop) { VerifyAttrNexthop("2.2.2.2"); } -// -// Table : inet.0 -// Source: eBGP, iBGP -// RibOut: eBGP with AS override. -// Intent: Ribout AS is overridden to local AS. -// -TEST_P(BgpTableExportParamTest4a, AsOverride) { - CreateRibOut(BgpProto::EBGP, RibExportPolicy::BGP, 100, true, IpAddress()); - AddPath(); - RunExport(); - VerifyExportAccept(); - VerifyAttrAsPrepend(); - VerifyAttrAsPathCount(PeerIsInternal() ? 1 : 2); - VerifyAttrNoAsPathLoop(100); -} - -// -// Table : inet.0 -// Source: eBGP, iBGP -// RibOut: eBGP with AS override and nexthop rewrite. -// Intent: Nexthop rewrite and AS override work together. -// -TEST_P(BgpTableExportParamTest4a, AsOverrideAndRewriteNexthop) { - boost::system::error_code ec; - IpAddress nexthop = IpAddress::from_string("2.2.2.2", ec); - CreateRibOut(BgpProto::EBGP, RibExportPolicy::BGP, 100, true, nexthop); - SetAttrExtCommunity(12345); - AddPath(); - RunExport(); - VerifyExportAccept(); - VerifyAttrExtCommunity(true); - VerifyAttrNexthop("2.2.2.2"); - VerifyAttrAsPrepend(); - VerifyAttrAsPathCount(PeerIsInternal() ? 1 : 2); - VerifyAttrNoAsPathLoop(100); -} - INSTANTIATE_TEST_CASE_P(Instance, BgpTableExportParamTest4a, ::testing::Combine( ::testing::Bool(), diff --git a/src/bgp/test/path_resolver_test.cc b/src/bgp/test/path_resolver_test.cc index 5b24f538c46..fc546e733fe 100644 --- a/src/bgp/test/path_resolver_test.cc +++ b/src/bgp/test/path_resolver_test.cc @@ -517,6 +517,16 @@ class PathResolverTest : public ::testing::Test { table->path_resolver()->EnableResolverPathUpdateProcessing(); } + void PauseResolverPathUpdateProcessing(const string &instance) { + BgpTable *table = GetTable(instance); + table->path_resolver()->PauseResolverPathUpdateProcessing(); + } + + void ResumeResolverPathUpdateProcessing(const string &instance) { + BgpTable *table = GetTable(instance); + table->path_resolver()->ResumeResolverPathUpdateProcessing(); + } + size_t ResolverPathUpdateListSize(const string &instance) { BgpTable *table = GetTable(instance); return table->path_resolver()->GetResolverPathUpdateListSize(); @@ -2188,6 +2198,50 @@ TYPED_TEST(PathResolverTest, Shutdown4) { TASK_UTIL_EXPECT_EQ(0, bgp_server->routing_instance_mgr()->count()); } +// +// Shutdown server and resolver before all resolver paths are deleted. +// Deletion does not finish till resolver paths are deleted and nexthop +// gets unregistered from condition listener. +// +TYPED_TEST(PathResolverTest, Shutdown5) { + BgpServerTestPtr bgp_server = this->bgp_server_; + PeerMock *bgp_peer1 = this->bgp_peer1_; + PeerMock *bgp_peer2 = this->bgp_peer2_; + + this->AddBgpPath(bgp_peer1, "blue", this->BuildPrefix(1), + this->BuildHostAddress(bgp_peer1->ToString())); + this->AddBgpPath(bgp_peer2, "blue", this->BuildPrefix(1), + this->BuildHostAddress(bgp_peer2->ToString())); + + task_util::WaitForIdle(); + this->PauseResolverPathUpdateProcessing("blue"); + + this->DeleteBgpPath(bgp_peer1, "blue", this->BuildPrefix(1)); + this->DeleteBgpPath(bgp_peer2, "blue", this->BuildPrefix(1)); + TASK_UTIL_EXPECT_EQ(2, this->ResolverPathUpdateListSize("blue")); + + TASK_UTIL_EXPECT_EQ(3, bgp_server->routing_instance_mgr()->count()); + bgp_server->Shutdown(false, false); + + // Verify that all instances are intact. + for (int idx = 0; idx < 100; ++idx) { + usleep(10000); + TASK_UTIL_EXPECT_EQ(3, bgp_server->routing_instance_mgr()->count()); + } + + // Ensure that all bgp::ResolverPath tasks are resumed before any of + // them run. Otherwise, it's possible that some of them run, trigger + // creation of bgp::ResolverNexthop task which runs and triggers the + // creation of bgp::Config task, which in turn destroys PathResolver. + // If this sequence happens before all the bgp::ResolverPath tasks + // are resumed, then would be accessing freed memory. + TaskScheduler::GetInstance()->Stop(); + this->ResumeResolverPathUpdateProcessing("blue"); + TaskScheduler::GetInstance()->Start(); + + TASK_UTIL_EXPECT_EQ(0, bgp_server->routing_instance_mgr()->count()); +} + class TestEnvironment : public ::testing::Environment { virtual ~TestEnvironment() { } }; diff --git a/src/bgp/testdata/config_test_41a.xml b/src/bgp/testdata/config_test_41a.xml new file mode 100644 index 00000000000..7a395f3dbcd --- /dev/null +++ b/src/bgp/testdata/config_test_41a.xml @@ -0,0 +1,76 @@ + + + +
192.168.1.1
+ 64512 + 192.168.1.1 + + + target:64512:1 + + bgpaas-server + 64512 + + true + + inet + + + inet6 + + + inet-vpn + + + + + inet + + + inet6 + + + inet-vpn + + + + + bgpaas-client + 65001 +
10.0.0.1
+ 10.0.0.254 + ::ffff:10.0.0.254 + 1024 + + + inet + + + inet6 + + + inet-vpn + + + + + bgpaas-client + 65002 +
10.0.0.2
+ 10.0.0.254 + ::ffff:10.0.0.254 + 1025 + + + inet + + + inet6 + + + inet-vpn + + + + + diff --git a/src/bgp/testdata/config_test_41b.xml b/src/bgp/testdata/config_test_41b.xml new file mode 100644 index 00000000000..54bf665e22b --- /dev/null +++ b/src/bgp/testdata/config_test_41b.xml @@ -0,0 +1,77 @@ + + + +
192.168.1.1
+ 64512 + 192.168.1.1 + + + target:64512:1 + + bgpaas-server + 64512 + + false + + inet + + + inet6 + + + inet-vpn + + + + true + + inet + + + inet6 + + + inet-vpn + + + + + bgpaas-client + 65001 +
10.0.0.1
+ 10.0.0.254 + ::ffff:10.0.0.254 + 1024 + + + inet + + + inet6 + + + inet-vpn + + + + + bgpaas-client + 65002 +
10.0.0.2
+ 10.0.0.254 + ::ffff:10.0.0.254 + 1025 + + + inet + + + inet6 + + + inet-vpn + + + + + diff --git a/src/schema/bgp_schema.xsd b/src/schema/bgp_schema.xsd index f039758f0d1..2cd8dbc51e1 100644 --- a/src/schema/bgp_schema.xsd +++ b/src/schema/bgp_schema.xsd @@ -167,11 +167,14 @@ the attributes apply to both ends of the session. A non-zero hold-time overrides the hold-time inherited from the bgp-router configuration. + AS Override causes the neighbors AS to be replaced with the BGP + speaker AS in the AS Path. +