Skip to content

Commit

Permalink
cleanup(sessionresolver): don't depend on netx
Browse files Browse the repository at this point in the history
This diff closes ooni/probe#2121 because it
removes the last unnecessary netx usage. All the other packages that
currently use netx are network experiments. We will eventually convert
them to not use netx as part of other tracked issues.

This diff closes ooni/probe#2135 because
now the sessionresolver does not depend on netx anymore.

We refactor the way in which we conditionally configure byte counting
for HTTP such that we use a free function rather than a method (we can
use methods with nil receiver in Go, but the free function seems to
be a better choice in this case).

We introduce and use a new bytecounter specifically for the system
resolver. This byte counter is very imprecise but seems still better
than using the system resolver doesn't use any network I/O.

We stop printing the sessionresolver stats when we close a session, since
this component has been in production for years now.

We improve the model.UnderlyingNetwork model to add support for changing
the root cert pool, which helps with writing some integration tests.

We modify the protocol to use and modify the netxlite tproxy (a singleton
instance of UnderlyingNetwork) to make it goroutine safe.

We introduce a new file inside sessionresolver, factory.go, which creates
and properly wraps the resolvers. This code replaces code for which we
previously used netx, and is the core change introduced here.

While there, we refactor how we log in the session resolver to use
the operation logger as we do in some experiments.

We write some additional tests that take advantage of the new netxlite
tproxy mocking functionality to ensure the sessionresolver continues
to work in two extreme use cases: no resolver is available and just the
system resolver is available. I introduced these new tests because I
originally broke the system resolver when I introduced factory.go and
I felt like it was useful to have more robustness here.
  • Loading branch information
bassosimone committed Feb 1, 2023
1 parent 9e7da33 commit 7c86d8f
Show file tree
Hide file tree
Showing 20 changed files with 1,101 additions and 43 deletions.
6 changes: 3 additions & 3 deletions internal/bytecounter/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ import (
// MaybeWrapHTTPTransport takes in input an HTTPTransport and either wraps it
// to perform byte counting, if this counter is not nil, or just returns to the
// caller the original transport, when the counter is nil.
func (c *Counter) MaybeWrapHTTPTransport(txp model.HTTPTransport) model.HTTPTransport {
if c != nil {
txp = WrapHTTPTransport(txp, c)
func MaybeWrapHTTPTransport(txp model.HTTPTransport, counter *Counter) model.HTTPTransport {
if counter != nil {
txp = WrapHTTPTransport(txp, counter)
}
return txp
}
Expand Down
4 changes: 2 additions & 2 deletions internal/bytecounter/http_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ func TestMaybeWrapHTTPTransport(t *testing.T) {
t.Run("when counter is not nil", func(t *testing.T) {
underlying := &mocks.HTTPTransport{}
counter := &Counter{}
txp := counter.MaybeWrapHTTPTransport(underlying)
txp := MaybeWrapHTTPTransport(underlying, counter)
realTxp := txp.(*httpTransport)
if realTxp.HTTPTransport != underlying {
t.Fatal("did not wrap correctly")
Expand All @@ -26,7 +26,7 @@ func TestMaybeWrapHTTPTransport(t *testing.T) {
t.Run("when counter is nil", func(t *testing.T) {
underlying := &mocks.HTTPTransport{}
var counter *Counter
txp := counter.MaybeWrapHTTPTransport(underlying)
txp := MaybeWrapHTTPTransport(underlying, counter)
if txp != underlying {
t.Fatal("unexpected result")
}
Expand Down
110 changes: 110 additions & 0 deletions internal/bytecounter/resolver.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
package bytecounter

import (
"context"
"net"

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

// MaybeWrapSystemResolver takes in input an Resolver and either wraps it
// to perform byte counting, if this counter is not nil, or just returns to the
// caller the original transport, when the counter is nil.
//
// # Bug
//
// The returned resolver will only approximately estimate the bytes
// sent and received by this resolver if this resolver is the system
// resolver. For more accurate counting when using DNS over HTTPS,
// you should instead count at the HTTP transport level. If you are
// using DNS over TCP, DNS over TLS, or DNS over UDP, you should instead
// count the bytes by just wrapping the connections you're using.
func MaybeWrapSystemResolver(reso model.Resolver, counter *Counter) model.Resolver {
if counter != nil {
reso = WrapSystemResolver(reso, counter)
}
return reso
}

// WrapSystemResolver creates a new byte-counting-aware resolver. This function
// returns a resolver with the same bugs of [MaybeWrapSystemResolver].
func WrapSystemResolver(reso model.Resolver, counter *Counter) model.Resolver {
return &resolver{
Resolver: reso,
Counter: counter,
}
}

// resolver is the type returned by WrapResolver.
type resolver struct {
Resolver model.Resolver
Counter *Counter
}

// Address implements model.Resolver
func (r *resolver) Address() string {
return r.Resolver.Address()
}

// CloseIdleConnections implements model.Resolver
func (r *resolver) CloseIdleConnections() {
r.Resolver.CloseIdleConnections()
}

// LookupHTTPS implements model.Resolver
func (r *resolver) LookupHTTPS(ctx context.Context, domain string) (*model.HTTPSSvc, error) {
r.updateCounterBytesSent(domain, 1)
out, err := r.Resolver.LookupHTTPS(ctx, domain)
r.updateCounterBytesRecv(err)
return out, err
}

// LookupHost implements model.Resolver
func (r *resolver) LookupHost(ctx context.Context, hostname string) (addrs []string, err error) {
r.updateCounterBytesSent(hostname, 2)
out, err := r.Resolver.LookupHost(ctx, hostname)
r.updateCounterBytesRecv(err)
return out, err
}

// LookupNS implements model.Resolver
func (r *resolver) LookupNS(ctx context.Context, domain string) ([]*net.NS, error) {
r.updateCounterBytesSent(domain, 1)
out, err := r.Resolver.LookupNS(ctx, domain)
r.updateCounterBytesRecv(err)
return out, err
}

// Network implements model.Resolver
func (r *resolver) Network() string {
return r.Resolver.Network()
}

// updateCounterBytesSent estimates the bytes sent.
func (r *resolver) updateCounterBytesSent(domain string, n int) {
// Assume we are sending N queries for the given domain, which is the
// correct byte counting strategy when using the system resolver
r.Counter.Sent.Add(int64(n * len(domain)))
}

// updateCounterBytesRecv estimates the bytes recv.
func (r *resolver) updateCounterBytesRecv(err error) {
if err != nil {
switch err.Error() {
case netxlite.FailureDNSNXDOMAINError,
netxlite.FailureDNSNoAnswer,
netxlite.FailureDNSRefusedError,
netxlite.FailureDNSNonRecoverableFailure,
netxlite.FailureDNSServfailError:
// In case it seems we received a message, let us
// pretent overall it was 128 bytes
r.Counter.Received.Add(128)
default:
// In this case we assume we did not receive any byte
}
return
}
// On success we assume we received 256 bytes
r.Counter.Received.Add(256)
}
Loading

0 comments on commit 7c86d8f

Please sign in to comment.