Allow adding nh_encap to rtnl_nh #443

cpaasch-oai · 2025-08-06T23:34:40Z

The goal is to allow setting a nexthop encapsulation on a nexthop object (and passing it down to the kernel). There already is an API to manipulate mpls encap on a route object. However, that API is exclusive to using rtnl_nexthop, while internally it is actually using a nh_encap.

Ideally, adding nexthop encaps is done all by using nh_encap objects (regardless of whether this is for a rtnl_nexthop or a rtnl_nh object. So, f1263ca is moving towards that direction. It also deprecated the previous rtnl_route_nh_encap_mpls() (and related APIs) as IMO a better way of handling this is by doing:

nh = rtnl_nh_encap_alloc()
rtnl_nh_encap_mpls(nh, ...)
rtnl_route_nh_set_encap(rtnh, nh)

The goal is to add full support for all types of nexthop encapsulations (ipvX tunnels, segment-routing,...). So, directly manipulating the nh_encap is cleaner.

Adding tests along the way, starting with a6a808b and from there on with each commit that is exposing new API.

For the tests to pass PR #442 is needed.

Makefile.am

tests/nl-test-util.c

include/netlink/route/nexthop.h

lib/route/nh_encap_mpls.c

include/netlink/route/nexthop.h

lib/route/nexthop.c

lib/route/nh.c

lib/route/route_obj.c

cpaasch-oai · 2025-08-13T04:39:45Z

Thanks for the feedback. Will continue going through it in the coming days.

lib/route/nh.c

lib/route/route_obj.c

thom311

getting close. Thank you!

lib/route/nh.c

cpaasch-oai · 2025-08-21T21:25:26Z

@thom311 - as I fix the backwards compatibility for all callers of nh_encap_parse_msg I opted to not create nh_encap_parse_msg_safe and rather handle the NOSUPPORT directly in nh_encap_parse_msg. LMK if you prefer another approach.

I'm not sure here to what extend we want to preserve past behavior vs. becoming more backwards compatible.

thom311 · 2025-08-21T22:32:30Z

@thom311 - as I fix the backwards compatibility for all callers of nh_encap_parse_msg I opted to not create nh_encap_parse_msg_safe and rather handle the NOSUPPORT directly in nh_encap_parse_msg. LMK if you prefer another approach.

OK, there is also

         if (!lwtunnel_encap_types[e_type].ops->parse_msg)
                  return -NLE_MSGTYPE_NOSUPPORT;

and

         if (e_type == LWTUNNEL_ENCAP_NONE) {
                  NL_DBG(2, "RTA_ENCAP_TYPE should not be LWTUNNEL_ENCAP_NONE\n");
                  return -NLE_INVAL;
         }

which I think warrant the same handling.

I'm not sure here to what extend we want to preserve past behavior vs. becoming more backwards compatible.

I think in general, unknown (or not implemented) attributes should be ignored. That is the only way to be forward compatible against future attributes. I don't think there is a lot of value in failing to parse unknown attributes (or unknown encap_type, in this case).

KanjiMonster

As a minor nitpick, I dislike brackets in commit subjects because anything in these get dropped by git-am when applying patches.

But that's just my preference.

libnl-route-3.sym

lib/route/nh.c

KanjiMonster · 2025-08-22T12:53:48Z

lib/route/nexthop.c

-	struct rtnl_nexthop *nh;
+	_nl_auto_rtnl_nexthop struct rtnl_nexthop *nh = NULL;


Switching to _nl_auto pointer should be a separate commit IMHO since this is unrelated to the changes according to the commit message, and makes reviewing harder.

Good point - I'm splitting out this commit.

lib/route/nexthop.c

KanjiMonster · 2025-08-22T13:03:28Z

lib/route/nexthop_encap.c

-	[LWTUNNEL_ENCAP_IP]   = { .name = "ip" },
-	[LWTUNNEL_ENCAP_IP6]  = { .name = "ip6" },
-	[LWTUNNEL_ENCAP_ILA]  = { .name = "ila" },
-	[LWTUNNEL_ENCAP_BPF]  = { .name = "bpf" },
+	[LWTUNNEL_ENCAP_IP] = { .name = "ip" },
+	[LWTUNNEL_ENCAP_IP6] = { .name = "ip6" },
+	[LWTUNNEL_ENCAP_ILA] = { .name = "ila" },
+	[LWTUNNEL_ENCAP_BPF] = { .name = "bpf" },


Likewise, being aligned around the equals sign seems to be a deliberate choice.

Unfortunately, the last time I checked there wasn't an option for that in clang-format.

lib/route/nh_encap_mpls.c

KanjiMonster · 2025-08-22T13:07:52Z

lib/route/nh.c

+		return NULL;
+
+	return nh->rtnh_encap;
+}


missing empty line after the function

include/netlink/route/nexthop.h

cpaasch-oai · 2025-08-26T21:42:27Z

As a minor nitpick, I dislike brackets in commit subjects because anything in these get dropped by git-am when applying patches.

But that's just my preference.

Fair enough! I will stop doing that. I'm wondering now myself why did I even start doing that 😅

Signed-off-by: Christoph Paasch <[email protected]>

Don't do `if (...) expr;`. Always start a new line after the if(). Signed-off-by: Christoph Paasch <[email protected]>

Primarily getter/setter tests as well kernel round-trip. Signed-off-by: Christoph Paasch <[email protected]>

The goal is to have struct rtnl_nh_encap be used for the route's nexthop (In struct rtnl_nexthop) but also directly in an IP nexthop (struct rtnl_nh). So, let's extract some functions to manipulate struct rtnl_nh_encap directly. Ideally, even for the route's nexthop we will use those to manipulate the encap. Ideally we stop using rtnl_route_nh_encap_mpls (and friends) and rather configure an encapsulation on a route's nexthop by using: nh = rtnl_nh_encap_alloc() rtnl_nh_encap_mpls(nh, ...) rtnl_route_nh_set_encap(rtnh, nh) Add tests for these new functions and drop the tests for the deprecated ones. Signed-off-by: Christoph Paasch <[email protected]>

For better and less error-prone failure handling. Signed-off-by: Christoph Paasch <[email protected]>

Signed-off-by: Christoph Paasch <[email protected]>

There is support for parsing and populating rtnl_nexthop's encap. However, not for rtnl_nh's encapsulation. Refactor this and make it such that the parsing code for both is reused. Signed-off-by: Christoph Paasch <[email protected]>

cpaasch-oai · 2025-08-26T22:27:33Z

@thom311 - as I fix the backwards compatibility for all callers of nh_encap_parse_msg I opted to not create nh_encap_parse_msg_safe and rather handle the NOSUPPORT directly in nh_encap_parse_msg. LMK if you prefer another approach.

OK, there is also
         if (!lwtunnel_encap_types[e_type].ops->parse_msg)
                  return -NLE_MSGTYPE_NOSUPPORT;

In my opinion, we should never have an encap in lwtunnel_encap_types whose ops has no parse_msg. Because, lwtunnel_encap_types is internal-only we can control what encaps are there. And, I think we can even drop the check here. Yes, we would crash if someone implements an "incorrect" encap, and I think failing loudly is good here.

and

         if (e_type == LWTUNNEL_ENCAP_NONE) {
                  NL_DBG(2, "RTA_ENCAP_TYPE should not be LWTUNNEL_ENCAP_NONE\n");
                  return -NLE_INVAL;
         }

Indeed. I see, iproute2 also handles this case gracefully.

which I think warrant the same handling.

There is also

	if (e_type > LWTUNNEL_ENCAP_MAX) {
		NL_DBG(2, "Unknown RTA_ENCAP_TYPE: %d\n", e_type);

for that one I am adding the graceful handling and just return 0.

A regular "nexthop object" à la `ip nexthop` can have an associated encapsulation. So, add the code to support that. Signed-off-by: Christoph Paasch <[email protected]>

thom311 · 2025-08-27T05:43:21Z

In my opinion, we should never have an encap in lwtunnel_encap_types whose ops has no parse_msg. Because, lwtunnel_encap_types is internal-only we can control what encaps are there. And, I think we can even drop the check here. Yes, we would crash if someone implements an "incorrect" encap, and I think failing loudly is good here.

Makes sense. In that case indeed there is no need for a check. A crash (in case of such a bug) is a perfectly fine (deterministic) behavior.

thom311 · 2025-08-29T09:39:31Z

lgtm. merged.

Thanks @cpaasch-oai for your work.
And thanks @KanjiMonster for caring!

cpaasch-oai force-pushed the t/nh_encap_generic branch from 06550e7 to b73f70b Compare August 7, 2025 03:18

thom311 reviewed Aug 12, 2025

View reviewed changes

thom311 reviewed Aug 13, 2025

View reviewed changes

lib/route/nh.c Show resolved Hide resolved

cpaasch-oai force-pushed the t/nh_encap_generic branch 2 times, most recently from ff1eac8 to 517050a Compare August 15, 2025 20:54

thom311 reviewed Aug 18, 2025

View reviewed changes

lib/route/nh.c Outdated Show resolved Hide resolved

lib/route/nh.c Show resolved Hide resolved

lib/route/nh.c Show resolved Hide resolved

lib/route/nh.c Outdated Show resolved Hide resolved

lib/route/route_obj.c Outdated Show resolved Hide resolved

cpaasch-oai force-pushed the t/nh_encap_generic branch from 517050a to 6abdf9a Compare August 18, 2025 19:14

thom311 reviewed Aug 20, 2025

View reviewed changes

lib/route/nh.c Outdated Show resolved Hide resolved

lib/route/nh.c Outdated Show resolved Hide resolved

lib/route/nh.c Outdated Show resolved Hide resolved

cpaasch-oai force-pushed the t/nh_encap_generic branch 2 times, most recently from da612f8 to de3f0fd Compare August 21, 2025 21:23

KanjiMonster reviewed Aug 22, 2025

View reviewed changes

cpaasch-oai added 7 commits August 26, 2025 15:00

nexthop: clang-format nexthop-related files

982cc3a

Signed-off-by: Christoph Paasch <[email protected]>

rtnl_nexthop: Minor cleanup

bb15356

Don't do `if (...) expr;`. Always start a new line after the if(). Signed-off-by: Christoph Paasch <[email protected]>

nexthop: Add testsuite for nexthop API

6154191

Primarily getter/setter tests as well kernel round-trip. Signed-off-by: Christoph Paasch <[email protected]>

nexthop: Use _nl_auto_rtnl_nexthop in rtnl_route_nh_clone()

fe179fb

For better and less error-prone failure handling. Signed-off-by: Christoph Paasch <[email protected]>

nh-encap: Support cloning rtnl_nh_encap in rtnl_route_nh_clone

1e68463

Signed-off-by: Christoph Paasch <[email protected]>

nh-encap: Allow adding a nh_encap to a rtnl_nh

6ec1802

A regular "nexthop object" à la `ip nexthop` can have an associated encapsulation. So, add the code to support that. Signed-off-by: Christoph Paasch <[email protected]>

cpaasch-oai force-pushed the t/nh_encap_generic branch from de3f0fd to 6ec1802 Compare August 26, 2025 22:28

thom311 merged commit 6c0c02f into thom311:main Aug 29, 2025
4 checks passed

		struct rtnl_nexthop *nh;
		_nl_auto_rtnl_nexthop struct rtnl_nexthop *nh = NULL;

Allow adding nh_encap to rtnl_nh #443

Allow adding nh_encap to rtnl_nh #443

Conversation

cpaasch-oai commented Aug 6, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpaasch-oai commented Aug 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thom311 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpaasch-oai commented Aug 21, 2025

Uh oh!

thom311 commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KanjiMonster left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KanjiMonster Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

cpaasch-oai Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KanjiMonster Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

KanjiMonster Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

cpaasch-oai Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cpaasch-oai commented Aug 26, 2025

Uh oh!

cpaasch-oai commented Aug 26, 2025

Uh oh!

thom311 commented Aug 27, 2025

Uh oh!

Uh oh!

thom311 commented Aug 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thom311 commented Aug 21, 2025 •

edited

Loading