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

345 Client Get Country #207

Merged
merged 21 commits into from Mar 21, 2018

Conversation

Projects
None yet
4 participants
@interro
Copy link
Contributor

commented Mar 15, 2018

No description provided.

@tadovas
Copy link
Member

left a comment

I am missing a simple test for detectCountry function

func detectCountry(ipResolver ip.Resolver, locationDetector location.Detector) (string, error) {
ip, err := ipResolver.GetPublicIP()
if err != nil {
return "", errors.New("IP detection failed: " + err.Error())

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 15, 2018

Member

Simply return error, don't wrap it into new one.

This comment has been minimized.

Copy link
@interro

interro Mar 15, 2018

Author Contributor

done


country, err := locationDetector.DetectCountry(ip)
if err != nil {
return "", errors.New("Country detection failed: " + err.Error())

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 15, 2018

Member

Same here - return error, dont create new one

This comment has been minimized.

Copy link
@interro

interro Mar 15, 2018

Author Contributor

done

@@ -34,7 +40,8 @@ func ConfigureVpnClientFactory(
signer := signerFactory(consumerID)

statsSaver := bytescount.NewSessionStatsSaver(statsKeeper)
statsSender := bytescount.NewSessionStatsSender(mysteriumAPIClient, vpnSession.ID, providerID, signer)
clientCountry, _ := detectCountry(ipResolver, locationDetector)

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 15, 2018

Member

Don't swallow error

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 15, 2018

Member

I would not throw error also. Lets implement logging. So that, client knows what is happening

This comment has been minimized.

Copy link
@interro

interro Mar 15, 2018

Author Contributor

made error checking

@@ -4,4 +4,5 @@ type SessionStats struct {
BytesSent int `json:"bytes_sent"`
BytesReceived int `json:"bytes_received"`
ProviderID string `json:"provider_id"`
ClientCountry string `json:"client_country"`

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 15, 2018

Member

why not just "country" ? client/server terms should disappear in long term.

This comment has been minimized.

Copy link
@donce

donce Mar 15, 2018

Contributor

Since these stats are for session, it's good to indicate which country it is, since there are 2 countries in session.
Just not ClientCountry, but ConsumerCountry.

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 15, 2018

Member

We have Providers and Consumer (server&client are software pieces)

This comment has been minimized.

Copy link
@interro

interro Mar 15, 2018

Author Contributor

renamed to ConsumerCountry

@@ -19,6 +23,8 @@ func ConfigureVpnClientFactory(
openvpnBinary, configDirectory, runtimeDirectory string,
signerFactory identity.SignerFactory,
statsKeeper bytescount.SessionStatsKeeper,
ipResolver ip.Resolver,

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 15, 2018

Member

Instead of ipResolver and locationDetector dependencies, it could be simple dependency - detectCountry function.

@@ -57,3 +64,18 @@ func ConfigureVpnClientFactory(
), nil
}
}

func detectCountry(ipResolver ip.Resolver, locationDetector location.Detector) (string, error) {

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 15, 2018

Member

This function can live on it's own package, it's not related to openvpn anyhow

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 15, 2018

Member

Plus its duplicated currently

This comment has been minimized.

Copy link
@interro

interro Mar 15, 2018

Author Contributor

now this function lives in location package

@@ -34,7 +40,8 @@ func ConfigureVpnClientFactory(
signer := signerFactory(consumerID)

statsSaver := bytescount.NewSessionStatsSaver(statsKeeper)
statsSender := bytescount.NewSessionStatsSender(mysteriumAPIClient, vpnSession.ID, providerID, signer)
clientCountry, _ := detectCountry(ipResolver, locationDetector)

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 15, 2018

Member

I would not throw error also. Lets implement logging. So that, client knows what is happening

@@ -51,13 +52,19 @@ func NewCommandWith(

statsKeeper := bytescount.NewSessionStatsKeeper(time.Now)

ipResolver := ip.NewResolver(options.IpifyUrl)

locationDetector := location.NewDetector(filepath.Join(options.DirectoryConfig, options.LocationDatabase))

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 15, 2018

Member

We need to start distributing DB file together with mysterium_client:

  • bin/client_package_debian
  • bin/client_docker/alpine/Dockerfile
@@ -121,13 +121,13 @@ func (cmd *Command) Start() (err error) {
return nil
}

func detectCountry(ipResolver ip.Resolver, locationDetector location.Detector) (dto_discovery.Location, error) {
func detectCountry(ipResolver ip.Resolver, locationResolver location.Resolver) (dto_discovery.Location, error) {

This comment has been minimized.

Copy link
@donce

donce Mar 15, 2018

Contributor

We can re-use country detector here as well to avoid duplicating logic behind detect country.

This comment has been minimized.

Copy link
@interro

interro Mar 16, 2018

Author Contributor

done reusing

// DetectCountry maps given ip to country
func (d *detector) DetectCountry(ip string) (string, error) {
db, err := geoip2.Open(d.databasePath)
func (d *detector) DetectCountry() (string, error) {

This comment has been minimized.

Copy link
@donce

donce Mar 15, 2018

Contributor

Missing doc comments. bin/lint_git can show more warnings, just be aware that not all of them are related to your changes.

// DetectCountry maps given ip to country
func (d *detector) DetectCountry(ip string) (string, error) {
db, err := geoip2.Open(d.databasePath)
func (d *detector) DetectCountry() (string, error) {

This comment has been minimized.

Copy link
@donce

donce Mar 15, 2018

Contributor

This can be tested using fakeResolver.

return country, nil
}
}

This comment has been minimized.

Copy link
@donce

donce Mar 15, 2018

Contributor

Please set-up the browser to avoid road-block signs :D


detectedCountry, err := locationDetector.DetectCountry()
if err != nil {
log.Error("Failed to detect country", err)

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 16, 2018

Member

Lower -1 level for this log (Warning)

This comment has been minimized.

Copy link
@interro

interro Mar 19, 2018

Author Contributor

done

@@ -34,7 +37,21 @@ func ConfigureVpnClientFactory(
signer := signerFactory(consumerID)

statsSaver := bytescount.NewSessionStatsSaver(statsKeeper)
statsSender := bytescount.NewSessionStatsSender(mysteriumAPIClient, vpnSession.ID, providerID, signer)

detectedCountry, err := locationDetector.DetectCountry()

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 16, 2018

Member

consumerCountry

@@ -69,10 +68,14 @@ func (cmd *Command) Start() (err error) {
return err
}

serviceLocation, err := detectCountry(cmd.ipResolver, cmd.locationDetector)
locationDetector := location.NewDetectorWithLocationResolver(cmd.ipResolver, cmd.locationResolver)

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 16, 2018

Member

locationDetector should be constructed and injected already

This comment has been minimized.

Copy link
@interro

interro Mar 19, 2018

Author Contributor

done

Merge remote-tracking branch 'origin/master' into feature/MYST-345-cl…
…ient-get-country

# Conflicts:
#	cmd/commands/server/command_server.go
@tadovas
Copy link
Member

left a comment

LGTM

return &detector{
databasePath: databasePath,
ipResolver: ipResolver,
locationResolver: NewResolver(databasePath),
}

This comment has been minimized.

Copy link
@donce

donce Mar 21, 2018

Contributor

This constructor can re-use other-one, that is:

return NewDetectorWithLocationResolver(ipResolver, NewResolver(databasePath))

That way, only one constructor has all the low-level struct initialisation stuff.


countryRecord, err := db.Country(ipObject)
// Maps current ip to country

This comment has been minimized.

Copy link
@donce

donce Mar 21, 2018

Contributor

Method commets should start with method name:

// DetectCountry maps current ip to country
country = countryRecord.RegisteredCountry.IsoCode
}

return country, nil

This comment has been minimized.

Copy link
@donce

donce Mar 21, 2018

Contributor

Value of country can still be "" here when countryRecord.RegisteredCountry.IsoCode is "" - we should return err in that case. That way, we can assume country is returned if err is nil.

@donce

donce approved these changes Mar 21, 2018

@interro interro merged commit c411d4c into master Mar 21, 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

@interro interro deleted the feature/MYST-345-client-get-country branch Mar 21, 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.