Merge pull request #8053 from hashicorp/jbardin/atlas-retry-policy

don't retry the atlas requests with the wrong cert

state/remote/atlas.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"crypto/md5"
 	"crypto/tls"
+	"crypto/x509"
 	"encoding/base64"
 	"fmt"
 	"io"
@@ -276,9 +277,26 @@ func (c *AtlasClient) http() (*retryablehttp.Client, error) {
 		return nil, err
 	}
 	rc := retryablehttp.NewClient()
+
+	rc.CheckRetry = func(resp *http.Response, err error) (bool, error) {
+		if err != nil {
+			// don't bother retrying if the certs don't match
+			if err, ok := err.(*url.Error); ok {
+				if _, ok := err.Err.(x509.UnknownAuthorityError); ok {
+					return false, nil
+				}
+			}
+			// continue retrying
+			return true, nil
+		}
+		return retryablehttp.DefaultRetryPolicy(resp, err)
+	}
+
 	t := cleanhttp.DefaultTransport()
 	t.TLSClientConfig = tlsConfig
 	rc.HTTPClient.Transport = t
+
+	c.HTTPClient = rc
 	return rc, nil
 }
 

state/remote/atlas_test.go

@@ -3,8 +3,11 @@ package remote
 import (
 	"bytes"
 	"crypto/md5"
+	"crypto/tls"
+	"crypto/x509"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"os"
 	"testing"
 	"time"
@@ -36,6 +39,53 @@ func TestAtlasClient(t *testing.T) {
 	testClient(t, client)
 }
 
+func TestAtlasClient_noRetryOnBadCerts(t *testing.T) {
+	acctest.RemoteTestPrecheck(t)
+
+	client, err := atlasFactory(map[string]string{
+		"access_token": "NOT_REQUIRED",
+		"name":         "hashicorp/test-remote-state",
+	})
+	if err != nil {
+		t.Fatalf("bad: %s", err)
+	}
+
+	ac := client.(*AtlasClient)
+	// trigger the AtlasClient to build the http client and assign HTTPClient
+	httpClient, err := ac.http()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// remove the CA certs from the client
+	brokenCfg := &tls.Config{
+		RootCAs: new(x509.CertPool),
+	}
+	httpClient.HTTPClient.Transport.(*http.Transport).TLSClientConfig = brokenCfg
+
+	// Instrument CheckRetry to make sure we didn't retry
+	retries := 0
+	oldCheck := httpClient.CheckRetry
+	httpClient.CheckRetry = func(resp *http.Response, err error) (bool, error) {
+		if retries > 0 {
+			t.Fatal("retried after certificate error")
+		}
+		retries++
+		return oldCheck(resp, err)
+	}
+
+	_, err = client.Get()
+	if err != nil {
+		if err, ok := err.(*url.Error); ok {
+			if _, ok := err.Err.(x509.UnknownAuthorityError); ok {
+				return
+			}
+		}
+	}
+
+	t.Fatalf("expected x509.UnknownAuthorityError, got %v", err)
+}
+
 func TestAtlasClient_ReportedConflictEqualStates(t *testing.T) {
 	fakeAtlas := newFakeAtlas(t, testStateModuleOrderChange)
 	srv := fakeAtlas.Server()

vendor/github.com/hashicorp/go-retryablehttp/.gitignore

@@ -1,3 +0,0 @@
-.idea/
-*.iml
-*.test

vendor/github.com/hashicorp/go-retryablehttp/.travis.yml

@@ -1,12 +0,0 @@
-sudo: false
-
-language: go
-
-go:
-  - 1.5.1
-
-branches:
-  only:
-    - master
-
-script: make updatedeps test

vendor/github.com/hashicorp/go-retryablehttp/client.go

@@ -38,6 +38,10 @@ var (
 	// defaultClient is used for performing requests without explicitly making
 	// a new client. It is purposely private to avoid modifications.
 	defaultClient = NewClient()
+
+	// We need to consume response bodies to maintain http connections, but
+	// limit the size we consume to respReadLimit.
+	respReadLimit = int64(4096)
 )
 
 // LenReader is an interface implemented by many in-memory io.Reader's. Used
@@ -86,6 +90,23 @@ func NewRequest(method, url string, body io.ReadSeeker) (*Request, error) {
 // consumers.
 type RequestLogHook func(*log.Logger, *http.Request, int)
 
+// ResponseLogHook is like RequestLogHook, but allows running a function
+// on each HTTP response. This function will be invoked at the end of
+// every HTTP request executed, regardless of whether a subsequent retry
+// needs to be performed or not. If the response body is read or closed
+// from this method, this will affect the response returned from Do().
+type ResponseLogHook func(*log.Logger, *http.Response)
+
+// CheckRetry specifies a policy for handling retries. It is called
+// following each request with the response and error values returned by
+// the http.Client. If CheckRetry returns false, the Client stops retrying
+// and returns the response to the caller. If CheckRetry returns an error,
+// that error value is returned in lieu of the error from the request. The
+// Client will close any response body when retrying, but if the retry is
+// aborted it is up to the CheckResponse callback to properly close any
+// response body before returning.
+type CheckRetry func(resp *http.Response, err error) (bool, error)
+
 // Client is used to make HTTP requests. It adds additional functionality
 // like automatic retries to tolerate minor outages.
 type Client struct {
@@ -99,6 +120,14 @@ type Client struct {
 	// RequestLogHook allows a user-supplied function to be called
 	// before each retry.
 	RequestLogHook RequestLogHook
+
+	// ResponseLogHook allows a user-supplied function to be called
+	// with the response from each HTTP request executed.
+	ResponseLogHook ResponseLogHook
+
+	// CheckRetry specifies the policy for handling retries, and is called
+	// after each request. The default policy is DefaultRetryPolicy.
+	CheckRetry CheckRetry
 }
 
 // NewClient creates a new Client with default settings.
@@ -109,9 +138,27 @@ func NewClient() *Client {
 		RetryWaitMin: defaultRetryWaitMin,
 		RetryWaitMax: defaultRetryWaitMax,
 		RetryMax:     defaultRetryMax,
+		CheckRetry:   DefaultRetryPolicy,
 	}
 }
 
+// DefaultRetryPolicy provides a default callback for Client.CheckRetry, which
+// will retry on connection errors and server errors.
+func DefaultRetryPolicy(resp *http.Response, err error) (bool, error) {
+	if err != nil {
+		return true, err
+	}
+	// Check the response code. We retry on 500-range responses to allow
+	// the server time to recover, as 500's are typically not permanent
+	// errors and may relate to outages on the server side. This will catch
+	// invalid response codes as well, like 0 and 999.
+	if resp.StatusCode == 0 || resp.StatusCode >= 500 {
+		return true, nil
+	}
+
+	return false, nil
+}
+
 // Do wraps calling an HTTP method with retries.
 func (c *Client) Do(req *Request) (*http.Response, error) {
 	c.Logger.Printf("[DEBUG] %s %s", req.Method, req.URL)
@@ -132,23 +179,36 @@ func (c *Client) Do(req *Request) (*http.Response, error) {
 
 		// Attempt the request
 		resp, err := c.HTTPClient.Do(req.Request)
+
+		// Check if we should continue with retries.
+		checkOK, checkErr := c.CheckRetry(resp, err)
+
 		if err != nil {
 			c.Logger.Printf("[ERR] %s %s request failed: %v", req.Method, req.URL, err)
-			goto RETRY
+		} else {
+			// Call this here to maintain the behavior of logging all requests,
+			// even if CheckRetry signals to stop.
+			if c.ResponseLogHook != nil {
+				// Call the response logger function if provided.
+				c.ResponseLogHook(c.Logger, resp)
+			}
 		}
-		code = resp.StatusCode
 
-		// Check the response code. We retry on 500-range responses to allow
-		// the server time to recover, as 500's are typically not permanent
-		// errors and may relate to outages on the server side.
-		if code%500 < 100 {
-			resp.Body.Close()
-			goto RETRY
+		// Now decide if we should continue.
+		if !checkOK {
+			if checkErr != nil {
+				err = checkErr
+			}
+			return resp, err
 		}
-		return resp, nil
 
-	RETRY:
-		if i == c.RetryMax {
+		// We're going to retry, consume any response to reuse the connection.
+		if err == nil {
+			c.drainBody(resp.Body)
+		}
+
+		remain := c.RetryMax - i
+		if remain == 0 {
 			break
 		}
 		wait := backoff(c.RetryWaitMin, c.RetryWaitMax, i)
@@ -156,7 +216,7 @@ func (c *Client) Do(req *Request) (*http.Response, error) {
 		if code > 0 {
 			desc = fmt.Sprintf("%s (status: %d)", desc, code)
 		}
-		c.Logger.Printf("[DEBUG] %s: retrying in %s", desc, wait)
+		c.Logger.Printf("[DEBUG] %s: retrying in %s (%d left)", desc, wait, remain)
 		time.Sleep(wait)
 	}
 
@@ -165,6 +225,15 @@ func (c *Client) Do(req *Request) (*http.Response, error) {
 		req.Method, req.URL, c.RetryMax+1)
 }
 
+// Try to read the response body so we can reuse this connection.
+func (c *Client) drainBody(body io.ReadCloser) {
+	defer body.Close()
+	_, err := io.Copy(ioutil.Discard, io.LimitReader(body, respReadLimit))
+	if err != nil {
+		c.Logger.Printf("[ERR] error reading response body: %v", err)
+	}
+}
+
 // Get is a shortcut for doing a GET request without making a new client.
 func Get(url string) (*http.Response, error) {
 	return defaultClient.Get(url)

vendor/vendor.json

@@ -1108,8 +1108,10 @@
 			"revision": "cccb4a1328abbb89898f3ecf4311a05bddc4de6d"
 		},
 		{
+			"checksumSHA1": "GBDE1KDl/7c5hlRPYRZ7+C0WQ0g=",
 			"path": "github.com/hashicorp/go-retryablehttp",
-			"revision": "5ec125ef739293cb4d57c3456dd92ba9af29ed6e"
+			"revision": "f4ed9b0fa01a2ac614afe7c897ed2e3d8208f3e8",
+			"revisionTime": "2016-08-10T17:22:55Z"
 		},
 		{
 			"path": "github.com/hashicorp/go-rootcerts",