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

Directory: Use configured Directory Public IP for Client when hole-punching #2819

Conversation

pljones
Copy link
Collaborator

@pljones pljones commented Aug 29, 2022

Short description of changes

Some Servers are inaccessible from a Client on the LAN of the Directory. This is because of how the "directory hole punch" works. We advertise this as a way of avoiding interfering with a router, to ease running a server. It works by sending a Jamulus message claiming to come from the client requesting the server list from the directory, to each server in the list.

However, where the client is on the same LAN as the directory, the source address for the client is the LAN address, not the public IP address which the server will subsequently receive. Hence the "hole punch" fails.

This change tries to make use of the directory public IP address, where that is explicitly known. (CHostAddress().InetAddr isn't enough - it needs to be set through --serverpublicip.) It avoids requiring a known value, however - it will simply continue failing as currently.

CHANGELOG: Directory: Use configured Directory Public IP for Client when hole-punching

Context: Fixes an issue?

Fixes: 2818 (#2818)

Does this change need documentation? What needs to be documented and how?

I would strongly suggest we review the documentation regarding the directory hole punch to make it clearer that there are known limitations in its effectiveness.

Status of this Pull Request

Genre Jazz is currently running with it. I can see most of the servers that were previously not working. The only one not working in my client is also not working in Jamulus Explorer. So I consider than 100% effective.

What is missing until this pull request can be merged?

Reviews.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@pljones pljones added this to the Release 3.9.1 milestone Aug 29, 2022
@pljones pljones added the bug Something isn't working label Aug 29, 2022
@pljones pljones self-assigned this Aug 29, 2022
@pljones pljones requested review from softins and hoffie August 29, 2022 15:13
src/serverlist.cpp Outdated Show resolved Hide resolved
src/serverlist.cpp Outdated Show resolved Hide resolved
@pljones pljones requested a review from ann0see August 29, 2022 15:25
@pljones pljones force-pushed the patch/#2818-workaround-local-client-problem branch 2 times, most recently from e9d7204 to 1422a03 Compare August 29, 2022 15:38
@softins
Copy link
Member

softins commented Aug 29, 2022

Will look at the code later, but just wanted to pick up on one part of the description:

It works by sending a Jamulus message claiming to come from the client requesting the server list from the directory, to each server in the list.

This is not accurate. The directory sends a Jamulus message to each listed server from itself (not claiming to be from the client). It is the content of the message that says "please send a message to <client>". When a server acts on that message by sending a message to the specified client, that has the effect of priming the server's NAT/firewall to accept subsequent messages from that client.

@pljones
Copy link
Collaborator Author

pljones commented Aug 29, 2022

It works by sending a Jamulus message claiming to come from the client requesting the server list from the directory, to each server in the list.

This is not accurate. The directory sends a Jamulus message to each listed server from itself (not claiming to be from the client). It is the content of the message that says "please send a message to <client>". When a server acts on that message by sending a message to the specified client, that has the effect of priming the server's NAT/firewall to accept subsequent messages from that client.

Yep, I realised that on a re-read of the code and changed an "as" to back to "to". So there's a protocol message actually from the directory (and source address says so), actually to the server (dest address says so) carrying the address of the client requesting the server list. The server is then expected to send (i.e. "reply" but not a reply as such at all) a message to that address, opening its firewall to a response (which may never happen but would likely be the client connect dialogue asking for the ping time to the server).

@ann0see ann0see removed their request for review August 29, 2022 17:10
@pljones pljones force-pushed the patch/#2818-workaround-local-client-problem branch from 1422a03 to badec10 Compare August 29, 2022 18:17
@hoffie hoffie added the needs documentation PRs requiring documentation changes or additions label Aug 29, 2022
Copy link
Member

@hoffie hoffie left a comment

Choose a reason for hiding this comment

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

Logic seems good to me, but haven't run any tests. Should certainly wait for Tony's review now that he has offered it! :)

@pljones PR title/Changelog/Commit title could even more specific. Maybe
Directory: Use configured Public IP for Client when hole-punching?

src/serverlist.cpp Outdated Show resolved Hide resolved
src/serverlist.cpp Outdated Show resolved Hide resolved
@pljones pljones changed the title Use Directory Public IP for Client if available Directory: Use configured Directory Public IP for Client when hole-punching Aug 29, 2022
@pljones pljones force-pushed the patch/#2818-workaround-local-client-problem branch 3 times, most recently from 745c909 to 30fb1c0 Compare August 30, 2022 16:22
@pljones
Copy link
Collaborator Author

pljones commented Aug 30, 2022

https://jamulus.io/wiki/Custom-Directories (to be renamed, IIRC) should have an additional note added under "Points to note about Directories":

  • When using a client on the same LAN as a directory, the directory itself needs to be run with the correct --serverpublicip value, so that any (non-LAN) servers registering with the directory that require the "hole punch" can be accessed by the LAN client(s). This is because otherwise the directory would provide the LAN address of the client to the server and the "hole punch" would fail.

Does that sound right?

@ann0see
Copy link
Member

ann0see commented Aug 30, 2022

@gilgongo should judge that

@pljones
Copy link
Collaborator Author

pljones commented Sep 1, 2022

And probably get confirmation I've got it clear from @softins, too 😄.

@pljones pljones force-pushed the patch/#2818-workaround-local-client-problem branch from 30fb1c0 to 0a27528 Compare September 2, 2022 15:30
@pljones pljones force-pushed the patch/#2818-workaround-local-client-problem branch from 0a27528 to ab396be Compare September 4, 2022 08:14
Copy link
Member

@softins softins left a comment

Choose a reason for hiding this comment

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

I can't really test this, as it is particular to a rare scenario, such as that of @pljones. But the logic looks sound.

As I understand it, it only affects the unusual situation where a directory server is on the same LAN as the client, but the directory server has external servers registered to it via port forwarding. In this situation, it ensures that the IP address specified in the body of the message request sent to the remote servers is actually the public IP (assumed shared by the client and the directory), and not the LAN IP of the local client.

@pljones pljones merged commit 72524e3 into jamulussoftware:master Sep 5, 2022
@pljones pljones deleted the patch/#2818-workaround-local-client-problem branch September 5, 2022 16:38
@pljones
Copy link
Collaborator Author

pljones commented Sep 5, 2022

Running on all my directories (and servers) apart from updatecheck2.jamulus.io, which remains on latest.

@pljones
Copy link
Collaborator Author

pljones commented Sep 7, 2022

jamulussoftware/jamuluswebsite#815 raised, so removing "Needs documentation" label.

@pljones pljones removed the needs documentation PRs requiring documentation changes or additions label Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some Servers are inaccessible from a Client on the LAN of the Directory
4 participants