Merge branch 'id-improve-retryable-tests' into 'main'

Stub retryable http values in tests

See merge request https://gitlab.com/gitlab-org/gitlab-shell/-/merge_requests/708

Merged-by: Ash McKenzie <amckenzie@gitlab.com>
Approved-by: Oscar Tovar <otovar@gitlab.com>
Approved-by: Ash McKenzie <amckenzie@gitlab.com>
Reviewed-by: Oscar Tovar <otovar@gitlab.com>
Co-authored-by: Igor Drozdov <idrozdov@gitlab.com>

3 files changed, 38 insertions, 18 deletions

diff --git a/client/client_test.go b/client/client_test.go
index 9bea4fd..e48bb67 100644
--- a/client/client_test.go
+++ b/client/client_test.go
@@ -20,7 +20,10 @@ import (
 	"gitlab.com/gitlab-org/gitlab-shell/v14/internal/testhelper"
 )
 
-var secret = "sssh, it's a secret"
+var (
+	secret          = "sssh, it's a secret"
+	defaultHttpOpts = []HTTPClientOpt{WithHTTPRetryOpts(time.Millisecond, time.Millisecond, 2)}
+)
 
 func TestClients(t *testing.T) {
 	testhelper.PrepareTestRootDir(t)
@@ -81,7 +84,7 @@ func TestClients(t *testing.T) {
 		t.Run(tc.desc, func(t *testing.T) {
 			url := tc.server(t, buildRequests(t, tc.relativeURLRoot))
 
-			httpClient, err := NewHTTPClientWithOpts(url, tc.relativeURLRoot, tc.caFile, "", 1, nil)
+			httpClient, err := NewHTTPClientWithOpts(url, tc.relativeURLRoot, tc.caFile, "", 1, defaultHttpOpts)
 			require.NoError(t, err)
 
 			client, err := NewGitlabNetClient("", "", tc.secret, httpClient)
@@ -313,7 +316,7 @@ func TestRetryableHTTPFeatureToggle(t *testing.T) {
 		}))
 		defer srv.Close()
 
-		httpClient, err := NewHTTPClientWithOpts(srv.URL, "/", "", "", 1, nil)
+		httpClient, err := NewHTTPClientWithOpts(srv.URL, "/", "", "", 1, defaultHttpOpts)
 		require.NoError(t, err)
 		require.NotNil(t, httpClient.HTTPClient)
 		require.Nil(t, httpClient.RetryableHTTP)
@@ -334,7 +337,7 @@ func TestRetryableHTTPFeatureToggle(t *testing.T) {
 		}))
 		defer srv.Close()
 
-		httpClient, err := NewHTTPClientWithOpts(srv.URL, "/", "", "", 1, nil)
+		httpClient, err := NewHTTPClientWithOpts(srv.URL, "/", "", "", 1, defaultHttpOpts)
 		require.NoError(t, err)
 		require.Nil(t, httpClient.HTTPClient)
 		require.NotNil(t, httpClient.RetryableHTTP)
diff --git a/client/httpclient.go b/client/httpclient.go
index fe6e2e5..82b4b40 100644
--- a/client/httpclient.go
+++ b/client/httpclient.go
@@ -24,6 +24,9 @@ const (
 	httpProtocol              = "http://"
 	httpsProtocol             = "https://"
 	defaultReadTimeoutSeconds = 300
+	defaultRetryWaitMin       = time.Second
+	defaultRetryWaitMax       = 15 * time.Second
+	defaultRetryMax           = 2
 )
 
 var ErrCafileNotFound = errors.New("cafile not found")
@@ -35,8 +38,10 @@ type HttpClient struct {
 }
 
 type httpClientCfg struct {
-	keyPath, certPath string
-	caFile, caPath    string
+	keyPath, certPath          string
+	caFile, caPath             string
+	retryWaitMin, retryWaitMax time.Duration
+	retryMax                   int
 }
 
 func (hcc httpClientCfg) HaveCertAndKey() bool { return hcc.keyPath != "" && hcc.certPath != "" }
@@ -53,6 +58,14 @@ func WithClientCert(certPath, keyPath string) HTTPClientOpt {
 	}
 }
 
+func WithHTTPRetryOpts(waitMin, waitMax time.Duration, maxAttempts int) HTTPClientOpt {
+	return func(hcc *httpClientCfg) {
+		hcc.retryWaitMin = waitMin
+		hcc.retryWaitMax = waitMax
+		hcc.retryMax = maxAttempts
+	}
+}
+
 func validateCaFile(filename string) error {
 	if filename == "" {
 		return nil
@@ -71,6 +84,18 @@ func validateCaFile(filename string) error {
 
 // NewHTTPClientWithOpts builds an HTTP client using the provided options
 func NewHTTPClientWithOpts(gitlabURL, gitlabRelativeURLRoot, caFile, caPath string, readTimeoutSeconds uint64, opts []HTTPClientOpt) (*HttpClient, error) {
+	hcc := &httpClientCfg{
+		caFile:       caFile,
+		caPath:       caPath,
+		retryWaitMin: defaultRetryWaitMin,
+		retryWaitMax: defaultRetryWaitMax,
+		retryMax:     defaultRetryMax,
+	}
+
+	for _, opt := range opts {
+		opt(hcc)
+	}
+
 	var transport *http.Transport
 	var host string
 	var err error
@@ -84,15 +109,6 @@ func NewHTTPClientWithOpts(gitlabURL, gitlabRelativeURLRoot, caFile, caPath stri
 			return nil, err
 		}
 
-		hcc := &httpClientCfg{
-			caFile: caFile,
-			caPath: caPath,
-		}
-
-		for _, opt := range opts {
-			opt(hcc)
-		}
-
 		transport, host, err = buildHttpsTransport(*hcc, gitlabURL)
 		if err != nil {
 			return nil, err
@@ -110,8 +126,9 @@ func NewHTTPClientWithOpts(gitlabURL, gitlabRelativeURLRoot, caFile, caPath stri
 
 	if os.Getenv("FF_GITLAB_SHELL_RETRYABLE_HTTP") == "1" {
 		c := retryablehttp.NewClient()
-		c.RetryMax = 2
-		c.RetryWaitMax = 15 * time.Second
+		c.RetryMax = hcc.retryMax
+		c.RetryWaitMax = hcc.retryWaitMax
+		c.RetryWaitMin = hcc.retryWaitMin
 		c.Logger = nil
 		c.HTTPClient.Transport = correlation.NewInstrumentedRoundTripper(tracing.NewRoundTripper(transport))
 		c.HTTPClient.Timeout = readTimeout(readTimeoutSeconds)
diff --git a/client/httpsclient_test.go b/client/httpsclient_test.go
index d220171..49152de 100644
--- a/client/httpsclient_test.go
+++ b/client/httpsclient_test.go
@@ -123,7 +123,7 @@ func setupWithRequests(t *testing.T, caFile, caPath, clientCAPath, clientCertPat
 
 	url := testserver.StartHttpsServer(t, requests, clientCAPath)
 
-	var opts []HTTPClientOpt
+	opts := defaultHttpOpts
 	if clientCertPath != "" && clientKeyPath != "" {
 		opts = append(opts, WithClientCert(clientCertPath, clientKeyPath))
 	}