Skip to content

bgpd: Format IGP Router-ID in BGP-LS NLRI based on protocol#21908

Open
cscarpitta wants to merge 6 commits into
FRRouting:masterfrom
cscarpitta:bgp_ls_fix_igp_router_id
Open

bgpd: Format IGP Router-ID in BGP-LS NLRI based on protocol#21908
cscarpitta wants to merge 6 commits into
FRRouting:masterfrom
cscarpitta:bgp_ls_fix_igp_router_id

Conversation

@cscarpitta
Copy link
Copy Markdown
Contributor

RFC 9552 defines the IGP Router-ID as an opaque value whose interpretation depends on the combination of Protocol-ID and field length. The IGP Router-ID is currently always emitted as a hex-dot string, ignoring both fields.

Replace the generic hex-dot loop with protocol-aware formatting:

  • IS-IS (6 or 7 bytes): %pPN
  • OSPF non-pseudonode (4 bytes): %pI4
  • DIRECT/STATIC IPv4 (4 bytes): %pI4
  • DIRECT/STATIC IPv6 (16 bytes): %pI6

Any unrecognised protocol/length combination is treated as an error and logged via flog_err(EC_BGP_LS_PACKET).

Applied to both the JSON display path (node_desc_to_json) and the string format path (format_node_desc).

@frrbot frrbot Bot added bgp tests Topotests, make check, etc labels May 10, 2026
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 10, 2026

Greptile Summary

This PR replaces the generic hex-dot loop for BGP-LS IGP Router-ID display with protocol-aware formatting in both the JSON (node_desc_to_json) and string (format_node_desc) output paths, using %pSY.00/%pPN for IS-IS, %pI4/%pI4:%pI4 for OSPF (including pseudonodes), and %pI4/%pI6 for DIRECT/STATIC, in compliance with RFC 9552.

  • The flat uint8_t igp_router_id[16] field is replaced by a typed union (sysid, pseudo_isis, ospf, pseudo_ospf, ipv4, ipv6, raw) across bgp_ls_nlri.h, and all encode/decode/originate/withdraw call sites are updated to use .raw.
  • Three inline protocol predicates (bgp_ls_protocol_is_isis, bgp_ls_protocol_is_ospf, bgp_ls_protocol_is_direct_static) are added to bgp_ls.h and used throughout, replacing open-coded protocol_id == OSPFV2 || ... checks.
  • Test fixtures are updated to reflect the new IS-IS non-pseudonode representation (0000.0000.0002.00 instead of 0000.0000.0002).

Confidence Score: 5/5

Safe to merge — the union refactor is mechanically consistent across all call sites and all RFC 9552-defined combinations are handled correctly.

All valid protocol/length combinations are handled correctly in both the JSON and format-string paths, including the previously missing OSPF pseudonode case. The VTY display path still omits the 8-byte OSPF pseudonode, but this was a pre-existing gap and not a regression introduced by this PR.

bgpd/bgp_ls_nlri.c — bgp_ls_nlri_display could be brought into parity with the other two output paths by adding the OSPF pseudonode display branch.

Important Files Changed

Filename Overview
bgpd/bgp_ls.c Replaced generic hex-dot loop in node_desc_to_json and format_node_desc with protocol+length-aware switch; both paths now correctly handle IS-IS (6/7 byte), OSPF (4/8 byte), DIRECT/STATIC (IPv4/IPv6), falling through to flog_err+ for any unrecognised combination.
bgpd/bgp_ls_nlri.c Updated bgp_ls_nlri_display and encode/decode helpers to use the new igp_router_id union. The display function dispatches on length only and still has no branch for the 8-byte OSPF pseudonode case, leaving that ID silently unprinted while the parallel JSON and format paths handle it correctly.
bgpd/bgp_ls_nlri.h Replaced the raw uint8_t[16] array with a named union exposing typed members for type-safe access throughout the codebase.
bgpd/bgp_ls.h Added three inline protocol predicate helpers to centralise protocol-ID checks.
bgpd/bgp_ls_ted.c Mechanically updated all memcpy igp_router_id call sites to use .raw and replaced open-coded protocol_id checks with bgp_ls_protocol_is_ospf().

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[IGP Router-ID present] --> B{protocol_id + igp_router_id_len}
    B -->|IS-IS len==6| C[pSY.00]
    B -->|IS-IS len==7| D[pPN]
    B -->|OSPF len==4| E[pI4]
    B -->|OSPF len==8| F[pI4:pI4]
    B -->|DIRECT/STATIC len==4| G[pI4]
    B -->|DIRECT/STATIC len==16| H[pI6]
    B -->|unrecognised| I[flog_err]
    C --> J[node_desc_to_json / format_node_desc]
    D --> J
    E --> J
    F --> J
    G --> J
    H --> J
    I --> J
    K[bgp_ls_nlri_display length-only] -->|len==4| L[Router ID IPv4]
    K -->|len==6| M[ISO Node ID pSY.00]
    K -->|len==7| N[ISO Node ID pPN]
    K -->|len==16| O[Router ID IPv6]
    K -->|len==8| P[silent - not displayed]
Loading
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
bgpd/bgp_ls_nlri.c:6271-6284
**OSPF pseudonode silently omitted in `bgp_ls_nlri_display`**

`bgp_ls_nlri_display` dispatches on length only (4 / 6 / 7 / 16 bytes) and has no branch for `BGP_LS_IGP_ROUTER_ID_OSPF_PSEUDO_LEN` (8 bytes). Any OSPF Designated Router NLRI received from a BGP-LS peer will silently produce no IGP Router-ID line in the VTY output, even though both `node_desc_to_json` and `format_node_desc` now correctly format the 8-byte pseudo-OSPF case with `pseudo_ospf.router_id` + `pseudo_ospf.ifaddr`. This creates a display gap between the three output paths. Consider adding the corresponding `else if (... == BGP_LS_IGP_ROUTER_ID_OSPF_PSEUDO_LEN)` branch here (and in the remote-node block below) to keep the three output paths consistent.

Reviews (4): Last reviewed commit: "tests: Apply clang-format to SRv6 BGP-LS..." | Re-trigger Greptile

Comment thread bgpd/bgp_ls.c
Comment thread bgpd/bgp_ls.c
@cscarpitta cscarpitta force-pushed the bgp_ls_fix_igp_router_id branch 4 times, most recently from 3e5dd1e to cbfe1bb Compare May 11, 2026 17:26
@cscarpitta
Copy link
Copy Markdown
Contributor Author

@greptile review

Comment thread bgpd/bgp_ls.c
@cscarpitta cscarpitta force-pushed the bgp_ls_fix_igp_router_id branch from cbfe1bb to 0291852 Compare May 11, 2026 18:15
@cscarpitta
Copy link
Copy Markdown
Contributor Author

@greptile review

RFC 9552 Section 5.2.1.4 defines TLV 515 (IGP Router-ID) as an opaque
variable-length field. The actual layout depends on the originating
protocol and whether the node is a pseudonode:

  - IS-IS non-pseudonode: 6-byte ISO System-ID
  - IS-IS pseudonode:     6-byte System-ID + 1-byte PSN
  - OSPFv2 non-pseudonode: 4-byte Router-ID
  - OSPFv2 pseudonode:     4-byte Router-ID + 4-byte Interface IP
  - Direct/Static:         4-byte IPv4 or 16-byte IPv6 address

The code currently stores this as a plain `uint8_t` array, leaving the
layout implicit and relying on byte-index access or unsafe pointer
casts in the VTY display code.

Improve this by replacing the flat array with a union that names each
protocol-specific layout explicitly, while keeping a `.raw` member for
encode/decode paths that treat the field as opaque bytes.

Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
Add three static inline bool helpers to bgp_ls.h:
  bgp_ls_protocol_is_isis()
  bgp_ls_protocol_is_ospf()
  bgp_ls_protocol_is_direct_static()

Use bgp_ls_protocol_is_ospf() in bgp_ls_ted.c to replace eight
repeated open-coded comparisons against BGP_LS_PROTO_OSPFV2 and
BGP_LS_PROTO_OSPFV3.

Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
node_desc_to_json() and format_node_desc() need to know the protocol
in order to format the IGP Router-ID correctly. Add protocol_id as a
parameter to both functions and update all call sites.

Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
RFC 9552 defines the IGP Router-ID as an opaque value whose
interpretation depends on the combination of Protocol-ID and
field length. The IGP Router-ID is currently always emitted as a
hex-dot string, ignoring both fields.

Replace the generic hex-dot loop with protocol-aware formatting:
  IS-IS (6 or 7 bytes): %pPN
  OSPF non-pseudonode (4 bytes): %pI4
  DIRECT/STATIC IPv4 (4 bytes): %pI4
  DIRECT/STATIC IPv6 (16 bytes): %pI6

Any unrecognised protocol/length combination is treated as an error
and logged via flog_err(EC_BGP_LS_PACKET).

Applied to both the JSON display path (node_desc_to_json) and the
string format path (format_node_desc).

Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
Adjust expected JSON fixtures for BGP-LS topotests so IS-IS node IDs
match the new formatter output:
  0000.0000.000X -> 0000.0000.000X.00

Update both NLRI strings and igpRouterId fields in:
- tests/topotests/bgp_link_state/r2/bgp_ls_nlri.json
- tests/topotests/bgp_link_state/rr/bgp_ls_nlri.json
- tests/topotests/bgp_link_state_srv6/bgp_ls_nlri_srv6.json

Also update hardcoded IS-IS node IDs in
tests/topotests/bgp_link_state/test_bgp_link_state.py.

Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
@cscarpitta cscarpitta force-pushed the bgp_ls_fix_igp_router_id branch from 0291852 to d09f8a9 Compare May 12, 2026 07:11
@cscarpitta
Copy link
Copy Markdown
Contributor Author

@greptile review

@github-actions
Copy link
Copy Markdown

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant