From 5aaca18a3c5da9d3546e110c440f34a54a9e27fb Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Thu, 26 May 2016 09:32:28 -0700 Subject: [PATCH] Do not resolve paths with nexthop same as prefix Change-Id: I10d8f51738eddcd8a62c31afddef19a9f41b1934 Closes-Bug: 1585832 --- src/bgp/routing-instance/path_resolver.cc | 41 ++++++++++----- src/bgp/test/path_resolver_test.cc | 62 +++++++++++++++++++++++ 2 files changed, 90 insertions(+), 13 deletions(-) diff --git a/src/bgp/routing-instance/path_resolver.cc b/src/bgp/routing-instance/path_resolver.cc index c12d17d479f..70b60f7fd0e 100644 --- a/src/bgp/routing-instance/path_resolver.cc +++ b/src/bgp/routing-instance/path_resolver.cc @@ -22,6 +22,27 @@ using std::make_pair; using std::string; using std::vector; +// +// Return true if the prefix for the BgpRoute is the same as given IpAddress. +// +static bool RoutePrefixIsAddress(Address::Family family, const BgpRoute *route, + const IpAddress &address) { + if (family == Address::INET) { + const InetRoute *inet_route = static_cast(route); + if (inet_route->GetPrefix().addr() == address.to_v4() && + inet_route->GetPrefix().prefixlen() == Address::kMaxV4PrefixLen) { + return true; + } + } else if (family == Address::INET6) { + const Inet6Route *inet6_route = static_cast(route); + if (inet6_route->GetPrefix().addr() == address.to_v6() && + inet6_route->GetPrefix().prefixlen() == Address::kMaxV6PrefixLen) { + return true; + } + } + return false; +} + class PathResolver::DeleteActor : public LifetimeActor { public: explicit DeleteActor(PathResolver *resolver) @@ -604,7 +625,12 @@ void PathResolverPartition::StartPathResolution(const BgpPath *path, return; if (table()->IsDeleted() || nh_table->IsDeleted()) return; + + Address::Family family = table()->family(); IpAddress address = path->GetAttr()->nexthop(); + if (table() == nh_table && RoutePrefixIsAddress(family, route, address)) + return; + ResolverNexthop *rnexthop = resolver_->LocateResolverNexthop(address, nh_table); assert(!FindResolverPath(path)); @@ -1053,19 +1079,8 @@ bool ResolverNexthop::Match(BgpServer *server, BgpTable *table, // Ignore if the route doesn't match the address. Address::Family family = table->family(); assert(family == Address::INET || family == Address::INET6); - if (family == Address::INET) { - const InetRoute *inet_route = static_cast(route); - if (inet_route->GetPrefix().addr() != address_.to_v4() || - inet_route->GetPrefix().prefixlen() != Address::kMaxV4PrefixLen) { - return false; - } - } else if (family == Address::INET6) { - const Inet6Route *inet6_route = static_cast(route); - if (inet6_route->GetPrefix().addr() != address_.to_v6() || - inet6_route->GetPrefix().prefixlen() != Address::kMaxV6PrefixLen) { - return false; - } - } + if (!RoutePrefixIsAddress(family, route, address_)) + return false; // Set or remove MatchState as appropriate. BgpConditionListener *condition_listener = diff --git a/src/bgp/test/path_resolver_test.cc b/src/bgp/test/path_resolver_test.cc index d731da34123..a9639d3a983 100644 --- a/src/bgp/test/path_resolver_test.cc +++ b/src/bgp/test/path_resolver_test.cc @@ -1841,6 +1841,68 @@ TYPED_TEST(PathResolverTest, SinglePrefixWithMultipathAndEcmp2) { this->DeleteBgpPath(bgp_peer2, "blue", this->BuildPrefix(1)); } +// +// Do not resolve BGP path with prefix which is same as nexthop. +// Add XMPP path before BGP path. +// +TYPED_TEST(PathResolverTest, Recursion1) { + if (this->GetFamily() == Address::INET && !nexthop_family_is_inet) + return; + if (this->GetFamily() == Address::INET6 && nexthop_family_is_inet) + return; + + PeerMock *bgp_peer1 = this->bgp_peer1_; + PeerMock *xmpp_peer1 = this->xmpp_peer1_; + + this->AddXmppPath(xmpp_peer1, "blue", + this->BuildPrefix(bgp_peer1->ToString(), 32), + this->BuildNextHopAddress("172.16.1.1"), 10000); + this->AddBgpPath(bgp_peer1, "blue", + this->BuildPrefix(bgp_peer1->ToString(), 32), + this->BuildHostAddress(bgp_peer1->ToString())); + + task_util::WaitForIdle(); + this->VerifyPathNoExists("blue", + this->BuildPrefix(bgp_peer1->ToString(), 32), + this->BuildNextHopAddress("172.16.1.1")); + + this->DeleteBgpPath(bgp_peer1, "blue", + this->BuildPrefix(bgp_peer1->ToString(), 32)); + this->DeleteXmppPath(xmpp_peer1, "blue", + this->BuildPrefix(bgp_peer1->ToString(), 32)); +} + +// +// Do not resolve BGP path with prefix which is same as nexthop. +// Add BGP path before XMPP path. +// +TYPED_TEST(PathResolverTest, Recursion2) { + if (this->GetFamily() == Address::INET && !nexthop_family_is_inet) + return; + if (this->GetFamily() == Address::INET6 && nexthop_family_is_inet) + return; + + PeerMock *bgp_peer1 = this->bgp_peer1_; + PeerMock *xmpp_peer1 = this->xmpp_peer1_; + + this->AddBgpPath(bgp_peer1, "blue", + this->BuildPrefix(bgp_peer1->ToString(), 32), + this->BuildHostAddress(bgp_peer1->ToString())); + this->AddXmppPath(xmpp_peer1, "blue", + this->BuildPrefix(bgp_peer1->ToString(), 32), + this->BuildNextHopAddress("172.16.1.1"), 10000); + + task_util::WaitForIdle(); + this->VerifyPathNoExists("blue", + this->BuildPrefix(bgp_peer1->ToString(), 32), + this->BuildNextHopAddress("172.16.1.1")); + + this->DeleteXmppPath(xmpp_peer1, "blue", + this->BuildPrefix(bgp_peer1->ToString(), 32)); + this->DeleteBgpPath(bgp_peer1, "blue", + this->BuildPrefix(bgp_peer1->ToString(), 32)); +} + // // BGP has multiple prefixes, each with the same nexthop. //