-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add -F flag for tcp fallback client config #193
Conversation
I think this will generate some confusion as users looking at the genconfig will expect it to just work, when in reality they'll need to know that they need to run two openvpn instances, one on UDP and one on TCP. Do that many people realllly want to do this? |
Every commercial VPN I've used has this behavior as the default. You already have documentation about the second container (https://github.com/kylemanna/docker-openvpn/blob/master/docs/tcp.md#running-a-second-fallback-tcp-container) and an entire test file (https://github.com/kylemanna/docker-openvpn/blob/master/test/tests/dual-proto/run.sh). Do you want me to change the usage to make it more clear? Edit: I've already needed this in some coffee shop environments while working. |
I plan to try this out and see if Tunnelblick works as well as Viscosity for handling the config files generated by it. It's a configuration that I'd like to see sorted out, either by further documenting of what this flag does (should you need to do it by hand), or coming up with an alternative. I can see the value of generating two separate configs with no fallback (udp-only, tcp-only) sharing the same data volume for credentials, and a 3d config with automatic fallback. But to make this work you need to explicitly update some docs to point to the dual-container setup. |
@ryansch I completely understand your use case. But, by just turning on the flag, it doesn't do what it suggests. Users need to run another instance. What will result, is if this this will get merged there will be more Github issues along the lines of This Github repo is catering to the simple mainstream use case. If it can't be enabled simply, but requires an extra steps (i.e. running another instance and baby sitting that) then its much simpler for the end user to modify the configuration file to add the one line they need on their one laptop that runs in to the problem. That's why it's documented, for those who can read and figure things out. If you look at all the other options (with the exception of 2FA maybe, but that updates the server config, not the client config), they standalone without external help. This addition would be the first that requires the user to do something additional to enable the feature. I think to merge the feature, there would need to be a way to simply and cleanly turn on the TCP instance. Unfortunately I have the philosophy of one process per docker container, so that sounds impossible within the container. Thoughts? |
One useful approach which might be code or documentation is the docker-compose config file that would launch a pair of containers, one for UDP, one for TCP. Generating that through code would allow keeping the one process per Docker container but still orchestrating just enough to get everything running. |
I'm a fan of docker-compose, that helps the orchestration for sure. |
We have to choose either orchestration for two containers or break the one process per container rule. Personally I'm also a fan of using orchestraion and I'm using rancherOS to launch these containers in production via the compose-ish format they support in cloud config. In this example I'm also using a storage container as rancherOS doesn't yet support compose v2. openvpn-storage:
image: outstand/openvpn:storage
environment:
- S3_BUCKET=${openvpn_bucket}
openvpn:
image: outstand/openvpn:latest
ports:
- '1194:1194/udp'
volumes_from:
- openvpn-storage
cap_add:
- NET_ADMIN
restart: always
openvpn-tcp:
image: outstand/openvpn:latest
command: ovpn_run --proto tcp
ports:
- '443:1194/tcp'
volumes_from:
- openvpn-storage
cap_add:
- NET_ADMIN
restart: always Would you expect this PR to generate a docker-compose v2 file? How do we instruct the user to run docker-compose? |
Ping @kylemanna. Do you want to voice/video chat this out? |
@ryansch I really don't think there is much to discuss. Unless this can be simply and fully implemented, it won't get merged. Also, the docker container is staying a single process for the other significant majority of users. Also, with running two containers, with the same config files / docker volume, I fear it won't be quite that simple and people will risk IP address collisions as I don't know how the OpenVPN daemon would coordinate between themselves. I was leaving it open for further discussion to see if anyone had suggestions for something clever. Perhaps someone wants to push an upstream feature addition to OpenVPN itself to listen on two sockets? |
@kylemanna Could we then discuss another way to accomplish this that doesn't require me to hand-edit all of the client config files before I hand them out? |
Also as far as running two containers, it's literally as easy as running both of them and mapping the ports as in my example. |
@ryansch I don't think you're following what I'm saying. I do not have the answer for you, I gave you the outline of what I would like to see, and you're still making this my problem. I'm closing this pull request as it has become unproductive. |
@kylemanna I think we've gotten our wires crossed. I'm certainly not trying to get you to do work for free or use up your time. To be clear, I want to write these features and documentation but I need to know what direction you want the project to go in. I don't want to put a bunch of work into something that you don't want and I understand that once something gets merged it becomes your responsibility. To that end would you consider a version of this PR with the docker-compose setup and clear documentation on how to use it? As a second option, would you consider a PR that adds the ability to add lines to the client config (like the |
If I'm understanding the problem correctly,
Without this PR, it is currently possible to add any options in a server configuration using Adding what @ryansch is suggesting in his last comment,
would add a lot of flexibility to those who know what they are doing. So, I would +1 adding a simple |
Thanks @slamont. I was leaning towards that solution as well. I'll get a PR going. |
This lets me generate client config that will fallback to the tcp container as described here: https://github.com/kylemanna/docker-openvpn/blob/master/docs/tcp.md#running-a-second-fallback-tcp-container
Resulting client config has two remote lines. Viscosity gracefully handles this by trying both of them in order.
Replaces #173