Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NAT traversal: ICE-esque candidate selection #134

Merged
merged 33 commits into from
Sep 1, 2021
Merged

NAT traversal: ICE-esque candidate selection #134

merged 33 commits into from
Sep 1, 2021

Conversation

mcginty
Copy link
Collaborator

@mcginty mcginty commented Aug 25, 2021

This change adds the ability for peers to report additional candidate endpoints for other peers to attempt connections with outside of the endpoint reported by the coordinating server.

While not a complete solution to the full spectrum of NAT traversal issues (TURN-esque proxying is still notably missing), it allows peers within the same NAT to connect to each other via their LAN addresses, which is a win nonetheless. In the future, more advanced candidate discovery could be used to punch through additional types of NAT cone types as well.

Related discussions #109 and #115

Copy link
Member

@strohel strohel left a comment

Choose a reason for hiding this comment

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

I've done an initial review, found one likely oversight and a few nits. I'll wait until this is not marked Draft to do a more thorough review and perhaps some testing?

Meta: the fact this was split to more or less incremental commits helped with review. Commits like 23f391b client: break out peer diff printing to function for readability that are strictly "just move stuff around" or strictly "add new functionality" are the best. (OTOH the very same function is moved again in f560598) I still don't have a good idea what the best balance between prototyping friendliness and review friendliness is.

@mcginty mcginty marked this pull request as ready for review August 31, 2021 04:38
Copy link
Member

@strohel strohel left a comment

Choose a reason for hiding this comment

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

Looks good, I was finally able to grasp this in its entirety! Only the netlink response parsing code is somewhat gnarly, but that's likely due to inherent complexity of the netlink API itself.

I've added a few nits/suggestions.

@mcginty mcginty merged commit 8903604 into main Sep 1, 2021
@mcginty mcginty deleted the ice branch September 1, 2021 09:58
@pri11er
Copy link

pri11er commented Sep 1, 2021

I was just giving this a try, going from 1.4.1, and server fails to start:
innernet-server[10422]: INFO innernet_server::db > migrated db version from 1 to 2
innernet-server[10422]: Error: internal database error
innernet-server[10422]: Caused by:
innernet-server[10422]: Invalid column type Null at index: 10, name: candidates
.service: Main process exited, code=exited, status=1/FAILURE

Revert and it starts ok again:
innernet-server[10561]: INFO innernet_server::db > migrated db version from 2 to 1
innernet-server[10561]: INFO innernet_server > bringing up interface.
innernet-server[10561]: INFO innernet_server > 12 peers added to wireguard interface.
innernet-server[10561]: INFO innernet_server > innernet-server 1.4.1 starting.

@mcginty
Copy link
Collaborator Author

mcginty commented Sep 1, 2021

You're on top of things @pri11er :).

This is fixed in #136.

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.

3 participants