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

MYST-726 Make openvpn tunnel setup mandatory for process #331

Merged

Conversation

Projects
None yet
5 participants
@Waldz
Copy link
Member

commented Sep 9, 2018

No description provided.

@Waldz Waldz requested review from tadovas, soffokl, zolia and mysteriumnetwork/core Sep 9, 2018

@Waldz Waldz force-pushed the Waldz:feature/MYST-726-openvpn-tunnel-setup branch from 5eaa73b to 6828c31 Sep 9, 2018

@@ -25,7 +25,11 @@ import (
)

// CreateNewProcess creates new openvpn process with given config params
func CreateNewProcess(openvpnBinary string, config *config.GenericConfig, middlewares ...management.Middleware) Process {
func CreateNewProcess(

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 10, 2018

Member

It feels like one-line function declaration was more readable for me.

This comment has been minimized.

Copy link
@zolia

zolia Sep 11, 2018

Member

it is better shorter line to comply with 80 symbols length style.

This comment has been minimized.

Copy link
@zolia

zolia Sep 11, 2018

Member

I would just call it 'NewProcess', Create and New its two of the same.

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 11, 2018

Member

I think 80 symbols lines is not a limit anymore:
Here is a mention about it in the Effective Go:

Line length
Go has no line length limit. Don't worry about overflowing a punched card.

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 11, 2018

Member

But humans do have line length limit, it's all about readability I guess

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 11, 2018

Member

Yep, that is why I mentioned that single line is looking more readable for me.

And another link to the line length in golang: https://github.com/golang/go/wiki/CodeReviewComments#line-length

func CreateNewProcess(openvpnBinary string, config *config.GenericConfig, middlewares ...management.Middleware) Process {
return NewLinuxProcess(openvpnBinary, config, middlewares...)
// CreateNewProcess function creates Linux OS customized openvpn process
func CreateNewProcess(

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 10, 2018

Member

The same here:

It feels like one-line function declaration was more readable for me.

import "github.com/mysteriumnetwork/node/openvpn/config"

// NewNoopSetup creates no-op tunnel setup
func NewNoopSetup() *noopSetup {

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 10, 2018

Member

Maybe just make the struct public, if we are doing nothing in the creation function?

This comment has been minimized.

Copy link
@zolia

zolia Sep 11, 2018

Member

👍 I also think we should move to such style. Especially for not over complicated initialisation of structs.

Stop()
type tunnelSetup interface {
Setup(config *config.GenericConfig) error
Teardown()
}

type openvpnProcess struct {

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 10, 2018

Member

As far as I remember OpenvpnProcess is part of public api ( it's returned from package to outside world) and can be safely made public. As general approach - structures returned by New.. functions should always be exported ones. Even linter mentions that.

This comment has been minimized.

Copy link
@zolia

zolia Sep 11, 2018

Member

👍 I think the same

waiter.Add(1)
go func() {
defer waiter.Done()
openvpn.tunnelSetup.Teardown()

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 10, 2018

Member

This tunnel tear down must be called ONLY AFTER openvnp process is completed, to avoid funny openvpn or tunnel setup fail errors. It cannot be go routined.

@@ -15,15 +15,16 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

package linux
package tunnel

This comment has been minimized.

Copy link
@zolia

zolia Sep 11, 2018

Member

tun device imho can be decoupled from openvpn. Other transports also could use tun device management.

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 11, 2018

Contributor

I don't think this single file deserves to be a library on its own, especially since we're moving openvpn away from the node repo. Can we leave it here for now and if needed split at a later date?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 11, 2018

Author Member

Both ideas is good, but lets do it then we need to reuse

}

func (service *serviceLinuxTun) Stop() {
func (service *serviceLinuxTun) Teardown() {

This comment has been minimized.

Copy link
@zolia

zolia Sep 11, 2018

Member

Why not Stop? Simpler naming is way of go..

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 11, 2018

Member

I don't know why we are calling it tun service at all, it's simple device manager with CreateNewDevice, RemoveDevice

func CreateNewProcess(openvpnBinary string, configuration *config.GenericConfig, middlewares ...management.Middleware) *OpenvpnProcess {
configuration.SetScriptParam("iproute", config.SimplePath("nonpriv-ip"))

tunnelSetup := tunnel.NewTunInterfaceSetup(

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 11, 2018

Author Member

+1

@Waldz

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2018

LGTM

package tunnel

// GetTunnelSetup sets the required configuration options for the tunnel and returns a Setup for it
func GetTunnelSetup() Setup {

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 12, 2018

Author Member

We tend to call factories New*() or new*()

)

type tunnelSetup interface {

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 13, 2018

Author Member

You reverted 2 improvements

  • Interface should be on caller side. Not in implementation
  • Implementation factory should return not interface but exact structs new() return *SetupTunInterface

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 13, 2018

Author Member

@tadovas, @sofokkl we kind of agreed to continue like this. But now we have 2 conventions and that missleads

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 13, 2018

Member

In this case the only option is to return interface, if we want to choose os specific implementation on compile time, and hide those details from process (generic tun setup on mac, win) (specific tun on linux),
On the other hand, we can have two functions with the same name and conditionaliy compiled, but with different return values, i.e. NewTunnelSetup() * LinuxTunnel NewTunnelSetup() * GenericTunnel, but I think it will hurt readability as the same function will return different result on different OS. I would vote for interface declaration on implementor side in this particular case.

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 13, 2018

Member

Return structures depend on interfaces is generic golang approach and it's fine, but there are edge cases where you have to choose :) I think it's the case

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 15, 2018

Author Member

..if we want to choose os specific implementation on compile time..

To solve this i moved conditional factory to upper layer:

  • caller defines interface TunnelSetup 84b2b3b

  • caller decides implementation to go for Linux/ARM because he knows where he is compiling

  • Caller can construct SetupTunInterface in ARM. And now is made private

This was idea - to make tunnel package independent from our process so that someone can use just Tunnel seperately.

@vkuznecovas vkuznecovas force-pushed the Waldz:feature/MYST-726-openvpn-tunnel-setup branch from b575ae1 to b343fc9 Sep 14, 2018

Waldz and others added some commits Sep 7, 2018

Dont do heavy stuff in tunnel factory
# Conflicts:
#	openvpn/linux/device.go
#	openvpn/linux/linux_tun_service.go
#	openvpn/linux_process.go
exported OpenvpnProcess
exported NoopSetup
newProcess now takes tunnel setup as a parameter
OpenvpnProcess.Stop() now waits for the openvpn to stop before closing the tunnel
inlined CreateNewProcess params
renamed tunnel terminate -> tunnel stop
renamed serviceLinuxTun -> tunDeviceManager

@vkuznecovas vkuznecovas force-pushed the Waldz:feature/MYST-726-openvpn-tunnel-setup branch from b343fc9 to 533445d Sep 17, 2018

@tadovas
Copy link
Member

left a comment

LGTM

@tadovas
Copy link
Member

left a comment

reLGTMed

@zolia

zolia approved these changes Sep 17, 2018

@vkuznecovas vkuznecovas merged commit 97150f0 into mysteriumnetwork:master Sep 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Waldz Waldz deleted the Waldz:feature/MYST-726-openvpn-tunnel-setup branch Sep 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.