chore: move go-nat to internal package

[MarcoPolo]

I believe we are the main users of this library, and we don't have enough bandwidth to maintain this for other use cases besides our own.

This PR brings in the commit history from the go-nat repo, and does some refactoring to clean up the code and surface errors to help debug issues.

After merging this, I'll archive the old repo.

Before merge:

• Squash refactor commits
• merge this as a merge commit

[MarcoPolo]

For reviewing, only the last commit is likely useful.

[MarcoPolo]

Shouldn't we do this on all services that match this string?

[marten-seemann]

I believe we are the main users of this library, and we don't have enough bandwidth to maintain this for other use cases besides our own.

Please don't do this. It's totally ok to refuse PRs to reduce the maintenance burden (as you've been doing with zeroconf), and you can call this out in the README. But code should only be moved if the code logically belongs somewhere else. This clearly is not the case for go-nat.

[MarcoPolo]

Are you using this library?

[MarcoPolo]

go-nat is a thin layer over what the underlying libraries provide (primarily github.com/huin/goupnp and github.com/jackpal/go-nat-pmp). Granted, it provides a higher level API that abstracts out the differences between 3 different ways to reserve a port. But this isn't particularly difficult, if not finicky. For our use case, only the p2p/net/nat package uses go-nat. Therefore, it's entirely reasonable to move go-nat into an internal package for p2p/net/nat. This lets us iterate quicker on these related packages.

A fair complaint would be that this makes it much harder for other folks to use go-nat and our future changes to that library. This is true. but from my brief cursory code search, I didn't find any recent usages of this library, but I could have missed something. If you're using it that would be a useful data point.

As an alternative, folks looking for a higher level NAT package could make use of go-libp2p's nat package directly github.com/libp2p/go-libp2p/p2p/net/nat. It provides a higher level API than go-nat, is easier to use, and something we will support.

Given that we have a supported high level API with github.com/libp2p/go-libp2p/p2p/net/nat and users could still access the lower level libraries of github.com/huin/goupnp and github.com/jackpal/go-nat-pmp, I'm not sure I see the value in supporting go-nat as a separate package.

[aschmahmann]

@MarcoPolo I reviewed f8bd11c. Is there anything else you wanted me to look at (e.g. the git merging things)?

[aschmahmann]

We need a comment somewhere (e.g. on this function or the top level caller of all the upnp discovery mechanisms) describing why we'll handle whatever service type gets sent to us even if we searched for a totally different one (e.g. searching for IG1 and getting IP2 which comes from IG2). This will help us later if there's a problem.

Note: do we want / need this behavior anymore now that we're doing the sequential lookups to deal with badly implemented routers (e.g. fritzbox)?

[MarcoPolo]

This will now collect an error if it sees a v2 service on a v1 device.

[MarcoPolo]

@MarcoPolo I reviewed f8bd11c. Is there anything else you wanted me to look at (e.g. the git merging things)?

That should be it. I'll merge this a merge commit so the history should be preserved.

[aschmahmann]

Regarding whether we should bring this functionality into the monorepo I'm +1 with @MarcoPolo's assessment.

• The library is small
• The library is only used by us (and it's had a whole bunch of years of independence if it was going to get independent users)
• It's much easier to work with and debug the library within the monorepo.... which is part of what inspired the move to the monorepo to begin with

FWIW if the above were not true (particularly, if the library was going to be used by others independent of go-libp2p and the Go module pruning was insufficient) I'd understand wanting to keep the repo separate. Additionally, this is not an uncommon thing to do. Other projects like Syncthing (https://github.com/syncthing/syncthing/tree/124673f7a89b73e7389dc664ce46441b41b7012b/lib/upnp) and Tailscale (https://github.com/tailscale/tailscale/tree/08dd4994d09cf4b960663321136b4fde3f5ccb18/net/portmapper) seem to take the same approach with bundling their portmapping code into their codebases (albeit they're less library focused than go-libp2p is).

If in the future say a group like Syncthing wanted to collaborate on / share a portmapping module and it made sense to do so in a separate repo that'd seem very reasonable to me.