diff --git a/internal/netxlite/dialer.go b/internal/netxlite/dialer.go index be8796c..2b40679 100644 --- a/internal/netxlite/dialer.go +++ b/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. diff --git a/internal/netxlite/legacy.go b/internal/netxlite/legacy.go index 9b30829..d2ddb98 100644 --- a/internal/netxlite/legacy.go +++ b/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 diff --git a/internal/netxlite/legacy_test.go b/internal/netxlite/legacy_test.go index 9cc54e6..5963f18 100644 --- a/internal/netxlite/legacy_test.go +++ b/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{ diff --git a/internal/netxlite/quic.go b/internal/netxlite/quic.go index cc5042e..ccb3785 100644 --- a/internal/netxlite/quic.go +++ b/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. diff --git a/internal/netxlite/quirks.go b/internal/netxlite/quirks.go new file mode 100644 index 0000000..dc3af6b --- /dev/null +++ b/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 . +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 +} diff --git a/internal/netxlite/quirks_test.go b/internal/netxlite/quirks_test.go new file mode 100644 index 0000000..2488332 --- /dev/null +++ b/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) + } +}