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.

Sign up for GitHub

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

Jump to bottom

MYST 361: add -discovery-address and -broker-address command line options #172

Merged
merged 8 commits into from Feb 26, 2018
Merged

MYST 361: add -discovery-address and -broker-address command line options #172

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
165b9bd
add -discovery-address and -broker-address command line options to ov…
zolia Feb 21, 2018
3df88d7
client does not need hardcoded broker IP - removed
zolia Feb 21, 2018
58ae6d5
fixed packges; client reuses the same external MysteriumApiUrl
zolia Feb 22, 2018
e6fbb7d
aMoved request functions to seaprate package. Could be reused in the …
zolia Feb 26, 2018
f75ce0a
env arguments with -- changed
zolia Feb 26, 2018
bd4d4ac
introduce MYSTERIUM_DISCOVERY_ADDRESS and MYSTERIUM_BROKER_ADDRESS en…
zolia Feb 26, 2018
41a53ab
irequestor package renamed to requests
zolia Feb 26, 2018
6791b53
naming fixes
zolia Feb 26, 2018
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
8 changes: 7 additions & 1 deletion bin/client_run
View file
Open in desktop
@@ -1,3 +1,9 @@
#!/bin/bash

sudo ./build/client/mysterium_client --config-dir=bin/client_package/config --runtime-dir=build/client $@
source bin/helpers/load_run_environment.sh

sudo ./build/client/mysterium_client \
--config-dir=bin/client_package/config \
--runtime-dir=build/client \
$DISCOVERY \
Copy link
Contributor

@donce donce Feb 26, 2018 •
edited

Choose a reason for hiding this comment

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

Shouldn't this param be passed in docker-entrypoints as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it depends from environment we want to build for.. we should compile in for testnet and only for development to show our own discovery.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is is passed now

$@
3 changes: 2 additions & 1 deletion bin/helpers/functions.sh
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@
function get_linker_ldflags {
echo "
-X 'github.com/mysterium/node/server.mysteriumApiUrl=${MYSTERIUM_API_URL}'
-X 'github.com/mysterium/node/communication/nats/discovery.natsServerIP=${NATS_SERVER_IP}'
-X 'github.com/mysterium/node/server.natsServerIP=${NATS_SERVER_IP}'
-X 'github.com/mysterium/node/client.mysteriumApiUrl=${MYSTERIUM_API_URL}'
Copy link
Member

Choose a reason for hiding this comment

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

Why mysteriumApiUrl several times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its in different packages. to remove duplication I could make it public..

"
}
8 changes: 8 additions & 0 deletions bin/helpers/load_run_environment.sh
View file
Open in desktop
@@ -0,0 +1,8 @@
#!/bin/bash

if [ -f .env ]; then
source .env

[ -n "$MYSTERIUM_API_URL" ] && DISCOVERY="-discovery-address=$MYSTERIUM_API_URL"
[ -n "$NATS_SERVER_IP" ] && BROKER="-broker-address=$NATS_SERVER_IP"
Copy link
Contributor

@donce donce Feb 26, 2018 •
edited

Choose a reason for hiding this comment

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

Isn't too dashes common practice for key-value params?

--discovery-address=FOO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but its true for all options. Legacy.

fi
9 changes: 8 additions & 1 deletion bin/server_run
View file
Open in desktop
@@ -1,3 +1,10 @@
#!/bin/bash

sudo ./build/server/mysterium_server --config-dir=bin/server_package/config --runtime-dir=build/server $@
source bin/helpers/load_run_environment.sh

sudo ./build/server/mysterium_server \
--config-dir=bin/server_package/config \
--runtime-dir=build/server \
$DISCOVERY \
$BROKER \
$@
2 changes: 1 addition & 1 deletion cmd/commands/client/command_client.go
View file
Open in desktop
Expand Up @@ -23,7 +23,7 @@ import (
func NewCommand(options CommandOptions) *Command {
return NewCommandWith(
options,
server.NewClient(),
server.NewClient(options.DiscoveryAPIAddress),
)
}

Expand Down
12 changes: 12 additions & 0 deletions cmd/commands/client/options.go
View file
Open in desktop
Expand Up @@ -16,8 +16,13 @@ type CommandOptions struct {
TequilapiPort int

CLI bool

DiscoveryAPIAddress string
BrokerAddress string
}

var mysteriumApiUrl string

// ParseArguments parses CLI flags and adds to CommandOptions structure
func ParseArguments(args []string) (options CommandOptions, err error) {
flags := flag.NewFlagSet(args[0], flag.ContinueOnError)
Expand Down Expand Up @@ -60,6 +65,13 @@ func ParseArguments(args []string) (options CommandOptions, err error) {
"Run an interactive CLI based Mysterium UI",
)

flags.StringVar(
&options.DiscoveryAPIAddress,
"discovery-address",
mysteriumApiUrl,
"Address (URL form) of discovery service",
)

err = flags.Parse(args[1:])
if err != nil {
return
Expand Down
4 changes: 2 additions & 2 deletions cmd/commands/server/factory.go
View file
Open in desktop
Expand Up @@ -24,7 +24,7 @@ import (
func NewCommand(options CommandOptions) *Command {
return NewCommandWith(
options,
server.NewClient(),
server.NewClient(options.DiscoveryAPIAddress),
ip.NewResolver(),
nat.NewService(),
)
Expand Down Expand Up @@ -71,7 +71,7 @@ func NewCommandWith(
natService: natService,
dialogWaiterFactory: func(myID identity.Identity) communication.DialogWaiter {
return nats_dialog.NewDialogWaiter(
nats_discovery.NewAddressGenerate(myID),
nats_discovery.NewAddressGenerate(options.BrokerAddress, myID),
identity.NewSigner(keystoreInstance, myID),
)
},
Expand Down
19 changes: 19 additions & 0 deletions cmd/commands/server/options.go
View file
Open in desktop
Expand Up @@ -17,10 +17,16 @@ type CommandOptions struct {

LocationCountry string
LocationDatabase string

DiscoveryAPIAddress string
BrokerAddress string
}

const defaultLocationDatabase = "GeoLite2-Country.mmdb"

var mysteriumApiUrl string
var natsServerIP string
Copy link
Contributor

Choose a reason for hiding this comment

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

brokerIP?


// ParseArguments parses CLI flags and adds to CommandOptions structure
func ParseArguments(args []string) (options CommandOptions, err error) {
flags := flag.NewFlagSet(args[0], flag.ContinueOnError)
Expand Down Expand Up @@ -69,6 +75,19 @@ func ParseArguments(args []string) (options CommandOptions, err error) {
"Service location country. If not given country is autodetected",
)

flags.StringVar(
&options.DiscoveryAPIAddress,
"discovery-address",
mysteriumApiUrl,
"Address (URL form) of discovery service",
)
flags.StringVar(
&options.BrokerAddress,
"broker-address",
natsServerIP,
"Address (IP or domain name) of message broker",
)

err = flags.Parse(args[1:])
if err != nil {
return
Expand Down
6 changes: 2 additions & 4 deletions communication/nats/discovery/address.go
View file
Open in desktop
Expand Up @@ -8,8 +8,6 @@ import (
nats_lib "github.com/nats-io/go-nats"
)

var natsServerIP string

// NewAddress creates NATS address to known host or cluster of hosts
func NewAddress(topic string, addresses ...string) *AddressNATS {
return &AddressNATS{
Expand All @@ -19,8 +17,8 @@ func NewAddress(topic string, addresses ...string) *AddressNATS {
}

// NewAddressGenerate generates NATS address for current node
func NewAddressGenerate(myID identity.Identity) *AddressNATS {
address := "nats://" + natsServerIP + ":4222"
func NewAddressGenerate(brokerIP string, myID identity.Identity) *AddressNATS {
address := "nats://" + brokerIP + ":4222"
return NewAddress(myID.Address, address)
}

Expand Down
5 changes: 3 additions & 2 deletions communication/nats/discovery/address_test.go
View file
Open in desktop
Expand Up @@ -23,12 +23,13 @@ func TestNewAddress(t *testing.T) {

func TestNewAddressGenerate(t *testing.T) {
myID := identity.FromAddress("provider1")
address := NewAddressGenerate(myID)
brokerIP := "127.0.0.1"
address := NewAddressGenerate(brokerIP, myID)

assert.Equal(
t,
&AddressNATS{
servers: []string{"nats://" + natsServerIP + ":4222"},
servers: []string{"nats://" + brokerIP + ":4222"},
topic: "provider1",
},
address,
Expand Down
16 changes: 9 additions & 7 deletions server/mysterium_api.go
View file
Open in desktop
Expand Up @@ -21,20 +21,22 @@ type HttpTransport interface {
}

type mysteriumAPI struct {
http HttpTransport
http HttpTransport
discoveryAPIAddress string
}

// NewClient creates Mysterium centralized api instance with real communication
func NewClient() Client {
func NewClient(discoveryAPIAddress string) Client {
return &mysteriumAPI{
&http.Client{
Transport: &http.Transport{},
},
discoveryAPIAddress,
}
}

func (mApi *mysteriumAPI) RegisterIdentity(id identity.Identity, signer identity.Signer) error {
req, err := newSignedPostRequest("identities", dto.CreateIdentityRequest{
req, err := newSignedPostRequest(mApi.discoveryAPIAddress, "identities", dto.CreateIdentityRequest{
Identity: id.Address,
}, signer)
if err != nil {
Expand All @@ -49,7 +51,7 @@ func (mApi *mysteriumAPI) RegisterIdentity(id identity.Identity, signer identity
}

func (mApi *mysteriumAPI) RegisterProposal(proposal dto_discovery.ServiceProposal, signer identity.Signer) error {
req, err := newSignedPostRequest("register_proposal", dto.NodeRegisterRequest{
req, err := newSignedPostRequest(mApi.discoveryAPIAddress, "register_proposal", dto.NodeRegisterRequest{
ServiceProposal: proposal,
}, signer)
if err != nil {
Expand All @@ -65,7 +67,7 @@ func (mApi *mysteriumAPI) RegisterProposal(proposal dto_discovery.ServiceProposa
}

func (mApi *mysteriumAPI) PingProposal(proposal dto_discovery.ServiceProposal, signer identity.Signer) error {
req, err := newSignedPostRequest("ping_proposal", dto.NodeStatsRequest{
req, err := newSignedPostRequest(mApi.discoveryAPIAddress, "ping_proposal", dto.NodeStatsRequest{
NodeKey: proposal.ProviderID,
}, signer)
if err != nil {
Expand All @@ -85,7 +87,7 @@ func (mApi *mysteriumAPI) FindProposals(providerID string) ([]dto_discovery.Serv
values.Set("node_key", providerID)
}

req, err := newGetRequest("proposals", values)
req, err := newGetRequest(mApi.discoveryAPIAddress, "proposals", values)
if err != nil {
return nil, err
}
Expand All @@ -103,7 +105,7 @@ func (mApi *mysteriumAPI) FindProposals(providerID string) ([]dto_discovery.Serv

func (mApi *mysteriumAPI) SendSessionStats(sessionId string, sessionStats dto.SessionStats, signer identity.Signer) error {
path := fmt.Sprintf("sessions/%s/stats", sessionId)
req, err := newSignedPostRequest(path, sessionStats, signer)
req, err := newSignedPostRequest(mApi.discoveryAPIAddress, path, sessionStats, signer)
if err != nil {
return err
}
Expand Down
18 changes: 8 additions & 10 deletions server/request.go
View file
Open in desktop
Expand Up @@ -15,22 +15,20 @@ const (
authenticationSchemaName = "Signature"
)

var mysteriumApiUrl string

func newGetRequest(path string, params url.Values) (*http.Request, error) {
func newGetRequest(apiURI, path string, params url.Values) (*http.Request, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since mApi.discoveryAPIAddress seems to be used as apiURL param value in all calls, does it make sense to parametrize it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure about this place, since its functions and not member methods we should not depend on internal state. Should we hardcoded in apiURI, can it be considered a state or never changing constant..

pathWithQuery := fmt.Sprintf("%v?%v", path, params.Encode())
return newRequest(http.MethodGet, pathWithQuery, nil)
return newRequest(http.MethodGet, apiURI, pathWithQuery, nil)
}

func newPostRequest(path string, requestBody interface{}) (*http.Request, error) {
func newPostRequest(apiURI, path string, requestBody interface{}) (*http.Request, error) {
bodyBytes, err := encodeToJSON(requestBody)
if err != nil {
return nil, err
}
return newRequest(http.MethodPost, path, bodyBytes)
return newRequest(http.MethodPost, apiURI, path, bodyBytes)
}

func newSignedPostRequest(path string, requestBody interface{}, signer identity.Signer) (*http.Request, error) {
func newSignedPostRequest(apiURI, path string, requestBody interface{}, signer identity.Signer) (*http.Request, error) {
bodyBytes, err := encodeToJSON(requestBody)
if err != nil {
return nil, err
Expand All @@ -41,7 +39,7 @@ func newSignedPostRequest(path string, requestBody interface{}, signer identity.
return nil, err
}

req, err := newRequest(http.MethodPost, path, bodyBytes)
req, err := newRequest(http.MethodPost, apiURI, path, bodyBytes)
if err != nil {
return nil, err
}
Expand All @@ -55,9 +53,9 @@ func encodeToJSON(value interface{}) ([]byte, error) {
return json.Marshal(value)
}

func newRequest(method, path string, body []byte) (*http.Request, error) {
func newRequest(method, apiURI, path string, body []byte) (*http.Request, error) {

fullUrl := fmt.Sprintf("%v/%v", mysteriumApiUrl, path)
fullUrl := fmt.Sprintf("%v/%v", apiURI, path)
req, err := http.NewRequest(method, fullUrl, bytes.NewBuffer(body))
if err != nil {
return nil, err
Expand Down
9 changes: 5 additions & 4 deletions server/request_test.go
View file
Open in desktop
Expand Up @@ -17,6 +17,8 @@ type mockedSigner struct {
signatureToReturn identity.Signature
}

var testMysteriumApiUrl = "http://testUrl"
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't depend that url is mysterium - it can be just testRequestURL.
Also, URL has to be upper-case.


func (signer *mockedSigner) Sign(message []byte) (identity.Signature, error) {
return signer.signatureToReturn, nil
}
Expand All @@ -25,19 +27,18 @@ func TestSignatureIsInsertedForSignedPost(t *testing.T) {

signer := mockedSigner{identity.SignatureBase64("deadbeef")}

req, err := newSignedPostRequest("/post-path", testPayload{"abc"}, &signer)
req, err := newSignedPostRequest(testMysteriumApiUrl, "/post-path", testPayload{"abc"}, &signer)
assert.NoError(t, err)
assert.Equal(t, req.Header.Get("Authorization"), "Signature deadbeef")
}

func TestDoGetContactsPassedValuesForUrl(t *testing.T) {
mysteriumApiUrl = "http://testUrl"

params := url.Values{}
params["param1"] = []string{"value1"}
params["param2"] = []string{"value2"}

req, err := newGetRequest("get-path", params)
req, err := newGetRequest(testMysteriumApiUrl, "get-path", params)

assert.NoError(t, err)
assert.Equal(t, "http://testUrl/get-path?param1=value1&param2=value2", req.URL.String())
Expand All @@ -46,7 +47,7 @@ func TestDoGetContactsPassedValuesForUrl(t *testing.T) {

func TestPayloadIsSerializedSuccessfullyForPostMethod(t *testing.T) {

req, err := newPostRequest("post-path", testPayload{"abc"})
req, err := newPostRequest(testMysteriumApiUrl, "post-path", testPayload{"abc"})

assert.NoError(t, err)

Expand Down
4 changes: 2 additions & 2 deletions server/response_test.go
View file
Open in desktop
Expand Up @@ -9,7 +9,7 @@ import (
)

func TestHttpErrorIsReportedAsErrorReturnValue(t *testing.T) {
req, err := newGetRequest("path", nil)
req, err := newGetRequest(testMysteriumApiUrl, "path", nil)
assert.NoError(t, err)

response := &http.Response{
Expand All @@ -26,7 +26,7 @@ type testResponse struct {

func TestHttpResponseBodyIsParsedCorrectly(t *testing.T) {

req, err := newGetRequest("path", nil)
req, err := newGetRequest(testMysteriumApiUrl, "path", nil)
assert.NoError(t, err)

response := &http.Response{
Expand Down