Switch to pester from go-retryablehttp to avoid swallowing 500 error messages

CHANGELOG.md

@@ -4,7 +4,6 @@ DEPRECATIONS/BREAKING CHANGES:
 
  * Issued certificates from the `pki` backend against new roles created or
    modified after upgrading will contain a set of default key usages. 
- * In the Go API, the function signature for `Request.ToHTTP()` has changed.
 
 FEATURES:
 
@@ -20,10 +19,9 @@ FEATURES:
    compatibility with OpenVPN and some other software. This set can be changed
    when writing a role definition. Existing roles are unaffected. [GH-1552]
  * **Request Retrying in the CLI and Go API**: Requests that fail with a `5xx`
-   error code will now retry after a backoff. The minimum and maximum backoff
+   error code will now retry after a backoff. The maximum total number of
-   times, as well as the maximum total number of retries (including disabling
+   retries (including disabling this functionality) can be set with an
-   this functionality) can be set with environment variables. See the
+   environment variable. See the [environment variable
-   [environment variable
    documentation](https://www.vaultproject.io/docs/commands/environment.html)
    for more details. [GH-1594]
 

api/client.go

@@ -13,8 +13,8 @@ import (
 	"time"
 
 	"github.com/hashicorp/go-cleanhttp"
-	"github.com/hashicorp/go-retryablehttp"
 	"github.com/hashicorp/go-rootcerts"
+	"github.com/sethgrid/pester"
 )
 
 const EnvVaultAddress = "VAULT_ADDR"
@@ -25,9 +25,7 @@ const EnvVaultClientKey = "VAULT_CLIENT_KEY"
 const EnvVaultInsecure = "VAULT_SKIP_VERIFY"
 const EnvVaultTLSServerName = "VAULT_TLS_SERVER_NAME"
 const EnvVaultWrapTTL = "VAULT_WRAP_TTL"
-const EnvVaultRetryWaitMin = "VAULT_RETRY_WAIT_MIN"
+const EnvVaultMaxRetries = "VAULT_MAX_RETRIES"
-const EnvVaultRetryWaitMax = "VAULT_RETRY_WAIT_MAX"
-const EnvVaultRetryMax = "VAULT_RETRY_MAX"
 
 var (
 	errRedirect = errors.New("redirect")
@@ -54,17 +52,9 @@ type Config struct {
 
 	redirectSetup sync.Once
 
-	// RetryWaitMin controls the minimum amount of time to wait between retries
+	// MaxRetries controls the maximum number of times to retry when a 5xx error
-	// when a 5xx error occurs
+	// occurs. Set to 1 or less to disable retrying.
-	RetryWaitMin time.Duration
+	MaxRetries int
-
-	// RetryWaitMax controls the maximum amount of time to wait between retries
-	// when a 5xx error occurs
-	RetryWaitMax time.Duration
-
-	// RetryMax controls the maximum number of times to retry when a 5xx error
-	// occurs. Set to 0 to disable retrying.
-	RetryMax int
 }
 
 // DefaultConfig returns a default configuration for the client. It is
@@ -89,9 +79,7 @@ func DefaultConfig() *Config {
 		config.Address = v
 	}
 
-	config.RetryWaitMin = 1 * time.Second
+	config.MaxRetries = pester.DefaultClient.MaxRetries
-	config.RetryWaitMax = 30 * time.Second
-	config.RetryMax = 15
 
 	return config
 }
@@ -109,9 +97,7 @@ func (c *Config) ReadEnvironment() error {
 	var foundInsecure bool
 	var envTLSServerName string
 
-	var envRetryWaitMin *time.Duration
+	var envMaxRetries *uint64
-	var envRetryWaitMax *time.Duration
-	var envRetryMax *uint64
 
 	var clientCert tls.Certificate
 	var foundClientCert bool
@@ -120,44 +106,13 @@ func (c *Config) ReadEnvironment() error {
 	if v := os.Getenv(EnvVaultAddress); v != "" {
 		envAddress = v
 	}
-
+	if v := os.Getenv(EnvVaultMaxRetries); v != "" {
-	// Handle retry parameters
+		maxRetries, err := strconv.ParseUint(v, 10, 32)
-	{
+		if err != nil {
-		if v := os.Getenv(EnvVaultRetryWaitMin); v != "" {
+			return err
-			waitMin, err := time.ParseDuration(v)
-			if err != nil {
-				return err
-			}
-			envRetryWaitMin = &waitMin
-		}
-		if v := os.Getenv(EnvVaultRetryWaitMax); v != "" {
-			waitMax, err := time.ParseDuration(v)
-			if err != nil {
-				return err
-			}
-			envRetryWaitMax = &waitMax
-		}
-		if v := os.Getenv(EnvVaultRetryMax); v != "" {
-			retryMax, err := strconv.ParseUint(v, 10, 32)
-			if err != nil {
-				return err
-			}
-			envRetryMax = &retryMax
-		}
-
-		min := c.RetryWaitMin
-		if envRetryWaitMin != nil {
-			min = *envRetryWaitMin
-		}
-		max := c.RetryWaitMax
-		if envRetryWaitMax != nil {
-			max = *envRetryWaitMax
-		}
-		if min > max {
-			return fmt.Errorf("Maximum retry delay is less than minimum retry delay")
 		}
+		envMaxRetries = &maxRetries
 	}
-
 	if v := os.Getenv(EnvVaultCACert); v != "" {
 		envCACert = v
 	}
@@ -208,14 +163,8 @@ func (c *Config) ReadEnvironment() error {
 		c.Address = envAddress
 	}
 
-	if envRetryWaitMin != nil {
+	if envMaxRetries != nil {
-		c.RetryWaitMin = *envRetryWaitMin
+		c.MaxRetries = int(*envMaxRetries)
-	}
-	if envRetryWaitMax != nil {
-		c.RetryWaitMax = *envRetryWaitMax
-	}
-	if envRetryMax != nil {
-		c.RetryMax = int(*envRetryMax)
 	}
 
 	if foundInsecure {
@@ -345,11 +294,9 @@ START:
 		return nil, err
 	}
 
-	client := retryablehttp.NewClient()
+	client := pester.NewExtendedClient(c.config.HttpClient)
-	client.HTTPClient = c.config.HttpClient
+	client.Backoff = pester.LinearJitterBackoff
-	client.RetryWaitMax = c.config.RetryWaitMax
+	client.MaxRetries = c.config.MaxRetries
-	client.RetryWaitMin = c.config.RetryWaitMin
-	client.RetryMax = c.config.RetryMax
 
 	var result *Response
 	resp, err := client.Do(req)

api/client_test.go

@@ -107,25 +107,19 @@ func TestClientEnvSettings(t *testing.T) {
 	oldClientCert := os.Getenv(EnvVaultClientCert)
 	oldClientKey := os.Getenv(EnvVaultClientKey)
 	oldSkipVerify := os.Getenv(EnvVaultInsecure)
-	oldRetryWaitMin := os.Getenv(EnvVaultRetryWaitMin)
+	oldMaxRetries := os.Getenv(EnvVaultMaxRetries)
-	oldRetryWaitMax := os.Getenv(EnvVaultRetryWaitMax)
-	oldRetryMax := os.Getenv(EnvVaultRetryMax)
 	os.Setenv(EnvVaultCACert, cwd+"/test-fixtures/keys/cert.pem")
 	os.Setenv(EnvVaultCAPath, cwd+"/test-fixtures/keys")
 	os.Setenv(EnvVaultClientCert, cwd+"/test-fixtures/keys/cert.pem")
 	os.Setenv(EnvVaultClientKey, cwd+"/test-fixtures/keys/key.pem")
 	os.Setenv(EnvVaultInsecure, "true")
-	os.Setenv(EnvVaultRetryWaitMin, "20s")
+	os.Setenv(EnvVaultMaxRetries, "5")
-	os.Setenv(EnvVaultRetryWaitMax, "25s")
-	os.Setenv(EnvVaultRetryMax, "20")
 	defer os.Setenv(EnvVaultCACert, oldCACert)
 	defer os.Setenv(EnvVaultCAPath, oldCAPath)
 	defer os.Setenv(EnvVaultClientCert, oldClientCert)
 	defer os.Setenv(EnvVaultClientKey, oldClientKey)
 	defer os.Setenv(EnvVaultInsecure, oldSkipVerify)
-	defer os.Setenv(EnvVaultRetryWaitMin, oldRetryWaitMin)
+	defer os.Setenv(EnvVaultMaxRetries, oldMaxRetries)
-	defer os.Setenv(EnvVaultRetryWaitMax, oldRetryWaitMax)
-	defer os.Setenv(EnvVaultRetryMax, oldRetryMax)
 
 	config := DefaultConfig()
 	if err := config.ReadEnvironment(); err != nil {
@@ -142,9 +136,4 @@ func TestClientEnvSettings(t *testing.T) {
 	if tlsConfig.InsecureSkipVerify != true {
 		t.Fatalf("bad: %v", tlsConfig.InsecureSkipVerify)
 	}
-
-	os.Setenv(EnvVaultRetryWaitMax, "15s")
-	if err := config.ReadEnvironment(); err == nil {
-		t.Fatal("expected error due to max retry time being less than min")
-	}
 }

api/request.go

@@ -3,11 +3,9 @@ package api
 import (
 	"bytes"
 	"encoding/json"
-	"fmt"
 	"io"
+	"net/http"
 	"net/url"
-
-	"github.com/hashicorp/go-retryablehttp"
 )
 
 // Request is a raw request configuration structure used to initiate
@@ -45,23 +43,14 @@ func (r *Request) ResetJSONBody() error {
 	return r.SetJSONBody(r.Obj)
 }
 
-// ToHTTP turns this request into a *retryablehttp.Request
+// ToHTTP turns this request into a valid *http.Request for use with the
-func (r *Request) ToHTTP() (*retryablehttp.Request, error) {
+// net/http package.
+func (r *Request) ToHTTP() (*http.Request, error) {
 	// Encode the query parameters
 	r.URL.RawQuery = r.Params.Encode()
 
-	// Create the HTTP request; retryable needs a ReadSeeker
+	// Create the HTTP request
-	body := bytes.NewBuffer(nil)
+	req, err := http.NewRequest(r.Method, r.URL.RequestURI(), r.Body)
-	if r.Body != nil {
-		n, err := body.ReadFrom(r.Body)
-		if err != nil {
-			return nil, err
-		}
-		if n != r.BodySize {
-			return nil, fmt.Errorf("Could not read full body size from Request")
-		}
-	}
-	req, err := retryablehttp.NewRequest(r.Method, r.URL.RequestURI(), bytes.NewReader(body.Bytes()))
 	if err != nil {
 		return nil, err
 	}

builtin/logical/transit/policy.go

@@ -345,7 +345,7 @@ func (p *Policy) Encrypt(context []byte, value string) (string, error) {
 	// Derive the key that should be used
 	key, err := p.DeriveKey(context, p.LatestVersion)
 	if err != nil {
-		return "", certutil.InternalError{Err: err.Error()}
+		return "", err
 	}
 
 	// Guard against a potentially invalid cipher-mode

vendor/github.com/kardianos/govendor/LICENSE

@@ -0,0 +1,27 @@
+Copyright (c) 2015 The Go Authors. All rights reserved.
+
+Redistribution and use in source and binary forms, with or without
+modification, are permitted provided that the following conditions are
+met:
+
+   * Redistributions of source code must retain the above copyright
+notice, this list of conditions and the following disclaimer.
+   * Redistributions in binary form must reproduce the above
+copyright notice, this list of conditions and the following disclaimer
+in the documentation and/or other materials provided with the
+distribution.
+   * Neither the name of Google Inc. nor the names of its
+contributors may be used to endorse or promote products derived from
+this software without specific prior written permission.
+
+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

vendor/github.com/kardianos/govendor/README.md

@@ -0,0 +1,181 @@
+# The Vendor Tool for Go
+`go get -u github.com/kardianos/govendor`
+
+New users please read the [FAQ](doc/faq.md)
+
+Package developers should read the [developer guide](doc/dev-guide.md).
+
+For a high level overview read the [whitepaper](doc/whitepaper.md)
+
+Uses the go1.5+ vendor folder. Multiple workflows supported, single tool.
+
+[![Build Status](https://travis-ci.org/kardianos/govendor.svg?branch=master)](https://travis-ci.org/kardianos/govendor)
+[![GoDoc](https://godoc.org/github.com/kardianos/govendor?status.svg)](https://godoc.org/github.com/kardianos/govendor)
+
+ * Copy existing dependencies from $GOPATH with `govendor add/update`.
+ * If you ignore `vendor/*/`, restore dependencies with `govendor sync`.
+ * Pull in new dependencies or update existing dependencies directly from
+	remotes with `govendor fetch`.
+ * Migrate from legacy systems with `govendor migrate`.
+ * Supports Linux, OS X, Windows, probably all others.
+ * Supports git, hg, svn, bzr (must be installed an on the PATH).
+
+## Notes
+
+ * The project must be within a $GOPATH.
+ * If using go1.5, ensure you `set GO15VENDOREXPERIMENT=1`.
+
+### Quick Start, also see the [FAQ](doc/faq.md)
+```
+# Setup your project.
+cd "my project in GOPATH"
+govendor init
+
+# Add existing GOPATH files to vendor.
+govendor add +external
+
+# View your work.
+govendor list
+
+# Look at what is using a package
+govendor list -v fmt
+
+# Specify a specific version or revision to fetch
+govendor fetch golang.org/x/net/context@a4bbce9fcae005b22ae5443f6af064d80a6f5a55
+govendor fetch golang.org/x/net/context@v1   # Get latest v1.*.* tag or branch.
+govendor fetch golang.org/x/net/context@=v1  # Get the tag or branch named "v1".
+
+# Update a package to latest, given any prior version constraint
+govendor fetch golang.org/x/net/context
+
+# Format your repository only
+govendor fmt +local
+
+# Build everything in your repository only
+govendor install +local
+
+# Test your repository only
+govendor test +local
+
+```
+
+## Sub-commands
+```
+	init     Create the "vendor" folder and the "vendor.json" file.
+	list     List and filter existing dependencies and packages.
+	add      Add packages from $GOPATH.
+	update   Update packages from $GOPATH.
+	remove   Remove packages from the vendor folder.
+	status   Lists any packages missing, out-of-date, or modified locally.
+	fetch    Add new or update vendor folder packages from remote repository.
+	sync     Pull packages into vendor folder from remote repository with revisions
+  	             from vendor.json file.
+	migrate  Move packages from a legacy tool to the vendor folder with metadata.
+	get      Like "go get" but copies dependencies into a "vendor" folder.
+	license  List discovered licenses for the given status or import paths.
+	shell    Run a "shell" to make multiple sub-commands more efficient for large
+	             projects.
+
+	go tool commands that are wrapped:
+	  `+<status>` package selection may be used with them
+	fmt, build, install, clean, test, vet, generate
+```
+
+## Status
+
+Packages can be specified by their "status".
+```
+	+local    (l) packages in your project
+	+external (e) referenced packages in GOPATH but not in current project
+	+vendor   (v) packages in the vendor folder
+	+std      (s) packages in the standard library
+
+	+excluded (x) external packages explicitely excluded from vendoring
+	+unused   (u) packages in the vendor folder, but unused
+	+missing  (m) referenced packages but not found
+
+	+program  (p) package is a main package
+
+	+outside  +external +missing
+	+all      +all packages
+```
+
+Status can be referenced by their initial letters.
+
+ * `+std` same as `+s`
+ * `+external` same as `+ext` same as `+e`
+ * `+excluded` same as `+exc` same as `+x`
+
+Status can be logically composed:
+
+ * `+local,program` (local AND program) local packages that are also programs
+ * `+local +vendor` (local OR vendor) local packages or vendor packages
+ * `+vendor,program +std` ((vendor AND program) OR std) vendor packages that are also programs
+	or std library packages
+ * `+vendor,^program` (vendor AND NOT program) vendor package that are not "main" packages.
+
+## Package specifier
+
+The full package-spec is:
+`<path>[::<origin>][{/...|/^}][@[<version-spec>]]`
+
+Some examples:
+
+ * `github.com/kardianos/govendor` specifies a single package and single folder.
+ * `github.com/kardianos/govendor/...` specifies `govendor` and all referenced
+	packages under that path.
+ * `github.com/kardianos/govendor/^` specifies the `govendor` folder and all
+	sub-folders. Useful for resources or if you don't want a partial repository.
+ * `github.com/kardianos/govendor/^::github.com/myself/govendor` same as above
+	but fetch from user "myself".
+ * `github.com/kardianos/govendor/...@abc12032` all referenced packages at
+	revision `abc12032`.
+ * `github.com/kardianos/govendor/...@v1` same as above, but get the most recent
+	"v1" tag, such as "v1.4.3".
+ * `github.com/kardianos/govendor/...@=v1` get the exact version "v1".
+
+## Packages and Status
+
+You may specify multiple package-specs and multiple status in a single command.
+Commands that accept status and package-spec:
+
+ * list
+ * add
+ * update
+ * remove
+ * fetch
+
+You may pass arguments to govendor through stdin if the last argument is a "-".
+For example `echo +vendor | govendor list -` will list all vendor packages.
+
+## Ignoring build tags and excluding packages
+Ignoring build tags is opt-out and is designed to be the opposite of the build
+file directives which are opt-in when specified. Typically a developer will
+want to support cross platform builds, but selectively opt out of tags, tests,
+and architectures as desired.
+
+To ignore additional tags edit the "vendor.json" file and add tag to the vendor
+"ignore" file field. The field uses spaces to separate tags to ignore.
+For example the following will ignore both test and appengine files.
+```
+{
+	"ignore": "test appengine",
+}
+```
+
+Similarly, some specific packages can be excluded from the vendoring process.
+These packages will be listed as `excluded` (`x`), and will not be copied to the
+"vendor" folder when running `govendor add|fetch|update`.
+
+Any sub-package `foo/bar` of an excluded package `foo` is also excluded (but
+package `bar/foo` is not). The import dependencies of excluded packages are not
+listed, and thus not vendored.
+
+To exclude packages, also use the "ignore" field of the "vendor.json" file.
+Packages are identified by their name, they should contain a "/" character
+(possibly at the end):
+```
+{
+	"ignore": "test appengine foo/",
+}
+```

vendor/github.com/kardianos/govendor/main.go

@@ -0,0 +1,52 @@
+// Copyright 2015 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// vendor tool to copy external source code from GOPATH or remote location to the
+// local vendor folder. See README.md for usage.
+package main
+
+import (
+	"bytes"
+	"flag"
+	"fmt"
+	"io"
+	"os"
+	"strings"
+
+	"github.com/kardianos/govendor/cliprompt"
+	"github.com/kardianos/govendor/help"
+	"github.com/kardianos/govendor/run"
+)
+
+func main() {
+	prompt := &cliprompt.Prompt{}
+
+	allArgs := os.Args
+
+	if allArgs[len(allArgs)-1] == "-" {
+		stdin := &bytes.Buffer{}
+		if _, err := io.Copy(stdin, os.Stdin); err == nil {
+			stdinArgs := strings.Fields(stdin.String())
+			allArgs = append(allArgs[:len(allArgs)-1], stdinArgs...)
+		}
+	}
+
+	msg, err := run.Run(os.Stdout, allArgs, prompt)
+	if err == flag.ErrHelp {
+		err = nil
+	}
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "Error: %v\n", err)
+	}
+	msgText := msg.String()
+	if len(msgText) > 0 {
+		fmt.Fprint(os.Stderr, msgText)
+	}
+	if err != nil {
+		os.Exit(2)
+	}
+	if msg != help.MsgNone {
+		os.Exit(1)
+	}
+}

vendor/vendor.json

@@ -574,6 +574,12 @@
 			"revision": "0b12d6b521d83fc7f755e7cfc1b1fbdd35a01a74",
 			"revisionTime": "2016-02-02T18:50:14Z"
 		},
+		{
+			"checksumSHA1": "bQfc4zGh8WUri465COdQNhJJTw4=",
+			"path": "github.com/kardianos/govendor",
+			"revision": "7a0d30eab9e67b3ff60f13cdc483f003c01e04c1",
+			"revisionTime": "2016-07-09T17:43:04Z"
+		},
 		{
 			"checksumSHA1": "QK3MNUdQwUBuznCHZcPijU/3DyI=",
 			"path": "github.com/lib/pq",

website/source/docs/commands/environment.html.md

@@ -47,16 +47,8 @@ The following table describes them:
     <td>Path to an unencrypted PEM-encoded private key matching the client certificate.</td>
   </tr>
   <tr>
-    <td><tt>VAULT_RETRY_MAX</tt></td>
+    <td><tt>VAULT_MAX_RETRIES</tt></td>
-    <td>The maximum number of retries when a `5xx` error code is encountered. Default is `15`; set to `0` to disable retrying.</td>
+    <td>The maximum number of retries when a `5xx` error code is encountered. Default is `3`; set to `1` or less to disable retrying.</td>
-  </tr>
-  <tr>
-    <td><tt>VAULT_RETRY_WAIT_MIN</tt></td>
-    <td>The minimum amount of time to wait between retries when a `5xx` error code is encountered. Default is `1s`.</td>
-  </tr>
-  <tr>
-    <td><tt>VAULT_RETRY_WAIT_MAX</tt></td>
-    <td>The maximum amount of time to wait between retries when a `5xx` error code is encountered. Default is `30s`.</td>
   </tr>
   <tr>
     <td><tt>VAULT_SKIP_VERIFY</tt></td>