From 53afb27eb892a107c9426a01dbba82bee6fa86a7 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 5b06bc3913..195298c815 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -1467,7 +1467,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; @@ -4740,7 +4740,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 1da2aac4e39fa263a34640a00fb33ae29f0d688b 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 6fda459405..655e9ad184 100644 --- a/tests/topotests/bgp_aigp/test_bgp_aigp.py +++ b/tests/topotests/bgp_aigp/test_bgp_aigp.py @@ -14,7 +14,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 @@ -109,16 +109,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) @@ -141,28 +141,30 @@ def test_bgp_aigp(): "10.0.0.71/32": { "paths": [ { - "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": { "paths": [ { - "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}], }, ], }, @@ -172,7 +174,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` @@ -186,27 +188,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"