feat(enginenetx): test bridgesPolicy with DNS success (#1554)

This diff introduces a test case for `bridgesPolicy` where we count
after how many policies we observe a DNS-generated policy. This test has
been crucial to investigate ooni/probe#2704.
Based on this test we can conclude the following:

1. if the bridge IP address gets blocked or stops working, we're still
falling back to using the DNS;
2. however, the current algorithm does that in a too-slow fashion.

Additionally, I manually verified that we're actually falling back to
the DNS and that it really takes a long time by changing the
implementation to use `10.0.0.1` as the bridge address and verifying
that the code behaves as expected (though the "expected" behavior here
is not nice at all and we should improve upon that).

While there, fix naming and comments.

internal/enginenetx/bridgespolicy_test.go

@@ -9,7 +9,7 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
 
-func TestBeaconsPolicy(t *testing.T) {
+func TestBridgesPolicy(t *testing.T) {
 	t.Run("for domains for which we don't have bridges and DNS failure", func(t *testing.T) {
 		expected := errors.New("mocked error")
 		p := &bridgesPolicy{
@@ -76,7 +76,7 @@ func TestBeaconsPolicy(t *testing.T) {
 		}
 	})
 
-	t.Run("for the api.ooni.io domain", func(t *testing.T) {
+	t.Run("for the api.ooni.io domain with DNS failure", func(t *testing.T) {
 		expected := errors.New("mocked error")
 		p := &bridgesPolicy{
 			Fallback: &dnsPolicy{
@@ -92,6 +92,7 @@ func TestBeaconsPolicy(t *testing.T) {
 		ctx := context.Background()
 		tactics := p.LookupTactics(ctx, "api.ooni.io", "443")
 
+		// since the DNS fails, we should only see tactics generated by bridges
 		var count int
 		for tactic := range tactics {
 			count++
@@ -117,6 +118,77 @@ func TestBeaconsPolicy(t *testing.T) {
 		}
 	})
 
+	t.Run("for the api.ooni.io domain with DNS success", func(t *testing.T) {
+		p := &bridgesPolicy{
+			Fallback: &dnsPolicy{
+				Logger: model.DiscardLogger,
+				Resolver: &mocks.Resolver{
+					MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
+						return []string{"130.192.91.211"}, nil
+					},
+				},
+			},
+		}
+
+		ctx := context.Background()
+		tactics := p.LookupTactics(ctx, "api.ooni.io", "443")
+
+		// since the DNS succeeds we should see bridge tactics mixed with DNS tactics
+		var (
+			bridgesCount int
+			dnsCount     int
+			overallCount int
+		)
+		const expectedDNSEntryCount = 153 // yikes!
+		for tactic := range tactics {
+			overallCount++
+
+			t.Log(overallCount, tactic)
+
+			if tactic.Port != "443" {
+				t.Fatal("the port should always be 443")
+			}
+
+			switch {
+			case overallCount == expectedDNSEntryCount:
+				if tactic.Address != "130.192.91.211" {
+					t.Fatal("the host should be 130.192.91.211 for count ==", expectedDNSEntryCount)
+				}
+
+				if tactic.SNI != "api.ooni.io" {
+					t.Fatal("we should see the `api.ooni.io` SNI on the wire for count ==", expectedDNSEntryCount)
+				}
+
+				dnsCount++
+
+			default:
+				if tactic.Address != "162.55.247.208" {
+					t.Fatal("the host should be 162.55.247.208 for count !=", expectedDNSEntryCount)
+				}
+
+				if tactic.SNI == "api.ooni.io" {
+					t.Fatal("we should not see the `api.ooni.io` SNI on the wire for count !=", expectedDNSEntryCount)
+				}
+
+				bridgesCount++
+			}
+
+			if tactic.VerifyHostname != "api.ooni.io" {
+				t.Fatal("the VerifyHostname field should always be like `api.ooni.io`")
+			}
+		}
+
+		if overallCount <= 0 {
+			t.Fatal("expected to see at least one tactic")
+		}
+		if dnsCount != 1 {
+			t.Fatal("expected to see exactly one DNS based tactic")
+		}
+		if bridgesCount <= 0 {
+			t.Fatal("expected to see at least one bridge tactic")
+		}
+	})
+
 	t.Run("for test helper domains", func(t *testing.T) {
 		for _, domain := range bridgesPolicyTestHelpersDomains {
 			t.Run(domain, func(t *testing.T) {

internal/enginenetx/httpsdialer.go

@@ -97,7 +97,7 @@ type httpsDialerPolicy interface {
 
 // httpsDialerEventsHandler handles events occurring while we try dialing TLS.
 type httpsDialerEventsHandler interface {
-	// These callbacks are invoked during the TLS handshake to inform this
+	// These callbacks are invoked during the TLS dialing to inform this
 	// interface about events that occurred. A policy SHOULD keep track of which
 	// addresses, SNIs, etc. work and return them more frequently.
 	//
@@ -236,8 +236,10 @@ func (hd *httpsDialer) DialTLSContext(ctx context.Context, network string, endpo
 				continue
 			}
 
-			// Save the conn and tell goroutines to stop ASAP
+			// Save the conn
 			connv = append(connv, result.Conn)
+
+			// Interrupt other concurrent dialing attempts
 			cancel()
 		}
 	}

internal/enginenetx/network.go

@@ -93,7 +93,8 @@ func NewNetwork(
 	netx := &netxlite.Netx{}
 	dialer := netx.NewDialerWithResolver(logger, resolver)
 
-	// Create manager for keeping track of statistics
+	// Create manager for keeping track of statistics. This implies creating a background
+	// goroutine that we'll need to close when we're done.
 	const trimInterval = 30 * time.Second
 	stats := newStatsManager(kvStore, logger, trimInterval)
 
@@ -118,15 +119,8 @@ func NewNetwork(
 	// the proxy, otherwise it means that we're using the ooni/oohttp library
 	// to dial for proxies, which has some restrictions.
 	//
-	// In particular, the returned transport uses dialer for dialing with
-	// cleartext proxies (e.g., socks5 and http) and httpsDialer for dialing
-	// with encrypted proxies (e.g., https). After this has happened,
-	// the code currently falls back to using the standard library's tls
-	// client code for establishing TLS connections over the proxy. The main
-	// implication here is that we're not using our custom mozilla CA for
-	// validating TLS certificates, rather we're using the system's cert store.
-	//
-	// Fixing this issue is TODO(https://github.com/ooni/probe/issues/2536).
+	// - this code does not work as intended when using netem and proxies
+	// as documented by TODO(https://github.com/ooni/probe/issues/2536).
 	txp := netxlite.NewHTTPTransportWithOptions(
 		logger, dialer, httpsDialer,
 		netxlite.HTTPTransportOptionDisableCompression(false),

internal/enginenetx/statsmanager.go

@@ -137,6 +137,8 @@ func statsDefensivelySortTacticsByDescendingSuccessRateWithAcceptPredicate(
 	input []*statsTactic, acceptfunc func(*statsTactic) bool) []*statsTactic {
 	// first let's create a working list such that we don't modify
 	// the input in place thus avoiding any data race
+	//
+	// make sure we explicitly filter out malformed entries
 	work := []*statsTactic{}
 	for _, t := range input {
 		if t != nil && t.Tactic != nil {
@@ -193,8 +195,8 @@ func (st *statsTactic) Clone() *statsTactic {
 	// a pointer to a location which is typically immutable, so it's perfectly
 	// fine to copy the LastUpdate field by assignment.
 	//
-	// here we're using a bunch of robustness aware mechanisms to clone
-	// considering that the struct may be edited by the user
+	// here we're using safe functions to clone the original struct considering
+	// that a user can edit the content on disk freely introducing nulls.
 	return &statsTactic{
 		CountStarted:               st.CountStarted,
 		CountTCPConnectError:       st.CountTCPConnectError,

internal/enginenetx/statspolicy.go

@@ -61,7 +61,7 @@ func (p *statsPolicy) LookupTactics(ctx context.Context, domain string, port str
 		}
 
 		// give priority to what we know from stats
-		for _, t := range statsPolicyPostProcessTactics(p.Stats.LookupTactics(domain, port)) {
+		for _, t := range statsPolicyFilterStatsTactics(p.Stats.LookupTactics(domain, port)) {
 			maybeEmitTactic(t)
 		}
 
@@ -74,7 +74,7 @@ func (p *statsPolicy) LookupTactics(ctx context.Context, domain string, port str
 	return out
 }
 
-func statsPolicyPostProcessTactics(tactics []*statsTactic, good bool) (out []*httpsDialerTactic) {
+func statsPolicyFilterStatsTactics(tactics []*statsTactic, good bool) (out []*httpsDialerTactic) {
 	// when good is false, it means p.Stats.LookupTactics failed
 	if !good {
 		return

internal/enginenetx/statspolicy_test.go

@@ -319,9 +319,9 @@ func (p *mocksPolicy) LookupTactics(ctx context.Context, domain string, port str
 	return p.MockLookupTactics(ctx, domain, port)
 }
 
-func TestStatsPolicyPostProcessTactics(t *testing.T) {
+func TestStatsPolicyFilterStatsTactics(t *testing.T) {
 	t.Run("we do nothing when good is false", func(t *testing.T) {
-		tactics := statsPolicyPostProcessTactics(nil, false)
+		tactics := statsPolicyFilterStatsTactics(nil, false)
 		if len(tactics) != 0 {
 			t.Fatal("expected zero-lenght return value")
 		}
@@ -390,7 +390,7 @@ func TestStatsPolicyPostProcessTactics(t *testing.T) {
 			},
 		}
 
-		got := statsPolicyPostProcessTactics(input, true)
+		got := statsPolicyFilterStatsTactics(input, true)
 
 		if len(got) != 1 {
 			t.Fatal("expected just one element")

internal/enginenetx/userpolicy.go

@@ -104,7 +104,7 @@ func (ldp *userPolicy) LookupTactics(
 		return ldp.Fallback.LookupTactics(ctx, domain, port)
 	}
 
-	// emit the resuults, which may possibly be empty
+	// emit the results, which may possibly be empty
 	out := make(chan *httpsDialerTactic)
 	go func() {
 		defer close(out) // let the caller know we're done