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

Inject Openvpn service on service provider side #392

Merged
merged 10 commits into from Sep 26, 2018

Conversation

Projects
None yet
5 participants
@Waldz
Copy link
Member

commented Sep 20, 2018

Updates #382

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

@Waldz Waldz referenced this pull request Sep 20, 2018

Closed

Make Openvpn plugable transport #382

10 of 10 tasks complete

@Waldz Waldz force-pushed the Waldz:feature/plugable-openvpn-service branch 3 times, most recently from f0a4aae to 2b3ee88 Sep 20, 2018

identityLoader identity_selector.Loader,
signerFactory identity.SignerFactory,
identityRegistry identity_registry.IdentityRegistry,
service Service,

This comment has been minimized.

Copy link
@zolia

zolia Sep 20, 2018

Member

here prob we should have a slice of services here. We will be creating more than 1 service soon.

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 20, 2018

Author Member

Yes, will be then needed. This is small PR to decouple and inject Openvpn service

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 20, 2018

Member

Why manager should get list of services at construction time? It would be nice to register/deregister any service on the fly. Especially if we are providing service management api later. I don't want to restart node simply because to enable new service

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 20, 2018

Author Member

This is why I call this PR "Inject openvpn service..". Dynamic loading will come later

@Waldz Waldz force-pushed the Waldz:feature/plugable-openvpn-service branch 3 times, most recently from 6ac9b0c to e6d2a49 Sep 20, 2018

@vkuznecovas
Copy link
Contributor

left a comment

Looks good!

cmd/di.go Outdated
@@ -105,15 +106,14 @@ func (di *Dependencies) BootstrapServiceComponents(nodeOptions node.Options, ser

discoveryService := discovery.NewService(di.IdentityRegistry, di.IdentityRegistration, di.MysteriumClient, di.SignerFactory)

serviceOpenvpn := openvpn_service.NewService(nodeOptions, serviceOptions, di.IPResolver, di.LocationResolver)

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 20, 2018

Contributor

if the above is the "discoveryService" then this should also be "openvpnService"

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 20, 2018

Author Member

Renamed to openvpnServiceManager

}
)

func Test_NewServiceProposal(t *testing.T) {

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 20, 2018

Contributor

does this really test the New proposal? Is it not update?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 20, 2018

Author Member

Fixed

generateID: idGenerator,
generateConfig: configProvider,
saveSession: saveCallback,
creationLock: sync.Mutex{},
}
}

type manager struct {
// ManagerInstance knows how to start and provision session
type ManagerInstance struct {

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 20, 2018

Contributor

Not a fan of having "instance" in struct names, but I do see why you use it here

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 20, 2018

Member

If it's hard to name it - it means its hard to define purpose

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 20, 2018

Author Member

Ditched this renaming. Will work on better SessionManager implementation and SessionHandler interface later

@Waldz Waldz changed the title Inject Openvpn service to service.Manager Inject Openvpn service on service provider side Sep 20, 2018

@@ -28,8 +28,3 @@ type Session struct {
Config ServiceConfiguration
ConsumerID identity.Identity
}

This comment has been minimized.

Copy link
@zolia

zolia Sep 20, 2018

Member

Removed interface, but interface file left

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 20, 2018

Author Member

Improved

@Waldz Waldz force-pushed the Waldz:feature/plugable-openvpn-service branch from e6d2a49 to c17525a Sep 20, 2018

@tadovas
Copy link
Member

left a comment

Some remarks.


// Service interface represents pluggable Mysterium service
type Service interface {
Start(providerID identity.Identity, dialogWaiter communication.DialogWaiter) (dto_discovery.ServiceProposal, error)

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 20, 2018

Member

That's strange that service depends on dialog waiter. Also does the same instance should be started with separate dialog waiters? This dependency looks more like implementation specific to me, not by service interface itself

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 20, 2018

Author Member

Currently did not know how to go. So I postponed to more clear interface, then we have more 2 services later

identityLoader identity_selector.Loader,
signerFactory identity.SignerFactory,
identityRegistry identity_registry.IdentityRegistry,
service Service,

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 20, 2018

Member

Why manager should get list of services at construction time? It would be nice to register/deregister any service on the fly. Especially if we are providing service management api later. I don't want to restart node simply because to enable new service

)

// UpdateProposal updates service proposal description with general data
func UpdateProposal(proposal *ServiceProposal, providerID identity.Identity, providerContact Contact) {

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 20, 2018

Member

function which changes passed parameter? I would expect a function which returns updated proposal, or even better, make this function part of proposal itself

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 20, 2018

Author Member

Good idea. Improved

generateID: idGenerator,
generateConfig: configProvider,
saveSession: saveCallback,
creationLock: sync.Mutex{},
}
}

type manager struct {
// ManagerInstance knows how to start and provision session
type ManagerInstance struct {

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 20, 2018

Member

If it's hard to name it - it means its hard to define purpose

@Waldz Waldz force-pushed the Waldz:feature/plugable-openvpn-service branch 3 times, most recently from c672000 to d87cb48 Sep 20, 2018

@Waldz
Copy link
Member Author

left a comment

All comments fixed, Please review again

@Waldz Waldz force-pushed the Waldz:feature/plugable-openvpn-service branch from d87cb48 to e6e3384 Sep 20, 2018


return &Manager{
identityLoader: identityLoader,
dialogWaiterFactory: func(myID identity.Identity) communication.DialogWaiter {

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 21, 2018

Member

Is there a better naming then myID?
It's not clear whose id should be there.

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 21, 2018

Author Member

Renamed to providerID

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 21, 2018

Author Member

Well it depends, You can create Dialog at consumer side and wait for dialogs. It's ID of the one who waits for dialogs

if manager.vpnServer != nil {
manager.vpnServer.Stop()
}
manager.service.Stop()

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 21, 2018

Member

manager.service.Stop() returns an error too, should we check it?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 21, 2018

Author Member

Added handling in any of subcomponent stop.


caSubject := pkix.Name{
Country: []string{currentCountry},
Organization: []string{"Mystermium.network"},

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 21, 2018

Member

typo: Mystermium -> mysterium

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 21, 2018

Author Member

Fixed

Show resolved Hide resolved session/create_dto.go
Show resolved Hide resolved session/create_dto.go
Show resolved Hide resolved session/create_dto.go
Show resolved Hide resolved session/dto.go
@@ -48,6 +50,14 @@ type ServiceProposal struct {
ProviderContacts []Contact `json:"provider_contacts"`
}

// SetDiscoveryData updates service proposal description with general data
func (proposal *ServiceProposal) SetDiscoveryData(providerID identity.Identity, providerContact Contact) {
proposal.Format = "service-proposal/v1"

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 24, 2018

Contributor

can we make this a const instead of a "magic string" so we can reuse it if needed?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 25, 2018

Author Member

defined constant

@Waldz Waldz force-pushed the Waldz:feature/plugable-openvpn-service branch 2 times, most recently from 5d1d14d to b245c3b Sep 25, 2018

@Waldz

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2018

All comments fixed

@@ -94,68 +83,46 @@ func (manager *Manager) Start() (err error) {

currentCountry, err := manager.locationResolver.ResolveCountry(publicIP)
if err != nil {
return err
return

This comment has been minimized.

Copy link
@zolia

zolia Sep 25, 2018

Member

how error should be handled here?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 26, 2018

Author Member

Implemented custom error ErrorLocation and special warning recommendation for Provider


proposal = manager.proposalFactory(currentLocation)
sessionManager = manager.sessionManagerFactory(primitives, outboundIP, publicIP)
err = nil

This comment has been minimized.

Copy link
@zolia

zolia Sep 25, 2018

Member

this prob is set to nil at initialisation. Excessive.

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 25, 2018

Author Member

Fixed

@zolia

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

travis fails..

@zolia zolia closed this Sep 25, 2018

@zolia zolia reopened this Sep 25, 2018

@Waldz Waldz force-pushed the Waldz:feature/plugable-openvpn-service branch 3 times, most recently from 3f1d666 to 4b7834b Sep 25, 2018

Gone for too long

@Waldz Waldz force-pushed the Waldz:feature/plugable-openvpn-service branch from 4b7834b to 9356bee Sep 26, 2018

@Waldz Waldz force-pushed the Waldz:feature/plugable-openvpn-service branch from 9356bee to abc7b53 Sep 26, 2018

Waldz added some commits Sep 18, 2018

@Waldz Waldz force-pushed the Waldz:feature/plugable-openvpn-service branch from abc7b53 to d447cd8 Sep 26, 2018

@zolia

zolia approved these changes Sep 26, 2018

@Waldz Waldz merged commit ce92235 into mysteriumnetwork:master Sep 26, 2018

1 check passed

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

@Waldz Waldz deleted the Waldz:feature/plugable-openvpn-service branch Sep 26, 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.