bgpd: Allow setting attributes over route-maps for conditional advertisements

Signed-off-by: Donatas Abraitis <donatas.abraitis@gmail.com>

diff --git a/bgpd/bgp_conditional_adv.c b/bgpd/bgp_conditional_adv.c
index c0dd3d6f810b34ebed3fd55b5d9105f714d68794..e5a4b0e9f707ebeb7b5ebad33b3f3d0c75763bc4 100644 (file)
--- 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 66c5d862a6d6235e8eb464a5ce3af3512d2666f7..6a50c538572411bc22819d61780e3c8b76853036 100644 (file)
--- 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};
@@ -2691,7 +2699,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 317995594a15a4467ae44f9ff227fe47258e715b..a8ec2dc907cae45d7995dad56ed87efdf95d34ec 100644 (file)
--- 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 dd140e48afef3d230f8243bbb984601b88e0db18..97b32643b0f8b272abad5ca4ed3009a9cf6b7aab 100644 (file)
--- 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 82525fac64e89bdd3d7b52b2d7103cf1d8ba652a..73f837c69b73818bf5b833d248ccb4cfff3b7a9a 100644 (file)
--- 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 e9b393ba7f8b4b260d3f0e50e9838089159cd2f1..6153aee4181874e14199791a83f5cfa5d0925fc0 100644 (file)
--- 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)