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

MYST 532 Tequilapi new Location Endpoint #235

Merged
merged 9 commits into from May 11, 2018
12 changes: 3 additions & 9 deletions client/connection/openvpn.go
@@ -1,7 +1,6 @@
package connection

import (
log "github.com/cihub/seelog"
"github.com/mysterium/node/identity"
"github.com/mysterium/node/location"
"github.com/mysterium/node/openvpn"
Expand All @@ -21,7 +20,7 @@ func ConfigureVpnClientFactory(
openvpnBinary, configDirectory, runtimeDirectory string,
signerFactory identity.SignerFactory,
statsKeeper bytescount.SessionStatsKeeper,
locationDetector location.Detector,
locationCache location.Cache,
) VpnClientCreator {
return func(vpnSession session.SessionDto, consumerID identity.Identity, providerID identity.Identity, stateCallback state.Callback) (openvpn.Client, error) {
vpnClientConfig, err := openvpn.NewClientConfigFromString(
Expand All @@ -38,19 +37,14 @@ func ConfigureVpnClientFactory(

statsSaver := bytescount.NewSessionStatsSaver(statsKeeper)

consumerCountry, err := locationDetector.DetectCountry()
Copy link
Member

Choose a reason for hiding this comment

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

Why not detecting on connection start?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

Choose a reason for hiding this comment

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

Does it mean that we are always getting startup location?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Getting location on app start

if err != nil {
log.Warn("Failed to detect country", err)
} else {
log.Info("Country detected: ", consumerCountry)
}
originalLocation, _ := locationCache.Get()

statsSender := bytescount.NewSessionStatsSender(
mysteriumAPIClient,
vpnSession.ID,
providerID,
signer,
consumerCountry,
originalLocation.Country,
)
asyncStatsSender := func(stats bytescount.SessionStats) error {
go statsSender(stats)
Expand Down
16 changes: 14 additions & 2 deletions cmd/commands/client/command_client.go
Expand Up @@ -61,14 +61,16 @@ func NewCommandWith(
filepath.Join(options.DirectoryConfig, options.LocationDatabase),
)

locationCache := location.NewLocationCache(locationDetector)

vpnClientFactory := connection.ConfigureVpnClientFactory(
mysteriumClient,
options.OpenvpnBinary,
options.DirectoryConfig,
options.DirectoryRuntime,
signerFactory,
statsKeeper,
locationDetector,
locationCache,
)
connectionManager := connection.NewManager(mysteriumClient, dialogFactory, vpnClientFactory, statsKeeper)

Expand All @@ -82,10 +84,12 @@ func NewCommandWith(
checkOpenvpn: func() error {
return openvpn.CheckOpenvpnBinary(options.OpenvpnBinary)
},
locationCache: locationCache,
}

tequilapi_endpoints.AddRoutesForIdentities(router, identityManager, mysteriumClient, signerFactory)
tequilapi_endpoints.AddRoutesForConnection(router, connectionManager, ipResolver, statsKeeper)
tequilapi_endpoints.AddRoutesForLocation(router, locationDetector, locationCache)
tequilapi_endpoints.AddRoutesForProposals(router, mysteriumClient)
tequilapi_endpoints.AddRouteForStop(router, node_cmd.NewApplicationStopper(command.Kill))

Expand All @@ -97,9 +101,10 @@ type Command struct {
connectionManager connection.Manager
httpAPIServer tequilapi.APIServer
checkOpenvpn func() error
locationCache location.Cache
}

// Start starts Tequilapi service - does not block
// Start starts Tequilapi service, fetches location
func (cmd *Command) Start() error {
log.Info("[Client version]", version.AsString())
err := cmd.checkOpenvpn()
Expand All @@ -118,6 +123,13 @@ func (cmd *Command) Start() error {
}
log.Infof("Api started on: %d", port)

originalLocation, err := cmd.locationCache.RefreshAndGet()
Copy link
Member

Choose a reason for hiding this comment

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

Why are't You using/injecting variable originalLocation anythere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because after calling RefreshAndGet() location is fetched and stored in cache for future uses

Copy link
Contributor

@tadovas tadovas May 10, 2018

Choose a reason for hiding this comment

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

By the way - I suggest moving log.xx line after location is fetched. In case of error it should not display Api started... and then error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if err != nil {
log.Warn("Failed to detect country", err)
} else {
log.Info("Country detected: ", originalLocation.Country)
}

return nil
}

Expand Down
6 changes: 3 additions & 3 deletions cmd/commands/server/command_server.go
Expand Up @@ -71,12 +71,12 @@ func (cmd *Command) Start() (err error) {
return err
}

serviceCountry, err := cmd.locationDetector.DetectCountry()
location, err := cmd.locationDetector.DetectLocation()
if err != nil {
return err
}
log.Info("Country detected: ", serviceCountry)
serviceLocation := dto_discovery.Location{Country: serviceCountry}
log.Info("Country detected: ", location.Country)
serviceLocation := dto_discovery.Location{Country: location.Country}

proposal := discovery.NewServiceProposalWithLocation(providerID, providerContact, serviceLocation, cmd.protocol)

Expand Down
9 changes: 5 additions & 4 deletions location/detector.go
Expand Up @@ -23,16 +23,17 @@ func NewDetectorWithLocationResolver(ipResolver ip.Resolver, locationResolver Re
}

// Maps current ip to country
func (d *detector) DetectCountry() (string, error) {
func (d *detector) DetectLocation() (Location, error) {
ip, err := d.ipResolver.GetPublicIP()
if err != nil {
return "", err
return Location{}, err
}

country, err := d.locationResolver.ResolveCountry(ip)
if err != nil {
return "", err
return Location{}, err
}

return country, nil
location := Location{Country: country, IP: ip}
return location, nil
}
15 changes: 8 additions & 7 deletions location/detector_test.go
Expand Up @@ -10,26 +10,27 @@ import (
func TestNewDetector(t *testing.T) {
ipResolver := ip.NewFakeResolver("8.8.8.8")
detector := NewDetector(ipResolver, "../bin/client_package/config/GeoLite2-Country.mmdb")
country, err := detector.DetectCountry()
assert.Equal(t, "US", country)
location, err := detector.DetectLocation()
assert.Equal(t, "US", location.Country)
assert.Equal(t, "8.8.8.8", location.IP)
assert.NoError(t, err)
}

func TestWithIpResolverFailing(t *testing.T) {
ipErr := errors.New("ip resolver error")
ipResolver := ip.NewFailingFakeResolver(ipErr)
detector := NewDetectorWithLocationResolver(ipResolver, NewResolverFake(""))
country, err := detector.DetectCountry()
location, err := detector.DetectLocation()
assert.EqualError(t, ipErr, err.Error())
assert.Equal(t, "", country)
assert.Equal(t, Location{}, location)
}

func TestWithLocationResolverFailing(t *testing.T) {
ipResolver := ip.NewFakeResolver("")
locationErr := errors.New("location resolver error")
locationResolver := NewFailingResolverFake(locationErr)
detector := NewDetectorWithLocationResolver(ipResolver, locationResolver)
country, err := detector.DetectCountry()
location, err := detector.DetectLocation()
assert.EqualError(t, locationErr, err.Error())
assert.Equal(t, "", country)
}
assert.Equal(t, Location{}, location)
}
8 changes: 7 additions & 1 deletion location/interface.go
Expand Up @@ -7,5 +7,11 @@ type Resolver interface {

// Detector allows detecting location by current ip
type Detector interface {
DetectCountry() (string, error)
DetectLocation() (Location, error)
}

// Cache allows caching location
type Cache interface {
Get() (Location, error)
RefreshAndGet() (Location, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of uses of RefreshAndGet seems to be made only for Refresh part, result is ignored.
We can simplify this method to assigning single responsibility to it - just Refresh().

Copy link
Member

@Waldz Waldz May 11, 2018

Choose a reason for hiding this comment

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

I agree. And printing can just take with cache.Get()

}
6 changes: 6 additions & 0 deletions location/location.go
@@ -0,0 +1,6 @@
package location

type Location struct {
IP string `json:"ip"`
Country string `json:"country"`
}
27 changes: 27 additions & 0 deletions location/location_cache.go
@@ -0,0 +1,27 @@
package location

type locationCache struct {
locationDetector Detector
location Location
err error
}

// NewLocationCache constructs Cache
func NewLocationCache(locationDetector Detector) Cache {
return &locationCache{
locationDetector: locationDetector,
}
}

// Gets location from cache
func (lc *locationCache) Get() (Location, error) {
return lc.location, lc.err
}

// Stores location to cache
func (lc *locationCache) RefreshAndGet() (Location, error) {
location, err := lc.locationDetector.DetectLocation()
lc.location = location
lc.err = err
return lc.location, lc.err
}
45 changes: 45 additions & 0 deletions location/location_cache_test.go
@@ -0,0 +1,45 @@
package location

import (
"testing"
"errors"
"github.com/mysterium/node/ip"
"github.com/stretchr/testify/assert"
)

func TestLocationCacheFirstCall(t *testing.T) {
ipResolver := ip.NewFakeResolver("100.100.100.100")
locationResolver := NewResolverFake("country")
locationDetector := NewDetectorWithLocationResolver(ipResolver, locationResolver)
locationCache := NewLocationCache(locationDetector)
location, err := locationCache.Get()
assert.Equal(t, Location{}, location)
assert.NoError(t, err)
}

func TestLocationCacheFirstSecondCalls(t *testing.T) {
ipResolver := ip.NewFakeResolver("100.100.100.100")
locationResolver := NewResolverFake("country")
locationDetector := NewDetectorWithLocationResolver(ipResolver, locationResolver)
locationCache := NewLocationCache(locationDetector)
location, err := locationCache.RefreshAndGet()
assert.Equal(t, "country", location.Country)
assert.Equal(t, "100.100.100.100", location.IP)
assert.NoError(t, err)

locationSecondCall, err := locationCache.Get()
assert.Equal(t, location, locationSecondCall)
assert.NoError(t, err)
}

func TestLocationCacheWithError(t *testing.T) {
ipResolver := ip.NewFakeResolver("")
locationErr := errors.New("location resolver error")
locationResolver := NewFailingResolverFake(locationErr)
locationDetector := NewDetectorWithLocationResolver(ipResolver, locationResolver)
locationCache := NewLocationCache(locationDetector)
locationCache.RefreshAndGet()
location, err := locationCache.Get()
assert.EqualError(t, locationErr, err.Error())
assert.Equal(t, Location{}, location)
}
3 changes: 1 addition & 2 deletions tequilapi/endpoints/connection_test.go
Expand Up @@ -49,9 +49,8 @@ func TestAddRoutesForConnectionAddsRoutes(t *testing.T) {
router := httprouter.New()
fakeManager := fakeManager{}
settableClock := utils.SettableClock{}
ipResolver := ip.NewFakeResolver("123.123.123.123")
statsKeeper := bytescount.NewSessionStatsKeeper(settableClock.GetTime)

ipResolver := ip.NewFakeResolver("123.123.123.123")
sessionStart := time.Date(2000, time.January, 0, 10, 0, 0, 0, time.UTC)
settableClock.SetTime(sessionStart)
statsKeeper.MarkSessionStart()
Expand Down
52 changes: 52 additions & 0 deletions tequilapi/endpoints/location.go
@@ -0,0 +1,52 @@
package endpoints

import (
"github.com/julienschmidt/httprouter"
"github.com/mysterium/node/location"
"github.com/mysterium/node/tequilapi/utils"
"net/http"
)

// LocationEndpoint struct represents /location resource and it's subresources
type LocationEndpoint struct {
locationDetector location.Detector
locationCache location.Cache
}

// NewLocationEndpoint creates and returns location endpoint
func NewLocationEndpoint(locationDetector location.Detector, locationCache location.Cache) *LocationEndpoint {
return &LocationEndpoint{
locationDetector: locationDetector,
locationCache: locationCache,
}
}

// GetLocation responds with original and current countries
func (ce *LocationEndpoint) GetLocation(writer http.ResponseWriter, request *http.Request, params httprouter.Params) {
originalLocation, err := ce.locationCache.Get()
Copy link
Contributor

Choose a reason for hiding this comment

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

If we failed to get location (original) on startup do we always display that error?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would make it '(le *LocationEndpoint)', not '(ce *LocationEndpoint)'

Copy link
Contributor

Choose a reason for hiding this comment

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

If we failed to get location (original) on startup do we always display that error?

In case only one of original and current ip is not available, it makes sense to return what we have. I.e.

{
  original: null,
  current: "1.1.1.1"
}

or

{
  original: "1.1.1.1",
  current: null
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tadovas changed logic where locationCache.Get() no longer returns error so the answer is no, we display error on startup only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@donce Currently if some data is not available, it has empty value

{
 "original": {
  "ip": "", 
  "country": ""
 },
 "current": {
  "ip": "1.1.1.1",
  "country": ""
 }
}

if err != nil {
utils.SendError(writer, err, http.StatusServiceUnavailable)
return
}

currentLocation, err := ce.locationDetector.DetectLocation()
if err != nil {
utils.SendError(writer, err, http.StatusServiceUnavailable)
return
}

response := struct {
Original location.Location `json:"original"`
Copy link
Contributor

Choose a reason for hiding this comment

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

original looks confusing - it doesn't specify what this location actually means. Maybe startup would be more clear?

Copy link
Contributor

@tadovas tadovas May 10, 2018

Choose a reason for hiding this comment

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

startup exposes 'when' it was looked up, while 'original' represents real collected country. I would go for original

Copy link
Member

Choose a reason for hiding this comment

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

original is just perfect

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, when you put it like this, it does make sense - but at first look, it didn't

Current location.Location `json:"current"`
}{
Original: originalLocation,
Current: currentLocation,
}
utils.WriteAsJSON(response, writer)
}

// AddRoutesForLocation adds location routes to given router
func AddRoutesForLocation(router *httprouter.Router, locationDetector location.Detector, locationCache location.Cache) {
locationEndpoint := NewLocationEndpoint(locationDetector, locationCache)
router.GET("/location", locationEndpoint.GetLocation)
}