Skip to content

Conversation

@hurricanehrndz
Copy link
Contributor

@hurricanehrndz hurricanehrndz commented Mar 21, 2025

On Unix systems, this module currently dumps and caches the complete routing table. Afterwards, when a user submits a query to a specific destination it walks the table looking for the most applicable route found in the cache.

If you compare this functionality to route get it is good, but not exact. Would it not be better to do almost exactly what route get does?

This patch is an attempt at just that. If uses the AF_ROUTE socket to formalate route get messages and parses the output. Until recently this would not have been possible due to bugs in the x/net/route package. I have fixed said bugs, so this approach is very feasible.

closes #56

A newer version of x/net required to appropriately query route socket.
@hurricanehrndz hurricanehrndz force-pushed the query_routes_via_routesocket branch from 53db2cc to 0d3e888 Compare March 21, 2025 14:57
@willscott
Copy link
Contributor

This does look generally nicer / less invasive than the current strategy, thanks for the PR.
I'll try to leave detailed comments tomorrow

@hurricanehrndz hurricanehrndz mentioned this pull request Mar 21, 2025
@hurricanehrndz hurricanehrndz force-pushed the query_routes_via_routesocket branch from 0d3e888 to 49c4ec3 Compare March 21, 2025 16:03
@hurricanehrndz hurricanehrndz force-pushed the query_routes_via_routesocket branch 2 times, most recently from 14f17cc to 91007f0 Compare March 24, 2025 16:45
Mimic what `route get` does, instead of dumping the routing table and
trying to determine the most specific route from the dump.
@hurricanehrndz hurricanehrndz force-pushed the query_routes_via_routesocket branch from 91007f0 to 02cbb33 Compare March 24, 2025 22:58
@hurricanehrndz hurricanehrndz force-pushed the query_routes_via_routesocket branch 2 times, most recently from a8f7214 to 9369077 Compare April 1, 2025 19:21
x/net module requires [email protected]:

>go mod tidy -go=1.23
>go: golang.org/x/[email protected] requires [email protected], but 1.23 is requested
@hurricanehrndz hurricanehrndz force-pushed the query_routes_via_routesocket branch from 9369077 to b875106 Compare April 1, 2025 19:29
@hurricanehrndz
Copy link
Contributor Author

@willscott any chance you can re-review

@willscott
Copy link
Contributor

Thanks for the comments, @djdv !

@hurricanehrndz
Copy link
Contributor Author

hurricanehrndz commented Apr 3, 2025 via email

@djdv
Copy link
Contributor

djdv commented Apr 3, 2025

Thanks

You're welcome. :^]
And good luck with it.

I was planning to look at this for real either Friday or Saturday (if I have free time).
But skimmed it early and didn't want to forget those comments.

I'm assuming some of this code is based around my original PR I'm tagged in, but just to be clear, I wanted to say explicitly that I have no official involvement with this project. So take my review remarks lightly.

@hurricanehrndz
Copy link
Contributor Author

hurricanehrndz commented Apr 3, 2025

Thanks

You're welcome. :^] And good luck with it.

I was planning to look at this for real either Friday or Saturday (if I have free time). But skimmed it early and didn't want to forget those comments.

I'm assuming some of this code is based around my original PR I'm tagged in, but just to be clear, I wanted to say explicitly that I have no official involvement with this project. So take my review remarks lightly.

Actually the code came about because this library was used in netbird, I only noticed your PR after I had submitted mine. Upon which point I did take some inspiration for dealing with the possibility of indefinite read loops, so thank you for that.

Prior to that I was developing similar applications to troubleshoot and fix some bugs in the x/net package.

This refactor tries to employ the single responsibility principal more
effectively. Certain large functions were broken down to smaller more
manageable scopes.

Additionally, unix error messages that are applicable for our scenario
have been annotated.
@hurricanehrndz hurricanehrndz force-pushed the query_routes_via_routesocket branch from f691123 to c1f0108 Compare April 3, 2025 18:03
 Please enter the commit message for your changes. Lines starting
…to query_routes_via_routesocket

* origin/query_routes_via_routesocket:
  Update netroute_bsd.go
Copy link
Contributor

@djdv djdv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for a different OS than I use, so I could not test executing it, but I did read netroute_bsd.go and it uses the same routing protocol.
The code is very nice and makes sense to me. 👍
Only some minor/negligible remarks.

@djdv
Copy link
Contributor

djdv commented Apr 6, 2025

Actually the code came about because this library was used in netbird, I only noticed your PR after I had submitted mine. Upon which point I did take some inspiration for dealing with the possibility of indefinite read loops, so thank you for that.

Prior to that I was developing similar applications to troubleshoot and fix some bugs in the x/net package.

Ahh, makes sense. The code looked familiar while I was reading it, and we're both targeting similar APIs after all.

Related to x/net, the golang.org/x/net/route dependency, does not seem to build on illumos.
Meaning I can't execute the library or test code, but did read it.

$ uname -irsv
SunOS 5.11 illumos-3caf57556b i86pc
$ go test -v .
# github.com/libp2p/go-netroute
package github.com/libp2p/go-netroute
        imports golang.org/x/net/route: build constraints exclude all Go files in /export/home/dd/go/pkg/mod/golang.org/x/[email protected]/route
FAIL    github.com/libp2p/go-netroute [setup failed]
FAIL

@hurricanehrndz
Copy link
Contributor Author

hurricanehrndz commented Apr 9, 2025

Actually the code came about because this library was used in netbird, I only noticed your PR after I had submitted mine. Upon which point I did take some inspiration for dealing with the possibility of indefinite read loops, so thank you for that.
Prior to that I was developing similar applications to troubleshoot and fix some bugs in the x/net package.

Ahh, makes sense. The code looked familiar while I was reading it, and we're both targeting similar APIs after all.

Related to x/net, the golang.org/x/net/route dependency, does not seem to build on illumos. Meaning I can't execute the library or test code, but did read it.

$ uname -irsv
SunOS 5.11 illumos-3caf57556b i86pc
$ go test -v .
# github.com/libp2p/go-netroute
package github.com/libp2p/go-netroute
        imports golang.org/x/net/route: build constraints exclude all Go files in /export/home/dd/go/pkg/mod/golang.org/x/[email protected]/route
FAIL    github.com/libp2p/go-netroute [setup failed]
FAIL

I wonder if you vendor and enable the build for illumos if it would work, the route socket doesn't change much from one unix os to another:
https://github.com/hurricanehrndz/golang-net/blob/master/route/address.go#L5

@hurricanehrndz
Copy link
Contributor Author

@willscott any further thought to this PR?

@djdv
Copy link
Contributor

djdv commented Apr 23, 2025

I wonder if you vendor and enable the build for illumos if it would work, the route socket doesn't change much from one unix os to another:

I'm sure something can be done to make it work on Solaris/Illumos, however-
I'm reluctant to admit this so bluntly, but I'll soon be completely out of money.
Implying the number of users is going to drop from 1 to 0. So it's not going to matter enough for anyone to put time into it.
(;´∀`)
Honesty aside, that concern can be addressed in a follow up PR anyhow. This one still seems like an improvement for the other systems.

@willscott
Copy link
Contributor

Sorry for my slow response, and sorry to hear that @djdv !

I do wonder if the route socket method here also is worth using in the linux case - it seems like it should work as well there as in BSD-derived cases?

I think i'm okay with merging this and doing the linux part in a followup, but will ideally wait on a next release until both are in the same place. I'm optimistic that the linux one can directly use this implementation even.

@hurricanehrndz
Copy link
Contributor Author

hurricanehrndz commented Apr 23, 2025

Let's do the linux in a follow up, it will be some work because Linux has moved the routing IPC to netlink socket, but it should be doable:
https://man7.org/linux/man-pages/man7/rtnetlink.7.html

@willscott willscott merged commit 3420296 into libp2p:master Apr 23, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Instead of parsing the routing table, why not query the route via the route socket

4 participants