From ee6159cd9dbeb6c13a8595ca7135dd09b9cfb5b8 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 10 Aug 2016 13:33:42 -0400 Subject: [PATCH 1/2] update github.com/hashicorp/go-retryablehttp --- .../hashicorp/go-retryablehttp/.gitignore | 3 - .../hashicorp/go-retryablehttp/.travis.yml | 12 --- .../hashicorp/go-retryablehttp/client.go | 93 ++++++++++++++++--- vendor/vendor.json | 4 +- 4 files changed, 84 insertions(+), 28 deletions(-) delete mode 100644 vendor/github.com/hashicorp/go-retryablehttp/.gitignore delete mode 100644 vendor/github.com/hashicorp/go-retryablehttp/.travis.yml diff --git a/vendor/github.com/hashicorp/go-retryablehttp/.gitignore b/vendor/github.com/hashicorp/go-retryablehttp/.gitignore deleted file mode 100644 index caab963a3..000000000 --- a/vendor/github.com/hashicorp/go-retryablehttp/.gitignore +++ /dev/null @@ -1,3 +0,0 @@ -.idea/ -*.iml -*.test diff --git a/vendor/github.com/hashicorp/go-retryablehttp/.travis.yml b/vendor/github.com/hashicorp/go-retryablehttp/.travis.yml deleted file mode 100644 index 49c8bb75d..000000000 --- a/vendor/github.com/hashicorp/go-retryablehttp/.travis.yml +++ /dev/null @@ -1,12 +0,0 @@ -sudo: false - -language: go - -go: - - 1.5.1 - -branches: - only: - - master - -script: make updatedeps test diff --git a/vendor/github.com/hashicorp/go-retryablehttp/client.go b/vendor/github.com/hashicorp/go-retryablehttp/client.go index db9eada8a..d0ec6b2ab 100644 --- a/vendor/github.com/hashicorp/go-retryablehttp/client.go +++ b/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) diff --git a/vendor/vendor.json b/vendor/vendor.json index ee31a419f..f25283096 100644 --- a/vendor/vendor.json +++ b/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", From df0c795b39c3c264862d20545809a118e4b94321 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 8 Aug 2016 13:33:45 -0400 Subject: [PATCH 2/2] Don't retry the atlas requests with the wrong cert This probably won't recover, so abort immediately. Requires retryablehttp CheckRetry patch. --- state/remote/atlas.go | 18 ++++++++++++++ state/remote/atlas_test.go | 50 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/state/remote/atlas.go b/state/remote/atlas.go index 5343c0236..ead0acbcb 100644 --- a/state/remote/atlas.go +++ b/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 } diff --git a/state/remote/atlas_test.go b/state/remote/atlas_test.go index 9d4f226fe..060d79455 100644 --- a/state/remote/atlas_test.go +++ b/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()