bgpd: support brief json for bgp v4 and v6 neighbors route#247
Open
suphawitwanl wants to merge 3 commits intomasterfrom
Open
bgpd: support brief json for bgp v4 and v6 neighbors route#247suphawitwanl wants to merge 3 commits intomasterfrom
suphawitwanl wants to merge 3 commits intomasterfrom
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 SummaryThis PR adds
|
| Filename | Overview |
|---|---|
| bgpd/bgp_route.c | Adds brief parameter to bgp_show_neighbor_route/bgp_show call chain and switches DEFUN to DEFPY; single argc-- leaves json token in the AFI/SAFI parser window when both json and brief are present. Also contains the unrelated bgp_fib_flags_info JSON schema regression (flagged in prior review). |
| tests/topotests/bgp_soo/test_bgp_soo.py | Adds four new neighbor-routes JSON/brief tests plus IPv6 topology; bestpath: None in test_bgp_soo_cpe1_neighbor_ipv6_routes_json is a no-op assertion (json_cmp ignores None values). Existing advertised-routes path counts bump due to RT reuse (flagged in prior review). |
| doc/user/bgp.rst | Adds clicmd documentation for the new neighbor routes [json [brief]] command; accurate description of brief-only-with-routes restriction. |
| tests/topotests/bgp_soo/pe1/bgpd.conf | Adds IPv6 VPN AF reusing the same RT (192.168.1.2:2) as the IPv4 VPN config in vrf RED, causing path count inflation in existing tests (flagged in prior review). |
| tests/topotests/bgp_soo/cpe1/bgpd.conf | Adds IPv6 eBGP and iBGP sessions and IPv6 unicast AF to support new neighbor-routes IPv6 tests. |
| tests/topotests/bgp_soo/cpe2/bgpd.conf | Adds IPv6 iBGP session and activates it in IPv6 unicast AF to extend the iBGP mesh for the new tests. |
| tests/topotests/bgp_soo/cpe1/zebra.conf | Adds IPv6 addresses and enables ipv6 forwarding to support the IPv6 session topology. |
| tests/topotests/bgp_soo/cpe2/zebra.conf | Adds IPv6 address to cpe2-eth1 to provide the iBGP link address for the new IPv6 session. |
| tests/topotests/bgp_soo/pe1/zebra.conf | Adds IPv6 address on pe1-eth0 vrf RED and enables ipv6 forwarding, matching the new IPv6 CE neighbor config. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["CLI: show bgp [vrf] [afi] unicast neighbors PEER routes [json [brief]]"] --> B{uj set?}
B -- yes --> C["argc-- (strips last token)"]
B -- no --> D["bgp_vty_find_and_parse_afi_safi_bgp"]
C --> D
D --> E{idx found?}
E -- no --> Z["CMD_WARNING"]
E -- yes --> F["peer_lookup_in_view"]
F --> G{peer found?}
G -- no --> Z
G -- yes --> H{brief && sh_type != neighbor?}
H -- yes --> I["CMD_WARNING: brief only with routes"]
H -- no --> J["bgp_show_neighbor_route(peer, afi, safi, sh_type, uj, brief)"]
J --> K["SET show_flags JSON if uj"]
K --> L["bgp_show(..., show_flags, RPKI_NOT_BEING_USED, brief)"]
L --> M{brief && JSON?}
M -- yes --> N["bgp_show_table: brief per-prefix JSON"]
M -- no --> O["bgp_show_table: full per-path JSON or text"]
style C fill:#ffcccc,stroke:#cc0000
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
tests/topotests/bgp_soo/test_bgp_soo.py:327-334
**`"bestpath": None` skips the non-best-path assertion entirely**
In FRR's `topotest.json_cmp`, a `None` value in the expected dict means "don't care" — the comparison ignores that key completely. So `"bestpath": None` for `2001:db8:10::/64` never verifies whether `bestpath` is absent or false. The PR description explicitly shows this route lacks a best path, so the test should assert that property.
Alternatively, remove the `bestpath` key from the expected dict entirely and rely on the `valid: True` check. Either way, the current `None` is a no-op assertion.
```suggestion
"2001:db8:10::/64": [
{
"valid": True,
"network": "2001:db8:10::/64",
"peerId": "2001:db8:10::2",
}
],
```
### Issue 2 of 2
bgpd/bgp_route.c:17750-17751
**Only one `argc--` when both `json` and `brief` are present**
With the grammar `[json$uj [brief$brief]]`, issuing `routes json brief` leaves `brief` as the last `argv` element. The single `if (uj) argc--` strips `brief`, not `json`. `json` remains in the argument window passed to `bgp_vty_find_and_parse_afi_safi_bgp`. Today that function uses `argv_find` rather than positional indexing so there is no crash, but the window is incorrect and fragile against future changes. The established FRR pattern is to decrement once per consumed trailing keyword.
```suggestion
if (uj)
argc--;
if (brief)
argc--;
```
Reviews (2): 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
+13524
to
+13546
| if (CHECK_FLAG(bgp->flags, BGP_FLAG_SUPPRESS_FIB_PENDING)) { | ||
| if (CHECK_FLAG(dest->flags, BGP_NODE_FIB_INSTALLED)) { | ||
| if (json_flags) | ||
| json_object_boolean_true_add(json_flags, "fibInstalled"); | ||
| else | ||
| vty_out(vty, ",\"fibInstalled\": \"true\" "); | ||
| } /* End of fib install flag */ | ||
|
|
||
| if (CHECK_FLAG(dest->flags, BGP_NODE_FIB_INSTALL_PENDING)) { | ||
| if (json_flags) | ||
| json_object_boolean_true_add(json_flags, "fibWaitForInstall"); | ||
| else | ||
| vty_out(vty, ",\"fibWaitForInstall\": \"true\" "); | ||
| } else { | ||
| if (json_flags) | ||
| json_object_boolean_false_add(json_flags, "fibWaitForInstall"); | ||
| else | ||
| vty_out(vty, ",\"fibWaitForInstall\": \"false\" "); | ||
| } | ||
| if (CHECK_FLAG(dest->flags, BGP_NODE_FIB_INSTALL_PENDING)) { | ||
| if (json_flags) | ||
| json_object_boolean_true_add(json_flags, "fibWaitForInstall"); | ||
| else | ||
| vty_out(vty, ",\"fibWaitForInstall\": \"true\" "); | ||
| } /* End of fib wait for install flag */ | ||
|
|
||
| if (CHECK_FLAG(bgp->flags, BGP_FLAG_SUPPRESS_FIB_PENDING)) { | ||
| if (json_flags) | ||
| json_object_boolean_true_add(json_flags, "fibSuppress"); | ||
| else | ||
| vty_out(vty, ",\"fibSuppress\": \"true\" "); | ||
| if (!CHECK_FLAG(dest->flags, BGP_NODE_FIB_INSTALLED) && | ||
| !CHECK_FLAG(dest->flags, BGP_NODE_FIB_INSTALL_PENDING)) { | ||
| if (!(CHECK_FLAG(dest->flags, BGP_NODE_FIB_INSTALLED)) && | ||
| (!CHECK_FLAG(dest->flags, BGP_NODE_FIB_INSTALL_PENDING))) { | ||
| if (json_flags) | ||
| json_object_boolean_true_add(json_flags, "fibInstallFailed"); | ||
| else | ||
| vty_out(vty, ",\"fibInstallFailed\": \"true\" "); | ||
| } else { | ||
| if (json_flags) | ||
| json_object_boolean_false_add(json_flags, "fibInstallFailed"); | ||
| else | ||
| vty_out(vty, ",\"fibInstallFailed\": \"false\" "); | ||
| } | ||
| } else { | ||
| if (json_flags) | ||
| json_object_boolean_false_add(json_flags, "fibSuppress"); | ||
| else | ||
| vty_out(vty, ",\"fibSuppress\": \"false\" "); | ||
| } | ||
| } /*End of suppress FIB check */ |
There was a problem hiding this comment.
bgp_fib_flags_info is unrelated and regressive
This rewrites the FIB flags output in a way that silently drops fields from the existing schema, breaking any consumer that relies on them:
fibSuppressis removed entirely — it previously appeared in everyshow bgp … jsonflags object wheneversuppress-fib-pendingwas configured.fibInstalledandfibWaitForInstallchange from always-present boolean fields (true/false) to appear-only-when-true, so the absence of a key now carries semantic meaning that never existed before.- The whole FIB block is now silently omitted when
BGP_FLAG_SUPPRESS_FIB_PENDINGis not set, where previously those fields were emitted (with false values).
This change is unrelated to the neighbor routes json brief feature described in the PR. It is a behavioural regression that should be separated, justified separately, and coordinated with downstream tooling (e.g. show bgp ipv4 unicast json consumers that check flags.fibSuppress or flags.fibInstalled).
Prompt To Fix With AI
This is a comment left during a code review.
Path: bgpd/bgp_route.c
Line: 13524-13546
Comment:
**Breaking JSON schema change in `bgp_fib_flags_info` is unrelated and regressive**
This rewrites the FIB flags output in a way that silently drops fields from the existing schema, breaking any consumer that relies on them:
1. `fibSuppress` is removed entirely — it previously appeared in every `show bgp … json` flags object whenever `suppress-fib-pending` was configured.
2. `fibInstalled` and `fibWaitForInstall` change from always-present boolean fields (true/false) to appear-only-when-true, so the absence of a key now carries semantic meaning that never existed before.
3. The whole FIB block is now silently omitted when `BGP_FLAG_SUPPRESS_FIB_PENDING` is not set, where previously those fields were emitted (with false values).
This change is unrelated to the `neighbor routes json brief` feature described in the PR. It is a behavioural regression that should be separated, justified separately, and coordinated with downstream tooling (e.g. `show bgp ipv4 unicast json` consumers that check `flags.fibSuppress` or `flags.fibInstalled`).
How can I resolve this? If you propose a fix, please make it concise.
Author
Comment on lines
17750
to
17751
| if (uj) | ||
| argc--; |
There was a problem hiding this comment.
argc-- when both json and brief are present
With the grammar [json$uj [brief$brief]], issuing routes json brief leaves brief as the last argv element. The single if (uj) argc-- strips brief, not json. json remains in the argument window passed to bgp_vty_find_and_parse_afi_safi_bgp. Today that function uses argv_find rather than positional indexing so there is no crash, but the window is incorrect and fragile against future changes. The established FRR pattern is to decrement once per consumed trailing keyword.
Suggested change
| if (uj) | |
| argc--; | |
| if (uj) | |
| argc--; | |
| if (brief) | |
| argc--; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: bgpd/bgp_route.c
Line: 17750-17751
Comment:
**Only one `argc--` when both `json` and `brief` are present**
With the grammar `[json$uj [brief$brief]]`, issuing `routes json brief` leaves `brief` as the last `argv` element. The single `if (uj) argc--` strips `brief`, not `json`. `json` remains in the argument window passed to `bgp_vty_find_and_parse_afi_safi_bgp`. Today that function uses `argv_find` rather than positional indexing so there is no crash, but the window is incorrect and fragile against future changes. The established FRR pattern is to decrement once per consumed trailing keyword.
```suggestion
if (uj)
argc--;
if (brief)
argc--;
```
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