From 115f95887ddd5578da3f7f8708dcb84958686318 Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Thu, 11 Aug 2016 14:25:59 -0700 Subject: [PATCH] Treat VN index >= 8000000 as global This is required for correct operation in multi-region scenarios with service chaining. If there are 2 or more regions re-originating routes for service chaining, we do not want to derive the origin vn from the route targets of routes received from other regions. We want the global controller to manage allocation of vn ids and trust received values if they have been allocated by global controller. Values greater than 8000000 are treated as global and are allocated by the global controller. Need to disregard the ASN and compare only the vn ids in this case. Change-Id: I2404517b5d0024ebbea04eb053273aea45ddf20d Closes-Bug: 1612415 --- src/bgp/bgp_origin_vn_path.cc | 8 +++++ src/bgp/origin-vn/origin_vn.cc | 4 +++ src/bgp/origin-vn/origin_vn.h | 2 ++ src/bgp/origin-vn/test/origin_vn_test.cc | 30 ++++++++++++++++ .../routing-instance/routepath_replicator.cc | 7 ++-- src/bgp/test/bgp_attr_test.cc | 36 ++++++++++++++++++- src/bgp/xmpp_message_builder.cc | 1 - 7 files changed, 84 insertions(+), 4 deletions(-) diff --git a/src/bgp/bgp_origin_vn_path.cc b/src/bgp/bgp_origin_vn_path.cc index 453475865e4..88990b87b36 100644 --- a/src/bgp/bgp_origin_vn_path.cc +++ b/src/bgp/bgp_origin_vn_path.cc @@ -9,6 +9,7 @@ #include #include "bgp/bgp_proto.h" +#include "bgp/origin-vn/origin_vn.h" using std::string; using std::vector; @@ -59,10 +60,17 @@ void OriginVnPath::Prepend(const OriginVnValue &value) { } bool OriginVnPath::Contains(const OriginVnValue &val) const { + OriginVn in_origin_vn(val); + int in_vn_index = in_origin_vn.IsGlobal() ? in_origin_vn.vn_index() : 0; for (OriginVnList::const_iterator it = origin_vns_.begin(); it != origin_vns_.end(); ++it) { if (*it == val) return true; + if (in_vn_index == 0) + continue; + OriginVn origin_vn(*it); + if (origin_vn.vn_index() == in_vn_index) + return true; } return false; } diff --git a/src/bgp/origin-vn/origin_vn.cc b/src/bgp/origin-vn/origin_vn.cc index 9516f230947..1e3acd276f4 100644 --- a/src/bgp/origin-vn/origin_vn.cc +++ b/src/bgp/origin-vn/origin_vn.cc @@ -110,6 +110,10 @@ int OriginVn::vn_index() const { return 0; } +bool OriginVn::IsGlobal() const { + return (vn_index() >= kMinGlobalId); +} + string OriginVn::ToString() { char temp[50]; snprintf(temp, sizeof(temp), "originvn:%u:%u", as_number(), vn_index()); diff --git a/src/bgp/origin-vn/origin_vn.h b/src/bgp/origin-vn/origin_vn.h index 51786524275..5697b2c1dec 100644 --- a/src/bgp/origin-vn/origin_vn.h +++ b/src/bgp/origin-vn/origin_vn.h @@ -16,6 +16,7 @@ class OriginVn { public: static const int kSize = 8; + static const int kMinGlobalId = 8000000; static OriginVn null_originvn; typedef boost::array bytes_type; @@ -24,6 +25,7 @@ class OriginVn { explicit OriginVn(const bytes_type &data); bool IsNull() { return operator==(OriginVn::null_originvn); } + bool IsGlobal() const; as_t as_number() const; int vn_index() const; diff --git a/src/bgp/origin-vn/test/origin_vn_test.cc b/src/bgp/origin-vn/test/origin_vn_test.cc index 09643efcf73..de256669ddc 100644 --- a/src/bgp/origin-vn/test/origin_vn_test.cc +++ b/src/bgp/origin-vn/test/origin_vn_test.cc @@ -20,6 +20,7 @@ TEST_F(OriginVnTest, ByteArray_1) { EXPECT_EQ(65412, origin_vn.as_number()); EXPECT_EQ(16909060, origin_vn.vn_index()); EXPECT_EQ("originvn:65412:16909060", origin_vn.ToString()); + EXPECT_TRUE(origin_vn.IsGlobal()); } TEST_F(OriginVnTest, ByteArray_2) { @@ -31,6 +32,7 @@ TEST_F(OriginVnTest, ByteArray_2) { EXPECT_EQ(65412, origin_vn.as_number()); EXPECT_EQ(67305985, origin_vn.vn_index()); EXPECT_EQ("originvn:65412:67305985", origin_vn.ToString()); + EXPECT_TRUE(origin_vn.IsGlobal()); } TEST_F(OriginVnTest, ByteArray_3) { @@ -42,6 +44,7 @@ TEST_F(OriginVnTest, ByteArray_3) { EXPECT_EQ(65412, origin_vn.as_number()); EXPECT_EQ(0, origin_vn.vn_index()); EXPECT_EQ("originvn:65412:0", origin_vn.ToString()); + EXPECT_FALSE(origin_vn.IsGlobal()); } TEST_F(OriginVnTest, ByteArray_4) { @@ -53,6 +56,19 @@ TEST_F(OriginVnTest, ByteArray_4) { EXPECT_EQ(65412, origin_vn.as_number()); EXPECT_EQ(2147483647, origin_vn.vn_index()); EXPECT_EQ("originvn:65412:2147483647", origin_vn.ToString()); + EXPECT_TRUE(origin_vn.IsGlobal()); +} + +TEST_F(OriginVnTest, ByteArray_5) { + OriginVn::bytes_type data = { + { 0x80, 0x71, 0xff, 0x84, 0x00, 0x00, 0xff, 0xff } + }; + OriginVn origin_vn(data); + EXPECT_FALSE(origin_vn.IsNull()); + EXPECT_EQ(65412, origin_vn.as_number()); + EXPECT_EQ(65535, origin_vn.vn_index()); + EXPECT_EQ("originvn:65412:65535", origin_vn.ToString()); + EXPECT_FALSE(origin_vn.IsGlobal()); } TEST_F(OriginVnTest, FromString_1) { @@ -62,6 +78,7 @@ TEST_F(OriginVnTest, FromString_1) { EXPECT_EQ(65412, origin_vn.as_number()); EXPECT_EQ(16909060, origin_vn.vn_index()); EXPECT_EQ("originvn:65412:16909060", origin_vn.ToString()); + EXPECT_TRUE(origin_vn.IsGlobal()); } TEST_F(OriginVnTest, FromString_2) { @@ -71,6 +88,7 @@ TEST_F(OriginVnTest, FromString_2) { EXPECT_EQ(65412, origin_vn.as_number()); EXPECT_EQ(67305985, origin_vn.vn_index()); EXPECT_EQ("originvn:65412:67305985", origin_vn.ToString()); + EXPECT_TRUE(origin_vn.IsGlobal()); } TEST_F(OriginVnTest, FromString_3) { @@ -80,6 +98,7 @@ TEST_F(OriginVnTest, FromString_3) { EXPECT_EQ(65412, origin_vn.as_number()); EXPECT_EQ(0, origin_vn.vn_index()); EXPECT_EQ("originvn:65412:0", origin_vn.ToString()); + EXPECT_FALSE(origin_vn.IsGlobal()); } TEST_F(OriginVnTest, FromString_4) { @@ -89,6 +108,17 @@ TEST_F(OriginVnTest, FromString_4) { EXPECT_EQ(65412, origin_vn.as_number()); EXPECT_EQ(2147483647, origin_vn.vn_index()); EXPECT_EQ("originvn:65412:2147483647", origin_vn.ToString()); + EXPECT_TRUE(origin_vn.IsGlobal()); +} + +TEST_F(OriginVnTest, FromString_5) { + boost::system::error_code ec; + OriginVn origin_vn = OriginVn::FromString("originvn:65412:65535", &ec); + EXPECT_EQ(0, ec.value()); + EXPECT_EQ(65412, origin_vn.as_number()); + EXPECT_EQ(65535, origin_vn.vn_index()); + EXPECT_EQ("originvn:65412:65535", origin_vn.ToString()); + EXPECT_FALSE(origin_vn.IsGlobal()); } // Does not contain a colon. diff --git a/src/bgp/routing-instance/routepath_replicator.cc b/src/bgp/routing-instance/routepath_replicator.cc index 5ea37e83a1f..9cce204a473 100644 --- a/src/bgp/routing-instance/routepath_replicator.cc +++ b/src/bgp/routing-instance/routepath_replicator.cc @@ -399,14 +399,17 @@ static ExtCommunityPtr UpdateExtCommunity(BgpServer *server, if (!ext_community) return ExtCommunityPtr(NULL); - // Nothing to do if we already have the OriginVn community with our AS. + // Nothing to do if we already have the OriginVn community with our AS + // or with a vn index from the global range. BOOST_FOREACH(const ExtCommunity::ExtCommunityValue &comm, ext_community->communities()) { if (!ExtCommunity::is_origin_vn(comm)) continue; OriginVn origin_vn(comm); - if (origin_vn.as_number() != server->autonomous_system()) + if (!origin_vn.IsGlobal() && + origin_vn.as_number() != server->autonomous_system()) { continue; + } return ExtCommunityPtr(ext_community); } diff --git a/src/bgp/test/bgp_attr_test.cc b/src/bgp/test/bgp_attr_test.cc index f2eccc8b863..1b0e591ba02 100644 --- a/src/bgp/test/bgp_attr_test.cc +++ b/src/bgp/test/bgp_attr_test.cc @@ -713,7 +713,11 @@ TEST_F(BgpAttrTest, OriginVnPathPrepend) { EXPECT_EQ(0, ovnpath2.CompareTo(ovnpath1)); } -TEST_F(BgpAttrTest, OriginVnPathContains) { +// +// Both AS and VN index are compared for OriginVns with index from non-global +// range. +// +TEST_F(BgpAttrTest, OriginVnPathContains1) { OriginVnPathSpec spec; for (int idx = 9; idx >= 1; idx -= 2) { OriginVn origin_vn(64512, 100 * idx); @@ -741,6 +745,36 @@ TEST_F(BgpAttrTest, OriginVnPathContains) { } } +// +// OriginVns with matching VN index from global range are treated as equal +// even if the AS numbers are different. +// OriginVns with matching VN index from non-global range are treated as not +// equal because AS numbers are different. +// +TEST_F(BgpAttrTest, OriginVnPathContains2) { + OriginVnPathSpec spec; + for (int idx = 9; idx >= 1; idx--) { + if (idx % 2 == 1) { + OriginVn origin_vn(64512, OriginVn::kMinGlobalId + 100 * idx); + spec.origin_vns.push_back(origin_vn.GetExtCommunityValue()); + } else { + OriginVn origin_vn(64512, 100 * idx); + spec.origin_vns.push_back(origin_vn.GetExtCommunityValue()); + } + } + OriginVnPath ovnpath(ovnpath_db_, spec); + + for (int idx = 1; idx <= 9; idx++) { + if (idx % 2 == 1) { + OriginVn origin_vn(64513, OriginVn::kMinGlobalId + 100 * idx); + EXPECT_TRUE(ovnpath.Contains(origin_vn.GetExtCommunity())); + } else { + OriginVn origin_vn(64513, OriginVn::kMinGlobalId + 100 * idx); + EXPECT_FALSE(ovnpath.Contains(origin_vn.GetExtCommunity())); + } + } +} + TEST_F(BgpAttrTest, OriginVnPathLocate) { OriginVnPathSpec spec1; for (int idx = 1; idx < 5; idx++) { diff --git a/src/bgp/xmpp_message_builder.cc b/src/bgp/xmpp_message_builder.cc index 14501421124..39f9447c776 100644 --- a/src/bgp/xmpp_message_builder.cc +++ b/src/bgp/xmpp_message_builder.cc @@ -18,7 +18,6 @@ #include "bgp/extended-community/mac_mobility.h" #include "bgp/ermvpn/ermvpn_route.h" #include "bgp/evpn/evpn_route.h" -#include "bgp/origin-vn/origin_vn.h" #include "bgp/routing-instance/routing_instance.h" #include "bgp/security_group/security_group.h" #include "net/community_type.h"