diff --git a/internal/engine/geolocate/geolocate.go b/internal/engine/geolocate/geolocate.go index ec75df6..443eaed 100644 --- a/internal/engine/geolocate/geolocate.go +++ b/internal/engine/geolocate/geolocate.go @@ -60,8 +60,8 @@ type Results struct { // CountryCode is the country code CountryCode string - // DidResolverLookup indicates whether we did a resolver lookup. - DidResolverLookup bool + // didResolverLookup indicates whether we did a resolver lookup. + didResolverLookup bool // NetworkName is the network name NetworkName string @@ -109,10 +109,6 @@ type Resolver interface { // Config contains configuration for a geolocate Task. type Config struct { - // EnableResolverLookup indicates whether we want to - // perform the optional resolver lookup. - EnableResolverLookup bool - // Resolver is the resolver we should use when // making requests for discovering the IP. When // this field is not set, we use the stdlib. @@ -147,12 +143,7 @@ func NewTask(config Config) (*Task, error) { } return &Task{ countryLookupper: mmdbLookupper{}, - enableResolverLookup: config.EnableResolverLookup, - probeIPLookupper: ipLookupClient{ - Resolver: config.Resolver, - Logger: config.Logger, - UserAgent: config.UserAgent, - }, + probeIPLookupper: ipLookupClient(config), probeASNLookupper: mmdbLookupper{}, resolverASNLookupper: mmdbLookupper{}, resolverIPLookupper: resolverLookupClient{}, @@ -163,7 +154,6 @@ func NewTask(config Config) (*Task, error) { // instance of Task using the NewTask factory. type Task struct { countryLookupper countryLookupper - enableResolverLookup bool probeIPLookupper probeIPLookupper probeASNLookupper asnLookupper resolverASNLookupper asnLookupper @@ -198,25 +188,23 @@ func (op Task) Run(ctx context.Context) (*Results, error) { return out, fmt.Errorf("lookupProbeCC failed: %w", err) } out.CountryCode = cc - if op.enableResolverLookup { - out.DidResolverLookup = true - // Note: ignoring the result of lookupResolverIP and lookupASN - // here is intentional. We don't want this (~minor) failure - // to influence the result of the overall lookup. Another design - // here could be that of retrying the operation N times? - resolverIP, err := op.resolverIPLookupper.LookupResolverIP(ctx) - if err != nil { - return out, nil - } - out.ResolverIP = resolverIP - resolverASN, resolverNetworkName, err := op.resolverASNLookupper.LookupASN( - out.ResolverIP, - ) - if err != nil { - return out, nil - } - out.ResolverASN = resolverASN - out.ResolverNetworkName = resolverNetworkName + out.didResolverLookup = true + // Note: ignoring the result of lookupResolverIP and lookupASN + // here is intentional. We don't want this (~minor) failure + // to influence the result of the overall lookup. Another design + // here could be that of retrying the operation N times? + resolverIP, err := op.resolverIPLookupper.LookupResolverIP(ctx) + if err != nil { + return out, nil // intentional } + out.ResolverIP = resolverIP + resolverASN, resolverNetworkName, err := op.resolverASNLookupper.LookupASN( + out.ResolverIP, + ) + if err != nil { + return out, nil // intentional + } + out.ResolverASN = resolverASN + out.ResolverNetworkName = resolverNetworkName return out, nil } diff --git a/internal/engine/geolocate/geolocate_test.go b/internal/engine/geolocate/geolocate_test.go index b7d8fbf..0391389 100644 --- a/internal/engine/geolocate/geolocate_test.go +++ b/internal/engine/geolocate/geolocate_test.go @@ -148,11 +148,10 @@ func (c taskResolverIPLookupper) LookupResolverIP(ctx context.Context) (string, func TestLocationLookupCannotLookupResolverIP(t *testing.T) { expected := errors.New("mocked error") op := Task{ - probeIPLookupper: taskProbeIPLookupper{ip: "1.2.3.4"}, - probeASNLookupper: taskASNLookupper{asn: 1234, name: "1234.com"}, - countryLookupper: taskCCLookupper{cc: "IT"}, - resolverIPLookupper: taskResolverIPLookupper{err: expected}, - enableResolverLookup: true, + probeIPLookupper: taskProbeIPLookupper{ip: "1.2.3.4"}, + probeASNLookupper: taskASNLookupper{asn: 1234, name: "1234.com"}, + countryLookupper: taskCCLookupper{cc: "IT"}, + resolverIPLookupper: taskResolverIPLookupper{err: expected}, } ctx := context.Background() out, err := op.Run(ctx) @@ -171,7 +170,7 @@ func TestLocationLookupCannotLookupResolverIP(t *testing.T) { if out.ProbeIP != "1.2.3.4" { t.Fatal("invalid ProbeIP value") } - if out.DidResolverLookup != true { + if out.didResolverLookup != true { t.Fatal("invalid DidResolverLookup value") } if out.ResolverASN != DefaultResolverASN { @@ -193,7 +192,6 @@ func TestLocationLookupCannotLookupResolverNetworkName(t *testing.T) { countryLookupper: taskCCLookupper{cc: "IT"}, resolverIPLookupper: taskResolverIPLookupper{ip: "4.3.2.1"}, resolverASNLookupper: taskASNLookupper{err: expected}, - enableResolverLookup: true, } ctx := context.Background() out, err := op.Run(ctx) @@ -212,7 +210,7 @@ func TestLocationLookupCannotLookupResolverNetworkName(t *testing.T) { if out.ProbeIP != "1.2.3.4" { t.Fatal("invalid ProbeIP value") } - if out.DidResolverLookup != true { + if out.didResolverLookup != true { t.Fatal("invalid DidResolverLookup value") } if out.ResolverASN != DefaultResolverASN { @@ -233,7 +231,6 @@ func TestLocationLookupSuccessWithResolverLookup(t *testing.T) { countryLookupper: taskCCLookupper{cc: "IT"}, resolverIPLookupper: taskResolverIPLookupper{ip: "4.3.2.1"}, resolverASNLookupper: taskASNLookupper{asn: 4321, name: "4321.com"}, - enableResolverLookup: true, } ctx := context.Background() out, err := op.Run(ctx) @@ -252,7 +249,7 @@ func TestLocationLookupSuccessWithResolverLookup(t *testing.T) { if out.ProbeIP != "1.2.3.4" { t.Fatal("invalid ProbeIP value") } - if out.DidResolverLookup != true { + if out.didResolverLookup != true { t.Fatal("invalid DidResolverLookup value") } if out.ResolverASN != 4321 { @@ -266,49 +263,8 @@ func TestLocationLookupSuccessWithResolverLookup(t *testing.T) { } } -func TestLocationLookupSuccessWithoutResolverLookup(t *testing.T) { - op := Task{ - probeIPLookupper: taskProbeIPLookupper{ip: "1.2.3.4"}, - probeASNLookupper: taskASNLookupper{asn: 1234, name: "1234.com"}, - countryLookupper: taskCCLookupper{cc: "IT"}, - resolverIPLookupper: taskResolverIPLookupper{ip: "4.3.2.1"}, - resolverASNLookupper: taskASNLookupper{asn: 4321, name: "4321.com"}, - } - ctx := context.Background() - out, err := op.Run(ctx) - if err != nil { - t.Fatalf("not the error we expected: %+v", err) - } - if out.ASN != 1234 { - t.Fatal("invalid ASN value") - } - if out.CountryCode != "IT" { - t.Fatal("invalid CountryCode value") - } - if out.NetworkName != "1234.com" { - t.Fatal("invalid NetworkName value") - } - if out.ProbeIP != "1.2.3.4" { - t.Fatal("invalid ProbeIP value") - } - if out.DidResolverLookup != false { - t.Fatal("invalid DidResolverLookup value") - } - if out.ResolverASN != DefaultResolverASN { - t.Fatalf("invalid ResolverASN value: %+v", out.ResolverASN) - } - if out.ResolverIP != DefaultResolverIP { - t.Fatalf("invalid ResolverIP value: %+v", out.ResolverIP) - } - if out.ResolverNetworkName != DefaultResolverNetworkName { - t.Fatal("invalid ResolverNetworkName value") - } -} - func TestSmoke(t *testing.T) { - config := Config{ - EnableResolverLookup: true, - } + config := Config{} task := Must(NewTask(config)) result, err := task.Run(context.Background()) if err != nil { diff --git a/internal/engine/session.go b/internal/engine/session.go index a0adfea..079d1f6 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -644,13 +644,10 @@ func (s *Session) MaybeLookupBackendsContext(ctx context.Context) error { // LookupLocationContext performs a location lookup. If you want memoisation // of the results, you should use MaybeLookupLocationContext. func (s *Session) LookupLocationContext(ctx context.Context) (*geolocate.Results, error) { - // Implementation note: we don't perform the lookup of the resolver IP - // when we are using a proxy because that might leak information. task := geolocate.Must(geolocate.NewTask(geolocate.Config{ - EnableResolverLookup: s.proxyURL == nil, - Logger: s.Logger(), - Resolver: s.resolver, - UserAgent: s.UserAgent(), + Logger: s.Logger(), + Resolver: s.resolver, + UserAgent: s.UserAgent(), })) return task.Run(ctx) }