Modify

Opened 9 years ago

Closed 7 years ago

#907 closed enhancement (fixed)

Amend OpenVPN client template to enable keepalive by default

Reported by: JonTheNiceGuy <jon@…> Owned by:
Priority: normal Milestone: Firmware 2.3.7.0
Component: fon-plugin-openvpn Version: N/A
Severity: normal
Cc: Hardware: both

Description

In /etc/openvpn/client.ovpn, add to the end:

keepalive 10 120

Attachments (3)

fon_keepalive.diff (3.0 KB) - added by jon@… 9 years ago.
Proposed patch for this issue.
fon_keepalive.2.diff (2.2 KB) - added by jon@… 9 years ago.
There were a few issues with the previous patch. Many thanks to blathijs on #fonosfera for pointing them out. I fixed these issues and spotted a couple more. Teamwork's great :)
fon_keepalive.3.diff (3.0 KB) - added by jon@… 9 years ago.
This file replaces the previous patch and provides Optional Server or Client end configurations, as well as providing some configuration around the frequency and delay around the keepalive values (as per previous comments). This has set the UCI values the way I was expecting it to :) For interest's sake, I am trying to get guidance on which value takes precidence - lowest value, server's value or something else.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 9 years ago by matthijs

What would be the usecase for this keepalive, and why did you choose the two values as you did?

comment:2 Changed 9 years ago by JonTheNiceGuy <jon@…>

This switch instructs the remote system to send a control-channel "ping" every 10 seconds, and to consider the remote system as down (and thus restart the connection) after 120 seconds.

I use this with systems which have non-reliable network connections, for example 3G sticks, or where the server end-point has a dynamic IP address which may change at very short notice, and it will keep the connection established through events like a router reboot.

The values are straight from the default configuration file.

comment:3 Changed 9 years ago by matthijs

  • Hardware changed from 2.0n (FON2300) to both
  • Milestone set to Firmware 2.3
  • Severity changed from unknown to normal
  • Status changed from new to confirmed

On the other hand, for people with a 3G stick that pay per MB, the keepalive option might cost them extra money. So I guess we should make this an option in luci...

Changed 9 years ago by jon@…

Proposed patch for this issue.

Changed 9 years ago by jon@…

There were a few issues with the previous patch. Many thanks to blathijs on #fonosfera for pointing them out. I fixed these issues and spotted a couple more. Teamwork's great :)

comment:4 Changed 9 years ago by matthijs

While working on upgrading OpenVPN, I noticed that the server already sends keepalives by default. I guess that these should be controlled by the same setting: It makes no sense to disable keepalives on the clients to save bandwidth but keep sending them from the server.

Also note that you've reused the "keepalive" option now: It was already passed to the openvpn server directly. Your current patch will thus pass --keepalive 1 to the openvpn server, instead of --keepalive 10 120 (at least I expect so, I did not test this).

This can probably exploited easily: Instead of making the values of the keepalive option "0" and "1" as in your patch, make them "" and "10 120". Then modify the client.sh script to just replace the value of the keepalive option instead of interpreting it as a boolean.

Finally, it might be good to make this option a tristate. e.g., Keepalive: never / often / seldom, so you can save bandwidth without completely disabling keepalives. With the above changes, this should be rather trivial to implement.

comment:5 Changed 9 years ago by JonTheNiceGuy <jon@…>

I guess you can tell my lack of knowledge about this stuff :)

I'll try and put something together tonight or tomorrow night in order to add a new diff. to this ticket.

Changed 9 years ago by jon@…

This file replaces the previous patch and provides Optional Server or Client end configurations, as well as providing some configuration around the frequency and delay around the keepalive values (as per previous comments). This has set the UCI values the way I was expecting it to :) For interest's sake, I am trying to get guidance on which value takes precidence - lowest value, server's value or something else.

comment:6 Changed 9 years ago by JonTheNiceGuy <jon@…>

Oops, forgot to update on here after the last Diff was uploaded.

The switch "--keepalive x y" actually sets "--ping x" and "--ping-restart y". It also checks whether it's in "server" mode, in which case it also sets "--push 'ping x'" and "--push 'ping-restart y'"

Therefore, setting keepalive on the server will override the client and force keepalive pings. Is it worth amending the Diff to reflect this information, or should we leave it as-is?

comment:7 Changed 7 years ago by matthijs

Hmm, it seems this patch somehow fell of my radar all that time ago, sorry for that...

Given that there's a number of OpenVPN improvements planned for 2.3.7.0 already, I think we should put this feature into that release.

If OpenVPN will actually push the keepalive settings to the client automatically, I think it would be best to not put the settings into the client configuration at all (and remove the keepalive_client setting altogether). It's best to minimize the number of options that affect the client configuration, since those require regenerating all client configurations, which might not always be obvious to users.

I'll go ahead and make these changes when committing the diff (hopefully somewhere next week).

One more question: We usually try to credit people that contribute patches in the svn log, using their full real names. If you are ok with this, would you tell me your real name?

comment:8 Changed 7 years ago by matthijs

  • Milestone changed from Firmware 2.3 to Firmware 2.3.7.0
  • Version changed from 2.3.6.1 (Gari jr.) to N/A

comment:9 Changed 7 years ago by Jon Spriggs <jon@…>

Sure, it's Jon Spriggs.

I would concur with stripping it from the client config - I've been experimenting with the new build, and applying it to my 2.0n and then running

uci set "openvpn.openvpn.keepalive=10 120"

This has done everything I was expecting it to, so I'd just probably just stick the top 6 lines of the patch for luci/applications/luci-openvpn/luasrc/model/cbi/openvpn/firewall.lua and ditch the rest of it.

comment:10 Changed 7 years ago by matthijs

I've made a few more changes, here's the updated patch:

diff --git a/luci/applications/luci-openvpn/luasrc/model/cbi/openvpn/firewall.lua b/luci/applications/luci-openvpn/luasrc/model/cbi/openvpn/firewall.lua
index 63812e8..f424605 100644
--- a/luci/applications/luci-openvpn/luasrc/model/cbi/openvpn/firewall.lua
+++ b/luci/applications/luci-openvpn/luasrc/model/cbi/openvpn/firewall.lua
@@ -20,4 +20,11 @@ x:value("0", translate("disable"))
 x:value("1", translate("enable"))
 x.default = require("luci.model.uci").cursor():get("openvpn", "openvpn", "wan") or "0"
 
+local x = s:option(ListValue, "keepalive", translate("openvpn_keepalive", "Send periodic keepalives"))
+x:value("0 0", translate("openvpn_keepalive_never", "Never"))
+x:value("10 120", translate("openvpn_keepalive_normal", "Normal (every 10 seconds)"))
+x:value("60 300", translate("openvpn_keepalive_infreq", "Reduced (every 60 seconds)"))
+x:value("600 1200", translate("openvpn_keepalive_rare", "Rare (every 10 minutes)"))
+x.default = require("luci.model.uci").cursor():get("openvpn", "openvpn", "keepalive") or "10 120"
+
 return m
diff --git a/luci/i18n/english/luasrc/i18n/default.en.lua b/luci/i18n/english/luasrc/i18n/default.en.lua
index 6eb2b8f..ca31d9c 100644
--- a/luci/i18n/english/luasrc/i18n/default.en.lua
+++ b/luci/i18n/english/luasrc/i18n/default.en.lua
@@ -121,6 +121,11 @@ openvpn_client_title = "Clients"
 openvpn_ddns = " You currently have no DynDns configured. <br>If you are on a dial-up line and your ip changes, you need to active DynDns."
 openvpn_desc = "Here you can configure your vpn server. VPN allows you to remotely connect to your home network from anywhere in the world."
 openvpn_generating = " Generating Keys"
+openvpn_keepalive = "Send periodic keepalives"
+openvpn_keepalive_never = "Never"
+openvpn_keepalive_normal = "Normal (every 10 secons)"
+openvpn_keepalive_rare = "Seldom (every 10 minutes)"
+openvpn_keepalive_reduced = "Reduced (every 60 secons)"
 openvpn_key = "OpenVPN is generating keys. This can take up to 10 minutes."
 openvpn_lan = "Local Network"
 openvpn_new_desc = "Please specify the name of this connection."

As you can see, I put the default back in, since as you commented on IRC, it's also done like that for the other options in the file (and since it doesn't matter much, let's go for consistency).

I've also changed the values a bit, now there is a really seldom option sending keepalive every 10 minutes. Does these look sensible to you?

I haven't actually tested anything about this yet, since I'm busy preparing the beta3 release. Perhaps you could see if the above works for you still?

comment:11 Changed 7 years ago by matthijs

w00ps, screwed up some of the translation string names. This one is better:

diff --git a/luci/applications/luci-openvpn/luasrc/model/cbi/openvpn/firewall.lua b/luci/applications/luci-openvpn/luasrc/model/cbi/openvpn/firewall.lua
index 63812e8..9104b47 100644
--- a/luci/applications/luci-openvpn/luasrc/model/cbi/openvpn/firewall.lua
+++ b/luci/applications/luci-openvpn/luasrc/model/cbi/openvpn/firewall.lua
@@ -20,4 +20,11 @@ x:value("0", translate("disable"))
 x:value("1", translate("enable"))
 x.default = require("luci.model.uci").cursor():get("openvpn", "openvpn", "wan") or "0"
 
+local x = s:option(ListValue, "keepalive", translate("openvpn_keepalive", "Send periodic keepalives"))
+x:value("0 0", translate("openvpn_keepalive_never", "Never"))
+x:value("10 120", translate("openvpn_keepalive_normal", "Normal (every 10 seconds)"))
+x:value("60 300", translate("openvpn_keepalive_reduced", "Reduced (every 60 seconds)"))
+x:value("600 1200", translate("openvpn_keepalive_seldom", "Seldom (every 10 minutes)"))
+x.default = require("luci.model.uci").cursor():get("openvpn", "openvpn", "keepalive") or "10 120"
+
 return m
diff --git a/luci/i18n/english/luasrc/i18n/default.en.lua b/luci/i18n/english/luasrc/i18n/default.en.lua
index 6eb2b8f..0810bcc 100644
--- a/luci/i18n/english/luasrc/i18n/default.en.lua
+++ b/luci/i18n/english/luasrc/i18n/default.en.lua
@@ -121,6 +121,11 @@ openvpn_client_title = "Clients"
 openvpn_ddns = " You currently have no DynDns configured. <br>If you are on a dial-up line and your ip changes, you need to active DynDns."
 openvpn_desc = "Here you can configure your vpn server. VPN allows you to remotely connect to your home network from anywhere in the world."
 openvpn_generating = " Generating Keys"
+openvpn_keepalive = "Send periodic keepalives"
+openvpn_keepalive_never = "Never"
+openvpn_keepalive_normal = "Normal (every 10 secons)"
+openvpn_keepalive_reduced = "Reduced (every 60 secons)"
+openvpn_keepalive_seldom = "Seldom (every 10 minutes)"
 openvpn_key = "OpenVPN is generating keys. This can take up to 10 minutes."
 openvpn_lan = "Local Network"
 openvpn_new_desc = "Please specify the name of this connection."

comment:12 Changed 7 years ago by Jon "The Nice Guy" Spriggs <jon@…>

There's a typo in default.en.lua - "secons" should be "seconds" but aside from that, it's all good.

comment:13 Changed 7 years ago by matthijs

  • Status changed from confirmed to testing-fix

I applied this patch locally, I'll push it out to svn later this week.

comment:14 Changed 7 years ago by matthijs

  • Resolution set to fixed
  • Status changed from testing-fix to closed

(In [2174]) luci-openvpn: Allow changing the keepalive settings.

Thanks to Jon Spriggs for this patch.

Closes: #907

Add Comment

Modify Ticket

Action
as closed The ticket will remain with no owner.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.