From f9f2d188e3980d2338747739848001a432ad30b2 Mon Sep 17 00:00:00 2001 From: Trey Aspelund Date: Fri, 10 Feb 2023 19:05:27 +0000 Subject: [PATCH 1/2] bgpd: fix 'json detail' output structure "show bgp json detail" was incorrectly displaying header information from route_vty_out_detail_header() as an element of the "paths" array. This corrects the behavior for 'json detail' so that a route holds a dictionary with keys for "paths" and header info, which aligns with how we structure the output for a specific prefix, e.g. "show bgp json". Before: ``` ub20# show ip bgp json detail { "vrfId": 0, "vrfName": "default", "tableVersion": 3, "routerId": "100.64.0.222", "defaultLocPrf": 100, "localAS": 1, "routes": { "2.2.2.2/32": [ { <<<<<<<<< should be outside the array "prefix":"2.2.2.2/32", "version":1, "advertisedTo":{ "192.168.122.12":{ "hostname":"ub20-2" } } }, { "aspath":{ "string":"Local", "segments":[ ], "length":0 }, ``` After: ``` ub20# show ip bgp json detail { "vrfId": 0, "vrfName": "default", "tableVersion": 3, "routerId": "100.64.0.222", "defaultLocPrf": 100, "localAS": 1, "routes": { "2.2.2.2/32": { "prefix": "2.2.2.2/32", "version": "1", "advertisedTo": { "192.168.122.12":{ "hostname":"ub20-2" } } ,"paths": [ { "aspath":{ "string":"Local", "segments":[ ], "length":0 }, ``` Signed-off-by: Trey Aspelund --- bgpd/bgp_evpn_vty.c | 12 +++---- bgpd/bgp_route.c | 84 +++++++++++++++++++++++++++------------------ bgpd/bgp_route.h | 3 +- 3 files changed, 58 insertions(+), 41 deletions(-) diff --git a/bgpd/bgp_evpn_vty.c b/bgpd/bgp_evpn_vty.c index 6b63c6e3aa..79d2bbbe39 100644 --- a/bgpd/bgp_evpn_vty.c +++ b/bgpd/bgp_evpn_vty.c @@ -2485,7 +2485,7 @@ static void evpn_show_route_vni_multicast(struct vty *vty, struct bgp *bgp, /* Prefix and num paths displayed once per prefix. */ route_vty_out_detail_header(vty, bgp, dest, bgp_dest_get_prefix(dest), - NULL, afi, safi, json); + NULL, afi, safi, json, false); /* Display each path for this prefix. */ for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next) { @@ -2587,7 +2587,7 @@ static void evpn_show_route_vni_macip(struct vty *vty, struct bgp *bgp, /* Prefix and num paths displayed once per prefix. */ route_vty_out_detail_header(vty, bgp, dest, (struct prefix *)&p, NULL, - afi, safi, json); + afi, safi, json, false); evp = (const struct prefix_evpn *)bgp_dest_get_prefix(dest); @@ -2722,7 +2722,7 @@ static void evpn_show_route_rd_macip(struct vty *vty, struct bgp *bgp, /* Prefix and num paths displayed once per prefix. */ route_vty_out_detail_header(vty, bgp, dest, bgp_dest_get_prefix(dest), - prd, afi, safi, json); + prd, afi, safi, json, false); if (json) json_paths = json_object_new_array(); @@ -2830,7 +2830,7 @@ static void evpn_show_route_rd(struct vty *vty, struct bgp *bgp, /* Prefix and num paths displayed once per prefix. */ route_vty_out_detail_header( vty, bgp, dest, bgp_dest_get_prefix(dest), prd, - afi, safi, json_prefix); + afi, safi, json_prefix, false); prefix_cnt++; } @@ -2965,7 +2965,7 @@ static void evpn_show_route_rd_all_macip(struct vty *vty, struct bgp *bgp, /* Prefix and num paths displayed once per prefix. */ route_vty_out_detail_header( vty, bgp, dest, p, (struct prefix_rd *)rd_destp, - AFI_L2VPN, SAFI_EVPN, json_prefix); + AFI_L2VPN, SAFI_EVPN, json_prefix, false); /* For EVPN, the prefix is displayed for each path (to * fit in with code that already exists). @@ -3119,7 +3119,7 @@ static void evpn_show_all_routes(struct vty *vty, struct bgp *bgp, int type, vty, bgp, dest, bgp_dest_get_prefix(dest), (struct prefix_rd *)rd_destp, AFI_L2VPN, - SAFI_EVPN, json_prefix); + SAFI_EVPN, json_prefix, false); /* For EVPN, the prefix is displayed for each path (to * fit in diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 23e6195d34..ea139c33ff 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -11219,7 +11219,7 @@ static int bgp_show_table(struct vty *vty, struct bgp *bgp, safi_t safi, for (dest = bgp_table_top(table); dest; dest = bgp_route_next(dest)) { const struct prefix *dest_p = bgp_dest_get_prefix(dest); enum rpki_states rpki_curr_state = RPKI_NOT_BEING_USED; - bool json_detail = json_detail_header; + bool json_detail_header_used = false; pi = bgp_dest_get_bgp_path_info(dest); if (pi == NULL) @@ -11489,27 +11489,7 @@ static int bgp_show_table(struct vty *vty, struct bgp *bgp, safi_t safi, : BGP_SHOW_HEADER)); header = false; - } else if (json_detail && json_paths != NULL) { - const struct prefix_rd *prd; - json_object *jtemp; - - /* Use common detail header, for most types; - * need a json 'object'. - */ - - jtemp = json_object_new_object(); - prd = bgp_rd_from_dest(dest, safi); - - route_vty_out_detail_header( - vty, bgp, dest, - bgp_dest_get_prefix(dest), prd, - table->afi, safi, jtemp); - - json_object_array_add(json_paths, jtemp); - - json_detail = false; } - if (rd != NULL && !display && !output_count) { if (!use_json) vty_out(vty, @@ -11540,7 +11520,7 @@ static int bgp_show_table(struct vty *vty, struct bgp *bgp, safi_t safi, bgp_dest_get_prefix( dest), prd, table->afi, safi, - NULL); + NULL, false); route_vty_out_detail( vty, bgp, dest, dest_p, pi, @@ -11586,6 +11566,23 @@ static int bgp_show_table(struct vty *vty, struct bgp *bgp, safi_t safi, else vty_out(vty, ",\"%pFX\": ", dest_p); } + + if (json_detail_header && json_paths != NULL) { + const struct prefix_rd *prd; + + vty_out(vty, "{\n"); + + prd = bgp_rd_from_dest(dest, safi); + + route_vty_out_detail_header( + vty, bgp, dest, + bgp_dest_get_prefix(dest), prd, + table->afi, safi, json_paths, true); + + vty_out(vty, "\"paths\": "); + json_detail_header_used = true; + } + /* * We are using no_pretty here because under * extremely high settings( say lots and lots of @@ -11596,6 +11593,10 @@ static int bgp_show_table(struct vty *vty, struct bgp *bgp, safi_t safi, * routers out there */ vty_json_no_pretty(vty, json_paths); + + if (json_detail_header_used) + vty_out(vty, "} "); + json_paths = NULL; first = 0; } else @@ -11780,7 +11781,8 @@ static void bgp_show_all_instances_routes_vty(struct vty *vty, afi_t afi, void route_vty_out_detail_header(struct vty *vty, struct bgp *bgp, struct bgp_dest *dest, const struct prefix *p, const struct prefix_rd *prd, afi_t afi, - safi_t safi, json_object *json) + safi_t safi, json_object *json, + bool incremental_print) { struct bgp_path_info *pi; struct peer *peer; @@ -11839,16 +11841,27 @@ void route_vty_out_detail_header(struct vty *vty, struct bgp *bgp, dest->version); } else { - json_object_string_addf(json, "prefix", "%pFX", p); - json_object_int_add(json, "version", dest->version); - + if (incremental_print) { + vty_out(vty, "\"prefix\": \"%pFX\",\n", p); + vty_out(vty, "\"version\": \"%" PRIu64 "\",\n", + dest->version); + } else { + json_object_string_addf(json, "prefix", "%pFX", + p); + json_object_int_add(json, "version", + dest->version); + } } } if (has_valid_label) { - if (json) - json_object_int_add(json, "localLabel", label); - else + if (json) { + if (incremental_print) + vty_out(vty, "\"localLabel\": \"%u\",\n", + label); + else + json_object_int_add(json, "localLabel", label); + } else vty_out(vty, "Local label: %d\n", label); } @@ -11972,13 +11985,16 @@ void route_vty_out_detail_header(struct vty *vty, struct bgp *bgp, } } - if (json) { - if (json_adv_to) { + if (json && json_adv_to) { + if (incremental_print) { + vty_out(vty, "\"advertisedTo\": "); + vty_json(vty, json_adv_to); + vty_out(vty, ","); + } else json_object_object_add(json, "advertisedTo", json_adv_to); - } } else { - if (first) + if (!json && first) vty_out(vty, " Not advertised to any peer"); vty_out(vty, "\n"); } @@ -12021,7 +12037,7 @@ static void bgp_show_path_info(const struct prefix_rd *pfx_rd, route_vty_out_detail_header( vty, bgp, bgp_node, bgp_dest_get_prefix(bgp_node), pfx_rd, AFI_IP, - safi, json_header); + safi, json_header, false); header = 0; } (*display)++; diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h index 452282926f..a288192e53 100644 --- a/bgpd/bgp_route.h +++ b/bgpd/bgp_route.h @@ -857,7 +857,8 @@ extern void route_vty_out_detail_header(struct vty *vty, struct bgp *bgp, struct bgp_dest *dest, const struct prefix *p, const struct prefix_rd *prd, afi_t afi, - safi_t safi, json_object *json); + safi_t safi, json_object *json, + bool incremental_print); extern void route_vty_out_detail(struct vty *vty, struct bgp *bgp, struct bgp_dest *bn, const struct prefix *p, struct bgp_path_info *path, afi_t afi, From 04705e4829d5690519781179846bd694ac2120b9 Mon Sep 17 00:00:00 2001 From: Trey Aspelund Date: Mon, 13 Feb 2023 22:14:16 +0000 Subject: [PATCH 2/2] tests: update tests using 'show bgp json detail' There were a few tests using "show bgp ... json detail" that did json comparisons against a predefined json structure. This updates those predefined json structures to match the new format of the output. (new output moves path array under "paths" key and adds header keys) Signed-off-by: Trey Aspelund --- tests/topotests/bgp_aigp/test_bgp_aigp.py | 52 +++++++------- .../test_bgp-community-alias.py | 16 +++-- .../test_bgp_path_attribute_discard.py | 72 ++++++++++--------- ...st_bgp_path_attribute_treat_as_withdraw.py | 38 +++++----- 4 files changed, 100 insertions(+), 78 deletions(-) diff --git a/tests/topotests/bgp_aigp/test_bgp_aigp.py b/tests/topotests/bgp_aigp/test_bgp_aigp.py index 9fa80c6da3..d8c23f9856 100644 --- a/tests/topotests/bgp_aigp/test_bgp_aigp.py +++ b/tests/topotests/bgp_aigp/test_bgp_aigp.py @@ -151,30 +151,34 @@ def test_bgp_aigp(): ) expected = { "routes": { - "10.0.0.71/32": [ - { - "aigpMetric": 101, - "valid": True, - }, - { - "aigpMetric": 91, - "valid": True, - "bestpath": {"selectionReason": "AIGP"}, - "nexthops": [{"hostname": "r3", "accessible": True}], - }, - ], - "10.0.0.72/32": [ - { - "aigpMetric": 102, - "valid": True, - }, - { - "aigpMetric": 92, - "valid": True, - "bestpath": {"selectionReason": "AIGP"}, - "nexthops": [{"hostname": "r3", "accessible": True}], - }, - ], + "10.0.0.71/32": { + "paths": [ + { + "aigpMetric": 101, + "valid": True, + }, + { + "aigpMetric": 91, + "valid": True, + "bestpath": {"selectionReason": "AIGP"}, + "nexthops": [{"hostname": "r3", "accessible": True}], + }, + ], + }, + "10.0.0.72/32": { + "paths": [ + { + "aigpMetric": 102, + "valid": True, + }, + { + "aigpMetric": 92, + "valid": True, + "bestpath": {"selectionReason": "AIGP"}, + "nexthops": [{"hostname": "r3", "accessible": True}], + }, + ], + }, } } return topotest.json_cmp(output, expected) diff --git a/tests/topotests/bgp_community_alias/test_bgp-community-alias.py b/tests/topotests/bgp_community_alias/test_bgp-community-alias.py index 0b41dc7c6f..8a6e0e4496 100644 --- a/tests/topotests/bgp_community_alias/test_bgp-community-alias.py +++ b/tests/topotests/bgp_community_alias/test_bgp-community-alias.py @@ -118,12 +118,16 @@ def test_bgp_community_alias(): ) expected = { "routes": { - "172.16.16.1/32": [ - { - "community": {"string": "community-r2-1 65001:2"}, - "largeCommunity": {"string": "large-community-r2-1 65001:1:2"}, - } - ] + "172.16.16.1/32": { + "paths": [ + { + "community": {"string": "community-r2-1 65001:2"}, + "largeCommunity": { + "string": "large-community-r2-1 65001:1:2" + }, + } + ] + } } } return topotest.json_cmp(output, expected) diff --git a/tests/topotests/bgp_path_attribute_discard/test_bgp_path_attribute_discard.py b/tests/topotests/bgp_path_attribute_discard/test_bgp_path_attribute_discard.py index 4badf64c37..ff8d8ed265 100644 --- a/tests/topotests/bgp_path_attribute_discard/test_bgp_path_attribute_discard.py +++ b/tests/topotests/bgp_path_attribute_discard/test_bgp_path_attribute_discard.py @@ -81,24 +81,28 @@ def test_bgp_path_attribute_discard(): output = json.loads(r1.vtysh_cmd("show bgp ipv4 unicast json detail")) expected = { "routes": { - "192.168.100.101/32": [ - { - "valid": True, - "atomicAggregate": True, - "community": { - "string": "65001:101", - }, - } - ], - "192.168.100.102/32": [ - { - "valid": True, - "originatorId": "10.0.0.2", - "community": { - "string": "65001:102", - }, - } - ], + "192.168.100.101/32": { + "paths": [ + { + "valid": True, + "atomicAggregate": True, + "community": { + "string": "65001:101", + }, + } + ], + }, + "192.168.100.102/32": { + "paths": [ + { + "valid": True, + "originatorId": "10.0.0.2", + "community": { + "string": "65001:102", + }, + } + ], + }, } } return topotest.json_cmp(output, expected) @@ -120,20 +124,24 @@ def test_bgp_path_attribute_discard(): output = json.loads(r1.vtysh_cmd("show bgp ipv4 unicast json detail")) expected = { "routes": { - "192.168.100.101/32": [ - { - "valid": True, - "atomicAggregate": None, - "community": None, - } - ], - "192.168.100.102/32": [ - { - "valid": True, - "originatorId": None, - "community": None, - } - ], + "192.168.100.101/32": { + "paths": [ + { + "valid": True, + "atomicAggregate": None, + "community": None, + } + ], + }, + "192.168.100.102/32": { + "paths": [ + { + "valid": True, + "originatorId": None, + "community": None, + } + ], + }, } } return topotest.json_cmp(output, expected) diff --git a/tests/topotests/bgp_path_attribute_treat_as_withdraw/test_bgp_path_attribute_treat_as_withdraw.py b/tests/topotests/bgp_path_attribute_treat_as_withdraw/test_bgp_path_attribute_treat_as_withdraw.py index 4aa297511a..a9d678a42d 100644 --- a/tests/topotests/bgp_path_attribute_treat_as_withdraw/test_bgp_path_attribute_treat_as_withdraw.py +++ b/tests/topotests/bgp_path_attribute_treat_as_withdraw/test_bgp_path_attribute_treat_as_withdraw.py @@ -82,17 +82,21 @@ def test_bgp_path_attribute_treat_as_withdraw(): output = json.loads(r2.vtysh_cmd("show bgp ipv4 unicast json detail")) expected = { "routes": { - "10.10.10.10/32": [ - { - "valid": True, - "atomicAggregate": True, - } - ], - "10.10.10.20/32": [ - { - "valid": True, - } - ], + "10.10.10.10/32": { + "paths": [ + { + "valid": True, + "atomicAggregate": True, + } + ], + }, + "10.10.10.20/32": { + "paths": [ + { + "valid": True, + } + ], + }, } } return topotest.json_cmp(output, expected) @@ -115,11 +119,13 @@ def test_bgp_path_attribute_treat_as_withdraw(): expected = { "routes": { "10.10.10.10/32": None, - "10.10.10.20/32": [ - { - "valid": True, - } - ], + "10.10.10.20/32": { + "paths": [ + { + "valid": True, + } + ], + }, } } return topotest.json_cmp(output, expected)