Retry NAT punching without duration on mapping failure #95
Conversation
|
//c @whyrusleeping for review |
|
what happens if we try to set a timeout on a router that doesnt support timeouts? |
|
The request fails, no mapping is created, this means that we didn't punch the NAT. |
|
If a request fails, can we tell that its because timeouts are not supported and not for some other reason? |
|
Oh lol I can see that happening :) Apart from this it's good to regularly retry the mapping even if it succeeded in the first place -- routers reset, forget stuff, etc. Rule of thumb: assume that your router is crap in one way or another until proven otherwise :) |
But that's for a different PR -- LGTM! |
|
@whyrusleeping we can't, go nat gives us only generic error that SOAP call failed. @lgierth didn't think about that, I will add it, shouldn't be hard. |
|
Yeah, this LGTM. only thing i would prefer is matching the errors more closely, but i doubt that we would get a consistent set of errors back in all cases |
|
Hmm, it looks like calling for new permanent mapping on the same port with the same agent returns same mapping. So I can just not short circuit calls in case of permanent mappings. @whyrusleeping retrying it with no timeout doesn't cause any damage and has just a benefit, worst case it will fail again, best case it will work. |
|
Yeah, that seems like a good idea to me |
Kubuxu
added
need/review
|
Updated. |
Kubuxu
force-pushed
the
feat/nat-permanent-retry
branch
from
9405fd4
to
6eb03f1
Compare
For some NATs. Some NATs likely don't do that. Few things are always true in networking but "there is a middlebox that will do that" is a close one. Basically, try and try again in a variety of ways, and you'll succeed. |
Kubuxu
force-pushed
the
feat/nat-permanent-retry
branch
from
6eb03f1
to
0fc5e42
Compare
|
@lgierth @whyrusleeping does it look good now? |
| newport, err := nat.nat.AddPortMapping(m.Protocol(), m.InternalPort(), "http", MappingDuration) | ||
| comment := "libp2p" | ||
| if m.comment != "" { | ||
| comment = "libp2p-" + comment |
There was a problem hiding this comment.
@Kubuxu erm is there any change you meant comment = "libp2p-" + m.comment?
Kubuxu
force-pushed
the
feat/nat-permanent-retry
branch
from
0fc5e42
to
51d3953
Compare
| log.Debugf("Attempting port map: %s/%d", m.Protocol(), m.InternalPort()) | ||
| newport, err := nat.nat.AddPortMapping(m.Protocol(), m.InternalPort(), "http", MappingDuration) | ||
| comment := "libp2p" | ||
| if m.comment != "" { |
There was a problem hiding this comment.
what if we get back our own comment here? would this end up with "libp2p-libp2p-libp2p-libp2p...." ?
There was a problem hiding this comment.
there is m.commentand comment.
comment is temporary variable. I wanted to use context.WithValue to pass it but unfortunately goprocess doesn't have something like it.
There was a problem hiding this comment.
Ah, i see. I had assumed the comment field came from a prexisting NAT mapping on a router
|
@Kubuxu Not sure if we should try to fix it here or not, but we have a race condition in the old code: ipfs/kubo#3205 Can you check and see if a similar issue is present in the new code? |
|
I think the fix is to just lock around any time we use the 'nat' object |
|
The code didn't change so the issue still will be there. |
|
@Kubuxu hrm... i think for now the easiest thing will be to just lock around it. |
|
Those changes LGTM, but things don't appear to build |
|
Hmm, I will check if once more. |
Some hardware doesn't support UPnP with durations.
If we use same NAT agent and call for the same permanent mapping again we get the same mapping, no harm done. If router dies, we will remap again. Just pros, no cons.
Kubuxu
force-pushed
the
feat/nat-permanent-retry
branch
from
39eb32a
to
90eeff4
Compare
|
LGTM |
avoid spawning goroutines for canceled dials
use the assigned role when upgrading a sim open connection
Update github.com/flynn/noise to address nonce handling security issues
chore(dep): update deps
Some hardware doesn't support UPnP with durations.