Merge pull request #17851 from FRRouting/mergify/bp/stable/10.1/pr-17832

bgpd: Aggregate backports (backport #17832)

3 files changed, 26 insertions, 83 deletions

diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 12397e87e3..c61462f2be 100644
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -7648,44 +7648,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,
@@ -7694,14 +7656,14 @@ 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;
 
 	table = bgp->rib[afi][safi];
 
 	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;
@@ -7710,28 +7672,34 @@ static void bgp_aggregate_install(
 	 * If we have paths with different MEDs, then don't install
 	 * (or uninstall) the aggregate route.
 	 */
-	if (aggregate->match_med && aggregate->med_mismatched)
+	if (aggregate->match_med && aggregate->med_mismatched) {
+		aspath_free(aspath);
+		community_free(&community);
+		ecommunity_free(&ecommunity);
+		lcommunity_free(&lcommunity);
 		goto uninstall_aggregate_route;
+	}
 
 	if (aggregate->count > 0) {
 		/*
 		 * If the aggregate information has not changed
 		 * no need to re-install it again.
 		 */
-		if (pi && (!aggregate->rmap.changed &&
-			   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);
+			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);
 			return;
 		}
 
@@ -7743,22 +7711,6 @@ static void bgp_aggregate_install(
 			bgp_process(bgp, dest, pi, afi, safi);
 		}
 
-		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 (BGP_DEBUG(update_groups, UPDATE_GROUPS))
-				zlog_debug("%s: %pFX null attribute", __func__,
-					   p);
-			return;
-		}
 
 		new = info_make(ZEBRA_ROUTE_BGP, BGP_ROUTE_AGGREGATE, 0,
 				bgp->peer_self, attr, dest);
@@ -7769,17 +7721,11 @@ static void bgp_aggregate_install(
 		bgp_process(bgp, dest, new, afi, safi);
 	} 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);
-		}
+			/* Withdraw the aggregate route from routing table. */
+			if (pi) {
+				bgp_path_info_delete(dest, pi);
+				bgp_process(bgp, dest, pi, afi, safi);
+			}
 	}
 
 	bgp_dest_unlock_node(dest);
@@ -8712,7 +8658,6 @@ static int bgp_aggregate_set(struct vty *vty, const char *prefix_str, afi_t afi,
 		aggregate->rmap.name =
 			XSTRDUP(MTYPE_ROUTE_MAP_NAME, rmap);
 		aggregate->rmap.map = route_map_lookup_by_name(rmap);
-		aggregate->rmap.changed = true;
 		route_map_counter_increment(aggregate->rmap.map);
 	}
 
diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h
index efabbc7980..ffae6995fe 100644
--- a/bgpd/bgp_route.h
+++ b/bgpd/bgp_route.h
@@ -432,7 +432,6 @@ struct bgp_aggregate {
 	struct {
 		char *name;
 		struct route_map *map;
-		bool changed;
 	} rmap;
 
 	/* Suppress-count. */
diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c
index aab04cb277..261b7d240c 100644
--- a/bgpd/bgp_routemap.c
+++ b/bgpd/bgp_routemap.c
@@ -4616,7 +4616,6 @@ static void bgp_route_map_process_update(struct bgp *bgp, const char *rmap_name,
 					route_map_counter_increment(map);
 
 				aggregate->rmap.map = map;
-				aggregate->rmap.changed = true;
 
 				matched = true;
 			}