Fix initialization of configured families for bgp neighbor

The code used to rely on bgp open messages exchanged with the remote
router to handle the negotiation. This does not work as desired in the
case where there are multiple representations for a bgp-router. In the
scenario described in the bug, there was a bgp-router object of type
control-node in cluster A (in which it actually exists) and another
bgp-router object of type external-control-node in cluster B (in order
to  represent the same control-node in cluster B). The problem happened
because the bgp-routers were configured with different adddress families
for a legitimate reason. Another bgp-router in cluster B was configured
to peer with the external-control-node, but the actual bgp negotiation
obviously happens with the control-node in cluster A (and a different
set of address families).

Fix by skipping address families that are not configured on the remote
bgp-router when populating the address family list for a neighbor.

Change-Id: I6d130cc59673dcdc42013678b4f93f6f6dfafad2
Closes-Bug: 1664343

src/bgp/bgp_config_ifmap.cc

@@ -154,11 +154,22 @@ static bool AddressFamilyIsValid(BgpNeighborConfig *neighbor,
 // that represent each family with a simple string.
 //
 static void BuildFamilyAttributesList(BgpNeighborConfig *neighbor,
-    const BgpNeighborConfig::AddressFamilyList &family_list) {
+    const BgpNeighborConfig::AddressFamilyList &family_list,
+    const vector<string> &remote_family_list) {
     BgpNeighborConfig::FamilyAttributesList family_attributes_list;
     BOOST_FOREACH(const string &family, family_list) {
+        // Skip families that are not valid/supported for the neighbor.
         if (!AddressFamilyIsValid(neighbor, family))
             continue;
+
+        // Skip families that are not configured on remote bgp-router.
+        if (!remote_family_list.empty()) {
+            vector<string>::const_iterator it = find(
+                remote_family_list.begin(), remote_family_list.end(), family);
+            if (it == remote_family_list.end())
+                continue;
+        }
+
         BgpFamilyAttributesConfig family_attributes(family);
         family_attributes_list.push_back(family_attributes);
     }
@@ -309,19 +320,20 @@ static BgpNeighborConfig *MakeBgpNeighborConfig(
     const BgpIfmapProtocolConfig *master_protocol =
         master_instance->protocol_config();
     if (master_protocol && master_protocol->bgp_router()) {
-        const autogen::BgpRouterParams &params =
+        const autogen::BgpRouterParams &master_params =
             master_protocol->router_params();
-        if (params.admin_down) {
+        if (master_params.admin_down) {
             neighbor->set_admin_down(true);
         }
-        Ip4Address localid = Ip4Address::from_string(params.identifier, err);
+        Ip4Address localid =
+            Ip4Address::from_string(master_params.identifier, err);
         if (err == 0) {
             neighbor->set_local_identifier(IpAddressToBgpIdentifier(localid));
         }
-        if (params.local_autonomous_system) {
-            neighbor->set_local_as(params.local_autonomous_system);
+        if (master_params.local_autonomous_system) {
+            neighbor->set_local_as(master_params.local_autonomous_system);
         } else {
-            neighbor->set_local_as(params.autonomous_system);
+            neighbor->set_local_as(master_params.autonomous_system);
         }
         if (instance != master_instance) {
             neighbor->set_passive(true);
@@ -332,18 +344,23 @@ static BgpNeighborConfig *MakeBgpNeighborConfig(
     // Note that there's no instance protocol config for non-master instances.
     const BgpIfmapProtocolConfig *protocol = instance->protocol_config();
     if (protocol && protocol->bgp_router()) {
-        const autogen::BgpRouterParams &params = protocol->router_params();
+        const autogen::BgpRouterParams &protocol_params =
+            protocol->router_params();
         if (neighbor->family_attributes_list().empty()) {
-            BuildFamilyAttributesList(neighbor, params.address_families.family);
+            BuildFamilyAttributesList(neighbor,
+                protocol_params.address_families.family,
+                params.address_families.family);
         }
         if (neighbor->auth_data().Empty()) {
-            const autogen::BgpRouterParams &lp = local_router->parameters();
-            BuildKeyChain(neighbor, lp.auth_data);
+            const autogen::BgpRouterParams &local_params =
+                local_router->parameters();
+            BuildKeyChain(neighbor, local_params.auth_data);
         }
     }
 
     if (neighbor->family_attributes_list().empty()) {
-        BuildFamilyAttributesList(neighbor, default_addr_family_list);
+        BuildFamilyAttributesList(neighbor, default_addr_family_list,
+            params.address_families.family);
     }
 
     return neighbor;

src/bgp/test/bgp_config_test.cc

@@ -1705,7 +1705,7 @@ TEST_F(BgpConfigTest, AddressFamilies1) {
 // The address-families config for the local bgp-router should  be used when
 // there's no per session address-families configuration.
 //
-TEST_F(BgpConfigTest, AddressFamilies2) {
+TEST_F(BgpConfigTest, AddressFamilies2a) {
     string content = FileRead("controller/src/bgp/testdata/config_test_13.xml");
     EXPECT_TRUE(parser_.Parse(content));
     task_util::WaitForIdle();
@@ -1729,11 +1729,40 @@ TEST_F(BgpConfigTest, AddressFamilies2) {
     TASK_UTIL_EXPECT_EQ(0, db_graph_.vertex_count());
 }
 
+//
+// The address-families config for the local bgp-router should  be used when
+// there's no per session address-families configuration.
+// Families that are not configured on the remote bgp-router should not be
+// included in the configured family list.
+//
+TEST_F(BgpConfigTest, AddressFamilies2b) {
+    string content = FileRead("controller/src/bgp/testdata/config_test_44.xml");
+    EXPECT_TRUE(parser_.Parse(content));
+    task_util::WaitForIdle();
+
+    RoutingInstance *rti = server_.routing_instance_mgr()->GetRoutingInstance(
+        BgpConfigManager::kMasterInstance);
+    TASK_UTIL_ASSERT_TRUE(rti != NULL);
+    BgpPeer *peer = rti->peer_manager()->PeerFind("10.1.1.1");
+    TASK_UTIL_ASSERT_TRUE(peer != NULL);
+
+    TASK_UTIL_EXPECT_EQ(2, peer->configured_families().size());
+    TASK_UTIL_EXPECT_TRUE(peer->LookupFamily(Address::RTARGET));
+    TASK_UTIL_EXPECT_TRUE(peer->LookupFamily(Address::EVPN));
+
+    boost::replace_all(content, "<config>", "<delete>");
+    boost::replace_all(content, "</config>", "</delete>");
+    EXPECT_TRUE(parser_.Parse(content));
+    task_util::WaitForIdle();
+
+    TASK_UTIL_EXPECT_EQ(0, db_graph_.vertex_count());
+}
+
 //
 // The default family should be used if there's no address-families config
 // for the session or the local bgp-router.
 //
-TEST_F(BgpConfigTest, AddressFamilies3) {
+TEST_F(BgpConfigTest, AddressFamilies3a) {
     string content = FileRead("controller/src/bgp/testdata/config_test_14.xml");
     EXPECT_TRUE(parser_.Parse(content));
     task_util::WaitForIdle();
@@ -1756,6 +1785,34 @@ TEST_F(BgpConfigTest, AddressFamilies3) {
     TASK_UTIL_EXPECT_EQ(0, db_graph_.vertex_count());
 }
 
+//
+// The default family should be used if there's no address-families config
+// for the session or the local bgp-router.
+// Families that are not configured on the remote bgp-router should not be
+// included in the configured family list.
+//
+TEST_F(BgpConfigTest, AddressFamilies3b) {
+    string content = FileRead("controller/src/bgp/testdata/config_test_45.xml");
+    EXPECT_TRUE(parser_.Parse(content));
+    task_util::WaitForIdle();
+
+    RoutingInstance *rti = server_.routing_instance_mgr()->GetRoutingInstance(
+        BgpConfigManager::kMasterInstance);
+    TASK_UTIL_ASSERT_TRUE(rti != NULL);
+    BgpPeer *peer = rti->peer_manager()->PeerFind("10.1.1.1");
+    TASK_UTIL_ASSERT_TRUE(peer != NULL);
+
+    TASK_UTIL_EXPECT_EQ(1, peer->configured_families().size());
+    TASK_UTIL_EXPECT_TRUE(peer->LookupFamily(Address::INETVPN));
+
+    boost::replace_all(content, "<config>", "<delete>");
+    boost::replace_all(content, "</config>", "</delete>");
+    EXPECT_TRUE(parser_.Parse(content));
+    task_util::WaitForIdle();
+
+    TASK_UTIL_EXPECT_EQ(0, db_graph_.vertex_count());
+}
+
 TEST_F(BgpConfigTest, RoutePolicy_0) {
     string content =
         FileRead("controller/src/bgp/testdata/routing_policy_0.xml");

src/bgp/testdata/config_test_44.xml

@@ -0,0 +1,22 @@
+<?xml version="1.0" encoding="utf-8"?>
+<config>
+    <routing-instance name='default-domain:default-project:ip-fabric:__default__'>
+      <bgp-router name='local'>
+          <address>127.0.0.1</address>
+          <autonomous-system>1</autonomous-system>
+          <address-families>
+              <family>route-target</family>
+              <family>inet-vpn</family>
+              <family>e-vpn</family>
+          </address-families>
+      </bgp-router>
+      <bgp-router name='remote1'>
+          <address>10.1.1.1</address>
+          <autonomous-system>101</autonomous-system>
+          <address-families>
+              <family>route-target</family>
+              <family>e-vpn</family>
+          </address-families>
+      </bgp-router>
+    </routing-instance>
+</config>

src/bgp/testdata/config_test_45.xml

@@ -0,0 +1,16 @@
+<?xml version="1.0" encoding="utf-8"?>
+<config>
+    <routing-instance name='default-domain:default-project:ip-fabric:__default__'>
+      <bgp-router name='local'>
+          <address>127.0.0.1</address>
+          <autonomous-system>1</autonomous-system>
+      </bgp-router>
+      <bgp-router name='remote1'>
+          <address>10.1.1.1</address>
+          <autonomous-system>101</autonomous-system>
+          <address-families>
+              <family>inet-vpn</family>
+          </address-families>
+      </bgp-router>
+    </routing-instance>
+</config>