bgpd: Do not announce routes immediatelly on filter updates

If we set `bgp route-map delay-timer X`, we should ignore starting to announce
routes immediately, and wait for delay timer to expire (or ignore at all if set
to zero).

f1aa49293a4a8302b70989aaa9ceb715385c3a7e broke this because we always sent
route refresh and on receiving BoRR before sending back EoRR.

Let's get fix this.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
(cherry picked from commit 4d8e44c7538c6479ac99ec842bebc42a1e6b2ebc)
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>

diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index 7e7d57e2c0f10403a1dd83e6a6d86b337bd7bea4..eaeed6c6000b863ad4305415a90fe72741c2c0c6 100644 (file)
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -5502,23 +5502,12 @@ static void peer_on_policy_change(struct peer *peer, afi_t afi, safi_t safi,
                        return;
 
                if (CHECK_FLAG(peer->af_flags[afi][safi],
-                              PEER_FLAG_SOFT_RECONFIG)) {
+                              PEER_FLAG_SOFT_RECONFIG))
                        bgp_soft_reconfig_in(peer, afi, safi);
-               } else if (CHECK_FLAG(peer->cap, PEER_CAP_REFRESH_OLD_RCV) ||
-                          CHECK_FLAG(peer->cap, PEER_CAP_REFRESH_NEW_RCV)) {
-                       if (CHECK_FLAG(peer->af_cap[afi][safi],
-                                      PEER_CAP_ORF_PREFIX_SM_ADV) &&
-                           (CHECK_FLAG(peer->af_cap[afi][safi],
-                                       PEER_CAP_ORF_PREFIX_RM_RCV) ||
-                            CHECK_FLAG(peer->af_cap[afi][safi],
-                                       PEER_CAP_ORF_PREFIX_RM_OLD_RCV)))
-                               peer_clear_soft(peer, afi, safi,
-                                               BGP_CLEAR_SOFT_IN_ORF_PREFIX);
-                       else
-                               bgp_route_refresh_send(
-                                       peer, afi, safi, 0, 0, 0,
-                                       BGP_ROUTE_REFRESH_NORMAL);
-               }
+               else if (CHECK_FLAG(peer->cap, PEER_CAP_REFRESH_OLD_RCV) ||
+                        CHECK_FLAG(peer->cap, PEER_CAP_REFRESH_NEW_RCV))
+                       bgp_route_refresh_send(peer, afi, safi, 0, 0, 0,
+                                              BGP_ROUTE_REFRESH_NORMAL);
        }
 }
 
@@ -6772,11 +6761,18 @@ static void peer_prefix_list_update(struct prefix_list *plist)
 
                                /* If we touch prefix-list, we need to process
                                 * new updates. This is important for ORF to
-                                * work correctly as well.
+                                * work correctly.
                                 */
-                               if (peer->afc_nego[afi][safi])
-                                       peer_on_policy_change(peer, afi, safi,
-                                                             0);
+                               if (CHECK_FLAG(peer->af_cap[afi][safi],
+                                              PEER_CAP_ORF_PREFIX_SM_ADV) &&
+                                   (CHECK_FLAG(peer->af_cap[afi][safi],
+                                               PEER_CAP_ORF_PREFIX_RM_RCV) ||
+                                    CHECK_FLAG(
+                                            peer->af_cap[afi][safi],
+                                            PEER_CAP_ORF_PREFIX_RM_OLD_RCV)))
+                                       peer_clear_soft(
+                                               peer, afi, safi,
+                                               BGP_CLEAR_SOFT_IN_ORF_PREFIX);
                        }
                }
                for (ALL_LIST_ELEMENTS(bgp->group, node, nnode, group)) {

diff --git a/tests/topotests/bgp_route_map_delay_timer/__init__.py b/tests/topotests/bgp_route_map_delay_timer/__init__.py
new file mode 100644 (file)
index 0000000..e69de29

diff --git a/tests/topotests/bgp_route_map_delay_timer/r1/bgpd.conf b/tests/topotests/bgp_route_map_delay_timer/r1/bgpd.conf
new file mode 100644 (file)
index 0000000..04dc73a
--- /dev/null
+++ b/tests/topotests/bgp_route_map_delay_timer/r1/bgpd.conf
@@ -0,0 +1,24 @@
+!
+debug bgp updates
+debug bgp neighbor
+!
+bgp route-map delay-timer 5
+!
+router bgp 65001
+ no bgp ebgp-requires-policy
+ no bgp network import-check
+ neighbor 192.168.1.2 remote-as external
+ address-family ipv4 unicast
+  network 10.10.10.1/32
+  network 10.10.10.2/32
+  network 10.10.10.3/32
+  aggregate-address 10.10.10.0/24 summary-only
+  neighbor 192.168.1.2 unsuppress-map r2
+ exit-address-family
+!
+ip prefix-list r1 seq 5 permit 10.10.10.1/32
+ip prefix-list r1 seq 10 permit 10.10.10.2/32
+!
+route-map r2 permit 10
+ match ip address prefix-list r1
+exit

diff --git a/tests/topotests/bgp_route_map_delay_timer/r1/zebra.conf b/tests/topotests/bgp_route_map_delay_timer/r1/zebra.conf
new file mode 100644 (file)
index 0000000..b29940f
--- /dev/null
+++ b/tests/topotests/bgp_route_map_delay_timer/r1/zebra.conf
@@ -0,0 +1,4 @@
+!
+int r1-eth0
+ ip address 192.168.1.1/24
+!

diff --git a/tests/topotests/bgp_route_map_delay_timer/r2/bgpd.conf b/tests/topotests/bgp_route_map_delay_timer/r2/bgpd.conf
new file mode 100644 (file)
index 0000000..36653e6
--- /dev/null
+++ b/tests/topotests/bgp_route_map_delay_timer/r2/bgpd.conf
@@ -0,0 +1,4 @@
+router bgp 65002
+ no bgp ebgp-requires-policy
+ neighbor 192.168.1.1 remote-as external
+!

diff --git a/tests/topotests/bgp_route_map_delay_timer/r2/zebra.conf b/tests/topotests/bgp_route_map_delay_timer/r2/zebra.conf
new file mode 100644 (file)
index 0000000..cffe827
--- /dev/null
+++ b/tests/topotests/bgp_route_map_delay_timer/r2/zebra.conf
@@ -0,0 +1,4 @@
+!
+int r2-eth0
+ ip address 192.168.1.2/24
+!

diff --git a/tests/topotests/bgp_route_map_delay_timer/test_bgp_route_map_delay_timer.py b/tests/topotests/bgp_route_map_delay_timer/test_bgp_route_map_delay_timer.py
new file mode 100644 (file)
index 0000000..15a077d
--- /dev/null
+++ b/tests/topotests/bgp_route_map_delay_timer/test_bgp_route_map_delay_timer.py
@@ -0,0 +1,120 @@
+#!/usr/bin/env python
+# SPDX-License-Identifier: ISC
+
+# Copyright (c) 2023 by
+# Donatas Abraitis <donatas@opensourcerouting.org>
+#
+
+"""
+
+"""
+
+import os
+import sys
+import json
+import pytest
+import functools
+
+pytestmark = pytest.mark.bgpd
+
+CWD = os.path.dirname(os.path.realpath(__file__))
+sys.path.append(os.path.join(CWD, "../"))
+
+# pylint: disable=C0413
+from lib import topotest
+from lib.topogen import Topogen, TopoRouter, get_topogen
+
+pytestmark = [pytest.mark.bgpd]
+
+
+def setup_module(mod):
+    topodef = {"s1": ("r1", "r2")}
+    tgen = Topogen(topodef, mod.__name__)
+    tgen.start_topology()
+
+    router_list = tgen.routers()
+
+    for i, (rname, router) in enumerate(router_list.items(), 1):
+        router.load_config(
+            TopoRouter.RD_ZEBRA, os.path.join(CWD, "{}/zebra.conf".format(rname))
+        )
+        router.load_config(
+            TopoRouter.RD_BGP, os.path.join(CWD, "{}/bgpd.conf".format(rname))
+        )
+
+    tgen.start_router()
+
+
+def teardown_module(mod):
+    tgen = get_topogen()
+    tgen.stop_topology()
+
+
+def test_bgp_route_map_delay_timer():
+    tgen = get_topogen()
+
+    if tgen.routers_have_failure():
+        pytest.skip(tgen.errors)
+
+    r1 = tgen.gears["r1"]
+    r2 = tgen.gears["r2"]
+
+    def _bgp_converge_1():
+        output = json.loads(
+            r1.vtysh_cmd(
+                "show bgp ipv4 unicast neighbor 192.168.1.2 advertised-routes json"
+            )
+        )
+        expected = {
+            "advertisedRoutes": {
+                "10.10.10.0/24": {},
+                "10.10.10.1/32": {},
+                "10.10.10.2/32": {},
+                "10.10.10.3/32": None,
+            }
+        }
+        return topotest.json_cmp(output, expected)
+
+    test_func = functools.partial(_bgp_converge_1)
+    _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
+    assert result is None, "10.10.10.3/32 should not be advertised to r2"
+
+    # Set route-map delay-timer to max value and remove 10.10.10.2/32.
+    # After this, r1 MUST do not announce updates immediately, and wait
+    # 600 seconds before withdrawing 10.10.10.2/32.
+    r2.vtysh_cmd(
+        """
+        configure terminal
+            bgp route-map delay-timer 600
+            no ip prefix-list r1 seq 10 permit 10.10.10.2/32
+    """
+    )
+
+    def _bgp_converge_2():
+        output = json.loads(
+            r1.vtysh_cmd(
+                "show bgp ipv4 unicast neighbor 192.168.1.2 advertised-routes json"
+            )
+        )
+        expected = {
+            "advertisedRoutes": {
+                "10.10.10.0/24": {},
+                "10.10.10.1/32": {},
+                "10.10.10.2/32": None,
+                "10.10.10.3/32": None,
+            }
+        }
+        return topotest.json_cmp(output, expected)
+
+    # We are checking `not None` here to wait count*wait time and if we have different
+    # results than expected, it means good - 10.10.10.2/32 wasn't withdrawn immediately.
+    test_func = functools.partial(_bgp_converge_2)
+    _, result = topotest.run_and_expect(test_func, not None, count=60, wait=0.5)
+    assert (
+        result is not None
+    ), "10.10.10.2/32 advertised, but should not be advertised to r2"
+
+
+if __name__ == "__main__":
+    args = ["-s"] + sys.argv[1:]
+    sys.exit(pytest.main(args))