bgpd: apply route-map for aggregate before attribute comparison

Currently when re-evaluating an aggregate route, the full attribute of
the aggregate route is not compared with the existing one in the BGP
table. That can result in unnecessary churns (un-install and then
install) of the aggregate route when a more specific route is added or
deleted, or when the route-map for the aggregate changes. The churn
would impact route installation and route advertisement.

The fix is to apply the route-map for the aggregate first and then
compare the attribute.

Here is an example of the churn:

debug bgp aggregate prefix 5.5.5.0/24
!
route-map set-comm permit 10
 set community 65004:200
!
router bgp 65001
 address-family ipv4 unicast
  redistribute static
  aggregate-address 5.5.5.0/24 route-map set-comm
!

Step 1:
  ip route 5.5.5.1/32 Null0

Jan  8 10:28:49 enke-vm1 bgpd[285786]: [J7PXJ-A7YA2] bgp_aggregate_install: aggregate 5.5.5.0/24, count 1
Jan  8 10:28:49 enke-vm1 bgpd[285786]: [Y444T-HEVNG]   aggregate 5.5.5.0/24: installed

Step 2:
  ip route 5.5.5.2/32 Null0

Jan  8 10:29:03 enke-vm1 bgpd[285786]: [J7PXJ-A7YA2] bgp_aggregate_install: aggregate 5.5.5.0/24, count 2
Jan  8 10:29:03 enke-vm1 bgpd[285786]: [S2EH5-EQSX6]   aggregate 5.5.5.0/24: existing, removed
Jan  8 10:29:03 enke-vm1 bgpd[285786]: [Y444T-HEVNG]   aggregate 5.5.5.0/24: installed
---

Signed-off-by: Enke Chen <enchen@paloaltonetworks.com>

diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 260bcfaa3ac88029acdf544d359302fc100e4947..beb5f40b8b9189c30755eb29bb9377614ad78c47 100644 (file)
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -7934,44 +7934,6 @@ static bool aggr_unsuppress_path(struct bgp_aggregate *aggregate,
        return false;
 }
 
-static bool bgp_aggregate_info_same(struct bgp_path_info *pi, uint8_t origin,
-                                   struct aspath *aspath,
-                                   struct community *comm,
-                                   struct ecommunity *ecomm,
-                                   struct lcommunity *lcomm)
-{
-       static struct aspath *ae = NULL;
-       enum asnotation_mode asnotation;
-
-       asnotation = bgp_get_asnotation(NULL);
-
-       if (!aspath)
-               ae = aspath_empty(asnotation);
-
-       if (!pi)
-               return false;
-
-       if (origin != pi->attr->origin)
-               return false;
-
-       if (!aspath_cmp(pi->attr->aspath, (aspath) ? aspath : ae))
-               return false;
-
-       if (!community_cmp(bgp_attr_get_community(pi->attr), comm))
-               return false;
-
-       if (!ecommunity_cmp(bgp_attr_get_ecommunity(pi->attr), ecomm))
-               return false;
-
-       if (!lcommunity_cmp(bgp_attr_get_lcommunity(pi->attr), lcomm))
-               return false;
-
-       if (!CHECK_FLAG(pi->flags, BGP_PATH_VALID))
-               return false;
-
-       return true;
-}
-
 static void bgp_aggregate_install(
        struct bgp *bgp, afi_t afi, safi_t safi, const struct prefix *p,
        uint8_t origin, struct aspath *aspath, struct community *community,
@@ -7980,7 +7942,7 @@ static void bgp_aggregate_install(
 {
        struct bgp_dest *dest;
        struct bgp_table *table;
-       struct bgp_path_info *pi, *orig, *new;
+       struct bgp_path_info *pi, *new;
        struct attr *attr;
        bool debug = bgp_debug_aggregate(p);
 
@@ -7991,7 +7953,7 @@ static void bgp_aggregate_install(
 
        dest = bgp_node_get(table, p);
 
-       for (orig = pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next)
+       for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next)
                if (pi->peer == bgp->peer_self && pi->type == ZEBRA_ROUTE_BGP
                    && pi->sub_type == BGP_ROUTE_AGGREGATE)
                        break;
@@ -8008,18 +7970,23 @@ static void bgp_aggregate_install(
                 * If the aggregate information has not changed
                 * no need to re-install it again.
                 */
-               if (pi && bgp_aggregate_info_same(pi, origin, aspath, community,
-                                                 ecommunity, lcommunity)) {
+               attr = bgp_attr_aggregate_intern(bgp, origin, aspath, community, ecommunity,
+                                                lcommunity, aggregate, atomic_aggregate, p);
+               if (!attr) {
+                       aspath_free(aspath);
+                       community_free(&community);
+                       ecommunity_free(&ecommunity);
+                       lcommunity_free(&lcommunity);
                        bgp_dest_unlock_node(dest);
+                       bgp_aggregate_delete(bgp, p, afi, safi, aggregate);
+                       if (debug)
+                               zlog_debug("%s: %pFX null attribute", __func__, p);
+                       return;
+               }
 
-                       if (aspath)
-                               aspath_free(aspath);
-                       if (community)
-                               community_free(&community);
-                       if (ecommunity)
-                               ecommunity_free(&ecommunity);
-                       if (lcommunity)
-                               lcommunity_free(&lcommunity);
+               if (pi && CHECK_FLAG(pi->flags, BGP_PATH_VALID) && attrhash_cmp(pi->attr, attr)) {
+                       bgp_attr_unintern(&attr);
+                       bgp_dest_unlock_node(dest);
                        if (debug)
                                zlog_debug("  aggregate %pFX: duplicate", p);
                        return;
@@ -8035,21 +8002,6 @@ static void bgp_aggregate_install(
                                zlog_debug("  aggregate %pFX: existing, removed", p);
                }
 
-               attr = bgp_attr_aggregate_intern(
-                       bgp, origin, aspath, community, ecommunity, lcommunity,
-                       aggregate, atomic_aggregate, p);
-
-               if (!attr) {
-                       aspath_free(aspath);
-                       community_free(&community);
-                       ecommunity_free(&ecommunity);
-                       lcommunity_free(&lcommunity);
-                       bgp_dest_unlock_node(dest);
-                       bgp_aggregate_delete(bgp, p, afi, safi, aggregate);
-                       if (debug)
-                               zlog_debug("%s: %pFX null attribute", __func__, p);
-                       return;
-               }
 
                new = info_make(ZEBRA_ROUTE_BGP, BGP_ROUTE_AGGREGATE, 0,
                                bgp->peer_self, attr, dest);
@@ -8062,19 +8014,13 @@ static void bgp_aggregate_install(
                        zlog_debug("  aggregate %pFX: installed", p);
        } else {
        uninstall_aggregate_route:
-               for (pi = orig; pi; pi = pi->next)
-                       if (pi->peer == bgp->peer_self
-                           && pi->type == ZEBRA_ROUTE_BGP
-                           && pi->sub_type == BGP_ROUTE_AGGREGATE)
-                               break;
-
-               /* Withdraw static BGP route from routing table. */
-               if (pi) {
-                       bgp_path_info_delete(dest, pi);
-                       bgp_process(bgp, dest, pi, afi, safi);
-                       if (debug)
-                               zlog_debug("  aggregate %pFX: uninstall", p);
-               }
+                       /* Withdraw the aggregate route from routing table. */
+                       if (pi) {
+                               bgp_path_info_delete(dest, pi);
+                               bgp_process(bgp, dest, pi, afi, safi);
+                               if (debug)
+                                       zlog_debug("  aggregate %pFX: uninstall", p);
+                       }
        }
 
        bgp_dest_unlock_node(dest);