summaryrefslogtreecommitdiff
path: root/internal
diff options
context:
space:
mode:
authorSteve Azzopardi <sazzopardi@gitlab.com>2023-01-02 15:50:27 +0100
committerAsh McKenzie <amckenzie@gitlab.com>2023-01-12 02:56:43 +0000
commita093c9d3cfc1ee18368ebbf828dc61c15b74540c (patch)
tree5e10805b8ecd9d582ef27766f5374e8150a16aeb /internal
parentc5b3accf0a8b2008137a90d40cfd65ba3ef2c99a (diff)
downloadgitlab-shell-a093c9d3cfc1ee18368ebbf828dc61c15b74540c.tar.gz
feat: retry on error
What --- Change the default `HTTP.Client` to `github.com/hashicorp/go-retryablehttp.Client` to get automatic retries and exponential backoff. We retry the request 2 times resulting in 3 attempts of sending the request, the min retry wait is 1 second, and the maximum is 15 seconds. Hide the retry logic behind a temporary feature flag `FF_GITLAB_SHELL_RETRYABLE_HTTP` to easily roll this out in GitLab.com. When we verify that this works as expected we will remove `FF_GITLAB_SHELL_RETRYABLE_HTTP` and have the retry logic as the default logic. Why --- In https://gitlab.com/gitlab-com/gl-infra/production/-/issues/7979 users end up seeing the following errors when trying to `git-clone(1)` a repository locally on in CI. ```shell remote: =============================== remote: remote: ERROR: Internal API unreachable remote: remote: ================================ ``` When we look at the application logs we see the following error: ```json { "err": "http://gitlab-webservice-git.gitlab.svc:8181/api/v4/internal/allowed": dial tcp 10.69.184.120:8181: connect: connection refused", "msg": "Internal API unreachable"} ``` In https://gitlab.com/gitlab-com/gl-infra/production/-/issues/7979#note_1222670120 we've correlated these `connection refused` errors with infrastructure events that remove the git pods that are hosting `gitlab-webservice-git` service. We could try to make the underlying infrastructure more reactive to these changes as suggested in https://gitlab.com/gitlab-com/gl-infra/production/-/issues/7979#note_1225164944 but we can still end up serving bad requests. Implementing retry logic for 5xx or other errors would allow users to still be able to `git-clone(1)` reposirories, although it being slower. This is espically important during CI runs so users don't have to retry jobs themselves. Reference: https://gitlab.com/gitlab-com/gl-infra/production/-/issues/7979 Closes: https://gitlab.com/gitlab-org/gitlab-shell/-/issues/604 Signed-off-by: Steve Azzopardi <sazzopardi@gitlab.com>
Diffstat (limited to 'internal')
-rw-r--r--internal/command/discover/discover_test.go50
-rw-r--r--internal/command/healthcheck/healthcheck_test.go2
-rw-r--r--internal/command/personalaccesstoken/personalaccesstoken_test.go6
-rw-r--r--internal/command/twofactorrecover/twofactorrecover_test.go6
-rw-r--r--internal/command/twofactorverify/twofactorverify_test.go2
-rw-r--r--internal/config/config.go4
-rw-r--r--internal/gitlabnet/lfsauthenticate/client_test.go2
7 files changed, 33 insertions, 39 deletions
diff --git a/internal/command/discover/discover_test.go b/internal/command/discover/discover_test.go
index ccbfa7e..df9ca47 100644
--- a/internal/command/discover/discover_test.go
+++ b/internal/command/discover/discover_test.go
@@ -16,33 +16,31 @@ import (
"gitlab.com/gitlab-org/gitlab-shell/v14/internal/config"
)
-var (
- requests = []testserver.TestRequestHandler{
- {
- Path: "/api/v4/internal/discover",
- Handler: func(w http.ResponseWriter, r *http.Request) {
- if r.URL.Query().Get("key_id") == "1" || r.URL.Query().Get("username") == "alex-doe" {
- body := map[string]interface{}{
- "id": 2,
- "username": "alex-doe",
- "name": "Alex Doe",
- }
- json.NewEncoder(w).Encode(body)
- } else if r.URL.Query().Get("username") == "broken_message" {
- body := map[string]string{
- "message": "Forbidden!",
- }
- w.WriteHeader(http.StatusForbidden)
- json.NewEncoder(w).Encode(body)
- } else if r.URL.Query().Get("username") == "broken" {
- w.WriteHeader(http.StatusInternalServerError)
- } else {
- fmt.Fprint(w, "null")
+var requests = []testserver.TestRequestHandler{
+ {
+ Path: "/api/v4/internal/discover",
+ Handler: func(w http.ResponseWriter, r *http.Request) {
+ if r.URL.Query().Get("key_id") == "1" || r.URL.Query().Get("username") == "alex-doe" {
+ body := map[string]interface{}{
+ "id": 2,
+ "username": "alex-doe",
+ "name": "Alex Doe",
+ }
+ json.NewEncoder(w).Encode(body)
+ } else if r.URL.Query().Get("username") == "broken_message" {
+ body := map[string]string{
+ "message": "Forbidden!",
}
- },
+ w.WriteHeader(http.StatusForbidden)
+ json.NewEncoder(w).Encode(body)
+ } else if r.URL.Query().Get("username") == "broken" {
+ w.WriteHeader(http.StatusInternalServerError)
+ } else {
+ fmt.Fprint(w, "null")
+ }
},
- }
-)
+ },
+}
func TestExecute(t *testing.T) {
url := testserver.StartSocketHttpServer(t, requests)
@@ -112,7 +110,7 @@ func TestFailingExecute(t *testing.T) {
{
desc: "When the API fails",
arguments: &commandargs.Shell{GitlabUsername: "broken"},
- expectedError: "Failed to get username: Internal API error (500)",
+ expectedError: "Failed to get username: Internal API unreachable",
},
}
diff --git a/internal/command/healthcheck/healthcheck_test.go b/internal/command/healthcheck/healthcheck_test.go
index f642226..12a8444 100644
--- a/internal/command/healthcheck/healthcheck_test.go
+++ b/internal/command/healthcheck/healthcheck_test.go
@@ -84,5 +84,5 @@ func TestFailingAPIExecute(t *testing.T) {
err := cmd.Execute(context.Background())
require.Empty(t, buffer.String())
- require.EqualError(t, err, "Internal API available: FAILED - Internal API error (500)")
+ require.EqualError(t, err, "Internal API available: FAILED - Internal API unreachable")
}
diff --git a/internal/command/personalaccesstoken/personalaccesstoken_test.go b/internal/command/personalaccesstoken/personalaccesstoken_test.go
index daa5834..492f745 100644
--- a/internal/command/personalaccesstoken/personalaccesstoken_test.go
+++ b/internal/command/personalaccesstoken/personalaccesstoken_test.go
@@ -17,9 +17,7 @@ import (
"gitlab.com/gitlab-org/gitlab-shell/v14/internal/gitlabnet/personalaccesstoken"
)
-var (
- requests []testserver.TestRequestHandler
-)
+var requests []testserver.TestRequestHandler
func setup(t *testing.T) {
requests = []testserver.TestRequestHandler{
@@ -147,7 +145,7 @@ func TestExecute(t *testing.T) {
GitlabKeyId: "broken",
SshArgs: []string{cmdname, "newtoken", "read_api,read_repository"},
},
- expectedError: "Internal API error (500)",
+ expectedError: "Internal API unreachable",
},
{
desc: "Without KeyID or User",
diff --git a/internal/command/twofactorrecover/twofactorrecover_test.go b/internal/command/twofactorrecover/twofactorrecover_test.go
index 81b4dff..7e20a06 100644
--- a/internal/command/twofactorrecover/twofactorrecover_test.go
+++ b/internal/command/twofactorrecover/twofactorrecover_test.go
@@ -18,9 +18,7 @@ import (
"gitlab.com/gitlab-org/gitlab-shell/v14/internal/gitlabnet/twofactorrecover"
)
-var (
- requests []testserver.TestRequestHandler
-)
+var requests []testserver.TestRequestHandler
func setup(t *testing.T) {
requests = []testserver.TestRequestHandler{
@@ -99,7 +97,7 @@ func TestExecute(t *testing.T) {
desc: "With API fails",
arguments: &commandargs.Shell{GitlabKeyId: "broken"},
answer: "yes\n",
- expectedOutput: question + errorHeader + "Internal API error (500)\n",
+ expectedOutput: question + errorHeader + "Internal API unreachable\n",
},
{
desc: "With missing arguments",
diff --git a/internal/command/twofactorverify/twofactorverify_test.go b/internal/command/twofactorverify/twofactorverify_test.go
index 640d3ed..213c025 100644
--- a/internal/command/twofactorverify/twofactorverify_test.go
+++ b/internal/command/twofactorverify/twofactorverify_test.go
@@ -136,7 +136,7 @@ func TestExecute(t *testing.T) {
{
desc: "With API fails",
arguments: &commandargs.Shell{GitlabKeyId: "broken"},
- expectedOutput: errorHeader + "Internal API error (500)\n",
+ expectedOutput: errorHeader + "Internal API unreachable\n",
},
{
desc: "With missing arguments",
diff --git a/internal/config/config.go b/internal/config/config.go
index 35d8e74..7f473c5 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -134,8 +134,8 @@ func (c *Config) HttpClient() (*client.HttpClient, error) {
return
}
- tr := client.Transport
- client.Transport = metrics.NewRoundTripper(tr)
+ tr := client.HTTPClient.Transport
+ client.HTTPClient.Transport = metrics.NewRoundTripper(tr)
c.httpClient = client
})
diff --git a/internal/gitlabnet/lfsauthenticate/client_test.go b/internal/gitlabnet/lfsauthenticate/client_test.go
index f62d8e1..01b3f84 100644
--- a/internal/gitlabnet/lfsauthenticate/client_test.go
+++ b/internal/gitlabnet/lfsauthenticate/client_test.go
@@ -74,7 +74,7 @@ func TestFailedRequests(t *testing.T) {
{
desc: "With API fails",
args: &commandargs.Shell{GitlabKeyId: "broken", CommandType: commandargs.LfsAuthenticate, SshArgs: []string{"git-lfs-authenticate", repo, "download"}},
- expectedOutput: "Internal API error (500)",
+ expectedOutput: "Internal API unreachable",
},
}