fix(netxlite): clearly document quirk and make code robust (#468)

This quirk really saddens me. It's a piece of tech debt we're
carrying over from the original netx implementation.

We cannot remove it _until_ we have legacy netx code around.

The second best thing we can do is to clearly move this code in
a place where it's clear it's a quirk and write and use some extra
code that makes sure the quirk's assumptions are always met.

Sigh.

See https://github.com/ooni/probe/issues/1591

internal/netxlite/dialer.go

@@ -84,6 +84,9 @@ func (d *dialerResolver) DialContext(ctx context.Context, network, address strin
 	// than just calling ReduceErrors. We are not ready to do that
 	// yet, though. To do that, we need first to modify nettests so
 	// that we actually avoid dialing when measuring.
+	//
+	// See also the quirks.go file. This is clearly a QUIRK.
+	addrs = quirkSortIPAddrs(addrs)
 	var errorslist []error
 	for _, addr := range addrs {
 		target := net.JoinHostPort(addr, onlyport)
@@ -93,7 +96,7 @@ func (d *dialerResolver) DialContext(ctx context.Context, network, address strin
 		}
 		errorslist = append(errorslist, err)
 	}
-	return nil, reduceErrors(errorslist)
+	return nil, quirkReduceErrors(errorslist)
 }
 
 // lookupHost performs a domain name resolution.

internal/netxlite/legacy.go

@@ -2,44 +2,9 @@ package netxlite
 
 import (
 	"context"
-	"errors"
 	"net"
-	"strings"
-
-	"github.com/ooni/probe-cli/v3/internal/errorsx"
 )
 
-// reduceErrors finds a known error in a list of errors since
-// it's probably most relevant.
-//
-// Deprecation warning
-//
-// Albeit still used, this function is now DEPRECATED.
-//
-// In perspective, we would like to transition to a scenario where
-// full dialing is NOT used for measurements and we return a multierror here.
-func reduceErrors(errorslist []error) error {
-	if len(errorslist) == 0 {
-		return nil
-	}
-	// If we have a known error, let's consider this the real error
-	// since it's probably most relevant. Otherwise let's return the
-	// first considering that (1) local resolvers likely will give
-	// us IPv4 first and (2) also our resolver does that. So, in case
-	// the user has no IPv6 connectivity, an IPv6 error is going to
-	// appear later in the list of errors.
-	for _, err := range errorslist {
-		var wrapper *errorsx.ErrWrapper
-		if errors.As(err, &wrapper) && !strings.HasPrefix(
-			err.Error(), "unknown_failure",
-		) {
-			return err
-		}
-	}
-	// TODO(bassosimone): handle this case in a better way
-	return errorslist[0]
-}
-
 // These vars export internal names to legacy ooni/probe-cli code.
 var (
 	DefaultDialer        = defaultDialer

internal/netxlite/legacy_test.go

@@ -1,52 +1,12 @@
 package netxlite
 
 import (
-	"errors"
 	"net"
 	"testing"
 
-	"github.com/ooni/probe-cli/v3/internal/errorsx"
 	"github.com/ooni/probe-cli/v3/internal/netxlite/mocks"
 )
 
-func TestReduceErrors(t *testing.T) {
-	t.Run("no errors", func(t *testing.T) {
-		result := reduceErrors(nil)
-		if result != nil {
-			t.Fatal("wrong result")
-		}
-	})
-	t.Run("single error", func(t *testing.T) {
-		err := errors.New("mocked error")
-		result := reduceErrors([]error{err})
-		if result != err {
-			t.Fatal("wrong result")
-		}
-	})
-	t.Run("multiple errors", func(t *testing.T) {
-		err1 := errors.New("mocked error #1")
-		err2 := errors.New("mocked error #2")
-		result := reduceErrors([]error{err1, err2})
-		if result.Error() != "mocked error #1" {
-			t.Fatal("wrong result")
-		}
-	})
-	t.Run("multiple errors with meaningful ones", func(t *testing.T) {
-		err1 := errors.New("mocked error #1")
-		err2 := &errorsx.ErrWrapper{
-			Failure: "unknown_failure: antani",
-		}
-		err3 := &errorsx.ErrWrapper{
-			Failure: errorsx.FailureConnectionRefused,
-		}
-		err4 := errors.New("mocked error #3")
-		result := reduceErrors([]error{err1, err2, err3, err4})
-		if result.Error() != errorsx.FailureConnectionRefused {
-			t.Fatal("wrong result")
-		}
-	})
-}
-
 func TestResolverLegacyAdapterWithCompatibleType(t *testing.T) {
 	var called bool
 	r := NewResolverLegacyAdapter(&mocks.Resolver{

internal/netxlite/quic.go

@@ -169,6 +169,9 @@ func (d *quicDialerResolver) DialContext(
 	// than just calling ReduceErrors. We are not ready to do that
 	// yet, though. To do that, we need first to modify nettests so
 	// that we actually avoid dialing when measuring.
+	//
+	// See also the quirks.go file. This is clearly a QUIRK.
+	addrs = quirkSortIPAddrs(addrs)
 	var errorslist []error
 	for _, addr := range addrs {
 		target := net.JoinHostPort(addr, onlyport)
@@ -179,7 +182,7 @@ func (d *quicDialerResolver) DialContext(
 		}
 		errorslist = append(errorslist, err)
 	}
-	return nil, reduceErrors(errorslist)
+	return nil, quirkReduceErrors(errorslist)
 }
 
 // maybeApplyTLSDefaults sets the SNI if it's not already configured.

internal/netxlite/quirks.go

@@ -0,0 +1,73 @@
+package netxlite
+
+import (
+	"errors"
+	"strings"
+
+	"github.com/ooni/probe-cli/v3/internal/errorsx"
+)
+
+// This file contains weird stuff that we carried over from
+// the original netx implementation and that we cannot remove
+// or change without thinking about the consequences.
+
+// quirkReduceErrors finds a known error in a list of errors since
+// it's probably most relevant. If this error is not found, just
+// return the first error according to this reasoning:
+//
+// If we have a known error, let's consider this the real error
+// since it's probably most relevant. Otherwise let's return the
+// first considering that (1) local resolvers likely will give
+// us IPv4 first and (2) also our resolver does that. So, in case
+// the user has no IPv6 connectivity, an IPv6 error is going to
+// appear later in the list of errors.
+//
+// Honestly, the above reasoning does not feel very solid and
+// we also have an IMPLICIT assumption on our resolver returning
+// IPv4 before IPv6 _which is a really fragile one_. We try to
+// remediate with quirkSortIPAddrs (see below).
+//
+// This is CLEARLY a QUIRK anyway. There may code depending on how
+// we do things here and it's tricky to remove this behavior.
+func quirkReduceErrors(errorslist []error) error {
+	if len(errorslist) == 0 {
+		return nil
+	}
+	for _, err := range errorslist {
+		var wrapper *errorsx.ErrWrapper
+		if errors.As(err, &wrapper) && !strings.HasPrefix(
+			err.Error(), "unknown_failure",
+		) {
+			return err
+		}
+	}
+	return errorslist[0]
+}
+
+// quirkSortIPAddrs sorts IP addresses so that IPv4 appears
+// before IPv6. Dialers SHOULD call this code.
+//
+// It saddens me to have this quirk, but it is here to pair
+// with quirkReduceErrors, which assumes that <facepalm>.
+func quirkSortIPAddrs(addrs []string) (out []string) {
+	isIPv6 := func(x string) bool {
+		// This check for identifying IPv6 is discussed
+		// at https://stackoverflow.com/questions/22751035
+		// and seems good-enough for our purposes.
+		return strings.Contains(x, ":")
+	}
+	isIPv4 := func(x string) bool {
+		return !isIPv6(x)
+	}
+	for _, addr := range addrs {
+		if isIPv4(addr) {
+			out = append(out, addr)
+		}
+	}
+	for _, addr := range addrs {
+		if isIPv6(addr) {
+			out = append(out, addr)
+		}
+	}
+	return
+}

internal/netxlite/quirks_test.go

@@ -0,0 +1,72 @@
+package netxlite
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/ooni/probe-cli/v3/internal/errorsx"
+)
+
+func TestQuirkReduceErrors(t *testing.T) {
+	t.Run("no errors", func(t *testing.T) {
+		result := quirkReduceErrors(nil)
+		if result != nil {
+			t.Fatal("wrong result")
+		}
+	})
+	t.Run("single error", func(t *testing.T) {
+		err := errors.New("mocked error")
+		result := quirkReduceErrors([]error{err})
+		if result != err {
+			t.Fatal("wrong result")
+		}
+	})
+	t.Run("multiple errors", func(t *testing.T) {
+		err1 := errors.New("mocked error #1")
+		err2 := errors.New("mocked error #2")
+		result := quirkReduceErrors([]error{err1, err2})
+		if result.Error() != "mocked error #1" {
+			t.Fatal("wrong result")
+		}
+	})
+	t.Run("multiple errors with meaningful ones", func(t *testing.T) {
+		err1 := errors.New("mocked error #1")
+		err2 := &errorsx.ErrWrapper{
+			Failure: "unknown_failure: antani",
+		}
+		err3 := &errorsx.ErrWrapper{
+			Failure: errorsx.FailureConnectionRefused,
+		}
+		err4 := errors.New("mocked error #3")
+		result := quirkReduceErrors([]error{err1, err2, err3, err4})
+		if result.Error() != errorsx.FailureConnectionRefused {
+			t.Fatal("wrong result")
+		}
+	})
+}
+
+func TestQuirkSortIPAddrs(t *testing.T) {
+	addrs := []string{
+		"::1",
+		"192.168.1.2",
+		"2a00:1450:4002:404::2004",
+		"142.250.184.36",
+		"2604:8800:5000:82:466:38ff:fecb:d46e",
+		"198.145.29.83",
+		"95.216.163.36",
+	}
+	expected := []string{
+		"192.168.1.2",
+		"142.250.184.36",
+		"198.145.29.83",
+		"95.216.163.36",
+		"::1",
+		"2a00:1450:4002:404::2004",
+		"2604:8800:5000:82:466:38ff:fecb:d46e",
+	}
+	out := quirkSortIPAddrs(addrs)
+	if diff := cmp.Diff(expected, out); diff != "" {
+		t.Fatal(diff)
+	}
+}