Skip to content

Commit a2232e3

Browse files
ashmckenzieSteve Azzopardi
ashmckenzie
and
Steve Azzopardi
committed
Merge branch 'feat/remove-unretryable-http' into 'main'
feat: make retryable http default client See merge request https://gitlab.com/gitlab-org/gitlab-shell/-/merge_requests/710 Merged-by: Ash McKenzie <[email protected]> Approved-by: Oscar Tovar <[email protected]> Approved-by: Ash McKenzie <[email protected]> Co-authored-by: Steve Azzopardi <[email protected]>
2 parents 51eab44 + 80f684e commit a2232e3

File tree

15 files changed

+90
-210
lines changed

15 files changed

+90
-210
lines changed

‎.gitlab-ci.yml

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -64,13 +64,6 @@ tests:
6464
- make coverage
6565
coverage: '/\d+.\d+%/'
6666

67-
tests-integration-retryableHttp:
68-
extends: .test
69-
variables:
70-
FF_GITLAB_SHELL_RETRYABLE_HTTP: '1'
71-
script:
72-
- make test_ruby
73-
7467
race:
7568
extends: .test
7669
script:

‎client/client_test.go

Lines changed: 17 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"io"
88
"net/http"
99
"net/http/httptest"
10-
"os"
1110
"path"
1211
"strings"
1312
"testing"
@@ -306,46 +305,21 @@ func buildRequests(t *testing.T, relativeURLRoot string) []testserver.TestReques
306305
return requests
307306
}
308307

309-
func TestRetryableHTTPFeatureToggle(t *testing.T) {
310-
t.Run("retryable http off", func(t *testing.T) {
311-
os.Setenv("FF_GITLAB_SHELL_RETRYABLE_HTTP", "0")
312-
reqAttempts := 0
313-
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
314-
reqAttempts++
315-
w.WriteHeader(500)
316-
}))
317-
defer srv.Close()
318-
319-
httpClient, err := NewHTTPClientWithOpts(srv.URL, "/", "", "", 1, defaultHttpOpts)
320-
require.NoError(t, err)
321-
require.NotNil(t, httpClient.HTTPClient)
322-
require.Nil(t, httpClient.RetryableHTTP)
323-
client, err := NewGitlabNetClient("", "", "", httpClient)
324-
require.NoError(t, err)
325-
326-
_, err = client.Get(context.Background(), "/")
327-
require.EqualError(t, err, "Internal API error (500)")
328-
require.Equal(t, 1, reqAttempts)
329-
})
330-
331-
t.Run("retryable http on", func(t *testing.T) {
332-
os.Setenv("FF_GITLAB_SHELL_RETRYABLE_HTTP", "1")
333-
reqAttempts := 0
334-
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
335-
reqAttempts++
336-
w.WriteHeader(500)
337-
}))
338-
defer srv.Close()
339-
340-
httpClient, err := NewHTTPClientWithOpts(srv.URL, "/", "", "", 1, defaultHttpOpts)
341-
require.NoError(t, err)
342-
require.Nil(t, httpClient.HTTPClient)
343-
require.NotNil(t, httpClient.RetryableHTTP)
344-
client, err := NewGitlabNetClient("", "", "", httpClient)
345-
require.NoError(t, err)
346-
347-
_, err = client.Get(context.Background(), "/")
348-
require.EqualError(t, err, "Internal API unreachable")
349-
require.Equal(t, 3, reqAttempts)
350-
})
308+
func TestRetryOnFailure(t *testing.T) {
309+
reqAttempts := 0
310+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
311+
reqAttempts++
312+
w.WriteHeader(500)
313+
}))
314+
defer srv.Close()
315+
316+
httpClient, err := NewHTTPClientWithOpts(srv.URL, "/", "", "", 1, defaultHttpOpts)
317+
require.NoError(t, err)
318+
require.NotNil(t, httpClient.RetryableHTTP)
319+
client, err := NewGitlabNetClient("", "", "", httpClient)
320+
require.NoError(t, err)
321+
322+
_, err = client.Get(context.Background(), "/")
323+
require.EqualError(t, err, "Internal API unreachable")
324+
require.Equal(t, 3, reqAttempts)
351325
}

‎client/gitlabnet.go

Lines changed: 4 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"fmt"
88
"io"
99
"net/http"
10-
"os"
1110
"strings"
1211
"time"
1312

@@ -89,26 +88,7 @@ func appendPath(host string, path string) string {
8988
return strings.TrimSuffix(host, "/") + "/" + strings.TrimPrefix(path, "/")
9089
}
9190

92-
func newRequest(ctx context.Context, method, host, path string, data interface{}) (*http.Request, error) {
93-
var jsonReader io.Reader
94-
if data != nil {
95-
jsonData, err := json.Marshal(data)
96-
if err != nil {
97-
return nil, err
98-
}
99-
100-
jsonReader = bytes.NewReader(jsonData)
101-
}
102-
103-
request, err := http.NewRequestWithContext(ctx, method, appendPath(host, path), jsonReader)
104-
if err != nil {
105-
return nil, err
106-
}
107-
108-
return request, nil
109-
}
110-
111-
func newRetryableRequest(ctx context.Context, method, host, path string, data interface{}) (*retryablehttp.Request, error) {
91+
func newRequest(ctx context.Context, method, host, path string, data interface{}) (*retryablehttp.Request, error) {
11292
var jsonReader io.Reader
11393
if data != nil {
11494
jsonData, err := json.Marshal(data)
@@ -139,7 +119,6 @@ func parseError(resp *http.Response) error {
139119
} else {
140120
return &ApiError{parsedResponse.Message}
141121
}
142-
143122
}
144123

145124
func (c *GitlabNetClient) Get(ctx context.Context, path string) (*http.Response, error) {
@@ -156,15 +135,9 @@ func (c *GitlabNetClient) DoRequest(ctx context.Context, method, path string, da
156135
return nil, err
157136
}
158137

159-
retryableRequest, err := newRetryableRequest(ctx, method, c.httpClient.Host, path, data)
160-
if err != nil {
161-
return nil, err
162-
}
163-
164138
user, password := c.user, c.password
165139
if user != "" && password != "" {
166140
request.SetBasicAuth(user, password)
167-
retryableRequest.SetBasicAuth(user, password)
168141
}
169142

170143
claims := jwt.RegisteredClaims{
@@ -178,40 +151,28 @@ func (c *GitlabNetClient) DoRequest(ctx context.Context, method, path string, da
178151
return nil, err
179152
}
180153
request.Header.Set(apiSecretHeaderName, tokenString)
181-
retryableRequest.Header.Set(apiSecretHeaderName, tokenString)
182154

183155
originalRemoteIP, ok := ctx.Value(OriginalRemoteIPContextKey{}).(string)
184156
if ok {
185157
request.Header.Add("X-Forwarded-For", originalRemoteIP)
186-
retryableRequest.Header.Add("X-Forwarded-For", originalRemoteIP)
187158
}
188159

189160
request.Header.Add("Content-Type", "application/json")
190-
retryableRequest.Header.Add("Content-Type", "application/json")
191161
request.Header.Add("User-Agent", c.userAgent)
192-
retryableRequest.Header.Add("User-Agent", c.userAgent)
193162
request.Close = true
194-
retryableRequest.Close = true
195163

196164
start := time.Now()
197165

198-
var response *http.Response
199-
var respErr error
200-
if c.httpClient.HTTPClient != nil {
201-
response, respErr = c.httpClient.HTTPClient.Do(request)
202-
}
203-
if os.Getenv("FF_GITLAB_SHELL_RETRYABLE_HTTP") == "1" && c.httpClient.RetryableHTTP != nil {
204-
response, respErr = c.httpClient.RetryableHTTP.Do(retryableRequest)
205-
}
166+
response, err := c.httpClient.RetryableHTTP.Do(request)
206167
fields := log.Fields{
207168
"method": method,
208169
"url": request.URL.String(),
209170
"duration_ms": time.Since(start) / time.Millisecond,
210171
}
211172
logger := log.WithContextFields(ctx, fields)
212173

213-
if respErr != nil {
214-
logger.WithError(respErr).Error("Internal API unreachable")
174+
if err != nil {
175+
logger.WithError(err).Error("Internal API unreachable")
215176
return nil, &ApiError{"Internal API unreachable"}
216177
}
217178

‎client/httpclient.go

Lines changed: 8 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ const (
3232
var ErrCafileNotFound = errors.New("cafile not found")
3333

3434
type HttpClient struct {
35-
HTTPClient *http.Client
3635
RetryableHTTP *retryablehttp.Client
3736
Host string
3837
}
@@ -117,28 +116,15 @@ func NewHTTPClientWithOpts(gitlabURL, gitlabRelativeURLRoot, caFile, caPath stri
117116
return nil, errors.New("unknown GitLab URL prefix")
118117
}
119118

120-
c := &http.Client{
121-
Transport: correlation.NewInstrumentedRoundTripper(tracing.NewRoundTripper(transport)),
122-
Timeout: readTimeout(readTimeoutSeconds),
123-
}
124-
125-
client := &HttpClient{HTTPClient: c, Host: host}
126-
127-
if os.Getenv("FF_GITLAB_SHELL_RETRYABLE_HTTP") == "1" {
128-
c := retryablehttp.NewClient()
129-
c.RetryMax = hcc.retryMax
130-
c.RetryWaitMax = hcc.retryWaitMax
131-
c.RetryWaitMin = hcc.retryWaitMin
132-
c.Logger = nil
133-
c.HTTPClient.Transport = correlation.NewInstrumentedRoundTripper(tracing.NewRoundTripper(transport))
134-
c.HTTPClient.Timeout = readTimeout(readTimeoutSeconds)
119+
c := retryablehttp.NewClient()
120+
c.RetryMax = hcc.retryMax
121+
c.RetryWaitMax = hcc.retryWaitMax
122+
c.RetryWaitMin = hcc.retryWaitMin
123+
c.Logger = nil
124+
c.HTTPClient.Transport = correlation.NewInstrumentedRoundTripper(tracing.NewRoundTripper(transport))
125+
c.HTTPClient.Timeout = readTimeout(readTimeoutSeconds)
135126

136-
client = &HttpClient{RetryableHTTP: c, Host: host}
137-
}
138-
139-
if client.HTTPClient == nil && client.RetryableHTTP == nil {
140-
panic("client/httpclient.go did not set http client")
141-
}
127+
client := &HttpClient{RetryableHTTP: c, Host: host}
142128

143129
return client, nil
144130
}

‎client/httpclient_test.go

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,7 @@ func TestReadTimeout(t *testing.T) {
2121
require.NoError(t, err)
2222

2323
require.NotNil(t, client)
24-
if client.HTTPClient != nil {
25-
require.Equal(t, time.Duration(expectedSeconds)*time.Second, client.HTTPClient.Timeout)
26-
}
27-
28-
if client.RetryableHTTP != nil {
29-
require.Equal(t, time.Duration(expectedSeconds)*time.Second, client.RetryableHTTP.HTTPClient.Timeout)
30-
}
24+
require.Equal(t, time.Duration(expectedSeconds)*time.Second, client.RetryableHTTP.HTTPClient.Timeout)
3125
}
3226

3327
const (

‎client/testserver/testserver.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,6 @@ func StartHttpServer(t *testing.T, handlers []TestRequestHandler) string {
6060
}
6161

6262
func StartRetryHttpServer(t *testing.T, handlers []TestRequestHandler) string {
63-
os.Setenv("FF_GITLAB_SHELL_RETRYABLE_HTTP", "1")
6463
attempts := map[string]int{}
6564

6665
retryMiddileware := func(next func(w http.ResponseWriter, r *http.Request)) http.Handler {

‎internal/command/discover/discover_test.go

Lines changed: 24 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -16,33 +16,31 @@ import (
1616
"gitlab.com/gitlab-org/gitlab-shell/v14/internal/config"
1717
)
1818

19-
var (
20-
requests = []testserver.TestRequestHandler{
21-
{
22-
Path: "/api/v4/internal/discover",
23-
Handler: func(w http.ResponseWriter, r *http.Request) {
24-
if r.URL.Query().Get("key_id") == "1" || r.URL.Query().Get("username") == "alex-doe" {
25-
body := map[string]interface{}{
26-
"id": 2,
27-
"username": "alex-doe",
28-
"name": "Alex Doe",
29-
}
30-
json.NewEncoder(w).Encode(body)
31-
} else if r.URL.Query().Get("username") == "broken_message" {
32-
body := map[string]string{
33-
"message": "Forbidden!",
34-
}
35-
w.WriteHeader(http.StatusForbidden)
36-
json.NewEncoder(w).Encode(body)
37-
} else if r.URL.Query().Get("username") == "broken" {
38-
w.WriteHeader(http.StatusInternalServerError)
39-
} else {
40-
fmt.Fprint(w, "null")
19+
var requests = []testserver.TestRequestHandler{
20+
{
21+
Path: "/api/v4/internal/discover",
22+
Handler: func(w http.ResponseWriter, r *http.Request) {
23+
if r.URL.Query().Get("key_id") == "1" || r.URL.Query().Get("username") == "alex-doe" {
24+
body := map[string]interface{}{
25+
"id": 2,
26+
"username": "alex-doe",
27+
"name": "Alex Doe",
28+
}
29+
json.NewEncoder(w).Encode(body)
30+
} else if r.URL.Query().Get("username") == "broken_message" {
31+
body := map[string]string{
32+
"message": "Forbidden!",
4133
}
42-
},
34+
w.WriteHeader(http.StatusForbidden)
35+
json.NewEncoder(w).Encode(body)
36+
} else if r.URL.Query().Get("username") == "broken" {
37+
w.WriteHeader(http.StatusInternalServerError)
38+
} else {
39+
fmt.Fprint(w, "null")
40+
}
4341
},
44-
}
45-
)
42+
},
43+
}
4644

4745
func TestExecute(t *testing.T) {
4846
url := testserver.StartSocketHttpServer(t, requests)
@@ -112,7 +110,7 @@ func TestFailingExecute(t *testing.T) {
112110
{
113111
desc: "When the API fails",
114112
arguments: &commandargs.Shell{GitlabUsername: "broken"},
115-
expectedError: "Failed to get username: Internal API error (500)",
113+
expectedError: "Failed to get username: Internal API unreachable",
116114
},
117115
}
118116

‎internal/command/healthcheck/healthcheck_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,5 +84,5 @@ func TestFailingAPIExecute(t *testing.T) {
8484

8585
err := cmd.Execute(context.Background())
8686
require.Empty(t, buffer.String())
87-
require.EqualError(t, err, "Internal API available: FAILED - Internal API error (500)")
87+
require.EqualError(t, err, "Internal API available: FAILED - Internal API unreachable")
8888
}

‎internal/command/personalaccesstoken/personalaccesstoken_test.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,7 @@ import (
1717
"gitlab.com/gitlab-org/gitlab-shell/v14/internal/gitlabnet/personalaccesstoken"
1818
)
1919

20-
var (
21-
requests []testserver.TestRequestHandler
22-
)
20+
var requests []testserver.TestRequestHandler
2321

2422
func setup(t *testing.T) {
2523
requests = []testserver.TestRequestHandler{
@@ -147,7 +145,7 @@ func TestExecute(t *testing.T) {
147145
GitlabKeyId: "broken",
148146
SshArgs: []string{cmdname, "newtoken", "read_api,read_repository"},
149147
},
150-
expectedError: "Internal API error (500)",
148+
expectedError: "Internal API unreachable",
151149
},
152150
{
153151
desc: "Without KeyID or User",

‎internal/command/twofactorrecover/twofactorrecover_test.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,7 @@ import (
1818
"gitlab.com/gitlab-org/gitlab-shell/v14/internal/gitlabnet/twofactorrecover"
1919
)
2020

21-
var (
22-
requests []testserver.TestRequestHandler
23-
)
21+
var requests []testserver.TestRequestHandler
2422

2523
func setup(t *testing.T) {
2624
requests = []testserver.TestRequestHandler{
@@ -99,7 +97,7 @@ func TestExecute(t *testing.T) {
9997
desc: "With API fails",
10098
arguments: &commandargs.Shell{GitlabKeyId: "broken"},
10199
answer: "yes\n",
102-
expectedOutput: question + errorHeader + "Internal API error (500)\n",
100+
expectedOutput: question + errorHeader + "Internal API unreachable\n",
103101
},
104102
{
105103
desc: "With missing arguments",

0 commit comments

Comments
 (0)