Add initial PCP support #17
Conversation
|
Not sure what is wrong with Travis, looks like it couldn't download a setup script. |
Stebalien
added
the
status/blocked
|
Awesome! I'll see if we can find someone to test this (I've asked on IRC, we'll see...). Could you quickly run your code through the go race detector? It looks like there might be a few issues. |
|
Yeah that'd be useful. I'm sure there are a few race conditions that I've missed, just trying to get the logic implemented atm, then will tidy it up and refactor where needed (hence WIP). If there are any that you see off the bat feel free to open an issue and I'll take a look. Wasn't aware of the race detector bundled with go. What is the proper way to use it? It looks as though it'll have to wait until I create some tests, unless there's something I'm missing? |
You're canceling by setting a variable that's read by a different thread. Go reserves the right to never observe that update. Check out https://golang.org/ref/mem for more details. (there may be others, that's why I suggested you try the race detector).
You can use it with |
Good spot, have corrected it, now uses a channel. Have run the race detector, doesn't give any output, which I assume means no race conditions were found. I've also got some code for implementing connection timeout/testing, but due to being unable to test on an actual server, I'm not committing it currently, as it could break the connection logic if the implementation is bad. I've stuck it in the udp-timeout branch, I'd appreciate any testing (running the example should be enough, it'll show some request and response bytes). |
|
Any updates or stuff that can be done to help push this along? Need functionality like this. |
|
This is blocked on testing the underlying library against a real device. |
|
I should hopefully be able to test this soon, have ordered a router with PCP support. I also need to rewrite some parts of it as things like connection checking are not implemented yet. |
Hmm is this not a feature widely supported? I did a google search and I couldn't find anything specific to PCP support with various routers. I have at my disposal:
|
feel free to test the underlying library if you have a router that supports it. Sadly I have to use a Vodafone supplied China Inc. router that has the bare minimum in terms of functionality. Am in the process of switching atm, then once that's done, will test with new router. |
So interestingly I tested the changes in this PR, and my router appears to support PCP and is recognized by the |
|
It’s possible that I missed something. I have just received the router, so I’ll be testing it over the coming days hopefully. |
|
Just to provide an update, the router arrived, I have been testing it, there are a few bugs that I have found, and will fix when I have a moment, mostly surrounding the PCP options logic and refresh logic. I encountered the same error that you had @bonedaddy but it was due to not having upnp2 enabled on the router, after that it was fine. |
|
miniupnpd should also implement PCP, so testing shouldn't require dedicated hardware. https://github.com/miniupnp/miniupnp/blob/master/README#L11-L12 |
After checking activity, it looks like upstream is dead. Am thus creating a PR for this repo. Copy pasted from original PR, last part may not apply to the same extent:
This isn't ready to merge yet, as some of the logic for a correct implementation is missing upstream, but I would be grateful if someone with a router that supports PCP could test this and let me know if it is working as expected (ie. it creates a mapping).
There is also potential for a bigger rework of go-nat in general, as the architecture of PCP is quite different from previous NAT traversal implementations, as it takes into account multiple network interfaces, supports IPv4 and IPv6, and server sent events.