From 456f800719f0d8e5f10e2c962312cc50935bff1f Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Mon, 13 Feb 2017 14:07:49 -0800 Subject: [PATCH] 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 | 41 +++++++++++++------ src/bgp/test/bgp_config_test.cc | 61 ++++++++++++++++++++++++++++- src/bgp/testdata/config_test_44.xml | 22 +++++++++++ src/bgp/testdata/config_test_45.xml | 16 ++++++++ 4 files changed, 126 insertions(+), 14 deletions(-) create mode 100644 src/bgp/testdata/config_test_44.xml create mode 100644 src/bgp/testdata/config_test_45.xml diff --git a/src/bgp/bgp_config_ifmap.cc b/src/bgp/bgp_config_ifmap.cc index 69755f88f11..7418a2bdc6a 100644 --- a/src/bgp/bgp_config_ifmap.cc +++ b/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 &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::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 ¶ms = + 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 ¶ms = 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; diff --git a/src/bgp/test/bgp_config_test.cc b/src/bgp/test/bgp_config_test.cc index 98206e3c8fc..1f0975244ed 100644 --- a/src/bgp/test/bgp_config_test.cc +++ b/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, "", ""); + boost::replace_all(content, "", ""); + 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, "", ""); + boost::replace_all(content, "", ""); + 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"); diff --git a/src/bgp/testdata/config_test_44.xml b/src/bgp/testdata/config_test_44.xml new file mode 100644 index 00000000000..f800fec685a --- /dev/null +++ b/src/bgp/testdata/config_test_44.xml @@ -0,0 +1,22 @@ + + + + +
127.0.0.1
+ 1 + + route-target + inet-vpn + e-vpn + + + +
10.1.1.1
+ 101 + + route-target + e-vpn + + + + diff --git a/src/bgp/testdata/config_test_45.xml b/src/bgp/testdata/config_test_45.xml new file mode 100644 index 00000000000..bf54fb0b5a9 --- /dev/null +++ b/src/bgp/testdata/config_test_45.xml @@ -0,0 +1,16 @@ + + + + +
127.0.0.1
+ 1 + + +
10.1.1.1
+ 101 + + inet-vpn + + + +