Skip to content

Switch to avast/retry-go for HTTP retry #2350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Switch to avast/retry-go for HTTP retry #2350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like these are still indirects (probably because of us supporting hashivault)... is that still an issue for CNCF?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, but the code does not seem to be imported from kubernetes-sigs/release-sdk, we may follow-up on that, though.

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
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
90 changes: 65 additions & 25 deletions pkg/client/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -30,7 +36,6 @@ type options struct {
RetryWaitMin time.Duration
RetryWaitMax time.Duration
InsecureTLS bool
Logger interface{}
NoDisableKeepalives bool
Headers map[string][]string
}
Expand Down Expand Up @@ -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.
Expand All @@ -113,33 +113,73 @@ 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)
return shouldRetry(res, err)
},
retry.Attempts(rt.options.RetryCount),
retry.Delay(rt.options.RetryWaitMin),
retry.MaxDelay(rt.options.RetryWaitMax),
retry.DelayType(retry.BackOffDelay),
)

return res, err
}

var tooManyRedirectyRe = regexp.MustCompile(`stopped after \d+ redirects\z`)

func shouldRetry(resp *http.Response, err error) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a change in behavior or is this copied from go-retryable's implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not copied over, but it should reflect the same retry behavior.

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 createRoundTripper(inner http.RoundTripper, o *options) http.RoundTripper {
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,
}
}
21 changes: 8 additions & 13 deletions pkg/client/options_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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)},
Expand All @@ -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)
}
})
Expand All @@ -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`")
}
})

Expand All @@ -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
Expand Down
37 changes: 20 additions & 17 deletions pkg/client/rekor_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -36,25 +36,28 @@ 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,
}
if o.InsecureTLS {
/* #nosec G402 */
defaultTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: o.InsecureTLS} //nolint:gosec
}
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 == "" {
Expand Down