Merge pull request #10585 from ton31337/feature/advmap_set

bgpd: Allow setting attributes over route-maps for conditional advert…

6 files changed, 57 insertions, 50 deletions

diff --git a/bgpd/bgp_conditional_adv.c b/bgpd/bgp_conditional_adv.c
index c0dd3d6f8..e5a4b0e9f 100644
--- a/bgpd/bgp_conditional_adv.c
+++ b/bgpd/bgp_conditional_adv.c
@@ -82,7 +82,7 @@ static void bgp_conditional_adv_routes(struct peer *peer, afi_t afi,
 	struct peer_af *paf;
 	const struct prefix *dest_p;
 	struct update_subgroup *subgrp;
-	struct attr dummy_attr = {0}, attr = {0};
+	struct attr advmap_attr = {0}, attr = {0};
 	struct bgp_path_info_extra path_extra = {0};
 	route_map_result_t ret;
 
@@ -110,55 +110,53 @@ static void bgp_conditional_adv_routes(struct peer *peer, afi_t afi,
 		assert(dest_p);
 
 		for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next) {
-			dummy_attr = *pi->attr;
+			advmap_attr = *pi->attr;
 
 			/* Fill temp path_info */
 			prep_for_rmap_apply(&path, &path_extra, dest, pi,
-					    pi->peer, &dummy_attr);
+					    pi->peer, &advmap_attr);
 
-			RESET_FLAG(dummy_attr.rmap_change_flags);
+			RESET_FLAG(advmap_attr.rmap_change_flags);
 
 			ret = route_map_apply(rmap, dest_p, &path);
-			bgp_attr_flush(&dummy_attr);
-
-			if (ret != RMAP_PERMITMATCH)
+			if (ret != RMAP_PERMITMATCH ||
+			    !bgp_check_selected(pi, peer, addpath_capable, afi,
+						safi)) {
+				bgp_attr_flush(&advmap_attr);
 				continue;
+			}
 
-			if (bgp_check_selected(pi, peer, addpath_capable, afi,
-					       safi)) {
-				/* Skip route-map checks in
-				 * subgroup_announce_check while executing from
-				 * the conditional advertise scanner process.
-				 * otherwise when route-map is also configured
-				 * on same peer, routes in advertise-map may not
-				 * be advertised as expected.
+			/* Skip route-map checks in
+			 * subgroup_announce_check while executing from
+			 * the conditional advertise scanner process.
+			 * otherwise when route-map is also configured
+			 * on same peer, routes in advertise-map may not
+			 * be advertised as expected.
+			 */
+			if (update_type == ADVERTISE &&
+			    subgroup_announce_check(dest, pi, subgrp, dest_p,
+						    &attr, &advmap_attr)) {
+				bgp_adj_out_set_subgroup(dest, subgrp, &attr,
+							 pi);
+			} else {
+				/* If default originate is enabled for
+				 * the peer, do not send explicit
+				 * withdraw. This will prevent deletion
+				 * of default route advertised through
+				 * default originate.
 				 */
-				if ((update_type == ADVERTISE)
-				    && subgroup_announce_check(dest, pi, subgrp,
-							       dest_p, &attr,
-							       true))
-					bgp_adj_out_set_subgroup(dest, subgrp,
-								 &attr, pi);
-				else {
-					/* If default originate is enabled for
-					 * the peer, do not send explicit
-					 * withdraw. This will prevent deletion
-					 * of default route advertised through
-					 * default originate.
-					 */
-					if (CHECK_FLAG(
-						    peer->af_flags[afi][safi],
-						    PEER_FLAG_DEFAULT_ORIGINATE)
-					    && is_default_prefix(dest_p))
-						break;
-
-					bgp_adj_out_unset_subgroup(
-						dest, subgrp, 1,
-						bgp_addpath_id_for_peer(
-							peer, afi, safi,
-							&pi->tx_addpath));
-				}
+				if (CHECK_FLAG(peer->af_flags[afi][safi],
+					       PEER_FLAG_DEFAULT_ORIGINATE) &&
+				    is_default_prefix(dest_p))
+					break;
+
+				bgp_adj_out_unset_subgroup(
+					dest, subgrp, 1,
+					bgp_addpath_id_for_peer(
+						peer, afi, safi,
+						&pi->tx_addpath));
 			}
+			bgp_attr_flush(&advmap_attr);
 		}
 	}
 	UNSET_FLAG(subgrp->sflags, SUBGRP_STATUS_TABLE_REPARSING);
diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 8311cb207..0238e36cd 100644
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -1836,7 +1836,7 @@ void subgroup_announce_reset_nhop(uint8_t family, struct attr *attr)
 bool subgroup_announce_check(struct bgp_dest *dest, struct bgp_path_info *pi,
 			     struct update_subgroup *subgrp,
 			     const struct prefix *p, struct attr *attr,
-			     bool skip_rmap_check)
+			     struct attr *post_attr)
 {
 	struct bgp_filter *filter;
 	struct peer *from;
@@ -2067,8 +2067,16 @@ bool subgroup_announce_check(struct bgp_dest *dest, struct bgp_path_info *pi,
 		}
 	}
 
-	/* For modify attribute, copy it to temporary structure. */
-	*attr = *piattr;
+	/* For modify attribute, copy it to temporary structure.
+	 * post_attr comes from BGP conditional advertisements, where
+	 * attributes are already processed by advertise-map route-map,
+	 * and this needs to be saved instead of overwriting from the
+	 * path attributes.
+	 */
+	if (post_attr)
+		*attr = *post_attr;
+	else
+		*attr = *piattr;
 
 	/* If local-preference is not set. */
 	if ((peer->sort == BGP_PEER_IBGP || peer->sort == BGP_PEER_CONFED)
@@ -2162,8 +2170,8 @@ bool subgroup_announce_check(struct bgp_dest *dest, struct bgp_path_info *pi,
 	bgp_peer_as_override(bgp, afi, safi, peer, attr);
 
 	/* Route map & unsuppress-map apply. */
-	if (!skip_rmap_check
-	    && (ROUTE_MAP_OUT_NAME(filter) || bgp_path_suppressed(pi))) {
+	if (!post_attr &&
+	    (ROUTE_MAP_OUT_NAME(filter) || bgp_path_suppressed(pi))) {
 		struct bgp_path_info rmap_path = {0};
 		struct bgp_path_info_extra dummy_rmap_path_extra = {0};
 		struct attr dummy_attr = {0};
@@ -2699,7 +2707,7 @@ void subgroup_process_announce_selected(struct update_subgroup *subgrp,
 
 	if (selected) {
 		if (subgroup_announce_check(dest, selected, subgrp, p, &attr,
-					    false)) {
+					    NULL)) {
 			/* Route is selected, if the route is already installed
 			 * in FIB, then it is advertised
 			 */
diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h
index 317995594..a8ec2dc90 100644
--- a/bgpd/bgp_route.h
+++ b/bgpd/bgp_route.h
@@ -785,7 +785,7 @@ extern bool subgroup_announce_check(struct bgp_dest *dest,
 				    struct bgp_path_info *pi,
 				    struct update_subgroup *subgrp,
 				    const struct prefix *p, struct attr *attr,
-				    bool skip_rmap_check);
+				    struct attr *post_attr);
 
 extern void bgp_peer_clear_node_queue_drain_immediate(struct peer *peer);
 extern void bgp_process_queues_drain_immediate(void);
diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c
index dd140e48a..97b32643b 100644
--- a/bgpd/bgp_updgrp_adv.c
+++ b/bgpd/bgp_updgrp_adv.c
@@ -691,7 +691,7 @@ void subgroup_announce_table(struct update_subgroup *subgrp,
 					       safi)) {
 				if (subgroup_announce_check(dest, ri, subgrp,
 							    dest_p, &attr,
-							    false)) {
+							    NULL)) {
 					/* Check if route can be advertised */
 					if (advertise) {
 						if (!bgp_check_withdrawal(bgp,
@@ -910,7 +910,7 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw)
 					if (subgroup_announce_check(
 						    dest, pi, subgrp,
 						    bgp_dest_get_prefix(dest),
-						    &attr, false))
+						    &attr, NULL))
 						bgp_adj_out_set_subgroup(
 							dest, subgrp, &attr,
 							pi);
diff --git a/tests/topotests/bgp_conditional_advertisement/r2/bgpd.conf b/tests/topotests/bgp_conditional_advertisement/r2/bgpd.conf
index 82525fac6..73f837c69 100644
--- a/tests/topotests/bgp_conditional_advertisement/r2/bgpd.conf
+++ b/tests/topotests/bgp_conditional_advertisement/r2/bgpd.conf
@@ -19,6 +19,7 @@ route-map ADV-MAP-1 permit 20
 !
 route-map ADV-MAP-2 permit 10
  match ip address prefix-list IP2
+ set metric 911
 !
 route-map EXIST-MAP permit 10
  match community DEFAULT-ROUTE
diff --git a/tests/topotests/bgp_conditional_advertisement/test_bgp_conditional_advertisement.py b/tests/topotests/bgp_conditional_advertisement/test_bgp_conditional_advertisement.py
index e9b393ba7..6153aee41 100644
--- a/tests/topotests/bgp_conditional_advertisement/test_bgp_conditional_advertisement.py
+++ b/tests/topotests/bgp_conditional_advertisement/test_bgp_conditional_advertisement.py
@@ -334,7 +334,7 @@ def test_bgp_conditional_advertisement():
             "192.0.2.1/32": None,
             "192.0.2.5/32": None,
             "10.139.224.0/20": None,
-            "203.0.113.1/32": [{"protocol": "bgp"}],
+            "203.0.113.1/32": [{"protocol": "bgp", "metric": 911}],
         }
         return topotest.json_cmp(output, expected)