ospfd: Add Opaque LSA decoder for json output #15042

odd22 · 2023-12-19T15:08:04Z

When dumping OSPF database with json output, this PR add decoder for the various TLVs and sub-TLVs instead of dumping bulk of data. Supported Opaque LSA are:

Traffic Engineering (Type 1)
Router Information (Type 4) including PCE and Segment Routing information
Inter-domain Link (Type 6)
Extended Prefix (Type 7)
Extended Link (Type 8)

ton31337

LGTM

ton31337 · 2023-12-20T21:03:42Z

ospfd/ospf_te.c

+			vty_out(vty, "  Maximum Reservable Bandwidth: %g (Bytes/sec)\n",
+				fval);
+		else
+			json_object_double_add(json, "maxReservableBandwidth",


nit: maximumReservableBandwidth, as we have maximumBandwidth (or use maxXX for both).

ton31337 · 2023-12-20T21:04:50Z

ospfd/ospf_te.c

+			vty_out(vty, "  Resource class/color: 0x%x\n",
+				(uint32_t)ntohl(top->value));
+		else
+			json_object_string_addf(json, "resClassColor", "0x%x",


resourceClassColor maybe? Don't see a reason to shorten here.

ton31337 · 2023-12-20T21:20:37Z

ospfd/ospf_ri.c

+				"    Node Maximum Stack Depth = %d\n",
+				msd->value);
+		else
+			json_object_int_add(json, "nodeMaximumSIDdepth",


According to our JSON convention it should be more like nodeMaximumSidDepth.

riw777

looks good

ton31337 · 2024-01-06T11:25:20Z

tests/topotests/ospfapi/test_ospf_clientapi.py

@@ -1684,7 +1694,9 @@ def _test_opaque_link_local_lsa_crash(tgen, apibin):
                            "linkStateId": "230.0.0.1",
                            "advertisingRouter": "1.0.0.0",
                            "lsaSeqNumber": "80000001",
-                            "opaqueData": "feedaceecafebeef",
+                            "opaqueData": {
+                              "opaqueData": "feedaceecafebeef",


Just noticed, that this is a new commit since the last review. Is this supposed to be double-named? I mean opaqueData->opaqueData? Are there more fields under the first opaqueData?

Yes and no. I use the generic key "opaqueData" to enclose all TLV carried by the Opaque LSA. Then, I discover that ospf client api uses also the generic term opaqueData.

To avoid this double name, let me know what do you prefer:

uses "opaqueTLV" as top key for the Opaque Data

replace the "opaqueData" key by the simple "data" key in ospf api client

To me personally the first sounds better because it clearly explains what's behind it. And keeping just data is too generic. What others think?

I like the first better, as well ...

I finally change for TLV but a little bit different from what I originally proposed. There is already opaqueType and opaqueDataLenght. Thus, I change for opaqueValues (the V of TLV) and change opaqueDataLength by opaqueLength. Thus, we got the Type, Length and Values of the TLV encoding of Opaque LSA. I hope it is fine for you.

riw777 · 2024-01-09T15:09:00Z

Waiting on @ton31337 's comments to finish this one up.

choppsv1

Are these values covered by any existing IETF YANG draft or RFC?

mwinter-osr · 2024-01-14T23:27:51Z

CI:rerun Retest for new ASAN Memory Leaks

odd22 · 2024-01-22T16:28:38Z

Are these values covered by any existing IETF YANG draft or RFC?

There is RFC8795 https://www.rfc-editor.org/rfc/rfc8795 but honestly, it is too complicated for us. This RFC defines the yang model for the complete Traffic Engineering topology, which is a super set of out IP traffic engineering. In addition, there is many many missing information (from OSPF point of view). And, in FRR, we use the camelCase convention while IETF use '-' dash-case convention. Thus, I don't know if we should follow IETF yang model.

choppsv1 · 2024-01-22T17:34:27Z

Are these values covered by any existing IETF YANG draft or RFC?

There is RFC8795 https://www.rfc-editor.org/rfc/rfc8795 but honestly, it is too complicated for us. This RFC defines the yang model for the complete Traffic Engineering topology, which is a super set of out IP traffic engineering. In addition, there is many many missing information (from OSPF point of view). And, in FRR, we use the camelCase convention while IETF use '-' dash-case convention. Thus, I don't know if we should follow IETF yang model.

Pleaes read this: https://docs.frrouting.org/projects/dev-guide/en/latest/workflow.html#json-output

You don't have to use the entire IETF model, but use the key names for sure, map as indicated. Our JSON does need to be backed by a model though.

Instead of output bulk of data with json output, prepare json context to decode opaque TLVs and sub-TLVs. Signed-off-by: Olivier Dugeon <[email protected]>

When dumping ospf database with json output, decode Traffic Engineering TLVs and sub-TLVs. Signed-off-by: Olivier Dugeon <[email protected]>

When dumping ospf database with json output, decode Router Information TLVs and sub-TLVs. Signed-off-by: Olivier Dugeon <[email protected]>

When dumping ospf database with json output, decode Extended Link and Extended Prefix TLVs and sub-TLVs. Signed-off-by: Olivier Dugeon <[email protected]>

Following new json decoder for Opaque LSA, this patch adapts the ospfapiclient test to the new json output. Signed-off-by: Olivier Dugeon <[email protected]>

odd22 · 2024-02-12T16:54:48Z

Are these values covered by any existing IETF YANG draft or RFC?

There is RFC8795 https://www.rfc-editor.org/rfc/rfc8795 but honestly, it is too complicated for us. This RFC defines the yang model for the complete Traffic Engineering topology, which is a super set of out IP traffic engineering. In addition, there is many many missing information (from OSPF point of view). And, in FRR, we use the camelCase convention while IETF use '-' dash-case convention. Thus, I don't know if we should follow IETF yang model.

Pleaes read this: https://docs.frrouting.org/projects/dev-guide/en/latest/workflow.html#json-output

You don't have to use the entire IETF model, but use the key names for sure, map as indicated. Our JSON does need to be backed by a model though.

I just update the PR with various yang models I could find (both RFC and draft). However, there are not covered all TE convey in OSPF Opaque LSA. So, in this case, I keep what I proposed previously.

@choppsv1 Let me know if it is OK for you

frrbot bot added the ospf label Dec 19, 2023

odd22 requested a review from donaldsharp December 19, 2023 15:08

github-actions bot added master size/XXL labels Dec 19, 2023

riw777 self-requested a review December 19, 2023 16:32

odd22 force-pushed the ospf-te branch from fd1caa6 to e4930ba Compare December 20, 2023 11:18

frrbot bot added the tools label Dec 20, 2023

odd22 force-pushed the ospf-te branch from e4930ba to e41d449 Compare December 20, 2023 11:21

odd22 removed the tools label Dec 20, 2023

ton31337 approved these changes Dec 20, 2023

View reviewed changes

frrbot bot added the tests Topotests, make check, etc label Dec 21, 2023

riw777 approved these changes Jan 2, 2024

View reviewed changes

ton31337 reviewed Jan 6, 2024

View reviewed changes

choppsv1 reviewed Jan 12, 2024

View reviewed changes

ton31337 added this to the 10.0 milestone Jan 22, 2024

odd22 force-pushed the ospf-te branch 2 times, most recently from 6908188 to 9136fcb Compare January 22, 2024 16:12

github-actions bot added the rebase PR needs rebase label Jan 22, 2024

odd22 removed size/XXL rebase PR needs rebase labels Jan 22, 2024

odd22 force-pushed the ospf-te branch from 9136fcb to efade61 Compare January 22, 2024 16:17

frrbot bot added libfrr rip ripng labels Jan 22, 2024

github-actions bot added the size/XXL label Jan 22, 2024

odd22 force-pushed the ospf-te branch from efade61 to df8368d Compare January 22, 2024 16:22

odd22 removed ripng rip labels Jan 22, 2024

odd22 removed libfrr size/XXL labels Jan 22, 2024

donaldsharp self-assigned this Feb 6, 2024

odd22 added 5 commits February 12, 2024 17:52

ospfd: Prepare Opaque LSA for json output

aa2b539

Instead of output bulk of data with json output, prepare json context to decode opaque TLVs and sub-TLVs. Signed-off-by: Olivier Dugeon <[email protected]>

ospfd: Decode TE TLVs for json output

95c9cb9

When dumping ospf database with json output, decode Traffic Engineering TLVs and sub-TLVs. Signed-off-by: Olivier Dugeon <[email protected]>

ospfd: Decode Router Info. TLVs for json output

b27d9e4

When dumping ospf database with json output, decode Router Information TLVs and sub-TLVs. Signed-off-by: Olivier Dugeon <[email protected]>

ospfd: Decode Extended Link & Prefix TLVs for json

6fcc86d

When dumping ospf database with json output, decode Extended Link and Extended Prefix TLVs and sub-TLVs. Signed-off-by: Olivier Dugeon <[email protected]>

tests: adapt ospfapiclient test to new json output

b46a1fe

Following new json decoder for Opaque LSA, this patch adapts the ospfapiclient test to the new json output. Signed-off-by: Olivier Dugeon <[email protected]>

odd22 force-pushed the ospf-te branch from df8368d to b46a1fe Compare February 12, 2024 16:52

github-actions bot added the size/XXL label Feb 12, 2024

choppsv1 approved these changes Feb 13, 2024

View reviewed changes

choppsv1 merged commit 7b94a92 into FRRouting:master Feb 13, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ospfd: Add Opaque LSA decoder for json output #15042

ospfd: Add Opaque LSA decoder for json output #15042

odd22 commented Dec 19, 2023

ton31337 left a comment

ton31337 Dec 20, 2023

odd22 Jan 22, 2024

ton31337 Dec 20, 2023

odd22 Jan 22, 2024

ton31337 Dec 20, 2023

odd22 Jan 22, 2024

riw777 left a comment

ton31337 Jan 6, 2024

odd22 Jan 8, 2024

ton31337 Jan 9, 2024

riw777 Jan 16, 2024

odd22 Jan 22, 2024

riw777 commented Jan 9, 2024

choppsv1 left a comment

mwinter-osr commented Jan 14, 2024

odd22 commented Jan 22, 2024

choppsv1 commented Jan 22, 2024 •

edited

Loading

odd22 commented Feb 12, 2024

ospfd: Add Opaque LSA decoder for json output #15042

ospfd: Add Opaque LSA decoder for json output #15042

Conversation

odd22 commented Dec 19, 2023

ton31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riw777 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riw777 commented Jan 9, 2024

choppsv1 left a comment

Choose a reason for hiding this comment

mwinter-osr commented Jan 14, 2024

odd22 commented Jan 22, 2024

choppsv1 commented Jan 22, 2024 • edited Loading

odd22 commented Feb 12, 2024

choppsv1 commented Jan 22, 2024 •

edited

Loading