Skip to content

Commit

Permalink
fix(geolocate): always use netxlite functionality
Browse files Browse the repository at this point in the history
This change ensures that, in turn, we're able to remote all the
traffic generated by geolocate, rather than missing some bits of
it that were still using the standard library.

Extracted from #969.

Closes ooni/probe#1383.

Part of ooni/probe#2340.
  • Loading branch information
bassosimone committed Oct 12, 2022
1 parent 86ffd6a commit 1b864ab
Show file tree
Hide file tree
Showing 11 changed files with 93 additions and 32 deletions.
1 change: 1 addition & 0 deletions internal/engine/geolocate/cloudflare.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ func cloudflareIPLookup(
httpClient *http.Client,
logger model.Logger,
userAgent string,
resolver model.Resolver,
) (string, error) {
data, err := (&httpx.APIClientTemplate{
BaseURL: "https://www.cloudflare.com",
Expand Down
2 changes: 2 additions & 0 deletions internal/engine/geolocate/cloudflare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/apex/log"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/netxlite"
)

func TestIPLookupWorksUsingcloudlflare(t *testing.T) {
Expand All @@ -16,6 +17,7 @@ func TestIPLookupWorksUsingcloudlflare(t *testing.T) {
http.DefaultClient,
log.Log,
model.HTTPHeaderUserAgent,
netxlite.NewStdlibResolver(model.DiscardLogger),
)
if err != nil {
t.Fatal(err)
Expand Down
4 changes: 3 additions & 1 deletion internal/engine/geolocate/geolocate.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,9 @@ func NewTask(config Config) *Task {
probeIPLookupper: ipLookupClient(config),
probeASNLookupper: mmdbLookupper{},
resolverASNLookupper: mmdbLookupper{},
resolverIPLookupper: resolverLookupClient{},
resolverIPLookupper: resolverLookupClient{
Resolver: config.Resolver,
},
}
}

Expand Down
1 change: 1 addition & 0 deletions internal/engine/geolocate/invalid_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ func invalidIPLookup(
httpClient *http.Client,
logger model.Logger,
userAgent string,
resolver model.Resolver,
) (string, error) {
return "invalid IP", nil
}
3 changes: 2 additions & 1 deletion internal/engine/geolocate/iplookup.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ var (
type lookupFunc func(
ctx context.Context, client *http.Client,
logger model.Logger, userAgent string,
resolver model.Resolver,
) (string, error)

type method struct {
Expand Down Expand Up @@ -89,7 +90,7 @@ func (c ipLookupClient) doWithCustomFunc(
txp := netxlite.NewHTTPTransportWithResolver(c.Logger, c.Resolver)
clnt := &http.Client{Transport: txp}
defer clnt.CloseIdleConnections()
ip, err := fn(ctx, clnt, c.Logger, c.UserAgent)
ip, err := fn(ctx, clnt, c.Logger, c.UserAgent, c.Resolver)
if err != nil {
return model.DefaultProbeIP, err
}
Expand Down
9 changes: 6 additions & 3 deletions internal/engine/geolocate/resolverlookup.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ package geolocate
import (
"context"
"errors"
"net"

"github.com/ooni/probe-cli/v3/internal/model"
)

var (
Expand All @@ -16,7 +17,9 @@ type dnsResolver interface {
LookupHost(ctx context.Context, host string) (addrs []string, err error)
}

type resolverLookupClient struct{}
type resolverLookupClient struct {
Resolver model.Resolver
}

func (rlc resolverLookupClient) do(ctx context.Context, r dnsResolver) (string, error) {
var ips []string
Expand All @@ -31,5 +34,5 @@ func (rlc resolverLookupClient) do(ctx context.Context, r dnsResolver) (string,
}

func (rlc resolverLookupClient) LookupResolverIP(ctx context.Context) (ip string, err error) {
return rlc.do(ctx, &net.Resolver{})
return rlc.do(ctx, rlc.Resolver)
}
16 changes: 13 additions & 3 deletions internal/engine/geolocate/resolverlookup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,16 @@ import (
"context"
"errors"
"testing"

"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/netxlite"
)

func TestLookupResolverIP(t *testing.T) {
addr, err := (resolverLookupClient{}).LookupResolverIP(context.Background())
rlc := resolverLookupClient{
Resolver: netxlite.NewStdlibResolver(model.DiscardLogger),
}
addr, err := rlc.LookupResolverIP(context.Background())
if err != nil {
t.Fatal(err)
}
Expand All @@ -26,7 +32,9 @@ func (bhl brokenHostLookupper) LookupHost(ctx context.Context, host string) ([]s

func TestLookupResolverIPFailure(t *testing.T) {
expected := errors.New("mocked error")
rlc := resolverLookupClient{}
rlc := resolverLookupClient{
Resolver: netxlite.NewStdlibResolver(model.DiscardLogger),
}
addr, err := rlc.do(context.Background(), brokenHostLookupper{
err: expected,
})
Expand All @@ -39,7 +47,9 @@ func TestLookupResolverIPFailure(t *testing.T) {
}

func TestLookupResolverIPNoAddressReturned(t *testing.T) {
rlc := resolverLookupClient{}
rlc := resolverLookupClient{
Resolver: netxlite.NewStdlibResolver(model.DiscardLogger),
}
addr, err := rlc.do(context.Background(), brokenHostLookupper{})
if !errors.Is(err, ErrNoIPAddressReturned) {
t.Fatalf("not the error we expected: %+v", err)
Expand Down
40 changes: 26 additions & 14 deletions internal/engine/geolocate/stun.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,41 +2,49 @@ package geolocate

import (
"context"
"net"
"net/http"

"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/netxlite"
"github.com/pion/stun"
)

// TODO(bassosimone): we should modify the stun code to use
// the session resolver rather than using its own.
//
// See https://github.com/ooni/probe/issues/1383.

type stunClient interface {
Close() error
Start(m *stun.Message, h stun.Handler) error
}

type stunConfig struct {
Dial func(network string, address string) (stunClient, error)
Endpoint string
Logger model.Logger
Dialer model.Dialer // optional
Endpoint string
Logger model.Logger
NewClient func(conn net.Conn) (stunClient, error) // optional
Resolver model.Resolver
}

func stunDialer(network string, address string) (stunClient, error) {
return stun.Dial(network, address)
func stunNewClient(conn net.Conn) (stunClient, error) {
return stun.NewClient(conn)
}

func stunIPLookup(ctx context.Context, config stunConfig) (string, error) {
config.Logger.Debugf("STUNIPLookup: start using %s", config.Endpoint)
ip, err := func() (string, error) {
dial := config.Dial
if dial == nil {
dial = stunDialer
dialer := config.Dialer
if dialer == nil {
dialer = netxlite.NewDialerWithResolver(config.Logger, config.Resolver)
}
conn, err := dialer.DialContext(ctx, "udp", config.Endpoint)
if err != nil {
return model.DefaultProbeIP, err
}
newClient := config.NewClient
if newClient == nil {
newClient = stunNewClient
}
clnt, err := dial("udp", config.Endpoint)
clnt, err := newClient(conn)
if err != nil {
conn.Close()
return model.DefaultProbeIP, err
}
defer clnt.Close()
Expand Down Expand Up @@ -78,10 +86,12 @@ func stunEkigaIPLookup(
httpClient *http.Client,
logger model.Logger,
userAgent string,
resolver model.Resolver,
) (string, error) {
return stunIPLookup(ctx, stunConfig{
Endpoint: "stun.ekiga.net:3478",
Logger: logger,
Resolver: resolver,
})
}

Expand All @@ -90,9 +100,11 @@ func stunGoogleIPLookup(
httpClient *http.Client,
logger model.Logger,
userAgent string,
resolver model.Resolver,
) (string, error) {
return stunIPLookup(ctx, stunConfig{
Endpoint: "stun.l.google.com:19302",
Logger: logger,
Resolver: resolver,
})
}
45 changes: 35 additions & 10 deletions internal/engine/geolocate/stun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import (

"github.com/apex/log"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/model/mocks"
"github.com/ooni/probe-cli/v3/internal/netxlite"
"github.com/pion/stun"
)

Expand All @@ -19,6 +21,7 @@ func TestSTUNIPLookupCanceledContext(t *testing.T) {
ip, err := stunIPLookup(ctx, stunConfig{
Endpoint: "stun.ekiga.net:3478",
Logger: log.Log,
Resolver: netxlite.NewStdlibResolver(model.DiscardLogger),
})
if !errors.Is(err, context.Canceled) {
t.Fatalf("not the error we expected: %+v", err)
Expand All @@ -32,8 +35,10 @@ func TestSTUNIPLookupDialFailure(t *testing.T) {
expected := errors.New("mocked error")
ctx := context.Background()
ip, err := stunIPLookup(ctx, stunConfig{
Dial: func(network, address string) (stunClient, error) {
return nil, expected
Dialer: &mocks.Dialer{
MockDialContext: func(ctx context.Context, network, address string) (net.Conn, error) {
return nil, expected
},
},
Endpoint: "stun.ekiga.net:3478",
Logger: log.Log,
Expand Down Expand Up @@ -70,11 +75,17 @@ func TestSTUNIPLookupStartReturnsError(t *testing.T) {
expected := errors.New("mocked error")
ctx := context.Background()
ip, err := stunIPLookup(ctx, stunConfig{
Dial: func(network, address string) (stunClient, error) {
return MockableSTUNClient{StartErr: expected}, nil
Dialer: &mocks.Dialer{
MockDialContext: func(ctx context.Context, network, address string) (net.Conn, error) {
conn := &mocks.Conn{}
return conn, nil
},
},
Endpoint: "stun.ekiga.net:3478",
Logger: log.Log,
NewClient: func(conn net.Conn) (stunClient, error) {
return MockableSTUNClient{StartErr: expected}, nil
},
})
if !errors.Is(err, expected) {
t.Fatalf("not the error we expected: %+v", err)
Expand All @@ -88,13 +99,19 @@ func TestSTUNIPLookupStunEventContainsError(t *testing.T) {
expected := errors.New("mocked error")
ctx := context.Background()
ip, err := stunIPLookup(ctx, stunConfig{
Dial: func(network, address string) (stunClient, error) {
Dialer: &mocks.Dialer{
MockDialContext: func(ctx context.Context, network, address string) (net.Conn, error) {
conn := &mocks.Conn{}
return conn, nil
},
},
Endpoint: "stun.ekiga.net:3478",
Logger: log.Log,
NewClient: func(conn net.Conn) (stunClient, error) {
return MockableSTUNClient{Event: stun.Event{
Error: expected,
}}, nil
},
Endpoint: "stun.ekiga.net:3478",
Logger: log.Log,
})
if !errors.Is(err, expected) {
t.Fatalf("not the error we expected: %+v", err)
Expand All @@ -107,13 +124,19 @@ func TestSTUNIPLookupStunEventContainsError(t *testing.T) {
func TestSTUNIPLookupCannotDecodeMessage(t *testing.T) {
ctx := context.Background()
ip, err := stunIPLookup(ctx, stunConfig{
Dial: func(network, address string) (stunClient, error) {
Dialer: &mocks.Dialer{
MockDialContext: func(ctx context.Context, network, address string) (net.Conn, error) {
conn := &mocks.Conn{}
return conn, nil
},
},
Endpoint: "stun.ekiga.net:3478",
Logger: log.Log,
NewClient: func(conn net.Conn) (stunClient, error) {
return MockableSTUNClient{Event: stun.Event{
Message: &stun.Message{},
}}, nil
},
Endpoint: "stun.ekiga.net:3478",
Logger: log.Log,
})
if !errors.Is(err, stun.ErrAttributeNotFound) {
t.Fatalf("not the error we expected: %+v", err)
Expand All @@ -129,6 +152,7 @@ func TestIPLookupWorksUsingSTUNEkiga(t *testing.T) {
http.DefaultClient,
log.Log,
model.HTTPHeaderUserAgent,
netxlite.NewStdlibResolver(model.DiscardLogger),
)
if err != nil {
t.Fatal(err)
Expand All @@ -144,6 +168,7 @@ func TestIPLookupWorksUsingSTUNGoogle(t *testing.T) {
http.DefaultClient,
log.Log,
model.HTTPHeaderUserAgent,
netxlite.NewStdlibResolver(model.DiscardLogger),
)
if err != nil {
t.Fatal(err)
Expand Down
1 change: 1 addition & 0 deletions internal/engine/geolocate/ubuntu.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ func ubuntuIPLookup(
httpClient *http.Client,
logger model.Logger,
userAgent string,
resolver model.Resolver,
) (string, error) {
data, err := (&httpx.APIClientTemplate{
BaseURL: "https://geoip.ubuntu.com/",
Expand Down
3 changes: 3 additions & 0 deletions internal/engine/geolocate/ubuntu_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

"github.com/apex/log"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/netxlite"
)

func TestUbuntuParseError(t *testing.T) {
Expand All @@ -23,6 +24,7 @@ func TestUbuntuParseError(t *testing.T) {
}},
log.Log,
model.HTTPHeaderUserAgent,
netxlite.NewStdlibResolver(model.DiscardLogger),
)
if err == nil || !strings.HasPrefix(err.Error(), "XML syntax error") {
t.Fatalf("not the error we expected: %+v", err)
Expand All @@ -38,6 +40,7 @@ func TestIPLookupWorksUsingUbuntu(t *testing.T) {
http.DefaultClient,
log.Log,
model.HTTPHeaderUserAgent,
netxlite.NewStdlibResolver(model.DiscardLogger),
)
if err != nil {
t.Fatal(err)
Expand Down

0 comments on commit 1b864ab

Please sign in to comment.