From 7c0f00e4940a367c5b101e2dbff9e37aba0b0c76 Mon Sep 17 00:00:00 2001 From: tcharding Date: Fri, 26 Oct 2018 08:45:59 +1100 Subject: [PATCH 1/9] Use sessionID instead of sessionId `golint` emits warnings: interface method parameter sessionId should be sessionID method parameter sessionId should be sessionID We should us capital letters for acronyms as is standard golang convention. After this is applied server/ lints cleanly. Use sessionID instead of sessionId for parameter. Signed-off-by: tcharding --- server/interface.go | 2 +- server/mysterium_api_fake.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/server/interface.go b/server/interface.go index 31531dd181..253084d0ed 100644 --- a/server/interface.go +++ b/server/interface.go @@ -33,5 +33,5 @@ type Client interface { UnregisterProposal(proposal dto_discovery.ServiceProposal, signer identity.Signer) (err error) PingProposal(proposal dto_discovery.ServiceProposal, signer identity.Signer) (err error) - SendSessionStats(sessionId session.ID, sessionStats dto.SessionStats, signer identity.Signer) (err error) + SendSessionStats(sessionID session.ID, sessionStats dto.SessionStats, signer identity.Signer) (err error) } diff --git a/server/mysterium_api_fake.go b/server/mysterium_api_fake.go index c11a827ab0..c7a06d84bf 100644 --- a/server/mysterium_api_fake.go +++ b/server/mysterium_api_fake.go @@ -96,8 +96,8 @@ func (client *ClientFake) FindProposals(providerID string) (proposals []dto_disc } // SendSessionStats heartbeats that session is still active + session upload and download amounts -func (client *ClientFake) SendSessionStats(sessionId session.ID, sessionStats dto.SessionStats, signer identity.Signer) (err error) { - log.Info(mysteriumAPILogPrefix, "Fake session stats sent: ", sessionId) +func (client *ClientFake) SendSessionStats(sessionID session.ID, sessionStats dto.SessionStats, signer identity.Signer) (err error) { + log.Info(mysteriumAPILogPrefix, "Fake session stats sent: ", sessionID) return nil } From 09341ae1b0e0d453b803383ac2147c1aa2164a98 Mon Sep 17 00:00:00 2001 From: tcharding Date: Wed, 31 Oct 2018 12:11:17 +1100 Subject: [PATCH 2/9] Rename NewAddressGenerate to NewAddressFromHostAndID We currently have four functions for creating a NATS address NewAddress NewAddressGenerate NewAddressForContact newAddressWithConnection `NewAddressGenerate` is not overly clear, lets rename it. In order to fit in with the other function names use `NewAddressFromHostAndID`. Signed-off-by: tcharding --- cmd/di.go | 2 +- communication/nats/discovery/address.go | 4 ++-- communication/nats/discovery/address_test.go | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/cmd/di.go b/cmd/di.go index 078629d5d2..efbe1ea680 100644 --- a/cmd/di.go +++ b/cmd/di.go @@ -231,7 +231,7 @@ func (di *Dependencies) bootstrapServiceComponents(nodeOptions node.Options) { newDialogWaiter := func(providerID identity.Identity) communication.DialogWaiter { return nats_dialog.NewDialogWaiter( - nats_discovery.NewAddressGenerate(di.NetworkDefinition.BrokerAddress, providerID), + nats_discovery.NewAddressFromHostAndID(di.NetworkDefinition.BrokerAddress, providerID), di.SignerFactory(providerID), di.IdentityRegistry, ) diff --git a/communication/nats/discovery/address.go b/communication/nats/discovery/address.go index 3bc19adada..6d37d85407 100644 --- a/communication/nats/discovery/address.go +++ b/communication/nats/discovery/address.go @@ -34,8 +34,8 @@ func NewAddress(topic string, addresses ...string) *AddressNATS { } } -// NewAddressGenerate generates NATS address for current node -func NewAddressGenerate(brokerIP string, myID identity.Identity) *AddressNATS { +// NewAddressFromHostAndID generates NATS address for current node +func NewAddressFromHostAndID(brokerIP string, myID identity.Identity) *AddressNATS { address := fmt.Sprintf("nats://%s:%d", brokerIP, BrokerPort) return NewAddress(myID.Address, address) diff --git a/communication/nats/discovery/address_test.go b/communication/nats/discovery/address_test.go index b370e2f06c..e9affafe9a 100644 --- a/communication/nats/discovery/address_test.go +++ b/communication/nats/discovery/address_test.go @@ -39,10 +39,10 @@ func TestNewAddress(t *testing.T) { ) } -func TestNewAddressGenerate(t *testing.T) { +func TestNewAddressFromHostAndID(t *testing.T) { myID := identity.FromAddress("provider1") brokerIP := "127.0.0.1" - address := NewAddressGenerate(brokerIP, myID) + address := NewAddressFromHostAndID(brokerIP, myID) assert.Equal( t, From 5fc7f651ddfd8c3b4d80b0571b77a12847246cd1 Mon Sep 17 00:00:00 2001 From: tcharding Date: Wed, 31 Oct 2018 12:18:51 +1100 Subject: [PATCH 3/9] Add table driven testing Add table driven testing to `nats/discovery`. Signed-off-by: tcharding --- communication/nats/discovery/address_test.go | 29 +++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/communication/nats/discovery/address_test.go b/communication/nats/discovery/address_test.go index e9affafe9a..15371cb23f 100644 --- a/communication/nats/discovery/address_test.go +++ b/communication/nats/discovery/address_test.go @@ -40,18 +40,27 @@ func TestNewAddress(t *testing.T) { } func TestNewAddressFromHostAndID(t *testing.T) { + var tests = []struct { + uri string + want string + }{ + {"127.0.0.1", "nats://127.0.0.1:4222"}, + } + myID := identity.FromAddress("provider1") - brokerIP := "127.0.0.1" - address := NewAddressFromHostAndID(brokerIP, myID) + for _, tc := range tests { + address := NewAddressFromHostAndID(tc.uri, myID) + + assert.Equal( + t, + &AddressNATS{ + servers: []string{tc.want}, + topic: "provider1", + }, + address, + ) - assert.Equal( - t, - &AddressNATS{ - servers: []string{"nats://" + brokerIP + ":4222"}, - topic: "provider1", - }, - address, - ) + } } func TestNewAddressForContact(t *testing.T) { From 6ad3dcb5ceadb08b08af8adfa79c181c4a574990 Mon Sep 17 00:00:00 2001 From: tcharding Date: Wed, 31 Oct 2018 12:38:54 +1100 Subject: [PATCH 4/9] Conditionally add 'nats://' scheme Currently we unconditionally add `nats://` scheme to address string of the broker. It is reasonable for the user to expect to be able to pass an address of form `nats://localhost` when configuring the broker. Only add the scheme `nats:` if it is not already present in the URI. Signed-off-by: tcharding --- communication/nats/discovery/address.go | 14 +++++++++++--- communication/nats/discovery/address_test.go | 1 + metadata/network.go | 2 +- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/communication/nats/discovery/address.go b/communication/nats/discovery/address.go index 6d37d85407..0773508989 100644 --- a/communication/nats/discovery/address.go +++ b/communication/nats/discovery/address.go @@ -19,6 +19,7 @@ package discovery import ( "fmt" + "strings" "github.com/mysteriumnetwork/node/communication/nats" "github.com/mysteriumnetwork/node/identity" @@ -35,10 +36,17 @@ func NewAddress(topic string, addresses ...string) *AddressNATS { } // NewAddressFromHostAndID generates NATS address for current node -func NewAddressFromHostAndID(brokerIP string, myID identity.Identity) *AddressNATS { - address := fmt.Sprintf("nats://%s:%d", brokerIP, BrokerPort) +func NewAddressFromHostAndID(uri string, myID identity.Identity) *AddressNATS { + var url string + if strings.HasPrefix(uri, "nats:") { + url = uri + } else { + url = fmt.Sprintf("nats://%s", uri) + } + + url = fmt.Sprintf("%s:%d", url, BrokerPort) - return NewAddress(myID.Address, address) + return NewAddress(myID.Address, url) } // NewAddressForContact extracts NATS address from given contact structure diff --git a/communication/nats/discovery/address_test.go b/communication/nats/discovery/address_test.go index 15371cb23f..8b77666cb2 100644 --- a/communication/nats/discovery/address_test.go +++ b/communication/nats/discovery/address_test.go @@ -45,6 +45,7 @@ func TestNewAddressFromHostAndID(t *testing.T) { want string }{ {"127.0.0.1", "nats://127.0.0.1:4222"}, + {"nats://127.0.0.1", "nats://127.0.0.1:4222"}, } myID := identity.FromAddress("provider1") diff --git a/metadata/network.go b/metadata/network.go index ce0c0cb815..e32f9c31a6 100644 --- a/metadata/network.go +++ b/metadata/network.go @@ -33,7 +33,7 @@ type NetworkDefinition struct { // TestnetDefinition defines parameters for test network (currently default network) var TestnetDefinition = NetworkDefinition{ "https://testnet-api.mysterium.network/v1", - "testnet-broker.mysterium.network", + "nats://testnet-broker.mysterium.network", "https://ropsten.infura.io", "https://testnet-morqa.mysterium.network/api/v1", common.HexToAddress("0xbe5F9CCea12Df756bF4a5Baf4c29A10c3ee7C83B"), From e9652e1feb1d852155fb40849daa3f2980194371 Mon Sep 17 00:00:00 2001 From: tcharding Date: Wed, 31 Oct 2018 13:04:45 +1100 Subject: [PATCH 5/9] Add error to DialogWaiterFactory In preparation for additional NATS address construction logic add an error return to the `DialogWaiterFactory`. Signed-off-by: tcharding --- cmd/di.go | 10 +++++++--- communication/nats/discovery/address.go | 4 ++-- communication/nats/discovery/address_test.go | 4 ++-- core/service/manager.go | 9 ++++++--- 4 files changed, 17 insertions(+), 10 deletions(-) diff --git a/cmd/di.go b/cmd/di.go index efbe1ea680..ce6c724778 100644 --- a/cmd/di.go +++ b/cmd/di.go @@ -229,12 +229,16 @@ func (di *Dependencies) bootstrapServiceComponents(nodeOptions node.Options) { return openvpn_service.NewManager(nodeOptions, transportOptions, di.IPResolver, di.LocationResolver, sessionStorage), nil } - newDialogWaiter := func(providerID identity.Identity) communication.DialogWaiter { + newDialogWaiter := func(providerID identity.Identity) (communication.DialogWaiter, error) { + address, err := nats_discovery.NewAddressFromHostAndID(di.NetworkDefinition.BrokerAddress, providerID) + if err != nil { + return nil, err + } return nats_dialog.NewDialogWaiter( - nats_discovery.NewAddressFromHostAndID(di.NetworkDefinition.BrokerAddress, providerID), + address, di.SignerFactory(providerID), di.IdentityRegistry, - ) + ), nil } newDialogHandler := func(proposal dto_discovery.ServiceProposal, configProvider session.ConfigProvider) communication.DialogHandler { promiseHandler := func(dialog communication.Dialog) session.PromiseProcessor { diff --git a/communication/nats/discovery/address.go b/communication/nats/discovery/address.go index 0773508989..bf17114870 100644 --- a/communication/nats/discovery/address.go +++ b/communication/nats/discovery/address.go @@ -36,7 +36,7 @@ func NewAddress(topic string, addresses ...string) *AddressNATS { } // NewAddressFromHostAndID generates NATS address for current node -func NewAddressFromHostAndID(uri string, myID identity.Identity) *AddressNATS { +func NewAddressFromHostAndID(uri string, myID identity.Identity) (*AddressNATS, error) { var url string if strings.HasPrefix(uri, "nats:") { url = uri @@ -46,7 +46,7 @@ func NewAddressFromHostAndID(uri string, myID identity.Identity) *AddressNATS { url = fmt.Sprintf("%s:%d", url, BrokerPort) - return NewAddress(myID.Address, url) + return NewAddress(myID.Address, url), nil } // NewAddressForContact extracts NATS address from given contact structure diff --git a/communication/nats/discovery/address_test.go b/communication/nats/discovery/address_test.go index 8b77666cb2..cb27e767e1 100644 --- a/communication/nats/discovery/address_test.go +++ b/communication/nats/discovery/address_test.go @@ -50,8 +50,8 @@ func TestNewAddressFromHostAndID(t *testing.T) { myID := identity.FromAddress("provider1") for _, tc := range tests { - address := NewAddressFromHostAndID(tc.uri, myID) - + address, err := NewAddressFromHostAndID(tc.uri, myID) + assert.NoError(t, err) assert.Equal( t, &AddressNATS{ diff --git a/core/service/manager.go b/core/service/manager.go index 56a17a88af..8601e0be95 100644 --- a/core/service/manager.go +++ b/core/service/manager.go @@ -47,7 +47,7 @@ type Service interface { } // DialogWaiterFactory initiates communication channel which waits for incoming dialogs -type DialogWaiterFactory func(providerID identity.Identity) communication.DialogWaiter +type DialogWaiterFactory func(providerID identity.Identity) (communication.DialogWaiter, error) // DialogHandlerFactory initiates instance which is able to handle incoming dialogs type DialogHandlerFactory func(dto_discovery.ServiceProposal, session.ConfigProvider) communication.DialogHandler @@ -73,7 +73,7 @@ func NewManager( type Manager struct { identityHandler identity_selector.Handler - dialogWaiterFactory func(identity identity.Identity) communication.DialogWaiter + dialogWaiterFactory func(identity identity.Identity) (communication.DialogWaiter, error) dialogWaiter communication.DialogWaiter dialogHandlerFactory DialogHandlerFactory @@ -100,7 +100,10 @@ func (manager *Manager) Start(options Options) (err error) { return err } - manager.dialogWaiter = manager.dialogWaiterFactory(providerID) + manager.dialogWaiter, err = manager.dialogWaiterFactory(providerID) + if err != nil { + return err + } providerContact, err := manager.dialogWaiter.Start() if err != nil { return err From f57336561df4e26192cbf01b7fad75f58a6ad309 Mon Sep 17 00:00:00 2001 From: tcharding Date: Wed, 31 Oct 2018 13:30:08 +1100 Subject: [PATCH 6/9] Conditionally add broker port number Currently we add the hard coded broker port number 4222 unconditionally. If a user wishes to configure nats to use a different port number then we need to accept this port number as part of the configuration for running myst. If we conditionally add the port number then the following is supported bin/run --broker-addres="localhost:3333" It seems that nats library should handle an address string that does not contain the port number by using the default port but our e2e tests fail if the port number is not present. Conditionally add broker port number to the address string. Signed-off-by: tcharding --- communication/nats/discovery/address.go | 19 ++++++++++++++----- communication/nats/discovery/address_test.go | 5 +++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/communication/nats/discovery/address.go b/communication/nats/discovery/address.go index bf17114870..ec9e891e1d 100644 --- a/communication/nats/discovery/address.go +++ b/communication/nats/discovery/address.go @@ -19,6 +19,7 @@ package discovery import ( "fmt" + "net/url" "strings" "github.com/mysteriumnetwork/node/communication/nats" @@ -37,16 +38,24 @@ func NewAddress(topic string, addresses ...string) *AddressNATS { // NewAddressFromHostAndID generates NATS address for current node func NewAddressFromHostAndID(uri string, myID identity.Identity) (*AddressNATS, error) { - var url string + // Add scheme first otherwise url.Parse() fails. + var rawurl string if strings.HasPrefix(uri, "nats:") { - url = uri + rawurl = uri } else { - url = fmt.Sprintf("nats://%s", uri) + rawurl = fmt.Sprintf("nats://%s", uri) } - url = fmt.Sprintf("%s:%d", url, BrokerPort) + url, err := url.Parse(rawurl) + if err != nil { + return nil, err + } + + if url.Port() == "" { + url.Host = fmt.Sprintf("%s:%d", url.Host, BrokerPort) + } - return NewAddress(myID.Address, url), nil + return NewAddress(myID.Address, url.String()), nil } // NewAddressForContact extracts NATS address from given contact structure diff --git a/communication/nats/discovery/address_test.go b/communication/nats/discovery/address_test.go index cb27e767e1..c64034b872 100644 --- a/communication/nats/discovery/address_test.go +++ b/communication/nats/discovery/address_test.go @@ -46,6 +46,11 @@ func TestNewAddressFromHostAndID(t *testing.T) { }{ {"127.0.0.1", "nats://127.0.0.1:4222"}, {"nats://127.0.0.1", "nats://127.0.0.1:4222"}, + {"127.0.0.1:4222", "nats://127.0.0.1:4222"}, + {"nats://127.0.0.1:4222", "nats://127.0.0.1:4222"}, + + {"nats://127.0.0.1:4333", "nats://127.0.0.1:4333"}, + {"nats://example.com:4333", "nats://example.com:4333"}, } myID := identity.FromAddress("provider1") From ca36afa2474f94c420d16780ba8803da0cf0990e Mon Sep 17 00:00:00 2001 From: tcharding Date: Wed, 31 Oct 2018 13:40:17 +1100 Subject: [PATCH 7/9] Update help menu for broker and discovery address Use `URI` and `URL` to be precise in what we accept for the broker address and the discovery service address. Also use backticks so that `URI`/`URL` shows up in the help menu instead of `value`. Signed-off-by: tcharding --- cmd/flags_network.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/flags_network.go b/cmd/flags_network.go index c0e06a2035..a474670845 100644 --- a/cmd/flags_network.go +++ b/cmd/flags_network.go @@ -45,12 +45,12 @@ var ( discoveryAddressFlag = cli.StringFlag{ Name: "discovery-address", - Usage: "Address (URL form) of discovery service", + Usage: "`URL` of discovery service", Value: metadata.DefaultNetwork.DiscoveryAPIAddress, } brokerAddressFlag = cli.StringFlag{ Name: "broker-address", - Usage: "Address (IP or domain name) of message broker", + Usage: "`URI` of message broker", Value: metadata.DefaultNetwork.BrokerAddress, } From 6de80649ef6461461d53a6c4e4eeb3c0b582adee Mon Sep 17 00:00:00 2001 From: tcharding Date: Wed, 31 Oct 2018 13:42:15 +1100 Subject: [PATCH 8/9] Capitalise Url in help output Currently we have help output `Url`, we should use capitals for acronyms here as we do in the rest of the help menu. Capitalise Url in help output, se 'URL' instead of 'Url'. Signed-off-by: tcharding --- cmd/flags_network.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/flags_network.go b/cmd/flags_network.go index a474670845..8ee8db4944 100644 --- a/cmd/flags_network.go +++ b/cmd/flags_network.go @@ -56,7 +56,7 @@ var ( etherRpcFlag = cli.StringFlag{ Name: "ether.client.rpc", - Usage: "Url or IPC socket to connect to ethereum node, anything what ethereum client accepts - works", + Usage: "URL or IPC socket to connect to ethereum node, anything what ethereum client accepts - works", Value: metadata.DefaultNetwork.EtherClientRPC, } etherContractPaymentsFlag = cli.StringFlag{ From a2689820a1d69d3e37b4cd84da19269b4131d215 Mon Sep 17 00:00:00 2001 From: Dmitry Shihovtsev Date: Thu, 15 Nov 2018 16:25:26 +0600 Subject: [PATCH 9/9] Fixed auto merge and conflict resolution issue. --- cmd/di.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/cmd/di.go b/cmd/di.go index 032257a035..125da55653 100644 --- a/cmd/di.go +++ b/cmd/di.go @@ -244,12 +244,6 @@ func (di *Dependencies) bootstrapServiceComponents(nodeOptions node.Options) { discoveryService := discovery.NewService(di.IdentityRegistry, di.IdentityRegistration, di.MysteriumClient, di.SignerFactory) - sessionStorage := session.NewStorageMemory() - newService := func(serviceOptions service.Options) (service.Service, error) { - transportOptions := serviceOptions.Options.(openvpn_service.Options) - return openvpn_service.NewManager(nodeOptions, transportOptions, di.IPResolver, di.LocationResolver, sessionStorage), nil - } - newDialogWaiter := func(providerID identity.Identity) (communication.DialogWaiter, error) { address, err := nats_discovery.NewAddressFromHostAndID(di.NetworkDefinition.BrokerAddress, providerID) if err != nil {