From b6cb6c0575d46d0877cf4e944d107ee45b3eeb04 Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Mon, 7 Mar 2016 15:22:46 -0800 Subject: [PATCH] BGPaaSv2: Use gateway address in RibExportPolicy Change-Id: Idd14d97d088b51206646e9a9d8578df14ac5497e Partial-Bug: 1552952 --- src/bgp/bgp_config_ifmap.cc | 38 ++++++++++-------- src/bgp/bgp_peer.cc | 57 ++++++++++++++++----------- src/bgp/bgp_peer.h | 4 +- src/bgp/bgp_ribout.cc | 13 +++--- src/bgp/bgp_ribout.h | 18 ++++++++- src/bgp/bgp_table.cc | 15 ++++--- src/bgp/inet/inet_table.cc | 18 ++++++--- src/bgp/inet6/inet6_table.cc | 18 ++++++--- src/bgp/test/bgp_table_export_test.cc | 33 ++++++++++++++++ src/bgp/test/bgp_table_test.cc | 46 +++++++++++++++++++++ 10 files changed, 193 insertions(+), 67 deletions(-) diff --git a/src/bgp/bgp_config_ifmap.cc b/src/bgp/bgp_config_ifmap.cc index d0c0ee57d80..090e6b70996 100644 --- a/src/bgp/bgp_config_ifmap.cc +++ b/src/bgp/bgp_config_ifmap.cc @@ -305,24 +305,28 @@ static BgpNeighborConfig *MakeBgpNeighborConfig( } neighbor->set_peer_identifier(IpAddressToBgpIdentifier(identifier)); - IpAddress inet_gw_address = - Ip4Address::from_string(params.gateway_address, err); - if (!params.gateway_address.empty() && err) { - BGP_LOG_STR(BgpConfig, SandeshLevel::SYS_WARN, BGP_LOG_FLAG_ALL, - "Invalid gateway address " << params.gateway_address << - " for neighbor " << neighbor->name()); - } else { - neighbor->set_gateway_address(Address::INET, inet_gw_address); - } + if (params.router_type == "bgpaas-client") { + IpAddress inet_gw_address = + Ip4Address::from_string(params.gateway_address, err); + if (!params.gateway_address.empty() && err) { + BGP_LOG_STR(BgpConfig, SandeshLevel::SYS_WARN, BGP_LOG_FLAG_ALL, + "Invalid gateway address " << + params.gateway_address << + " for neighbor " << neighbor->name()); + } else { + neighbor->set_gateway_address(Address::INET, inet_gw_address); + } - IpAddress inet6_gw_address = - Ip6Address::from_string(params.ipv6_gateway_address, err); - if (!params.ipv6_gateway_address.empty() && err) { - BGP_LOG_STR(BgpConfig, SandeshLevel::SYS_WARN, BGP_LOG_FLAG_ALL, - "Invalid ipv6 gateway address " << params.ipv6_gateway_address << - " for neighbor " << neighbor->name()); - } else { - neighbor->set_gateway_address(Address::INET6, inet6_gw_address); + IpAddress inet6_gw_address = + Ip6Address::from_string(params.ipv6_gateway_address, err); + if (!params.ipv6_gateway_address.empty() && err) { + BGP_LOG_STR(BgpConfig, SandeshLevel::SYS_WARN, BGP_LOG_FLAG_ALL, + "Invalid ipv6 gateway address " << + params.ipv6_gateway_address << + " for neighbor " << neighbor->name()); + } else { + neighbor->set_gateway_address(Address::INET6, inet6_gw_address); + } } neighbor->set_port(params.port); diff --git a/src/bgp/bgp_peer.cc b/src/bgp/bgp_peer.cc index f649c8c2c96..bc2dbe7d54b 100644 --- a/src/bgp/bgp_peer.cc +++ b/src/bgp/bgp_peer.cc @@ -248,10 +248,27 @@ BgpPeerFamilyAttributes::BgpPeerFamilyAttributes( loop_count = config->loop_count(); } prefix_limit = family_config.prefix_limit; - if (family_config.family == "inet") { - gateway_address = config->gateway_address(Address::INET); - } else if (family_config.family == "inet6") { - gateway_address = config->gateway_address(Address::INET6); + + if (config->router_type() == "bgpaas-client") { + if (family_config.family == "inet") { + gateway_address = config->gateway_address(Address::INET); + } else if (family_config.family == "inet6") { + gateway_address = config->gateway_address(Address::INET6); + } + } +} + +RibExportPolicy BgpPeer::BuildRibExportPolicy(Address::Family family) const { + BgpPeerFamilyAttributes *family_attributes = + family_attributes_list_[family]; + if (!family_attributes || + family_attributes->gateway_address.is_unspecified()) { + return RibExportPolicy( + peer_type_, RibExportPolicy::BGP, peer_as_, -1, 0); + } else { + IpAddress nexthop = family_attributes->gateway_address; + return RibExportPolicy( + peer_type_, RibExportPolicy::BGP, peer_as_, nexthop, -1, 0); } } @@ -363,9 +380,6 @@ BgpPeer::BgpPeer(BgpServer *server, RoutingInstance *instance, peer_bgp_id_(0), peer_type_((config->peer_as() == config->local_as()) ? BgpProto::IBGP : BgpProto::EBGP), - policy_((config->peer_as() == config->local_as()) ? - BgpProto::IBGP : BgpProto::EBGP, - RibExportPolicy::BGP, config->peer_as(), -1, 0), state_machine_(BgpObjectFactory::Create(this)), peer_close_(new PeerClose(this)), peer_stats_(new PeerStats(this)), @@ -619,12 +633,6 @@ void BgpPeer::ConfigUpdate(const BgpNeighborConfig *config) { } ProcessEndpointConfig(config); - // Check if there is any change in the configured address families. - if (ProcessFamilyAttributesConfig(config)) { - peer_info.set_configured_families(configured_families_); - clear_session = true; - } - BgpProto::BgpPeerType old_type = PeerType(); if (local_as_ != config->local_as()) { local_as_ = config->local_as(); @@ -657,8 +665,12 @@ void BgpPeer::ConfigUpdate(const BgpNeighborConfig *config) { if (old_type != PeerType()) { peer_info.set_peer_type( PeerType() == BgpProto::IBGP ? "internal" : "external"); - policy_.type = peer_type_; - policy_.as_number = peer_as_; + clear_session = true; + } + + // Check if there is any change in the configured address families. + if (ProcessFamilyAttributesConfig(config)) { + peer_info.set_configured_families(configured_families_); clear_session = true; } @@ -888,8 +900,8 @@ void BgpPeer::RegisterAllTables() { BgpTable *table = instance->GetTable(family); BGP_LOG_PEER_TABLE(this, SandeshLevel::SYS_DEBUG, BGP_LOG_FLAG_TRACE, table, "Register peer with the table"); - membership_mgr->Register(this, table, policy_, -1, - boost::bind(&BgpPeer::MembershipRequestCallback, this, _1, _2)); + membership_mgr->Register(this, table, BuildRibExportPolicy(family), + -1, boost::bind(&BgpPeer::MembershipRequestCallback, this, _1, _2)); membership_req_pending_++; } @@ -899,11 +911,12 @@ void BgpPeer::RegisterAllTables() { return; } - BgpTable *table = instance->GetTable(Address::RTARGET); + Address::Family family = Address::RTARGET; + BgpTable *table = instance->GetTable(family); BGP_LOG_PEER_TABLE(this, SandeshLevel::SYS_DEBUG, BGP_LOG_FLAG_TRACE, table, "Register peer with the table"); - membership_mgr->Register(this, table, policy_, -1, - boost::bind(&BgpPeer::MembershipRequestCallback, this, _1, _2)); + membership_mgr->Register(this, table, BuildRibExportPolicy(family), + -1, boost::bind(&BgpPeer::MembershipRequestCallback, this, _1, _2)); membership_req_pending_++; StartEndOfRibTimer(); @@ -1354,8 +1367,8 @@ void BgpPeer::RegisterToVpnTables() { BgpTable *table = instance->GetTable(vpn_family); BGP_LOG_PEER_TABLE(this, SandeshLevel::SYS_INFO, BGP_LOG_FLAG_TRACE, table, "Register peer with the table"); - membership_mgr->Register(this, table, policy_, -1, - boost::bind(&BgpPeer::MembershipRequestCallback, this, _1, _2)); + membership_mgr->Register(this, table, BuildRibExportPolicy(vpn_family), + -1, boost::bind(&BgpPeer::MembershipRequestCallback, this, _1, _2)); membership_req_pending_++; } } diff --git a/src/bgp/bgp_peer.h b/src/bgp/bgp_peer.h index f367867e0dc..3a51d7aa4b5 100644 --- a/src/bgp/bgp_peer.h +++ b/src/bgp/bgp_peer.h @@ -309,6 +309,7 @@ class BgpPeer : public IPeer { void StopKeepaliveTimerUnlocked(); bool KeepaliveTimerExpired(); + RibExportPolicy BuildRibExportPolicy(Address::Family family) const; void ReceiveEndOfRIB(Address::Family family, size_t msgsize); void SendEndOfRIB(Address::Family family); void StartEndOfRibTimer(); @@ -318,8 +319,6 @@ class BgpPeer : public IPeer { virtual void BindLocalEndpoint(BgpSession *session); - void UnregisterAllTables(); - uint32_t GetPathFlags(Address::Family family, const BgpAttr *attr) const; virtual bool MpNlriAllowed(uint16_t afi, uint8_t safi); BgpAttrPtr GetMpNlriNexthop(BgpMpNlri *nlri, BgpAttrPtr attr); @@ -401,7 +400,6 @@ class BgpPeer : public IPeer { std::vector configured_families_; std::vector negotiated_families_; BgpProto::BgpPeerType peer_type_; - RibExportPolicy policy_; boost::scoped_ptr state_machine_; boost::scoped_ptr peer_close_; boost::scoped_ptr peer_stats_; diff --git a/src/bgp/bgp_ribout.cc b/src/bgp/bgp_ribout.cc index 84441a69abb..ef5b1654a28 100644 --- a/src/bgp/bgp_ribout.cc +++ b/src/bgp/bgp_ribout.cc @@ -41,6 +41,12 @@ bool RibExportPolicy::operator<(const RibExportPolicy &rhs) const { if (as_number > rhs.as_number) { return false; } + if (nexthop < rhs.nexthop) { + return true; + } + if (nexthop > rhs.nexthop) { + return false; + } if (affinity < rhs.affinity) { return true; } @@ -206,13 +212,8 @@ void RibOutAttr::set_attr(const BgpTable *table, const BgpAttrPtr &attrp, return; } - bool nexthop_changed = attr_out_->nexthop() != attrp->nexthop(); + assert(attr_out_->nexthop() == attrp->nexthop()); attr_out_ = attrp; - - // Update the next-hop list ? We would need label too then. - if (nexthop_changed) { - assert(false); - } } RouteState::RouteState() { diff --git a/src/bgp/bgp_ribout.h b/src/bgp/bgp_ribout.h index 597e76e0767..e4adcea048b 100644 --- a/src/bgp/bgp_ribout.h +++ b/src/bgp/bgp_ribout.h @@ -218,6 +218,10 @@ class RouteState : public DBState { // practical deployment scenarios all eBGP peers will belong to the same // neighbor AS anyway. // +// Including the nexthop as part of the policy results in creation of a +// different RibOut for each set of BGPaaS clients that belong to the same +// subnet. This allows us to rewrite the nexthop to the specified value. +// // Including the CPU affinity as part of the RibExportPolicy allows us to // artificially create more RibOuts than otherwise necessary. This is used // to achieve higher concurrency at the expense of creating more state. @@ -234,7 +238,7 @@ struct RibExportPolicy { } RibExportPolicy(BgpProto::BgpPeerType type, Encoding encoding, - int affinity, u_int32_t cluster_id) + int affinity, u_int32_t cluster_id) : type(type), encoding(encoding), as_number(0), affinity(affinity), cluster_id(cluster_id) { if (encoding == XMPP) @@ -244,7 +248,7 @@ struct RibExportPolicy { } RibExportPolicy(BgpProto::BgpPeerType type, Encoding encoding, - as_t as_number, int affinity, u_int32_t cluster_id) + as_t as_number, int affinity, u_int32_t cluster_id) : type(type), encoding(encoding), as_number(as_number), affinity(affinity), cluster_id(cluster_id) { if (encoding == XMPP) @@ -253,11 +257,20 @@ struct RibExportPolicy { assert(type == BgpProto::IBGP || type == BgpProto::EBGP); } + RibExportPolicy(BgpProto::BgpPeerType type, Encoding encoding, + as_t as_number, IpAddress nexthop, int affinity, u_int32_t cluster_id) + : type(type), encoding(BGP), as_number(as_number), + nexthop(nexthop), affinity(affinity), cluster_id(cluster_id) { + assert(type == BgpProto::IBGP || type == BgpProto::EBGP); + assert(encoding == BGP); + } + bool operator<(const RibExportPolicy &rhs) const; BgpProto::BgpPeerType type; Encoding encoding; as_t as_number; + IpAddress nexthop; int affinity; uint32_t cluster_id; }; @@ -329,6 +342,7 @@ class RibOut { BgpProto::BgpPeerType peer_type() const { return policy_.type; } as_t peer_as() const { return policy_.as_number; } + const IpAddress &nexthop() const { return policy_.nexthop; } bool IsEncodingXmpp() const { return (policy_.encoding == RibExportPolicy::XMPP); } diff --git a/src/bgp/bgp_table.cc b/src/bgp/bgp_table.cc index ea5f2dcbd9d..935e5e9db67 100644 --- a/src/bgp/bgp_table.cc +++ b/src/bgp/bgp_table.cc @@ -196,11 +196,13 @@ UpdateInfo *BgpTable::GetUpdateInfo(RibOut *ribout, BgpRoute *route, attr_ptr = attr->attr_db()->Locate(clone); attr = attr_ptr.get(); } else if (ribout->peer_type() == BgpProto::EBGP) { - // Don't advertise any routes from non-master instances. - // The ribout can only be for bgpaas-clients since that's - // the only case with bgp peers in non-master instance. - if (!rtinstance_->IsMasterRoutingInstance()) + // Don't advertise routes from non-master instances if there's + // no nexthop. The ribout has to be for bgpaas-clients because + // that's the only case with bgp peers in non-master instance. + if (!rtinstance_->IsMasterRoutingInstance() && + ribout->nexthop().is_unspecified()) { return NULL; + } // Sender side AS path loop check. if (attr->as_path() && @@ -218,6 +220,10 @@ UpdateInfo *BgpTable::GetUpdateInfo(RibOut *ribout, BgpRoute *route, BgpAttr *clone = new BgpAttr(*attr); + // Update nexthop. + if (!ribout->nexthop().is_unspecified()) + clone->set_nexthop(ribout->nexthop()); + // Reset LocalPref. if (attr->local_pref()) clone->set_local_pref(0); @@ -285,7 +291,6 @@ bool BgpTable::InputCommon(DBTablePartBase *root, BgpRoute *rt, BgpPath *path, if ((path->GetAttr() != attrs.get()) || (path->GetFlags() != flags) || (path->GetLabel() != label)) { - // Update Attributes and notify (if needed) if (path->NeedsResolution()) path_resolver_->StopPathResolution(root->index(), path); diff --git a/src/bgp/inet/inet_table.cc b/src/bgp/inet/inet_table.cc index 9cdbed89d16..9a76c8baf3a 100644 --- a/src/bgp/inet/inet_table.cc +++ b/src/bgp/inet/inet_table.cc @@ -127,13 +127,19 @@ bool InetTable::Export(RibOut *ribout, Route *route, const RibPeerSet &peerset, if (!uinfo) return false; - // Strip BGP extended communities out by default. if (ribout->ExportPolicy().encoding == RibExportPolicy::BGP) { - if (uinfo->roattr.attr()->ext_community() != NULL) { - BgpAttrDB *attr_db = routing_instance()->server()->attr_db(); - BgpAttrPtr new_attr = - attr_db->ReplaceExtCommunityAndLocate( - uinfo->roattr.attr(), NULL); + BgpAttrDB *attr_db = routing_instance()->server()->attr_db(); + // Strip ExtCommunity. + if (uinfo->roattr.attr()->ext_community()) { + BgpAttrPtr new_attr = attr_db->ReplaceExtCommunityAndLocate( + uinfo->roattr.attr(), NULL); + uinfo->roattr.set_attr(this, new_attr); + } + + // Strip OriginVnPath. + if (uinfo->roattr.attr()->origin_vn_path()) { + BgpAttrPtr new_attr = attr_db->ReplaceOriginVnPathAndLocate( + uinfo->roattr.attr(), NULL); uinfo->roattr.set_attr(this, new_attr); } } diff --git a/src/bgp/inet6/inet6_table.cc b/src/bgp/inet6/inet6_table.cc index ed3db029826..037ca6a1c65 100644 --- a/src/bgp/inet6/inet6_table.cc +++ b/src/bgp/inet6/inet6_table.cc @@ -128,13 +128,19 @@ bool Inet6Table::Export(RibOut *ribout, Route *route, const RibPeerSet &peerset, return false; } - // Strip BGP extended communities out by default. if (ribout->ExportPolicy().encoding == RibExportPolicy::BGP) { - if (uinfo->roattr.attr()->ext_community() != NULL) { - BgpAttrDB *attr_db = routing_instance()->server()->attr_db(); - BgpAttrPtr new_attr = - attr_db->ReplaceExtCommunityAndLocate( - uinfo->roattr.attr(), NULL); + BgpAttrDB *attr_db = routing_instance()->server()->attr_db(); + // Strip ExtCommunity. + if (uinfo->roattr.attr()->ext_community()) { + BgpAttrPtr new_attr = attr_db->ReplaceExtCommunityAndLocate( + uinfo->roattr.attr(), NULL); + uinfo->roattr.set_attr(this, new_attr); + } + + // Strip OriginVnPath. + if (uinfo->roattr.attr()->origin_vn_path()) { + BgpAttrPtr new_attr = attr_db->ReplaceOriginVnPathAndLocate( + uinfo->roattr.attr(), NULL); uinfo->roattr.set_attr(this, new_attr); } } diff --git a/src/bgp/test/bgp_table_export_test.cc b/src/bgp/test/bgp_table_export_test.cc index 720497392bd..1b832cb900d 100644 --- a/src/bgp/test/bgp_table_export_test.cc +++ b/src/bgp/test/bgp_table_export_test.cc @@ -206,6 +206,13 @@ class BgpTableExportTest : public ::testing::Test { ribout_ = table_->RibOutLocate(&mgr_, policy); } + void CreateRibOut(BgpProto::BgpPeerType type, + RibExportPolicy::Encoding encoding, as_t as_number, + IpAddress nexthop) { + RibExportPolicy policy(type, encoding, as_number, nexthop, -1, 0); + ribout_ = table_->RibOutLocate(&mgr_, policy); + } + void SetAttrAsPath(as_t as_number) { BgpAttr *attr = new BgpAttr(*attr_ptr_); const AsPathSpec &path_spec = attr_ptr_->as_path()->path(); @@ -327,6 +334,12 @@ class BgpTableExportTest : public ::testing::Test { EXPECT_EQ(is_null, attr->ext_community() == NULL); } + void VerifyAttrNexthop(const string &nexthop_str) { + const UpdateInfo &uinfo = uinfo_slist_->front(); + const BgpAttr *attr = uinfo.roattr.attr(); + EXPECT_EQ(nexthop_str, attr->nexthop().to_string()); + } + EventManager evm_; BgpServerTest server_; SchedulingGroupManager mgr_; @@ -778,6 +791,7 @@ TEST_P(BgpTableExportParamTest4a, StripExtendedCommunity1) { RunExport(); VerifyExportAccept(); VerifyAttrExtCommunity(true); + VerifyAttrNexthop("1.1.1.1"); } // @@ -796,9 +810,28 @@ TEST_P(BgpTableExportParamTest4a, StripExtendedCommunity2) { } else { VerifyExportAccept(); VerifyAttrExtCommunity(true); + VerifyAttrNexthop("1.1.1.1"); } } +// +// Table : inet.0 +// Source: eBGP, iBGP +// RibOut: eBGP with nexthop rewrite +// Intent: +// +TEST_P(BgpTableExportParamTest4a, RewriteNexthop) { + boost::system::error_code ec; + IpAddress nexthop = IpAddress::from_string("2.2.2.2", ec); + CreateRibOut(BgpProto::EBGP, RibExportPolicy::BGP, 300, nexthop); + SetAttrExtCommunity(12345); + AddPath(); + RunExport(); + VerifyExportAccept(); + VerifyAttrExtCommunity(true); + VerifyAttrNexthop("2.2.2.2"); +} + INSTANTIATE_TEST_CASE_P(Instance, BgpTableExportParamTest4a, ::testing::Combine( ::testing::Bool(), diff --git a/src/bgp/test/bgp_table_test.cc b/src/bgp/test/bgp_table_test.cc index 01a04359ba5..d4556ef9b36 100644 --- a/src/bgp/test/bgp_table_test.cc +++ b/src/bgp/test/bgp_table_test.cc @@ -285,6 +285,52 @@ TEST_F(BgpTableTest, RiboutAS) { ASSERT_TRUE(ribout3 == NULL); } +// Different nexthops result in creation of different RibOuts. +TEST_F(BgpTableTest, RiboutNexthop) { + boost::system::error_code ec; + IpAddress nexthop1 = IpAddress::from_string("10.1.1.1", ec); + IpAddress nexthop2 = IpAddress::from_string("10.1.1.2", ec); + IpAddress nexthop3 = IpAddress::from_string("10.1.1.3", ec); + RibOut *ribout1 = NULL, *ribout2 = NULL, *ribout3 = NULL, *temp = NULL; + RibExportPolicy policy1( + BgpProto::EBGP, RibExportPolicy::BGP, 100, nexthop1, -1, 0); + RibExportPolicy policy2( + BgpProto::EBGP, RibExportPolicy::BGP, 100, nexthop2, -1, 0); + RibExportPolicy policy3( + BgpProto::EBGP, RibExportPolicy::BGP, 100, nexthop3, -1, 0); + + // Create 3 ribouts. + ribout1 = rt_table_->RibOutLocate(&mgr_, policy1); + ASSERT_TRUE(ribout1 != NULL); + ribout2 = rt_table_->RibOutLocate(&mgr_, policy2); + ASSERT_TRUE(ribout2 != NULL); + ribout3 = rt_table_->RibOutLocate(&mgr_, policy3); + ASSERT_TRUE(ribout3 != NULL); + ASSERT_EQ(rt_table_->ribout_map().size(), 3); + + // Check if we can find them. + temp = rt_table_->RibOutFind(policy1); + ASSERT_EQ(temp, ribout1); + temp = rt_table_->RibOutFind(policy2); + ASSERT_EQ(temp, ribout2); + temp = rt_table_->RibOutFind(policy3); + ASSERT_EQ(temp, ribout3); + + // Delete all of them. + rt_table_->RibOutDelete(policy1); + rt_table_->RibOutDelete(policy2); + rt_table_->RibOutDelete(policy3); + ASSERT_EQ(rt_table_->ribout_map().size(), 0); + + // Make sure they are all gone. + ribout1 = rt_table_->RibOutFind(policy1); + ASSERT_TRUE(ribout1 == NULL); + ribout2 = rt_table_->RibOutFind(policy2); + ASSERT_TRUE(ribout2 == NULL); + ribout3 = rt_table_->RibOutFind(policy3); + ASSERT_TRUE(ribout3 == NULL); +} + static void SetUp() { bgp_log_test::init(); ControlNode::SetDefaultSchedulingPolicy();