Add retries to kibana client (#1563)

Adding it by now only to kibana client because for elasticsearch client
we rely on the Go SDK, that has its own retry mechanisms.

As it is implemented, we can later consider using it in more places
where an http client is used now.

Author: jsoriano

go.mod

@@ -24,6 +24,7 @@ require (
 	github.com/google/go-github/v32 v32.1.0
 	github.com/google/go-querystring v1.1.0
 	github.com/google/uuid v1.4.0
+	github.com/hashicorp/go-retryablehttp v0.7.5
 	github.com/jedib0t/go-pretty v4.3.0+incompatible
 	github.com/magefile/mage v1.15.0
 	github.com/mholt/archiver/v3 v3.5.1
@@ -93,6 +94,7 @@ require (
 	github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirect
 	github.com/gregjones/httpcache v0.0.0-20190611155906-901d90724c79 // 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/huandu/xstrings v1.4.0 // indirect
 	github.com/imdario/mergo v0.3.16 // indirect

go.sum

@@ -301,8 +301,14 @@ github.com/gregjones/httpcache v0.0.0-20190611155906-901d90724c79/go.mod h1:Fecb
 github.com/hashicorp/errwrap v1.0.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4=
 github.com/hashicorp/errwrap v1.1.0 h1:OxrOeh75EUXMY8TBjag2fzXGZ40LB6IKw45YeGUDY2I=
 github.com/hashicorp/errwrap v1.1.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4=
+github.com/hashicorp/go-cleanhttp v0.5.2 h1:035FKYIWjmULyFRBKPs8TBQoi0x6d9G4xc9neXJWAZQ=
+github.com/hashicorp/go-cleanhttp v0.5.2/go.mod h1:kO/YDlP8L1346E6Sodw+PrpBSV4/SoxCXGY6BqNFT48=
+github.com/hashicorp/go-hclog v0.9.2 h1:CG6TE5H9/JXsFWJCfoIVpKFIkFe6ysEuHirp4DxCsHI=
+github.com/hashicorp/go-hclog v0.9.2/go.mod h1:5CU+agLiy3J7N7QjHK5d05KxGsuXiQLrjA0H7acj2lQ=
 github.com/hashicorp/go-multierror v1.1.1 h1:H5DkEtf6CXdFp0N0Em5UCwQpXMWke8IA0+lD48awMYo=
 github.com/hashicorp/go-multierror v1.1.1/go.mod h1:iw975J/qwKPdAO1clOe2L8331t/9/fmwbPZ6JB6eMoM=
+github.com/hashicorp/go-retryablehttp v0.7.5 h1:bJj+Pj19UZMIweq/iie+1u5YCdGrnxCT9yvm0e+Nd5M=
+github.com/hashicorp/go-retryablehttp v0.7.5/go.mod h1:Jy/gPYAdjqffZ/yFGCFV2doI5wjtH1ewM9u8iYVjtX8=
 github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8=
 github.com/hashicorp/golang-lru v0.5.1/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8=
 github.com/hinshun/vt10x v0.0.0-20220119200601-820417d04eec h1:qv2VnGeEQHchGaZ/u7lxST/RaJw+cv273q79D81Xbog=

internal/kibana/client.go

@@ -18,6 +18,7 @@ import (
 	"github.com/elastic/elastic-package/internal/certs"
 	"github.com/elastic/elastic-package/internal/install"
 	"github.com/elastic/elastic-package/internal/logger"
+	"github.com/elastic/elastic-package/internal/retry"
 )
 
 var ErrUndefinedHost = errors.New("missing kibana host")
@@ -33,14 +34,19 @@ type Client struct {
 
 	versionInfo VersionInfo
 	semver      *semver.Version
+
+	retryMax int
+	http     *http.Client
 }
 
 // ClientOption is functional option modifying Kibana client.
 type ClientOption func(*Client)
 
 // NewClient creates a new instance of the client.
 func NewClient(opts ...ClientOption) (*Client, error) {
-	c := &Client{}
+	c := &Client{
+		retryMax: 10,
+	}
 	for _, opt := range opts {
 		opt(c)
 	}
@@ -49,6 +55,12 @@ func NewClient(opts ...ClientOption) (*Client, error) {
 		return nil, ErrUndefinedHost
 	}
 
+	httpClient, err := c.newHttpClient()
+	if err != nil {
+		return nil, err
+	}
+	c.http = httpClient
+
 	// Allow to initialize version from tests.
 	var zeroVersion VersionInfo
 	if c.semver == nil || c.versionInfo == zeroVersion {
@@ -95,6 +107,13 @@ func Password(password string) ClientOption {
 	}
 }
 
+// RetryMax configures the number of retries before failing.
+func RetryMax(retryMax int) ClientOption {
+	return func(c *Client) {
+		c.retryMax = retryMax
+	}
+}
+
 // CertificateAuthority sets the certificate authority to be used by the client.
 func CertificateAuthority(certificateAuthority string) ClientOption {
 	return func(c *Client) {
@@ -156,31 +175,42 @@ func (c *Client) newRequest(method, resourcePath string, reqBody io.Reader) (*ht
 }
 
 func (c *Client) doRequest(request *http.Request) (int, []byte, error) {
-	client := http.Client{}
+	resp, err := c.http.Do(request)
+	if err != nil {
+		return 0, nil, fmt.Errorf("could not send request to Kibana API: %w", err)
+	}
+
+	defer resp.Body.Close()
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return resp.StatusCode, nil, fmt.Errorf("could not read response body: %w", err)
+	}
+
+	return resp.StatusCode, body, nil
+}
+
+func (c *Client) newHttpClient() (*http.Client, error) {
+	client := &http.Client{}
 	if c.tlSkipVerify {
 		client.Transport = &http.Transport{
 			TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
 		}
 	} else if c.certificateAuthority != "" {
 		rootCAs, err := certs.SystemPoolWithCACertificate(c.certificateAuthority)
 		if err != nil {
-			return 0, nil, fmt.Errorf("reading CA certificate: %w", err)
+			return nil, fmt.Errorf("reading CA certificate: %w", err)
 		}
 		client.Transport = &http.Transport{
 			TLSClientConfig: &tls.Config{RootCAs: rootCAs},
 		}
 	}
 
-	resp, err := client.Do(request)
-	if err != nil {
-		return 0, nil, fmt.Errorf("could not send request to Kibana API: %w", err)
-	}
-
-	defer resp.Body.Close()
-	body, err := io.ReadAll(resp.Body)
-	if err != nil {
-		return resp.StatusCode, nil, fmt.Errorf("could not read response body: %w", err)
+	if c.retryMax > 0 {
+		opts := retry.HTTPOptions{
+			RetryMax: c.retryMax,
+		}
+		client = retry.WrapHTTPClient(client, opts)
 	}
 
-	return resp.StatusCode, body, nil
+	return client, nil
 }

internal/retry/http.go

@@ -0,0 +1,137 @@
+// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+// or more contributor license agreements. Licensed under the Elastic License;
+// you may not use this file except in compliance with the Elastic License.
+
+package retry
+
+import (
+	"context"
+	"crypto/tls"
+	"crypto/x509"
+	"errors"
+	"fmt"
+	"log/slog"
+	"net/http"
+	"net/url"
+	"time"
+
+	"github.com/hashicorp/go-retryablehttp"
+)
+
+const (
+	defaultRetryWaitMin = 1 * time.Second
+	defaultRetryWaitMax = 5 * time.Second
+)
+
+type HTTPOptions struct {
+	RetryMax int
+
+	retryWaitMin time.Duration
+	retryWaitMax time.Duration
+}
+
+func WrapHTTPClient(client *http.Client, opts HTTPOptions) *http.Client {
+	if opts.RetryMax <= 0 {
+		return client
+	}
+	retryWaitMin := opts.retryWaitMin
+	if retryWaitMin == 0 {
+		retryWaitMin = defaultRetryWaitMin
+	}
+	retryWaitMax := opts.retryWaitMax
+	if retryWaitMax == 0 {
+		retryWaitMax = defaultRetryWaitMax
+	}
+
+	if client == nil {
+		client = &http.Client{}
+	}
+	if client.CheckRedirect == nil {
+		client.CheckRedirect = checkRedirect
+	}
+	retryClient := retryablehttp.NewClient()
+	retryClient.HTTPClient = client
+	retryClient.CheckRetry = checkRetry
+	retryClient.ErrorHandler = retryablehttp.PassthroughErrorHandler
+	retryClient.RetryMax = opts.RetryMax
+	retryClient.RetryWaitMin = retryWaitMin
+	retryClient.RetryWaitMax = retryWaitMax
+
+	// It needs to be a logger with support for attributes as key-value pairs.
+	retryClient.Logger = slog.Default()
+	return retryClient.StandardClient()
+}
+
+var (
+	maxRedirects        = 10
+	errTooManyRedirects = fmt.Errorf("stopped after %d redirects", maxRedirects)
+)
+
+// checkRedirect reimplements default http redirect policy but returning a typed error.
+func checkRedirect(req *http.Request, via []*http.Request) error {
+	if len(via) >= maxRedirects {
+		return errTooManyRedirects
+	}
+	return nil
+}
+
+// checkRetry reimplements retryablehttp.DefaultRetryPolicy with better error checking.
+func checkRetry(ctx context.Context, resp *http.Response, err error) (bool, error) {
+	if ctx.Err() != nil {
+		return false, ctx.Err()
+	}
+
+	if err != nil {
+		if errors.Is(err, errTooManyRedirects) {
+			// Too many redirects, let's stop here.
+			return false, nil
+		}
+
+		var urlError *url.Error
+		if errors.As(err, &urlError) {
+			// URL is invalid, not recoverable.
+			return false, nil
+		}
+
+		var certVerificationError *tls.CertificateVerificationError
+		if errors.As(err, &certVerificationError) {
+			// Something failed while verifying certificates.
+			return false, nil
+		}
+
+		var certError *x509.CertificateInvalidError
+		if errors.As(err, &certError) {
+			// Invalid certificate, not recoverable.
+			return false, nil
+		}
+
+		var caError x509.UnknownAuthorityError
+		if errors.As(err, &caError) {
+			// Unknown CA, not recoverable.
+			return false, nil
+		}
+
+		// Consider other errors as recoverable and retry.
+		return true, nil
+	}
+
+	// 429 Too Many Requests is recoverable. Sometimes the server puts
+	// a Retry-After response header to indicate when the server is
+	// available to start processing request from client.
+	if resp.StatusCode == http.StatusTooManyRequests {
+		return true, nil
+	}
+
+	// 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 && resp.StatusCode != http.StatusNotImplemented) {
+		// Return the underlying error, that will probably be nil.
+		// retryablehttp.DefaultRetryPolicy did generate an error for these cases,
+		// but this is not what the default HTTP client does.
+		return true, err
+	}
+
+	return false, nil
+}