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

Send a keep alive signal every second to keep UDP tunnels from collapsin... #45

Open
wants to merge 6 commits into
base: 1.1
Choose a base branch
from

Conversation

vollkorn1982
Copy link

...g

Some routes might drop UDP tunnels through NAT firewalls after a very
short time frame. This leads to data being sent to the corresponding port
being lost. To prevent this every second a small mtu probe packet is sent
in order to keep the tunnel open and working. This does not eliminate the
possibility of packet loss due to eager routers clearing their tables, but
reduces the packet loss significantly.

jan added 6 commits July 30, 2014 14:06
…sing

Some routes might drop UDP tunnels through NAT firewalls after a very
short time frame. This leads to data being sent to the corresponding port
being lost. To prevent this every second a small mtu probe packet is sent
in order to keep the tunnel open and working. This does not eliminate the
possibility of packet loss due to eager routers clearing their tables, but
reduces the packet loss significantly.
This state machine replaces the rather complex and hard to mainain code
from before. Through this preparation step tinc can be easily extended to
implement a keepalive signal.
@dechamps
Copy link
Contributor

I see you're trying to improve the MTU discovery state machine in your patch, which is indeed welcome, but I think there's an even better way to approach this. When we get down to it, we can observe that the MTU probing code has two responsibilities: (1) ensure that the UDP tunnel is usable for minmtu-sized packets, and (2) try to increase minmtu (by sending probes larger than minmtu).

Currently these two responsibilities are intermingled in the same MTU probing code, but in my opinion, they don't need to be. It should be perfectly fine to have "UDP tunnel testing and keepalive" and "MTU probing" be handled by two separate and relatively independent pieces of code, which would make the whole thing clearer and would end up removing (or at least greatly simplifying) the state machine that you are trying to improve in your patch.

I'm currently trying to code a patch that implements the approach I'm suggesting.

@vollkorn1982
Copy link
Author

Hi Etienne,

Am 28.12.2014 um 13:07 schrieb Etienne Dechamps:

Currently these two responsibilities are intermingled in the same MTU probing code, but in my opinion, they don't need to be. It should be perfectly fine to have "UDP tunnel testing and keepalive" and "MTU probing" be handled by two separate and relatively independent pieces of code, which would make the whole thing clearer and would end up removing (or at least greatly simplifying) the state machine that you are trying to improve in your patch.

that sounds like a resonable idea. It took me a while understanding the
idea of the state machine and some more to rewrite it like I did.

Cheers
vollkorn

@dechamps
Copy link
Contributor

dechamps commented Jan 2, 2015

I've filed #61. As you can imagine, it goes in a completely different direction compared to your pull request.

Unfortunately, sending keepalives was not part of my motivations when writing #61 - in fact, it probably makes it worse in that regard. That said, if you set UDPDiscoveryTimeout from my pull request to an aggressive value, it will force tinc to assume that a UDP tunnel that has not seen any activity in the last few seconds should not be relied upon. That should fix your problem, as long as the inactivity timeout on your NAT is not set crazy low.

@mark-stopka
Copy link

So should this be addressed further, or closed as non-relevant anymore?

@vollkorn1982
Copy link
Author

I did not test if the code in #61 does help this problem and won't do so anymore, since I'm not working with tinc anymore. But if the code does what @dechamps says it would do, then I think the problem is solved since the original problem was lost communication when connections dropped out.

@fangfufu fangfufu added 1.1 Issue related to Tinc 1.1 enhancement New feature requests or performance improvement. merge_conflict labels Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.1 Issue related to Tinc 1.1 enhancement New feature requests or performance improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants