From c3d4edcf0c0b648d9eb85c208ca486d57bbaef21 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jean-R=C3=A9my=20Bancel?= <bancel@google.com>
Date: Fri, 4 Oct 2019 14:58:08 -0700
Subject: [PATCH] Handle and retry 'connection reset by peer' errors. (#746)

---
 test/spoof/error_checks.go      | 11 ++++---
 test/spoof/error_checks_test.go | 57 ++++++++++++++++++++++++---------
 test/spoof/spoof.go             | 11 ++++---
 3 files changed, 55 insertions(+), 24 deletions(-)

diff --git a/test/spoof/error_checks.go b/test/spoof/error_checks.go
index 0cd2995ca..8a913b99d 100644
--- a/test/spoof/error_checks.go
+++ b/test/spoof/error_checks.go
@@ -40,13 +40,14 @@ func isDNSError(err error) bool {
 	return strings.Contains(msg, "no such host") || strings.Contains(msg, ":53")
 }
 
-func isTCPConnectRefuse(err error) bool {
+func isConnectionRefused(err error) bool {
 	// The alternative for the string check is:
 	// 	errNo := (((err.(*url.Error)).Err.(*net.OpError)).Err.(*os.SyscallError).Err).(syscall.Errno)
 	// if errNo == syscall.Errno(0x6f) {...}
 	// But with assertions, of course.
-	if err != nil && strings.Contains(err.Error(), "connect: connection refused") {
-		return true
-	}
-	return false
+	return err != nil && strings.Contains(err.Error(), "connect: connection refused")
+}
+
+func isConnectionReset(err error) bool {
+	return err != nil && strings.Contains(err.Error(), "connection reset by peer")
 }
diff --git a/test/spoof/error_checks_test.go b/test/spoof/error_checks_test.go
index 9d96d0504..6d7ae0158 100644
--- a/test/spoof/error_checks_test.go
+++ b/test/spoof/error_checks_test.go
@@ -19,6 +19,7 @@ limitations under the License.
 package spoof
 
 import (
+	"errors"
 	"net/http"
 	"testing"
 )
@@ -57,31 +58,57 @@ func TestDNSError(t *testing.T) {
 	}
 }
 
-func TestTCPConnectRefuse(t *testing.T) {
+func TestConnectionRefused(t *testing.T) {
 	client := &http.Client{}
 
 	for _, tt := range []struct {
-		name      string
-		url       string
-		tcpRefuse bool
+		name        string
+		url         string
+		connRefused bool
 	}{{
-		name:      "nothing listening",
-		url:       "http://localhost:60001",
-		tcpRefuse: true,
+		name:        "nothing listening",
+		url:         "http://localhost:60001",
+		connRefused: true,
 	}, {
-		name:      "dns error",
-		url:       "http://this.url.does.not.exist",
-		tcpRefuse: false,
+		name:        "dns error",
+		url:         "http://this.url.does.not.exist",
+		connRefused: false,
 	}, {
-		name:      "google.com",
-		url:       "https://google.com",
-		tcpRefuse: false,
+		name:        "google.com",
+		url:         "https://google.com",
+		connRefused: false,
 	}} {
 		t.Run(tt.name, func(t *testing.T) {
 			req, _ := http.NewRequest("GET", tt.url, nil)
 			_, err := client.Do(req)
-			if tcpRefuse := isTCPConnectRefuse(err); tt.tcpRefuse != tcpRefuse {
-				t.Errorf("Expected tcpRefuse=%v, got %v", tt.tcpRefuse, tcpRefuse)
+			if connRefused := isConnectionRefused(err); tt.connRefused != connRefused {
+				t.Errorf("Expected connRefused=%v, got %v", tt.connRefused, connRefused)
+			}
+		})
+	}
+}
+
+func TestConnectionReset(t *testing.T) {
+	for _, tt := range []struct {
+		name      string
+		err       error
+		connReset bool
+	}{{
+		name:      "error matching",
+		err:       errors.New("read tcp 10.60.2.57:47882->104.154.144.94:80: read: connection reset by peer"),
+		connReset: true,
+	}, {
+		name:      "error not matching",
+		err:       errors.New("dial tcp: lookup this.url.does.not.exist on 127.0.0.1:53: no such host"),
+		connReset: false,
+	}, {
+		name:      "nil error",
+		err:       nil,
+		connReset: false,
+	}} {
+		t.Run(tt.name, func(t *testing.T) {
+			if connReset := isConnectionReset(tt.err); tt.connReset != connReset {
+				t.Errorf("Expected connReset=%v, got %v", tt.connReset, connReset)
 			}
 		})
 	}
diff --git a/test/spoof/spoof.go b/test/spoof/spoof.go
index 6640e54f6..413878522 100644
--- a/test/spoof/spoof.go
+++ b/test/spoof/spoof.go
@@ -214,19 +214,22 @@ func (sc *SpoofingClient) Poll(req *http.Request, inState ResponseChecker) (*Res
 		resp, err = sc.Do(req)
 		if err != nil {
 			if isTCPTimeout(err) {
-				sc.Logf("Retrying %s for TCP timeout %v", req.URL.String(), err)
+				sc.Logf("Retrying %s for TCP timeout: %v", req.URL, err)
 				return false, nil
 			}
 			// Retrying on DNS error, since we may be using xip.io or nip.io in tests.
 			if isDNSError(err) {
-				sc.Logf("Retrying %s for DNS error %v", req.URL.String(), err)
+				sc.Logf("Retrying %s for DNS error: %v", req.URL, err)
 				return false, nil
 			}
 			// Repeat the poll on `connection refused` errors, which are usually transient Istio errors.
-			if isTCPConnectRefuse(err) {
-				sc.Logf("Retrying %s for connection refused %v", req.URL.String(), err)
+			if isConnectionRefused(err) {
+				sc.Logf("Retrying %s for connection refused: %v", req.URL, err)
 				return false, nil
 			}
+			if isConnectionReset(err) {
+				sc.Logf("Retrying %s for connection reset: %v", req.URL, err)
+			}
 			return true, err
 		}