From e6f3abe15e00e3594109de976c2b18c015d1f8c7 Mon Sep 17 00:00:00 2001
From: Michael Marineau <michael.marineau@coreos.com>
Date: Thu, 11 May 2017 12:43:42 -0700
Subject: [PATCH] client: support retrying requests after transient HTTP errors

Wraps failed http.Request objects in a net.Error interface.
---
 omaha/client/error.go     |  57 ++++++++++++++++++++
 omaha/client/http.go      |   9 +++-
 omaha/client/http_test.go | 109 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 174 insertions(+), 1 deletion(-)
 create mode 100644 omaha/client/error.go

diff --git a/omaha/client/error.go b/omaha/client/error.go
new file mode 100644
index 0000000..cfe8f86
--- /dev/null
+++ b/omaha/client/error.go
@@ -0,0 +1,57 @@
+// Copyright 2017 CoreOS, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package client
+
+import (
+	"net/http"
+)
+
+// httpError implements error and net.Error for http responses.
+type httpError struct {
+	*http.Response
+}
+
+func (he *httpError) Error() string {
+	return "http error: " + he.Status
+}
+
+func (he *httpError) Timeout() bool {
+	switch he.StatusCode {
+	case http.StatusRequestTimeout: // 408
+		return true
+	case http.StatusGatewayTimeout: // 504
+		return true
+	default:
+		return false
+	}
+}
+
+func (he *httpError) Temporary() bool {
+	if he.Timeout() {
+		return true
+	}
+	switch he.StatusCode {
+	case http.StatusTooManyRequests: // 429
+		return true
+	case http.StatusInternalServerError: // 500
+		return true
+	case http.StatusBadGateway: // 502
+		return true
+	case http.StatusServiceUnavailable: // 503
+		return true
+	default:
+		return false
+	}
+}
diff --git a/omaha/client/http.go b/omaha/client/http.go
index bde2610..7c90378 100644
--- a/omaha/client/http.go
+++ b/omaha/client/http.go
@@ -51,7 +51,14 @@ func (hc *httpClient) doPost(url string, reqBody []byte) (*omaha.Response, error
 	defer resp.Body.Close()
 
 	contentType := resp.Header.Get("Content-Type")
-	return omaha.ParseResponse(contentType, resp.Body)
+	omahaResp, err := omaha.ParseResponse(contentType, resp.Body)
+
+	// Prefer reporting HTTP errors over XML parsing errors.
+	if resp.StatusCode != http.StatusOK {
+		err = &httpError{resp}
+	}
+
+	return omahaResp, err
 }
 
 // Omaha encodes and sends an omaha request, retrying on any transient errors.
diff --git a/omaha/client/http_test.go b/omaha/client/http_test.go
index fa7eed2..9dda26f 100644
--- a/omaha/client/http_test.go
+++ b/omaha/client/http_test.go
@@ -15,6 +15,9 @@
 package client
 
 import (
+	"net"
+	"net/http"
+	"strings"
 	"testing"
 
 	"github.com/coreos/go-omaha/omaha"
@@ -55,3 +58,109 @@ func TestHTTPClientDoPost(t *testing.T) {
 		t.Fatalf("Bad apps status: %q", resp.Apps[0].Status)
 	}
 }
+
+type flakyHandler struct {
+	omaha.OmahaHandler
+	flakes int
+	reqs   int
+}
+
+func (f *flakyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	f.reqs++
+	if f.flakes > 0 {
+		f.flakes--
+		http.Error(w, "Flake!", http.StatusInternalServerError)
+		return
+	}
+	f.OmahaHandler.ServeHTTP(w, r)
+}
+
+type flakyServer struct {
+	l net.Listener
+	s *http.Server
+	h *flakyHandler
+}
+
+func newFlakyServer() (*flakyServer, error) {
+	l, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		return nil, err
+	}
+
+	f := &flakyServer{
+		l: l,
+		s: &http.Server{},
+		h: &flakyHandler{
+			OmahaHandler: omaha.OmahaHandler{
+				Updater: omaha.UpdaterStub{},
+			},
+			flakes: 1,
+		},
+	}
+	f.s.Handler = f.h
+
+	go f.s.Serve(l)
+	return f, nil
+}
+
+func TestHTTPClientError(t *testing.T) {
+	f, err := newFlakyServer()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer f.l.Close()
+
+	c := newHTTPClient()
+	url := "http://" + f.l.Addr().String()
+
+	_, err = c.doPost(url, []byte(sampleRequest))
+	switch err := err.(type) {
+	case nil:
+		t.Fatal("doPost succeeded but should have failed")
+	case *httpError:
+		if err.StatusCode != http.StatusInternalServerError {
+			t.Fatalf("Unexpected http error: %v", err)
+		}
+		if err.Timeout() {
+			t.Fatal("http 500 error reported as timeout")
+		}
+		if !err.Temporary() {
+			t.Fatal("http 500 error not reported as temporary")
+		}
+	default:
+		t.Fatalf("Unexpected error: %v", err)
+	}
+}
+
+func TestHTTPClientRetry(t *testing.T) {
+	f, err := newFlakyServer()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer f.l.Close()
+
+	req, err := omaha.ParseRequest("", strings.NewReader(sampleRequest))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	c := newHTTPClient()
+	url := "http://" + f.l.Addr().String()
+
+	resp, err := c.Omaha(url, req)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if len(resp.Apps) != 1 {
+		t.Fatalf("Should be 1 app, not %d", len(resp.Apps))
+	}
+
+	if resp.Apps[0].Status != omaha.AppOK {
+		t.Fatalf("Bad apps status: %q", resp.Apps[0].Status)
+	}
+
+	if f.h.reqs != 2 {
+		t.Fatalf("Server received %d requests, not 2", f.h.reqs)
+	}
+}