Compile in geoip2 database and avoid external file reading #281

tadovas · 2018-07-04T15:24:43Z

Idea is to reduce external file dependencies and to build-in as much data/info as possible. First candidate geoip database.
Short summary:

Geoip database is converted to few source files by generator.go
New option for builtin country resolving for both client/server - used by default
No need to package db for runtime anymore

Waldz · 2018-07-05T10:24:56Z

+100 kudos, awesome idea

Waldz · 2018-07-05T10:25:01Z

https://tech.townsourced.com/post/embedding-static-files-in-go/

tadovas · 2018-07-13T06:18:21Z

@zolia @Waldz this one is ready for review

donce

This PR is shown for me on each pull request reminder, so I'm putting request changes to hide it.

zolia

lgtm, just a note that we will need to remove 'bin/common_package/GeoLite2-Country.mmdb' and all related packaging references. Maybe in separate task.

stale · 2018-10-12T06:22:42Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

tadovas · 2018-10-15T11:04:30Z

It comes a bit important now

tadovas · 2018-10-15T12:57:02Z

@mysteriumnetwork/core you can all take a look, while its being verified by CI

tadovas · 2018-10-15T13:13:18Z

@Waldz @vkuznecovas ping. Ready for review

Waldz · 2018-10-15T13:22:12Z

cmd/flags_location.go

@@ -28,8 +28,12 @@ var (
 		Usage: "Address (URL form) of ipify service",
 		Value: "https://api.ipify.org/",
 	}
+	locationBuiltInDbFlag = cli.BoolTFlag{


Dont need this flag, compiled-in should be default

Waldz · 2018-10-15T13:23:03Z

cmd/di.go

-		di.LocationResolver = location.NewResolverFake(options.Country)
+		di.LocationResolver = location.NewStaticResolver(options.Country)
+	case options.BuiltInDb:
+		di.LocationResolver = location.NewBuiltInResolver()


NewBuiltInResolver should be default resolver if no other option other resolver given

Waldz · 2018-10-15T13:54:23Z

core/location/built_in_resolver.go

+		return "", errors.New("failed to parse IP")
+	}
+
+	countryRecord, err := r.db.Country(ipObject)


Looks like duplicated logic, lets DRY with composition:

DBResolver{ db resource } BuiltinDBResolver { Resolver DBResolver }

Waldz · 2018-10-15T13:56:28Z

core/node/options_location.go

-	Country  string
+	IpifyUrl   string
+	ExternalDb string
+	BuiltInDb  bool


If build-in is default, we dont need it

True. Removed

vkuznecovas · 2018-10-15T13:36:27Z

core/location/built_in_resolver.go

+
+// NewBuiltInResolver returns new BuiltInDbResolver
+func NewBuiltInResolver() Resolver {
+


the empty line at the start of the func seems odd.

vkuznecovas · 2018-10-15T13:36:31Z

core/location/built_in_resolver.go

+
+// ResolveCountry maps given ip to country
+func (r *BuiltInDbResolver) ResolveCountry(ip string) (string, error) {
+


the empty line at the start of the func seems odd.

vkuznecovas · 2018-10-15T13:57:42Z

core/location/built_in_resolver.go

+
+	country := countryRecord.Country.IsoCode
+	if country == "" {
+		country = countryRecord.RegisteredCountry.IsoCode


if country = countryRecord.RegisteredCountry.IsoCode; country == "" {
return "", errors.New("failed to resolve country")
}

Well I am not sure if this looks more readable :)

tadovas · 2018-10-15T15:39:28Z

@Waldz @vkuznecovas all comments addressed. Ready for re-review

soffokl

btw, did we try to use existing libraries that allows embedding files into executable binary?

soffokl · 2018-10-16T04:42:02Z

cmd/di.go

@@ -43,6 +41,7 @@ import (
 	"github.com/mysteriumnetwork/node/services/openvpn"
 	openvpn_service "github.com/mysteriumnetwork/node/services/openvpn/service"
 	"github.com/mysteriumnetwork/node/session"
+	"path/filepath"


This does not look goimports formatted.

bin/check_goimports: line 23: goimports: command not found
hm, looks like we have broken goimports check.

Created issue for fixing goimports on the CI.

Fixed code.

core/location/built_in_resolver_test.go

soffokl · 2018-10-16T04:57:41Z

core/location/gendb/loader.go

+
+// EncodedDataLoader returns emmbeded database as byte array
+func EncodedDataLoader(data string, originalSize int, compressed bool) (decompressed []byte, err error) {
+	var reader io.Reader


This can be just reader := base64.NewDecoder(base64.RawStdEncoding, strings.NewReader(data))

It cannot. Because of the compressed flag, reader can be further decorated with gzip.Reader

Sorry. Got your point. It can be just reader := ...
Thanks.

tadovas · 2018-10-16T05:54:15Z

Yea I checked a few embedding libraries, didn't find small and efficient enough, they all were either to big, ugly to use, or not compatible with go generate style integration

soffokl

Looks good to me.

soffokl · 2018-10-16T06:38:19Z

core/location/gendb/db_index.go

+const originalSize = 3303090
+const dbData = "" + dbDataPart0 + dbDataPart1
+
+// LoadData returns emmbeded database as byte array


typo: emmbeded -> embedded

Waldz · 2018-10-16T10:51:00Z

cmd/flags_location.go

@@ -29,9 +29,9 @@ var (
 		Value: "https://api.ipify.org/",
 	}
 	locationDatabaseFlag = cli.StringFlag{
-		Name:  "location.database",
+		Name:  "location.db.external",


Lets not change contract

location.database

tadovas · 2018-10-16T13:00:59Z

@soffokl @vkuznecovas reapprove please

tadovas requested review from shroomist, donce, Waldz and zolia July 4, 2018 15:24

tadovas force-pushed the improvement/build-in-geoip-db branch 6 times, most recently from 68d1164 to 6cf482b Compare July 13, 2018 06:07

tadovas changed the title ~~WIP: Compile in geoip2 database and avoid external file reading~~ Compile in geoip2 database and avoid external file reading Jul 13, 2018

Waldz changed the title ~~Compile in geoip2 database and avoid external file reading~~ WIP Compile in geoip2 database and avoid external file reading Aug 15, 2018

donce suggested changes Aug 31, 2018

View reviewed changes

zolia previously approved these changes Aug 31, 2018

View reviewed changes

Waldz added the work in progress label Sep 18, 2018

stale bot added the stale label Oct 12, 2018

tadovas changed the title ~~WIP Compile in geoip2 database and avoid external file reading~~ Compile in geoip2 database and avoid external file reading Oct 15, 2018

stale bot removed the stale label Oct 15, 2018

tadovas dismissed zolia’s stale review via 92847e4 October 15, 2018 11:07

tadovas force-pushed the improvement/build-in-geoip-db branch 5 times, most recently from eb26da5 to 4d9a653 Compare October 15, 2018 12:54

tadovas removed the work in progress label Oct 15, 2018

Waldz requested changes Oct 15, 2018

View reviewed changes

vkuznecovas previously approved these changes Oct 15, 2018

View reviewed changes

tadovas dismissed vkuznecovas’s stale review via 2b7555b October 15, 2018 15:37

tadovas force-pushed the improvement/build-in-geoip-db branch 2 times, most recently from 2b7555b to e4aed78 Compare October 15, 2018 15:39

tadovas force-pushed the improvement/build-in-geoip-db branch from e4aed78 to dde38ea Compare October 15, 2018 15:49

soffokl reviewed Oct 16, 2018

View reviewed changes

tadovas force-pushed the improvement/build-in-geoip-db branch from dde38ea to 3206da2 Compare October 16, 2018 06:01

vkuznecovas previously approved these changes Oct 16, 2018

View reviewed changes

soffokl previously approved these changes Oct 16, 2018

View reviewed changes

soffokl reviewed Oct 16, 2018

View reviewed changes

Waldz reviewed Oct 16, 2018

View reviewed changes

Compile in geoip2 database and avoid external file reading

537a4e3

tadovas dismissed stale reviews from soffokl and vkuznecovas via 537a4e3 October 16, 2018 10:55

tadovas force-pushed the improvement/build-in-geoip-db branch from 3206da2 to 537a4e3 Compare October 16, 2018 10:55

Waldz approved these changes Oct 16, 2018

View reviewed changes

vkuznecovas approved these changes Oct 16, 2018

View reviewed changes

tadovas merged commit c61e995 into master Oct 16, 2018

tadovas deleted the improvement/build-in-geoip-db branch October 16, 2018 13:07

Waldz mentioned this pull request Oct 24, 2018

Initial mobile entrypoint for iOS/Swift #447

Merged

13 tasks


		// NewBuiltInResolver returns new BuiltInDbResolver
		func NewBuiltInResolver() Resolver {


		// ResolveCountry maps given ip to country
		func (r *BuiltInDbResolver) ResolveCountry(ip string) (string, error) {

Compile in geoip2 database and avoid external file reading #281

Compile in geoip2 database and avoid external file reading #281

Conversation

tadovas commented Jul 4, 2018 • edited Loading

Waldz commented Jul 5, 2018

Waldz commented Jul 5, 2018

tadovas commented Jul 13, 2018

donce left a comment

Choose a reason for hiding this comment

zolia left a comment

Choose a reason for hiding this comment

stale bot commented Oct 12, 2018

tadovas commented Oct 15, 2018

tadovas commented Oct 15, 2018

tadovas commented Oct 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadovas commented Oct 15, 2018

soffokl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadovas commented Oct 16, 2018

soffokl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadovas commented Oct 16, 2018

tadovas commented Jul 4, 2018 •

edited

Loading