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-102 client connection manager #65

Merged
merged 26 commits into from Dec 21, 2017

Conversation

Projects
None yet
4 participants
@tadovas
Copy link
Member

commented Dec 19, 2017

Last portion of migrating client to daemon world and exposing local api

tadovas added some commits Dec 15, 2017

Merge branch 'feature/MYST-102-client-connection-manager' of github.c…
…om:MysteriumNetwork/node into feature/MYST-102-client-connection-manager

* 'feature/MYST-102-client-connection-manager' of github.com:MysteriumNetwork/node:
  A bit of renaming
  Finally found a way to move parenthesis to new line
Merge branch 'master' of github.com:MysteriumNetwork/node into featur…
…e/MYST-102-client-connection-manager

* 'master' of github.com:MysteriumNetwork/node:
  fixed identity cache tests
Merge branch 'master' of github.com:MysteriumNetwork/node into featur…
…e/MYST-102-client-connection-manager

* 'master' of github.com:MysteriumNetwork/node:
  removed unused imports
Merge branch 'master' of github.com:MysteriumNetwork/node into featur…
…e/MYST-102-client-connection-manager

* 'master' of github.com:MysteriumNetwork/node:
  Do address connection during factoring
  Drop nested address factory
  Mock Connection to dialog tests
  Add Close to mocked NATS connection
  Use Connection interface in NATS Address
  Mock NATS connection it tests

@Waldz Waldz changed the title Feature/myst 102 client connection manager MYST-102 client connection manager Dec 20, 2017

}

func (tc *test_context) TearDownTest() {

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 20, 2017

Member

Is this needed?

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member

Should not You destroy some resources gracefully?

  • connManager.Stop()
  • fakeOpenVpn.Stop()
cmd.clientCommand, err = command_client.NewCommand(command_client.CommandOptions{
DirectoryRuntime: options.DirectoryRuntime,
})

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 20, 2017

Member

Unneeded newline before error handling

@tadovas

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2017

Fixed

cmd.httpApiServer.Stop()
}

func configureVpnClientFactory(mysteriumApiClient server.Client, vpnClientRuntimeDirectory string) client_connection.VpnClientFactory {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member

Looks like Openvpn specific logic, so I expect it to be next to vpnConnectionManager

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 20, 2017

Author Member

It can be moved

cmd.httpApiServer.Stop()
}

func configureVpnClientFactory(mysteriumApiClient server.Client, vpnClientRuntimeDirectory string) client_connection.VpnClientFactory {
return func(vpnSession *session.VpnSession, session *dto.Session) (openvpn.Client, error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member
  • Very strange to inject all *session.VpnSession but need just sessionConfig
  • Very strange to inject all *dto.Session, then You need just sessionId

Extra tradeoff:

  • Then you try to pass by reference, which leads to unsafe nil passing

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 20, 2017

Author Member

It's a result of breaking a much bigger function and trying to isolate creating vpn client from dialog passed configuration. But a few moments - dto.Session should disappear at all, as vpnSession should contain all config and ids needed to construct vpn client. Secondly our only supported transport carrier is VPN, but this factory would be a good place to create a specific transport connection negotiated between client and node, i.e. WebRTC connection or anything else. VpnSession is bad naming as it's esentially an agreement between client and node - what kind of service will be used, it could be not only VPN.
As there is single place that calls this method - caller guarantees that nils will not be passed. Again - why to make extra copy of those two structures?

return err
}

manager.vpnClient, err = manager.vpnClientFactory(vpnSession, &session)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member

Strange passing by reference, is it mutated inside?

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 20, 2017

Author Member

No it's not mutated, that's why passing by reference avoids unnecessary copying

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 21, 2017

Author Member

Deciced: pass by value

return nil
}

func (nm *fakeManager) Status() ConnectionStatus {
return ConnectionStatus{NOT_CONNECTED, ""}
func (manager *connectionManager) Wait() error {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member

We dont need Wait() anymore, correct?

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 20, 2017

Author Member

Well if we are shutting connection manager on client exiting, maybe we would like to wait until it stops gracefully

const (
NOT_CONNECTED = State("NOT_CONNECTED")
NEGOTIATING = State("NEGOTIATING")
CONNECTED = State("CONNECTED")

This comment has been minimized.

Copy link
@donce

donce Dec 20, 2017

Contributor

Why are we using ALL_CAPS_CASE for constants as in other programming languages? It should be NotConnected, Negotiating, Connected in Go.

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 20, 2017

Member

I agree, but this convention is throughout the project:)

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 20, 2017

Author Member

Well we can agree to start using go way constants and this is place would be an example :) Will fix names

type test_context struct {
suite.Suite
connManager *connectionManager
fakeDiscoveryClient server.Client

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member

Specify more exact type, if You know that it is fake

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 20, 2017

Author Member

Good point

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 20, 2017

Author Member

Actually I cannot do that, as server.NewClientFake() returns Client interface already.

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 21, 2017

Author Member

Decided: NewClientFake() should return not interface but concrete structure.

}

func (nm *fakeManager) Connect(identity identity.Identity, NodeKey string) error {
func (manager *connectionManager) Disconnect() error {

This comment has been minimized.

Copy link
@donce

donce Dec 20, 2017

Contributor

This method never returns an error - why do we need it as a return param?

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 20, 2017

Author Member

Will return possible error returned by vpnclient.stop

return nil
}

type fake_dialog struct {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member

I would add this Fake next to communication package, so that others could reuse it.
Or we can do it on next case

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 20, 2017

Author Member

In my opinion, reusing these "fake" or "stub" implementations is not very practical, as these implementations are usually test case specific anyway and will differ from component to component testing needs.

fmt.Fprintln(os.Stderr, err)
os.Exit(1)
}

cmd.Run()

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member

Run without errors? Really really?

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 20, 2017

Author Member

cm.Run() does not return any errors, all errors are catched on command creation.

fmt.Fprintln(os.Stderr, "Client starting error: ", err)
os.Exit(1)
}
clientCommand.Run()

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member

Drop unused constants

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 20, 2017

Author Member

All constants are still in use. Even NodeKey is still used by the node. Refactored constant names to camelcase anyway.

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 21, 2017

Author Member

Done.

cmd.clientCommand = command_client.NewCommand(
state_client.NewMiddleware(cmd.checkClientIpWhenConnected),
)
cmd.clientCommand, err = command_client.NewCommand(command_client.CommandOptions{

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member

Would be good not to disconnect middleware state_client.NewMiddleware(cmd.checkClientIpWhenConnected)

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 20, 2017

Author Member

I think this client api checking needs reworking anyway, as it's not practical to pass low level VPN spefic middlewares to client just for testing purposes. Since a lot has changed, how client setups VPN connection. Maybe we should write a simple functionality which setups vpn connection by using tequilapi and then checks client's outgoing ip address?

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 21, 2017

Author Member

This will be fixed later with mysterium_monitor

@@ -2,25 +2,31 @@ package openvpn

import "sync"

func NewClient(config *ClientConfig, directoryRuntime string, middlewares ...ManagementMiddleware) *Client {
type Client interface {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member

interface.go?

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 20, 2017

Author Member

openvpn package is very wide, and there is server/client functionality in one place. So interface.go with Client interface only would be a bit confusing from source files perspective

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 21, 2017

Author Member

Good idea but with next

@tadovas
Copy link
Member Author

left a comment

Review comments updated/fixed

@donce donce changed the title MYST-102 client connection manager MYST-185 client connection manager Dec 21, 2017

@donce donce changed the title MYST-185 client connection manager MYST-102 client connection manager Dec 21, 2017

)

var (
AlreadyConnectedError = errors.New("already connected")

This comment has been minimized.

Copy link
@donce

donce Dec 21, 2017

Contributor

This seems to be unused.

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 21, 2017

Author Member

Removed

@tadovas

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2017

@Waldz as per discussion, fixed essential places

@tadovas tadovas merged commit 6a04846 into master Dec 21, 2017

@tadovas tadovas deleted the feature/MYST-102-client-connection-manager branch Dec 21, 2017

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.