Further progress on BGPaaS implementation

Highlights:

- Schema updates for BGPaaS
- Do not use BgpAsAServiceParameters as it's being removed
- Use the port number from the remote bgp-router
- Update unit tests based on above changes
- Do not export routes to BgpPeers in non-master instances
- Add router type and port fields to ShowBgpNeighborConfig
- Add router type, peer port and transport address to BgpNeighborResp
- Cosmetic changes to BgpNeighborResp related methods
- Add couple of paranoid checks to RibOut and PeerRibMembershipManager

Change-Id: Id9c06579549f648996e22bf07bd4f79fd47768fa
Partial-Bug: 1518047

src/bgp/bgp_config.cc

@@ -160,7 +160,6 @@ void BgpNeighborConfig::CopyValues(const BgpNeighborConfig &rhs) {
     identifier_ = rhs.identifier_;
     address_ = rhs.address_;
     port_ = rhs.port_;
-    remote_endpoint_ = rhs.remote_endpoint_;
     hold_time_ = rhs.hold_time_;
     loop_count_ = rhs.loop_count_;
     local_as_ = rhs.local_as_;
@@ -180,7 +179,6 @@ int BgpNeighborConfig::CompareTo(const BgpNeighborConfig &rhs) const {
     KEY_COMPARE(identifier_, rhs.identifier_);
     KEY_COMPARE(address_, rhs.address_);
     KEY_COMPARE(port_, rhs.port_);
-    KEY_COMPARE(remote_endpoint_, rhs.remote_endpoint_);
     KEY_COMPARE(hold_time_, rhs.hold_time_);
     KEY_COMPARE(loop_count_, rhs.loop_count_);
     KEY_COMPARE(local_as_, rhs.local_as_);

src/bgp/bgp_config.h

@@ -185,11 +185,6 @@ class BgpNeighborConfig {
     int port() const { return port_; }
     void set_port(int port) { port_ = port; }
 
-    TcpSession::Endpoint remote_endpoint() const { return remote_endpoint_; }
-    void set_remote_endpoint(TcpSession::Endpoint remote_endpoint) {
-        remote_endpoint_ = remote_endpoint;
-    }
-
     std::string router_type() const { return router_type_; }
     void set_router_type(const std::string &router_type) {
         router_type_ = router_type;

src/bgp/bgp_config_ifmap.cc

@@ -302,29 +302,6 @@ static BgpNeighborConfig *MakeBgpNeighborConfig(
         NeighborSetSessionAttributes(neighbor, local_name, session);
     }
 
-    // Get remote endpoint information for bgpaas-clients if appropriate.
-    if (params.router_type == "bgpaas-client" &&
-        remote_router->IsPropertySet(
-            autogen::BgpRouter::BGP_AS_A_SERVICE_PARAMETERS)) {
-        const autogen::BgpAsAServiceParameters &bgpaas_params =
-            remote_router->bgp_as_a_service_parameters();
-        Ip4Address vrouter_address =
-            Ip4Address::from_string(bgpaas_params.vrouter_ip_address, err);
-        if (err) {
-            BGP_LOG_STR(BgpConfig, SandeshLevel::SYS_WARN, BGP_LOG_FLAG_ALL,
-                "Invalid bgpaas-client vrouter ip address " <<
-                bgpaas_params.vrouter_ip_address <<
-                " for neighbor " << neighbor->name());
-        }
-        if (bgpaas_params.port < 0 || bgpaas_params.port > 65535) {
-            BGP_LOG_STR(BgpConfig, SandeshLevel::SYS_WARN, BGP_LOG_FLAG_ALL,
-                "Invalid bgpaas-client port " << bgpaas_params.port <<
-                " for neighbor " << neighbor->name());
-        }
-        neighbor->set_remote_endpoint(TcpSession::Endpoint(vrouter_address,
-            static_cast<uint16_t>(bgpaas_params.port)));
-    }
-
     // Get the local identifier and local as from the master protocol config.
     const BgpIfmapProtocolConfig *master_protocol =
         master_instance->protocol_config();

src/bgp/bgp_config_parser.cc

@@ -369,10 +369,10 @@ static bool ParseBgpRouter(const string &instance, const xml_node &node,
     }
 
     if (add_change) {
-        MapObjectSetProperty("bgp-router", fqname,
-            "bgp-router-parameters", property.release(), requests);
         MapObjectLink("routing-instance", instance,
             "bgp-router", fqname, "instance-bgp-router", requests);
+        MapObjectSetProperty("bgp-router", fqname,
+            "bgp-router-parameters", property.release(), requests);
     } else {
         MapObjectClearProperty("bgp-router", fqname,
             "bgp-router-parameters", requests);

src/bgp/bgp_peer.cc

@@ -343,7 +343,6 @@ BgpPeer::BgpPeer(BgpServer *server, RoutingInstance *instance,
         : server_(server),
           rtinstance_(instance),
           peer_key_(config),
-          remote_endpoint_(config->remote_endpoint()),
           peer_name_(config->name()),
           router_type_(config->router_type()),
           config_(config),
@@ -394,6 +393,7 @@ BgpPeer::BgpPeer(BgpServer *server, RoutingInstance *instance,
     refcount_ = 0;
     primary_path_count_ = 0;
 
+    ProcessEndpointConfig(config);
     ProcessAuthKeyChainConfig(config);
     ProcessFamilyAttributesConfig(config);
 
@@ -550,6 +550,14 @@ bool BgpPeer::ProcessFamilyAttributesConfig(const BgpNeighborConfig *config) {
     return (ret != 0);
 }
 
+void BgpPeer::ProcessEndpointConfig(const BgpNeighborConfig *config) {
+    if (config->router_type() == "bgpaas-client") {
+        endpoint_ = TcpSession::Endpoint(Ip4Address(), config->port());
+    } else {
+        endpoint_ = TcpSession::Endpoint();
+    }
+}
+
 void BgpPeer::ConfigUpdate(const BgpNeighborConfig *config) {
     if (IsDeleted())
         return;
@@ -596,6 +604,7 @@ void BgpPeer::ConfigUpdate(const BgpNeighborConfig *config) {
         peer_info.set_peer_address(peer_key_.endpoint.address().to_string());
         clear_session = true;
     }
+    ProcessEndpointConfig(config);
     ProcessAuthKeyChainConfig(config);
 
     // Check if there is any change in the configured address families.
@@ -604,11 +613,6 @@ void BgpPeer::ConfigUpdate(const BgpNeighborConfig *config) {
         clear_session = true;
     }
 
-    if (remote_endpoint_ != config->remote_endpoint()) {
-        remote_endpoint_ = config->remote_endpoint();
-        clear_session = true;
-    }
-
     BgpProto::BgpPeerType old_type = PeerType();
     if (local_as_ != config->local_as()) {
         local_as_ = config->local_as();
@@ -724,6 +728,15 @@ string BgpPeer::bgp_identifier_string() const {
     return Ip4Address(ntohl(peer_bgp_id_)).to_string();
 }
 
+string BgpPeer::transport_address_string() const {
+    TcpSession::Endpoint endpoint;
+    ostringstream oss;
+    if (session_)
+        endpoint = session_->remote_endpoint();
+    oss << endpoint;
+    return oss.str();
+}
+
 //
 // Customized close routing for BgpPeers.
 //
@@ -1605,45 +1618,45 @@ static void FillSocketStats(const IPeerDebugStats::SocketStats &socket_stats,
     }
 }
 
-void BgpPeer::FillBgpNeighborDebugState(BgpNeighborResp &resp,
+void BgpPeer::FillBgpNeighborDebugState(BgpNeighborResp *bnr,
                                         const IPeerDebugStats *peer_state) {
-    resp.set_last_state(peer_state->last_state());
-    resp.set_last_event(peer_state->last_event());
-    resp.set_last_error(peer_state->last_error());
-    resp.set_last_state_at(peer_state->last_state_change_at());
-    resp.set_flap_count(peer_state->num_flaps());
-    resp.set_flap_time(peer_state->last_flap());
+    bnr->set_last_state(peer_state->last_state());
+    bnr->set_last_event(peer_state->last_event());
+    bnr->set_last_error(peer_state->last_error());
+    bnr->set_last_state_at(peer_state->last_state_change_at());
+    bnr->set_flap_count(peer_state->num_flaps());
+    bnr->set_flap_time(peer_state->last_flap());
 
     IPeerDebugStats::ProtoStats stats;
     PeerProtoStats proto_stats;
     peer_state->GetRxProtoStats(&stats);
     FillProtoStats(stats, proto_stats);
-    resp.set_rx_proto_stats(proto_stats);
+    bnr->set_rx_proto_stats(proto_stats);
 
     peer_state->GetTxProtoStats(&stats);
     FillProtoStats(stats, proto_stats);
-    resp.set_tx_proto_stats(proto_stats);
+    bnr->set_tx_proto_stats(proto_stats);
 
     IPeerDebugStats::UpdateStats update_stats;
     PeerUpdateStats rt_stats;
     peer_state->GetRxRouteUpdateStats(&update_stats);
     FillRouteUpdateStats(update_stats, rt_stats);
-    resp.set_rx_update_stats(rt_stats);
+    bnr->set_rx_update_stats(rt_stats);
 
     peer_state->GetTxRouteUpdateStats(&update_stats);
     FillRouteUpdateStats(update_stats, rt_stats);
-    resp.set_tx_update_stats(rt_stats);
+    bnr->set_tx_update_stats(rt_stats);
 
     IPeerDebugStats::SocketStats socket_stats;
     PeerSocketStats peer_socket_stats;
 
     peer_state->GetRxSocketStats(&socket_stats);
     FillSocketStats(socket_stats, peer_socket_stats);
-    resp.set_rx_socket_stats(peer_socket_stats);
+    bnr->set_rx_socket_stats(peer_socket_stats);
 
     peer_state->GetTxSocketStats(&socket_stats);
     FillSocketStats(socket_stats, peer_socket_stats);
-    resp.set_tx_socket_stats(peer_socket_stats);
+    bnr->set_tx_socket_stats(peer_socket_stats);
 }
 
 void BgpPeer::FillBgpNeighborFamilyAttributes(BgpNeighborResp *nbr) const {
@@ -1664,43 +1677,43 @@ void BgpPeer::FillBgpNeighborFamilyAttributes(BgpNeighborResp *nbr) const {
 }
 
 void BgpPeer::FillNeighborInfo(const BgpSandeshContext *bsc,
-        vector<BgpNeighborResp> *nbr_list, bool summary) const {
-    BgpNeighborResp nbr;
-    nbr.set_peer(peer_basename_);
-    nbr.set_deleted(IsDeleted());
-    nbr.set_deleted_at(UTCUsecToString(deleter_->delete_time_stamp_usecs()));
-    nbr.set_admin_down(admin_down_);
-    nbr.set_passive(passive_);
-    nbr.set_peer_address(peer_key_.endpoint.address().to_string());
-    nbr.set_peer_id(bgp_identifier_string());
-    nbr.set_peer_asn(peer_as());
-    nbr.set_encoding("BGP");
-    nbr.set_peer_type(PeerType() == BgpProto::IBGP ? "internal" : "external");
-    nbr.set_state(state_machine_->StateName());
-    nbr.set_local_address(server_->ToString());
-    nbr.set_local_id(Ip4Address(ntohl(local_bgp_id_)).to_string());
-    nbr.set_local_asn(local_as());
-    nbr.set_negotiated_hold_time(state_machine_->hold_time());
-    nbr.set_primary_path_count(GetPrimaryPathCount());
-    nbr.set_auth_type(AuthenticationData::KeyTypeToString(inuse_authkey_type_));
+    BgpNeighborResp *bnr, bool summary) const {
+    bnr->set_peer(peer_basename_);
+    bnr->set_deleted(IsDeleted());
+    bnr->set_deleted_at(UTCUsecToString(deleter_->delete_time_stamp_usecs()));
+    bnr->set_admin_down(admin_down_);
+    bnr->set_passive(passive_);
+    bnr->set_peer_address(peer_address_string());
+    bnr->set_peer_id(bgp_identifier_string());
+    bnr->set_peer_asn(peer_as());
+    bnr->set_peer_port(peer_port());
+    bnr->set_transport_address(transport_address_string());
+    bnr->set_encoding("BGP");
+    bnr->set_peer_type(PeerType() == BgpProto::IBGP ? "internal" : "external");
+    bnr->set_router_type(router_type_);
+    bnr->set_state(state_machine_->StateName());
+    bnr->set_local_address(server_->ToString());
+    bnr->set_local_id(Ip4Address(ntohl(local_bgp_id_)).to_string());
+    bnr->set_local_asn(local_as());
+    bnr->set_negotiated_hold_time(state_machine_->hold_time());
+    bnr->set_primary_path_count(GetPrimaryPathCount());
+    bnr->set_auth_type(
+        AuthenticationData::KeyTypeToString(inuse_authkey_type_));
     if (bsc->test_mode()) {
-        nbr.set_auth_keys(auth_data_.KeysToStringDetail());
+        bnr->set_auth_keys(auth_data_.KeysToStringDetail());
     }
 
-    if (summary) {
-        nbr_list->push_back(nbr);
+    if (summary)
         return;
-    }
 
-    nbr.set_configured_address_families(configured_families_);
-    nbr.set_negotiated_address_families(negotiated_families_);
-    nbr.set_configured_hold_time(state_machine_->GetConfiguredHoldTime());
-    FillBgpNeighborFamilyAttributes(&nbr);
-    FillBgpNeighborDebugState(nbr, peer_stats_.get());
+    bnr->set_configured_address_families(configured_families_);
+    bnr->set_negotiated_address_families(negotiated_families_);
+    bnr->set_configured_hold_time(state_machine_->GetConfiguredHoldTime());
+    FillBgpNeighborFamilyAttributes(bnr);
+    FillBgpNeighborDebugState(bnr, peer_stats_.get());
     PeerRibMembershipManager *mgr = server_->membership_mgr();
-    mgr->FillPeerMembershipInfo(this, &nbr);
-    nbr.set_routing_instances(vector<BgpNeighborRoutingInstance>());
-    nbr_list->push_back(nbr);
+    mgr->FillPeerMembershipInfo(this, bnr);
+    bnr->set_routing_instances(vector<BgpNeighborRoutingInstance>());
 }
 
 void BgpPeer::inc_rx_open() {

src/bgp/bgp_peer.h

@@ -137,10 +137,12 @@ class BgpPeer : public IPeer {
         return peer_key_.endpoint.address().to_string();
     }
     const BgpPeerKey &peer_key() const { return peer_key_; }
-    TcpSession::Endpoint remote_endpoint() const { return remote_endpoint_; }
+    uint16_t peer_port() const { return peer_key_.endpoint.port(); }
+    std::string transport_address_string() const;
     const std::string &peer_name() const { return peer_name_; }
     const std::string &peer_basename() const { return peer_basename_; }
     std::string router_type() const { return router_type_; }
+    TcpSession::Endpoint endpoint() const { return endpoint_; }
 
     StateMachine::State GetState() const;
     virtual const std::string GetStateName() const;
@@ -184,8 +186,8 @@ class BgpPeer : public IPeer {
     const BgpNeighborConfig *config() const { return config_; }
 
     virtual void SetDataCollectionKey(BgpPeerInfo *peer_info) const;
-    void FillNeighborInfo(const BgpSandeshContext *bsc,
-            std::vector<BgpNeighborResp> *nbr_list, bool summary) const;
+    void FillNeighborInfo(const BgpSandeshContext *bsc, BgpNeighborResp *bnr,
+        bool summary) const;
 
     // thread-safe
     bool IsDeleted() const;
@@ -256,7 +258,8 @@ class BgpPeer : public IPeer {
     uint64_t get_open_error() const;
     uint64_t get_update_error() const;
 
-    static void FillBgpNeighborDebugState(BgpNeighborResp &resp, const IPeerDebugStats *peer);
+    static void FillBgpNeighborDebugState(BgpNeighborResp *bnr,
+        const IPeerDebugStats *peer);
 
     bool ResumeClose();
     void MembershipRequestCallback(IPeer *ipeer, BgpTable *table);
@@ -325,6 +328,7 @@ class BgpPeer : public IPeer {
     void ResetInuseAuthKeyInfo();
 
     bool ProcessFamilyAttributesConfig(const BgpNeighborConfig *config);
+    void ProcessEndpointConfig(const BgpNeighborConfig *config);
 
     void PostCloseRelease();
     void CustomClose();
@@ -336,8 +340,8 @@ class BgpPeer : public IPeer {
     BgpServer *server_;
     // Backpointer to routing instance
     RoutingInstance *rtinstance_;
+    TcpSession::Endpoint endpoint_;
     BgpPeerKey peer_key_;
-    TcpSession::Endpoint remote_endpoint_;
     std::string peer_name_;
     std::string peer_basename_;
     std::string router_type_;         // bgp_schema.xsd:BgpRouterType

src/bgp/bgp_peer.sandesh

@@ -21,10 +21,6 @@ traceobject sandesh BgpPeerObjectTrace {
     1: BgpPeerInfo peer;
 }
 
-request sandesh BgpNeighborReq {
-    1: string search_string;
-}
-
 struct BgpNeighborRoutingInstance {
     1: string name;
     2: string state;
@@ -54,8 +50,11 @@ struct BgpNeighborResp {
     2: string peer_address (link="BgpNeighborReq");
     25: string peer_id;
     3: u32 peer_asn;
+    50: u32 peer_port;
+    51: string transport_address;
     6: string encoding;         // BGP/XMPP
     7: string peer_type         // internal/external
+    52: string router_type;     // bgp_schema.xsd:BgpRouterType
     8: string state;
     4: string local_address;    // local ip address and port
     26: string local_id;
@@ -92,6 +91,10 @@ response sandesh BgpNeighborListResp {
                                    link_title="next_batch");
 }
 
+request sandesh BgpNeighborReq {
+    1: string search_string;
+}
+
 response sandesh ShowBgpNeighborSummaryResp {
     1: list<BgpNeighborResp> neighbors;
     2: optional string next_batch (link="ShowBgpNeighborSummaryReqIterate",
@@ -461,12 +464,14 @@ struct ShowBgpNeighborConfig {
     2: string name;
     13: bool admin_down;
     15: bool passive;
+    18: string router_type;
     8: string local_identifier;
     9: i32 local_as;
     3: string vendor;
     4: i32 autonomous_system;
     5: string identifier;
     6: string address;
+    19: u32 port;
     14: i32 hold_time;
     16: i32 loop_count;
     10: string last_change_at;

src/bgp/bgp_peer_membership.cc

@@ -294,10 +294,17 @@ void IPeerRib::RibOutLeave(DBTablePartBase *root, DBEntryBase *db_entry,
     if (!(action_mask & MembershipRequest::RIBOUT_DELETE)) {
         return;
     }
+    if (!ribout_registered_) {
+        return;
+    }
 
-    RibPeerSet lmask;
-    lmask.set(ribout_->GetPeerIndex(ipeer_));
+    int index = ribout_->GetPeerIndex(ipeer_);
+    if (index < 0) {
+        return;
+    }
 
+    RibPeerSet lmask;
+    lmask.set(index);
     ribout_->bgp_export()->Leave(root, lmask, db_entry);
 }