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

Add -F flag for tcp fallback client config #193

Closed
wants to merge 2 commits into from

Conversation

ryansch
Copy link
Contributor

@ryansch ryansch commented Jan 3, 2017

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

@kylemanna
Copy link
Owner

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?

@ryansch
Copy link
Contributor Author

ryansch commented Jan 3, 2017

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.

@vielmetti
Copy link
Contributor

vielmetti commented Jan 4, 2017

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.

@kylemanna
Copy link
Owner

@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 fallback mode not working as expected and I'll have to answer them after I spend time remind myself when, where and why fall back mode got introduced.

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?

@vielmetti
Copy link
Contributor

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.

@kylemanna
Copy link
Owner

I'm a fan of docker-compose, that helps the orchestration for sure.

@ryansch
Copy link
Contributor Author

ryansch commented Jan 4, 2017

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?

@ryansch
Copy link
Contributor Author

ryansch commented Jan 9, 2017

Ping @kylemanna. Do you want to voice/video chat this out?

@kylemanna
Copy link
Owner

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

@ryansch
Copy link
Contributor Author

ryansch commented Jan 10, 2017

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

@ryansch
Copy link
Contributor Author

ryansch commented Jan 10, 2017

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.

@kylemanna
Copy link
Owner

@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 kylemanna closed this Jan 10, 2017
@ryansch
Copy link
Contributor Author

ryansch commented Jan 10, 2017

@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 -e option for server config)?

@slamont
Copy link
Contributor

slamont commented Jan 26, 2017

If I'm understanding the problem correctly,

  • @ryansch need a way to generate an openvpn config for a client, that would have more than one Server defined in it.
  • @kylemanna Doesn't want a complicated solution that would confuse users

Without this PR, it is currently possible to add any options in a server configuration using -e the_option

Adding what @ryansch is suggesting in his last comment,

would you consider a PR that adds the ability to add lines to the client config (like the -e option for server config)

would add a lot of flexibility to those who know what they are doing.

So, I would +1 adding a simple -e style option to client config generation script and I would not add -F as I would use -e for the server config

@ryansch
Copy link
Contributor Author

ryansch commented Jan 27, 2017

Thanks @slamont. I was leaning towards that solution as well. I'll get a PR going.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants