podman Support + performance tunes + misc

[pablos-here]

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 Switch is enabled and there is no Internet: not checking for a bad value from dig. If this happens. we exit 1 from the script and we restart. I also set dig's +tries=1 (default is 3. This way we can fail straight away. Further, I exposed DIG_TIMEOUT for 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 the echos accordingly and added more calls to log_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 (podman users), I'm listing the devices the container knows about. For example, with podman, the tapped device is tun0. The script uses eth0 as that is what docker presents. eth0 can 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.sh into 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 openvpn was 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).

[wfg]

First off, I appreciate your interest in this project. :+1: Since this is a big PR, I'll hit all points individually:

Additionally, I fixed an edge-case when the Kill Switch is enabled and there is no Internet: not checking for a bad value from dig. If this happens. we exit 1 from the script and we restart.

Nice catch.

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 the echos accordingly and added more calls to log_msg(). I sprinkled a bit more instrumentation in the scripts to aid with possible user issues.

I like it. echo gets the job done, but I agree that a nicely-formatted log message is better.

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 (podman users), I'm listing the devices the container knows about. For example, with podman, the tapped device is tun0. The script uses eth0 as that is what docker presents. eth0 can be overridden by the user.

Good addition.

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.

Possibly even more controversial, moved big chunks of code in entry.sh into functions. The idea is to make the main body more readable and thus, easier to maintain.

Not too controversial.

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.)

---

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.

[pablos-here]

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.

[wfg]

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'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.

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.

[pablos-here]

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?

[wfg]

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. :)

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.

[pablos-here]

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!

[pablos-here]

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!

[wfg]

Without knowing how many users are currently using this functionality, I think there should be a phase-out period.

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.

[pablos-here]

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.

[pablos-here]

These changes capture our discussion:

• Reverted shell variables to how they were. :+1:
• Removed SNDBUF and RCVBUF as they fall into the expert-level tuning.

[wfg]

@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?

[pablos-here]

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.

[wfg]

Fair enough. I should've merged sooner to not waste your time.

What functionality was cut? The SOCKS5 proxy?

[pablos-here]

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.