podman Support + performance tunes + misc #98
Conversation
Matches openvpn. o With little CPU cost, make availability sooner by reducing tun0, poll sleep from 1 to 0.5 o Standardize shell variables to be uppercase. o Tacked 'exit $?' at the end, though never reached. :)
Matches openvpn. o With little CPU cost, make availability sooner by reducing tun0, poll sleep from 1 to 0.5 o Standardize shell variables to be uppercase. o Tacked 'exit $?' at the end, though never reached. :)
Matches openvpn. o Standardize shell variables to be uppercase. o Refactored cleanup() to wait up to 15s for the graceful termination of openvpn. o Re-worked how the modified config file is handled. If $DEBUG_VPN_CONFIG_FILE is set, expose it to the caller via /data/vpn, otherwise it's written to /data/tmp o Improved the clarity of the output by creating headings, etc. o Improve code readabliity: Migrated functional sections of code into shell functions o Addressed the following issues: #93, #94 and #97 o For wireless connections mute replay warnings via --mute-replay-warnings o Force no compression via '--comp-lzo no' as compression is a potential vulnerability. o While never reached, tacked a call to cleanup() and 'exit 0' at the end.
|
First off, I appreciate your interest in this project. 👍 Since this is a big PR, I'll hit all points individually:
Nice catch.
I like it.
Good addition.
I'd like to keep it mixed. When I write scripts, I reserve uppercase variables for environment variables and variables local to the script will be lowercase. When I see
Not too controversial.
My thinking on tunables is that I would rather have users set them in their configuration files since they have to supply them anyways. As far as I can tell you could technically run OpenVPN without a config file at all, having everything passed in as command line arguments. So unless you expose everything as a tunable, you are in a weird spot where only an arbitrary set of options are available, and you leave the door open for "can you add With all of that said, I'd like you to take a look at the rewrite branch. I personally have a lot of problems with the Dante proxy, and HTTP proxies are useless to me since SOCKS5 proxies exist. I've also figured out significantly cleaner way to handle the kill switch. For these reasons I'm eventually going to merge the rewrite branch to cut the proxies entirely and clean up the iptables rules. While orthogonal to this discussion, I would like to point out that I also maintain a WireGuard version of this image. At this point, I exclusively use the WireGuard image (which already does not use embedded proxies) and an additional container that is a dedicated SOCKS5 proxy using WireGuard's network stack. I am very happy with my current personal configuration. I'm curious about your thoughts on WireGuard. |
|
On 2023-02-05 17:41, Wyatt Gill wrote:
First off, I appreciate your interest in this project. 👍
YW.
Since this is a big PR, I'll hit all points individually:
+1
For brevity, I'm snipping only what we should discuss. :)
I hope it's not too controversial but I made all lowercase shell
variables uppercase. The script had a mix.
I'd like to keep it mixed. When I write scripts, I reserve uppercase
variables for environment variables and variables local to the script
will be lowercase. When I see |$VARIABLE| used, I assume it is/can be
set as an environment variable.
The response below supports your position. In particular, I forgot that
variables names are case-sensitive. We can avoid name-space
collisions. I like it!
https://stackoverflow.com/questions/673055/correct-bash-and-shell-script-variable-capitalization
I exposed some performance tunables as well as tuneables to reduce
the VPN availability lag on an internet hiccup.
My thinking on tunables is that I would rather have users set them in
their configuration files since they have to supply them anyways. As
far as I can tell you could /technically/ run OpenVPN without a config
file at all, having everything passed in as command line arguments. So
unless you expose /everything/ as a tunable, you are in a weird spot
where only an arbitrary set of options are available, and you leave
the door open for "can you add |--flag-no-one-has-ever-heard-of| as an
option?". For simplicity's sake, I'd like to go the other direction:
let users handle all the tunables in their config files. (Having said
that, |VPN_LOG_LEVEL| should be removed as an option and handled via
the config file.)
If our target user-base includes non-experts, we might want to expose
best-of tuneables and let the experts dink with the .ovpn files.
With the above approach, we can shove the
|--flag-no-one-has-ever-heard-of| into the *expert* bucket, while still
exposing tuneables.
In my Use Case, my VPN generates my .ovpn's. Ideally, I would prefer to
treat them read-only and use the tunebles to accordingly.
If we opt with the hybrid model, we can keep |VPN_LOG_LEVEL|. I think
it's good for user support.
Please let me know what you think.
------------------------------------------------------------------------
With all of that said, I'd like you to take a look at the rewrite
branch <https://github.com/wfg/docker-openvpn-client/tree/rewrite>. I
personally have a lot of problems with the Dante proxy, and HTTP
proxies are useless to me since SOCKS5 proxies exist. I've also
figured out significantly cleaner way to handle the kill switch. For
these reasons I'm eventually going to merge the rewrite branch to cut
the proxies entirely and clean up the iptables rules.
I'd be happy to have a look. What in particular would you like me to
review?
I should set your expectation that my field of expertise is not
VPN/proxy. :)
While orthogonal to this discussion, I would like to point out that I
also maintain a WireGuard version
<https://github.com/wfg/docker-wireguard> of this image. At this
point, I exclusively use the WireGuard image (which already does not
use embedded proxies) and an additional container that is a dedicated
SOCKS5 proxy using WireGuard's network stack
<https://github.com/wfg/docker-wireguard#using-with-other-containers>.
I am very happy with my current personal configuration. I'm curious
about your thoughts on WireGuard.
Generally I treat VPNs as fire-and-forget. Once they're set up, I press on.
Thank you for asking me to look at WireGuard. A quick web-search shows
that it's a nice, light-weight, performant solution. I also like the
UNIX-y thing you're doing by breaking up the needs: a SOCKS5 container
+ WireGurade container.
After my PR. I think I'll dink around with what you're using. I may
switch as my VPN provider generates WireGuard configs.
|
I see your point, but instead of having users add Treating your config files as read-only is a little trickier though. Exposing tunables is probably the best/only option for this use case.
Primarily just the relative simplicity of it. Since I'm going to merge it eventually, it may be best to work from that branch instead of master.
I found WireGuard faster and more reliable than OpenVPN. I'd recommend giving it a try :). At this point, I'm just maintaining ("maintaining" used loosely here) this image for the people. |
|
On 2023-02-05 18:47, Wyatt Gill wrote:
If our target user-base includes non-experts, we might want to expose
best-of tuneables and let the experts dink with the .ovpn files.
I see your point, but instead of having users add |--env
SNDBUF=524288| to the |docker run| command/Compose file, I think
having them add |sndbuf 524288| to their config file is easier.
Treating your config files as read-only is a little trickier though.
Exposing tunables is probably the best/only option for this use case.
I'm going to nuke SNDBUF/RCVBUFF as I think that's in the expert-zone. :)
I'd be happy to have a look. What in particular would you like me to
review?
Primarily just the relative simplicity of it. Since I'm going to merge
it eventually, it may be best to work from that branch instead of master.
Thank you for the heads up.
When are you planning on doing the merge?
I think I'll dink around with what you're using. I may
switch as my VPN provider generates WireGuard configs.
I found WireGuard faster and more reliable than OpenVPN. I'd recommend
giving it a try :). At this point, I'm just maintaining ("maintaining"
used loosely here) this image for the people.
I believe my future is soon to be WireGuard! *grin*
My guess is that not all VPN providers support WireGuard which is why
this project is still live eh?
|
I might as well do it tonight I guess. Would you be able to test the changes on your end tonight to make sure I didn't break anything? Nothing looks broken to me, but twice as many eyes on it would be nice. :)
A combination of that plus all the people that either don't know about or don't want to use WireGuard. There's not much to do maintenance-wise (especially after the rewrite merge), so there's little reason to kill it off. |
|
On 2023-02-05 19:54, Wyatt Gill wrote:
When are you planning on doing the merge?
I might as well do it tonight I guess.
+1
Would you be able to test the changes on your end tonight to make sure
I didn't break anything?
As I use podman, I'd have to bake some of my changes before I could
test. I'm somewhat done for the night. :) I'll hit it tomorrow after
a few tasks.
If for some reason something goes sideways, how'd you like me to let you
know?
Nothing looks broken to me, but twice as many eyes on it would be nice. :)
+1!
My guess is that not all VPN providers support WireGuard which is why
this project is still live eh?
A combination of that plus all the people that either don't know about
or don't want to use WireGuard. There's not much to do
maintenance-wise (especially after the rewrite merge), so there's
little reason to kill it off.
Makes sense.
Thx a lot!
|
|
On 2023-02-05 19:54, Wyatt Gill wrote:
When are you planning on doing the merge?
I might as well do it tonight I guess. Would you be able to test the
changes on your end tonight to make sure I didn't break anything?
Nothing looks broken to me, but twice as many eyes on it would be nice. :)
Hi again!
I was curious so I peeked at the branch.
I just realized that when you said before that you were planning on
cutting the proxies entirely, you mean "I'm removing the functionality
from the container."
Without knowing how many users are currently using this functionality, I
think there should be a phase-out period.
As an example, I am currently using the SOCKS5 functionality.
Now, I have to go ... l8r!
|
These changes will be v4.0.0, and I think I'll stop offering a |
|
These changes will be v4.0.0, and I think I'll stop offering a
|:latest| tag now since I think they're dangerous for the non-expert
users. With this, people would have to explicitly pull |:4| to use the
rewrite because |:latest| would be stuck on |:3.1.0|.
You might consider having two containers: the all-in-one and the the
focused.
|
file: exist and non-zero length.
|
These changes capture our discussion:
|
|
@pablos-here Following the rewrite merge, all the changes are to files that have been heavily modified and moved. I'm going to close this PR. Would you mind resubmitting with your changes against the latest files? |
|
On 2023-02-08 20:03, Wyatt Gill wrote:
@pablos-here <https://github.com/pablos-here> Following the rewrite
merge, all the changes are to files that have been heavily modified
and moved.
Indeed and it is unfortunate.
I'm going to close this PR.
Okay.
Would you mind resubmitting with your changes against the latest files?
As the rewrite branch lops off functionality that I need, and throws
away some of the work I've done, I won't be resubmitting a PR.
For now, I have something that is working.
|
|
Fair enough. I should've merged sooner to not waste your time. What functionality was cut? The SOCKS5 proxy? |
|
On February 8, 2023 20:35:57 Wyatt Gill ***@***.***> wrote:
What functionality was cut? The SOCKS5 proxy?
A gap analysis is the proper way to answer the above.
|
Hi,
In retrospect, I think I should have created a smaller PR to avoid this monolithic PR. I'm terribly sorry about it. If need be, please reject it and I'll resubmit it piecemeal. I'll use it as a learning experience. :)
This PR addresses the following issues: #97, #94 and #93.
Additionally, I fixed an edge-case when the
Kill Switchis enabled and there is no Internet: not checking for a bad value fromdig. If this happens. weexit 1from the script and we restart. I also setdig's+tries=1(default is 3. This way we can fail straight away. Further, I exposedDIG_TIMEOUTfor users to tune.I created a log_msg() function in each of the core scripts to standardize the output:
YYYY-MM-DD 24H:MI:SS [scriptname]. I replaced theechos accordingly and added more calls tolog_msg(). I sprinkled a bit more instrumentation in the scripts to aid with possible user issues.I've tried to make the output a bit more user-friendly by listing the user parameters at the top in a section. For end-user debugging purposes (
podmanusers), I'm listing the devices the container knows about. For example, withpodman, the tapped device istun0. The script useseth0as that is whatdockerpresents.eth0can be overridden by the user.I hope it's not too controversial but I made all lowercase shell variables uppercase. The script had a mix. Possibly even more controversial, moved big chunks of code in
entry.shinto functions. The idea is to make the main body more readable and thus, easier to maintain.I exposed some performance tunables as well as tuneables to reduce the VPN availability lag on an internet hiccup.
On termination, I noticed that the child
openvpnwas not given adequate time to clean up (sleep 0.5). I've implemented a simple check with fail-safe to allow for a graceful termination (and tear down).