Skip to content

Commit

Permalink
Merge pull request #17234 from FRRouting/mergify/bp/dev/10.2/pr-17199
Browse files Browse the repository at this point in the history
bgpd: compare aigp after local route check in bgp_path_info_cmp() (backport #17199)
  • Loading branch information
ton31337 authored Oct 25, 2024
2 parents 2368b61 + 9be765d commit 6b64228
Show file tree
Hide file tree
Showing 5 changed files with 83 additions and 33 deletions.
55 changes: 26 additions & 29 deletions bgpd/bgp_route.c
Original file line number Diff line number Diff line change
Expand Up @@ -1064,7 +1064,32 @@ int bgp_path_info_cmp(struct bgp *bgp, struct bgp_path_info *new,
}
}

/* Tie-breaker - AIGP (Metric TLV) attribute */
/* 3. Local route check. We prefer:
* - BGP_ROUTE_STATIC
* - BGP_ROUTE_AGGREGATE
* - BGP_ROUTE_REDISTRIBUTE
*/
new_origin = !(new->sub_type == BGP_ROUTE_NORMAL || new->sub_type == BGP_ROUTE_IMPORTED);
exist_origin = !(exist->sub_type == BGP_ROUTE_NORMAL ||
exist->sub_type == BGP_ROUTE_IMPORTED);

if (new_origin && !exist_origin) {
*reason = bgp_path_selection_local_route;
if (debug)
zlog_debug("%s: %s wins over %s due to preferred BGP_ROUTE type", pfx_buf,
new_buf, exist_buf);
return 1;
}

if (!new_origin && exist_origin) {
*reason = bgp_path_selection_local_route;
if (debug)
zlog_debug("%s: %s loses to %s due to preferred BGP_ROUTE type", pfx_buf,
new_buf, exist_buf);
return 0;
}

/* 3.5. Tie-breaker - AIGP (Metric TLV) attribute */
if (CHECK_FLAG(newattr->flag, ATTR_FLAG_BIT(BGP_ATTR_AIGP)) &&
CHECK_FLAG(existattr->flag, ATTR_FLAG_BIT(BGP_ATTR_AIGP)) &&
CHECK_FLAG(bgp->flags, BGP_FLAG_COMPARE_AIGP)) {
Expand Down Expand Up @@ -1094,34 +1119,6 @@ int bgp_path_info_cmp(struct bgp *bgp, struct bgp_path_info *new,
}
}

/* 3. Local route check. We prefer:
* - BGP_ROUTE_STATIC
* - BGP_ROUTE_AGGREGATE
* - BGP_ROUTE_REDISTRIBUTE
*/
new_origin = !(new->sub_type == BGP_ROUTE_NORMAL ||
new->sub_type == BGP_ROUTE_IMPORTED);
exist_origin = !(exist->sub_type == BGP_ROUTE_NORMAL ||
exist->sub_type == BGP_ROUTE_IMPORTED);

if (new_origin && !exist_origin) {
*reason = bgp_path_selection_local_route;
if (debug)
zlog_debug(
"%s: %s wins over %s due to preferred BGP_ROUTE type",
pfx_buf, new_buf, exist_buf);
return 1;
}

if (!new_origin && exist_origin) {
*reason = bgp_path_selection_local_route;
if (debug)
zlog_debug(
"%s: %s loses to %s due to preferred BGP_ROUTE type",
pfx_buf, new_buf, exist_buf);
return 0;
}

/* Here if these are imported routes then get ultimate pi for
* path compare.
*/
Expand Down
8 changes: 4 additions & 4 deletions doc/user/bgp.rst
Original file line number Diff line number Diff line change
Expand Up @@ -160,16 +160,16 @@ bottom until one of the factors can be used.

Prefer higher local preference routes to lower.

3. **Local route check**

Prefer local routes (statics, aggregates, redistributed) to received routes.

If ``bgp bestpath aigp`` is enabled, and both paths that are compared have
AIGP attribute, BGP uses AIGP tie-breaking unless both of the paths have the
AIGP metric attribute. This means that the AIGP attribute is not evaluated
during the best path selection process between two paths when one path does
not have the AIGP attribute.

3. **Local route check**

Prefer local routes (statics, aggregates, redistributed) to received routes.

4. **AS path length check**

Prefer shortest hop-count AS_PATHs.
Expand Down
5 changes: 5 additions & 0 deletions tests/topotests/bgp_aigp_rr/r1/bgpd.conf
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,15 @@ router bgp 65001
neighbor 10.0.0.4 timers connect 1
neighbor 10.0.0.4 route-reflector-client
address-family ipv4
network 10.0.1.2/32 route-map set-aigp
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
!
route-map set-aigp permit 10
set aigp 50
set weight 0
!
4 changes: 4 additions & 0 deletions tests/topotests/bgp_aigp_rr/r2/bgpd.conf
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ router bgp 65001
neighbor 10.0.0.1 timers connect 1
address-family ipv4
redistribute connected route-map connected-to-bgp
network 10.0.1.2/32 route-map set-aigp
neighbor 10.0.0.1 next-hop-self
exit-address-family
!
Expand All @@ -16,3 +17,6 @@ route-map connected-to-bgp permit 10
match ip address prefix-list p22
set aigp 2
!
route-map set-aigp permit 10
set aigp 10
!
44 changes: 44 additions & 0 deletions tests/topotests/bgp_aigp_rr/test_bgp_aigp_rr.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,45 @@ def _bgp_check_aigp_metric(router, prefix, aigp):
expected = {"paths": [{"aigpMetric": aigp, "valid": True}]}
return topotest.json_cmp(output, expected)

def _bgp_check_aigp_bestpath():
output = json.loads(r1.vtysh_cmd("show bgp ipv4 unicast 10.0.1.2/32 json"))
expected = {
"prefix": "10.0.1.2/32",
"paths": [
{
"aigpMetric": 50,
"valid": True,
"sourced": True,
"local": True,
"bestpath": {"overall": True, "selectionReason": "Local Route"},
"nexthops": [
{
"ip": "0.0.0.0",
"hostname": "r1",
"afi": "ipv4",
"metric": 0,
"accessible": True,
"used": True,
}
],
},
{
"aigpMetric": 10,
"valid": True,
"nexthops": [
{
"ip": "10.0.0.2",
"hostname": "r2",
"afi": "ipv4",
"metric": 10,
"accessible": True,
"used": 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)
Expand All @@ -122,6 +161,11 @@ def _bgp_check_aigp_metric(router, prefix, aigp):
_, 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"

# r1, check if the local route is favored over AIGP comparison
test_func = functools.partial(_bgp_check_aigp_bestpath)
_, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
assert result is None, "Local route is not favored over AIGP in best-path selection"


if __name__ == "__main__":
args = ["-s"] + sys.argv[1:]
Expand Down

0 comments on commit 6b64228

Please sign in to comment.