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

Feature/myst 708 kill switch #330

Merged
merged 3 commits into from Sep 13, 2018

Conversation

Projects
None yet
3 participants
@zolia
Copy link
Member

commented Sep 7, 2018

tried to find middle ground between @Waldz and @tadovas remarks.

@zolia zolia requested review from tadovas and Waldz as code owners Sep 7, 2018

@tadovas
Copy link
Member

left a comment

ConnectOptions as jsonstructure has meaning in endpoints only. Connection manager should at least define own "feature" options, and openvpn client config should define only fine grained methodds like setConnectRetry(infinte). Don't trash the same dto through the system

@@ -19,6 +19,7 @@ package connection

import (
"github.com/mysteriumnetwork/node/communication"
dto2 "github.com/mysteriumnetwork/node/core/node/dto"

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 7, 2018

Member

avoid generic packages like DTO

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Member

I'd like dto69 more :)

@@ -31,12 +32,12 @@ type DialogCreator func(consumerID, providerID identity.Identity, contact dto.Co

// VpnClientCreator creates new vpn client by given session,
// consumer identity, provider identity and uses state callback to report state changes
type VpnClientCreator func(session.SessionDto, identity.Identity, identity.Identity, state.Callback) (openvpn.Process, error)
type VpnClientCreator func(session.SessionDto, identity.Identity, identity.Identity, state.Callback, dto2.ConnectOptions) (openvpn.Process, error)

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 7, 2018

Member

Connection manager should not depend on external Connect options. At least define them in the same package

@@ -26,6 +27,13 @@ type ClientConfig struct {
*config.GenericConfig
}

// SetConnectOptions sets connect options provided through endpoint parameters
func (c *ClientConfig) SetConnectOptions(options dto.ConnectOptions) {

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 7, 2018

Member

Client config should have clear methods -> RestrictReconnects and so on, dont add external dependency as ConnectOptions

@@ -23,6 +23,8 @@ import (
"log"
"strings"

"strconv"

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Member

Why 2 separations?

@@ -19,6 +19,7 @@ package connection

import (
"github.com/mysteriumnetwork/node/communication"
dto2 "github.com/mysteriumnetwork/node/core/node/dto"

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Member

I'd like dto69 more :)

@@ -71,7 +72,7 @@ func NewManager(mysteriumClient server.Client, dialogCreator DialogCreator,
}
}

func (manager *connectionManager) Connect(consumerID, providerID identity.Identity) (err error) {
func (manager *connectionManager) Connect(consumerID, providerID identity.Identity, options dto2.ConnectOptions) (err error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Member

+100 for DTO, I am doing same wiith serviceOptions for vpn server


// ConnectOptions holds tequilapi connect options
// swagger:model ConnectOptionsDTO
type ConnectOptions struct {

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Member

Lets have this DTO in connection package

@@ -35,6 +32,9 @@ import (
"github.com/ethereum/go-ethereum/ethclient"
"github.com/ethereum/go-ethereum/params"
"github.com/mysteriumnetwork/node/tequilapi/client"
"github.com/mysteriumnetwork/payments/cli/helpers"
mysttoken "github.com/mysteriumnetwork/payments/mysttoken/generated"

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Member

+1 good idea


err := NewDefaultValidator().IsValid(vpnConfig)
if err != nil {
return nil, err
}

clientFileConfig := newClientConfig(runtimeDir, configDir)
clientFileConfig.SetConnectOptions(options)

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Member

You should map ConnectionOptions -> openvpn.config here

@@ -100,13 +102,17 @@ func (client *Client) RegistrationStatus(address string) (RegistrationStatusDTO,
}

// Connect initiates a new connection to a host identified by providerID
func (client *Client) Connect(consumerID, providerID string) (status StatusDTO, err error) {
func (client *Client) Connect(consumerID, providerID string, disableKill bool) (status StatusDTO, err error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Member

Lets have pass all dto.ConnectOptions as argument


// connect options
// required: false
ConnectOptions dto.ConnectOptions `json:"connectOptions,omitempty"`

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Member

It's dont have to repeat connectOptions in connectionRequest. IMHO options would be enought in REST contract


// Manager interface provides methods to manage connection
type Manager interface {
// Connect creates new connection from given consumer to provider, reports error if connection already exists
Connect(consumerID identity.Identity, providerID identity.Identity) error
Connect(consumerID identity.Identity, providerID identity.Identity, options dto2.ConnectOptions) error

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Member

+100 for separating openvopn specific things to separate struct

ProviderID string `json:"providerId"`
Identity string `json:"consumerId"`
ProviderID string `json:"providerId"`
ConnectOptions dto.ConnectOptions `json:"connectOptions"`

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Member

It's dont have to repeat connectOptions in connectionRequest. IMHO options would be enought in REST contract

@zolia zolia force-pushed the feature/MYST-708-kill-switch branch 3 times, most recently from f064527 to ac6af31 Sep 7, 2018

@Waldz

This comment has been minimized.

Copy link
Member

commented Sep 8, 2018

I imagine transformation between ConnectOption to ClientConfig:

clientConfig.setConnectRetry(times)
clientConfig.setConnectRetryInfinite()

@zolia zolia force-pushed the feature/MYST-708-kill-switch branch 3 times, most recently from deeed2f to 692f004 Sep 10, 2018

@zolia

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2018

Branch rebased on master

@zolia zolia force-pushed the feature/MYST-708-kill-switch branch 3 times, most recently from 2c446ef to d098d64 Sep 11, 2018

@zolia zolia self-assigned this Sep 12, 2018

zolia added some commits Sep 7, 2018

@zolia zolia force-pushed the feature/MYST-708-kill-switch branch from ae5b821 to 924d7fa Sep 12, 2018

@zolia zolia force-pushed the feature/MYST-708-kill-switch branch from 924d7fa to 6d3226f Sep 12, 2018

@Waldz

Waldz approved these changes Sep 12, 2018

@tadovas
Copy link
Member

left a comment

LGTMing this now. But for future reference:
Connection manager should be responsible for tracking service consuming session (promises etc.) and managing underlying payable service. Therefore service abstraction is needed where concrete service will provide and manage all features expected by consumer. I.e. firewall style traffic blocking in case of VPN fails is only needed for Layer3 tunneling. Proxy style service already has it - if proxy conneciton is lost and proxy is set in system settings - no connection will go out by default.

@zolia zolia merged commit 3a306c1 into master Sep 13, 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

@zolia zolia deleted the feature/MYST-708-kill-switch branch Sep 13, 2018

@zolia zolia referenced this pull request Sep 13, 2018

Closed

service abstraction #360

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.