ospf: Add RFC 9339 support #78

nrybowski · 2025-06-30T17:40:03Z

This PR adds the support for Reverse Metric and Reverse TE Metric advertisement through the LLS data block of Hello packets, as defined by RFC 9339.

It depends on RFC 5613 whose support has been added with #77.

TODO

rwestphal · 2025-07-01T15:24:06Z

Add YANG model. (There is no YANG model specific to OSPF for now, I will adapt the one for IS-IS.)

Indeed. I see there's [email protected] but no equivalent module for OSPF.

There's draft-ietf-lsr-ospf-yang-augmentation-v1, which adds several augmenting modules to ietf-ospf. We can add our own holo-ospf-reverse-metric and potentially ask the IETF folks to integrate it into that draft. That would probably be welcome on their part, especially since it would come with a working implementation.

holo-ospf/src/events.rs

rwestphal

Awesome work. I really like how this is shaping up!

As a preliminary review comment, I think the YANG module could be simplified.

Currently, the interface augmentation looks like this:

     +--rw holo-ospf-reverse-metric:reverse-metric
        +--rw holo-ospf-reverse-metric:enable-receive?     boolean
        +--rw holo-ospf-reverse-metric:enable-advertise?   boolean
        +--rw holo-ospf-reverse-metric:config* [mtid]
           +--rw holo-ospf-reverse-metric:mtid      uint8
           +--rw holo-ospf-reverse-metric:metric?   ospf:ospf-link-metric
           +--rw holo-ospf-reverse-metric:flags
              +--rw holo-ospf-reverse-metric:higher?   boolean
              +--rw holo-ospf-reverse-metric:offset?   boolean

It could be simplified to something like this, more in line with the IS-IS RM module:

     +--rw holo-ospf-reverse-metric:reverse-metric
        +--rw holo-ospf-reverse-metric:enable-receive?     boolean
        +--rw holo-ospf-reverse-metric:metric?   ospf:ospf-link-metric
        +--rw holo-ospf-reverse-metric:flags
          +--rw holo-ospf-reverse-metric:higher?   boolean
          +--rw holo-ospf-reverse-metric:offset?   boolean

Later, once support for RFC 4915 is implemented, a separate augmentation could be added for "interface/topologies/topology". I think that would be cleaner, especially since "mtid" only makes sense for OSPFv2.

Also, the "enable-advertise" leaf can be removed, as the presence of "metric" already implies we want to advertise the reverse metric.

What do you think?

nrybowski · 2025-10-02T23:49:50Z

Thanks for this first review!

Also, the "enable-advertise" leaf can be removed, as the presence of "metric" already implies we want to advertise the reverse metric.

This was also my initial though but when I implemented the Default trait for ReverseMetricMtCfg, I added a default value for the metric which enables 'by default' the RM. A more clever way to implement that would be storing the metric value in an Option so that the default value is None.
Also, RFC9339 states in Section 7 that "An implementation MUST NOT signal RM to neighbors by default and MUST provide a configuration option to enable the signaling of RM on specific links.", and I understood this sentence as "we have to provide an explicit switch to enable the metric".
I will remove the "enable-advertise" leaf and use an Option to store the RM.

Later, once support for RFC 4915 is implemented, a separate augmentation could be added for "interface/topologies/topology".

I prefer defining a module that completely specifies RFC 9339 and then add a deviation that indicates unsupported augmentations.

I think that would be cleaner, especially since "mtid" only makes sense for OSPFv2.

That's a good point that I completely overlooked. I will add two augmentations then, one that accepts a single RM on the interface and enforce that mt-id equals 0 and one for OSPFv2 that adds a RM to "interface/topologies/topology" as you suggested.

We will then add an 'unsupported' deviation for the last one in holo-ospf-dev.yang.

Does that sounds reasonable to you?

This commit addresses the comment from holo-routing#78 (review) with the suggestion proposed in holo-routing#78 (comment). Signed-off-by: Nicolas Rybowski <[email protected]>

This patch adds Reverse Metric and Reverse TE Metric TLVs to LLS data blocks. This patch hence depends on RFC 5613, added with holo-routing#77. Signed-off-by: Nicolas Rybowski <[email protected]>

Signed-off-by: Nicolas Rybowski <[email protected]>

This commit adds a custom YANG module to configure Reverse Metrics per interface and the related callbacks. It also implements the handling of such Reverse Metric both at the sender and receiver sides. Signed-off-by: Nicolas Rybowski <[email protected]>

rwestphal · 2025-10-03T01:41:23Z

This was also my initial though but when I implemented the Default trait for ReverseMetricMtCfg, I added a default value for the metric which enables 'by default' the RM. A more clever way to implement that would be storing the metric value in an Option so that the default value is None. Also, RFC9339 states in Section 7 that "An implementation MUST NOT signal RM to neighbors by default and MUST provide a configuration option to enable the signaling of RM on specific links.", and I understood this sentence as "we have to provide an explicit switch to enable the metric". I will remove the "enable-advertise" leaf and use an Option to store the RM.

Awesome. Option is a perfect fit for configuration options without default values.

I prefer defining a module that completely specifies RFC 9339 and then add a deviation that indicates unsupported augmentations.

That's indeed better!

That's a good point that I completely overlooked. I will add two augmentations then, one that accepts a single RM on the interface and enforce that mt-id equals 0 and one for OSPFv2 that adds a RM to "interface/topologies/topology" as you suggested.

We will then add an 'unsupported' deviation for the last one in holo-ospf-dev.yang.

Does that sounds reasonable to you?

Sounds good to me.

Just keep in mind that in "interface/topologies/topology" you can configure a topology name, but not an "mtid", which can be a bit confusing. This is probably because the IETF folks couldn't agree on whether the MT-ID should be explicitly configured or inferred from the selected RIB. In any case, this shouldn't affect your augmentation. I just wanted to point it out in case it causes any confusion :)

This commit addresses the comment from holo-routing#78 (review) with the suggestion proposed in holo-routing#78 (comment). Signed-off-by: Nicolas Rybowski <[email protected]>

nrybowski · 2025-10-03T01:49:33Z

Just rebased on master to clear the conflicts. 81df85f contains the changes we just discussed.

Just keep in mind that in "interface/topologies/topology" you can configure a topology name, but not an "mtid", which can be a bit confusing. This is probably because the IETF folks couldn't agree on whether the MT-ID should be explicitly configured or inferred from the selected RIB. In any case, this shouldn't affect your augmentation. I just wanted to point it out in case it causes any confusion :)

The good news is that the "mtid" augmentation for OSPFv2 is gated with the feature "multi-topology" so we don't even need a deviation to "unsupport" the augmentation and we can keep the headache of handling that when RFC 4915 is implemented :)

rwestphal · 2025-10-03T01:59:46Z

The good news is that the "mtid" augmentation for OSPFv2 is gated with the feature "multi-topology" so we don't even need a deviation to "unsupport" the augmentation and we can keep the headache of handling that when RFC 4915 is implemented :)

Yeah that makes things easier for us :)

From a quick look, it seems most of the RFC is already implemented. Tomorrow I'll run some tests and do a full review. Looking forward to it :)

rwestphal

I just ran some tests and it's very interesting to see the reverse metric in action! It's quite a useful feature for operators to have in their toolbox.

Please find my review comments below.

rwestphal · 2025-10-03T22:48:43Z

holo-ospf/src/interface.rs

+    // Dynamic value of the interface cost.
+    pub cost: u16,
+    // History of known Reverse Metric received on this interface.
+    pub rms: BTreeMap<u8, (ReverseMetricFlags, u16)>,


I think s/History of known/Last known/ would be more accurate.

Also, since we don't support RFC 4915 yet, it would be simpler to replace the BTreeMap with an Option. We don't need to keep track of MT-IDs other than zero. If we were to keep the BTreeMap, we would also need to use a BTreeMap for the cost field.

rwestphal · 2025-10-03T22:49:38Z

holo-ospf/src/events.rs

+            // If it has not changed, we do not update the metric nor we
+            // originate a new Router LSA.
+            // If the Reverse Metric from the neighbor has been updated since
+            // the last one we received, then we propagate the information.


In this code, we are handling changes in the reverse metric, but the case where the RM TLV is no longer present is being ignored.

rwestphal · 2025-10-03T22:50:43Z

holo-yang/modules/augmentations/holo-ospf-reverse-metric.yang

+                  "RFC 9339: OSPF Reverse Metric, Section 4.";
+            }
+        }
+        leaf mtid {


This leaf should be removed. The MT-ID is a property of the topology, not from the reverse metric. (yes, "interface/topologies/topology" is missing an "mtid" leaf, but that's a separate issue)

rwestphal · 2025-10-03T22:51:15Z

holo-yang/modules/augmentations/holo-ospf-reverse-metric.yang

@@ -0,0 +1,186 @@
+module holo-ospf-reverse-metric {
+    yang-version 1.1;


nit: both the IETF and Holo YANG modules use two spaces for indentation, not four.

rwestphal · 2025-10-03T22:52:06Z

holo-ospf/src/ospfv2/packet/lls.rs

+    }
+}
+
+impl From<&LlsHelloData> for LlsDataBlock {


Why this instead of implementing From<LlsHelloData> for LlsDataBlock?

rwestphal · 2025-10-03T22:53:40Z

holo-ospf/src/ospfv2/interface.rs

-            None
-        };
+        let advertise_rm = iface.config.reverse_metric.metric.is_some();
+        let lls = (iface.config.lls_enabled & advertise_rm).then(|| {


I think you meant && and not & :)

In any case, this code can be a bit simpler if we use a mutable lls variable:

// Handle Reverse Metric (RFC 9339). let mut lls = None; let rm_cfg = &iface.config.reverse_metric; if iface.config.lls_enabled && let Some(metric) = rm_cfg.metric { let mut lls_data = LlsHelloData::default(); let mut flags = ReverseMetricFlags::empty(); if rm_cfg.higher { flags |= ReverseMetricFlags::H; } if rm_cfg.offset { flags |= ReverseMetricFlags::O; } lls_data.reverse_metric.insert(0, (flags, metric)); lls = Some(lls_data); }

rwestphal · 2025-10-03T22:54:57Z

holo-ospf/src/ospfv2/packet/lls.rs

+impl From<&LlsDataBlock> for LlsHelloData {
+    fn from(value: &LlsDataBlock) -> Self {
+        let mut reverse_metrics = BTreeMap::new();
+        value.reverse_metric.iter().for_each(|tlv| {


nit: this is a matter of style, but I'd prefer sticking to regular for loops instead of using for_each. It would be good to keep the codebase consistent by using a single style.

rwestphal · 2025-10-03T22:55:57Z

holo-ospf/tests/packet/ospfv3.rs

+            0x04, 0x00, 0x01, 0x12, 0x34, 0x00, 0x13, 0x00, 0x04, 0x01, 0x02,
+            0x43, 0x21, 0x00, 0x14, 0x00, 0x08, 0x02, 0x00, 0x00, 0x00, 0x00,
+            0x00, 0x56, 0x78,
+            // 0x00, 0x00, 0x00, 0x03,


nit: unnecessary comments.

nrybowski force-pushed the rfc9339 branch from 5dfa951 to 692fba9 Compare July 1, 2025 09:12

nrybowski force-pushed the rfc9339 branch from 692fba9 to 50230ed Compare October 1, 2025 01:22

nrybowski commented Oct 1, 2025

View reviewed changes

holo-ospf/src/events.rs Outdated Show resolved Hide resolved

nrybowski commented Oct 1, 2025

View reviewed changes

holo-ospf/src/events.rs Outdated Show resolved Hide resolved

rwestphal reviewed Oct 2, 2025

View reviewed changes

nrybowski force-pushed the rfc9339 branch from 50230ed to 46ab06e Compare October 2, 2025 23:22

nrybowski added 4 commits October 3, 2025 03:39

ospf: Add support for RFC 9339

c4fbf59

This patch adds Reverse Metric and Reverse TE Metric TLVs to LLS data blocks. This patch hence depends on RFC 5613, added with holo-routing#77. Signed-off-by: Nicolas Rybowski <[email protected]>

ospfv2: Encode/decode tests for Reverse(TE) Metric

7ae0d0d

Signed-off-by: Nicolas Rybowski <[email protected]>

ospfv3: Encode/decode tests for Reverse(TE) Metric

62a36bc

Signed-off-by: Nicolas Rybowski <[email protected]>

ospf: Handle Reverse Metric.

b789ee9

This commit adds a custom YANG module to configure Reverse Metrics per interface and the related callbacks. It also implements the handling of such Reverse Metric both at the sender and receiver sides. Signed-off-by: Nicolas Rybowski <[email protected]>

ospf: Simplify Reverse Metric YANG model.

81df85f

This commit addresses the comment from holo-routing#78 (review) with the suggestion proposed in holo-routing#78 (comment). Signed-off-by: Nicolas Rybowski <[email protected]>

nrybowski force-pushed the rfc9339 branch from d0236c0 to 81df85f Compare October 3, 2025 01:43

rwestphal reviewed Oct 3, 2025

View reviewed changes

		@@ -0,0 +1,186 @@
		module holo-ospf-reverse-metric {
		yang-version 1.1;

ospf: Add RFC 9339 support #78

Are you sure you want to change the base?

ospf: Add RFC 9339 support #78

Uh oh!

Conversation

nrybowski commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

rwestphal commented Jul 1, 2025

Uh oh!

Uh oh!

Uh oh!

rwestphal left a comment

Choose a reason for hiding this comment

Uh oh!

nrybowski commented Oct 2, 2025

Uh oh!

rwestphal commented Oct 3, 2025

Uh oh!

nrybowski commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwestphal commented Oct 3, 2025

Uh oh!

rwestphal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nrybowski commented Jun 30, 2025 •

edited

Loading

nrybowski commented Oct 3, 2025 •

edited

Loading