lib, isisd, bgpd: BGP-LS add several tlvs

[hedrok]

(In draft before I look into Greptile comment about IS-IS Area TLV)

This PR adds support for 2 TLVs:

• SR Capabilities (TLV 1034)
• Node MSD (TLV 266)

Adds IS-IS Area Identifier to lib/link_state, and populates it in isisd.
Updates IS-IS Area Identifier (TLV 1027) (adds console/json output and population from TED)

For each of these TLVs testing is added.

Additionaly two fix/refactoring commits:

• Fix bgp_ls_attr_prefix_sid_len (there was error in case V-Flag and L-Flag are 1)
• Simplify bgp_ls_attr_to_json using json_object_string_addf instead of snprintfrr + json_object_string_add

@cscarpitta I can split this into several PRs if that will be easier to review - all changes are in separate commits, so it will be quick and easy to do.

Thanks.

[greptile-apps]

Greptile Summary

This PR adds BGP-LS support for SR Capabilities (TLV 1034) and Node MSD (TLV 266), populates IS-IS Area Identifier into lib/link_state, improves its JSON/VTY output, and fixes the bgp_ls_attr_prefix_sid_len logic for the V-Flag+L-Flag case. All four issues flagged in previous review threads (wrong ISIS Area bit in JSON, stream_forward_endp vs stream_forward_getp, TLV 1134 typo, and missing bounds check in ls_parse_node) appear to be corrected in this revision.

Confidence Score: 5/5

PR is safe to merge; all four previously identified P1 issues have been addressed in this revision.

All prior P1 findings (wrong ISIS Area bit in JSON, stream_forward_endp bug, TLV 1134 typo, missing bounds check in ls_parse_node) are corrected in the current diff. No new P1 or P0 issues were found. The parsing, encoding, comparison, and serialization logic for the new TLVs is consistent with existing patterns. Test coverage is added for all new functionality.

No files require special attention.

Important Files Changed

Filename	Overview
bgpd/bgp_ls_nlri.c	Adds parse_sr_capabilities and parse_node_msd parsers, encoding for both TLVs, JSON/VTY display, and cmp/hash coverage; also refactors bgp_ls_attr_to_json to use json_object_string_addf. No new P1 issues found.
bgpd/bgp_ls_ted.c	Populates IS-IS Area ID, SR Capabilities, and Node MSD into bgp_ls_attr from ls_node fields; straightforward and correct.
isisd/isis_te.c	Reads IS-IS Area ID from lsp->tlvs->area_addresses (originator's actual addresses) instead of local area config; correctly warns about multi-area-ID case.
lib/link_state.c	Adds LS_NODE_ISIS_AREA_ID support to ls_node_same, ls_parse_node (with bounds check), ls_format_node, and show vertex VTY/JSON functions.
lib/link_state.h	Adds LS_NODE_ISIS_AREA_ID flag and isis_area_id/isis_area_id_len fields to struct ls_node using ISO_ADDR_SIZE (20 bytes).
bgpd/bgp_ls_nlri.h	Adds BGP_LS_IGP_MSD_TYPE_BASE_MPLS constant and bgp_ls_srgb/msd fields to bgp_ls_attr.
tests/topotests/bgp_link_state/rr/bgp_ls_attrs_node4.json	New test fixture verifying srCapabilities, msd, and isisAreaId for r4; values consistent with r4 frr.conf.
tests/topotests/bgp_link_state/test_bgp_link_state.py	Adds node attribute test using the new JSON fixture; follows existing test patterns.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[IS-IS LSP] -->|lsp->tlvs->area_addresses| B[isisd/isis_te.c\nlsp_to_vertex]
    B -->|LS_NODE_ISIS_AREA_ID\nLS_NODE_SR / LS_NODE_MSD| C[lib/link_state\nls_node]

    C -->|bgp_ls_populate_node_attr| D[bgpd/bgp_ls_ted.c]
    D -->|BGP_LS_ATTR_ISIS_AREA_BIT\nBGP_LS_ATTR_SR_CAPABILITIES_BIT\nBGP_LS_ATTR_NODE_MSD_BIT| E[bgp_ls_attr]

    E -->|bgp_ls_encode_attr| F[Wire: TLV 1027\nTLV 1034\nTLV 266]
    F -->|bgp_ls_parse_attr| G[parse_isis_area_id\nparse_sr_capabilities\nparse_node_msd]
    G --> E

    E -->|bgp_ls_attr_to_json| H[JSON Output\nisisAreaId\nsrCapabilities\nmsd]
    E -->|bgp_ls_attr_display| I[VTY Output\nIS-IS Area ID\nSR Capabilities\nMSD]

Loading

_{Reviews (9): Last reviewed commit: "bgpd: BGP-LS: simplify bgp_ls_attr_to_js..." | Re-trigger Greptile}

[cscarpitta]

@greptile review

[riw777]

looks good

[hedrok]

Updated to fix appropriate comment from Greptile about node->isis_area_id_len in lib/link_state.c.

[hedrok]

@greptile review

[hedrok]

P2 findings from Greptile:

(1) the unchecked inner stream_put_tlv_hdr in bgpd/bgp_ls_nlri.c:2171 - this is subTLV unlike other calls to the function and Greptile acknowledges that available stream buffer is checked right before the call correctly. I don't think it should be fixed for better readability, but if any human reviewer thinks it should - no problem from my side.

(2) "only label encoding is valid under the SR Capabilities TLV.", 4 is invalid here, making whole NLRI invalid seems appropriate (RFC: https://datatracker.ietf.org/doc/html/rfc9085#section-2.1.2-6.10.2.4) Again, if any human reviewer thinks just warning + ignoring TLV is better - just write, I'll update the code.

[riw777]

P2 findings from Greptile:

(1) the unchecked inner stream_put_tlv_hdr in bgpd/bgp_ls_nlri.c:2171 - this is subTLV unlike other calls to the function and Greptile acknowledges that available stream buffer is checked right before the call correctly. I don't think it should be fixed for better readability, but if any human reviewer thinks it should - no problem from my side.

(2) "only label encoding is valid under the SR Capabilities TLV.", 4 is invalid here, making whole NLRI invalid seems appropriate (RFC: https://datatracker.ietf.org/doc/html/rfc9085#section-2.1.2-6.10.2.4) Again, if any human reviewer thinks just warning + ignoring TLV is better - just write, I'll update the code.

both of these look fine to me ...

[riw777]

looks good, don't see any reason to address the last few ai notes

[cscarpitta]

Please rebase the PR and address the comments below.

[hedrok]

@cscarpitta Thanks for review, I've updated branch with rebase + your comments fixes only. Here is diff of branch with rebased:

diff --git a/bgpd/bgp_ls_nlri.c b/bgpd/bgp_ls_nlri.c
index 6519b37add..72630c77e0 100644
--- a/bgpd/bgp_ls_nlri.c
+++ b/bgpd/bgp_ls_nlri.c
@@ -2035,23 +2035,25 @@ int bgp_ls_encode_attr(struct stream *s, const struct bgp_ls_attr *attr)
 
 	/* SR Capabilities (TLV 1034) */
 	if (BGP_LS_TLV_CHECK(attr->present_tlvs, BGP_LS_ATTR_SR_CAPABILITIES_BIT)) {
-		if (stream_put_tlv_hdr(s, BGP_LS_ATTR_SR_CAPABILITIES, 12) < 0)
-			return -1;
-		if (STREAM_WRITEABLE(s) < 12)
+		if (STREAM_WRITEABLE(s) < (size_t)BGP_LS_TLV_HDR_SIZE + 12)
 			return -1;
+		stream_putw(s, BGP_LS_ATTR_SR_CAPABILITIES);
+		stream_putw(s, 12);
 		stream_putc(s, attr->srgb.flag);
 		stream_putc(s, 0);
 		stream_put3(s, attr->srgb.range_size);
-		stream_put_tlv_hdr(s, BGP_LS_ATTR_SID_LABEL, 3);
+		/* sub-TLV */
+		stream_putw(s, BGP_LS_ATTR_SID_LABEL);
+		stream_putw(s, 3);
 		stream_put3(s, attr->srgb.lower_bound);
 	}
 
 	/* Node MSD (TLV 266) */
 	if (BGP_LS_TLV_CHECK(attr->present_tlvs, BGP_LS_ATTR_NODE_MSD_BIT)) {
-		if (stream_put_tlv_hdr(s, BGP_LS_ATTR_NODE_MSD, 2) < 0)
-			return -1;
-		if (STREAM_WRITEABLE(s) < 2)
+		if (STREAM_WRITEABLE(s) < (size_t)BGP_LS_TLV_HDR_SIZE + 2)
 			return -1;
+		stream_putw(s, BGP_LS_ATTR_NODE_MSD);
+		stream_putw(s, 2);
 		stream_putc(s, BGP_LS_IGP_MSD_TYPE_BASE_MPLS);
 		stream_putc(s, attr->msd);
 	}
diff --git a/bgpd/bgp_ls_ted.c b/bgpd/bgp_ls_ted.c
index d377b10274..3a32aa7203 100644
--- a/bgpd/bgp_ls_ted.c
+++ b/bgpd/bgp_ls_ted.c
@@ -56,7 +56,7 @@ int bgp_ls_populate_node_attr(struct ls_node *ls_node, struct bgp_ls_attr *attr)
 		attr->isis_area_id_len = ls_node->isis_area_id_len;
 		attr->isis_area_id = XCALLOC(MTYPE_BGP_LS_ATTR, attr->isis_area_id_len);
 		memcpy(attr->isis_area_id, ls_node->isis_area_id, attr->isis_area_id_len);
-		attr->present_tlvs |= (1ULL << BGP_LS_ATTR_ISIS_AREA_BIT);
+		BGP_LS_TLV_SET(attr->present_tlvs, BGP_LS_ATTR_ISIS_AREA_BIT);
 	}
 
 	/* SR Capabilities (TLV 1034) */
@@ -64,13 +64,13 @@ int bgp_ls_populate_node_attr(struct ls_node *ls_node, struct bgp_ls_attr *attr)
 		attr->srgb.flag = ls_node->srgb.flag;
 		attr->srgb.lower_bound = ls_node->srgb.lower_bound;
 		attr->srgb.range_size = ls_node->srgb.range_size;
-		attr->present_tlvs |= (1ULL << BGP_LS_ATTR_SR_CAPABILITIES_BIT);
+		BGP_LS_TLV_SET(attr->present_tlvs, BGP_LS_ATTR_SR_CAPABILITIES_BIT);
 	}
 
 	/* Node MSD (TLV 266) */
 	if (CHECK_FLAG(ls_node->flags, LS_NODE_MSD)) {
 		attr->msd = ls_node->msd;
-		attr->present_tlvs |= (1ULL << BGP_LS_ATTR_NODE_MSD_BIT);
+		BGP_LS_TLV_SET(attr->present_tlvs, BGP_LS_ATTR_NODE_MSD_BIT);
 	}
 
 	/* IPv4 Router-ID (TLV 1028) */

[cscarpitta]

@greptile review

[cscarpitta]

Overall looks good. Just a few more comments.

[hedrok]

@cscarpitta I've updated MSD, thanks.
@greptile review.

[hedrok]

@cscarpitta I'll look into this comment from Greptile about IS-IS Area ID - I've really tested only on single-area case.
It may take some time, moving to Draft for now for it not be merged before that...

[github-actions]

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

[hedrok]

@greptile review

[hedrok]

@cscarpitta Yes, Greptile was correct: the source of IS-IS Area ID I've used was wrong, it always gave Area ID of current router.

• Updated to proper sourse, updated test so that r1, r2, r3, r4 have each separate IS-IS Area ID (they all are level 2, so this shouldn't be a problem), checking IS-IS Area ID of r4 in test, without changes it gave wrong result (Area ID of r2).
• Rebased on master, so use CHECK_FLAG/SET_FLAG now in all commits.

Hope now everything is fine.

@greptile review

[riw777]

@greptile can you verify your comments have been resolved?

[hedrok]

Fixed new P2 issue from greptile (duplicate ISO_ADDR_SIZE definition).
@greptile review.