Skip to content

fix: remove GetNATRSIPStatus check #39

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

wlynxg
Copy link

@wlynxg wlynxg commented Oct 24, 2024

In my operating environment, I found that tailscale and qBittorrent can perform port mapping through UPnP, but libp2p cannot.

After investigation, I found that libp2p has an extra step to get the GetNATRSIPStatus status check compared to other UPnP programs. This step has problems on my router.

I analyzed the UPnP data packets of different routers and found that GetNATRSIPStatus is not enabled on all routers. However, these routers have the AddPortMapping/DeletePortMapping/GetExternalIPAddress capabilities, so GetNATRSIPStatus seems to be less important here:

  • TP-Link:
    image

  • OpenWRT:
    image

  • Huawei:
    image

@Stebalien
Copy link
Member

This may cause us to pick the wrong device if multiple are available. Ideally we'd just use all discovered devices and attempt to port-map with all of them, but that's a larger refactor.

A simple solution here is to move this logic up the stack a bit to DiscoverGateway:

  1. Add an IsNAT() (bool, error) method to the NAT interface that calls GetNATSIPStatus.
  2. Inside DiscoverGateway, try calling IsNAT().
    1. If it returns false, ignore it.
    2. If it returns true, prefer it.
    3. If it returns an error, use it as long as there are no other NATs that return true.

@wlynxg
Copy link
Author

wlynxg commented Oct 29, 2024

When filtering services, WANIPConnection and WANPPPConnection have been filtered. When executing DiscoverGateway, NAT devices are filtered by the default gateway. These two default conditions can correctly filter out the correct NAT device in most cases.

If we really need further filtering conditions, we should determine whether the NAT device has the AddPortMapping/DeletePortMapping/GetExternalIPAddress capabilities, because these methods are the capabilities we need for port mapping. wdyt?

@Stebalien
Copy link
Member

If we really need further filtering conditions, we should determine whether the NAT device has the AddPortMapping/DeletePortMapping/GetExternalIPAddress capabilities, because these methods are the capabilities we need for port mapping. wdyt?

That would be ideal if you know how to do that without jumping through too many hoops. Really, the biggest issue we have here is that we lack a good test setup so I try to avoid changes that could potentially break someone's setup even if I have no evidence that it'll actually be an issue.

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.

2 participants