Switch to avast/retry-go for HTTP retry

go.mod

@@ -56,12 +56,11 @@ require (
 	cloud.google.com/go/pubsub v1.45.3
 	github.com/AdamKorcz/go-fuzz-headers-1 v0.0.0-20230919221257-8b5d3ce2d11d
 	github.com/DATA-DOG/go-sqlmock v1.5.2
+	github.com/avast/retry-go/v4 v4.6.0
 	github.com/cyberphone/json-canonicalization v0.0.0-20220623050100-57a0ce2678a7
 	github.com/go-redis/redismock/v9 v9.2.0
 	github.com/go-sql-driver/mysql v1.8.1
 	github.com/golang/mock v1.6.0
-	github.com/hashicorp/go-cleanhttp v0.5.2
-	github.com/hashicorp/go-retryablehttp v0.7.7
 	github.com/jmoiron/sqlx v1.4.0
 	github.com/redis/go-redis/v9 v9.7.0
 	github.com/sassoftware/relic/v7 v7.6.2
@@ -132,7 +131,9 @@ require (
 	github.com/google/pprof v0.0.0-20240727154555-813a5fbdbec8 // indirect
 	github.com/google/s2a-go v0.1.9 // indirect
 	github.com/hashicorp/errwrap v1.1.0 // indirect
+	github.com/hashicorp/go-cleanhttp v0.5.2 // indirect
 	github.com/hashicorp/go-multierror v1.1.1 // indirect
+	github.com/hashicorp/go-retryablehttp v0.7.7 // indirect
 	github.com/hashicorp/go-rootcerts v1.0.2 // indirect
 	github.com/hashicorp/go-secure-stdlib/parseutil v0.1.7 // indirect
 	github.com/hashicorp/go-secure-stdlib/strutil v0.1.2 // indirect

go.sum

@@ -64,6 +64,8 @@ github.com/alessio/shellescape v1.4.1/go.mod h1:PZAiSCk0LJaZkiCSkPv8qIobYglO3FPp
 github.com/armon/go-radix v0.0.0-20180808171621-7fddfc383310/go.mod h1:ufUuZ+zHj4x4TnLV4JWEpy2hxWSpsRywHrMgIH9cCH8=
 github.com/asaskevich/govalidator v0.0.0-20230301143203-a9d515a09cc2 h1:DklsrG3dyBCFEj5IhUbnKptjxatkF07cF2ak3yi77so=
 github.com/asaskevich/govalidator v0.0.0-20230301143203-a9d515a09cc2/go.mod h1:WaHUgvxTVq04UNunO+XhnAqY/wQc+bxr74GqbsZ/Jqw=
+github.com/avast/retry-go/v4 v4.6.0 h1:K9xNA+KeB8HHc2aWFuLb25Offp+0iVRXEvFx8IinRJA=
+github.com/avast/retry-go/v4 v4.6.0/go.mod h1:gvWlPhBVsvBbLkVGDg/KwvBv0bEkCOLRRSHKIr2PyOE=
 github.com/aws/aws-sdk-go v1.55.5 h1:KKUZBfBoyqy5d3swXyiC7Q76ic40rYcbqH7qjh59kzU=
 github.com/aws/aws-sdk-go v1.55.5/go.mod h1:eRwEWoyTWFMVYVQzKMNHWP5/RV4xIUGMQfXQHfHkpNU=
 github.com/aws/aws-sdk-go-v2 v1.32.8 h1:cZV+NUS/eGxKXMtmyhtYPJ7Z4YLoI/V8bkTdRZfYhGo=

pkg/client/options.go

@@ -15,10 +15,16 @@
 package client
 
 import (
+	"crypto/tls"
+	"errors"
+	"fmt"
 	"net/http"
+	"net/url"
+	"regexp"
+	"strings"
 	"time"
 
-	"github.com/hashicorp/go-retryablehttp"
+	"github.com/avast/retry-go/v4"
 )
 
 // Option is a functional option for customizing static signatures.
@@ -30,7 +36,6 @@ type options struct {
 	RetryWaitMin        time.Duration
 	RetryWaitMax        time.Duration
 	InsecureTLS         bool
-	Logger              interface{}
 	NoDisableKeepalives bool
 	Headers             map[string][]string
 }
@@ -81,14 +86,9 @@ func WithRetryWaitMax(t time.Duration) Option {
 	}
 }
 
-// WithLogger sets the logger; it must implement either retryablehttp.Logger or retryablehttp.LeveledLogger; if not, this will not take effect.
-func WithLogger(logger interface{}) Option {
-	return func(o *options) {
-		switch logger.(type) {
-		case retryablehttp.Logger, retryablehttp.LeveledLogger:
-			o.Logger = logger
-		}
-	}
+// WithLogger sets the logger; this method is deprecated and will not take any effect.
+func WithLogger(interface{}) Option {
+	return func(*options) {}
 }
 
 // WithInsecureTLS disables TLS verification.
@@ -113,33 +113,75 @@ func WithHeaders(h map[string][]string) Option {
 }
 
 type roundTripper struct {
-	http.RoundTripper
-	UserAgent string
-	Headers   map[string][]string
+	inner http.RoundTripper
+	*options
 }
 
 // RoundTrip implements `http.RoundTripper`
-func (rt *roundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
-	req.Header.Set("User-Agent", rt.UserAgent)
-	for k, v := range rt.Headers {
+func (rt *roundTripper) RoundTrip(req *http.Request) (res *http.Response, err error) {
+	req.Header.Set("User-Agent", rt.options.UserAgent)
+	for k, v := range rt.options.Headers {
 		for _, h := range v {
 			req.Header.Add(k, h)
 		}
 	}
-	return rt.RoundTripper.RoundTrip(req)
+
+	err = retry.Do(func() (err error) {
+		res, err = rt.inner.RoundTrip(req)
+		if retryErr := shouldRetry(res, err); retryErr != nil {
+			return retryErr
+		}
+		return nil
+	},
+		retry.Attempts(rt.options.RetryCount),
+		retry.Delay(rt.options.RetryWaitMin),
+		retry.MaxDelay(rt.options.RetryWaitMax),
+	)
+
+	return res, err
 }
 
-func createRoundTripper(inner http.RoundTripper, o *options) http.RoundTripper {
+var tooManyRedirectyRe = regexp.MustCompile(`stopped after \d+ redirects\z`)
+
+func shouldRetry(resp *http.Response, err error) error {
+	if err != nil {
+		urlErr := &url.Error{}
+
+		// Filter well known URL errors
+		if errors.As(err, &urlErr) {
+			certVerificationErr := &tls.CertificateVerificationError{}
+
+			if tooManyRedirectyRe.MatchString(urlErr.Error()) ||
+				strings.Contains(urlErr.Error(), "unsupported protocol scheme") ||
+				strings.Contains(urlErr.Error(), "invalid header") ||
+				strings.Contains(urlErr.Error(), "certificate is not trusted") ||
+				errors.As(urlErr.Err, &certVerificationErr) {
+				return nil
+			}
+		}
+
+		// Retry any other errror
+		return err
+	}
+
+	if resp.StatusCode == http.StatusTooManyRequests {
+		return fmt.Errorf("retry %d: %s", resp.StatusCode, resp.Status)
+	}
+
+	if resp.StatusCode == 0 || (resp.StatusCode >= 500 &&
+		resp.StatusCode != http.StatusNotImplemented) {
+		return fmt.Errorf("retry unexpected HTTP status %d: %s", resp.StatusCode, resp.Status)
+	}
+
+	return nil
+}
+
+func wrapRoundTripper(inner http.RoundTripper, o *options) http.RoundTripper {
 	if inner == nil {
 		inner = http.DefaultTransport
 	}
-	if o.UserAgent == "" && o.Headers == nil {
-		// There's nothing to do...
-		return inner
-	}
 	return &roundTripper{
-		RoundTripper: inner,
-		UserAgent:    o.UserAgent,
-		Headers:      o.Headers,
+		inner:   inner,
+		options: o,
 	}
 }

pkg/client/options_test.go

@@ -15,17 +15,14 @@
 package client
 
 import (
-	"log"
 	"net/http"
-	"os"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"go.uber.org/zap"
 )
 
 func TestMakeOptions(t *testing.T) {
-	customLogger := log.New(os.Stdout, "", log.LstdFlags)
-
 	tests := []struct {
 		desc string
 
@@ -42,10 +39,6 @@ func TestMakeOptions(t *testing.T) {
 		desc: "WithRetryCount",
 		opts: []Option{WithRetryCount(2)},
 		want: &options{UserAgent: "", RetryCount: 2},
-	}, {
-		desc: "WithLogger",
-		opts: []Option{WithLogger(customLogger)},
-		want: &options{UserAgent: "", RetryCount: DefaultRetryCount, Logger: customLogger},
 	}, {
 		desc: "WithLoggerNil",
 		opts: []Option{WithLogger(nil)},
@@ -62,7 +55,9 @@ func TestMakeOptions(t *testing.T) {
 	for _, tc := range tests {
 		t.Run(tc.desc, func(t *testing.T) {
 			got := makeOptions(tc.opts...)
-			if d := cmp.Diff(tc.want, got, cmp.Comparer(func(a, b *log.Logger) bool { return a == b })); d != "" {
+			if d := cmp.Diff(tc.want, got, cmp.Comparer(func(a, b zap.SugaredLogger) bool {
+				return a == b
+			})); d != "" {
 				t.Errorf("makeOptions(%v) returned unexpected result (-want +got): %s", tc.desc, d)
 			}
 		})
@@ -82,11 +77,11 @@ func (m *mockRoundTripper) RoundTrip(req *http.Request) (*http.Response, error)
 	return m.resp, m.err
 }
 
-func TestCreateRoundTripper(t *testing.T) {
+func TestWrapRoundTripper(t *testing.T) {
 	t.Run("always returns non-nil", func(t *testing.T) {
-		got := createRoundTripper(nil, &options{})
+		got := wrapRoundTripper(nil, &options{})
 		if got == nil {
-			t.Errorf("createRoundTripper() should never return a nil `http.RoundTripper`")
+			t.Errorf("wrapRoundTripper() should never return a nil `http.RoundTripper`")
 		}
 	})
 
@@ -104,7 +99,7 @@ func TestCreateRoundTripper(t *testing.T) {
 	expectedUserAgent := "test UserAgent"
 
 	m := &mockRoundTripper{}
-	rt := createRoundTripper(m, &options{
+	rt := wrapRoundTripper(m, &options{
 		UserAgent: expectedUserAgent,
 	})
 	m.resp = testResp

pkg/client/rekor_client.go

@@ -16,15 +16,15 @@ package client
 
 import (
 	"crypto/tls"
+	"net"
 	"net/http"
 	"net/url"
+	"time"
 
 	"github.com/go-openapi/runtime"
 	httptransport "github.com/go-openapi/runtime/client"
 	"github.com/go-openapi/strfmt"
 
-	"github.com/hashicorp/go-cleanhttp"
-	retryablehttp "github.com/hashicorp/go-retryablehttp"
 	"github.com/sigstore/rekor/pkg/generated/client"
 	"github.com/sigstore/rekor/pkg/util"
 )
@@ -36,25 +36,26 @@ func GetRekorClient(rekorServerURL string, opts ...Option) (*client.Rekor, error
 	}
 	o := makeOptions(opts...)
 
-	retryableClient := retryablehttp.NewClient()
-	defaultTransport := cleanhttp.DefaultTransport()
-	if o.NoDisableKeepalives {
-		defaultTransport.DisableKeepAlives = false
+	transport := &http.Transport{
+		Proxy: http.ProxyFromEnvironment,
+		DialContext: (&net.Dialer{
+			Timeout:   30 * time.Second,
+			KeepAlive: 30 * time.Second,
+			DualStack: true,
+		}).DialContext,
+		MaxIdleConns:          100,
+		IdleConnTimeout:       90 * time.Second,
+		TLSHandshakeTimeout:   10 * time.Second,
+		ExpectContinueTimeout: 1 * time.Second,
+		ForceAttemptHTTP2:     true,
+		MaxIdleConnsPerHost:   -1,
+		DisableKeepAlives:     !o.NoDisableKeepalives,
+		TLSClientConfig:       &tls.Config{InsecureSkipVerify: o.InsecureTLS}, //nolint:gosec
 	}
-	if o.InsecureTLS {
-		/* #nosec G402 */
-		defaultTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
-	}
-	retryableClient.HTTPClient = &http.Client{
-		Transport: defaultTransport,
-	}
-	retryableClient.RetryMax = int(o.RetryCount)
-	retryableClient.RetryWaitMin = o.RetryWaitMin
-	retryableClient.RetryWaitMax = o.RetryWaitMax
-	retryableClient.Logger = o.Logger
 
-	httpClient := retryableClient.StandardClient()
-	httpClient.Transport = createRoundTripper(httpClient.Transport, o)
+	httpClient := &http.Client{
+		Transport: wrapRoundTripper(transport, o),
+	}
 
 	// sanitize path
 	if url.Path == "" {