fix(oohelperd): reduce errors to what the old TH would emit (#543)

Reducing the errors is not done in a perfect way.

We have documented the most striking differences inside
https://github.com/ooni/probe/issues/1707#issuecomment-942283746 and
some attempts to improve the situation further inside
https://github.com/ooni/probe/issues/1707#issuecomment-942341255.

A better strategy for the future would be to introduce more
specific timeout errors, such as dns_timeout_error, etc.

More testing may be needed to further validate and compare the
old and the new TH, but this requires Jafar improvements to
more precisely simulate more complex censorship.

internal/cmd/oohelperd/internal/webconnectivity/dns.go

@@ -7,6 +7,7 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/engine/experiment/webconnectivity"
 	"github.com/ooni/probe-cli/v3/internal/engine/netx"
 	"github.com/ooni/probe-cli/v3/internal/engine/netx/archival"
+	"github.com/ooni/probe-cli/v3/internal/netxlite"
 )
 
 // newfailure is a convenience shortcut to save typing
@@ -31,5 +32,35 @@ func DNSDo(ctx context.Context, config *DNSConfig) {
 	if addrs == nil {
 		addrs = []string{} // fix: the old test helper did that
 	}
-	config.Out <- CtrlDNSResult{Failure: newfailure(err), Addrs: addrs}
+	failure := dnsMapFailure(newfailure(err))
+	config.Out <- CtrlDNSResult{Failure: failure, Addrs: addrs}
+}
+
+// dnsMapFailure attempts to map netxlite failures to the strings
+// used by the original OONI test helper.
+//
+// See https://github.com/ooni/backend/blob/6ec4fda5b18/oonib/testhelpers/http_helpers.py#L430
+func dnsMapFailure(failure *string) *string {
+	switch failure {
+	case nil:
+		return nil
+	default:
+		switch *failure {
+		case netxlite.FailureDNSNXDOMAINError:
+			// We have a name for this string because dnsanalysis.go is
+			// already checking for this specific error string.
+			s := webconnectivity.DNSNameError
+			return &s
+		case netxlite.FailureDNSNoAnswer,
+			netxlite.FailureDNSNonRecoverableFailure,
+			netxlite.FailureDNSRefusedError,
+			netxlite.FailureDNSServerMisbehaving,
+			netxlite.FailureDNSTemporaryFailure:
+			s := "dns_server_failure"
+			return &s
+		default:
+			s := "unknown_error"
+			return &s
+		}
+	}
 }

internal/cmd/oohelperd/internal/webconnectivity/dns_test.go

@@ -0,0 +1,87 @@
+package webconnectivity
+
+import (
+	"context"
+	"sync"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/ooni/probe-cli/v3/internal/engine/experiment/webconnectivity"
+	"github.com/ooni/probe-cli/v3/internal/netxlite"
+	"github.com/ooni/probe-cli/v3/internal/netxlite/mocks"
+)
+
+func stringPointerForString(s string) *string {
+	return &s
+}
+
+func Test_dnsMapFailure(t *testing.T) {
+	tests := []struct {
+		name    string
+		failure *string
+		want    *string
+	}{{
+		name:    "nil",
+		failure: nil,
+		want:    nil,
+	}, {
+		name:    "nxdomain",
+		failure: stringPointerForString(netxlite.FailureDNSNXDOMAINError),
+		want:    stringPointerForString(webconnectivity.DNSNameError),
+	}, {
+		name:    "no answer",
+		failure: stringPointerForString(netxlite.FailureDNSNoAnswer),
+		want:    stringPointerForString("dns_server_failure"),
+	}, {
+		name:    "non recoverable failure",
+		failure: stringPointerForString(netxlite.FailureDNSNonRecoverableFailure),
+		want:    stringPointerForString("dns_server_failure"),
+	}, {
+		name:    "refused",
+		failure: stringPointerForString(netxlite.FailureDNSRefusedError),
+		want:    stringPointerForString("dns_server_failure"),
+	}, {
+		name:    "server misbehaving",
+		failure: stringPointerForString(netxlite.FailureDNSServerMisbehaving),
+		want:    stringPointerForString("dns_server_failure"),
+	}, {
+		name:    "temporary failure",
+		failure: stringPointerForString(netxlite.FailureDNSTemporaryFailure),
+		want:    stringPointerForString("dns_server_failure"),
+	}, {
+		name:    "anything else",
+		failure: stringPointerForString(netxlite.FailureEOFError),
+		want:    stringPointerForString("unknown_error"),
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := dnsMapFailure(tt.failure)
+			if diff := cmp.Diff(tt.want, got); diff != "" {
+				t.Fatal(diff)
+			}
+		})
+	}
+}
+
+func TestDNSDo(t *testing.T) {
+	t.Run("returns non-nil addresses list on nxdomin", func(t *testing.T) {
+		ctx := context.Background()
+		config := &DNSConfig{
+			Domain: "antani.ooni.org",
+			Out:    make(chan webconnectivity.ControlDNSResult, 1),
+			Resolver: &mocks.Resolver{
+				MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
+					return nil, netxlite.ErrOODNSNoSuchHost
+				},
+			},
+			Wg: &sync.WaitGroup{},
+		}
+		config.Wg.Add(1)
+		DNSDo(ctx, config)
+		config.Wg.Wait()
+		resp := <-config.Out
+		if resp.Addrs == nil || len(resp.Addrs) != 0 {
+			t.Fatal("returned nil Addrs or Addrs containing replies")
+		}
+	})
+}

internal/cmd/oohelperd/internal/webconnectivity/http.go

@@ -8,6 +8,7 @@ import (
 	"sync"
 
 	"github.com/ooni/probe-cli/v3/internal/engine/experiment/webconnectivity"
+	"github.com/ooni/probe-cli/v3/internal/engine/netx/archival"
 	"github.com/ooni/probe-cli/v3/internal/netxlite"
 )
 
@@ -32,7 +33,7 @@ func HTTPDo(ctx context.Context, config *HTTPConfig) {
 	if err != nil {
 		config.Out <- CtrlHTTPResponse{ // fix: emit -1 like the old test helper does
 			BodyLength: -1,
-			Failure:    newfailure(err),
+			Failure:    httpMapFailure(err),
 			StatusCode: -1,
 			Headers:    map[string]string{},
 		}
@@ -52,7 +53,7 @@ func HTTPDo(ctx context.Context, config *HTTPConfig) {
 	if err != nil {
 		config.Out <- CtrlHTTPResponse{ // fix: emit -1 like old test helper does
 			BodyLength: -1,
-			Failure:    newfailure(err),
+			Failure:    httpMapFailure(err),
 			StatusCode: -1,
 			Headers:    map[string]string{},
 		}
@@ -67,9 +68,56 @@ func HTTPDo(ctx context.Context, config *HTTPConfig) {
 	data, err := netxlite.ReadAllContext(ctx, reader)
 	config.Out <- CtrlHTTPResponse{
 		BodyLength: int64(len(data)),
-		Failure:    newfailure(err),
+		Failure:    httpMapFailure(err),
 		StatusCode: int64(resp.StatusCode),
 		Headers:    headers,
 		Title:      webconnectivity.GetTitle(string(data)),
 	}
 }
+
+// httpMapFailure attempts to map netxlite failures to the strings
+// used by the original OONI test helper.
+//
+// See https://github.com/ooni/backend/blob/6ec4fda5b18/oonib/testhelpers/http_helpers.py#L361
+func httpMapFailure(err error) *string {
+	failure := newfailure(err)
+	failedOperation := archival.NewFailedOperation(err)
+	switch failure {
+	case nil:
+		return nil
+	default:
+		switch *failure {
+		case netxlite.FailureDNSNXDOMAINError,
+			netxlite.FailureDNSNoAnswer,
+			netxlite.FailureDNSNonRecoverableFailure,
+			netxlite.FailureDNSRefusedError,
+			netxlite.FailureDNSServerMisbehaving,
+			netxlite.FailureDNSTemporaryFailure:
+			// Strangely the HTTP code uses the more broad
+			// dns_lookup_error and does not check for
+			// the NXDOMAIN-equivalent-error dns_name_error
+			s := "dns_lookup_error"
+			return &s
+		case netxlite.FailureGenericTimeoutError:
+			// The old TH would return "dns_lookup_error" when
+			// there is a timeout error during the DNS phase of HTTP.
+			switch failedOperation {
+			case nil:
+				// nothing
+			default:
+				switch *failedOperation {
+				case netxlite.ResolveOperation:
+					s := "dns_lookup_error"
+					return &s
+				}
+			}
+			return failure // already using the same name
+		case netxlite.FailureConnectionRefused:
+			s := "connection_refused_error"
+			return &s
+		default:
+			s := "unknown_error"
+			return &s
+		}
+	}
+}

internal/cmd/oohelperd/internal/webconnectivity/http_test.go

@@ -4,9 +4,11 @@ import (
 	"context"
 	"errors"
 	"net/http"
-	"strings"
 	"sync"
 	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/ooni/probe-cli/v3/internal/netxlite"
 )
 
 func TestHTTPDoWithInvalidURL(t *testing.T) {
@@ -25,7 +27,7 @@ func TestHTTPDoWithInvalidURL(t *testing.T) {
 	// wait for measurement steps to complete
 	wg.Wait()
 	resp := <-httpch
-	if resp.Failure == nil || !strings.HasSuffix(*resp.Failure, `invalid port "aaaa" after host`) {
+	if resp.Failure == nil || *resp.Failure != "unknown_error" {
 		t.Fatal("not the failure we expected")
 	}
 }
@@ -51,7 +53,79 @@ func TestHTTPDoWithHTTPTransportFailure(t *testing.T) {
 	// wait for measurement steps to complete
 	wg.Wait()
 	resp := <-httpch
-	if resp.Failure == nil || !strings.HasSuffix(*resp.Failure, "mocked error") {
+	if resp.Failure == nil || *resp.Failure != "unknown_error" {
 		t.Fatal("not the error we expected")
 	}
 }
+
+func newErrWrapper(failure, operation string) error {
+	return &netxlite.ErrWrapper{
+		Failure:    failure,
+		Operation:  operation,
+		WrappedErr: nil, // should not matter
+	}
+}
+
+func newErrWrapperTopLevel(failure string) error {
+	return newErrWrapper(failure, netxlite.TopLevelOperation)
+}
+
+func Test_httpMapFailure(t *testing.T) {
+	tests := []struct {
+		name    string
+		failure error
+		want    *string
+	}{{
+		name:    "nil",
+		failure: nil,
+		want:    nil,
+	}, {
+		name:    "nxdomain",
+		failure: newErrWrapperTopLevel(netxlite.FailureDNSNXDOMAINError),
+		want:    stringPointerForString("dns_lookup_error"),
+	}, {
+		name:    "no answer",
+		failure: newErrWrapperTopLevel(netxlite.FailureDNSNoAnswer),
+		want:    stringPointerForString("dns_lookup_error"),
+	}, {
+		name:    "non recoverable failure",
+		failure: newErrWrapperTopLevel(netxlite.FailureDNSNonRecoverableFailure),
+		want:    stringPointerForString("dns_lookup_error"),
+	}, {
+		name:    "refused",
+		failure: newErrWrapperTopLevel(netxlite.FailureDNSRefusedError),
+		want:    stringPointerForString("dns_lookup_error"),
+	}, {
+		name:    "server misbehaving",
+		failure: newErrWrapperTopLevel(netxlite.FailureDNSServerMisbehaving),
+		want:    stringPointerForString("dns_lookup_error"),
+	}, {
+		name:    "temporary failure",
+		failure: newErrWrapperTopLevel(netxlite.FailureDNSTemporaryFailure),
+		want:    stringPointerForString("dns_lookup_error"),
+	}, {
+		name:    "timeout outside of dns lookup",
+		failure: newErrWrapperTopLevel(netxlite.FailureGenericTimeoutError),
+		want:    stringPointerForString(netxlite.FailureGenericTimeoutError),
+	}, {
+		name:    "timeout inside of dns lookup",
+		failure: newErrWrapper(netxlite.FailureGenericTimeoutError, netxlite.ResolveOperation),
+		want:    stringPointerForString("dns_lookup_error"),
+	}, {
+		name:    "connection refused",
+		failure: newErrWrapperTopLevel(netxlite.FailureConnectionRefused),
+		want:    stringPointerForString("connection_refused_error"),
+	}, {
+		name:    "anything else",
+		failure: newErrWrapperTopLevel(netxlite.FailureEOFError),
+		want:    stringPointerForString("unknown_error"),
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := httpMapFailure(tt.failure)
+			if diff := cmp.Diff(tt.want, got); diff != "" {
+				t.Fatal(diff)
+			}
+		})
+	}
+}

internal/cmd/oohelperd/internal/webconnectivity/tcpconnect.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/ooni/probe-cli/v3/internal/engine/experiment/webconnectivity"
 	"github.com/ooni/probe-cli/v3/internal/engine/netx"
+	"github.com/ooni/probe-cli/v3/internal/netxlite"
 )
 
 // CtrlTCPResult is the result of the TCP check performed by the test helper.
@@ -35,8 +36,34 @@ func TCPDo(ctx context.Context, config *TCPConfig) {
 	config.Out <- TCPResultPair{
 		Endpoint: config.Endpoint,
 		Result: CtrlTCPResult{
-			Failure: newfailure(err),
+			Failure: tcpMapFailure(newfailure(err)),
 			Status:  err == nil,
 		},
 	}
 }
+
+// tcpMapFailure attempts to map netxlite failures to the strings
+// used by the original OONI test helper.
+//
+// See https://github.com/ooni/backend/blob/6ec4fda5b18/oonib/testhelpers/http_helpers.py#L392
+func tcpMapFailure(failure *string) *string {
+	switch failure {
+	case nil:
+		return nil
+	default:
+		switch *failure {
+		case netxlite.FailureGenericTimeoutError:
+			return failure // already using the same name
+		case netxlite.FailureConnectionRefused:
+			s := "connection_refused_error"
+			return &s
+		default:
+			// The definition of this error according to Twisted is
+			// "something went wrong when connecting". Because we are
+			// indeed basically just connecting here, it seems safe
+			// to map any other error to "connect_error" here.
+			s := "connect_error"
+			return &s
+		}
+	}
+}

internal/cmd/oohelperd/internal/webconnectivity/tcpconnect_test.go

@@ -0,0 +1,40 @@
+package webconnectivity
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/ooni/probe-cli/v3/internal/netxlite"
+)
+
+func Test_tcpMapFailure(t *testing.T) {
+	tests := []struct {
+		name    string
+		failure *string
+		want    *string
+	}{{
+		name:    "nil",
+		failure: nil,
+		want:    nil,
+	}, {
+		name:    "timeout",
+		failure: stringPointerForString(netxlite.FailureGenericTimeoutError),
+		want:    stringPointerForString(netxlite.FailureGenericTimeoutError),
+	}, {
+		name:    "connection refused",
+		failure: stringPointerForString(netxlite.FailureConnectionRefused),
+		want:    stringPointerForString("connection_refused_error"),
+	}, {
+		name:    "anything else",
+		failure: stringPointerForString(netxlite.FailureEOFError),
+		want:    stringPointerForString("connect_error"),
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := tcpMapFailure(tt.failure)
+			if diff := cmp.Diff(tt.want, got); diff != "" {
+				t.Fatal(diff)
+			}
+		})
+	}
+}