Merge pull request #549 from ooni/issue/1839

stable: backport from master useful patches for 3.11.0 

See ooni/probe#1839

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,40 @@ 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:
+ // In this case the legacy TH would produce an empty
+ // reply that is not attached to any error.
+ //
+ // See https://github.com/ooni/probe/issues/1707#issuecomment-944322725
+ return nil
+ case 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: nil,
+ }, {
+ 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,8 +33,9 @@ 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{},
  }
  return
  }
@@ -51,8 +53,9 @@ 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{},
  }
  return
  }
@@ -65,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/measure.go

@@ -76,7 +76,12 @@ func Measure(ctx context.Context, config MeasureConfig, creq *CtrlRequest) (*Ctr
  select {
  case cresp.DNS = <-dnsch:
  default:
- // we land here when there's no domain name
+ // we need to emit a non-nil Addrs to match exactly
+ // the behavior of the legacy TH
+ cresp.DNS = CtrlDNSResult{
+ Failure: nil,
+ Addrs: []string{},
+ }
  }
  cresp.HTTPRequest = <-httpch
  cresp.TCPConnect = make(map[string]CtrlTCPResult)

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
+ }
+ }
+}