Merge pull request #17168 from enkechen-panw/aigp-fix3

bgpd: fix AIGP calculation in route advertisement

bgpd/bgp_attr.c

@@ -480,12 +480,11 @@ static bool bgp_attr_aigp_get_tlv_metric(uint8_t *pnt, int length,
 	return false;
 }
 
-static void stream_put_bgp_aigp_tlv_metric(struct stream *s,
+static void stream_put_bgp_aigp_tlv_metric(struct stream *s, uint64_t aigp)
-					   struct bgp_path_info *bpi)
 {
 	stream_putc(s, BGP_AIGP_TLV_METRIC);
 	stream_putw(s, BGP_AIGP_TLV_METRIC_LEN);
-	stream_putq(s, bgp_aigp_metric_total(bpi));
+	stream_putq(s, aigp);
 }
 
 static bool bgp_attr_aigp_valid(uint8_t *pnt, int length)
@@ -4546,14 +4545,11 @@ static void bgp_packet_ecommunity_attribute(struct stream *s, struct peer *peer,
 }
 
 /* Make attribute packet. */
-bgp_size_t bgp_packet_attribute(struct bgp *bgp, struct peer *peer,
+bgp_size_t bgp_packet_attribute(struct bgp *bgp, struct peer *peer, struct stream *s,
-				struct stream *s, struct attr *attr,
+				struct attr *attr, struct bpacket_attr_vec_arr *vecarr,
-				struct bpacket_attr_vec_arr *vecarr,
+				struct prefix *p, afi_t afi, safi_t safi, struct peer *from,
-				struct prefix *p, afi_t afi, safi_t safi,
+				struct prefix_rd *prd, mpls_label_t *label, uint8_t num_labels,
-				struct peer *from, struct prefix_rd *prd,
+				bool addpath_capable, uint32_t addpath_tx_id)
-				mpls_label_t *label, uint8_t num_labels,
-				bool addpath_capable, uint32_t addpath_tx_id,
-				struct bgp_path_info *bpi)
 {
 	size_t cp;
 	size_t aspath_sizep;
@@ -5032,10 +5028,7 @@ bgp_size_t bgp_packet_attribute(struct bgp *bgp, struct peer *peer,
 	}
 
 	/* AIGP */
-	if (bpi && CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_AIGP)) &&
+	if (CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_AIGP)) && AIGP_TRANSMIT_ALLOWED(peer)) {
-	    (CHECK_FLAG(peer->flags, PEER_FLAG_AIGP) ||
-	     peer->sub_sort == BGP_PEER_EBGP_OAD ||
-	     peer->sort != BGP_PEER_EBGP)) {
 		/* At the moment only AIGP Metric TLV exists for AIGP
 		 * attribute. If more comes in, do not forget to update
 		 * attr_len variable to include new ones.
@@ -5045,7 +5038,7 @@ bgp_size_t bgp_packet_attribute(struct bgp *bgp, struct peer *peer,
 		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);
+		stream_put_bgp_aigp_tlv_metric(s, attr->aigp_metric);
 	}
 
 	/* Unknown transit attribute. */
@@ -5314,7 +5307,7 @@ void bgp_dump_routes_attr(struct stream *s, struct bgp_path_info *bpi,
 		stream_putc(s, BGP_ATTR_FLAG_OPTIONAL | BGP_ATTR_FLAG_TRANS);
 		stream_putc(s, BGP_ATTR_AIGP);
 		stream_putc(s, attr_len);
-		stream_put_bgp_aigp_tlv_metric(s, bpi);
+		stream_put_bgp_aigp_tlv_metric(s, attr->aigp_metric);
 	}
 
 	/* Return total size of attribute. */

bgpd/bgp_attr.h

@@ -387,12 +387,12 @@ extern struct attr *bgp_attr_aggregate_intern(
 	struct community *community, struct ecommunity *ecommunity,
 	struct lcommunity *lcommunity, struct bgp_aggregate *aggregate,
 	uint8_t atomic_aggregate, const struct prefix *p);
-extern bgp_size_t bgp_packet_attribute(
+extern bgp_size_t bgp_packet_attribute(struct bgp *bgp, struct peer *peer, struct stream *s,
-	struct bgp *bgp, struct peer *peer, struct stream *s, struct attr *attr,
+				       struct attr *attr, struct bpacket_attr_vec_arr *vecarr,
-	struct bpacket_attr_vec_arr *vecarr, struct prefix *p, afi_t afi,
+				       struct prefix *p, afi_t afi, safi_t safi, struct peer *from,
-	safi_t safi, struct peer *from, struct prefix_rd *prd,
+				       struct prefix_rd *prd, mpls_label_t *label,
-	mpls_label_t *label, uint8_t num_labels, bool addpath_capable,
+				       uint8_t num_labels, bool addpath_capable,
-	uint32_t addpath_tx_id, struct bgp_path_info *bpi);
+				       uint32_t addpath_tx_id);
 extern void bgp_dump_routes_attr(struct stream *s, struct bgp_path_info *bpi,
 				 const struct prefix *p);
 extern bool attrhash_cmp(const void *arg1, const void *arg2);
@@ -585,6 +585,10 @@ static inline void bgp_attr_set_transit(struct attr *attr,
 	attr->transit = transit;
 }
 
+#define AIGP_TRANSMIT_ALLOWED(peer)                                                                \
+	(CHECK_FLAG((peer)->flags, PEER_FLAG_AIGP) || ((peer)->sub_sort == BGP_PEER_EBGP_OAD) ||   \
+	 ((peer)->sort != BGP_PEER_EBGP))
+
 static inline uint64_t bgp_attr_get_aigp_metric(const struct attr *attr)
 {
 	return attr->aigp_metric;

bgpd/bgp_bmp.c

@@ -1016,9 +1016,8 @@ static struct stream *bmp_update(const struct prefix *p, struct prefix_rd *prd,
 	stream_putw(s, 0);
 
 	/* 5: Encode all the attributes, except MP_REACH_NLRI attr. */
-	total_attr_len =
+	total_attr_len = bgp_packet_attribute(NULL, peer, s, attr, &vecarr, NULL, afi, safi, peer,
-		bgp_packet_attribute(NULL, peer, s, attr, &vecarr, NULL, afi,
+					      NULL, NULL, 0, 0, 0);
-				     safi, peer, NULL, NULL, 0, 0, 0, NULL);
 
 	/* space check? */
 

bgpd/bgp_route.c

@@ -2821,6 +2821,21 @@ bool subgroup_announce_check(struct bgp_dest *dest, struct bgp_path_info *pi,
 					false));
 	}
 
+	/*
+	 * Adjust AIGP for propagation when the nexthop is set to ourselves,
+	 * e.g., using "set ip nexthop peer-address" or when advertising to
+	 * EBGP. Note in route reflection the nexthop is usually unmodified
+	 * and the AIGP should not be adjusted in that case.
+	 */
+	if (CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_AIGP)) && AIGP_TRANSMIT_ALLOWED(peer)) {
+		if (nh_reset ||
+		    CHECK_FLAG(attr->rmap_change_flags, BATTR_RMAP_NEXTHOP_PEER_ADDRESS)) {
+			uint64_t aigp = bgp_aigp_metric_total(pi);
+
+			bgp_attr_set_aigp_metric(attr, aigp);
+		}
+	}
+
 	return true;
 }
 

bgpd/bgp_updgrp_packet.c

@@ -738,9 +738,9 @@ struct bpacket *subgroup_update_packet(struct update_subgroup *subgrp)
 
 			/* 5: Encode all the attributes, except MP_REACH_NLRI
 			 * attr. */
-			total_attr_len = bgp_packet_attribute(
+			total_attr_len = bgp_packet_attribute(NULL, peer, s, adv->baa->attr,
-				NULL, peer, s, adv->baa->attr, &vecarr, NULL,
+							      &vecarr, NULL, afi, safi, from, NULL,
-				afi, safi, from, NULL, NULL, 0, 0, 0, path);
+							      NULL, 0, 0, 0);
 
 			space_remaining =
 				STREAM_CONCAT_REMAIN(s, snlri, STREAM_SIZE(s))
@@ -1149,12 +1149,9 @@ void subgroup_default_update_packet(struct update_subgroup *subgrp,
 	/* Make place for total attribute length.  */
 	pos = stream_get_endp(s);
 	stream_putw(s, 0);
-	total_attr_len =
+	total_attr_len = bgp_packet_attribute(NULL, peer, s, attr, &vecarr, &p, afi, safi, from,
-		bgp_packet_attribute(NULL, peer, s, attr, &vecarr, &p, afi,
+					      NULL, &label, num_labels, addpath_capable,
-				     safi, from, NULL, &label, num_labels,
+					      BGP_ADDPATH_TX_ID_FOR_DEFAULT_ORIGINATE);
-				     addpath_capable,
-				     BGP_ADDPATH_TX_ID_FOR_DEFAULT_ORIGINATE,
-				     NULL);
 
 	/* Set Total Path Attribute Length. */
 	stream_putw_at(s, pos, total_attr_len);

tests/topotests/bgp_aigp_rr/r1/bgpd.conf

@@ -0,0 +1,27 @@
+router bgp 65001
+ no bgp ebgp-requires-policy
+ no bgp network import-check
+ bgp route-reflector allow-outbound-policy
+ neighbor 10.0.0.2 remote-as internal
+ neighbor 10.0.0.2 update-source lo
+ neighbor 10.0.0.2 timers 1 3
+ neighbor 10.0.0.2 timers connect 1
+ neighbor 10.0.0.2 route-reflector-client
+ neighbor 10.0.0.3 remote-as internal
+ neighbor 10.0.0.3 update-source lo
+ neighbor 10.0.0.3 timers 1 3
+ neighbor 10.0.0.3 timers connect 1
+ neighbor 10.0.0.3 route-reflector-client
+ neighbor 10.0.0.4 remote-as internal
+ neighbor 10.0.0.4 update-source lo
+ neighbor 10.0.0.4 timers 1 3
+ neighbor 10.0.0.4 timers connect 1
+ neighbor 10.0.0.4 route-reflector-client
+ address-family ipv4
+  neighbor 10.0.0.4 route-map set-nexthop out
+ exit-address-family
+!
+route-map set-nexthop permit 10
+ set ip next-hop peer-address
+exit
+!

tests/topotests/bgp_aigp_rr/r1/ospfd.conf

@@ -0,0 +1,23 @@
+!
+interface lo
+ ip ospf passive
+!
+interface r1-eth0
+ ip ospf dead-interval 4
+ ip ospf hello-interval 1
+ ip ospf cost 10
+!
+interface r1-eth1
+ ip ospf dead-interval 4
+ ip ospf hello-interval 1
+ ip ospf cost 10
+!
+interface r1-eth2
+ ip ospf dead-interval 4
+ ip ospf hello-interval 1
+ ip ospf cost 10
+!
+router ospf
+ router-id 10.0.0.1
+ network 0.0.0.0/0 area 0
+!

tests/topotests/bgp_aigp_rr/r1/zebra.conf

@@ -0,0 +1,13 @@
+!
+interface lo
+ ip address 10.0.0.1/32
+!
+interface r1-eth0
+ ip address 192.168.12.1/24
+!
+interface r1-eth1
+ ip address 192.168.13.1/24
+!
+interface r1-eth2
+ ip address 192.168.14.1/24
+!

tests/topotests/bgp_aigp_rr/r2/bgpd.conf

@@ -0,0 +1,18 @@
+router bgp 65001
+ no bgp ebgp-requires-policy
+ no bgp network import-check
+ neighbor 10.0.0.1 remote-as internal
+ neighbor 10.0.0.1 update-source lo
+ neighbor 10.0.0.1 timers 1 3
+ neighbor 10.0.0.1 timers connect 1
+ address-family ipv4
+  redistribute connected route-map connected-to-bgp
+  neighbor 10.0.0.1 next-hop-self
+ exit-address-family
+!
+ip prefix-list p22 seq 5 permit 10.0.2.2/32
+!
+route-map connected-to-bgp permit 10
+ match ip address prefix-list p22
+ set aigp 2
+!

tests/topotests/bgp_aigp_rr/r2/ospfd.conf

@@ -0,0 +1,18 @@
+!
+interface lo
+ ip ospf passive
+!
+interface r2-eth0
+ ip ospf dead-interval 4
+ ip ospf hello-interval 1
+ ip ospf cost 10
+!
+interface r2-eth1
+ ip ospf dead-interval 4
+ ip ospf hello-interval 1
+ ip ospf cost 10
+!
+router ospf
+ router-id 10.0.0.2
+ network 0.0.0.0/0 area 0
+!

tests/topotests/bgp_aigp_rr/r2/zebra.conf

@@ -0,0 +1,11 @@
+!
+interface lo
+ ip address 10.0.0.2/32
+ ip address 10.0.2.2/32
+!
+interface r2-eth0
+ ip address 192.168.12.2/24
+!
+interface r2-eth1
+ ip address 192.168.23.2/24
+!

tests/topotests/bgp_aigp_rr/r3/bgpd.conf

@@ -0,0 +1,11 @@
+router bgp 65001
+ no bgp ebgp-requires-policy
+ no bgp network import-check
+ neighbor 10.0.0.1 remote-as internal
+ neighbor 10.0.0.1 update-source lo
+ neighbor 10.0.0.1 timers 1 3
+ neighbor 10.0.0.1 timers connect 1
+ address-family ipv4
+  neighbor 10.0.0.1 next-hop-self
+ exit-address-family
+!

tests/topotests/bgp_aigp_rr/r3/ospfd.conf

@@ -0,0 +1,18 @@
+!
+interface lo
+ ip ospf passive
+!
+interface r3-eth0
+ ip ospf dead-interval 4
+ ip ospf hello-interval 1
+ ip ospf cost 10
+!
+interface r3-eth1
+ ip ospf dead-interval 4
+ ip ospf hello-interval 1
+ ip ospf cost 10
+!
+router ospf
+ router-id 10.0.0.3
+ network 0.0.0.0/0 area 0
+!

tests/topotests/bgp_aigp_rr/r3/zebra.conf

@@ -0,0 +1,10 @@
+!
+interface lo
+ ip address 10.0.0.3/32
+!
+interface r3-eth0
+ ip address 192.168.13.3/24
+!
+interface r3-eth1
+ ip address 192.168.23.3/24
+!

tests/topotests/bgp_aigp_rr/r4/bgpd.conf

@@ -0,0 +1,11 @@
+router bgp 65001
+ no bgp ebgp-requires-policy
+ no bgp network import-check
+ neighbor 10.0.0.1 remote-as internal
+ neighbor 10.0.0.1 update-source lo
+ neighbor 10.0.0.1 timers 1 3
+ neighbor 10.0.0.1 timers connect 1
+ address-family ipv4
+  neighbor 10.0.0.1 next-hop-self
+ exit-address-family
+!

tests/topotests/bgp_aigp_rr/r4/ospfd.conf

@@ -0,0 +1,13 @@
+!
+interface lo
+ ip ospf passive
+!
+interface r4-eth0
+ ip ospf dead-interval 4
+ ip ospf hello-interval 1
+ ip ospf cost 10
+!
+router ospf
+ router-id 10.0.0.4
+ network 0.0.0.0/0 area 0
+!

tests/topotests/bgp_aigp_rr/r4/zebra.conf

@@ -0,0 +1,7 @@
+!
+interface lo
+ ip address 10.0.0.4/32
+!
+interface r4-eth0
+ ip address 192.168.14.4/24
+!

tests/topotests/bgp_aigp_rr/test_bgp_aigp_rr.py

@@ -0,0 +1,128 @@
+#!/usr/bin/env python
+# SPDX-License-Identifier: ISC
+
+#
+# Copyright (c) 2024, Palo Alto Networks, Inc.
+# Enke Chen <enchen@paloaltonetworks.com>
+#
+
+"""
+r1, r2, and r3 are directly connectd to each other.
+r4 is only connected to r1 directly.
+
+r1 is the route reflector.
+r1 sets the nexthop to itself when advertising routes to r4.
+
+r2 sources 10.0.2.2/32 with agigp-metric 2.
+
+Results:
+
+r1, r2 and r3 should have aigp-meric 2.
+r4 should have aigp-metric 12, i.e., aigp + nexthop-metric.
+"""
+
+import os
+import sys
+import json
+import pytest
+import functools
+
+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 build_topo(tgen):
+    for routern in range(1, 5):
+        tgen.add_router("r{}".format(routern))
+
+    switch = tgen.add_switch("s1")
+    switch.add_link(tgen.gears["r1"])
+    switch.add_link(tgen.gears["r2"])
+
+    switch = tgen.add_switch("s2")
+    switch.add_link(tgen.gears["r1"])
+    switch.add_link(tgen.gears["r3"])
+
+    switch = tgen.add_switch("s3")
+    switch.add_link(tgen.gears["r1"])
+    switch.add_link(tgen.gears["r4"])
+
+    switch = tgen.add_switch("s4")
+    switch.add_link(tgen.gears["r2"])
+    switch.add_link(tgen.gears["r3"])
+
+
+def setup_module(mod):
+    tgen = Topogen(build_topo, mod.__name__)
+    tgen.start_topology()
+
+    router_list = tgen.routers()
+
+    for _, (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_OSPF, os.path.join(CWD, "{}/ospfd.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_aigp_rr():
+    tgen = get_topogen()
+
+    if tgen.routers_have_failure():
+        pytest.skip(tgen.errors)
+
+    r1 = tgen.gears["r1"]
+    r2 = tgen.gears["r2"]
+    r3 = tgen.gears["r3"]
+    r4 = tgen.gears["r4"]
+
+    def _bgp_check_aigp_metric(router, prefix, aigp):
+        output = json.loads(
+            router.vtysh_cmd("show bgp ipv4 unicast {} json".format(prefix))
+        )
+        expected = {"paths": [{"aigpMetric": aigp, "valid": True}]}
+        return topotest.json_cmp(output, expected)
+
+
+    # r2, 10.0.2.2/32 with aigp-metric 2
+    test_func = functools.partial(_bgp_check_aigp_metric, r2, "10.0.2.2/32", 2)
+    _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
+    assert result is None, "aigp-metric for 10.0.2.2/32 is not 2"
+
+    # r1, 10.0.2.2/32 with aigp-metric 2
+    test_func = functools.partial(_bgp_check_aigp_metric, r1, "10.0.2.2/32", 2)
+    _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
+    assert result is None, "aigp-metric for 10.0.2.2/32 is not 2"
+
+    # r3, 10.0.2.2/32 with aigp-metric 2
+    test_func = functools.partial(_bgp_check_aigp_metric, r3, "10.0.2.2/32", 2)
+    _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
+    assert result is None, "aigp-metric for 10.0.2.2/32 is not 2"
+
+    # r4, 10.0.2.2/32 with aigp-metric 12: aigp + nexthop-metric
+    test_func = functools.partial(_bgp_check_aigp_metric, r4, "10.0.2.2/32", 12)
+    _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
+    assert result is None, "aigp-metric for 10.0.2.2/32 is not 12"
+
+
+if __name__ == "__main__":
+    args = ["-s"] + sys.argv[1:]
+    sys.exit(pytest.main(args))