bgpd: support brief json for bgp v4 and v6 neighbors route#232
Open
mwinter-osr wants to merge 3 commits intoopensourcerouting:masterfrom
Open
bgpd: support brief json for bgp v4 and v6 neighbors route#232mwinter-osr wants to merge 3 commits intoopensourcerouting:masterfrom
mwinter-osr wants to merge 3 commits intoopensourcerouting:masterfrom
Conversation
brief-json support is added for show bgp v4 and v6 unicast neighbour routes. Commands supported: show bgp vrf <vrf> ipv4 unicast neighbors <neigh> routes json brief show bgp vrf <vrf> ipv6 unicast neighbors <neigh> routes json brief Signed-off-by: harini <hnattamaisub@nvidia.com>
Signed-off-by: harini <hnattamaisub@nvidia.com>
show bgp vrf <> ipv4 unicast neighbors routes brief json show bgp vrf <>ipv6 unicast neighbors routes brief json Signed-off-by: harini <hnattamaisub@nvidia.com>
Greptile SummaryAdds
|
| Filename | Overview |
|---|---|
| bgpd/bgp_route.c | Adds bool brief to bgp_show_neighbor_route and wires it through to bgp_show/bgp_show_table. CLI DEFPY updated to [json$uj [brief$brief]]. Guard added to reject brief for non-routes show types. Logic is correct and consistent with the existing loc-rib brief implementation. |
| doc/user/bgp.rst | New clicmd entry added for [json [brief]] on the neighbor routes command. Documentation accurately describes the brief-only-with-routes restriction and the compact output format. |
| tests/topotests/bgp_soo/test_bgp_soo.py | Adds four new test functions covering IPv4/IPv6 neighbor routes JSON and brief JSON on pe1 vrf RED and cpe1 default. IPv6 forwarding sysctl is applied after start_router(), creating a potential race; hardcoded totalPaths values (5, 8) are topology-sensitive. |
| tests/topotests/bgp_soo/cpe1/bgpd.conf | Adds IPv6 eBGP neighbor (2001:db8:1::2/pe1) and iBGP neighbor (2001:db8:10::2/cpe2) plus address-family ipv6 unicast with connected redistribution. Correct for the new IPv6 test scenarios. |
| tests/topotests/bgp_soo/pe1/bgpd.conf | Adds IPv6 VPN AF (VPNv6 with pe2), IPv6 neighbor in vrf RED, and address-family ipv6 unicast with VPN export/import. Uses the same label (1111) for both IPv4 and IPv6 VPN exports from vrf RED. |
Sequence Diagram
sequenceDiagram
participant VTY
participant show_ip_bgp_neighbor_routes as DEFPY handler
participant bgp_show_neighbor_route
participant bgp_show
participant bgp_show_table
VTY->>show_ip_bgp_neighbor_routes: show bgp vrf X ipv4 unicast neighbors P routes json brief
show_ip_bgp_neighbor_routes->>show_ip_bgp_neighbor_routes: parse AFI/SAFI/peer (argc-- for json)
show_ip_bgp_neighbor_routes->>show_ip_bgp_neighbor_routes: guard: brief only with routes
show_ip_bgp_neighbor_routes->>bgp_show_neighbor_route: "afi, safi, bgp_show_type_neighbor, use_json=true, brief=true"
bgp_show_neighbor_route->>bgp_show_neighbor_route: SET_FLAG(show_flags, BGP_SHOW_OPT_JSON)
bgp_show_neighbor_route->>bgp_show: "show_flags, brief=true"
bgp_show->>bgp_show_table: "table, show_flags, brief=true"
bgp_show_table-->>VTY: "brief JSON header {"routes": {"
loop per dest (filtered by peer address)
bgp_show_table-->>VTY: ""prefix": { "pathCount":N, "multiPathCount":M, "flags":{...} }"
end
bgp_show_table-->>VTY: footer: totalRoutes, totalPaths, numRoutes
Prompt To Fix All With AI
This is a comment left during a code review.
Path: tests/topotests/bgp_soo/test_bgp_soo.py
Line: 92-93
Comment:
**IPv6 forwarding sysctl applied after `start_router()`**
`sysctl -w net.ipv6.conf.all.forwarding=1` is set after `tgen.start_router()`, meaning FRR daemons start and attempt to bring up IPv6 BGP sessions before the kernel forwarding flag is guaranteed to be set. In a network namespace that defaults to forwarding=0, this creates a race: BGP Open messages can be dropped before zebra has processed `ipv6 forwarding` from the config. Compare the existing pre-start IPv4 forwarding setup at lines 69–70 for pe1/pe2. Move this loop before `tgen.start_router()` to match that pattern.
```suggestion
for _, (_, router) in enumerate(router_list.items(), 1):
router.run("sysctl -w net.ipv6.conf.all.forwarding=1")
tgen.start_router()
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: tests/topotests/bgp_soo/test_bgp_soo.py
Line: 247-251
Comment:
**Hard-coded `totalPaths` assertion is fragile**
`"totalPaths": 8` (and `"totalPaths": 5` in the IPv6 test) counts all path entries in the entire BGP table, not just paths from the queried neighbor. This value is tightly coupled to the exact redistribution state and number of multipath slots at convergence. If the topology acquires any additional paths (e.g., an extra redistributed prefix from a loopback, a transient ECMP slot), the assertion will flip to a spurious failure. Either relax this to `"*"` (wildcard in `json_cmp`) or add a comment explaining exactly how each path is counted so reviewers can verify determinism.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: bgpd/bgp_route.c
Line: 17570-17573
Comment:
**`brief` silently ignored for non-unicast SAFIs**
The guard here correctly rejects `brief` for `flap-statistics` and `dampened-routes`, but it does not reject it for non-unicast SAFIs (e.g., `vpn`, `multicast`, `flowspec`). For those, `bgp_show()` dispatches to `bgp_show_table_rd()` or `bgp_evpn_show_all_routes()`, neither of which accepts a `brief` parameter, so the flag is silently dropped and full JSON is returned. The doc string already limits the feature to "unicast neighbor routes," but the runtime path produces no error. Consider adding a SAFI check alongside the `sh_type` check:
```c
if (brief && (sh_type != bgp_show_type_neighbor ||
(safi != SAFI_UNICAST && safi != SAFI_LABELED_UNICAST))) {
vty_out(vty, "%% brief option is only supported with unicast 'routes'\n");
return CMD_WARNING;
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "tests: Enhanced tests to validate new co..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+92
to
+93
| for _, (_, router) in enumerate(router_list.items(), 1): | ||
| router.run("sysctl -w net.ipv6.conf.all.forwarding=1") |
There was a problem hiding this comment.
start_router()
sysctl -w net.ipv6.conf.all.forwarding=1 is set after tgen.start_router(), meaning FRR daemons start and attempt to bring up IPv6 BGP sessions before the kernel forwarding flag is guaranteed to be set. In a network namespace that defaults to forwarding=0, this creates a race: BGP Open messages can be dropped before zebra has processed ipv6 forwarding from the config. Compare the existing pre-start IPv4 forwarding setup at lines 69–70 for pe1/pe2. Move this loop before tgen.start_router() to match that pattern.
Suggested change
| for _, (_, router) in enumerate(router_list.items(), 1): | |
| router.run("sysctl -w net.ipv6.conf.all.forwarding=1") | |
| for _, (_, router) in enumerate(router_list.items(), 1): | |
| router.run("sysctl -w net.ipv6.conf.all.forwarding=1") | |
| tgen.start_router() |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/topotests/bgp_soo/test_bgp_soo.py
Line: 92-93
Comment:
**IPv6 forwarding sysctl applied after `start_router()`**
`sysctl -w net.ipv6.conf.all.forwarding=1` is set after `tgen.start_router()`, meaning FRR daemons start and attempt to bring up IPv6 BGP sessions before the kernel forwarding flag is guaranteed to be set. In a network namespace that defaults to forwarding=0, this creates a race: BGP Open messages can be dropped before zebra has processed `ipv6 forwarding` from the config. Compare the existing pre-start IPv4 forwarding setup at lines 69–70 for pe1/pe2. Move this loop before `tgen.start_router()` to match that pattern.
```suggestion
for _, (_, router) in enumerate(router_list.items(), 1):
router.run("sysctl -w net.ipv6.conf.all.forwarding=1")
tgen.start_router()
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+247
to
+251
| pe1 = tgen.gears["pe1"] | ||
|
|
||
| if tgen.routers_have_failure(): | ||
| pytest.skip(tgen.errors) | ||
|
|
There was a problem hiding this comment.
totalPaths assertion is fragile
"totalPaths": 8 (and "totalPaths": 5 in the IPv6 test) counts all path entries in the entire BGP table, not just paths from the queried neighbor. This value is tightly coupled to the exact redistribution state and number of multipath slots at convergence. If the topology acquires any additional paths (e.g., an extra redistributed prefix from a loopback, a transient ECMP slot), the assertion will flip to a spurious failure. Either relax this to "*" (wildcard in json_cmp) or add a comment explaining exactly how each path is counted so reviewers can verify determinism.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/topotests/bgp_soo/test_bgp_soo.py
Line: 247-251
Comment:
**Hard-coded `totalPaths` assertion is fragile**
`"totalPaths": 8` (and `"totalPaths": 5` in the IPv6 test) counts all path entries in the entire BGP table, not just paths from the queried neighbor. This value is tightly coupled to the exact redistribution state and number of multipath slots at convergence. If the topology acquires any additional paths (e.g., an extra redistributed prefix from a loopback, a transient ECMP slot), the assertion will flip to a spurious failure. Either relax this to `"*"` (wildcard in `json_cmp`) or add a comment explaining exactly how each path is counted so reviewers can verify determinism.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+17570
to
+17573
| if (brief && sh_type != bgp_show_type_neighbor) { | ||
| vty_out(vty, "%% brief option is only supported with 'routes'\n"); | ||
| return CMD_WARNING; | ||
| } |
There was a problem hiding this comment.
brief silently ignored for non-unicast SAFIs
The guard here correctly rejects brief for flap-statistics and dampened-routes, but it does not reject it for non-unicast SAFIs (e.g., vpn, multicast, flowspec). For those, bgp_show() dispatches to bgp_show_table_rd() or bgp_evpn_show_all_routes(), neither of which accepts a brief parameter, so the flag is silently dropped and full JSON is returned. The doc string already limits the feature to "unicast neighbor routes," but the runtime path produces no error. Consider adding a SAFI check alongside the sh_type check:
if (brief && (sh_type != bgp_show_type_neighbor ||
(safi != SAFI_UNICAST && safi != SAFI_LABELED_UNICAST))) {
vty_out(vty, "%% brief option is only supported with unicast 'routes'\n");
return CMD_WARNING;
}Prompt To Fix With AI
This is a comment left during a code review.
Path: bgpd/bgp_route.c
Line: 17570-17573
Comment:
**`brief` silently ignored for non-unicast SAFIs**
The guard here correctly rejects `brief` for `flap-statistics` and `dampened-routes`, but it does not reject it for non-unicast SAFIs (e.g., `vpn`, `multicast`, `flowspec`). For those, `bgp_show()` dispatches to `bgp_show_table_rd()` or `bgp_evpn_show_all_routes()`, neither of which accepts a `brief` parameter, so the flag is silently dropped and full JSON is returned. The doc string already limits the feature to "unicast neighbor routes," but the runtime path produces no error. Consider adding a SAFI check alongside the `sh_type` check:
```c
if (brief && (sh_type != bgp_show_type_neighbor ||
(safi != SAFI_UNICAST && safi != SAFI_LABELED_UNICAST))) {
vty_out(vty, "%% brief option is only supported with unicast 'routes'\n");
return CMD_WARNING;
}
```
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
brief-json support is added for show bgp v4 and v6 unicast neighbour routes.
Commands supported:
show bgp vrf ipv4 unicast neighbors routes brief json
show bgp vrf ipv6 unicast neighbors routes brief json
cpe1# show bgp vrf default ipv6 unicast neighbors 2001:db8:10::2 routes json brief
{
"routes": { "2001:db8:2::/64": {
"pathCount":1
,"multiPathCount":1
,"flags": {"bestPathExists":true}
} ,"2001:db8:10::/64": {
"pathCount":1
,"multiPathCount":0
,"flags": {"bestPathExists":false}
} } , "totalRoutes": 2, "totalPaths": 5,"numRoutes":2
}
cpe1# show bgp vrf default ipv6 unicast neighbors 2001:db8:10::2 routes json
{
"vrfId": 0,
"vrfName": "default",
"tableVersion": 4,
"routerId": "172.16.255.1",
"defaultLocPrf": 100,
"localAS": 65000,
"routes": { "2001:db8:2::/64": [{"valid":true,"bestpath":true,"selectionReason":"First path received","pathFrom":"internal","prefix":"2001:db8:2::","prefixLen":64,"network":"2001:db8:2::/64","version":3,"metric":0,"locPrf":100,"weight":0,"peerId":"2001:db8:10::2","path":"","origin":"incomplete","nexthops":[{"ip":"2001:db8:10::2","hostname":"cpe2","afi":"ipv6","scope":"global","linkLocalOnly":false,"length":32},{"ip":"fe80::2c7d:9eff:fecb:65ab","hostname":"cpe2","afi":"ipv6","scope":"link-local","used":true}]}]
,"2001:db8:10::/64": [{"valid":true,"pathFrom":"internal","prefix":"2001:db8:10::","prefixLen":64,"network":"2001:db8:10::/64","version":2,"metric":0,"locPrf":100,"weight":0,"peerId":"2001:db8:10::2","path":"","origin":"incomplete","nexthops":[{"ip":"2001:db8:10::2","hostname":"cpe2","afi":"ipv6","scope":"global","linkLocalOnly":false,"length":32},{"ip":"fe80::2c7d:9eff:fecb:65ab","hostname":"cpe2","afi":"ipv6","scope":"link-local","used":true}]}]
} , "totalRoutes": 2, "totalPaths": 5,"numRoutes":2
}
cpe1#
pe1# show bgp vrf RED ipv4 unicast neighbors 192.168.1.1 routes json brief
{
"routes": { "10.0.0.0/24": {
"pathCount":1
,"multiPathCount":2
,"flags": {"bestPathExists":true}
} ,"172.16.255.1/32": {
"pathCount":1
,"multiPathCount":2
,"flags": {"bestPathExists":true}
} ,"192.168.1.0/24": {
"pathCount":1
,"multiPathCount":2
,"flags": {"bestPathExists":true}
} ,"192.168.2.0/24": {
"pathCount":1
,"multiPathCount":2
,"flags": {"bestPathExists":true}
} } , "totalRoutes": 4, "totalPaths": 8,"numRoutes":4
}
pe1# show bgp vrf RED ipv4 unicast neighbors 192.168.1.1 routes json
{
"vrfId": 4,
"vrfName": "RED",
"tableVersion": 8,
"routerId": "192.168.1.2",
"defaultLocPrf": 100,
"localAS": 65001,
"routes": { "10.0.0.0/24": [{"valid":true,"bestpath":true,"selectionReason":"Older Path","multipath":true,"pathFrom":"external","prefix":"10.0.0.0","prefixLen":24,"network":"10.0.0.0/24","version":5,"metric":0,"weight":0,"peerId":"192.168.1.1","path":"65000","origin":"incomplete","nexthops":[{"ip":"192.168.1.1","hostname":"cpe1","afi":"ipv4","used":true}]}]
,"172.16.255.1/32": [{"valid":true,"bestpath":true,"selectionReason":"Older Path","multipath":true,"pathFrom":"external","prefix":"172.16.255.1","prefixLen":32,"network":"172.16.255.1/32","version":6,"metric":0,"weight":0,"peerId":"192.168.1.1","path":"65000","origin":"incomplete","nexthops":[{"ip":"192.168.1.1","hostname":"cpe1","afi":"ipv4","used":true}]}]
,"192.168.1.0/24": [{"valid":true,"bestpath":true,"selectionReason":"Older Path","multipath":true,"pathFrom":"external","prefix":"192.168.1.0","prefixLen":24,"network":"192.168.1.0/24","version":7,"metric":0,"weight":0,"peerId":"192.168.1.1","path":"65000","origin":"incomplete","nexthops":[{"ip":"192.168.1.1","hostname":"cpe1","afi":"ipv4","used":true}]}]
,"192.168.2.0/24": [{"valid":true,"bestpath":true,"selectionReason":"Older Path","multipath":true,"pathFrom":"external","prefix":"192.168.2.0","prefixLen":24,"network":"192.168.2.0/24","version":8,"weight":0,"peerId":"192.168.1.1","path":"65000","origin":"incomplete","nexthops":[{"ip":"192.168.1.1","hostname":"cpe1","afi":"ipv4","used":true}]}]
} , "totalRoutes": 4, "totalPaths": 8,"numRoutes":4
}
pe1#
Original PR21414 by hnattamaisub