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

Fail fast if could not locate openvpn binary on both client and node #187

Merged
merged 6 commits into from Feb 27, 2018

Conversation

Projects
None yet
3 participants
@tadovas
Copy link
Member

commented Feb 27, 2018

No description provided.

@@ -34,11 +34,17 @@ type Command struct {
vpnServerFactory func(sessionManager session.Manager, serviceLocation dto_discovery.Location,
providerID identity.Identity, callback state.Callback) *openvpn.Server

vpnServer *openvpn.Server
vpnServer *openvpn.Server
openvpnBinaryCheck func() error

This comment has been minimized.

Copy link
@donce

donce Feb 27, 2018

Contributor

Why not checkOpenvpn as in client command?

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 27, 2018

Author Member

Saw this after PR was submitted. Will make the names the same

)

//Version function checks that openvpn is available, given path to openvpn binary
func Version(openvpnBinary string) error {

This comment has been minimized.

Copy link
@donce

donce Feb 27, 2018

Contributor

Function name declares implementation, not the purpose of this function. CheckOpenVPNBinary ?

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 27, 2018

Author Member

The idea was to make a function whchi returns (string, error) with string as openvpn version and error if exec somehow fails. But for the moment yes - it can be named CheckOpenVpnBinary

exitCode, err := extractExitCodeFromCmdResult(cmdResult)
//openvpn returns exit code 1 in case of --version paramter, if anything else is returned - treat as error
if err != nil || exitCode != 1 {
return err

This comment has been minimized.

Copy link
@donce

donce Feb 27, 2018

Contributor

In case err == nil and exitCode == 0, you'll return nil, which means that Version check succeeded. A separate error should be returned in that case, i.e.

if err != nil {
  return err
}
if exitCode != 1 {
  return errors.New("Oh shit!")
}

Maybe this indicates a need for unit-test? ;)

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 27, 2018

Author Member

Yes and yes :) it could be unittested

cmdResult := process.Wait()

exitCode, err := extractExitCodeFromCmdResult(cmdResult)
//openvpn returns exit code 1 in case of --version paramter, if anything else is returned - treat as error

This comment has been minimized.

Copy link
@zolia

zolia Feb 27, 2018

Member

It will rarely will be something else on error than '1', in theory we should parse and look for version.. but prob, we can skip this for now.. :/

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 27, 2018

Author Member

That's the idea of this PR - to check that path to openvpn was correct. Exact openvpn version maching and even freeze is out of scope for this PR and I not sure that its needed.
This exitCode == 1 test is simply to disable Error messages that executable exited with status non-zero which does openvpn given version parameter

@tadovas

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2018

All comments have been addressed

return 0, cmdResult
}

exitStatus, ok := exitError.Sys().(syscall.WaitStatus)

This comment has been minimized.

Copy link
@zolia

zolia Feb 27, 2018

Member

Question is if it will behave the same for Windows..

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 27, 2018

Author Member

Confirmed by guys in stackoverflow. Works on windows and linux. And I just tried on MacOS :)

#!/usr/bin/env bash

echo "Some output"
exit 1

This comment has been minimized.

Copy link
@donce

donce Feb 27, 2018

Contributor

Newline

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 27, 2018

Author Member

Fixed.

@@ -0,0 +1,3 @@
#!/usr/bin/env bash
echo "Some output"
exit 0

This comment has been minimized.

Copy link
@donce

donce Feb 27, 2018

Contributor

Newline.

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 27, 2018

Author Member

Fixed.


func TestNoErrorIsReturnedOnExitCodeOne(t *testing.T) {
assert.NoError(t, CheckOpenvpnBinary("testdata/exit-with-one.sh"))
}

This comment has been minimized.

Copy link
@donce

donce Feb 27, 2018

Contributor

Nice ;)

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 27, 2018

Author Member

Fixed.

"syscall"
)

//CheckOpenvpnBinary function checks that openvpn is available, given path to openvpn binary

This comment has been minimized.

Copy link
@donce

donce Feb 27, 2018

Contributor

Space after //

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 27, 2018

Author Member

Fixed.

)

//CheckOpenvpnBinary function checks that openvpn is available, given path to openvpn binary
func CheckOpenvpnBinary(openvpnBinary string) error {

This comment has been minimized.

Copy link
@donce

donce Feb 27, 2018

Contributor

Shouldn't we use abbrevation naming conventions for vpn, i.e. CheckOpenVPNBinary?

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 27, 2018

Author Member

Well its not an abrevation but more well known trademark "openvpn".

@donce

donce approved these changes Feb 27, 2018

@tadovas tadovas merged commit f737383 into master Feb 27, 2018

2 checks passed

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

@tadovas tadovas deleted the improvement/check-opevpn-on-client-node-startup branch Feb 27, 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.