use go-retry to do more than one retry with configurable strategy

Signed-off-by: Sandor Szücs <[email protected]>

go.mod

@@ -109,6 +109,7 @@ require (
 	github.com/prometheus/common v0.42.0 // indirect
 	github.com/prometheus/procfs v0.9.0 // indirect
 	github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529 // indirect
+	github.com/sethvargo/go-retry v0.2.4 // indirect
 	github.com/shirou/gopsutil/v3 v3.21.2 // indirect
 	github.com/streadway/quantile v0.0.0-20220407130108-4246515d968d // indirect
 	github.com/tidwall/match v1.1.1 // indirect

go.sum

@@ -351,6 +351,8 @@ github.com/sarslanhan/cronmask v0.0.0-20190709075623-766eca24d011/go.mod h1:NmI1
 github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529 h1:nn5Wsu0esKSJiIVhscUtVbo7ada43DJhG55ua/hjS5I=
 github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529/go.mod h1:DxrIzT+xaE7yg65j358z/aeFdxmN0P9QXhEzd20vsDc=
 github.com/seccomp/libseccomp-golang v0.9.2-0.20220502022130-f33da4d89646/go.mod h1:JA8cRccbGaA1s33RQf7Y1+q9gHmZX1yB/z9WDN1C6fg=
+github.com/sethvargo/go-retry v0.2.4 h1:T+jHEQy/zKJf5s95UkguisicE0zuF9y7+/vgz08Ocec=
+github.com/sethvargo/go-retry v0.2.4/go.mod h1:1afjQuvh7s4gflMObvjLPaWgluLLyhA1wmVZ6KLpICw=
 github.com/shirou/gopsutil/v3 v3.21.2 h1:fIOk3hyqV1oGKogfGNjUZa0lUbtlkx3+ZT0IoJth2uM=
 github.com/shirou/gopsutil/v3 v3.21.2/go.mod h1:ghfMypLDrFSWN2c9cDYFLHyynQ+QUht0cv/18ZqVczw=
 github.com/shurcooL/sanitized_anchor_name v1.0.0/go.mod h1:1NzhyTcUVG4SuEtjjoZeVRXNmyL/1OwPU0+IJeTBvfc=

net/httpclient.go

@@ -15,15 +15,39 @@ import (
 
 	"github.com/opentracing/opentracing-go"
 	"github.com/opentracing/opentracing-go/ext"
+	"github.com/sirupsen/logrus"
 	"github.com/zalando/skipper/logging"
 	"github.com/zalando/skipper/secrets"
+
+	goretry "github.com/sethvargo/go-retry"
 )
 
 const (
 	defaultIdleConnTimeout = 30 * time.Second
 	defaultRefreshInterval = 5 * time.Minute
 )
 
+type retryTyp int
+
+const (
+	constant retryTyp = iota + 1
+	exponential
+)
+
+type RetryConfig struct {
+	typ retryTyp
+	// Duration mandatory configuration how long to wait before retry
+	Duration time.Duration
+	// Max is an optional configuration maximum time.Duration to spent for retry
+	Max time.Duration
+	// Jitter is an optional configuration time.Duration to calculate jitter for a retry
+	Jitter time.Duration
+	// Cap is an optional configuration maximum time.Duration to spent for a single retry
+	Cap time.Duration
+	// Attempts
+	Attempts int
+}
+
 // Client adds additional features like Bearer token injection, and
 // opentracing to the wrapped http.Client with the same interface as
 // http.Client from the stdlib.
@@ -33,7 +57,7 @@ type Client struct {
 	tr     *Transport
 	log    logging.Logger
 	sr     secrets.SecretsReader
-	retry  bool
+	retry  goretry.Backoff
 }
 
 // NewClient creates a wrapped http.Client and uses Transport to
@@ -63,6 +87,22 @@ func NewClient(o Options) *Client {
 		sr = secrets.NewStaticDelegateSecret(sp, o.BearerTokenFile)
 	}
 
+	// type RetryFunc func(ctx context.Context) error
+	var retry goretry.Backoff
+	if o.RetryConfig != nil {
+		cfg := o.RetryConfig
+		switch cfg.typ {
+		case constant:
+			retry = goretry.NewConstant(cfg.Duration)
+			retry = goretry.WithMaxRetries(uint64(cfg.Attempts), retry)
+		case exponential:
+			retry = goretry.NewExponential(cfg.Duration)
+			retry = goretry.WithMaxRetries(uint64(cfg.Attempts), retry)
+			retry = goretry.WithJitter(cfg.Jitter, retry)
+			retry = goretry.WithCappedDuration(cfg.Cap, retry)
+			retry = goretry.WithMaxDuration(cfg.Max, retry)
+		}
+	}
 	c := &Client{
 		once: sync.Once{},
 		client: http.Client{
@@ -72,7 +112,7 @@ func NewClient(o Options) *Client {
 		tr:    tr,
 		log:   o.Log,
 		sr:    sr,
-		retry: o.Retry,
+		retry: retry,
 	}
 
 	return c
@@ -84,7 +124,6 @@ func (c *Client) Close() {
 		if c.sr != nil {
 			c.sr.Close()
 		}
-		c.client.CloseIdleConnections()
 	})
 }
 
@@ -130,21 +169,42 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) {
 		}
 	}
 
-	rsp, err := c.client.Do(req)
+	var (
+		rsp *http.Response
+		err error
+	)
 
-	if (rsp != nil && rsp.StatusCode >= 500 || err != nil) && c.retry {
-		c.retry = false
-		if req.Body != nil {
-			body, err := req.GetBody()
+	if c.retry == nil {
+		rsp, err = c.client.Do(req)
+	} else {
+		ctx := req.Context()
+		if e := goretry.Do(ctx, c.retry, func(ctx context.Context) error {
+			select {
+			case <-ctx.Done():
+				return ctx.Err()
+			default:
+			}
 			if err != nil {
-				return rsp, err
+				if req.Body != nil {
+					body, err := req.GetBody()
+					if err != nil {
+						return fmt.Errorf("failed to get body, but request had a non-nil body: %w", err)
+					}
+					req2 := req.Clone(ctx)
+					req2.Body = body
+					req = req2
+				}
 			}
-			req2 := req.Clone(context.Background())
-			req2.Body = body
-			return c.client.Do(req2)
-		} else {
-			return c.client.Do(req.Clone(req.Context()))
+			rsp, err = c.client.Do(req.Clone(req.Context()))
+			if err != nil || rsp.StatusCode >= 500 {
+				return goretry.RetryableError(fmt.Errorf("Do request error: %w", err))
+			}
+
+			return nil
+		}); e != nil {
+			logrus.Errorf("Failed to retry: %v", e)
 		}
+
 	}
 
 	return rsp, err
@@ -230,8 +290,8 @@ type Options struct {
 	// Log is used for error logging
 	Log logging.Logger
 
-	// Retry
-	Retry bool
+	// RetryConfig configures the retry
+	RetryConfig *RetryConfig
 }
 
 // Transport wraps an http.Transport and adds support for tracing and

net/httpclient_test.go

@@ -307,6 +307,7 @@ func TestRetry(t *testing.T) {
 	s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		buf := bytes.NewBuffer(make([]byte, 0, N))
 		if r.Method == "POST" {
+			println("HERE")
 			n, err := io.Copy(buf, r.Body)
 			if err != nil {
 				t.Fatalf("Failed to get POST with body. counter=%d: %v", counter, err)
@@ -315,11 +316,14 @@ func TestRetry(t *testing.T) {
 				t.Fatalf("Server-Failed to get the full body want %d, got %d", N, n)
 			}
 		}
-		if counter%2 == 0 {
-			t.Logf("send 500")
+		t.Logf("counter: %d", counter)
+		MAX := 1 // 1 ok / 2 fails
+		switch counter % (MAX + 1) {
+		default:
+			t.Logf("send 500: %d", counter)
 			w.WriteHeader(http.StatusInternalServerError)
 			w.Write([]byte(http.StatusText(http.StatusInternalServerError)))
-		} else {
+		case MAX:
 			t.Logf("send 200")
 			w.WriteHeader(http.StatusOK)
 			w.Write(buf.Bytes())
@@ -328,22 +332,25 @@ func TestRetry(t *testing.T) {
 	}))
 	defer s.Close()
 
-	options := Options{
-		Retry:                 true,
-		ResponseHeaderTimeout: 500 * time.Millisecond,
-	}
 	target := fmt.Sprintf("http://%s/", s.Listener.Addr())
 	req, err := http.NewRequest("POST", target, strings.NewReader(strings.Repeat("A", N)))
 	if err != nil {
 		t.Fatalf("Failed to create request: %v", err)
 	}
 
-	tr := NewTransport(options)
-	defer tr.Close()
-	options.Transport = tr.tr
-	defer tr.tr.CloseIdleConnections()
-
-	cli := NewClient(options)
+	cli := NewClient(Options{
+		RetryConfig: &RetryConfig{
+			typ:      exponential,
+			Duration: 250 * time.Millisecond,
+			Attempts: 3,
+			Jitter:   5 * time.Millisecond,
+			Cap:      time.Second,
+			Max:      5 * time.Second,
+		},
+		Transport: &http.Transport{
+			ResponseHeaderTimeout: 4500 * time.Millisecond,
+		},
+	})
 	defer cli.Close()
 
 	rsp, err := cli.Do(req)