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
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
5ef5f67
Finally found a way to move parenthesis to new line
tadovas Dec 15, 2017
5f08388
A bit of renaming
tadovas Dec 18, 2017
e81a143
Finally found a way to move parenthesis to new line
tadovas Dec 15, 2017
d22d553
A bit of renaming
tadovas Dec 18, 2017
b9a2209
Merge branch 'feature/MYST-102-client-connection-manager' of github.c…
tadovas Dec 18, 2017
8c4530f
All dialog/vpn client connection functionality moved to connection_ma…
tadovas Dec 18, 2017
58c70b1
Merge branch 'master' of github.com:MysteriumNetwork/node into featur…
tadovas Dec 18, 2017
90d05e7
Major refactoring of mysterium client command
tadovas Dec 19, 2017
554f5ef
Pulled in idenity refactoring from master
tadovas Dec 19, 2017
86103ad
Merge branch 'master' of github.com:MysteriumNetwork/node into featur…
tadovas Dec 19, 2017
cb2981a
Removed unused client command fields
tadovas Dec 19, 2017
29b5453
Small polishing
tadovas Dec 19, 2017
21c80c9
OpenVPN client creation refactored to separate function, client conne…
tadovas Dec 19, 2017
0327dad
Merge branch 'master' of github.com:MysteriumNetwork/node into featur…
tadovas Dec 19, 2017
479cf40
VpnFactory functionality moved to separate configurer function
tadovas Dec 20, 2017
0d06700
Status management a bit cleaned up, two more tests
tadovas Dec 20, 2017
493f80f
Test added for correct status to return when connect is in progress
tadovas Dec 20, 2017
d1db2c3
Removed unused function, removed new line
tadovas Dec 20, 2017
d4771aa
Constant renaming
tadovas Dec 20, 2017
653e4d4
Renaming, manager disconnect function returns error returned by vpncl…
tadovas Dec 20, 2017
58440f3
cmd.Run starts serving tequila api now
tadovas Dec 20, 2017
142db8a
Constant renamed to camel case
tadovas Dec 20, 2017
8ddce78
Pass dto structures by value
tadovas Dec 21, 2017
f505aea
Fake client structure exposed and returned from factory
tadovas Dec 21, 2017
2d8ec9a
Removed unused constant
tadovas Dec 21, 2017
c5c4c76
Code moved around, connection manager (temporary) knows how to create…
tadovas Dec 21, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 31 additions & 0 deletions client_connection/interface.go
@@ -0,0 +1,31 @@
package client_connection

import (
"errors"
"github.com/mysterium/node/identity"
)

type State string

const (
NOT_CONNECTED = State("NOT_CONNECTED")
NEGOTIATING = State("NEGOTIATING")
CONNECTED = State("CONNECTED")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)

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

type ConnectionStatus struct {
State State
SessionId string
LastError error
}

type Manager interface {
Connect(identity identity.Identity, NodeKey string) error
Status() ConnectionStatus
Disconnect() error
Wait() error
}
108 changes: 76 additions & 32 deletions client_connection/manager.go
@@ -1,55 +1,99 @@
package client_connection

import (
"errors"
"github.com/mysterium/node/identity"
"github.com/mysterium/node/communication"
"github.com/mysterium/node/identity"
"github.com/mysterium/node/openvpn"
vpn_session "github.com/mysterium/node/openvpn/session"
"github.com/mysterium/node/server"
"github.com/mysterium/node/server/dto"
)

type State string
type DialogEstablisherFactory func(identity identity.Identity) communication.DialogEstablisher

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

var (
ALREADY_CONNECTED = errors.New("already connected")
)
type VpnClientFactory func(vpnSession *vpn_session.VpnSession, session *dto.Session) (openvpn.Client, error)

type ConnectionStatus struct {
State State
SessionId string
type connectionManager struct {
//these are passed on creation
mysteriumClient server.Client
dialogEstablisherFactory DialogEstablisherFactory
vpnClientFactory VpnClientFactory
//these are populated by Connect at runtime
dialog communication.Dialog
vpnClient openvpn.Client
status ConnectionStatus
}

type Manager interface {
Connect(identity identity.Identity, NodeKey string) error
Status() ConnectionStatus
Disconnect() error
Wait() error
func NewManager(mysteriumClient server.Client, dialogEstablisherFactory DialogEstablisherFactory, vpnClientFactory VpnClientFactory) *connectionManager {
return &connectionManager{
mysteriumClient,
dialogEstablisherFactory,
vpnClientFactory,
nil,
nil,
statusNotConnected(),
}
}

type fakeManager struct {
errorChannel chan error
func (manager *connectionManager) Connect(identity identity.Identity, NodeKey string) error {
manager.status = statusConnecting()
session, err := manager.mysteriumClient.SessionCreate(NodeKey)
if err != nil {
manager.status = statusError(err)
return err
}

proposal := session.ServiceProposal

dialogEstablisher := manager.dialogEstablisherFactory(identity)
manager.dialog, err = dialogEstablisher.CreateDialog(proposal.ProviderContacts[0])
if err != nil {
manager.status = statusError(err)
return err
}

vpnSession, err := vpn_session.RequestSessionCreate(manager.dialog, proposal.Id)
if err != nil {
manager.status = statusError(err)
return err
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange passing by reference, is it mutated inside?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deciced: pass by value


if err := manager.vpnClient.Start(); err != nil {
manager.status = statusError(err)
return err
}
manager.status = statusConnected(session.Id)
return nil
}

func NewManager() *fakeManager {
return &fakeManager{make(chan error)}
func (manager *connectionManager) Status() ConnectionStatus {
return manager.status
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@tadovas tadovas Dec 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will return possible error returned by vpnclient.stop

manager.dialog.Close()
manager.vpnClient.Stop()
return nil
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We dont need Wait() anymore, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return manager.vpnClient.Wait()
}

func (nm *fakeManager) Disconnect() error {
nm.errorChannel <- errors.New("disconnected")
return nil
func statusError(err error) ConnectionStatus {
return ConnectionStatus{NOT_CONNECTED, "", err}
}

func statusConnecting() ConnectionStatus {
return ConnectionStatus{NEGOTIATING, "", nil}
}

func statusConnected(sessionId string) ConnectionStatus {
return ConnectionStatus{CONNECTED, sessionId, nil}
}

func (nm *fakeManager) Wait() error {
return <-nm.errorChannel
func statusNotConnected() ConnectionStatus {
return ConnectionStatus{NOT_CONNECTED, "", nil}
}
141 changes: 141 additions & 0 deletions client_connection/manager_test.go
@@ -0,0 +1,141 @@
package client_connection

import (
"errors"
"github.com/mysterium/node/communication"
"github.com/mysterium/node/identity"
"github.com/mysterium/node/openvpn"
"github.com/mysterium/node/openvpn/service_discovery"
"github.com/mysterium/node/openvpn/session"
"github.com/mysterium/node/server"
mysterium_api_client "github.com/mysterium/node/server/dto"
"github.com/mysterium/node/service_discovery/dto"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/suite"
"sync"
"testing"
)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fakeOpenVpn *fake_openvpn_client
}

func (tc *test_context) SetupTest() {

tc.fakeDiscoveryClient = server.NewClientFake()

serviceProposal := service_discovery.NewServiceProposal(identity.FromAddress("vpn-node-1"), dto.Contact{})
tc.fakeDiscoveryClient.NodeRegister(serviceProposal)

dialogEstablisherFactory := func(identity identity.Identity) communication.DialogEstablisher {
return &fake_dialog{}
}

tc.fakeOpenVpn = &fake_openvpn_client{make(chan int, 1), nil}
fakeVpnClientFactory := func(vpnSession *session.VpnSession, session *mysterium_api_client.Session) (openvpn.Client, error) {
return tc.fakeOpenVpn, nil
}

tc.connManager = NewManager(tc.fakeDiscoveryClient, dialogEstablisherFactory, fakeVpnClientFactory)
}

func (tc *test_context) TestWhenNoConnectionIsMadeStatusIsNotConnected() {
assert.Equal(tc.T(), ConnectionStatus{NOT_CONNECTED, "", nil}, tc.connManager.Status())
}

func (tc *test_context) TestOnConnectErrorStatusIsNotConnectedAndLastErrorIsSet() {
fatalVpnError := errors.New("fatal connection error")
tc.fakeOpenVpn.onConnectReturnError = fatalVpnError
tc.fakeOpenVpn.resumeStart()

assert.Error(tc.T(), tc.connManager.Connect(identity.FromAddress("identity-1"), "vpn-node-1"))
assert.Equal(tc.T(), ConnectionStatus{NOT_CONNECTED, "", fatalVpnError}, tc.connManager.Status())
}

func (tc *test_context) TestWhenManagerMadeConnectionStatusReturnsConnectedStateAndSessionId() {
tc.fakeOpenVpn.resumeStart()

err := tc.connManager.Connect(identity.FromAddress("identity-1"), "vpn-node-1")

assert.NoError(tc.T(), err)
assert.Equal(tc.T(), ConnectionStatus{CONNECTED, "vpn-node-1-session", nil}, tc.connManager.Status())
}

func (tc *test_context) TestStatusReportsConnectingWhenConnectionIsInProgress() {
wg := sync.WaitGroup{}
wg.Add(1)
go func() {
wg.Done()
tc.connManager.Connect(identity.FromAddress("identity-1"), "vpn-node-1")
}()
//wait for go function actually start, to avoid race condition, when we query Status before Connect call even begins.
wg.Wait()
assert.Equal(tc.T(), ConnectionStatus{NEGOTIATING, "", nil}, tc.connManager.Status())
tc.fakeOpenVpn.resumeStart()
}

func (tc *test_context) TearDownTest() {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not You destroy some resources gracefully?

  • connManager.Stop()
  • fakeOpenVpn.Stop()

}

func TestConnectionManagerSuite(t *testing.T) {
suite.Run(t, new(test_context))
}

type fake_openvpn_client struct {
connectionDelay chan int
onConnectReturnError error
}

func (foc *fake_openvpn_client) resumeStart() {
foc.connectionDelay <- 1
}

func (foc *fake_openvpn_client) Start() error {
<-foc.connectionDelay
return foc.onConnectReturnError
}

func (foc *fake_openvpn_client) Wait() error {
return nil
}

func (foc *fake_openvpn_client) Stop() error {
return nil
}

type fake_dialog struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

func (fd *fake_dialog) CreateDialog(contact dto.Contact) (communication.Dialog, error) {
return fd, nil
}

func (fd *fake_dialog) Close() error {
return nil
}

func (fd *fake_dialog) Receive(consumer communication.MessageConsumer) error {
return nil
}
func (fd *fake_dialog) Respond(consumer communication.RequestConsumer) error {
return nil
}

func (fd *fake_dialog) Send(producer communication.MessageProducer) error {
return nil
}

func (fd *fake_dialog) Request(producer communication.RequestProducer) (responsePtr interface{}, err error) {
return &session.SessionCreateResponse{
true,
"Everything is great!",
session.VpnSession{
"vpn-session-id",
"vpn-session-config"},
},
nil
}