From 231e4de73f46b9c75a07bf0144aafce18aeb3a55 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Mon, 8 May 2023 22:49:10 +0300 Subject: [PATCH 1/2] bgpd: Make sure AIGP attribute is non-transitive The AIGP attribute is an optional, non-transitive BGP path attribute. Signed-off-by: Donatas Abraitis --- bgpd/bgp_attr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 8e66a229c3..3aad9484c9 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -1495,7 +1495,7 @@ const uint8_t attr_flags_values[] = { [BGP_ATTR_PREFIX_SID] = BGP_ATTR_FLAG_OPTIONAL | BGP_ATTR_FLAG_TRANS, [BGP_ATTR_IPV6_EXT_COMMUNITIES] = BGP_ATTR_FLAG_OPTIONAL | BGP_ATTR_FLAG_TRANS, - [BGP_ATTR_AIGP] = BGP_ATTR_FLAG_OPTIONAL | BGP_ATTR_FLAG_TRANS, + [BGP_ATTR_AIGP] = BGP_ATTR_FLAG_OPTIONAL, }; static const size_t attr_flags_values_max = array_size(attr_flags_values) - 1; @@ -4834,7 +4834,7 @@ bgp_size_t bgp_packet_attribute(struct bgp *bgp, struct peer *peer, */ uint8_t attr_len = BGP_AIGP_TLV_METRIC_LEN; - stream_putc(s, BGP_ATTR_FLAG_OPTIONAL | BGP_ATTR_FLAG_TRANS); + stream_putc(s, BGP_ATTR_FLAG_OPTIONAL); stream_putc(s, BGP_ATTR_AIGP); stream_putc(s, attr_len); stream_put_bgp_aigp_tlv_metric(s, bpi); From d539fd9449555fd90873a66b1d2062aa4a5112af Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Mon, 8 May 2023 23:22:26 +0300 Subject: [PATCH 2/2] tests: Adjust AIGP metric numbers for iBGP setup Signed-off-by: Donatas Abraitis --- tests/topotests/bgp_aigp/r2/bgpd.conf | 3 +- tests/topotests/bgp_aigp/r3/bgpd.conf | 3 +- tests/topotests/bgp_aigp/r4/bgpd.conf | 5 ++- tests/topotests/bgp_aigp/r5/bgpd.conf | 5 ++- tests/topotests/bgp_aigp/r6/bgpd.conf | 2 +- tests/topotests/bgp_aigp/r7/bgpd.conf | 2 +- tests/topotests/bgp_aigp/test_bgp_aigp.py | 46 ++++++++++++----------- 7 files changed, 36 insertions(+), 30 deletions(-) diff --git a/tests/topotests/bgp_aigp/r2/bgpd.conf b/tests/topotests/bgp_aigp/r2/bgpd.conf index ae72f215c0..4db4687536 100644 --- a/tests/topotests/bgp_aigp/r2/bgpd.conf +++ b/tests/topotests/bgp_aigp/r2/bgpd.conf @@ -4,7 +4,8 @@ router bgp 65001 neighbor 10.0.0.1 remote-as internal neighbor 10.0.0.1 timers 1 3 neighbor 10.0.0.1 timers connect 1 - neighbor 192.168.24.4 remote-as external + neighbor 10.0.0.1 route-reflector-client + neighbor 192.168.24.4 remote-as internal neighbor 192.168.24.4 timers 1 3 neighbor 192.168.24.4 timers connect 1 neighbor 192.168.24.4 aigp diff --git a/tests/topotests/bgp_aigp/r3/bgpd.conf b/tests/topotests/bgp_aigp/r3/bgpd.conf index 7572e268c5..5ab712eaba 100644 --- a/tests/topotests/bgp_aigp/r3/bgpd.conf +++ b/tests/topotests/bgp_aigp/r3/bgpd.conf @@ -4,7 +4,8 @@ router bgp 65001 neighbor 10.0.0.1 remote-as internal neighbor 10.0.0.1 timers 1 3 neighbor 10.0.0.1 timers connect 1 - neighbor 192.168.35.5 remote-as external + neighbor 10.0.0.1 route-reflector-client + neighbor 192.168.35.5 remote-as internal neighbor 192.168.35.5 timers 1 3 neighbor 192.168.35.5 timers connect 1 neighbor 192.168.35.5 aigp diff --git a/tests/topotests/bgp_aigp/r4/bgpd.conf b/tests/topotests/bgp_aigp/r4/bgpd.conf index d2b96b7b0a..aa88bac913 100644 --- a/tests/topotests/bgp_aigp/r4/bgpd.conf +++ b/tests/topotests/bgp_aigp/r4/bgpd.conf @@ -1,10 +1,11 @@ -router bgp 65002 +router bgp 65001 no bgp ebgp-requires-policy no bgp network import-check - neighbor 192.168.24.2 remote-as external + neighbor 192.168.24.2 remote-as internal neighbor 192.168.24.2 timers 1 3 neighbor 192.168.24.2 timers connect 1 neighbor 192.168.24.2 aigp + neighbor 192.168.24.2 route-reflector-client neighbor 10.0.0.6 remote-as internal neighbor 10.0.0.6 timers 1 3 neighbor 10.0.0.6 timers connect 1 diff --git a/tests/topotests/bgp_aigp/r5/bgpd.conf b/tests/topotests/bgp_aigp/r5/bgpd.conf index 944872289d..4fde262053 100644 --- a/tests/topotests/bgp_aigp/r5/bgpd.conf +++ b/tests/topotests/bgp_aigp/r5/bgpd.conf @@ -1,10 +1,11 @@ -router bgp 65002 +router bgp 65001 no bgp ebgp-requires-policy no bgp network import-check - neighbor 192.168.35.3 remote-as external + neighbor 192.168.35.3 remote-as internal neighbor 192.168.35.3 timers 1 3 neighbor 192.168.35.3 timers connect 1 neighbor 192.168.35.3 aigp + neighbor 192.168.35.3 route-reflector-client neighbor 10.0.0.6 remote-as internal neighbor 10.0.0.6 timers 1 3 neighbor 10.0.0.6 timers connect 1 diff --git a/tests/topotests/bgp_aigp/r6/bgpd.conf b/tests/topotests/bgp_aigp/r6/bgpd.conf index 15d9437a8e..2faae7720c 100644 --- a/tests/topotests/bgp_aigp/r6/bgpd.conf +++ b/tests/topotests/bgp_aigp/r6/bgpd.conf @@ -1,4 +1,4 @@ -router bgp 65002 +router bgp 65001 no bgp ebgp-requires-policy no bgp network import-check neighbor 10.0.0.4 remote-as internal diff --git a/tests/topotests/bgp_aigp/r7/bgpd.conf b/tests/topotests/bgp_aigp/r7/bgpd.conf index 00d85cfadd..639dcfeca8 100644 --- a/tests/topotests/bgp_aigp/r7/bgpd.conf +++ b/tests/topotests/bgp_aigp/r7/bgpd.conf @@ -1,4 +1,4 @@ -router bgp 65002 +router bgp 65001 no bgp ebgp-requires-policy no bgp network import-check neighbor 192.168.67.6 remote-as internal diff --git a/tests/topotests/bgp_aigp/test_bgp_aigp.py b/tests/topotests/bgp_aigp/test_bgp_aigp.py index 9fa80c6da3..ad81b700fb 100644 --- a/tests/topotests/bgp_aigp/test_bgp_aigp.py +++ b/tests/topotests/bgp_aigp/test_bgp_aigp.py @@ -27,7 +27,7 @@ r6 receives those routes with aigp-metric TLV. r2 and r3 receives those routes with aigp-metric TLV increased by 20, and 30 appropriately. -r1 receives routes with aigp-metric TLV 91,101 and 92,102 appropriately. +r1 receives routes with aigp-metric TLV 111,131 and 112,132 appropriately. """ import os @@ -122,16 +122,16 @@ def test_bgp_aigp(): expected = { "paths": [ { - "aigpMetric": 101, - "valid": True, - "bestpath": {"selectionReason": "Router ID"}, - "nexthops": [{"hostname": "r2", "accessible": True}], - }, - { - "aigpMetric": 91, + "aigpMetric": 111, "valid": True, "nexthops": [{"hostname": "r3", "accessible": True}], }, + { + "aigpMetric": 131, + "valid": True, + "bestpath": {"selectionReason": "Neighbor IP"}, + "nexthops": [{"hostname": "r2", "accessible": True}], + }, ] } return topotest.json_cmp(output, expected) @@ -153,26 +153,28 @@ def test_bgp_aigp(): "routes": { "10.0.0.71/32": [ { - "aigpMetric": 101, + "aigpMetric": 111, + "bestpath": {"selectionReason": "AIGP"}, "valid": True, + "nexthops": [{"hostname": "r3", "accessible": True}], }, { - "aigpMetric": 91, + "aigpMetric": 131, "valid": True, - "bestpath": {"selectionReason": "AIGP"}, - "nexthops": [{"hostname": "r3", "accessible": True}], + "nexthops": [{"hostname": "r2", "accessible": True}], }, ], "10.0.0.72/32": [ { - "aigpMetric": 102, + "aigpMetric": 112, + "bestpath": {"selectionReason": "AIGP"}, "valid": True, + "nexthops": [{"hostname": "r3", "accessible": True}], }, { - "aigpMetric": 92, + "aigpMetric": 132, "valid": True, - "bestpath": {"selectionReason": "AIGP"}, - "nexthops": [{"hostname": "r3", "accessible": True}], + "nexthops": [{"hostname": "r2", "accessible": True}], }, ], } @@ -181,7 +183,7 @@ def test_bgp_aigp(): # Initial converge, AIGP is not involved in best-path selection process test_func = functools.partial(_bgp_converge) - _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5) + _, result = topotest.run_and_expect(test_func, None, count=60, wait=1) assert result is None, "can't converge initially" # Enable `bgp bestpath aigp` @@ -195,27 +197,27 @@ def test_bgp_aigp(): # r4, 10.0.0.71/32 with aigp-metric 71 test_func = functools.partial(_bgp_check_aigp_metric, r4, "10.0.0.71/32", 71) - _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5) + _, result = topotest.run_and_expect(test_func, None, count=60, wait=1) assert result is None, "aigp-metric for 10.0.0.71/32 is not 71" # r5, 10.0.0.72/32 with aigp-metric 72 test_func = functools.partial(_bgp_check_aigp_metric, r5, "10.0.0.72/32", 72) - _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5) + _, result = topotest.run_and_expect(test_func, None, count=60, wait=1) assert result is None, "aigp-metric for 10.0.0.72/32 is not 72" # r2, 10.0.0.71/32 with aigp-metric 101 (71 + 30) test_func = functools.partial(_bgp_check_aigp_metric, r2, "10.0.0.71/32", 101) - _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5) + _, result = topotest.run_and_expect(test_func, None, count=60, wait=1) assert result is None, "aigp-metric for 10.0.0.71/32 is not 101" # r3, 10.0.0.72/32 with aigp-metric 92 (72 + 20) test_func = functools.partial(_bgp_check_aigp_metric, r3, "10.0.0.72/32", 92) - _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5) + _, result = topotest.run_and_expect(test_func, None, count=60, wait=1) assert result is None, "aigp-metric for 10.0.0.72/32 is not 92" # r1, check if AIGP is considered in best-path selection (lowest wins) test_func = functools.partial(_bgp_check_aigp_metric_bestpath) - _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5) + _, result = topotest.run_and_expect(test_func, None, count=60, wait=1) assert result is None, "AIGP attribute is not considered in best-path selection"