From 276beb871910afbd185d2073f928133a16e1e340 Mon Sep 17 00:00:00 2001
From: Simone Basso <bassosimone@gmail.com>
Date: Sun, 5 Sep 2021 12:06:02 +0200
Subject: [PATCH] fix(oohelper): do not stop if DNS fails locally (#450)

When a probe gets a local DNS failure, it will continue and nonetheless
query the test helper without any IP address, just an empty list.

This diff fixes the behavior of cmd/oohelper to do the same.

Work part of https://github.com/ooni/probe/issues/1707.
---
 internal/cmd/oohelper/internal/client.go      | 12 ++++++------
 internal/cmd/oohelper/internal/client_test.go | 19 ++++++++++++++-----
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/internal/cmd/oohelper/internal/client.go b/internal/cmd/oohelper/internal/client.go
index 468b284..0f9fd23 100644
--- a/internal/cmd/oohelper/internal/client.go
+++ b/internal/cmd/oohelper/internal/client.go
@@ -91,12 +91,12 @@ func (oo OOClient) Do(ctx context.Context, config OOConfig) (*CtrlResponse, erro
 		return nil, fmt.Errorf("%w: %s", ErrInvalidURL, err.Error())
 	}
 	addrs, err := oo.Resolver.LookupHost(ctx, targetURL.Hostname())
-	if err != nil {
-		return nil, err
-	}
-	endpoints, err := MakeTCPEndpoints(targetURL, addrs)
-	if err != nil {
-		return nil, err
+	endpoints := []string{}
+	if err == nil {
+		endpoints, err = MakeTCPEndpoints(targetURL, addrs)
+		if err != nil {
+			return nil, err
+		}
 	}
 	creq := ctrlRequest{
 		HTTPRequest: config.TargetURL,
diff --git a/internal/cmd/oohelper/internal/client_test.go b/internal/cmd/oohelper/internal/client_test.go
index f663408..2427d49 100644
--- a/internal/cmd/oohelper/internal/client_test.go
+++ b/internal/cmd/oohelper/internal/client_test.go
@@ -104,14 +104,23 @@ func TestOOClientDoWithResolverFailure(t *testing.T) {
 		ServerURL: "https://wcth.ooni.io",
 	}
 	clnt := internal.OOClient{
-		Resolver: internal.NewFakeResolverThatFails(),
+		HTTPClient: http.DefaultClient,
+		Resolver:   internal.NewFakeResolverThatFails(),
 	}
 	cresp, err := clnt.Do(ctx, config)
-	if !errors.Is(err, internal.ErrNotFound) {
-		t.Fatalf("not the error we expected: %+v", err)
+	if err != nil {
+		t.Fatal(err)
 	}
-	if cresp != nil {
-		t.Fatal("expected nil response")
+	if len(cresp.TCPConnect) > 0 {
+		// The current implementation of the test helper (the legacy codebase)
+		// only follows the IP addresses returned by the client.
+		t.Fatal("expected empty TCPConnect here")
+	}
+	if cresp.HTTPRequest.StatusCode != 200 {
+		t.Fatal("expected 200 status code here")
+	}
+	if len(cresp.DNS.Addrs) < 1 {
+		t.Fatal("expected at least an IP address here")
 	}
 }