ospf6d: Fix metric when sending AS-external LSAs #15627

Max-Mustermann33 · 2024-03-27T13:50:03Z

When ospf6d originates an AS-external route that has been read from a kernel routing table, then the metric of that route was ignored until now. In my case all of these routes were announced with metric 1 in LS-updates.

Now the metric of the kernel route is added to whatever metric ospf6d already computed originally.

aceelindem

This is wrong to change the behavior to unconditionally add the existing route metric to the redistributed metric. I'd hope we have a topotest that would catch this. If this behavior is desired is should be conditional (possibly via a route-map) and be available for all redistributions (not just applied to OSPFv3).

Max-Mustermann33 · 2024-04-08T10:59:34Z

This is wrong to change the behavior to unconditionally add the existing route metric to the redistributed metric. I'd hope we have a topotest that would catch this. If this behavior is desired is should be conditional (possibly via a route-map) and be available for all redistributions (not just applied to OSPFv3).

I could not find a lot about this in the FRRouting documentation, but after some tests it looks like the following config-option allows using the existing route metric as redistributed metric. At least for IPv4 it does.

redistribute kernel route-map ABC

route-map ABC permit 10
set metric +1

When using the same config with IPv6 I end up with metric 1 for all redistributed kernel-routes. (without my patch)

Is there possibly a bug in ospf6d that prevents this config from actually working or is there an intention behind the different behaviour?

aceelindem · 2024-04-08T17:47:33Z

This is wrong to change the behavior to unconditionally add the existing route metric to the redistributed metric. I'd hope we have a topotest that would catch this. If this behavior is desired is should be conditional (possibly via a route-map) and be available for all redistributions (not just applied to OSPFv3).

I could not find a lot about this in the FRRouting documentation, but after some tests it looks like the following config-option allows using the existing route metric as redistributed metric. At least for IPv4 it does.

redistribute kernel route-map ABC

route-map ABC permit 10 set metric +1

When using the same config with IPv6 I end up with metric 1 for all redistributed kernel-routes. (without my patch)

Is there possibly a bug in ospf6d that prevents this config from actually working or is there an intention behind the different behaviour?

Ok - I see that the route-map increment and decrement functions are not implemented. However, you have the wrong fix. The right fix is to add this capability to the route-map callback for ospf6d. static enum route_map_cmd_result_t
ospf6_routemap_rule_set_metric(void *rule, const struct prefix *prefix,
void *object)
Unfortunatlely, I don't have time to work on this but hopefully I've pointed you in the right direction.

aceelindem · 2024-04-09T10:57:08Z

This is wrong to change the behavior to unconditionally add the existing route metric to the redistributed metric. I'd hope we have a topotest that would catch this. If this behavior is desired is should be conditional (possibly via a route-map) and be available for all redistributions (not just applied to OSPFv3).

I could not find a lot about this in the FRRouting documentation, but after some tests it looks like the following config-option allows using the existing route metric as redistributed metric. At least for IPv4 it does.
redistribute kernel route-map ABC
route-map ABC permit 10 set metric +1
When using the same config with IPv6 I end up with metric 1 for all redistributed kernel-routes. (without my patch)
Is there possibly a bug in ospf6d that prevents this config from actually working or is there an intention behind the different behaviour?

Ok - I see that the route-map increment and decrement functions are not implemented. However, you have the wrong fix. The right fix is to add this capability to the route-map callback for ospf6d. static enum route_map_cmd_result_t ospf6_routemap_rule_set_metric(void *rule, const struct prefix *prefix, void *object) Unfortunatlly, I don't have time to work on this but hopefully I've pointed you in the right direction.

Look at ospfd/ospf_routemap.c - route_set_metric(). You need to stash the redistributed metric in the route (but not as the the default metric) and do something like this.

Max-Mustermann33 · 2024-04-09T11:04:26Z

Thank you, I will look into that.

Max-Mustermann33 · 2024-04-09T14:04:04Z

I added support for metric increment and decrement in ospf6d routemaps.
The kernel metric will now only be redistributed if such a routemap is configured.
For my use case this still works.

Is this what you had in your mind?

aceelindem

Looks like it should work but it could read a lot better and the range checking is needed to assure the metric is valid for AS External LSAs.

ospf6d/ospf6_asbr.c

aceelindem · 2024-04-10T14:42:43Z

ospf6d/ospf6_asbr.c

+		arg++;
+	}
+
+	metric->metric = strtoul(arg, NULL, 10);


Does this work with a metric subtraction since this could be misinterpreted? Shouldn't you skip the +/- so we don't have to wonder about the precise strtoul() behavior where "u" stands for unsigned.

Skipping the +/- is exactly what this code is doing. strtoul() will only see a number with no sign beforehand.
btw this is the same code as ospfd is using.

aceelindem · 2024-04-10T14:45:33Z

ospf6d/ospf6_asbr.c

+		route->path.cost += metric->metric;
+	else if (metric->type == metric_decrement)
+		route->path.cost -= metric->metric;
+	else if (metric->type == metric_absolute)


Don't you need range checking for the increment and decrement cases?

I added range checking in those cases.
As this code is basically copy-paste from ospfd, I think there is also a problem - except if the range checking is done somewhere else.

aceelindem

Looks good to me.

aceelindem · 2024-04-11T14:57:23Z

ospf6d/ospf6_asbr.c

+
+		/* Check overflow */
+		if (route->path.cost == 0 || route->path.cost > route->path.redistribute_cost)
+			route->path.cost = 1;


Not sure if OSPFv2 default to 1 or 0 in this case but it is a real cornercase anyway.

I was asking myself the same question. After reading the RFC It looks like metric 0 is only for some special cases.

Max-Mustermann33 · 2024-04-18T12:03:40Z

Is there something to do on my side? Like rebasing for example? Or will that be done upon merging?

aceelindem · 2024-04-18T15:45:53Z

Is there something to do on my side? Like rebasing for example? Or will that be done upon merging

Yes - you need to rebase as indicated by the @github-actions github-actions bot added the rebase label last week

Note that this could also help with the CI tests as the intermittent BFD failure (not related to your change) has been mitigated.

riw777

looks good

ton31337 · 2024-05-03T08:02:12Z

Please fix styling/formatting (frrbot check) before merging.

When ospf6d originates an AS-external route that has been read from a kernel routing table, then the metric of that route was ignored until now. If a routemap is configured, then this metric will be redistributed from now on. Using metric increment and decrement in routemaps is supported by ospf6d now. Signed-off-by: Alexander Rose <[email protected]>

Max-Mustermann33 · 2024-05-06T06:55:18Z

Please fix styling/formatting (frrbot check) before merging.

Thanks for the hint. I missed that one.

frrbot bot added the ospfv3 label Mar 27, 2024

github-actions bot added master size/S labels Mar 27, 2024

Max-Mustermann33 force-pushed the ospf6d_metric_for_type5_lsa branch 3 times, most recently from a481212 to f5a1687 Compare April 2, 2024 10:56

riw777 self-requested a review April 2, 2024 15:18

aceelindem requested changes Apr 2, 2024

View reviewed changes

Max-Mustermann33 force-pushed the ospf6d_metric_for_type5_lsa branch from f5a1687 to 85e39be Compare April 9, 2024 14:01

github-actions bot added size/M and removed size/S labels Apr 9, 2024

aceelindem requested changes Apr 10, 2024

View reviewed changes

Max-Mustermann33 force-pushed the ospf6d_metric_for_type5_lsa branch from 85e39be to a8fc0d3 Compare April 11, 2024 11:14

github-actions bot added the rebase PR needs rebase label Apr 11, 2024

Max-Mustermann33 force-pushed the ospf6d_metric_for_type5_lsa branch from a8fc0d3 to 8c83b82 Compare April 11, 2024 11:23

aceelindem approved these changes Apr 11, 2024

View reviewed changes

Max-Mustermann33 force-pushed the ospf6d_metric_for_type5_lsa branch from 8c83b82 to d0320e8 Compare April 19, 2024 07:38

riw777 approved these changes Apr 30, 2024

View reviewed changes

Max-Mustermann33 force-pushed the ospf6d_metric_for_type5_lsa branch from d0320e8 to 84d1285 Compare May 6, 2024 06:52

github-actions bot added size/L and removed size/M labels May 6, 2024

riw777 merged commit e941145 into FRRouting:master May 7, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ospf6d: Fix metric when sending AS-external LSAs #15627

ospf6d: Fix metric when sending AS-external LSAs #15627

Max-Mustermann33 commented Mar 27, 2024

aceelindem left a comment •

edited

Loading

Max-Mustermann33 commented Apr 8, 2024

aceelindem commented Apr 8, 2024

aceelindem commented Apr 9, 2024

Max-Mustermann33 commented Apr 9, 2024

Max-Mustermann33 commented Apr 9, 2024

aceelindem left a comment

aceelindem Apr 10, 2024

Max-Mustermann33 Apr 11, 2024

aceelindem Apr 10, 2024

Max-Mustermann33 Apr 11, 2024

aceelindem left a comment

aceelindem Apr 11, 2024

Max-Mustermann33 Apr 12, 2024

Max-Mustermann33 commented Apr 18, 2024

aceelindem commented Apr 18, 2024

riw777 left a comment

ton31337 commented May 3, 2024

Max-Mustermann33 commented May 6, 2024

ospf6d: Fix metric when sending AS-external LSAs #15627

ospf6d: Fix metric when sending AS-external LSAs #15627

Conversation

Max-Mustermann33 commented Mar 27, 2024

aceelindem left a comment • edited Loading

Choose a reason for hiding this comment

Max-Mustermann33 commented Apr 8, 2024

aceelindem commented Apr 8, 2024

aceelindem commented Apr 9, 2024

Max-Mustermann33 commented Apr 9, 2024

Max-Mustermann33 commented Apr 9, 2024

aceelindem left a comment

Choose a reason for hiding this comment

aceelindem Apr 10, 2024

Choose a reason for hiding this comment

Max-Mustermann33 Apr 11, 2024

Choose a reason for hiding this comment

aceelindem Apr 10, 2024

Choose a reason for hiding this comment

Max-Mustermann33 Apr 11, 2024

Choose a reason for hiding this comment

aceelindem left a comment

Choose a reason for hiding this comment

aceelindem Apr 11, 2024

Choose a reason for hiding this comment

Max-Mustermann33 Apr 12, 2024

Choose a reason for hiding this comment

Max-Mustermann33 commented Apr 18, 2024

aceelindem commented Apr 18, 2024

riw777 left a comment

Choose a reason for hiding this comment

ton31337 commented May 3, 2024

Max-Mustermann33 commented May 6, 2024

aceelindem left a comment •

edited

Loading